Mailing List Archive

Re: svn commit: r1904269 - in /httpd/httpd/trunk: changes-entries/ docs/manual/mod/ modules/http2/ test/modules/http2/
On 9/26/22 2:29 PM, icing@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).
>
> - source sync with github version
> - fix for keepalive idle wait in mpm_worker setup
> - ensuring EOS when secondary connection has been handled
> - fixed race in late input EOS arrival when stream was
> already scheduled for execution.
>
>
> Added:
> httpd/httpd/trunk/changes-entries/h2_header_strictness.txt
> Modified:
> httpd/httpd/trunk/docs/manual/mod/mod_http2.xml
> httpd/httpd/trunk/modules/http2/config2.m4
> httpd/httpd/trunk/modules/http2/h2.h
> httpd/httpd/trunk/modules/http2/h2_bucket_beam.c
> httpd/httpd/trunk/modules/http2/h2_bucket_beam.h
> httpd/httpd/trunk/modules/http2/h2_c1.c
> httpd/httpd/trunk/modules/http2/h2_c1_io.c
> httpd/httpd/trunk/modules/http2/h2_c2.c
> httpd/httpd/trunk/modules/http2/h2_config.c
> httpd/httpd/trunk/modules/http2/h2_config.h
> httpd/httpd/trunk/modules/http2/h2_mplx.c
> httpd/httpd/trunk/modules/http2/h2_mplx.h
> httpd/httpd/trunk/modules/http2/h2_request.c
> httpd/httpd/trunk/modules/http2/h2_session.c
> httpd/httpd/trunk/modules/http2/h2_stream.c
> httpd/httpd/trunk/modules/http2/h2_util.c
> httpd/httpd/trunk/modules/http2/h2_version.h
> httpd/httpd/trunk/modules/http2/h2_workers.c
> httpd/httpd/trunk/modules/http2/h2_workers.h
> httpd/httpd/trunk/test/modules/http2/test_105_timeout.py
>

> Modified: httpd/httpd/trunk/modules/http2/h2_workers.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_workers.c?rev=1904269&r1=1904268&r2=1904269&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/http2/h2_workers.c (original)
> +++ httpd/httpd/trunk/modules/http2/h2_workers.c Mon Sep 26 12:29:47 2022

> @@ -451,7 +452,7 @@ h2_workers *h2_workers_create(server_rec
> workers->pool = pool;
> workers->min_active = min_active;
> workers->max_slots = max_slots;
> - workers->idle_limit = (idle_limit > 0)? idle_limit : apr_time_from_sec(10);
> + workers->idle_limit = (int)((idle_limit > 0)? idle_limit : apr_time_from_sec(10));

Is it possible to change idle_limit in the workers struct to an apr_time_t? Otherwise the cast could truncate here in an
unexpected way as users are not limited in what they can configure for H2MaxWorkerIdleSeconds.
I am also not sure if a cast of a large value could result in a negative value.

> workers->dynamic = (workers->min_active < workers->max_slots);
>
> ap_log_error(APLOG_MARK, APLOG_INFO, 0, s,

Regards

Rüdiger
Re: svn commit: r1904269 - in /httpd/httpd/trunk: changes-entries/ docs/manual/mod/ modules/http2/ test/modules/http2/ [ In reply to ]
> Am 27.09.2022 um 08:49 schrieb Ruediger Pluem <rpluem@apache.org>:
>
>
>
> On 9/26/22 2:29 PM, icing@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).
>>
>> - source sync with github version
>> - fix for keepalive idle wait in mpm_worker setup
>> - ensuring EOS when secondary connection has been handled
>> - fixed race in late input EOS arrival when stream was
>> already scheduled for execution.
>>
>>
>> Added:
>> httpd/httpd/trunk/changes-entries/h2_header_strictness.txt
>> Modified:
>> httpd/httpd/trunk/docs/manual/mod/mod_http2.xml
>> httpd/httpd/trunk/modules/http2/config2.m4
>> httpd/httpd/trunk/modules/http2/h2.h
>> httpd/httpd/trunk/modules/http2/h2_bucket_beam.c
>> httpd/httpd/trunk/modules/http2/h2_bucket_beam.h
>> httpd/httpd/trunk/modules/http2/h2_c1.c
>> httpd/httpd/trunk/modules/http2/h2_c1_io.c
>> httpd/httpd/trunk/modules/http2/h2_c2.c
>> httpd/httpd/trunk/modules/http2/h2_config.c
>> httpd/httpd/trunk/modules/http2/h2_config.h
>> httpd/httpd/trunk/modules/http2/h2_mplx.c
>> httpd/httpd/trunk/modules/http2/h2_mplx.h
>> httpd/httpd/trunk/modules/http2/h2_request.c
>> httpd/httpd/trunk/modules/http2/h2_session.c
>> httpd/httpd/trunk/modules/http2/h2_stream.c
>> httpd/httpd/trunk/modules/http2/h2_util.c
>> httpd/httpd/trunk/modules/http2/h2_version.h
>> httpd/httpd/trunk/modules/http2/h2_workers.c
>> httpd/httpd/trunk/modules/http2/h2_workers.h
>> httpd/httpd/trunk/test/modules/http2/test_105_timeout.py
>>
>
>> Modified: httpd/httpd/trunk/modules/http2/h2_workers.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_workers.c?rev=1904269&r1=1904268&r2=1904269&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/http2/h2_workers.c (original)
>> +++ httpd/httpd/trunk/modules/http2/h2_workers.c Mon Sep 26 12:29:47 2022
>
>> @@ -451,7 +452,7 @@ h2_workers *h2_workers_create(server_rec
>> workers->pool = pool;
>> workers->min_active = min_active;
>> workers->max_slots = max_slots;
>> - workers->idle_limit = (idle_limit > 0)? idle_limit : apr_time_from_sec(10);
>> + workers->idle_limit = (int)((idle_limit > 0)? idle_limit : apr_time_from_sec(10));
>
> Is it possible to change idle_limit in the workers struct to an apr_time_t? Otherwise the cast could truncate here in an
> unexpected way as users are not limited in what they can configure for H2MaxWorkerIdleSeconds.
> I am also not sure if a cast of a large value could result in a negative value.

You are correct. Changed the type to apr_time_t and no more casting in r1904299.

Thanks for reviewing.

/Stefan
>
>> workers->dynamic = (workers->min_active < workers->max_slots);
>>
>> ap_log_error(APLOG_MARK, APLOG_INFO, 0, s,
>
> Regards
>
> Rüdiger
Re: svn commit: r1904269 - in /httpd/httpd/trunk: changes-entries/ docs/manual/mod/ modules/http2/ test/modules/http2/ [ In reply to ]
> On Sep 26, 2022, at 5:29 AM, icing@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?

....Roy
Re: svn commit: r1904269 - in /httpd/httpd/trunk: changes-entries/ docs/manual/mod/ modules/http2/ test/modules/http2/ [ In reply to ]
On Wed, Oct 5, 2022 at 12:44 PM Roy T. Fielding <fielding@gbiv.com> wrote:
>
> > On Sep 26, 2022, at 5:29 AM, icing@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?

+1 for opt-out.
Re: svn commit: r1904269 - in /httpd/httpd/trunk: changes-entries/ docs/manual/mod/ modules/http2/ test/modules/http2/ [ In reply to ]
> Am 05.10.2022 um 18:48 schrieb Eric Covener <covener@gmail.com>:
>
> On Wed, Oct 5, 2022 at 12:44 PM Roy T. Fielding <fielding@gbiv.com> wrote:
>>
>>> On Sep 26, 2022, at 5:29 AM, icing@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?
>
> +1 for opt-out.

People downgraded nghttp2 v1.49 where this was on by default because of various interop problems.

I am all for strictness, but this improvement in the rfc seems to have thorns for users.

- Stefan
Re: svn commit: r1904269 - in /httpd/httpd/trunk: changes-entries/ docs/manual/mod/ modules/http2/ test/modules/http2/ [ In reply to ]
> 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 <covener@gmail.com>:
>>
>> On Wed, Oct 5, 2022 at 12:44 PM Roy T. Fielding <fielding@gbiv.com> wrote:
>>>
>>>> On Sep 26, 2022, at 5:29 AM, icing@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?

- Stefan

>>
>> +1 for opt-out.
>
> People downgraded nghttp2 v1.49 where this was on by default because of various interop problems.
>
> I am all for strictness, but this improvement in the rfc seems to have thorns for users.
>
> - Stefan
Re: svn commit: r1904269 - in /httpd/httpd/trunk: changes-entries/ docs/manual/mod/ modules/http2/ test/modules/http2/ [ In reply to ]
> 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 <covener@gmail.com>:
>>>
>>> On Wed, Oct 5, 2022 at 12:44 PM Roy T. Fielding <fielding@gbiv.com> wrote:
>>>>
>>>>> On Sep 26, 2022, at 5:29 AM, icing@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.

Cheers,

....Roy
Re: svn commit: r1904269 - in /httpd/httpd/trunk: changes-entries/ docs/manual/mod/ modules/http2/ test/modules/http2/ [ In reply to ]
> Am 18.10.2022 um 21:20 schrieb Roy T. Fielding <fielding@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 <covener@gmail.com>:
>>>>
>>>> On Wed, Oct 5, 2022 at 12:44 PM Roy T. Fielding <fielding@gbiv.com> wrote:
>>>>>
>>>>>> On Sep 26, 2022, at 5:29 AM, icing@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.

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
Re: svn commit: r1904269 - in /httpd/httpd/trunk: changes-entries/ docs/manual/mod/ modules/http2/ test/modules/http2/ [ In reply to ]
On 10/19/22 11:28 AM, Stefan Eissing via dev wrote:
>
>
>> Am 18.10.2022 um 21:20 schrieb Roy T. Fielding <fielding@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 <covener@gmail.com>:
>>>>>
>>>>> On Wed, Oct 5, 2022 at 12:44 PM Roy T. Fielding <fielding@gbiv.com> wrote:
>>>>>>
>>>>>>> On Sep 26, 2022, at 5:29 AM, icing@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.

Regards

Rüdiger
Re: svn commit: r1904269 - in /httpd/httpd/trunk: changes-entries/ docs/manual/mod/ modules/http2/ test/modules/http2/ [ In reply to ]
> On Oct 19, 2022, at 4:50 AM, Ruediger Pluem <rpluem@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 <fielding@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 <covener@gmail.com>:
>>>>>>
>>>>>> On Wed, Oct 5, 2022 at 12:44 PM Roy T. Fielding <fielding@gbiv.com> wrote:
>>>>>>>
>>>>>>>> On Sep 26, 2022, at 5:29 AM, icing@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
Re: svn commit: r1904269 - in /httpd/httpd/trunk: changes-entries/ docs/manual/mod/ modules/http2/ test/modules/http2/ [ In reply to ]
> 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 <fielding@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 <covener@gmail.com>:
>>>>>
>>>>> On Wed, Oct 5, 2022 at 12:44 PM Roy T. Fielding <fielding@gbiv.com> wrote:
>>>>>>
>>>>>>> On Sep 26, 2022, at 5:29 AM, icing@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"?).

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.

....Roy
Re: svn commit: r1904269 - in /httpd/httpd/trunk: changes-entries/ docs/manual/mod/ modules/http2/ test/modules/http2/ [ In reply to ]
> Am 20.10.2022 um 21:38 schrieb Roy T. Fielding <fielding@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 <fielding@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 <covener@gmail.com>:
>>>>>>
>>>>>> On Wed, Oct 5, 2022 at 12:44 PM Roy T. Fielding <fielding@gbiv.com> wrote:
>>>>>>>
>>>>>>>> On Sep 26, 2022, at 5:29 AM, icing@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