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 <semarie-openbsd at latrappe dot fr>.

diff --git a/cgi.c b/cgi.c
index 9445845bc1e77ced9670e6829571e2a26ad33999..0b3ac81405b101beb7f59b875f819f6651b69645 100644 (file)
--- a/cgi.c
+++ b/cgi.c
@@ -1,4 +1,4 @@
-/*     $Id: cgi.c,v 1.79 2014/07/21 22:33:01 schwarze Exp $ */
+/*     $Id: cgi.c,v 1.80 2014/07/22 18:14:13 schwarze Exp $ */

 /*
  * Copyright (c) 2011, 2012 Kristaps Dzonsons <kristaps@bsd.lv>
  * Copyright (c) 2014 Ingo Schwarze <schwarze@usta.de>
 /*
  * Copyright (c) 2011, 2012 Kristaps Dzonsons <kristaps@bsd.lv>
  * Copyright (c) 2014 Ingo Schwarze <schwarze@usta.de>


@@ -466,6 +466,20 @@ resp_searchform(const struct req *req)
        puts("<!-- End search form. //-->");
 }
 
        puts("<!-- End search form. //-->");
 }
 

+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)
 {
 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 (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
        /*
         * 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);
        }
 
                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");
        /* 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 *));
                        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 ) {
        }
 
        if ( req->p == NULL ) {

diff --git a/man.cgi.8 b/man.cgi.8
index 74ab525aa3d7384424cc3dde87c14a04c50d006b..69335e632ec99453ab02fcda0d86bb3d69d67881 100644 (file)
--- a/man.cgi.8
+++ b/man.cgi.8
@@ -1,4 +1,4 @@
-.\"     $Id: man.cgi.8,v 1.8 2014/07/21 15:45:17 schwarze Exp $
+.\"     $Id: man.cgi.8,v 1.9 2014/07/22 18:14:13 schwarze Exp $

 .\"
 .\" Copyright (c) 2014 Ingo Schwarze <schwarze@openbsd.org>
 .\"
 .\"
 .\" Copyright (c) 2014 Ingo Schwarze <schwarze@openbsd.org>
 .\"


@@ -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.
 .\"
 .\" 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
 .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
 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 :
 .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.
 .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
 .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.
 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
 .It Pa /man/OpenBSD-current/man1/mandoc.1
 An example
 .Xr mdoc 7