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

[Eric Covener]

On Mon, Dec 12, 2016 at 2:45 PM, Yann Ylavic  wrote:
> I think it's addressed in r1773861 and r1773862, thanks all for testing.

Looks good, thank you Yann!

-- 
Eric Covener
cove...@gmail.com

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

[Yann Ylavic]

On Mon, Dec 12, 2016 at 7:17 PM, Jacob Champion  wrote:
> On 12/12/2016 08:18 AM, Yann Ylavic wrote:
>>
>> On Mon, Dec 12, 2016 at 3:32 PM, Eric Covener  wrote:
>>>
>>> On Mon, Dec 12, 2016 at 6:25 AM, Yann Ylavic 
>>> wrote:

 Tested with "Header set 'X-Bad' ''" with no redirect
 loop.
>>>
>>>
>>> w/ 'Header always set...' this still repeats
>>
>>
>> Right, with r1773812 it looks good.
>
>
> Even with r1773812, I'm still getting recursion with the `Header always set`
> case. ap_is_initial_req() is returning true during the ap_die() response.

The issue was that ap_die() calls ap_send_error_response() and not
ap_internal_redirect() when no ErrorDocument is configured (I was
blinded by my ErrorDocument test...).

I think it's addressed in r1773861 and r1773862, thanks all for testing.

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

[William A Rowe Jr]

On Mon, Dec 12, 2016 at 12:59 PM, Eric Covener  wrote:

> On Mon, Dec 12, 2016 at 1:54 PM, William A Rowe Jr 
> wrote:
> >> The problem seems to be that `Headers always set` negates the header
> >> removal, and the anti-recursion check doesn't seem to be working as
> >> intended.
> >
> >
> > By removal, I'm suggesting this should happen in the http output filter
> > just as we are about to transmit them.
> >
> > So the header will be set, then it would then be un-set, but my issue
> > is that I can't find the programatic pattern for apr_table_do to
> manipulate
> > the elts, and even if it exists, apr_table_do will quit once the first
> bad
> > elt
> > is found and the callback first returns 0, preventing us from reviewing
> the
> > remaining header lines.
>
> We can loop over either apr_table_do or check_headers while they're
> failing, as long as you are removing 1 header each time to make
> progress.
>

Dozens of good headers followed by dozens of bad headers sounds like
a DOS vector. Probably easier if we just iterate the list ourselves and
skip apr_table_do(), although this sounds like a good example for an
APR 1.next enhancement later on.

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

[Eric Covener]

On Mon, Dec 12, 2016 at 1:54 PM, William A Rowe Jr  wrote:
>> The problem seems to be that `Headers always set` negates the header
>> removal, and the anti-recursion check doesn't seem to be working as
>> intended.
>
>
> By removal, I'm suggesting this should happen in the http output filter
> just as we are about to transmit them.
>
> So the header will be set, then it would then be un-set, but my issue
> is that I can't find the programatic pattern for apr_table_do to manipulate
> the elts, and even if it exists, apr_table_do will quit once the first bad
> elt
> is found and the callback first returns 0, preventing us from reviewing the
> remaining header lines.

We can loop over either apr_table_do or check_headers while they're
failing, as long as you are removing 1 header each time to make
progress.

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

[Jacob Champion]

On 12/12/2016 10:38 AM, William A Rowe Jr wrote:

Looks like the right approach, tweaking the test framework to exercise
this solution. Thx!


I've added a regression test to the suite for this.

--Jacob

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

[William A Rowe Jr]

On Mon, Dec 12, 2016 at 12:43 PM, Jacob Champion 
wrote:

> On 12/12/2016 10:40 AM, William A Rowe Jr wrote:
>
>> I expect we still must have a header unset in the 500 case for all invalid
>> header names or values.
>>
>
> The problem seems to be that `Headers always set` negates the header
> removal, and the anti-recursion check doesn't seem to be working as
> intended.


By removal, I'm suggesting this should happen in the http output filter
just as we are about to transmit them.

So the header will be set, then it would then be un-set, but my issue
is that I can't find the programatic pattern for apr_table_do to manipulate
the elts, and even if it exists, apr_table_do will quit once the first bad
elt
is found and the callback first returns 0, preventing us from reviewing the
remaining header lines.

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

[Jacob Champion]

On 12/12/2016 10:40 AM, William A Rowe Jr wrote:

I expect we still must have a header unset in the 500 case for all invalid
header names or values.


The problem seems to be that `Headers always set` negates the header 
removal, and the anti-recursion check doesn't seem to be working as 
intended.


--Jacob

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

[William A Rowe Jr]

On Mon, Dec 12, 2016 at 12:17 PM, Jacob Champion 
wrote:

> On 12/12/2016 08:18 AM, Yann Ylavic wrote:
>
>> On Mon, Dec 12, 2016 at 3:32 PM, Eric Covener  wrote:
>>
>>> On Mon, Dec 12, 2016 at 6:25 AM, Yann Ylavic 
>>> wrote:
>>>
 Tested with "Header set 'X-Bad' ''" with no redirect
 loop.

>>>
>>> w/ 'Header always set...' this still repeats
>>>
>>
>> Right, with r1773812 it looks good.
>>
>
> Even with r1773812, I'm still getting recursion with the `Header always
> set` case. ap_is_initial_req() is returning true during the ap_die()
> response.
>
> Can anyone else confirm?


I expect we still must have a header unset in the 500 case for all invalid
header names or values.

A simple test in Strict mode is

Foo?Bar: Bash

A simple test in non-strict mode is

FooBar: Bash\a

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

[William A Rowe Jr]

On Mon, Dec 12, 2016 at 10:18 AM, Yann Ylavic  wrote:

> On Mon, Dec 12, 2016 at 3:32 PM, Eric Covener  wrote:
> > On Mon, Dec 12, 2016 at 6:25 AM, Yann Ylavic 
> wrote:
> >> Tested with "Header set 'X-Bad' ''" with no redirect
> loop.
> >
> > w/ 'Header always set...' this still repeats
>
> Right, with r1773812 it looks good.
> We'd only recurse if not in internal redirect already, otherwise fall
> through with:
> HTTP/1.1 500 Internal Server Error
> Date: Mon, 12 Dec 2016 16:11:59 GMT
> Server: Apache/2.5.0-dev (Unix) OpenSSL/1.0.2g
> Content-Length: 0
> Connection: close
>
> WDYT?
>

Looks like the right approach, tweaking the test framework to exercise
this solution. Thx!

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

[Eric Covener]

On Mon, Dec 12, 2016 at 11:18 AM, Yann Ylavic  wrote:
> On Mon, Dec 12, 2016 at 3:32 PM, Eric Covener  wrote:
>> On Mon, Dec 12, 2016 at 6:25 AM, Yann Ylavic  wrote:
>>> Tested with "Header set 'X-Bad' ''" with no redirect 
>>> loop.
>>
>> w/ 'Header always set...' this still repeats
>
> Right, with r1773812 it looks good.
> We'd only recurse if not in internal redirect already, otherwise fall
> through with:
> HTTP/1.1 500 Internal Server Error
> Date: Mon, 12 Dec 2016 16:11:59 GMT
> Server: Apache/2.5.0-dev (Unix) OpenSSL/1.0.2g
> Content-Length: 0
> Connection: close
>
> WDYT?

Still failing my accidental test. Will send you a separate note.

#519 0x0048af51 in ap_http_header_filter (f=0x7fffc80061d0,
b=0x7fffc85e9ad0) at http_filters.c:1257
#520 0x004396d0 in ap_pass_brigade (next=0x7fffc80061d0,
bb=0x7fffc85e9ad0) at util_filter.c:610
#521 0x004408a9 in ap_content_length_filter (f=0x7fffc8006198,
b=0x7fffc85e9ad0) at protocol.c:1776
#522 0x004396d0 in ap_pass_brigade (next=0x7fffc8006198,
bb=0x7fffc85e9ad0) at util_filter.c:610
#523 0x7fffe838daa5 in ap_headers_error_filter (f=0x7fffc85e9850,
in=0x7fffc85e9ad0) at mod_headers.c:917
#524 0x004396d0 in ap_pass_brigade (next=0x7fffc85e9850,
bb=0x7fffc85e9ad0) at util_filter.c:610
#525 0x7fffe6103dfc in session_output_filter (f=0x7fffc85e9818,
in=0x7fffc85e9ad0) at mod_session.c:489
#526 0x004396d0 in ap_pass_brigade (next=0x7fffc85e9818,
bb=0x7fffc85e9ad0) at util_filter.c:610
#527 0x00440b5d in ap_old_write_filter (f=0x7fffc85e98d8,
bb=0x7fffc85e9ad0) at protocol.c:1845
#528 0x004396d0 in ap_pass_brigade (next=0x7fffc85e98d8,
bb=0x7fffc85e9ad0) at util_filter.c:610
#529 0x004400a6 in end_output_stream (r=0x7fffc8004980) at
protocol.c:1540
#530 0x0044011e in ap_finalize_request_protocol
(r=0x7fffc8004980) at protocol.c:1565
#531 0x00486611 in ap_send_error_response (r=0x7fffc8004980,
recursive_error=500) at http_protocol.c:1395
#532 0x00486da7 in ap_die_r (type=500, r=0x7fffc8004980,
recursive_error=500) at http_request.c:224
#533 0x00486dd3 in ap_die (type=500, r=0x7fffc8004980) at
http_request.c:229
#534 0x0048af51 in ap_http_header_filter (f=0x7fffc80061d0,
b=0x7fffc85e7790) at http_filters.c:1257
#535 0x004396d0 in ap_pass_brigade (next=0x7fffc80061d0,
bb=0x7fffc85e7790) at util_filter.c:610
#536 0x004408a9 in ap_content_length_filter (f=0x7fffc8006198,
b=0x7fffc85e7790) at protocol.c:1776
#537 0x004396d0 in ap_pass_brigade (next=0x7fffc8006198,
bb=0x7fffc85e7790) at util_filter.c:610
#538 0x7fffe838daa5 in ap_headers_error_filter (f=0x7fffc85ed568,
in=0x7fffc85e7790) at mod_headers.c:917
#539 0x004396d0 in ap_pass_brigade (next=0x7fffc85ed568,
bb=0x7fffc85e7790) at util_filter.c:610
#540 0x7fffe6103dfc in session_output_filter (f=0x7fffc85ed530,
in=0x7fffc85e7790) at mod_session.c:489
#541 0x004396d0 in ap_pass_brigade (next=0x7fffc85ed530,
bb=0x7fffc85e7790) at util_filter.c:610
#542 0x00440b5d in ap_old_write_filter (f=0x7fffc85ed5f0,
bb=0x7fffc85e7790) at protocol.c:1845
#543 0x004396d0 in ap_pass_brigade (next=0x7fffc85ed5f0,
bb=0x7fffc85e7790) at util_filter.c:610
#544 0x004400a6 in end_output_stream (r=0x7fffc8004980) at
protocol.c:1540
#545 0x0044011e in ap_finalize_request_protocol
(r=0x7fffc8004980) at protocol.c:1565
#546 0x00486611 in ap_send_error_response (r=0x7fffc8004980,
recursive_error=500) at http_protocol.c:1395
#547 0x00486da7 in ap_die_r (type=500, r=0x7fffc8004980,
recursive_error=500) at http_request.c:224
#548 0x00486dd3 in ap_die (type=500, r=0x7fffc8004980) at
http_request.c:229
#549 0x0048af51 in ap_http_header_filter (f=0x7fffc80061d0,
b=0x7fffc85ed4e0) at http_filters.c:1257


-- 
Eric Covener
cove...@gmail.com

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

[Jim Jagielski]

Upon inspection it looks good... doing some further testing
but so far, +1

Thx!
> On Dec 12, 2016, at 11:18 AM, Yann Ylavic  wrote:
> 
> On Mon, Dec 12, 2016 at 3:32 PM, Eric Covener  wrote:
>> On Mon, Dec 12, 2016 at 6:25 AM, Yann Ylavic  wrote:
>>> Tested with "Header set 'X-Bad' ''" with no redirect 
>>> loop.
>> 
>> w/ 'Header always set...' this still repeats
> 
> Right, with r1773812 it looks good.
> We'd only recurse if not in internal redirect already, otherwise fall
> through with:
>HTTP/1.1 500 Internal Server Error
>Date: Mon, 12 Dec 2016 16:11:59 GMT
>Server: Apache/2.5.0-dev (Unix) OpenSSL/1.0.2g
>Content-Length: 0
>Connection: close
> 
> WDYT?

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

[Yann Ylavic]

On Mon, Dec 12, 2016 at 3:32 PM, Eric Covener  wrote:
> On Mon, Dec 12, 2016 at 6:25 AM, Yann Ylavic  wrote:
>> Tested with "Header set 'X-Bad' ''" with no redirect loop.
>
> w/ 'Header always set...' this still repeats

Right, with r1773812 it looks good.
We'd only recurse if not in internal redirect already, otherwise fall
through with:
HTTP/1.1 500 Internal Server Error
Date: Mon, 12 Dec 2016 16:11:59 GMT
Server: Apache/2.5.0-dev (Unix) OpenSSL/1.0.2g
Content-Length: 0
Connection: close

WDYT?

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

[Eric Covener]

On Mon, Dec 12, 2016 at 6:25 AM, Yann Ylavic  wrote:
> Tested with "Header set 'X-Bad' ''" with no redirect loop.

w/ 'Header always set...' this still repeats

#2010 0x00486399 in ap_send_error_response (r=0x7fffc8004980,
recursive_error=0) at http_protocol.c:1336
#2011 0x0048af39 in ap_http_header_filter (f=0x7fffc80061d0,
b=0x7fffc8367b58) at http_filters.c:1252
#2012 0x004396d0 in ap_pass_brigade (next=0x7fffc80061d0,
bb=0x7fffc8367b58) at util_filter.c:610
#2013 0x004408a9 in ap_content_length_filter
(f=0x7fffc8006198, b=0x7fffc8367b58) at protocol.c:1776
#2014 0x004396d0 in ap_pass_brigade (next=0x7fffc8006198,
bb=0x7fffc8367b58) at util_filter.c:610
#2015 0x7fffe838daa5 in ap_headers_error_filter (f=0x7fffc8367a50,
in=0x7fffc8367b58) at mod_headers.c:917
#2016 0x004396d0 in ap_pass_brigade (next=0x7fffc8367a50,
bb=0x7fffc8367b58) at util_filter.c:610
#2017 0x7fffe6103dfc in session_output_filter (f=0x7fffc8367a18,
in=0x7fffc8367b58) at mod_session.c:489
#2018 0x004396d0 in ap_pass_brigade (next=0x7fffc8367a18,
bb=0x7fffc8367b58) at util_filter.c:610
#2019 0x00440b5d in ap_old_write_filter (f=0x7fffc8367ae0,
bb=0x7fffc8367b58) at protocol.c:1845
#2020 0x004396d0 in ap_pass_brigade (next=0x7fffc8367ae0,
bb=0x7fffc8367b58) at util_filter.c:610
#2021 0x004400a6 in end_output_stream (r=0x7fffc8004980) at
protocol.c:1540
#2022 0x0044011e in ap_finalize_request_protocol
(r=0x7fffc8004980) at protocol.c:1565
#2023 0x00486399 in ap_send_error_response (r=0x7fffc8004980,
recursive_error=0) at http_protocol.c:1336
#2024 0x0048af39 in ap_http_header_filter (f=0x7fffc80061d0,
b=0x7fffc83679c8) at http_filters.c:1252


-- 
Eric Covener
cove...@gmail.com

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

[Yann Ylavic]

On Mon, Dec 12, 2016 at 11:26 AM,   wrote:
> Author: ylavic
> Date: Mon Dec 12 10:26:16 2016
> New Revision: 1773761
>
> URL: http://svn.apache.org/viewvc?rev=1773761=rev
> Log:
> Follow up to r1773293.
> When check_headers() fails, clear anything (headers and body) from 
> original/errorneous
> response before returning 500.

This returns an error 500 with its associated headers and body (not
the ones of the original response).

Tested with "Header set 'X-Bad' ''" with no redirect loop.


> +if (ctx->headers_error) {
> +/* We'll come back here from ap_send_error_response(),
> + * so clear anything from this response.
> + */
> +apr_brigade_cleanup(b);
> +if (!eos) {
> +return APR_SUCCESS;
> +}
> +ctx->headers_error = 0;
> +apr_table_clear(r->headers_out);
> +apr_table_clear(r->err_headers_out);
> +r->status = HTTP_INTERNAL_SERVER_ERROR;
> +ap_send_error_response(r, 0);

We could also use ap_die() here for ErrorDocument to work, in my
"Header set ..." case (with "ErrorDocument /error.html") there is
indeed a second loop because the internal-redirect's request has the
X-Bad header too, but the looping stops here thanks to the
recursive_error caught by ap_die() the second time around (no custom
response in this case).

Maybe Eric's case is another beast so I committed the safer
ap_send_error_response() first, but if it works for everyone I'm happy
to change to ap_die() instead...

Any objection to add this to STATUS (it addresses Bill's nack AFAICT)?

Regards,
Yann.