Re: T of 2.4.24

[William A Rowe Jr]

On Dec 12, 2016 7:44 PM, "Daniel Ruggeri"  wrote:


On 12/12/2016 12:26 AM, William A Rowe Jr wrote:
> In spite of 34 registered project committee members, until other
> contributors come forward to participate in the security patch review
> process, we may simply have to declare all further efforts are currently
> on pause.

Does one have to be on PMC to review security patches? If not, can you
give me a general idea on volume? This would be something I think
$dayjob would be OK with me doing as part of keeping a shirt on my back
and roof over the childrens' heads ;-)


This is something our httpd security team has revisited a few times over
the past few years.

To be on *httpd* security list, we require a certain level of trust. In the
past, this was based on PMC membership. We have since tweaked things to
bring in proven committers who are not yet on the PMC.

Also, all ASF Members have access to private archived lists; this includes
any PMC private lists and security lists across the foundation.

In terms of volume, there are only a handful of security issues per year,
from none to a dozen, but many dozens of reports we have to evaluate and
filter. It often takes probing questions of the reporter to distinguish
their defect report from a vulnerablity, or to quantify and qualify the
exposure and risk.

The ASF-wide list is another beast, it is a massive spam trap, exceeding
dozens of garbage messages per day, to capture about a dozen legitimate
messages a day, and only a tiny handful of new inbound messages a day that
are dispatched to the appropriate PMC's team. That list does require ASF
Membership to volunteer because it has full visibility into most every
defect.

Re: T of 2.4.24

[Eric Covener]

On Mon, Dec 12, 2016 at 8:43 PM, Daniel Ruggeri  wrote:
>
> On 12/12/2016 12:26 AM, William A Rowe Jr wrote:
>> In spite of 34 registered project committee members, until other
>> contributors come forward to participate in the security patch review
>> process, we may simply have to declare all further efforts are currently
>> on pause.
>
> Does one have to be on PMC to review security patches? If not, can you
> give me a general idea on volume? This would be something I think
> $dayjob would be OK with me doing as part of keeping a shirt on my back
> and roof over the childrens' heads ;-)


Not necessary. Go ahead and subscribe to secur...@httpd.apache.org and
someone should approve.

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

Re: T of 2.4.24

[Daniel Ruggeri]

On 12/12/2016 12:26 AM, William A Rowe Jr wrote:
> In spite of 34 registered project committee members, until other 
> contributors come forward to participate in the security patch review 
> process, we may simply have to declare all further efforts are currently
> on pause.

Does one have to be on PMC to review security patches? If not, can you
give me a general idea on volume? This would be something I think
$dayjob would be OK with me doing as part of keeping a shirt on my back
and roof over the childrens' heads ;-)

-- 
Daniel Ruggeri

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

[William A Rowe Jr]

On Dec 12, 2016 6:07 PM, "Yann Ylavic"  wrote:

On Tue, Dec 13, 2016 at 12:17 AM, Jacob Champion 
wrote:
> On 12/12/2016 03:10 PM, William A Rowe Jr wrote:
>>
>> On Mon, Dec 12, 2016 at 5:00 PM, Jacob Champion > > wrote:
>>
>> On 12/12/2016 01:23 PM, Yann Ylavic wrote:
>>
>> On Mon, Dec 12, 2016 at 10:07 PM, Jacob Champion
>> > wrote:
>>
>>
>> What's the case where this catches recursion that the
>> previous logic in
>> r1773861 did not handle? I'm trying to write a test that
>> fails on r1773861
>> and succeeds on r1773865, but I haven't figured it out yet.
>>
>>
>> I think it's more r1773862 that fixes your test case.
>>
>>
>> To clarify: I can't reproduce any problems with r1773861 in the
>> first place, even with ErrorDocument. I agree that r1773862 (and
>> r1773865) work for me; I just don't know what makes them
>> functionally different. In my attempted test cases, I can't find any
>> case where the rr->pool used during the internal redirect differs
>> from the original r->pool.
>>
>> Can you send me a config snippet that reproduces the loop with
>> ErrorDocument? I'm not arguing against your followup patches; I just
>> want to make sure a correct test case gets into the suite. :D
>>
>>
>> Speaking of the test suite behavior, your mission had succeeded. My quad
>> core machine was pegged, X-Windows/Gnome unresponsive.
>>
>> Do we want to put such tests in the framework?
>
>
> I would say yes, definitely. Better for us to bring down a tester's
machine
> with a regression and fix the problem before it goes live, than to spare
the
> tester and end up shipping said regression.


Not worried about my machine. Hanging a build machine with no feedback
would be an issue

>> If any of our perl gurus
>> have a good suggestion to throttle the top limit of cpu/time consumed,
>> that would be a good enhancement to the framework.
>
>
> Agreed!

BSD::Resource's setrlimit(RLIMIT_CPU, soft, hard)?


If portable to Linux/Windows, then mission accomplished.

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

[Yann Ylavic]

On Tue, Dec 13, 2016 at 12:17 AM, Jacob Champion  wrote:
> On 12/12/2016 03:10 PM, William A Rowe Jr wrote:
>>
>> On Mon, Dec 12, 2016 at 5:00 PM, Jacob Champion > > wrote:
>>
>> On 12/12/2016 01:23 PM, Yann Ylavic wrote:
>>
>> On Mon, Dec 12, 2016 at 10:07 PM, Jacob Champion
>> > wrote:
>>
>>
>> What's the case where this catches recursion that the
>> previous logic in
>> r1773861 did not handle? I'm trying to write a test that
>> fails on r1773861
>> and succeeds on r1773865, but I haven't figured it out yet.
>>
>>
>> I think it's more r1773862 that fixes your test case.
>>
>>
>> To clarify: I can't reproduce any problems with r1773861 in the
>> first place, even with ErrorDocument. I agree that r1773862 (and
>> r1773865) work for me; I just don't know what makes them
>> functionally different. In my attempted test cases, I can't find any
>> case where the rr->pool used during the internal redirect differs
>> from the original r->pool.
>>
>> Can you send me a config snippet that reproduces the loop with
>> ErrorDocument? I'm not arguing against your followup patches; I just
>> want to make sure a correct test case gets into the suite. :D
>>
>>
>> Speaking of the test suite behavior, your mission had succeeded. My quad
>> core machine was pegged, X-Windows/Gnome unresponsive.
>>
>> Do we want to put such tests in the framework?
>
>
> I would say yes, definitely. Better for us to bring down a tester's machine
> with a regression and fix the problem before it goes live, than to spare the
> tester and end up shipping said regression.
>
>> If any of our perl gurus
>> have a good suggestion to throttle the top limit of cpu/time consumed,
>> that would be a good enhancement to the framework.
>
>
> Agreed!

BSD::Resource's setrlimit(RLIMIT_CPU, soft, hard)?

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

[Yann Ylavic]

On Tue, Dec 13, 2016 at 12:20 AM, Yann Ylavic  wrote:
>
> So I guess r1773861 is OK already, though the below may be simpler/cleaner 
> now:
>
> Index: modules/http/http_filters.c
> ===
> --- modules/http/http_filters.c(revision 1773865)
> +++ modules/http/http_filters.c(working copy)
> @@ -689,13 +689,10 @@ static APR_INLINE int check_headers(request_rec *r
>
>  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(, "check_headers_recursion", rr->pool);
> -if (dying) {
> -return 1;
> -}
> +void *dying = NULL;
> +apr_pool_userdata_get(, "check_headers_recursion", rr->pool);

s/rr/r/ here of course.

> +if (dying) {
> +return 1;
>  }
>  apr_pool_userdata_setn("true", "check_headers_recursion", NULL, r->pool);
>  return 0;
> _

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

[Jacob Champion]

On 12/12/2016 03:20 PM, Yann Ylavic wrote:

On Tue, Dec 13, 2016 at 12:00 AM, Jacob Champion  wrote:

On 12/12/2016 01:23 PM, Yann Ylavic wrote:


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



What's the case where this catches recursion that the previous logic in
r1773861 did not handle? I'm trying to write a test that fails on
r1773861
and succeeds on r1773865, but I haven't figured it out yet.



I think it's more r1773862 that fixes your test case.



To clarify: I can't reproduce any problems with r1773861 in the first place,
even with ErrorDocument. I agree that r1773862 (and r1773865) work for me; I
just don't know what makes them functionally different. In my attempted test
cases, I can't find any case where the rr->pool used during the internal
redirect differs from the original r->pool.


Oh, right, I thought internal-requests were like sub-requests, with
rr->pool being a subpool of r->pool, but that's not the case as you
mention.

So I guess r1773861 is OK already, though the below may be simpler/cleaner now:


Thanks for the clarification. I am all for simpler and cleaner, assuming 
it works the same. :D


--Jacob

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

[Yann Ylavic]

On Tue, Dec 13, 2016 at 12:00 AM, Jacob Champion  wrote:
> On 12/12/2016 01:23 PM, Yann Ylavic wrote:
>>
>> On Mon, Dec 12, 2016 at 10:07 PM, Jacob Champion 
>> wrote:
>>
>>>
>>> What's the case where this catches recursion that the previous logic in
>>> r1773861 did not handle? I'm trying to write a test that fails on
>>> r1773861
>>> and succeeds on r1773865, but I haven't figured it out yet.
>>
>>
>> I think it's more r1773862 that fixes your test case.
>
>
> To clarify: I can't reproduce any problems with r1773861 in the first place,
> even with ErrorDocument. I agree that r1773862 (and r1773865) work for me; I
> just don't know what makes them functionally different. In my attempted test
> cases, I can't find any case where the rr->pool used during the internal
> redirect differs from the original r->pool.

Oh, right, I thought internal-requests were like sub-requests, with
rr->pool being a subpool of r->pool, but that's not the case as you
mention.

So I guess r1773861 is OK already, though the below may be simpler/cleaner now:

Index: modules/http/http_filters.c
===
--- modules/http/http_filters.c(revision 1773865)
+++ modules/http/http_filters.c(working copy)
@@ -689,13 +689,10 @@ static APR_INLINE int check_headers(request_rec *r

 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(, "check_headers_recursion", rr->pool);
-if (dying) {
-return 1;
-}
+void *dying = NULL;
+apr_pool_userdata_get(, "check_headers_recursion", rr->pool);
+if (dying) {
+return 1;
 }
 apr_pool_userdata_setn("true", "check_headers_recursion", NULL, r->pool);
 return 0;
_

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

[Jacob Champion]

On 12/12/2016 03:10 PM, William A Rowe Jr wrote:

On Mon, Dec 12, 2016 at 5:00 PM, Jacob Champion > wrote:

On 12/12/2016 01:23 PM, Yann Ylavic wrote:

On Mon, Dec 12, 2016 at 10:07 PM, Jacob Champion
> wrote:


What's the case where this catches recursion that the
previous logic in
r1773861 did not handle? I'm trying to write a test that
fails on r1773861
and succeeds on r1773865, but I haven't figured it out yet.


I think it's more r1773862 that fixes your test case.


To clarify: I can't reproduce any problems with r1773861 in the
first place, even with ErrorDocument. I agree that r1773862 (and
r1773865) work for me; I just don't know what makes them
functionally different. In my attempted test cases, I can't find any
case where the rr->pool used during the internal redirect differs
from the original r->pool.

Can you send me a config snippet that reproduces the loop with
ErrorDocument? I'm not arguing against your followup patches; I just
want to make sure a correct test case gets into the suite. :D


Speaking of the test suite behavior, your mission had succeeded. My quad
core machine was pegged, X-Windows/Gnome unresponsive.

Do we want to put such tests in the framework?


I would say yes, definitely. Better for us to bring down a tester's 
machine with a regression and fix the problem before it goes live, than 
to spare the tester and end up shipping said regression.



If any of our perl gurus
have a good suggestion to throttle the top limit of cpu/time consumed,
that would be a good enhancement to the framework.


Agreed!

--Jacob

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

[William A Rowe Jr]

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

> On 12/12/2016 01:23 PM, Yann Ylavic wrote:
>
>> On Mon, Dec 12, 2016 at 10:07 PM, Jacob Champion 
>> wrote:
>>
>>
>>> What's the case where this catches recursion that the previous logic in
>>> r1773861 did not handle? I'm trying to write a test that fails on
>>> r1773861
>>> and succeeds on r1773865, but I haven't figured it out yet.
>>>
>>
>> I think it's more r1773862 that fixes your test case.
>>
>
> To clarify: I can't reproduce any problems with r1773861 in the first
> place, even with ErrorDocument. I agree that r1773862 (and r1773865) work
> for me; I just don't know what makes them functionally different. In my
> attempted test cases, I can't find any case where the rr->pool used during
> the internal redirect differs from the original r->pool.
>
> Can you send me a config snippet that reproduces the loop with
> ErrorDocument? I'm not arguing against your followup patches; I just want
> to make sure a correct test case gets into the suite. :D


Speaking of the test suite behavior, your mission had succeeded. My quad
core machine was pegged, X-Windows/Gnome unresponsive.

Do we want to put such tests in the framework? If any of our perl gurus
have a good suggestion to throttle the top limit of cpu/time consumed,
that would be a good enhancement to the framework.

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

[Jacob Champion]

On 12/12/2016 01:23 PM, Yann Ylavic wrote:

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



What's the case where this catches recursion that the previous logic in
r1773861 did not handle? I'm trying to write a test that fails on r1773861
and succeeds on r1773865, but I haven't figured it out yet.


I think it's more r1773862 that fixes your test case.


To clarify: I can't reproduce any problems with r1773861 in the first 
place, even with ErrorDocument. I agree that r1773862 (and r1773865) 
work for me; I just don't know what makes them functionally different. 
In my attempted test cases, I can't find any case where the rr->pool 
used during the internal redirect differs from the original r->pool.


Can you send me a config snippet that reproduces the loop with 
ErrorDocument? I'm not arguing against your followup patches; I just 
want to make sure a correct test case gets into the suite. :D



In r1773861, the recursion wouldn't work with "ErrorDocument
/some/file" for example, because ap_die() may either call
ap_intern_redirects() which is caught by ap_is_initial_req(), or
ap_send_error_response() directly which is caught by pool userdata
"dying".

r1773865 allows to not depend on ap_die() internals, and also allows
to try ap_die() (once) for internal redirects that
ap_http_header_filter() did not generated itself...

Regards,
Yann.

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

[William A Rowe Jr]

On Mon, Dec 12, 2016 at 3:32 PM, Yann Ylavic  wrote:

> On Mon, Dec 12, 2016 at 10:16 PM, William A Rowe Jr 
> wrote:
> > On Mon, Dec 12, 2016 at 3:07 PM, Jacob Champion 
> > wrote:+
> >>
> >>
> >> What's the case where this catches recursion that the previous logic in
> >> r1773861 did not handle? I'm trying to write a test that fails on
> r1773861
> >> and succeeds on r1773865, but I haven't figured it out yet.
> >
> >
> > I'm confused by a different aspect.
> >
> > In trashing the body-in-flight, whose headers caused us to 500-reject
> > the response, have we also trashed any and all correct error documents
> > or built-in short 500 response explanation?
>
> No, I tried (quite hard, in a second time) to honor ErrorDocument by
> calling ap_die() when check_headers() fails.
>
> That's only if/when that ErrorDocument is caught by check_headers that
> we end up generating a minimal 500 response (with Server, Date,
> Connection: close and empty body), to avoid infinite recursion.
>

I set up a test case of a constant text line and an html error doc, and can
confirm that everything is working as expected/hoped for.

Thank you for all the troubleshooting and coding on this effort!

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

[Yann Ylavic]

On Mon, Dec 12, 2016 at 10:16 PM, William A Rowe Jr  wrote:
> On Mon, Dec 12, 2016 at 3:07 PM, Jacob Champion 
> wrote:+
>>
>>
>> What's the case where this catches recursion that the previous logic in
>> r1773861 did not handle? I'm trying to write a test that fails on r1773861
>> and succeeds on r1773865, but I haven't figured it out yet.
>
>
> I'm confused by a different aspect.
>
> In trashing the body-in-flight, whose headers caused us to 500-reject
> the response, have we also trashed any and all correct error documents
> or built-in short 500 response explanation?

No, I tried (quite hard, in a second time) to honor ErrorDocument by
calling ap_die() when check_headers() fails.

That's only if/when that ErrorDocument is caught by check_headers that
we end up generating a minimal 500 response (with Server, Date,
Connection: close and empty body), to avoid infinite recursion.

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

[Yann Ylavic]

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

>
> What's the case where this catches recursion that the previous logic in
> r1773861 did not handle? I'm trying to write a test that fails on r1773861
> and succeeds on r1773865, but I haven't figured it out yet.

I think it's more r1773862 that fixes your test case.
In r1773861, the recursion wouldn't work with "ErrorDocument
/some/file" for example, because ap_die() may either call
ap_intern_redirects() which is caught by ap_is_initial_req(), or
ap_send_error_response() directly which is caught by pool userdata
"dying".

r1773865 allows to not depend on ap_die() internals, and also allows
to try ap_die() (once) for internal redirects that
ap_http_header_filter() did not generated itself...

Regards,
Yann.

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

[William A Rowe Jr]

On Mon, Dec 12, 2016 at 3:07 PM, Jacob Champion 
wrote:+
>
>
> What's the case where this catches recursion that the previous logic in
> r1773861 did not handle? I'm trying to write a test that fails on r1773861
> and succeeds on r1773865, but I haven't figured it out yet.


I'm confused by a different aspect.

In trashing the body-in-flight, whose headers caused us to 500-reject
the response, have we also trashed any and all correct error documents
or built-in short 500 response explanation? Unlike a 400-reject (which is
always caused by a badly implemented client, and should never arrive
from a conventional browser user-agent), the 500-reject speaks to some
module or backend that isn't under the requestor's control. An error doc
may be quite helpful in this situation.

I've verified that the bad headers in the current patch are redacted looking
at wireshark capture. Only one (first) bad header is logged, which is fine
IMO, no need for excessive error.log noise.

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

[Jacob Champion]

On 12/12/2016 12:31 PM, yla...@apache.org wrote:

Author: ylavic
Date: Mon Dec 12 20:31:44 2016
New Revision: 1773865

URL: http://svn.apache.org/viewvc?rev=1773865=rev
Log:
Follow up to r1773761: improved recursion detection.

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=1773865=1773864=1773865=diff
==
--- httpd/httpd/trunk/modules/http/http_filters.c (original)
+++ httpd/httpd/trunk/modules/http/http_filters.c Mon Dec 12 20:31:44 2016
@@ -687,6 +687,20 @@ static APR_INLINE int check_headers(requ
apr_table_do(check_header, , 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(, "check_headers_recursion", rr->pool);
+if (dying) {
+return 1;
+}
+}
+apr_pool_userdata_setn("true", "check_headers_recursion", NULL, r->pool);
+return 0;
+}
+


What's the case where this catches recursion that the previous logic in 
r1773861 did not handle? I'm trying to write a test that fails on 
r1773861 and succeeds on r1773865, but I haven't figured it out yet.


--Jacob

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: Making test framework and trunk approachable

[Jacob Champion]

On 12/12/2016 11:03 AM, William A Rowe Jr wrote:

On Dec 12, 2016 3:52 AM, "Petr Pisar" > wrote:
I think the inability to build httpd against system APR and to run test
against not yet installed httpd is quite surprising.


Big ++1 to this; I would be happy to provide dev cycles towards running 
tests inside the build tree, without installation, once 2.4.24 is out 
the door.


--Jacob

Making test framework and trunk approachable (was: PCRE 10 and puzzling edge cases)

[William A Rowe Jr]

On Dec 12, 2016 3:52 AM, "Petr Pisar"  wrote:


I think the inability to build httpd against system APR and to run test
against not yet installed httpd is quite surprising.


Thanks for taking the additional time to document your experiences!

System APR does require the -devel package. That RHEL package aught to
install the .m4 logic required by httpd, but it is on us to find that in a
conventional place.

Httpd trunk is already hunting for yet unreleased APR 1.6 features, but we
should better document that.

Not yet installed httpd is a little more complex, maybe we can add an
interim apxs in the build tree to facilitate this?

Working from my hardcopy of your list once our current release effort is
finished. Again, thanks.

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: Breakage on trunk head due to PCRE

[William A Rowe Jr]

On Dec 12, 2016 9:37 AM, "Jim Jagielski"  wrote:

> ./configure: line 6194: with_pcre: command not found
> checking for -pcre2-config... no
> checking for -pcre-config... no
> checking for pcre2-config... no
> checking for pcre-config... no
> configure: error: pcre(2)-config for libpcre not found. PCRE is required
> and available from http://pcre.org/
> make: *** No targets specified and no makefile found.  Stop.
>

Shell var assignment syntax error. Please re-./buildconf on trunk
once you svn up... sorry about that.

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?

Breakage on trunk head due to PCRE

[Jim Jagielski]

./configure: line 6194: with_pcre: command not found
checking for -pcre2-config... no
checking for -pcre-config... no
checking for pcre2-config... no
checking for pcre-config... no
configure: error: pcre(2)-config for libpcre not found. PCRE is required and 
available from http://pcre.org/
make: *** No targets specified and no makefile found.  Stop.

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: 2.6/3.0

[Jim Jagielski]

> On Dec 9, 2016, at 5:24 PM, William A Rowe Jr  wrote:
> 
>  
> Instead, maybe we could backport all that stuff to 2.4, in a backwards
> compatible fashion. That is, basically backport trunk to 2.4. This
> would give us more runway to work on httpd-nextgen.
> 
> That is very unrealistic, given that the trunk patches have broken
> ABI more than once a month for four years. Adapting such patches
> would lead to many more inane forks like the HttpProtocol Strict 
> backport effort.
> 

Well, instead of just throwing one's hands up and saying "That
is very unrealistic" I am sure that there are some among us
who instead are obstructionists, actually want to drive this
project further along and would, as I explicitly state, try to
figure out some way of doing so in a "backwards compatible
fashion."

Re: T of 2.4.24

[Jim Jagielski]

> On Dec 12, 2016, at 1:26 AM, William A Rowe Jr  wrote:
> 
> On Thu, Dec 8, 2016 at 8:55 AM, Jim Jagielski  wrote:
> Things are looking good for a T of 2.4.24 sometime late
> today.
> 
> If you have any issues or concerns, let me know asap.
> 
> Hi Jim,
> 
> we may have to concede, in light of many already partially disclosed
> CVE's, that it is impossible to proceed.
> 

Well, Bill, I'm sorry to say I disagree. I also think that
your characterization is wrong, incomplete and unwarranted.

See, the thing about releasing 2.4.24 is that it implies that
at some time, we can also release a 2.4.25.

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.

Re: PCRE 10 and puzzling edge cases

[Reindl Harald]

Am 12.12.2016 um 10:52 schrieb Petr Pisar:

I made sure I have installed all Perl modules I found relevant, but I was
unable to run the tests against SVN httpd sources. I played with
LD_LIBRARY_PATH, apxs etc. but without any good result. At the end
I reconfigured httpd sources and installed the binaries into a /tmp
subdirectory and that allowed me to run the tests. It's quite annoying to have
to install httpd into the system just only to test it


since your email is @redhat.com take a look at the Fedora php.spec and 
how the testsuite is called there from the rpm builddir *before* make 
install - same for mariadb/mysql - i guess httpd wouldn't bee that different

Re: PCRE 10 and puzzling edge cases

[Petr Pisar]

On Fri, Dec 09, 2016 at 01:07:54PM -0600, William A Rowe Jr wrote:
> 
> Thanks again, if there is anything that we didn't document well about
> getting the tests to run, we would like to fix that and make it easier for
> developers to cobble together their own test environment. We certainly don't
> want it to take hours for an experienced newcomer to get from point a to
> point b.
> 
First problem was how to generate ./configure from SVN sources. Obvious
autoreconf or autoconf did not work because of some missing macros. Then
I found ./buildconf.

But it still failed because of missing APR sources despite I had installed
APR 1.5.4 including header files and M4 macros from my distribution packages.
Then I found in the documentation that building from SVN requires APR sources.
So I cloned APR sources. Then I was able to build httpd.

I wanted to run test, but there are no tests you complained about a failure.
After searching web, I got an idea that httpd-framework is what I need.

I made sure I have installed all Perl modules I found relevant, but I was
unable to run the tests against SVN httpd sources. I played with
LD_LIBRARY_PATH, apxs etc. but without any good result. At the end
I reconfigured httpd sources and installed the binaries into a /tmp
subdirectory and that allowed me to run the tests. It's quite annoying to have
to install httpd into the system just only to test it.

But still the tests failed because of an undefined symbol in a session test
module. The symbol was defined in the httpd session module but the session
module was not named by the LoadModule keyword in the generated
t/conf/httpd.conf. I did not found a way how to disable the session tests
other than removing c-modules/test_session/mod_test_session.c file.

Finally I obtained a pass.

I think the inability to build httpd against system APR and to run test
against not yet installed httpd is quite surprising.

-- Petr


signature.asc
Description: PGP signature