CVE-2015-3183: Improper Input Validation (CWE-20)
value: The chunked transfer coding implementation in the Apache HTTP Server before 2.4.14 does not properly parse chunk headers, which allows remote attackers to conduct HTTP request smuggling attacks via a crafted request, related to mishandling of large chunk-size values and invalid chunk-extension characters in modules/http/http_filters.c.
CWE description: The product receives input or data, but it does
        not validate or incorrectly validates that the input has the
        properties that are required to process the data safely and
        correctly.
Published: 2015-07-20T23:59Z
Repository: https://github.com/apache/httpd
Language: Other
Files changed: CHANGES, docs/log-message-tags/next-number, modules/http/http_filters.c
Commit date: 2015-06-09 20:12:31 +0000
Commit message: Limit accepted chunk-size to 2^63-1 and be strict about chunk-ext
authorized characters.

Submitted by: Yann Ylavic



git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1684513 13f79535-47bb-0310-9956-ffa450edef68

Diff: diff --git a/CHANGES b/CHANGES
index 63e7d0fc11..5361e50fbe 100644
--- a/CHANGES
+++ b/CHANGES
@@ -1,8 +1,11 @@
                                                          -*- coding: utf-8 -*-
 Changes with Apache 2.5.0
 
+  *) core: Limit accepted chunk-size to 2^63-1 and be strict about chunk-ext
+     authorized characters.  [Yann Ylavic]
+
   *) mod_ssl: When SSLVerify is disabled (NONE), don't force a renegotiation if
      the SSLVerifyDepth applied with the default/handshaken vhost differs from
      the one applicable with the finally selected vhost.  [Yann Ylavic]
      
   *) core: Avoid a possible truncation of the faulty header included in the
diff --git a/docs/log-message-tags/next-number b/docs/log-message-tags/next-number
index c914c6a5d9..6ba94a4313 100644
--- a/docs/log-message-tags/next-number
+++ b/docs/log-message-tags/next-number
@@ -1 +1 @@
-2901
+2902
diff --git a/modules/http/http_filters.c b/modules/http/http_filters.c
index 7ebb619f55..3ad3dced4c 100644
--- a/modules/http/http_filters.c
+++ b/modules/http/http_filters.c
@@ -55,28 +55,28 @@
 #include <unistd.h>
 #endif
 
 APLOG_USE_MODULE(http);
 
-#define INVALID_CHAR -2
-
 typedef struct http_filter_ctx
 {
     apr_off_t remaining;
     apr_off_t limit;
     apr_off_t limit_used;
     apr_int32_t chunk_used;
-    apr_int16_t chunkbits;
+    apr_int32_t chunkbits;
     enum
     {
         BODY_NONE, /* streamed data */
         BODY_LENGTH, /* data constrained by content length */
         BODY_CHUNK, /* chunk expected */
         BODY_CHUNK_PART, /* chunk digits */
         BODY_CHUNK_EXT, /* chunk extension */
+        BODY_CHUNK_LF, /* got CR, expect LF after digits/extension */
         BODY_CHUNK_DATA, /* data constrained by chunked encoding */
-        BODY_CHUNK_END, /* chunk terminating CRLF */
+        BODY_CHUNK_END, /* chunked data terminating CRLF */
+        BODY_CHUNK_END_LF, /* got CR, expect LF after data */
         BODY_CHUNK_TRAILER /* trailers */
     } state;
     unsigned int eos_sent :1;
 } http_ctx_t;
 
@@ -87,70 +87,98 @@ typedef struct http_filter_ctx
  *  2) If the conversion used the correct number of bits, but an overflow
  *     caused only the sign bit to flip, then APR_ENOSPC is returned.
  * In general, any negative number can be considered an overflow error.
  */
 static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer,
-        apr_size_t len, int linelimit)
+                                     apr_size_t len, int linelimit)
 {
     apr_size_t i = 0;
 
     while (i < len) {
         char c = buffer[i];
 
         ap_xlate_proto_from_ascii(&c, 1);
 
         /* handle CRLF after the chunk */
-        if (ctx->state == BODY_CHUNK_END) {
+        if (ctx->state == BODY_CHUNK_END
+                || ctx->state == BODY_CHUNK_END_LF) {
             if (c == LF) {
                 ctx->state = BODY_CHUNK;
             }
+            else if (c == CR && ctx->state == BODY_CHUNK_END) {
+                ctx->state = BODY_CHUNK_END_LF;
+            }
+            else {
+                /*
+                 * LF expected.
+                 */
+                return APR_EINVAL;
+            }
             i++;
             continue;
         }
 
         /* handle start of the chunk */
         if (ctx->state == BODY_CHUNK) {
             if (!apr_isxdigit(c)) {
                 /*
-                 * Detect invalid character at beginning. This also works for empty
-                 * chunk size lines.
+                 * Detect invalid character at beginning. This also works for
+                 * empty chunk size lines.
                  */
-                return APR_EGENERAL;
+                return APR_EINVAL;
             }
             else {
                 ctx->state = BODY_CHUNK_PART;
             }
             ctx->remaining = 0;
-            ctx->chunkbits = sizeof(long) * 8;
+            ctx->chunkbits = sizeof(apr_off_t) * 8;
             ctx->chunk_used = 0;
         }
 
-        /* handle a chunk part, or a chunk extension */
-        /*
-         * In theory, we are supposed to expect CRLF only, but our
-         * test suite sends LF only. Tolerate a missing CR.
-         */
-        if (c == ';' || c == CR) {
-            ctx->state = BODY_CHUNK_EXT;
-        }
-        else if (c == LF) {
+        if (c == LF) {
             if (ctx->remaining) {
                 ctx->state = BODY_CHUNK_DATA;
             }
             else {
                 ctx->state = BODY_CHUNK_TRAILER;
             }
         }
-        else if (ctx->state != BODY_CHUNK_EXT) {
-            int xvalue = 0;
+        else if (ctx->state == BODY_CHUNK_LF) {
+            /*
+             * LF expected.
+             */
+            return APR_EINVAL;
+        }
+        else if (c == CR) {
+            ctx->state = BODY_CHUNK_LF;
+        }
+        else if (c == ';') {
+            ctx->state = BODY_CHUNK_EXT;
+        }
+        else if (ctx->state == BODY_CHUNK_EXT) {
+            /*
+             * Control chars (but tabs) are invalid.
+             */
+            if (c != '\t' && apr_iscntrl(c)) {
+                return APR_EINVAL;
+            }
+        }
+        else if (ctx->state == BODY_CHUNK_PART) {
+            int xvalue;
 
             /* ignore leading zeros */
             if (!ctx->remaining && c == '0') {
                 i++;
                 continue;
             }
 
+            ctx->chunkbits -= 4;
+            if (ctx->chunkbits < 0) {
+                /* overflow */
+                return APR_ENOSPC;
+            }
+
             if (c >= '0' && c <= '9') {
                 xvalue = c - '0';
             }
             else if (c >= 'A' && c <= 'F') {
                 xvalue = c - 'A' + 0xa;
@@ -158,20 +186,22 @@ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer,
             else if (c >= 'a' && c <= 'f') {
                 xvalue = c - 'a' + 0xa;
             }
             else {
                 /* bogus character */
-                return APR_EGENERAL;
+                return APR_EINVAL;
             }
 
             ctx->remaining = (ctx->remaining << 4) | xvalue;
-            ctx->chunkbits -= 4;
-            if (ctx->chunkbits <= 0 || ctx->remaining < 0) {
+            if (ctx->remaining < 0) {
                 /* overflow */
                 return APR_ENOSPC;
             }
-
+        }
+        else {
+            /* Should not happen */
+            return APR_EGENERAL;
         }
 
         i++;
     }
 
@@ -230,11 +260,12 @@ static apr_status_t read_chunked_trailers(http_ctx_t *ctx, ap_filter_t *f,
  * proxied servers (mod_proxy).  It handles chunked and content-length
  * bodies.  This can only be inserted/used after the headers
  * are successfully parsed.
  */
 apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
-        ap_input_mode_t mode, apr_read_type_e block, apr_off_t readbytes)
+                            ap_input_mode_t mode, apr_read_type_e block,
+                            apr_off_t readbytes)
 {
     core_server_config *conf;
     apr_bucket *e;
     http_ctx_t *ctx = f->ctx;
     apr_status_t rv;
@@ -280,12 +311,12 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
                  * present in a response and the chunked transfer coding is not
                  * the final encoding, the message body length is determined by
                  * reading the connection until it is closed by the server."
                  */
                 ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(02555)
-                              "Unknown Transfer-Encoding: %s;"
-                              " using read-until-close", tenc);
+                              "Unknown Transfer-Encoding: %s; "
+                              "using read-until-close", tenc);
                 tenc = NULL;
             }
             else {
                 /* Something that isn't a HTTP request, unless some future
                  * edition defines new transfer encodings, is unsupported.
@@ -306,26 +337,24 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
              * and a negative number. */
             if (apr_strtoff(&ctx->remaining, lenp, &endstr, 10)
                     || endstr == lenp || *endstr || ctx->remaining < 0) {
 
                 ctx->remaining = 0;
-                ap_log_rerror(
-                        APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01587)
-                        "Invalid Content-Length");
+                ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01587)
+                              "Invalid Content-Length");
 
-                return APR_ENOSPC;
+                return APR_EINVAL;
             }
 
             /* If we have a limit in effect and we know the C-L ahead of
              * time, stop it here if it is invalid.
              */
             if (ctx->limit && ctx->limit < ctx->remaining) {
-                ap_log_rerror(
-                        APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01588)
-                        "Requested content-length of %" APR_OFF_T_FMT
-                        " is larger than the configured limit"
-                        " of %" APR_OFF_T_FMT, ctx->remaining, ctx->limit);
+                ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01588)
+                          "Requested content-length of %" APR_OFF_T_FMT
+                          " is larger than the configured limit"
+                          " of %" APR_OFF_T_FMT, ctx->remaining, ctx->limit);
                 return APR_ENOSPC;
             }
         }
 
         /* If we don't have a request entity indicated by the headers, EOS.
@@ -376,10 +405,11 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
                 APR_BRIGADE_INSERT_HEAD(bb, e);
                 e = apr_bucket_flush_create(f->c->bucket_alloc);
                 APR_BRIGADE_INSERT_TAIL(bb, e);
 
                 rv = ap_pass_brigade(f->c->output_filters, bb);
+                apr_brigade_cleanup(bb);
                 if (rv != APR_SUCCESS) {
                     return AP_FILTER_ERROR;
                 }
             }
         }
@@ -399,11 +429,13 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
         /* read and handle the brigade */
         switch (ctx->state) {
         case BODY_CHUNK:
         case BODY_CHUNK_PART:
         case BODY_CHUNK_EXT:
-        case BODY_CHUNK_END: {
+        case BODY_CHUNK_LF:
+        case BODY_CHUNK_END:
+        case BODY_CHUNK_END_LF: {
 
             rv = ap_get_brigade(f->next, b, AP_MODE_GETLINE, block, 0);
 
             /* for timeout */
             if (block == APR_NONBLOCK_READ
@@ -431,12 +463,13 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
                     if (rv == APR_SUCCESS) {
                         rv = parse_chunk_size(ctx, buffer, len,
                                 f->r->server->limit_req_fieldsize);
                     }
                     if (rv != APR_SUCCESS) {
-                        ap_log_rerror(
-                                APLOG_MARK, APLOG_INFO, rv, f->r, APLOGNO(01590) "Error reading chunk %s ", (APR_ENOSPC == rv) ? "(overflow)" : "");
+                        ap_log_rerror(APLOG_MARK, APLOG_INFO, rv, f->r, APLOGNO(01590)
+                                      "Error reading chunk %s ",
+                                      (APR_ENOSPC == rv) ? "(overflow)" : "");
                         return rv;
                     }
                 }
 
                 apr_bucket_delete(e);
@@ -444,13 +477,12 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
             }
             again = 1; /* come around again */
 
             if (ctx->state == BODY_CHUNK_TRAILER) {
                 /* Treat UNSET as DISABLE - trailers aren't merged by default */
-                int merge_trailers =
-                    conf->merge_trailers == AP_MERGE_TRAILERS_ENABLE;
-                return read_chunked_trailers(ctx, f, b, merge_trailers);
+                return read_chunked_trailers(ctx, f, b,
+                            conf->merge_trailers == AP_MERGE_TRAILERS_ENABLE);
             }
 
             break;
         }
         case BODY_NONE:
@@ -520,13 +552,14 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
                 /* FIXME: Note that we might get slightly confused on chunked inputs
                  * as we'd need to compensate for the chunk lengths which may not
                  * really count.  This seems to be up for interpretation.  */
                 ctx->limit_used += totalread;
                 if (ctx->limit < ctx->limit_used) {
-                    ap_log_rerror(
-                            APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01591) "Read content-length of %" APR_OFF_T_FMT " is larger than the configured limit"
-                            " of %" APR_OFF_T_FMT, ctx->limit_used, ctx->limit);
+                    ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01591)
+                                  "Read content-length of %" APR_OFF_T_FMT
+                                  " is larger than the configured limit"
+                                  " of %" APR_OFF_T_FMT, ctx->limit_used, ctx->limit);
                     return APR_ENOSPC;
                 }
             }
 
             break;
@@ -547,11 +580,14 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
             }
 
             break;
         }
         default: {
-            break;
+            /* Should not happen */
+            ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, f->r, APLOGNO(02901)
+                          "Unexpected body state (%i)", (int)ctx->state);
+            return APR_EGENERAL;
         }
         }
 
     } while (again);
 

Vulnerable code: 2901
#define INVALID_CHAR -2

    apr_int16_t chunkbits;
        BODY_CHUNK_END, /* chunk terminating CRLF */
        apr_size_t len, int linelimit)
        if (ctx->state == BODY_CHUNK_END) {
                 * Detect invalid character at beginning. This also works for empty
                 * chunk size lines.
                return APR_EGENERAL;
            ctx->chunkbits = sizeof(long) * 8;
        /* handle a chunk part, or a chunk extension */
        /*
         * In theory, we are supposed to expect CRLF only, but our
         * test suite sends LF only. Tolerate a missing CR.
         */
        if (c == ';' || c == CR) {
            ctx->state = BODY_CHUNK_EXT;
        }
        else if (c == LF) {
        else if (ctx->state != BODY_CHUNK_EXT) {
            int xvalue = 0;
                return APR_EGENERAL;
            ctx->chunkbits -= 4;
            if (ctx->chunkbits <= 0 || ctx->remaining < 0) {

        ap_input_mode_t mode, apr_read_type_e block, apr_off_t readbytes)
                              "Unknown Transfer-Encoding: %s;"
                              " using read-until-close", tenc);
                ap_log_rerror(
                        APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01587)
                        "Invalid Content-Length");
                return APR_ENOSPC;
                ap_log_rerror(
                        APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01588)
                        "Requested content-length of %" APR_OFF_T_FMT
                        " is larger than the configured limit"
                        " of %" APR_OFF_T_FMT, ctx->remaining, ctx->limit);
        case BODY_CHUNK_END: {
                        ap_log_rerror(
                                APLOG_MARK, APLOG_INFO, rv, f->r, APLOGNO(01590) "Error reading chunk %s ", (APR_ENOSPC == rv) ? "(overflow)" : "");
                int merge_trailers =
                    conf->merge_trailers == AP_MERGE_TRAILERS_ENABLE;
                return read_chunked_trailers(ctx, f, b, merge_trailers);
                    ap_log_rerror(
                            APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01591) "Read content-length of %" APR_OFF_T_FMT " is larger than the configured limit"
                            " of %" APR_OFF_T_FMT, ctx->limit_used, ctx->limit);
            break;
Fixed code:   *) core: Limit accepted chunk-size to 2^63-1 and be strict about chunk-ext
     authorized characters.  [Yann Ylavic]

2902
    apr_int32_t chunkbits;
        BODY_CHUNK_LF, /* got CR, expect LF after digits/extension */
        BODY_CHUNK_END, /* chunked data terminating CRLF */
        BODY_CHUNK_END_LF, /* got CR, expect LF after data */
                                     apr_size_t len, int linelimit)
        if (ctx->state == BODY_CHUNK_END
                || ctx->state == BODY_CHUNK_END_LF) {
            else if (c == CR && ctx->state == BODY_CHUNK_END) {
                ctx->state = BODY_CHUNK_END_LF;
            }
            else {
                /*
                 * LF expected.
                 */
                return APR_EINVAL;
            }
                 * Detect invalid character at beginning. This also works for
                 * empty chunk size lines.
                return APR_EINVAL;
            ctx->chunkbits = sizeof(apr_off_t) * 8;
        if (c == LF) {
        else if (ctx->state == BODY_CHUNK_LF) {
            /*
             * LF expected.
             */
            return APR_EINVAL;
        }
        else if (c == CR) {
            ctx->state = BODY_CHUNK_LF;
        }
        else if (c == ';') {
            ctx->state = BODY_CHUNK_EXT;
        }
        else if (ctx->state == BODY_CHUNK_EXT) {
            /*
             * Control chars (but tabs) are invalid.
             */
            if (c != '\t' && apr_iscntrl(c)) {
                return APR_EINVAL;
            }
        }
        else if (ctx->state == BODY_CHUNK_PART) {
            int xvalue;
            ctx->chunkbits -= 4;
            if (ctx->chunkbits < 0) {
                /* overflow */
                return APR_ENOSPC;
            }

                return APR_EINVAL;
            if (ctx->remaining < 0) {
        }
        else {
            /* Should not happen */
            return APR_EGENERAL;
                            ap_input_mode_t mode, apr_read_type_e block,
                            apr_off_t readbytes)
                              "Unknown Transfer-Encoding: %s; "
                              "using read-until-close", tenc);
                ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01587)
                              "Invalid Content-Length");
                return APR_EINVAL;
                ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01588)
                          "Requested content-length of %" APR_OFF_T_FMT
                          " is larger than the configured limit"
                          " of %" APR_OFF_T_FMT, ctx->remaining, ctx->limit);
                apr_brigade_cleanup(bb);
        case BODY_CHUNK_LF:
        case BODY_CHUNK_END:
        case BODY_CHUNK_END_LF: {
                        ap_log_rerror(APLOG_MARK, APLOG_INFO, rv, f->r, APLOGNO(01590)
                                      "Error reading chunk %s ",
                                      (APR_ENOSPC == rv) ? "(overflow)" : "");
                return read_chunked_trailers(ctx, f, b,
                            conf->merge_trailers == AP_MERGE_TRAILERS_ENABLE);
                    ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01591)
                                  "Read content-length of %" APR_OFF_T_FMT
                                  " is larger than the configured limit"
                                  " of %" APR_OFF_T_FMT, ctx->limit_used, ctx->limit);
            /* Should not happen */
            ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, f->r, APLOGNO(02901)
                          "Unexpected body state (%i)", (int)ctx->state);
            return APR_EGENERAL;