> Am 20.10.2022 um 21:38 schrieb Roy T. Fielding <field...@gbiv.com>:
>
>> On Oct 19, 2022, at 2:28 AM, Stefan Eissing via dev <dev@httpd.apache.org>
>> wrote:
>>> Am 18.10.2022 um 21:20 schrieb Roy T. Fielding <field...@gbiv.com>:
>>>
>>>> On Oct 6, 2022, at 2:17 AM, Stefan Eissing via dev <dev@httpd.apache.org>
>>>> wrote:
>>>>
>>>>> Am 05.10.2022 um 19:34 schrieb Stefan Eissing via dev
>>>>> <dev@httpd.apache.org>:
>>>>>
>>>>>
>>>>>
>>>>>> Am 05.10.2022 um 18:48 schrieb Eric Covener <cove...@gmail.com>:
>>>>>>
>>>>>> On Wed, Oct 5, 2022 at 12:44 PM Roy T. Fielding <field...@gbiv.com>
>>>>>> 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&view=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.
>
> Well, it's a little more complicated than that. An HTTP/1.1 parser is required
> to exclude leading and trailing whitespace when extracting the field value.
> The Header directive, OTOH, is not an HTTP parser. It's just setting a value.
>
> I'd normally place that in the "Doctor, it hurts when I do this!" category,
> but
> we should be scanning that string at directive time anyway to ensure that
> the value is actually compliant and not hiding a smuggled response (which
> might allow one part of a site to corrupt a client cache for some other
> part's URIs... Umm, did you happen test "123\n 123"?).
Some trickery involved: "expr=%{unescape:123%0A 123}" gives a 500 with error
log:
...AH02430: Response header 'ap-test-007' value of '123\n 123' contains invalid
characters, aborting request
Also tested this via mod_proxy_http and the leading/trailing ws is stripped by
its parsing.
> In terms of compliance, the core HTTP sender should be excluding extra
> whitespace, but did not do so in the past because it was historically
> meaningless for HTTP/1.x recipients (because it gets parsed out upon
> receipt) and thus not worth the string scan. Since it isn't meaningless
> for h2 and h3, my old code (if it's still there) is probably doing the
> wrong thing now.
For consistency, we should strip leading/trailing ws in HTTP transcoding
(parsing and formatting) for all HTTP versions. We already scan field names and
values for invalid chars. This should not add much burden.
For H2, I'll remove the H2HeaderStrictness, tell nghttp2 not to complain and
strip field values before passing them on/out.
>
> ....Roy
