From cf3a545c474d437bdc0fdecc2dd1c48d35f3f18b Mon Sep 17 00:00:00 2001 From: schwarze Date: Tue, 22 Jul 2014 18:14:05 +0000 Subject: [PATCH] Security fix to prevent XSS attacks: Restrict the character set of strings passed into html_alloc(), in particular architecture names that come from the QUERY_STRING, but also SCRIPT_NAME and manpath.conf content for additional safety, and bail out safely on violations. Issue reported by Sebastien Marie . --- usr.bin/mandoc/cgi.c | 44 ++++++++++++++++++++++++++++++++++++++-- usr.bin/mandoc/man.cgi.8 | 42 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 82 insertions(+), 4 deletions(-) diff --git a/usr.bin/mandoc/cgi.c b/usr.bin/mandoc/cgi.c index dbe73e591e8..970c256972d 100644 --- a/usr.bin/mandoc/cgi.c +++ b/usr.bin/mandoc/cgi.c @@ -1,4 +1,4 @@ -/* $Id: cgi.c,v 1.19 2014/07/21 22:32:55 schwarze Exp $ */ +/* $Id: cgi.c,v 1.20 2014/07/22 18:14:05 schwarze Exp $ */ /* * Copyright (c) 2011, 2012 Kristaps Dzonsons * Copyright (c) 2014 Ingo Schwarze @@ -462,6 +462,20 @@ resp_searchform(const struct req *req) puts(""); } +static int +validate_urifrag(const char *frag) +{ + + while ('\0' != *frag) { + if ( ! (isalnum((unsigned char)*frag) || + '-' == *frag || '.' == *frag || + '/' == *frag || '_' == *frag)) + return(0); + frag++; + } + return(1); +} + static int validate_manpath(const struct req *req, const char* manpath) { @@ -956,6 +970,13 @@ main(void) if (NULL == (scriptname = getenv("SCRIPT_NAME"))) scriptname = ""; + if ( ! validate_urifrag(scriptname)) { + fprintf(stderr, "unsafe SCRIPT_NAME \"%s\"\n", + scriptname); + pg_error_internal(); + return(EXIT_FAILURE); + } + /* * First we change directory into the MAN_DIR so that * subsequent scanning for manpath directories is rooted @@ -983,6 +1004,12 @@ main(void) return(EXIT_FAILURE); } + if ( ! (NULL == req.q.arch || validate_urifrag(req.q.arch))) { + pg_error_badrequest( + "You specified an invalid architecture."); + return(EXIT_FAILURE); + } + /* Dispatch to the three different pages. */ path = getenv("PATH_INFO"); @@ -1034,7 +1061,20 @@ pathgen(struct req *req) dpsz--; req->p = mandoc_realloc(req->p, (req->psz + 1) * sizeof(char *)); - req->p[req->psz++] = mandoc_strndup(dp, dpsz); + dp = mandoc_strndup(dp, dpsz); + if ( ! validate_urifrag(dp)) { + fprintf(stderr, "%s/manpath.conf contains " + "unsafe path \"%s\"\n", MAN_DIR, dp); + pg_error_internal(); + exit(EXIT_FAILURE); + } + if (NULL != strchr(dp, '/')) { + fprintf(stderr, "%s/manpath.conf contains " + "path with slash \"%s\"\n", MAN_DIR, dp); + pg_error_internal(); + exit(EXIT_FAILURE); + } + req->p[req->psz++] = dp; } if ( req->p == NULL ) { diff --git a/usr.bin/mandoc/man.cgi.8 b/usr.bin/mandoc/man.cgi.8 index ee860d6540e..b6260874ebf 100644 --- a/usr.bin/mandoc/man.cgi.8 +++ b/usr.bin/mandoc/man.cgi.8 @@ -1,4 +1,4 @@ -.\" $Id: man.cgi.8,v 1.6 2014/07/21 15:44:22 schwarze Exp $ +.\" $Id: man.cgi.8,v 1.7 2014/07/22 18:14:05 schwarze Exp $ .\" .\" Copyright (c) 2014 Ingo Schwarze .\" @@ -14,7 +14,7 @@ .\" ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF .\" OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. .\" -.Dd $Mdocdate: July 21 2014 $ +.Dd $Mdocdate: July 22 2014 $ .Dt MAN.CGI 8 .Os .Sh NAME @@ -267,6 +267,34 @@ For backward compatibility with the traditional is supported as an alias for .Cm sec . .El +.Ss Restricted character set +For security reasons, in particular to prevent cross site scripting +attacks, some strings used by +.Nm +can only contain the following characters: +.Pp +.Bl -dash -compact -offset indent +.It +lower case and upper case ASCII letters +.It +the ten decimal digits +.It +the dash +.Pq Sq - +.It +the dot +.Pq Sq \&. +.It +the slash +.Pq Sq / +.It +the underscore +.Pq Sq _ +.El +.Pp +In particular, this applies to the +.Ev SCRIPT_NAME , +to all manpaths, and to all architecture names. .Sh ENVIRONMENT The web server may pass the following CGI variables to .Nm : @@ -293,6 +321,10 @@ binary relative to the server root, usually .Pa /cgi-bin/man.cgi . This is used for generating URIs to be embedded in generated HTML code and HTTP headers. +If this contains any character not contained in the +.Sx Restricted character set , +.Nm +reports an internal server error and exits without doing anything. .El .Sh FILES .Bl -tag -width Ds @@ -332,6 +364,12 @@ Manual pages documenting itself, linked from the index page. .It Pa /man/manpath.conf The list of available manpaths, one per line. +If any of the lines in this file contains a slash +.Pq Sq / +or any character not contained in the +.Sx Restricted character set , +.Nm +reports an internal server error and exits without doing anything. .It Pa /man/OpenBSD-current/man1/mandoc.1 An example .Xr mdoc 7 -- 2.20.1