From 65cb0941933336f1e6502825398c255d1c4d858e Mon Sep 17 00:00:00 2001 From: Ingo Schwarze Date: Tue, 22 Jul 2014 18:14:13 +0000 Subject: 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 . --- cgi.c | 42 +++++++++++++++++++++++++++++++++++++++++- man.cgi.8 | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 1 deletion(-) diff --git a/cgi.c b/cgi.c index 66af1151..28efb73f 100644 --- a/cgi.c +++ b/cgi.c @@ -466,6 +466,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) { @@ -960,6 +974,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 @@ -987,6 +1008,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"); @@ -1038,7 +1065,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/man.cgi.8 b/man.cgi.8 index 169b2ae2..1f001b01 100644 --- a/man.cgi.8 +++ b/man.cgi.8 @@ -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 -- cgit