LWS in H1 header values may be stripped out when proxying.

[antoniovicente]

The code to strip out whitespace at the beginning and end of H1 header values is a bit too aggressive. It strips out spaces at the beginning and end of each chunk provided by the H1 parser to ConnectionImpl::onHeaderValue. Those chunks roughly map to either buffer slices or readv boundaries.

Sample request that reproduces the issue:

GET / HTTP/1.1
host: foo.com
k: v <32kb of spaces> v

Proxied request:
GET / HTTP/1.1
host: foo.com
k: vv

[mattklein123]

cc @alyssawilk

[alyssawilk]

Now that http_parser is fixed, we should just bump that dependency and remove the in-process workaround. It'll fix this bug and be less fragile anyway.

[antoniovicente]

Do you know what change to nodejs/http-parser addresses this whitespace issue? I see several CVE related fixes in the parser, but none stands out as a change related to handling of whitespace. I'm going to try moving forward and see if tests for the CVE issue fail when I undo the changes to the codec.

[alyssawilk]

Offhand: fixed in nodejs release 2020-02-06 nodejs/security-wg#631?email_token=AELALPNIZISSNY4SMD44EQDREQHQ7A5CNFSM4KYRAG2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMZA4WQ#issuecomment-590483034

…

On Wed, Mar 11, 2020 at 1:42 PM antonio ***@***.***> wrote: Do you know what change to nodejs/http-parser addresses this whitespace issue? I see several CVE related fixes in the parser, but none stands out as a change related to handling of whitespace. I'm going to try moving forward and see if tests for the CVE issue fail when I undo the changes to the codec. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10270 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AELALPLX7LRNDZGSA53IF5LRG7EQ5ANCNFSM4LCRQOCQ> .

[antoniovicente]

I tried upgrading to http-parser 2.9.3 which is the latest release from Feb 6th, I see test failures if I remove the trim change in codec_impl

[antoniovicente]

The fix I see for trimming whitespace from header is on nodejs/node which uses a different H1 parser.

nodejs/node@fe39757

[antoniovicente]

I'm looking into this, feel free to assign to me for now.