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 --git a/CHANGES b/CHANGES
index 63e7d0fc11071eb1b4d22aa7d19c931e31bfae1d..5361e50fbede3ac98b2f6e9e28224aa6a815760c 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -1,6 +1,9 @@
                                                          -*- 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]

diff --git a/docs/log-message-tags/next-number b/docs/log-message-tags/next-number
index c914c6a5d92ac836303a6d856c07a835a3043945..6ba94a4313d83259f6026fdd401ed032bd3180ad 100644 (file)
--- 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 7ebb619f556b53c32e05c4c3b531d20067f865c0..3ad3dced4c1ea917565d7bf87331506835aac662 100644 (file)
--- a/modules/http/http_filters.c
+++ b/modules/http/http_filters.c
@@ -57,15 +57,13 @@
 
 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 */
@@ -73,8 +71,10 @@ typedef struct http_filter_ctx
         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;
@@ -89,7 +89,7 @@ typedef struct http_filter_ctx
  * 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;
 
@@ -99,10 +99,20 @@ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer,
         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;
         }
@@ -111,28 +121,20 @@ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer,
         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;
             }
@@ -140,8 +142,28 @@ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer,
                 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') {
@@ -149,6 +171,12 @@ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer,
                 continue;
             }
 
+            ctx->chunkbits -= 4;
+            if (ctx->chunkbits < 0) {
+                /* overflow */
+                return APR_ENOSPC;
+            }
+
             if (c >= '0' && c <= '9') {
                 xvalue = c - '0';
             }
@@ -160,16 +188,18 @@ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer,
             }
             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++;
@@ -232,7 +262,8 @@ static apr_status_t read_chunked_trailers(http_ctx_t *ctx, ap_filter_t *f,
  * 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;
@@ -282,8 +313,8 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
                  * 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 {
@@ -308,22 +339,20 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
                     || 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;
             }
         }
@@ -378,6 +407,7 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
                 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;
                 }
@@ -401,7 +431,9 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
         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);
 
@@ -433,8 +465,9 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
                                 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;
                     }
                 }
@@ -446,9 +479,8 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
 
             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;
@@ -522,9 +554,10 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
                  * 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;
                 }
             }
@@ -549,7 +582,10 @@ 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;
         }
         }