Re: svn commit: r1904269 - in /httpd/httpd/trunk: changes-entries/ docs/manual/mod/ modules/http2/ test/modules/http2/

2022-10-19 Thread Ruediger Pluem



On 10/19/22 11:28 AM, Stefan Eissing via dev wrote:
> 
> 
>> Am 18.10.2022 um 21:20 schrieb Roy T. Fielding :
>>
>>> On Oct 6, 2022, at 2:17 AM, Stefan Eissing via dev  
>>> wrote:
>>>
 Am 05.10.2022 um 19:34 schrieb Stefan Eissing via dev 
 :



> Am 05.10.2022 um 18:48 schrieb Eric Covener :
>
> On Wed, Oct 5, 2022 at 12:44 PM Roy T. Fielding  wrote:
>>
>>> On Sep 26, 2022, at 5:29 AM, ic...@apache.org wrote:
>>>
>>> Author: icing
>>> Date: Mon Sep 26 12:29:47 2022
>>> New Revision: 1904269
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1904269=rev
>>> Log:
>>> *) mod_http2: new directive "H2HeaderStrictness" to control the 
>>> compliance
>>>  level of header checks as defined in the HTTP/2 RFCs. Default is 7540.
>>>  9113 activates the checks for forbidden leading/trailing whitespace in
>>>  field values (available from nghttp2 v1.50.0 on).
>>
>> I am not seeing why that should be optional. It adds overhead, but it 
>> reduces
>> variability (for HPACK) and might prevent some downstream 
>> vulnerabilities, IIRC.
>> Maybe an internal switch for testing with default on?
>>>
>>> To add some more detail:
>>>
>>> - rfc9113 ch 8.2.1 states: "A field value MUST NOT start or end with an 
>>> ASCII whitespace character"
>>> - nghttp2 v1.49.0 implemented that check, non-optional. Things broke.
>>> - nghttp2 v1.50.0 added to its API so that the application can control the 
>>> behaviour on request of Daniel Stenberg
>>> - I added "H2HeaderStrictness" (in trunk only for now) to steer that.
>>>
>>> This is not a normalization issue, it's a hard "MUST NOT" e.g. rejection of 
>>> the request/response carrying such a field. While I agree that there are 
>>> many advantages in having fields more strict, the way to get there is not 
>>> clear.
>>>
>>> I am totally in favour of avoiding "H2HeaderStrictness", but just enforcing 
>>> rfc9113 here will not do. What would our response be for people whose 
>>> legacy clients/fronted applications will no longer work?
>>
>> Well, normally, I don't have a problem with just breaking them.
>> They are broken. Someone can fix them.
>>
>> It isn't a normalization issue. Whitespace that magically appeared when h2
>> changed the framing of header field values immediately became a security
>> vulnerability for all downstream recipients of h2/h3 messages that use
>> generic HTTP (semantic) field values expecting that stuff to have
>> been excluded during parsing.
>>
>> IOW, it's a security hole and our code either fixes it or gets a CVE.
>> We MUST NOT forward the malformed data in fields. That is not an option.
>>
>> OTOH, how we deal with a request containing malformed data in fields
>> (after making it well-formed) is a SHOULD send 400, not a MUST.
>> If we want to be super nice to the folks shipping bad code (or running pen
>> testers) and have an option to strip the naughty bits out while forwarding
>> the message, that's fine with me as an optional directive. But that won't
>> help them interop with the rest of the Internet.
> 
> Speaking about our implementation, I just added tests to trunk. Configuring 
> 'Header add name "$value"' and making http/1.1 requests, curl sees the 
> returned headers as:
> 
> configured, returned
> ['123 ', '123 '],  # trailing space stays
> ['123\t', '123\t'],# trailing tab stays
> [' 123', '123'],   # leading space is stripped
> ['  123', '123'],  # leading spaces are stripped
> ['\t123', '123'],  # leading tab is stripped
> 
> Test case 'test_h1_007_02'. 
> 
> Linking nghttp2 v1.49.0 and newer, without further config, will RST_STREAM 
> cases 1 and 2 when using HTTP/2. While succeeding with HTTP/1.1. I would find 
> that highly confusing.

Thanks. Initially I thought it was about request headers, but the tests are 
about response headers. Does the "no leading/trailing
whitespace" rule apply to both?

> 
> I understand that in the definition of Core HTTP, leading/trailing whitespace 
> MUST NOT carry any semantics and SHOULD be avoided. If Apache httpd, in its 
> version-independent field handling, should decide to universally strip such 
> ws, that would be totally fine with me.
> 
> The question in implementing mod_http2 is when cases 1+2 should FAIL and when 
> we should serve them. If we have any other config I can derive that from, I'd 
> happily remove "H2HeaderStrictness" again.

As far as I understand Roy, we should deny by default but have a directive that 
allows us to strip leading/trailing whitespaces
from the header values and accept them then (provided they are ok otherwise).
But I would be also fine if we just silently strip the leading/trailing ws 
always.

Regards

RĂ¼diger



Re: svn commit: r1904269 - in /httpd/httpd/trunk: changes-entries/ docs/manual/mod/ modules/http2/ test/modules/http2/

2022-10-19 Thread Stefan Eissing via dev



> Am 18.10.2022 um 21:20 schrieb Roy T. Fielding :
> 
>> On Oct 6, 2022, at 2:17 AM, Stefan Eissing via dev  
>> wrote:
>> 
>>> Am 05.10.2022 um 19:34 schrieb Stefan Eissing via dev 
>>> :
>>> 
>>> 
>>> 
 Am 05.10.2022 um 18:48 schrieb Eric Covener :
 
 On Wed, Oct 5, 2022 at 12:44 PM Roy T. Fielding  wrote:
> 
>> On Sep 26, 2022, at 5:29 AM, ic...@apache.org wrote:
>> 
>> Author: icing
>> Date: Mon Sep 26 12:29:47 2022
>> New Revision: 1904269
>> 
>> URL: http://svn.apache.org/viewvc?rev=1904269=rev
>> Log:
>> *) mod_http2: new directive "H2HeaderStrictness" to control the 
>> compliance
>>  level of header checks as defined in the HTTP/2 RFCs. Default is 7540.
>>  9113 activates the checks for forbidden leading/trailing whitespace in
>>  field values (available from nghttp2 v1.50.0 on).
> 
> I am not seeing why that should be optional. It adds overhead, but it 
> reduces
> variability (for HPACK) and might prevent some downstream 
> vulnerabilities, IIRC.
> Maybe an internal switch for testing with default on?
>> 
>> To add some more detail:
>> 
>> - rfc9113 ch 8.2.1 states: "A field value MUST NOT start or end with an 
>> ASCII whitespace character"
>> - nghttp2 v1.49.0 implemented that check, non-optional. Things broke.
>> - nghttp2 v1.50.0 added to its API so that the application can control the 
>> behaviour on request of Daniel Stenberg
>> - I added "H2HeaderStrictness" (in trunk only for now) to steer that.
>> 
>> This is not a normalization issue, it's a hard "MUST NOT" e.g. rejection of 
>> the request/response carrying such a field. While I agree that there are 
>> many advantages in having fields more strict, the way to get there is not 
>> clear.
>> 
>> I am totally in favour of avoiding "H2HeaderStrictness", but just enforcing 
>> rfc9113 here will not do. What would our response be for people whose legacy 
>> clients/fronted applications will no longer work?
> 
> Well, normally, I don't have a problem with just breaking them.
> They are broken. Someone can fix them.
> 
> It isn't a normalization issue. Whitespace that magically appeared when h2
> changed the framing of header field values immediately became a security
> vulnerability for all downstream recipients of h2/h3 messages that use
> generic HTTP (semantic) field values expecting that stuff to have
> been excluded during parsing.
> 
> IOW, it's a security hole and our code either fixes it or gets a CVE.
> We MUST NOT forward the malformed data in fields. That is not an option.
> 
> OTOH, how we deal with a request containing malformed data in fields
> (after making it well-formed) is a SHOULD send 400, not a MUST.
> If we want to be super nice to the folks shipping bad code (or running pen
> testers) and have an option to strip the naughty bits out while forwarding
> the message, that's fine with me as an optional directive. But that won't
> help them interop with the rest of the Internet.

Speaking about our implementation, I just added tests to trunk. Configuring 
'Header add name "$value"' and making http/1.1 requests, curl sees the returned 
headers as:

configured, returned
['123 ', '123 '],  # trailing space stays
['123\t', '123\t'],# trailing tab stays
[' 123', '123'],   # leading space is stripped
['  123', '123'],  # leading spaces are stripped
['\t123', '123'],  # leading tab is stripped

Test case 'test_h1_007_02'. 

Linking nghttp2 v1.49.0 and newer, without further config, will RST_STREAM 
cases 1 and 2 when using HTTP/2. While succeeding with HTTP/1.1. I would find 
that highly confusing.

I understand that in the definition of Core HTTP, leading/trailing whitespace 
MUST NOT carry any semantics and SHOULD be avoided. If Apache httpd, in its 
version-independent field handling, should decide to universally strip such ws, 
that would be totally fine with me.

The question in implementing mod_http2 is when cases 1+2 should FAIL and when 
we should serve them. If we have any other config I can derive that from, I'd 
happily remove "H2HeaderStrictness" again.

Cheers,
Stefan


> Cheers,
> 
> Roy