Commit 00b51ea7 authored by Jim Jagielski's avatar Jim Jagielski
Browse files

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
parent 574b6520

CHANGES

+7 −3
Original line number Diff line number Diff line
@@ -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]

STATUS

+0 −15
Original line number Diff line number Diff line
@@ -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

modules/http/http_filters.c

+70 −19
Original line number Diff line number Diff line
@@ -738,11 +738,23 @@ 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);

}



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 {

    apr_pool_t *pool;
@@ -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->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... */