Re: HTTP/1.1 strict ruleset

[Christian Folini]

On Wed, Aug 03, 2016 at 06:58:26PM -0500, William A Rowe Jr wrote:
> > I see a lot of value in logging when not applying the strict parsing,
> > so you can passively assess your traffic for a day/week/month.
> 
> That requires additional CPU, and significantly more code complexity.
> In fact, I wonder whether such 'logging-only' behavior shouldn't simply
> be a no-choice default? I also wonder if those tools or others such as
> mod_security won't already provide such an option and we can wash
> our hands of this 'extra effort'?

ModSecurity Core Rules committer here.

As you know it's all in the rules with ModSecurity and the 
OWASP ModSecurity Core Rules (CRS) are the most widespread ruleset 
on the net.

We block per default, but all the checks can run log-only. 

They are listed in these rulefiles:
https://github.com/SpiderLabs/owasp-modsecurity-crs/blob/v3.0.0-rc1/rules/REQUEST-911-METHOD-ENFORCEMENT.conf
https://github.com/SpiderLabs/owasp-modsecurity-crs/blob/v3.0.0-rc1/rules/REQUEST-920-PROTOCOL-ENFORCEMENT.conf
https://github.com/SpiderLabs/owasp-modsecurity-crs/blob/v3.0.0-rc1/rules/REQUEST-921-PROTOCOL-ATTACK.conf

The default policy definitions: 
https://github.com/SpiderLabs/owasp-modsecurity-crs/blob/v3.0.0-rc1/modsecurity_crs_10_setup.conf.example

(Links are for the upcoming major release 3.0, RC1 will be out within
days now).

Overall, I think the rules are not overly aggressive. Apache has been
liberal so far and we try to avoid too many false positives due to
crazy clients and bad implementations. Missing Accept headers,
silly Range headers and numerical Host headers as frequent source
of false positives spring to mind.

Also, I think the coverage is not very systematic. Joining forces and
providing a systematic coverage for all aspects of RFC 2068 for 
CRS 3.1 would be very interesting for our project. If it would simplify
the httpd code base to refer users to ModSecurity and CRS, the
CRS could profit a lot from the endorsement (and the httpd-dev
experience brought to our rules resulting in a higher security level
overall).

A possible issue is the fact that ModSecurity runs fairly late in the
lifecycle. In fact, the default hook for the first ModSecurity rule
phase has been shifted backwards a few years ago. I take it a httpd
implementation of protocol enforcement rules would run immediately after
receiving the request line and then as the headers come in. ModSecurity
would definitely run later. However, there have been discussions to
introduce additional rule phase(s) into the ModSecurity engine / module
in the past and if there is a need from the Apache project, then the
development might be open in this regard (but it would certainly take
quite a while to get this out the door).

Cheers,

Christian Folini

-- 
https://www.feistyduck.com/training/modsecurity-training-course
mailto:christian.fol...@netnea.com
twitter: @ChrFolini

Re: HTTP/1.1 strict ruleset

[Eric Covener]

> Well, is there any point of detailing the specific offending field names?
> The bad text received? To the consumer in the response error message,
> or strictly to the logs?

I don't think it's too important in the response.

> We should log (once) the cause of a 400, but are the details interesting?
> Or is it enough to report that a bad field name, a bad header value etc
> caused the fault without any sprintf() style processing?

Kind of difficult to talk about in the abstract. I can't say the cause
needs to be "clear" in the logs but I think the offending data (line
if it's line-based even if we could already be lost for bad input) and
some hint about what we were expecting or didn't like.

Re: HTTP/1.1 strict ruleset

[William A Rowe Jr]

On Wed, Aug 3, 2016 at 6:51 PM, Eric Covener  wrote:

>
> > 2. leave the default as 'not-strict'? Seems we should most
> >strongly recommend that the server observe RFC's 2068,
> >2616 and 723x, and not tolerate ancient behavior by default
> >unless the admin insists on being foolish.
>
> I am with you on default-to-strict in 2.4 and up.  I'm hesitant about 2.2.
>

Since the project consensus is to support 2.2 through up to next
summer, and these issues pose interop concerns, I don't think that
we can separate 2.2 from 2.4 discussions at this level.

Re: HTTP/1.1 strict ruleset

[William A Rowe Jr]

On Wed, Aug 3, 2016 at 6:51 PM, Eric Covener  wrote:

> On Wed, Aug 3, 2016 at 7:33 PM, William A Rowe Jr 
> wrote:
> > So it seems pretty absurd we are coming back to this over
> > three years later, but is there any reason to preserve pre-RFC 2068
> > behaviors? I appreciate that Stefan was trying to avoid harming
> > existing deployment scenarios, but even as I'm about to propose
> > that we backport all of this to 2.4 and 2.2, I have several questions;
> >
> > 1. offer a logging-only option? Why? It seems like a simple
> >choice, follow the spec, or don't. If you want to see what's
> >going on, Wireshark, Fiddler and dozens of other tools let
> >you inspect the conversation.
>
> I see a lot of value in logging when not applying the strict parsing,
> so you can passively assess your traffic for a day/week/month.


That requires additional CPU, and significantly more code complexity.
In fact, I wonder whether such 'logging-only' behavior shouldn't simply
be a no-choice default? I also wonder if those tools or others such as
mod_security won't already provide such an option and we can wash
our hands of this 'extra effort'?

> 4. detail the error to the error log? Again, there are inspection
> >tools, but more importantly, no visual user-agent is going
> >to send this garbage, and automated requests are going
> >to discard the 400 response. Seems we can save a lot of
> >code throwing away the details that just don't help, and
> >are generally the product of abusive traffic.
>
> I don't have a good understanding of the savings or where the line
> would be drawn on depth, but i think it's important to log (or stash
> where it can be logged) a high level reason that the core/http_filters
> ditched a request based on syntax.
>

Well, is there any point of detailing the specific offending field names?
The bad text received? To the consumer in the response error message,
or strictly to the logs?

We should log (once) the cause of a 400, but are the details interesting?
Or is it enough to report that a bad field name, a bad header value etc
caused the fault without any sprintf() style processing?

Re: HTTP/1.1 strict ruleset

[Eric Covener]

On Wed, Aug 3, 2016 at 7:33 PM, William A Rowe Jr  wrote:
> So it seems pretty absurd we are coming back to this over
> three years later, but is there any reason to preserve pre-RFC 2068
> behaviors? I appreciate that Stefan was trying to avoid harming
> existing deployment scenarios, but even as I'm about to propose
> that we backport all of this to 2.4 and 2.2, I have several questions;
>
> 1. offer a logging-only option? Why? It seems like a simple
>choice, follow the spec, or don't. If you want to see what's
>going on, Wireshark, Fiddler and dozens of other tools let
>you inspect the conversation.

I see a lot of value in logging when not applying the strict parsing,
so you can passively assess your traffic for a day/week/month.

> 2. leave the default as 'not-strict'? Seems we should most
>strongly recommend that the server observe RFC's 2068,
>2616 and 723x, and not tolerate ancient behavior by default
>unless the admin insists on being foolish.

I am with you on default-to-strict in 2.4 and up.  I'm hesitant about 2.2.

> 3. retain these legacy faulty behaviors in httpd 2.next/3.0?
>Seems that once we agree on a backport, the ancient
>side of this logic should all just disappear from trunk.

I don't see a good reason to keep the behavior in current trunk, but
preserving the merge-ability sounds worthwhile.

>
> 4. detail the error to the error log? Again, there are inspection
>tools, but more importantly, no visual user-agent is going
>to send this garbage, and automated requests are going
>to discard the 400 response. Seems we can save a lot of
>code throwing away the details that just don't help, and
>are generally the product of abusive traffic.

I don't have a good understanding of the savings or where the line
would be drawn on depth, but i think it's important to log (or stash
where it can be logged) a high level reason that the core/http_filters
ditched a request based on syntax.

HTTP/1.1 strict ruleset

[William A Rowe Jr]

So it seems pretty absurd we are coming back to this over
three years later, but is there any reason to preserve pre-RFC 2068
behaviors? I appreciate that Stefan was trying to avoid harming
existing deployment scenarios, but even as I'm about to propose
that we backport all of this to 2.4 and 2.2, I have several questions;

1. offer a logging-only option? Why? It seems like a simple
   choice, follow the spec, or don't. If you want to see what's
   going on, Wireshark, Fiddler and dozens of other tools let
   you inspect the conversation.

2. leave the default as 'not-strict'? Seems we should most
   strongly recommend that the server observe RFC's 2068,
   2616 and 723x, and not tolerate ancient behavior by default
   unless the admin insists on being foolish.

3. retain these legacy faulty behaviors in httpd 2.next/3.0?
   Seems that once we agree on a backport, the ancient
   side of this logic should all just disappear from trunk.

4. detail the error to the error log? Again, there are inspection
   tools, but more importantly, no visual user-agent is going
   to send this garbage, and automated requests are going
   to discard the 400 response. Seems we can save a lot of
   code throwing away the details that just don't help, and
   are generally the product of abusive traffic.

Thoughts?


-

http://svn.apache.org/viewvc?view=revision=1426877
Author: sf
Date: Sun Dec 30 01:23:24 2012 UTC (3 years, 7 months ago)
Changed paths: 9
Log Message:
Add an option to enforce stricter HTTP conformance

This is a first stab, the checks will likely have to be revised.
For now, we check

 * if the request line contains control characters
 * if the request uri has fragment or username/password
 * that the request method is standard or registered with RegisterHttpMethod
 * that the request protocol is of the form HTTP/[1-9]+.[0-9]+,
   or missing for 0.9
 * if there is garbage in the request line after the protocol
 * if any request header contains control characters
 * if any request header has an empty name
 * for the host name in the URL or Host header:
   - if an IPv4 dotted decimal address: Reject octal or hex values, require
 exactly four parts
   - if a DNS host name: Reject non-alphanumeric characters besides '.' and
 '-'. As a side effect, this rejects multiple Host headers.
 * if any response header contains control characters
 * if any response header has an empty name
 * that the Location response header (if present) has a valid scheme and is
   absolute

If we have a host name both from the URL and the Host header, we replace the
Host header with the value from the URL to enforce RFC conformance.

There is a log-only mode, but the loglevels of the logged messages need some
thought/work. Currently, the  checks for incoming data log for 'core' and
the
checks for outgoing data log for 'http'. Maybe we need a way to configure
the
loglevels separately from the core/http loglevels.

Re: svn commit: r1755098 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/protocol.c

[William A Rowe Jr]

On Wed, Aug 3, 2016 at 4:58 PM, William A Rowe Jr 
wrote:

> On Wed, Aug 3, 2016 at 3:21 PM, Jacob Champion 
>> wrote:
>>
>>>
>>> I don't think this is an equivalent transformation. More logic below
>>> this case relies on the last_field NULL check, and I'm currently getting
>>> segfaults on trunk due to the strchr on line 907.
>>>
>>
>> You were correct, that was my oversight. It was easier to simply revert
> the
> work so that I'm not overlaying one whitespace change after another, since
> the fix causes everything to be re-indented anyways, so we are back to
> trunk of last evening before my patches this morning.
>

And I've reapplied the now-corrected logic. Thanks for your detailed and
careful review of these changes, please feel free to review this code again
offer any feedback.

We'll fix the obs-fold whitespace question next.

Cheers,

Bill

Re: svn commit: r1754548 - /httpd/httpd/trunk/server/protocol.c

[William A Rowe Jr]

On Wed, Aug 3, 2016 at 4:28 PM, William A Rowe Jr 
wrote:

> On Wed, Aug 3, 2016 at 1:53 PM, Roy T. Fielding  wrote:
>
>> > On Aug 3, 2016, at 11:44 AM, Jacob Champion 
>> wrote:
>> >
>> > On 07/31/2016 09:18 AM, William A Rowe Jr wrote:
>> >>> So all the trailing SP/HTAB are part of obs-fold IMHO.
>> >>> Should we replace all of them (plus the CRLF) with a single SP or with
>> >>> as many SP?
>> >>
>> >> Hmmm... Good point. Advancing over them in our HTTP_STRICT mode seems
>> >> best, if we have a consensus on this.
>> >
>> > Agreed that we should process all the obs-fold whitespace, and not just
>> one byte.
>> >
>> > Replacing each byte with a separate space (as opposed to condensing
>> into a single space) *might* help prevent adversaries from playing games
>> with header length checks in more complicated/layered systems. That's
>> probably a stretch though. And if we consume the CRLF in a different layer
>> of logic, adding on two spaces just to keep everything "consistent" may
>> also be a stretch. I'm not feeling strongly either way.
>>
>> What the spec is trying to say is that we can either replace all those
>> bytes
>> with a single SP (semantically speaking they are the same) or we we can
>> replace
>> them all with a sequence of SP (still the same, but doesn't require
>> splitting
>> or recomposing the buffer).
>>
>> > >> > So the obs-fold itself consists of CR LF [ SP | TAB ]
>> > >>
>> > >>obs-fold = CRLF 1*( SP / HTAB )
>> > >>
>> >
>> > Note that this section of the spec has Errata associated with it; I'm
>> reading through the conversation [1] and it's seeming like they *may* want
>> to treat OWS preceding the CRLF as part of the obs-fold as well. I don't
>> know what our position is on adopting pieces of Errata that have been Held
>> for Document Update.
>>
>> No, that is just an ABNF issue for matching purposes.  We don't use it.
>>
>
> So AIUI, the leading SP / TAB whitespace in a field is a no-op (usually
> represented by a single space by convention), and trailing whitespace
> in the field value is a no-op, all leading tabs/spaces (beyond one SP)
> in the obs-fold line is a no-op. Is there any reason to preserve trailing
> spaces before the obs-fold?
>
> If not, then stripping trailing whitespace from the line prior to obs-fold
> and
> eating all leading whitespace on the obs-fold line will result in a single
> SP
> character, which should be just fine unless spaces were significant within
> a quoted value. The only way for the client to preserve such significant
> spaces would be to place them after the opening quote before the obs-fold.
>

To ensure the patch is entirely correct on my 'next try', I'd like
clarification
here before resuming this code review and correction. If we know the spaces
and tabs around an obs-fold, both leading - and trailing, are all
collapsible
to a single SP, and the leading and trailing spaces around the field value
are all discarded, then the code and resulting patch gets simpler, not more
confusing.

Re: svn commit: r1755098 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/protocol.c

[William A Rowe Jr]

On Wed, Aug 3, 2016 at 4:29 PM, William A Rowe Jr 
wrote:

> On Wed, Aug 3, 2016 at 3:21 PM, Jacob Champion 
> wrote:
>
>> On 08/03/2016 09:46 AM, wr...@apache.org wrote:
>>
>>> Modified: httpd/httpd/trunk/server/protocol.c
>>> URL:
>>> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1755098=1755097=1755098=diff
>>>
>>> ==
>>> --- httpd/httpd/trunk/server/protocol.c (original)
>>> +++ httpd/httpd/trunk/server/protocol.c Wed Aug  3 16:46:20 2016
>>> @@ -835,8 +835,15 @@ AP_DECLARE(void) ap_get_mime_headers_cor
>>>  return;
>>>  }
>>>
>>> -if (last_field != NULL) {
>>> -if ((len > 0) && ((*field == '\t') || *field == ' ')) {
>>> +if ((len > 0) && ((*field == '\t') || *field == ' ')) {
>>> +if (last_field == NULL) {
>>> +r->status = HTTP_BAD_REQUEST;
>>> +ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
>>> APLOGNO(03442)
>>> +  "Line folding encounterd before first"
>>> +  " header line");
>>> +return;
>>> +}
>>> +
>>>
>>
>> I don't think this is an equivalent transformation. More logic below this
>> case relies on the last_field NULL check, and I'm currently getting
>> segfaults on trunk due to the strchr on line 907.
>>
>> The addition of the `== NULL` check also triggers a C90 compiler warning
>> for the combo declaration/assignment of fold_len.
>
>
> Thanks for the heads-up. Investigating.
>

You were correct, that was my oversight. It was easier to simply revert the
work so that I'm not overlaying one whitespace change after another, since
the fix causes everything to be re-indented anyways, so we are back to
trunk of last evening before my patches this morning.

Again, thank you for the feedback!

Cheers,

Bill

Re: svn commit: r1755098 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/protocol.c

[William A Rowe Jr]

On Wed, Aug 3, 2016 at 3:21 PM, Jacob Champion  wrote:

> On 08/03/2016 09:46 AM, wr...@apache.org wrote:
>
>> Modified: httpd/httpd/trunk/server/protocol.c
>> URL:
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1755098=1755097=1755098=diff
>>
>> ==
>> --- httpd/httpd/trunk/server/protocol.c (original)
>> +++ httpd/httpd/trunk/server/protocol.c Wed Aug  3 16:46:20 2016
>> @@ -835,8 +835,15 @@ AP_DECLARE(void) ap_get_mime_headers_cor
>>  return;
>>  }
>>
>> -if (last_field != NULL) {
>> -if ((len > 0) && ((*field == '\t') || *field == ' ')) {
>> +if ((len > 0) && ((*field == '\t') || *field == ' ')) {
>> +if (last_field == NULL) {
>> +r->status = HTTP_BAD_REQUEST;
>> +ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
>> APLOGNO(03442)
>> +  "Line folding encounterd before first"
>> +  " header line");
>> +return;
>> +}
>> +
>>
>
> I don't think this is an equivalent transformation. More logic below this
> case relies on the last_field NULL check, and I'm currently getting
> segfaults on trunk due to the strchr on line 907.
>
> The addition of the `== NULL` check also triggers a C90 compiler warning
> for the combo declaration/assignment of fold_len.


Thanks for the heads-up. Investigating.

Re: svn commit: r1754548 - /httpd/httpd/trunk/server/protocol.c

[William A Rowe Jr]

On Wed, Aug 3, 2016 at 1:53 PM, Roy T. Fielding  wrote:

> > On Aug 3, 2016, at 11:44 AM, Jacob Champion 
> wrote:
> >
> > On 07/31/2016 09:18 AM, William A Rowe Jr wrote:
> >>> So all the trailing SP/HTAB are part of obs-fold IMHO.
> >>> Should we replace all of them (plus the CRLF) with a single SP or with
> >>> as many SP?
> >>
> >> Hmmm... Good point. Advancing over them in our HTTP_STRICT mode seems
> >> best, if we have a consensus on this.
> >
> > Agreed that we should process all the obs-fold whitespace, and not just
> one byte.
> >
> > Replacing each byte with a separate space (as opposed to condensing into
> a single space) *might* help prevent adversaries from playing games with
> header length checks in more complicated/layered systems. That's probably a
> stretch though. And if we consume the CRLF in a different layer of logic,
> adding on two spaces just to keep everything "consistent" may also be a
> stretch. I'm not feeling strongly either way.
>
> What the spec is trying to say is that we can either replace all those
> bytes
> with a single SP (semantically speaking they are the same) or we we can
> replace
> them all with a sequence of SP (still the same, but doesn't require
> splitting
> or recomposing the buffer).
>
> > >> > So the obs-fold itself consists of CR LF [ SP | TAB ]
> > >>
> > >>obs-fold = CRLF 1*( SP / HTAB )
> > >>
> >
> > Note that this section of the spec has Errata associated with it; I'm
> reading through the conversation [1] and it's seeming like they *may* want
> to treat OWS preceding the CRLF as part of the obs-fold as well. I don't
> know what our position is on adopting pieces of Errata that have been Held
> for Document Update.
>
> No, that is just an ABNF issue for matching purposes.  We don't use it.
>

So AIUI, the leading SP / TAB whitespace in a field is a no-op (usually
represented by a single space by convention), and trailing whitespace
in the field value is a no-op, all leading tabs/spaces (beyond one SP)
in the obs-fold line is a no-op. Is there any reason to preserve trailing
spaces before the obs-fold?

If not, then stripping trailing whitespace from the line prior to obs-fold
and
eating all leading whitespace on the obs-fold line will result in a single
SP
character, which should be just fine unless spaces were significant within
a quoted value. The only way for the client to preserve such significant
spaces would be to place them after the opening quote before the obs-fold.

Re: svn commit: r1755098 - in /httpd/httpd/trunk: docs/log-message-tags/next-number server/protocol.c

[Jacob Champion]

On 08/03/2016 09:46 AM, wr...@apache.org wrote:

Modified: httpd/httpd/trunk/server/protocol.c
URL: 
http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1755098=1755097=1755098=diff
==
--- httpd/httpd/trunk/server/protocol.c (original)
+++ httpd/httpd/trunk/server/protocol.c Wed Aug  3 16:46:20 2016
@@ -835,8 +835,15 @@ AP_DECLARE(void) ap_get_mime_headers_cor
 return;
 }

-if (last_field != NULL) {
-if ((len > 0) && ((*field == '\t') || *field == ' ')) {
+if ((len > 0) && ((*field == '\t') || *field == ' ')) {
+if (last_field == NULL) {
+r->status = HTTP_BAD_REQUEST;
+ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03442)
+  "Line folding encounterd before first"
+  " header line");
+return;
+}
+


I don't think this is an equivalent transformation. More logic below 
this case relies on the last_field NULL check, and I'm currently getting 
segfaults on trunk due to the strchr on line 907.


The addition of the `== NULL` check also triggers a C90 compiler warning 
for the combo declaration/assignment of fold_len.


--Jacob

Re: svn commit: r1754548 - /httpd/httpd/trunk/server/protocol.c

[Jacob Champion]

On 08/03/2016 12:05 PM, Jacob Champion wrote:

So if there is an HTAB directly *before* the obs-fold CRLF, we should
not try to replace with a SP?


Ad never mind here; looks like we already strip trailing OWS from 
the end of the line before the fold. That'll teach me to theorycraft 
before reading the code.


Thanks Roy for the clarifications. :)

--Jacob

Re: svn commit: r1754548 - /httpd/httpd/trunk/server/protocol.c

[Jacob Champion]

On 08/03/2016 12:05 PM, Jacob Champion wrote:

Right, I was just wondering out loud if condensing into a single space
could give anyone the chance to defeat a header length check in a
multi-layered system. It's admittedly a pretty "tinfoil hat" concern.


Never mind. It would need to be the other way around (expanding a header 
value rather than contracting) to cause a problem for a backend.


--Jacob

Re: svn commit: r1754548 - /httpd/httpd/trunk/server/protocol.c

[Jacob Champion]

On 08/03/2016 11:53 AM, Roy T. Fielding wrote:

Replacing each byte with a separate space (as opposed to condensing into a single space) 
*might* help prevent adversaries from playing games with header length checks in more 
complicated/layered systems. That's probably a stretch though. And if we consume the CRLF 
in a different layer of logic, adding on two spaces just to keep everything 
"consistent" may also be a stretch. I'm not feeling strongly either way.


What the spec is trying to say is that we can either replace all those bytes
with a single SP (semantically speaking they are the same) or we we can replace
them all with a sequence of SP (still the same, but doesn't require splitting
or recomposing the buffer).


Right, I was just wondering out loud if condensing into a single space 
could give anyone the chance to defeat a header length check in a 
multi-layered system. It's admittedly a pretty "tinfoil hat" concern.



So the obs-fold itself consists of CR LF [ SP | TAB ]


   obs-fold = CRLF 1*( SP / HTAB )



Note that this section of the spec has Errata associated with it; I'm reading 
through the conversation [1] and it's seeming like they *may* want to treat OWS 
preceding the CRLF as part of the obs-fold as well. I don't know what our 
position is on adopting pieces of Errata that have been Held for Document 
Update.


No, that is just an ABNF issue for matching purposes.  We don't use it.


So if there is an HTAB directly *before* the obs-fold CRLF, we should 
not try to replace with a SP?


--Jacob

Re: svn commit: r1754548 - /httpd/httpd/trunk/server/protocol.c

[Roy T. Fielding]

> On Aug 3, 2016, at 11:44 AM, Jacob Champion  wrote:
> 
> On 07/31/2016 09:18 AM, William A Rowe Jr wrote:
>>> So all the trailing SP/HTAB are part of obs-fold IMHO.
>>> Should we replace all of them (plus the CRLF) with a single SP or with
>>> as many SP?
>> 
>> Hmmm... Good point. Advancing over them in our HTTP_STRICT mode seems
>> best, if we have a consensus on this.
> 
> Agreed that we should process all the obs-fold whitespace, and not just one 
> byte.
> 
> Replacing each byte with a separate space (as opposed to condensing into a 
> single space) *might* help prevent adversaries from playing games with header 
> length checks in more complicated/layered systems. That's probably a stretch 
> though. And if we consume the CRLF in a different layer of logic, adding on 
> two spaces just to keep everything "consistent" may also be a stretch. I'm 
> not feeling strongly either way.

What the spec is trying to say is that we can either replace all those bytes
with a single SP (semantically speaking they are the same) or we we can replace
them all with a sequence of SP (still the same, but doesn't require splitting
or recomposing the buffer).

> >> > So the obs-fold itself consists of CR LF [ SP | TAB ]
> >>
> >>obs-fold = CRLF 1*( SP / HTAB )
> >>
> 
> Note that this section of the spec has Errata associated with it; I'm reading 
> through the conversation [1] and it's seeming like they *may* want to treat 
> OWS preceding the CRLF as part of the obs-fold as well. I don't know what our 
> position is on adopting pieces of Errata that have been Held for Document 
> Update.

No, that is just an ABNF issue for matching purposes.  We don't use it.

Roy

Re: svn commit: r1754548 - /httpd/httpd/trunk/server/protocol.c

[Jacob Champion]

On 07/31/2016 09:18 AM, William A Rowe Jr wrote:

So all the trailing SP/HTAB are part of obs-fold IMHO.
Should we replace all of them (plus the CRLF) with a single SP or with
as many SP?


Hmmm... Good point. Advancing over them in our HTTP_STRICT mode seems
best, if we have a consensus on this.


Agreed that we should process all the obs-fold whitespace, and not just 
one byte.


Replacing each byte with a separate space (as opposed to condensing into 
a single space) *might* help prevent adversaries from playing games with 
header length checks in more complicated/layered systems. That's 
probably a stretch though. And if we consume the CRLF in a different 
layer of logic, adding on two spaces just to keep everything 
"consistent" may also be a stretch. I'm not feeling strongly either way.


>> > So the obs-fold itself consists of CR LF [ SP | TAB ]
>>
>>obs-fold = CRLF 1*( SP / HTAB )
>>

Note that this section of the spec has Errata associated with it; I'm 
reading through the conversation [1] and it's seeming like they *may* 
want to treat OWS preceding the CRLF as part of the obs-fold as well. I 
don't know what our position is on adopting pieces of Errata that have 
been Held for Document Update.


--Jacob

[1] https://www.ietf.org/mail-archive/web/httpbisa/current/msg23721.html

Re: svn commit: r1753263 - in /httpd/httpd/trunk: include/httpd.h modules/http/http_protocol.c

[William A Rowe Jr]

On Wed, Aug 3, 2016 at 12:19 PM, William A Rowe Jr 
wrote:

> On Wed, Aug 3, 2016 at 8:13 AM, Graham Leggett  wrote:
>
>> On 18 Jul 2016, at 6:32 PM, William A Rowe Jr 
>> wrote:
>>
>> > Worse, in http 2.4, the first two registered methods collide with BREW
>> and WHEN. That said, the 'fix', if we wanted to resolve it, is to use
>> M_INVALID +3 as the first value.
>>
>> I’m not seeing the problem here, there is no expectation of binary
>> compatibility between httpd v2.4 and trunk. Can you confirm what you’re
>> trying to achieve by removing this?
>>
>
> The first problem in dropping BREW and WHEN is that any provider against
> 2.6/3.0 can register these methods itself. The ftp module, for example,
> registers some 50 commands. These exceed the number of AllowMethod
> limitable methods. The right answer for future-proofing would be to either
> have providers register only extension methods they want to use, and/or
> increase that from a 64-bit word to 128-bit index of allow methods (say
> 16 chars for simplicity's sake).
>
> The problem here is that BREW would collide with the first registered
> method,
> and WHEN would collide with the second registered method, wherein two
> method
> identifiers would share the same ID and same AllowMethod rule. That's bad
> (if these two were implemented by a provider).
>
> httpd 2.4 requires binary compatibility, if we don't want these to
> collide, we must
> assign the first registered method to 1+WHEN. The IDs can't change on
> 2.4.x,
> but the first number allocated for a registered method certainly can.
>
> In all, this was a cleanup-[de?]optimization of convoluted trunk logic, I
> don't see
> the point in backporting such noise to a maintenance branch. Only trunk,
> moving forwards, aught to be 'optimal' in terms of legibility and
> performance.
>
> So it wasn't my intent to backport. If we want to avoid BREW/WHEN
> collisions
> we should simply fix that.
>

Never mind, BREW and WHEN were simply not registered at all in 2.4/2.2.

I am going to suggest we register the hash lookup value for HEAD though as
a one-line patch to 2.2/2.4, aliased to M_GET just as on trunk.

Re: svn commit: r1753263 - in /httpd/httpd/trunk: include/httpd.h modules/http/http_protocol.c

[William A Rowe Jr]

On Wed, Aug 3, 2016 at 8:13 AM, Graham Leggett  wrote:

> On 18 Jul 2016, at 6:32 PM, William A Rowe Jr  wrote:
>
> > Worse, in http 2.4, the first two registered methods collide with BREW
> and WHEN. That said, the 'fix', if we wanted to resolve it, is to use
> M_INVALID +3 as the first value.
>
> I’m not seeing the problem here, there is no expectation of binary
> compatibility between httpd v2.4 and trunk. Can you confirm what you’re
> trying to achieve by removing this?
>

The first problem in dropping BREW and WHEN is that any provider against
2.6/3.0 can register these methods itself. The ftp module, for example,
registers some 50 commands. These exceed the number of AllowMethod
limitable methods. The right answer for future-proofing would be to either
have providers register only extension methods they want to use, and/or
increase that from a 64-bit word to 128-bit index of allow methods (say
16 chars for simplicity's sake).

The problem here is that BREW would collide with the first registered
method,
and WHEN would collide with the second registered method, wherein two method
identifiers would share the same ID and same AllowMethod rule. That's bad
(if these two were implemented by a provider).

httpd 2.4 requires binary compatibility, if we don't want these to collide,
we must
assign the first registered method to 1+WHEN. The IDs can't change on 2.4.x,
but the first number allocated for a registered method certainly can.

In all, this was a cleanup-[de?]optimization of convoluted trunk logic, I
don't see
the point in backporting such noise to a maintenance branch. Only trunk,
moving forwards, aught to be 'optimal' in terms of legibility and
performance.

So it wasn't my intent to backport. If we want to avoid BREW/WHEN collisions
we should simply fix that.

Re: svn commit: r1753263 - in /httpd/httpd/trunk: include/httpd.h modules/http/http_protocol.c

[Graham Leggett]

On 18 Jul 2016, at 6:32 PM, William A Rowe Jr  wrote:

> Worse, in http 2.4, the first two registered methods collide with BREW and 
> WHEN. That said, the 'fix', if we wanted to resolve it, is to use M_INVALID 
> +3 as the first value.

I’m not seeing the problem here, there is no expectation of binary 
compatibility between httpd v2.4 and trunk. Can you confirm what you’re trying 
to achieve by removing this?

Regards,
Graham
—

Re: svn commit: r1754732 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c

[Luca Toscano]

2016-08-03 1:50 GMT+02:00 Yann Ylavic :

> On Tue, Aug 2, 2016 at 6:58 PM, Luca Toscano 
> wrote:
> >
> > 2016-08-02 17:54 GMT+02:00 Yann Ylavic :
> >>
> >> On Tue, Aug 2, 2016 at 5:05 PM, Yann Ylavic 
> wrote:
> >>
> >> Actually, unless we want to check/enforce that a Status 304 (as
> >> opposed to a 304 set by ap_meets_conditions) is not followed by a
> >> body, the correct behaviour is probably just to revert this commit
> >> (r1754732).
> >>
> >> We already ignore the body (when we ought to) until
> >> AP_FCGI_END_REQUEST, and I see no reason to close the connection
> >> underneath the backend if we turn a 200 to a 304, this connection can
> >> even be reused if all goes well.
> >
> > So basically keeping only http://svn.apache.org/r1752347 that avoids to
> > break before AP_FCGI_END_REQUEST ?
>
> Yes, I think it was the right fix already, thanks Luca.
>
> > The only downside that I see is that in
> > case a FCGI response turns up into a 304 (the 'status = 304' use case
> > mentioned earlier) then the HTTP headers are shipped to the external
> client
> > very quickly,
>
> Yes, but we also shouldn't close (even when not reusing) before having
> read the end or the backend may consider its response/transaction is
> incomplete (turning into 304 should be transparent on both sides).
>
>
> > but then some latency is added because httpd needs to read all
> > the bytes from the FCGI connection before completing the HTTP response
> (at
> > least this is what I have observed in my tests).
>
> There is no latency from the client (thanks to EOS), right?
> Or would we need a FLUSH?
>

I re-studied a bit the difference between EOS/EOR and reviewed my tests.
The only use case in which I see a big "delay" between headers and end of
request on the client side is when I use telnet, but not with other tools
like curl, so I have been probably fooled by it. As you mentioned below the
delay should not be visible to the external client but only to httpd (since
the thread will still be busy).


> But yes, the thread may still be busy after AP_FCGI_END_REQUEST
> (done), with a last call to get_data_full() before leaving.
>
> I think we should let this read for the next request, so how about:
>
> Index: modules/proxy/mod_proxy_fcgi.c
> ===
> --- modules/proxy/mod_proxy_fcgi.c(revision 1755008)
> +++ modules/proxy/mod_proxy_fcgi.c(working copy)
> @@ -445,7 +445,7 @@ static apr_status_t dispatch(proxy_conn_rec *conn,
>   int *bad_request, int *has_responded)
>  {
>  apr_bucket_brigade *ib, *ob;
> -int seen_end_of_headers = 0, done = 0, ignore_body = 0;
> +int seen_end_of_headers = 0, ignore_body = 0;
>  apr_status_t rv = APR_SUCCESS;
>  int script_error_status = HTTP_OK;
>  conn_rec *c = r->connection;
> @@ -472,7 +472,7 @@ static apr_status_t dispatch(proxy_conn_rec *conn,
>  ib = apr_brigade_create(r->pool, c->bucket_alloc);
>  ob = apr_brigade_create(r->pool, c->bucket_alloc);
>
> -while (! done) {
> +while (1) {
>  apr_interval_time_t timeout;
>  apr_size_t len;
>  int n;
> @@ -772,7 +772,7 @@ recv_again:
>  break;
>
>  case AP_FCGI_END_REQUEST:
> -done = 1;
> +/* we are done below */
>  break;
>
>  default:
> @@ -780,8 +780,8 @@ recv_again:
>"Got bogus record %d", type);
>  break;
>  }
> -/* Leave on above switch's inner error. */
> -if (rv != APR_SUCCESS) {
> +/* Leave on above switch's inner end/error. */
> +if (rv != APR_SUCCESS || type == AP_FCGI_END_REQUEST) {
>  break;
>  }
>
> ?
>
> The final EOR will do its work faster, and we'll be noticed of
> spurious data on next request (r1750474 series).
>

I tested the patch and it works fine in my testing environment, together
with r1750474 it could be a good improvement.

Thanks a lot for the long email thread and all the pointers!

Regards,

Luca

Re: svn commit: r1754916 - in /httpd/test/mod_h2/trunk: conf/sites/test.example.org.conf test/go/ test/go/get.go test/go/test_get.sh test/test.sh test/test_common.sh

[Stefan Eissing]

We discovered some incompatibilities when testing the current go RC against 
2.4.23. That's why I am adding go to the list of test clients. I would also 
like to add Python's hyper, given time. Maybe even headless browsers.

The reasons why it's important is that current tests rely mainly on curl and 
nghttp clients, both of which use libnghttp2. While this is good to test 
various conditions, true interop with other http2 implementation can only be 
achieved by running other implementations. ;-)

-Stefan

> Am 03.08.2016 um 00:19 schrieb Jacob Champion :
> 
> On 08/02/2016 06:42 AM, ic...@apache.org wrote:
>> 
>> start of go test client, changed h2/state url
> 
> Hmm, now I'm curious. nghttp2, last I saw, was also basing its tests in Go. 
> What are the advantages/disadvantages of Go for test writing? (I'm coming 
> from a primarily Python/Ruby test background, for the rich standard libraries 
> and easy glue code.)
> 
> --Jacob