httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
Subject [PATCH] CAN-2004-0942 fix
Date Mon, 01 Nov 2004 16:56:55 GMT
http://lists.netsys.com/pipermail/full-disclosure/2004-November/028248.html

describes a memory consumption DoS against 2.0, which has been assigned
CVE CAN-2004-0942; in the case given, ap_rgetline_core will allocate
approx N * 3 bytes to hold the line, but then trim all but one byte of
trailing whitespace, so the field length limit is not correctly
enforced.

The simplest fix is to move the trailing-whitespace-trimming logic out
of ap_rgetline_core into ap_get_mime_headers_core, patch below does that
and also simplifies the over-complicated logic for stripping LWS between
field-name and colon.

Looking at the other places where ap_rgetline is used: in the proxy,
ap_proxy_read_headers already strips trailing whitespace, it doesn't
really matter to ap_proxy_http_process_response whether trailing
whitespace is stripped on the status-line.

In protocol.c, read_request_line will use the sscanf path rather than
the fast-path to handle status-lines with trailing whitespace, which is 
OK too.

Index: server/protocol.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/protocol.c,v
retrieving revision 1.158
diff -u -r1.158 protocol.c
--- server/protocol.c	25 Oct 2004 15:59:43 -0000	1.158
+++ server/protocol.c	1 Nov 2004 16:32:48 -0000
@@ -306,35 +306,13 @@
         }
     }
 
-    /* We now go backwards over any CR (if present) or white spaces.
-     *
-     * Trim any extra trailing spaces or tabs except for the first
-     * space or tab at the beginning of a blank string.  This makes
-     * it much easier to check field values for exact matches, and
-     * saves memory as well.  Terminate string at end of line.
-     */
-    pos = last_char;
-    if (pos > *s && *(pos - 1) == APR_ASCII_CR) {
-        --pos;
-    }
-
-    /* Trim any extra trailing spaces or tabs except for the first
-     * space or tab at the beginning of a blank string.  This makes
-     * it much easier to check field values for exact matches, and
-     * saves memory as well.
-     */
-    while (pos > ((*s) + 1)
-           && (*(pos - 1) == APR_ASCII_BLANK || *(pos - 1) == APR_ASCII_TAB)) {
-        --pos;
+    /* Now NUL-terminate the string at the end of the line; 
+     * if the last-but-one character is a CR, terminate there */
+    if (last_char > *s && last_char[-1] == APR_ASCII_CR) {
+        last_char--;
     }
-
-    /* Since we want to remove the LF from the line, we'll go ahead
-     * and set this last character to be the term NULL and reset
-     * bytes_handled accordingly.
-     */
-    *pos = '\0';
-    last_char = pos;
-    bytes_handled = pos - *s;
+    *last_char = '\0';
+    bytes_handled = last_char - *s;
 
     /* If we're folding, we have more work to do.
      *
@@ -760,7 +738,7 @@
                 last_len += len;
                 folded = 1;
             }
-            else {
+            else /* not a continuation line */ {
 
                 if (r->server->limit_req_fields
                     && (++fields_read > r->server->limit_req_fields)) {
@@ -783,29 +761,26 @@
                                                "</pre>\n", NULL));
                     return;
                 }
+                
+                tmp_field = value - 1; /* last character of field-name */
+
+                *value++ = '\0'; /* NUL-terminate at colon */
 
-                *value = '\0';
-                tmp_field = value;  /* used to trim the whitespace between key
-                                     * token and separator
-                                     */
-                ++value;
                 while (*value == ' ' || *value == '\t') {
                     ++value;            /* Skip to start of value   */
                 }
 
-                /* This check is to avoid any invalid memory reference while
-                 * traversing backwards in the key. To avoid a case where
-                 * the header starts with ':' (or with just some white
-                 * space and the ':') followed by the value
-                 */
-                if (tmp_field > last_field) {
-                    --tmp_field;
-                    while ((tmp_field > last_field) &&
-                           (*tmp_field == ' ' || *tmp_field == '\t')) {
-                        --tmp_field;   /* Removing LWS between key and ':' */
-                    }
-                    ++tmp_field;
-                    *tmp_field = '\0';
+                /* Strip LWS after field-name: */
+                while (tmp_field > last_field 
+                       && (*tmp_field == ' ' || *tmp_field == '\t')) {
+                    *tmp_field-- = '\0';
+                }
+                
+                /* Strip LWS after field-value: */
+                tmp_field = last_field + last_len - 1;
+                while (tmp_field > value
+                       && (*tmp_field == ' ' || *tmp_field == '\t')) {
+                    *tmp_field-- = '\0';
                 }
 
                 apr_table_addn(r->headers_in, last_field, value);

Mime
View raw message