CVE-2015-3183: DEPRECATED: Code (CWE-17)
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: This entry has been deprecated.  It was originally used for organizing the Development View (CWE-699) and some other views, but it introduced unnecessary complexity and depth to the resulting tree.
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;