Bug 61860 - Headers duplication when 416 status code occurs
Summary: Headers duplication when 416 status code occurs
Status: NEW
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: Core (show other bugs)
Version: 2.4.6
Hardware: PC Linux
: P2 minor (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL:
Keywords: FixedInTrunk
Depends on:
Blocks:
 
Reported: 2017-12-05 16:23 UTC by blaah
Modified: 2020-11-10 09:56 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description blaah 2017-12-05 16:23:25 UTC
Hi,

apache duplicates some specific headers when a 416 error occurs:


[root@S1 ~]# curl https://192.168.122.183/ -kv --header "Range: bytes=6549-"
* About to connect() to 192.168.122.183 port 443 (#0)
*   Trying 192.168.122.183...
* Connected to 192.168.122.183 (192.168.122.183) port 443 (#0)
* Initializing NSS with certpath: sql:/etc/pki/nssdb
* skipping SSL peer certificate verification
* SSL connection using TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
* Server certificate:
* 	subject: C=FR
* 	start date: déc. 05 12:18:20 2017 GMT
* 	expire date: déc. 05 12:18:20 2018 GMT
* 	common name: (nil)
* 	issuer: C=FR
> GET / HTTP/1.1
> User-Agent: curl/7.29.0
> Host: 192.168.122.183
> Accept: */*
> Range: bytes=6549-
> 
< HTTP/1.1 416 Requested Range Not Satisfiable
< Date: Tue, 05 Dec 2017 15:43:52 GMT
< Server: Apache/2.4.6 (CentOS) OpenSSL/1.0.2k-fips
< Public-Key-Pins-Report-Only: pin-sha256="Jh0ZSUYEc97HRYcwZIOkH2jKOUpmQhLO48MYd1s5pRM="; pin-sha256="2ZnCTNQBrKShr4c1olKfwNG53KiL6qoNcQi65YGRBn8="; pin-sha256="1D76xWwHug9p4iQWVBiDZF+e3UcxtPte/ig5pkYnmRU="; max-age=300; report-uri="https://protonmail.com/pkp-report"
< X-Frame-Options: deny
< X-Content-Type-Options: nosniff
< X-XSS-Protection: 1; mode=block
< Public-Key-Pins-Report-Only: pin-sha256="Jh0ZSUYEc97HRYcwZIOkH2jKOUpmQhLO48MYd1s5pRM="; pin-sha256="2ZnCTNQBrKShr4c1olKfwNG53KiL6qoNcQi65YGRBn8="; pin-sha256="1D76xWwHug9p4iQWVBiDZF+e3UcxtPte/ig5pkYnmRU="; max-age=300; report-uri="https://protonmail.com/pkp-report"
< X-Frame-Options: deny
< X-Content-Type-Options: nosniff
< X-XSS-Protection: 1; mode=block
< Transfer-Encoding: chunked
< Content-Type: text/html; charset=iso-8859-1
< 
<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
<html><head>
<title>416 Requested Range Not Satisfiable</title>
</head><body>
<h1>Requested Range Not Satisfiable</h1>
<p>None of the range-specifier values in the Range
request-header field overlap the current extent
of the selected resource.</p>
</body></html>
* Connection #0 to host 192.168.122.183 left intact
[root@S1 ~]# 



The expected behavior is to not get the headers duplicated, here's the configuration of the vhost:


<VirtualHost *:443>

	ServerName backend
	DocumentRoot /var/www/html/
	SSLEngine on
   	SSLProtocol all -SSLv2 -SSLv3
   	SSLCipherSuite HIGH:!aNULL:!MD5
   	SSLOptions +FakeBasicAuth +ExportCertData +StdEnvVars +OptRenegotiate
   	SSLCertificateFile /etc/ssl/apache2/cert.pem
   	SSLCertificateKeyFile /etc/ssl/apache2/key.pem

	    Header always set Public-Key-Pins-Report-Only "pin-sha256=\"Jh0ZSUYEc97HRYcwZIOkH2jKOUpmQhLO48MYd1s5pRM=\"; pin-sha256=\"2ZnCTNQBrKShr4c1olKfwNG53KiL6qoNcQi65YGRBn8=\"; pin-sha256=\"1D76xWwHug9p4iQWVBiDZF+e3UcxtPte/ig5pkYnmRU=\"; max-age=300;"
    Header always set X-Frame-Options "deny"
    Header always set X-Content-Type-Options "nosniff"
    Header always set X-XSS-Protection "1; mode=block"



	<Directory "/var/www/html">
		Options ExecCGI FollowSymLinks Includes
       AllowOverride All
       order deny,allow
       allow from all
	</Directory>


	ErrorLog /var/log/httpd/error_log
    CustomLog /var/log/httpd/access_log common
</VirtualHost>


I installed apache on CentOS 7 through yum install.
Comment 1 Luca Toscano 2018-04-02 17:18:55 UTC
Hi,

I can reproduce the issue easily, and the following quick hack seems working:

Index: modules/http/byterange_filter.c
===================================================================
--- modules/http/byterange_filter.c	(revision 1828144)
+++ modules/http/byterange_filter.c	(working copy)
@@ -389,6 +389,7 @@
     conn_rec *c = f->r->connection;
     ap_remove_output_filter(f);
     f->r->status = HTTP_OK;
+    apr_table_clear(f->r->err_headers_out);
     e = ap_bucket_error_create(HTTP_RANGE_NOT_SATISFIABLE, NULL,
                                f->r->pool, c->bucket_alloc);
     APR_BRIGADE_INSERT_TAIL(tmpbb, e);


My understanding is that the 'always' bit in 'Header' forces the header value to be placed in r->err_headers_out too, so when the 416 is forced the headers are duplicated (because both r->headers_out and r->headers_out_err seems to be added to the response). The above hack explicitly clears all the headers that are not supposed to be maintained even in error conditions, but a more generic approach might be needed.
Comment 2 Eric Covener 2018-04-02 17:47:14 UTC
> My understanding is that the 'always' bit in 'Header' forces the header
> value to be placed in r->err_headers_out too, so when the 416 is forced the
> headers are duplicated (because both r->headers_out and r->headers_out_err
> seems to be added to the response). The above hack explicitly clears all the
> headers that are not supposed to be maintained even in error conditions, but
> a more generic approach might be needed.

It could be related to this in http_filters.c (byterange_filter adds an error bucket to the output that will be seen here)

    else if (eb) {
        int status;
        status = eb->status;
        apr_brigade_cleanup(b);
        ap_die(status, r);
        return AP_FILTER_ERROR;
    }

The 416 is generated the first time the output filter are invoked, but after mod_headers has added the configured headers as an earlier filter.

I think the internal redirect for the error document adds them again.
Removing them from the table there might not make a difference.
Comment 3 Luca Toscano 2018-04-04 21:50:28 UTC
Eric the following bit in ap_send_error_respose (called after ap_die) puzzles me:

    if (!r->assbackwards) {
        apr_table_t *tmp = r->headers_out;

        /* For all HTTP/1.x responses for which we generate the message,
         * we need to avoid inheriting the "normal status" header fields
         * that may have been set by the request handler before the
         * error or redirect, except for Location on external redirects.
         */
        r->headers_out = r->err_headers_out;
        r->err_headers_out = tmp;
        apr_table_clear(r->err_headers_out);

If I comment everything but the apr_table_clear line I get no header duplication (but clearing err_headers_out causes more things like Content-Lenght, Accept-Ranges, etc.. to be included).

I am not getting what's happening in here, in my opinion a simple apr_table_clear(r->headers_out) would be sufficient to preserve only the headers that we need but I am surely missing something..
Comment 4 Luca Toscano 2018-04-05 05:59:30 UTC
Ok now I think I know what's happening (and I got what Eric was trying to suggest). One of the things that ap_send_error_response does is running ap_run_insert_error_filter, that allows modules to insert their filters (that will be executed). mod_headers does it, specifically it adds this bit:

/*
 * Make sure we propagate any "Header always" settings on the error
 * path through http_protocol.c.
 */
static apr_status_t ap_headers_error_filter(ap_filter_t *f,
                                            apr_bucket_brigade *in)

It makes sure that the Headers set via 'always' are in err_headers_out, to allow them to be added in the response. The issue in my opinion is that in ap_send_error_respose we swap r->headers_out with r->err_headers_out, and clear r->err_headers_out (that will be re-populated afterwards). Should we simply do:

Index: modules/http/http_protocol.c
===================================================================
--- modules/http/http_protocol.c        (revision 1828234)
+++ modules/http/http_protocol.c        (working copy)
@@ -1262,7 +1262,6 @@
     }

     if (!r->assbackwards) {
-        apr_table_t *tmp = r->headers_out;

         /* For all HTTP/1.x responses for which we generate the message,
          * we need to avoid inheriting the "normal status" header fields
@@ -1269,9 +1268,7 @@
          * that may have been set by the request handler before the
          * error or redirect, except for Location on external redirects.
          */
-        r->headers_out = r->err_headers_out;
-        r->err_headers_out = tmp;
-        apr_table_clear(r->err_headers_out);
+        apr_table_clear(r->headers_out);

         if (ap_is_HTTP_REDIRECT(status) || (status == HTTP_CREATED)) {
             if ((location != NULL) && *location) {
Comment 5 Luca Toscano 2018-05-13 14:11:02 UTC
The above patch seems to cause the following failures:

Test Summary Report
-------------------
t/modules/authz_core.t (Wstat: 0 Tests: 504 Failed: 25)
  Failed tests:  7, 178, 192, 206, 220, 234, 248, 262, 276
                290, 304, 318, 332, 346, 360, 374, 388
                402, 416, 430, 444, 458, 472, 486, 500

Still need to investigate why. I am going to add a specific test for the 416 case and try to find an alternative solution.
Comment 6 Luca Toscano 2018-05-13 14:32:39 UTC
Not true, I tested the wrong patch, this one seems to work just fine. Need to clear some existing errors in my runs of the test suite and then I'll report back.
Comment 7 Luca Toscano 2018-05-14 18:31:00 UTC
Added http://svn.apache.org/r1831585 to the test suite (one test with TODO).
Comment 8 Luca Toscano 2018-05-27 08:05:03 UTC
Fix available in trunk: http://svn.apache.org/r1832092