Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Subject: Re: svn commit: r1773293 -
 /httpd/httpd/trunk/modules/http/http_filters.c
To: dev@httpd.apache.org
References: <20161208195758.307DC3A0026@svn01-us-west.apache.org>
From: Jacob Champion <champion.p@gmail.com>
Message-ID: <b456a8f3-e062-e50c-b5b1-e5e7a652e7d0@gmail.com>
Date: Thu, 8 Dec 2016 13:55:49 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.5.1
MIME-Version: 1.0
In-Reply-To: <20161208195758.307DC3A0026@svn01-us-west.apache.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
archived-at: Thu, 08 Dec 2016 21:56:13 -0000

On 12/08/2016 11:57 AM, covener@apache.org wrote:
> Author: covener
> Date: Thu Dec  8 19:57:57 2016
> New Revision: 1773293
>
> URL: http://svn.apache.org/viewvc?rev=1773293&view=rev
> Log:
> 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)
>
>
>
> Modified:
>     httpd/httpd/trunk/modules/http/http_filters.c
>
> Modified: httpd/httpd/trunk/modules/http/http_filters.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=1773293&r1=1773292&r2=1773293&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/http/http_filters.c (original)
> +++ httpd/httpd/trunk/modules/http/http_filters.c Thu Dec  8 19:57:57 2016
> @@ -632,6 +632,7 @@ apr_status_t ap_http_filter(ap_filter_t
>  struct check_header_ctx {
>      request_rec *r;
>      int strict;
> +    const char *badheader;
>  };
>
>  /* check a single header, to be used with apr_table_do() */
> @@ -643,6 +644,7 @@ static int check_header(void *arg, const
>      if (name[0] == '\0') {
>          ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, ctx->r, APLOGNO(02428)
>                        "Empty response header name, aborting request");
> +        ctx->badheader = name;
>          return 0;
>      }
>
> @@ -657,6 +659,7 @@ static int check_header(void *arg, const
>                        "Response header name '%s' contains invalid "
>                        "characters, aborting request",
>                        name);
> +        ctx->badheader = name;
>          return 0;
>      }
>
> @@ -666,6 +669,7 @@ static int check_header(void *arg, const
>                        "Response header '%s' value of '%s' contains invalid "
>                        "characters, aborting request",
>                        name, val);
> +        ctx->badheader = name;
>          return 0;
>      }
>      return 1;
> @@ -680,13 +684,21 @@ static APR_INLINE int check_headers(requ
>      struct check_header_ctx ctx;
>      core_server_config *conf =
>              ap_get_core_module_config(r->server->module_config);
> +    int rv = 1;
>
>      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() */
> +    ctx.badheader = NULL;
>
> -    return 1;
> +    while (!apr_table_do(check_header, &ctx, r->headers_out, NULL)){
> +       if (ctx.badheader) {
> +           apr_table_unset(r->headers_out, ctx.badheader);
> +           apr_table_unset(r->err_headers_out, ctx.badheader);

Hmm, I suppose that if something like Transfer-/Content-Encoding is one 
of the headers to get axed here, we'll end up breaking the protocol 
entirely if and when we push out the original response body...

Ugh, this is tough.

> +       }
> +       rv = 0; /* problem has been logged by check_header() */
> +    }
> +
> +    return rv;
>  }
>
>  typedef struct header_struct {
> @@ -1249,8 +1261,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>      }
>
>      if (!check_headers(r)) {
> -        ap_die(HTTP_INTERNAL_SERVER_ERROR, r);
> -        return AP_FILTER_ERROR;
> +        r->status = 500;
>      }
>
>      /*
>
>