* Fixes #201: do not count repeated headers with same name against t…

…he field count limit. The are merged internally, as if sent in a single HTTP/1 line.
icing · Jul 8, 2020 · 09f9b6a · 09f9b6a
1 parent f60fbde
commit 09f9b6a
Show file tree

Hide file tree

Showing 9 changed files with 107 additions and 45 deletions.
diff --git a/ChangeLog b/ChangeLog
@@ -1,3 +1,5 @@
+ * Fixes #201: do not count repeated headers with same name against the field
+   count limit. The are merged internally, as if sent in a single HTTP/1 line.
  * Refrain from detecting (intermediate) responses, when the connection has
    already been aborted or non-flush meta buckets are encountered.
  * Changed terminology to master/secondary connections in the source.

diff --git a/configure.ac b/configure.ac
@@ -14,7 +14,7 @@
 #
 
 AC_PREREQ([2.69])
-AC_INIT([mod_http2], [1.15.11], [[email protected]])
+AC_INIT([mod_http2], [1.15.12], [[email protected]])
 
 LT_PREREQ([2.2.6])
 LT_INIT()

diff --git a/mod_http2/h2_request.c b/mod_http2/h2_request.c
@@ -47,9 +47,9 @@ typedef struct {
 static int set_h1_header(void *ctx, const char *key, const char *value)
 {
     h1_ctx *x = ctx;
-    x->status = h2_req_add_header(x->headers, x->pool, key, strlen(key), 
-                                  value, strlen(value));
-    return (x->status == APR_SUCCESS)? 1 : 0;
+    int was_added;
+    h2_req_add_header(x->headers, x->pool, key, strlen(key), value, strlen(value), 0, &was_added);
+    return 1;
 }
 
 apr_status_t h2_request_rcreate(h2_request **preq, apr_pool_t *pool, 
@@ -99,10 +99,12 @@ apr_status_t h2_request_rcreate(h2_request **preq, apr_pool_t *pool,
 
 apr_status_t h2_request_add_header(h2_request *req, apr_pool_t *pool, 
                                    const char *name, size_t nlen,
-                                   const char *value, size_t vlen)
+                                   const char *value, size_t vlen,
+                                   size_t max_field_len, int *pwas_added)
 {
     apr_status_t status = APR_SUCCESS;
 
+    *pwas_added = 0;
     if (nlen <= 0) {
         return status;
     }
@@ -143,8 +145,9 @@ apr_status_t h2_request_add_header(h2_request *req, apr_pool_t *pool,
         }
     }
     else {
-        /* non-pseudo header, append to work bucket of stream */
-        status = h2_req_add_header(req->headers, pool, name, nlen, value, vlen);
+        /* non-pseudo header, add to table */
+        status = h2_req_add_header(req->headers, pool, name, nlen, value, vlen, 
+                                   max_field_len, pwas_added);
     }
 
     return status;

diff --git a/mod_http2/h2_request.h b/mod_http2/h2_request.h
@@ -24,7 +24,8 @@ apr_status_t h2_request_rcreate(h2_request **preq, apr_pool_t *pool,
 
 apr_status_t h2_request_add_header(h2_request *req, apr_pool_t *pool,
                                    const char *name, size_t nlen,
-                                   const char *value, size_t vlen);
+                                   const char *value, size_t vlen,
+                                   size_t max_field_len, int *pwas_added);
 
 apr_status_t h2_request_add_trailer(h2_request *req, apr_pool_t *pool,
                                     const char *name, size_t nlen,

diff --git a/mod_http2/h2_stream.c b/mod_http2/h2_stream.c
@@ -654,11 +654,14 @@ static void set_error_response(h2_stream *stream, int http_status)
 
 static apr_status_t add_trailer(h2_stream *stream,
                                 const char *name, size_t nlen,
-                                const char *value, size_t vlen)
+                                const char *value, size_t vlen,
+                                size_t max_field_len, int *pwas_added)
 {
     conn_rec *c = stream->session->c;
     char *hname, *hvalue;
+    const char *existing;
 
+    *pwas_added = 0;
     if (nlen == 0 || name[0] == ':') {
         ap_log_cerror(APLOG_MARK, APLOG_DEBUG, APR_EINVAL, c, 
                       H2_STRM_LOG(APLOGNO(03060), stream, 
@@ -672,8 +675,15 @@ static apr_status_t add_trailer(h2_stream *stream,
         stream->trailers = apr_table_make(stream->pool, 5);
     }
     hname = apr_pstrndup(stream->pool, name, nlen);
-    hvalue = apr_pstrndup(stream->pool, value, vlen);
     h2_util_camel_case_header(hname, nlen);
+    existing = apr_table_get(stream->trailers, hname);
+    if (max_field_len 
+        && ((existing? strlen(existing)+2 : 0) + vlen + nlen + 2 > max_field_len)) {
+        /* "key: (oldval, )?nval" is too long */
+        return APR_EINVAL;
+    }
+    if (!existing) *pwas_added = 1;
+    hvalue = apr_pstrndup(stream->pool, value, vlen);
     apr_table_mergen(stream->trailers, hname, hvalue);
     ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, c, 
                   H2_STRM_MSG(stream, "added trailer '%s: %s'"), hname, hvalue);
@@ -686,13 +696,13 @@ apr_status_t h2_stream_add_header(h2_stream *stream,
                                   const char *value, size_t vlen)
 {
     h2_session *session = stream->session;
-    int error = 0;
-    apr_status_t status;
+    int error = 0, was_added = 0;
+    apr_status_t status = APR_SUCCESS;
 
     if (stream->has_response) {
         return APR_EINVAL;    
     }
-    ++stream->request_headers_added;
+
     if (name[0] == ':') {
         if ((vlen) > session->s->limit_req_line) {
             /* pseudo header: approximation of request line size check */
@@ -703,9 +713,35 @@ apr_status_t h2_stream_add_header(h2_stream *stream,
                                           "LimitRequestFieldSize: %s"), name);
             }
             error = HTTP_REQUEST_URI_TOO_LARGE;
+            goto cleanup;
+        }
+    }
+
+    if (stream->request_headers_added > session->s->limit_req_fields) {
+        /* already over limit, count this attempt, but do not take it in */
+        ++stream->request_headers_added;
+    }
+    else if (H2_SS_IDLE == stream->state) {
+        if (!stream->rtmp) {
+            stream->rtmp = h2_req_create(stream->id, stream->pool, 
+                                         NULL, NULL, NULL, NULL, NULL, 0);
         }
+        status = h2_request_add_header(stream->rtmp, stream->pool,
+                                       name, nlen, value, vlen,
+                                       session->s->limit_req_fieldsize, &was_added);
+        if (was_added) ++stream->request_headers_added;
+    }
+    else if (H2_SS_OPEN == stream->state) {
+        status = add_trailer(stream, name, nlen, value, vlen,
+                             session->s->limit_req_fieldsize, &was_added);
+        if (was_added) ++stream->request_headers_added;
     }
-    else if ((nlen + 2 + vlen) > session->s->limit_req_fieldsize) {
+    else {
+        status = APR_EINVAL;
+        goto cleanup;
+    }
+
+    if (APR_EINVAL == status) {
         /* header too long */
         if (!h2_stream_is_ready(stream)) {
             ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, session->c,
@@ -714,13 +750,13 @@ apr_status_t h2_stream_add_header(h2_stream *stream,
                           (int)H2MIN(nlen, 80), name);
         }
         error = HTTP_REQUEST_HEADER_FIELDS_TOO_LARGE;
+        goto cleanup;
     }
 
-    if (stream->request_headers_added > session->s->limit_req_fields + 4) {
-        /* too many header lines, include 4 pseudo headers */
-        if (stream->request_headers_added 
-            > session->s->limit_req_fields + 4 + 100) {
-            /* yeah, right */
+    if (stream->request_headers_added > session->s->limit_req_fields) {
+        /* too many header lines */
+        if (stream->request_headers_added > session->s->limit_req_fields + 100) {
+            /* yeah, right, this request is way over the limit, say goodbye */
             h2_stream_rst(stream, H2_ERR_ENHANCE_YOUR_CALM);
             return APR_ECONNRESET;
         }
@@ -730,28 +766,15 @@ apr_status_t h2_stream_add_header(h2_stream *stream,
                                       "exceeds LimitRequestFields"));
         }
         error = HTTP_REQUEST_HEADER_FIELDS_TOO_LARGE;
+        goto cleanup;
     }
 
+cleanup:
     if (error) {
         set_error_response(stream, error);
         return APR_EINVAL; 
     }
-    else if (H2_SS_IDLE == stream->state) {
-        if (!stream->rtmp) {
-            stream->rtmp = h2_req_create(stream->id, stream->pool, 
-                                         NULL, NULL, NULL, NULL, NULL, 0);
-        }
-        status = h2_request_add_header(stream->rtmp, stream->pool,
-                                       name, nlen, value, vlen);
-    }
-    else if (H2_SS_OPEN == stream->state) {
-        status = add_trailer(stream, name, nlen, value, vlen);
-    }
-    else {
-        status = APR_EINVAL;
-    }
-
-    if (status != APR_SUCCESS) {
+    else if (status != APR_SUCCESS) {
         ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, session->c,
                       H2_STRM_MSG(stream, "header %s not accepted"), name);
         h2_stream_dispatch(stream, H2_SEV_CANCELLED);

diff --git a/mod_http2/h2_util.c b/mod_http2/h2_util.c
@@ -1803,22 +1803,29 @@ int h2_res_ignore_trailer(const char *name, size_t len)
 }
 
 apr_status_t h2_req_add_header(apr_table_t *headers, apr_pool_t *pool, 
-                               const char *name, size_t nlen,
-                               const char *value, size_t vlen)
+                              const char *name, size_t nlen,
+                              const char *value, size_t vlen,
+                              size_t max_field_len, int *pwas_added)
 {
     char *hname, *hvalue;
+    const char *existing;
 
+    *pwas_added = 0;
     if (h2_req_ignore_header(name, nlen)) {
         return APR_SUCCESS;
     }
     else if (H2_HD_MATCH_LIT("cookie", name, nlen)) {
-        const char *existing = apr_table_get(headers, "cookie");
+        existing = apr_table_get(headers, "cookie");
         if (existing) {
             char *nval;
 
             /* Cookie header come separately in HTTP/2, but need
              * to be merged by "; " (instead of default ", ")
              */
+            if (max_field_len && strlen(existing) + vlen + nlen + 4 > max_field_len) {
+                /* "key: oldval, nval" is too long */
+                return APR_EINVAL;
+            }
             hvalue = apr_pstrndup(pool, value, vlen);
             nval = apr_psprintf(pool, "%s; %s", existing, hvalue);
             apr_table_setn(headers, "Cookie", nval);
@@ -1832,8 +1839,16 @@ apr_status_t h2_req_add_header(apr_table_t *headers, apr_pool_t *pool,
     }
 
     hname = apr_pstrndup(pool, name, nlen);
-    hvalue = apr_pstrndup(pool, value, vlen);
     h2_util_camel_case_header(hname, nlen);
+    existing = apr_table_get(headers, hname);
+    if (max_field_len) {
+        if ((existing? strlen(existing)+2 : 0) + vlen + nlen + 2 > max_field_len) {
+            /* "key: (oldval, )?nval" is too long */
+            return APR_EINVAL;
+        }
+    }
+    if (!existing) *pwas_added = 1;
+    hvalue = apr_pstrndup(pool, value, vlen);
     apr_table_mergen(headers, hname, hvalue);
 
     return APR_SUCCESS;

diff --git a/mod_http2/h2_util.h b/mod_http2/h2_util.h
@@ -410,9 +410,14 @@ apr_status_t h2_res_create_ngheader(h2_ngheader **ph, apr_pool_t *p,
 apr_status_t h2_req_create_ngheader(h2_ngheader **ph, apr_pool_t *p, 
                                     const struct h2_request *req);
 
+/**
+ * Add a HTTP/2 header and return the table key if it really was added
+ * and not ignored.
+ */
 apr_status_t h2_req_add_header(apr_table_t *headers, apr_pool_t *pool, 
                                const char *name, size_t nlen,
-                               const char *value, size_t vlen);
+                               const char *value, size_t vlen,
+                               size_t max_field_len, int *pwas_added);
 
 /*******************************************************************************
  * h2_request helpers

diff --git a/mod_http2/h2_version.h b/mod_http2/h2_version.h
@@ -27,15 +27,15 @@
  * @macro
  * Version number of the http2 module as c string
  */
-#define MOD_HTTP2_VERSION "1.15.11-git"
+#define MOD_HTTP2_VERSION "1.15.12-git"
 
 /**
  * @macro
  * Numerical representation of the version number of the http2 module
  * release. This is a 24 bit number with 8 bits for major number, 8 bits
  * for minor and 8 bits for patch. Version 1.2.3 becomes 0x010203.
  */
-#define MOD_HTTP2_VERSION_NUM 0x010f0b
+#define MOD_HTTP2_VERSION_NUM 0x010f0c
 
 
 #endif /* mod_h2_h2_version_h */
diff --git a/test/e2e/test_200_header_invalid.py b/test/e2e/test_200_header_invalid.py
@@ -91,15 +91,16 @@ def test_200_11(self):
         for i in range(3): # make a 10000 char string
             val = "%s%s%s%s%s%s%s%s%s%s" % (val, val, val, val, val, val, val, val, val, val)
          # LimitRequestFieldSize 8190 ok, one more char -> 400 in HTTP/1.1
-         # (we send 4000+4188 since they are concatenated by ", "
-        r = TestEnv.curl_get(url, options=[ "-H", "x: %s" % (val[:4000]),  "-H", "x: %s" % (val[:4188]) ])
+         # (we send 4000+4185 since they are concatenated by ", " and start with "x: "
+        r = TestEnv.curl_get(url, options=[ "-H", "x: %s" % (val[:4000]),  "-H", "x: %s" % (val[:4185]) ])
         assert 200 == r["response"]["status"]
         r = TestEnv.curl_get(url, options=[ "--http1.1", "-H", "x: %s" % (val[:4000]),  "-H", "x: %s" % (val[:4189]) ])
         assert 400 == r["response"]["status"]
         r = TestEnv.curl_get(url, options=[ "-H", "x: %s" % (val[:4000]),  "-H", "x: %s" % (val[:4191]) ])
         assert 431 == r["response"]["status"]
 
-    # test header field lengths check, LimitRequestFields (default 100)
+    # test header field count, LimitRequestFields (default 100)
+    # see #201: several headers with same name are mered and count only once
     def test_200_12(self):
         url = TestEnv.mkurl("https", "cgi", "/")
         opt=[]
@@ -108,5 +109,17 @@ def test_200_12(self):
         r = TestEnv.curl_get(url, options=opt)
         assert 200 == r["response"]["status"]
         r = TestEnv.curl_get(url, options=(opt + [ "-H", "y: 2" ]))
+        assert 200 == r["response"]["status"]
+
+    # test header field count, LimitRequestFields (default 100)
+    # different header names count each
+    def test_200_13(self):
+        url = TestEnv.mkurl("https", "cgi", "/")
+        opt=[]
+        for i in range(98): # curl sends 2 headers itself (user-agent and accept)
+            opt += [ "-H", "x{0}: 1".format(i) ]
+        r = TestEnv.curl_get(url, options=opt)
+        assert 200 == r["response"]["status"]
+        r = TestEnv.curl_get(url, options=(opt + [ "-H", "y: 2" ]))
         assert 431 == r["response"]["status"]