Merge r1773293, r1773761, r1773779, r1773812, r1773861, r1773862, r1773865 from trunk:

change error handling for bad resp headers

 - avoid looping between ap_die and the http filter
 - remove the header that failed the check
 - keep calling apr_table_do until our fn stops matching

This is still not great. We get the original body, a 500 status
code and status line.

(r1773285 + fix for first return from check_headers)

Follow up to r1773293.
When check_headers() fails, clear anything (headers and body) from original/errorneous
response before returning 500.

Follow up to r1773761: don't check_headers() more than once.

Follow up to r1773761: don't recurse on internal redirects.

Follow up to r1773761: don't recurse on ap_send_error_response() either.

Follow up to r1773761: we need to check both ap_send_error_response() and internal redirect recursions.

Follow up to r1773761: improved recursion detection.
Submitted by: covener, ylavic, ylavic, ylavic, ylavic, ylavic, ylavic
Reviewed/backported by: jim

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1773995 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/CHANGES b/CHANGES
index ab18d3b42097d18f7dd62ed8b3a7e171cfb8364e..316a95aa7400a7e0df20e27cbb06cac1d1bc24e9 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -13,15 +13,19 @@ Changes with Apache 2.4.24
      [Dominic Scheirlinck <dominic vendhq.com>, Yann Ylavic]
 
   *) SECURITY: CVE-2016-2161 (cve.mitre.org)
-     mod_auth_digest: Prevent segfaults during client entry allocation when the
-     shared memory space is exhausted. [Maksim Malyutin <m.malyutin dsec.ru>,
-     Eric Covener, Jacob Champion]
+     mod_auth_digest: Prevent segfaults during client entry allocation when
+     the shared memory space is exhausted.
+     [Maksim Malyutin <m.malyutin dsec.ru>, Eric Covener, Jacob Champion]
 
   *) SECURITY: CVE-2016-0736 (cve.mitre.org)
      mod_session_crypto: Authenticate the session data/cookie with a
      MAC (SipHash) to prevent deciphering or tampering with a padding
      oracle attack.  [Yann Ylavic, Colm MacCarthaigh]
 
+  *) http_filters: Fix potential looping in new check_headers() due to new
+     pattern of ap_die() from http header filter. Explicitly clear the
+     previous headers and body.
+
   *) core: Drop Content-Length header and message-body from HTTP 204 responses.
      PR 51350 [Luca Toscano]
 

diff --git a/STATUS b/STATUS
index 39667338e8756e2041ba6c2fee9a7e55a32d10cf..5971ba22ec8b2ebf81142c93db1efca82bfd424a 100644 (file)
--- a/STATUS
+++ b/STATUS
@@ -113,21 +113,6 @@ CURRENT RELEASE NOTES:
 
 RELEASE SHOWSTOPPERS:
 
-  *) Looping during check_headers() failure.
-     Fix potential looping in new check_headers() due to new pattern of
-     ap_die() from http header filter.  Also, clear the previous headers
-     and body explicitly.
-     Trunk patch:  https://svn.apache.org/r1773293
-                   https://svn.apache.org/r1773761
-                   https://svn.apache.org/r1773779
-                   https://svn.apache.org/r1773812
-                   https://svn.apache.org/r1773861
-                   https://svn.apache.org/r1773862
-                   https://svn.apache.org/r1773865
-     2.4.x patch: trunk works, or (to ease review):
-                  http://home.apache.org/~ylavic/patches/httpd-2.4.x-r1773293_and_co.patch
-     +1: ylavic, wrowe, jim
-
   *) Final CVE check
 
 

diff --git a/modules/http/http_filters.c b/modules/http/http_filters.c
index de0b580ac5eed2598044cd56ee87d325863f80e3..92ab36dd49ff8672e94578e261edc9486be0c200 100644 (file)
--- a/modules/http/http_filters.c
+++ b/modules/http/http_filters.c
@@ -738,10 +738,22 @@ static APR_INLINE int check_headers(request_rec *r)
 
     ctx.r = r;
     ctx.strict = (conf->http_conformance != AP_HTTP_CONFORMANCE_UNSAFE);
-    if (!apr_table_do(check_header, &ctx, r->headers_out, NULL))
-        return 0; /* problem has been logged by check_header() */
+    return apr_table_do(check_header, &ctx, r->headers_out, NULL) &&
+           apr_table_do(check_header, &ctx, r->err_headers_out, NULL);
+}
 
-    return 1;
+static int check_headers_recursion(request_rec *r)
+{
+    request_rec *rr;
+    for (rr = r; rr; rr = rr->prev) {
+        void *dying = NULL;
+        apr_pool_userdata_get(&dying, "check_headers_recursion", rr->pool);
+        if (dying) {
+            return 1;
+        }
+    }
+    apr_pool_userdata_setn("true", "check_headers_recursion", NULL, r->pool);
+    return 0;
 }
 
 typedef struct header_struct {
@@ -1234,6 +1246,7 @@ AP_DECLARE_NONSTD(int) ap_send_http_trace(request_rec *r)
 
 typedef struct header_filter_ctx {
     int headers_sent;
+    int headers_error;
 } header_filter_ctx;
 
 AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f,
@@ -1249,23 +1262,34 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f,
     header_filter_ctx *ctx = f->ctx;
     const char *ctype;
     ap_bucket_error *eb = NULL;
+    int eos = 0;
 
     AP_DEBUG_ASSERT(!r->main);
 
-    if (r->header_only || r->status == HTTP_NO_CONTENT) {
-        if (!ctx) {
-            ctx = f->ctx = apr_pcalloc(r->pool, sizeof(header_filter_ctx));
-        }
-        else if (ctx->headers_sent) {
+    if (!ctx) {
+        ctx = f->ctx = apr_pcalloc(r->pool, sizeof(header_filter_ctx));
+    }
+    if (ctx->headers_sent) {
+        /* Eat body if response must not have one. */
+        if (r->header_only || r->status == HTTP_NO_CONTENT) {
             apr_brigade_cleanup(b);
-            return OK;
+            return APR_SUCCESS;
         }
     }
-
+    else if (!ctx->headers_error && !check_headers(r)) {
+        ctx->headers_error = 1;
+    }
     for (e = APR_BRIGADE_FIRST(b);
          e != APR_BRIGADE_SENTINEL(b);
          e = APR_BUCKET_NEXT(e))
     {
+        if (ctx->headers_error) {
+            if (APR_BUCKET_IS_EOS(e)) {
+                eos = 1;
+                break;
+            }
+            continue;
+        }
         if (AP_BUCKET_IS_ERROR(e) && !eb) {
             eb = e->data;
             continue;
@@ -1279,9 +1303,41 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f,
             return ap_pass_brigade(f->next, b);
         }
     }
-    if (eb) {
-        int status;
+    if (ctx->headers_error) {
+        if (!eos) {
+            /* Eat body until EOS */
+            apr_brigade_cleanup(b);
+            return APR_SUCCESS;
+        }
 
+        /* We may come back here from ap_die() below,
+         * so clear anything from this response.
+         */
+        ctx->headers_error = 0;
+        apr_table_clear(r->headers_out);
+        apr_table_clear(r->err_headers_out);
+
+        /* Don't recall ap_die() if we come back here (from its own internal
+         * redirect or error response), otherwise we can end up in infinite
+         * recursion; better fall through with 500, minimal headers and an
+         * empty body (EOS only).
+         */
+        if (!check_headers_recursion(r)) {
+            apr_brigade_cleanup(b);
+            ap_die(HTTP_INTERNAL_SERVER_ERROR, r);
+            return AP_FILTER_ERROR;
+        }
+        AP_DEBUG_ASSERT(APR_BUCKET_IS_EOS(e));
+        APR_BUCKET_REMOVE(e);
+        apr_brigade_cleanup(b);
+        APR_BRIGADE_INSERT_TAIL(b, e);
+        r->status = HTTP_INTERNAL_SERVER_ERROR;
+        r->content_type = r->content_encoding = NULL;
+        r->content_languages = NULL;
+        ap_set_content_length(r, 0);
+    }
+    else if (eb) {
+        int status;
         status = eb->status;
         apr_brigade_cleanup(b);
         ap_die(status, r);
@@ -1304,11 +1360,6 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f,
                                            r->headers_out);
     }
 
-    if (!check_headers(r)) {
-        ap_die(HTTP_INTERNAL_SERVER_ERROR, r);
-        return AP_FILTER_ERROR;
-    }
-
     /*
      * Remove the 'Vary' header field if the client can't handle it.
      * Since this will have nasty effects on HTTP/1.1 caches, force
@@ -1435,11 +1486,11 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f,
     terminate_header(b2);
 
     ap_pass_brigade(f->next, b2);
+    ctx->headers_sent = 1;
 
     if (r->header_only || r->status == HTTP_NO_CONTENT) {
         apr_brigade_cleanup(b);
-        ctx->headers_sent = 1;
-        return OK;
+        return APR_SUCCESS;
     }
 
     r->sent_bodyct = 1;         /* Whatever follows is real body stuff... */