Re: svn commit: r1904269 - in /httpd/httpd/trunk: changes-entries/ docs/manual/mod/ modules/http2/ test/modules/http2/
> On Oct 19, 2022, at 2: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. > > 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"?). 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
Re: svn commit: r1904269 - in /httpd/httpd/trunk: changes-entries/ docs/manual/mod/ modules/http2/ test/modules/http2/
> On Oct 19, 2022, at 4:50 AM, Ruediger Pluem wrote: > > > > 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. I meant to say that we must strip by
Re: Define variable in modules
Le 20/10/2022 à 15:55, Nick Gearls a écrit : Hello, It would sometimes be very handy to be able to define a variable (like -D on command-line or "Define xxx" in the config) inside a module. This would, for instance, allow to have a config file based on a define from the module, knowing if the new syntax is known or not, etc. Concrete example: in mod_security2, they introduced a new "collection". If you use this collection in an old version, it's a syntax error. If mod_security2 defines the variable "support_new_collection" (OK, very bad name obviously), then we can enclose the rule using a ". Would it be possible to export a function to define such a variable? Thanks Hi, Maybe this would not be super-clean, but the module could define some (useless) directives (i.e. MOD_SEC2_HAS_NEW_COLLECTION_SUPPORT) and config files could be tweaked with . This should work as-is without any new code. Does it match your use-case? CJ
Re: Define variable in modules
On Thu, Oct 20, 2022 at 9:56 AM Nick Gearls wrote: > > Hello, > > It would sometimes be very handy to be able to define a variable (like -D on > command-line or "Define xxx" in the config) inside a module. > This would, for instance, allow to have a config file based on a define from > the module, knowing if the new syntax is known or not, etc. > > Concrete example: in mod_security2, they introduced a new "collection". > If you use this collection in an old version, it's a syntax error. > If mod_security2 defines the variable "support_new_collection" (OK, very bad > name obviously), then we can enclose the rule using a " support_new_collection>. > > Would it be possible to export a function to define such a variable? I think it's feasible. Might want to safeguard against it being called in unexpected times (after pre-config?) or ways (in child processes?)
Define variable in modules
Hello, It would sometimes be very handy to be able to define a variable (like -D on command-line or "Define xxx" in the config) inside a module. This would, for instance, allow to have a config file based on a define from the module, knowing if the new syntax is known or not, etc. Concrete example: in mod_security2, they introduced a new "collection". If you use this collection in an old version, it's a syntax error. If mod_security2 defines the variable "support_new_collection" (OK, very bad name obviously), then we can enclose the rule using a ". Would it be possible to export a function to define such a variable? Thanks
