Re: Mitigating XSS in the mod_perl API

On Tue, Jan 28, 2003 at 11:30:18AM +0000, Matt Sergeant wrote:
> Here's the patch in case anyone is interested in applying it to their own
> apache. I think there's probably a bug in that \n might not always be the
> right thing to look for (CRLF issues), so please send me corrections ;-)
> 
> I haven't actually run this, so it might not work. But to quote Lord
> Flashheart: "That's the kind of guy I am". :-)
> 
> Index: src/main/http_protocol.c
> ===================================================================
> RCS file: /home/cvs/apache-1.3/src/main/http_protocol.c,v
> retrieving revision 1.329
> diff -u -r1.329 http_protocol.c
> --- src/main/http_protocol.c    3 Oct 2002 20:51:53 -0000       1.329
> +++ src/main/http_protocol.c    28 Jan 2003 11:26:37 -0000
> @@ -1561,8 +1561,16 @@
>                                              const char *fieldname,
>                                              const char *fieldval)
>  {
> +    char *line_feed;
>      if (strcasecmp(fieldname, "ETag") == 0) {
>          if (ap_table_get(r->notes, "no-etag") != NULL) {
> +            return 1;
> +        }
> +    }
> +    if ((line_feed = strchr(fieldval, '\n')) != NULL) {
> +        /* don't allow any headers with line feeds in them */
> +        if (line_feed[1] != ' ' && line_feed[1] != '\t') {
> +            /* unless it's a continuation */
>              return 1;
>          }
>      }

I'm wondering if it's not smarter to go over fieldval and at the first
occourance of a \r or \n terminate the line there? I think there will
be more problems by just not outputting the headers in the first place.

Something like:

--- http_protocol.c.old Mon Jan 27 09:45:57 2003
+++ http_protocol.c     Tue Jan 28 08:54:37 2003
@@ -1561,10 +1561,14 @@
                                             const char *fieldname,
                                             const char *fieldval)
 {
+    char *f;
     if (strcasecmp(fieldname, "ETag") == 0) {
         if (ap_table_get(r->notes, "no-etag") != NULL) {
             return 1;
         }
+    }
+    if ((f = strchr(fieldval, '\r')) != NULL || (f = strchr(fieldval, '\n')) 
!= NULL) {
+       *f = '\0';
     }
     return (0 < ap_rvputs(r, fieldname, ": ", fieldval, CRLF, NULL));
 }

Of course, a check for continuation like in your patch is needed.

Attached is a module to test it with

mod_break.c
Description: Text document

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@xxxxxxxxxxxxxxx
For additional commands, e-mail: dev-help@xxxxxxxxxxxxxxx