> On Oct 19, 2022, at 4:50 AM, Ruediger Pluem <rpl...@apache.org> wrote: > > > > On 10/19/22 11:28 AM, Stefan Eissing via dev 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. > > 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.
I meant to say that we must strip by default, whereas the deny is a should, so that's fine with me too if we are working around breakage. I believe the downstream vulnerabilities are avoided by stripping, IIRC from that discussion, whereas the hard deny is more of a "cultural thing" for h2. ....Roy
