Re: svn commit: r1773293 - /httpd/httpd/trunk/modules/http/http_filters.c

[Jacob Champion]

On 12/08/2016 11:57 AM, cove...@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;
     }

     /*