From: Jim Jagielski Date: Tue, 13 Dec 2016 12:46:30 +0000 (+0000) Subject: Merge r1773293, r1773761, r1773779, r1773812, r1773861, r1773862, r1773865 from trunk: X-Git-Tag: 2.4.24~33 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a8a0cca6facecaf3895f3e8dc9a741fd09b44879;p=thirdparty%2Fapache%2Fhttpd.git 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 ab18d3b4209..316a95aa740 100644 --- a/CHANGES +++ b/CHANGES @@ -13,15 +13,19 @@ Changes with Apache 2.4.24 [Dominic Scheirlinck , 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 , - Eric Covener, Jacob Champion] + mod_auth_digest: Prevent segfaults during client entry allocation when + the shared memory space is exhausted. + [Maksim Malyutin , 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 39667338e87..5971ba22ec8 100644 --- 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 de0b580ac5e..92ab36dd49f 100644 --- 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... */