Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why does headers-normalize-response.htm expect null bytes to be allowed? #165

Open
wisniewskit opened this issue Oct 16, 2017 · 21 comments
Open

Comments

@wisniewskit
Copy link

Per spec (first line is from the XHR spec; others are from Fetch spec which it links to):

A request has an associated header list (a header list). Unless stated otherwise it is empty.
A header list consists of zero or more headers.
A header consists of a name and value.
A value is a byte sequence that matches the following conditions:

  • Has no leading or trailing HTTP whitespace bytes.
  • Contains no 0x00, 0x0A or 0x0D bytes.

Apparently only Firefox goes against the spec and supports null bytes for XMLHttpRequests (but not Fetch in general): https://wptdashboard.appspot.com/XMLHttpRequest/headers-normalize-response.htm

Should the tests be changed to reflect the spec, or the spec (and other browsers) updated to match Firefox?

@annevk
Copy link
Member

annevk commented Oct 16, 2017

Hmm, that's for setting header values. I'm not sure response header values were fully considered. Sigh.

Dropping the header completely makes some sense (the null results), but would require changes to Chrome as well.

cc @mnot @tyoshino @yutakahirano

@mnot
Copy link
Member

mnot commented Oct 19, 2017

FWIW, corresponding HTTP spec is field-value:
http://httpwg.org/specs/rfc7230.html#header.fields

VCHAR gets you 0x21-7E, field-content adds in 0x20 and 0x09, and obs-text gets you 0x80-FF.

obs-fold gets you 0x0A and 0x0D in certain patterns (but you can also add that they're automagically folded in, which probably makes the best sense).

That means HTTP doesn't like 0x00-09 and 0x0A-1F.

Anybody have data about how often they're seen in the wild?

@annevk
Copy link
Member

annevk commented Oct 19, 2017

@mnot see httpwg/http-core#19 and whatwg/fetch#332 for how the new field-value is broken. I suspect the same might go for other places where the HTTP revision started to ban C0 controls.

@yutakahirano
Copy link
Member

@davidben Probably you know more why Chrome behaves in the current way.

@davidben
Copy link

I don't know anything about this stuff, sorry. (Also not sure what the current behavior is to begin with... ask git log or mmenke? Dunno what his Github is.)

@wisniewskit
Copy link
Author

@RByers, is there anyone we could ping to help figure this Blink XHR quirk out?

@annevk
Copy link
Member

annevk commented Apr 11, 2018

@MattMenke2 @bencebeky any thoughts on this?

@bencebeky
Copy link

My gut feeling is that (1) if no other browsers support null bytes, then it is probably safe for Firefox to reject them too, and (2) it might be less error-prone if the requirements for headers for XMLHttpRequests were consistent with the requirements for other requests. Other that this, I do not have strong feelings, and am happy to change Chrome behavior if there is a compelling use case.

@annevk
Copy link
Member

annevk commented Apr 11, 2018

@bencebeky thanks for the quick reply! I think the main problem with Chrome's behavior at the moment is that it strips 0x00 from header values. It doesn't reject anything.

http://w3c-test.org/xhr/headers-normalize-response.htm was added in https://bugzilla.mozilla.org/show_bug.cgi?id=1277019 and apparently Safari also passes all tests.

From that bug it seems this was already tested in http://w3c-test.org/cors/allow-headers.htm which reveals some minor security implications. (Disregarding the response or response header would also work for that specific case.)

The simplest solution based on this is probably accepting that response header values can contain 0x00 (at least when going over the network, no need to add support for this to Response objects and such). Rejecting headers or responses that have headers whose values contain 0x00 seems more involved (as it would require changing all clients), but doable if there's enough willingness.

(I also looked at Edge and it simply seems to hang on loading the page, which doesn't seem great. @travisleithead you might want to look at that.)

@bencebeky
Copy link

I understand. Indeed stripping characters seems fragile.

What is the plan with the Fetch spec? Is it going to be changed to allow null characters in header values? Or changed to require that such header fields are ignored?

@annevk
Copy link
Member

annevk commented Apr 11, 2018

@bencebeky well, I tried to solicit a plan with the above comment. I think the easiest would be to allow response headers to contain 0x00, but never include them in request headers and never allow 0x00 to be set through the API. @jakearchibald or would that be problematic for service workers if you wanted to forward a response that included 0x00 in header values?

@jakearchibald
Copy link
Contributor

@annevk how would a request with a header like this show up in a service worker?

Rejecting headers or responses that have headers whose values contain 0x00 seems more involved

At what point would it be rejected, and what kind of rejection would it be?

@bencebeky
Copy link

It seems inconsistent to allow null characters in responses and not in requests. Why not stick to the current specification and disallow them in both? In any case, I agree that Chrome's current behavior is not optimal.

What is currently done with requests that have null bytes? Are the header fields ignored, or the whole request rejected with an error? If they are rejected, could responses be rejected with a similar error? If they are happily accepted, maybe the spec could reflect that.

Are you aware of a use case that would justify the need to accept headers with null characters?

@annevk
Copy link
Member

annevk commented Apr 11, 2018

@jakearchibald it would only be a response. (If we go with rejection, we'd probably have to treat it as a network error. Only dropping the header seems too dangerous.)

@bencebeky there's no way that I know of whereby you can set a request header value that contains a 0x00 from anywhere within the web platform. API-wise there's no way for response header values either, but I had forgotten to test this scenario (whereby the server sets it).

@bencebeky
Copy link

Okay, so it seems like it makes sense to disallow null bytes in request headers. For consistency, it would make sense to do the same in response headers, both XHR and otherwise. I agree that network error would be the best choice, otherwise there would remain server deployments that send null bytes and we would be stuck with having to support them forever.

@annevk
Copy link
Member

annevk commented Apr 11, 2018

Okay, I definitely support trying that out. I'll create a PR for web-platform-tests changes, will file bugs against Edge, Firefox, and Safari, and report back. I take it you're willing to take care of Chrome?

@bencebeky
Copy link

Yes, I'll take care of Chrome. Thank you for driving this!

annevk added a commit to web-platform-tests/wpt that referenced this issue Apr 11, 2018
As discussed in whatwg/xhr#165 these should turn the response into a network error.
@MattMenke2
Copy link

MattMenke2 commented Apr 11, 2018 via email

@bencebeky
Copy link

Didn't find bug against Chromium, so just filed https://crbug.com/832086.

annevk added a commit to web-platform-tests/wpt that referenced this issue Aug 28, 2018
As discussed in whatwg/xhr#165 these should turn the response into a network error.
@annevk
Copy link
Member

annevk commented Aug 28, 2018

The tests finally landed. It seems no implementation has been updated so far. I guess the one thing that remains here is clarify somewhere that if an HTTP response header value contains a 0x00 a network error is to be returned?

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Aug 31, 2018
…ntaining 0x00, a=testonly

Automatic update from web-platform-testsFetch/XHR: response header value containing 0x00

As discussed in whatwg/xhr#165 these should turn the response into a network error.
--

wpt-commits: 3d172bc612e03a896b5611d9e9652f860995feef
wpt-pr: 10424
jankeromnes pushed a commit to jankeromnes/gecko that referenced this issue Aug 31, 2018
…ntaining 0x00, a=testonly

Automatic update from web-platform-testsFetch/XHR: response header value containing 0x00

As discussed in whatwg/xhr#165 these should turn the response into a network error.
--

wpt-commits: 3d172bc612e03a896b5611d9e9652f860995feef
wpt-pr: 10424
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 22, 2018
While the HTTP spec further limits what values are legal, nulls are
particularly concerning, and it's safest just to reject them. See
discussion here: whatwg/xhr#165

Chrome will be the first browser to reject nulls in responses, despite
there being wpt tests for this, so we'll have to keep an eye out for
breakages.

For reference, 0x00 through 0x1F aren't allowed in header values or
fields, (https://tools.ietf.org/html/rfc7230#section-3.2 - VCHAR
excludes those characters).  CRs and LFs are of course needed, and
0x0C and 0x0B are allowed by other specs for particular
header parsers, strangely.

This CL does not affect other code that can generate HTTP response
headers, which still uses the old behavior of just removing nulls.
ServiceWorkers, extensions, WebPackages, Dial (?), and various tests
still inherit the old behavior, since they create headers directly
with a method that can't fail.  It does introduce a new helper method,
however, that they should eventually be switched to use:
HttpResponseHeaders::TryToCreate().  We should probably put off
conversion until this successfully makes it to stable.

Bug: 832086
Change-Id: Ib75ac03a6a298238cafb41eaa5f046c082fd0bdf
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 22, 2018
While the HTTP spec further limits what values are legal, nulls are
particularly concerning, and it's safest just to reject them. See
discussion here: whatwg/xhr#165

Chrome will be the first browser to reject nulls in responses, despite
there being wpt tests for this, so we'll have to keep an eye out for
breakages.

For reference, 0x00 through 0x1F aren't allowed in header values or
fields, (https://tools.ietf.org/html/rfc7230#section-3.2 - VCHAR
excludes those characters).  CRs and LFs are of course needed, and
0x0C and 0x0B are allowed by other specs for particular
header parsers, strangely.

This CL does not affect other code that can generate HTTP response
headers, which still uses the old behavior of just removing nulls.
ServiceWorkers, extensions, WebPackages, Dial (?), and various tests
still inherit the old behavior, since they create headers directly
with a method that can't fail.  It does introduce a new helper method,
however, that they should eventually be switched to use:
HttpResponseHeaders::TryToCreate().  We should probably put off
conversion until this successfully makes it to stable.

Bug: 832086
Change-Id: Ib75ac03a6a298238cafb41eaa5f046c082fd0bdf
Reviewed-on: https://chromium-review.googlesource.com/c/1291812
Reviewed-by: Asanka Herath <[email protected]>
Commit-Queue: Matt Menke <[email protected]>
Cr-Commit-Position: refs/heads/master@{#601776}
aarongable pushed a commit to chromium/chromium that referenced this issue Oct 22, 2018
While the HTTP spec further limits what values are legal, nulls are
particularly concerning, and it's safest just to reject them. See
discussion here: whatwg/xhr#165

Chrome will be the first browser to reject nulls in responses, despite
there being wpt tests for this, so we'll have to keep an eye out for
breakages.

For reference, 0x00 through 0x1F aren't allowed in header values or
fields, (https://tools.ietf.org/html/rfc7230#section-3.2 - VCHAR
excludes those characters).  CRs and LFs are of course needed, and
0x0C and 0x0B are allowed by other specs for particular
header parsers, strangely.

This CL does not affect other code that can generate HTTP response
headers, which still uses the old behavior of just removing nulls.
ServiceWorkers, extensions, WebPackages, Dial (?), and various tests
still inherit the old behavior, since they create headers directly
with a method that can't fail.  It does introduce a new helper method,
however, that they should eventually be switched to use:
HttpResponseHeaders::TryToCreate().  We should probably put off
conversion until this successfully makes it to stable.

Bug: 832086
Change-Id: Ib75ac03a6a298238cafb41eaa5f046c082fd0bdf
Reviewed-on: https://chromium-review.googlesource.com/c/1291812
Reviewed-by: Asanka Herath <[email protected]>
Commit-Queue: Matt Menke <[email protected]>
Cr-Commit-Position: refs/heads/master@{#601776}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 22, 2018
While the HTTP spec further limits what values are legal, nulls are
particularly concerning, and it's safest just to reject them. See
discussion here: whatwg/xhr#165

Chrome will be the first browser to reject nulls in responses, despite
there being wpt tests for this, so we'll have to keep an eye out for
breakages.

For reference, 0x00 through 0x1F aren't allowed in header values or
fields, (https://tools.ietf.org/html/rfc7230#section-3.2 - VCHAR
excludes those characters).  CRs and LFs are of course needed, and
0x0C and 0x0B are allowed by other specs for particular
header parsers, strangely.

This CL does not affect other code that can generate HTTP response
headers, which still uses the old behavior of just removing nulls.
ServiceWorkers, extensions, WebPackages, Dial (?), and various tests
still inherit the old behavior, since they create headers directly
with a method that can't fail.  It does introduce a new helper method,
however, that they should eventually be switched to use:
HttpResponseHeaders::TryToCreate().  We should probably put off
conversion until this successfully makes it to stable.

Bug: 832086
Change-Id: Ib75ac03a6a298238cafb41eaa5f046c082fd0bdf
Reviewed-on: https://chromium-review.googlesource.com/c/1291812
Reviewed-by: Asanka Herath <[email protected]>
Commit-Queue: Matt Menke <[email protected]>
Cr-Commit-Position: refs/heads/master@{#601776}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 7, 2018
…lls in them., a=testonly

Automatic update from web-platform-testsHttpStreamParser: Reject headers with nulls in them.

While the HTTP spec further limits what values are legal, nulls are
particularly concerning, and it's safest just to reject them. See
discussion here: whatwg/xhr#165

Chrome will be the first browser to reject nulls in responses, despite
there being wpt tests for this, so we'll have to keep an eye out for
breakages.

For reference, 0x00 through 0x1F aren't allowed in header values or
fields, (https://tools.ietf.org/html/rfc7230#section-3.2 - VCHAR
excludes those characters).  CRs and LFs are of course needed, and
0x0C and 0x0B are allowed by other specs for particular
header parsers, strangely.

This CL does not affect other code that can generate HTTP response
headers, which still uses the old behavior of just removing nulls.
ServiceWorkers, extensions, WebPackages, Dial (?), and various tests
still inherit the old behavior, since they create headers directly
with a method that can't fail.  It does introduce a new helper method,
however, that they should eventually be switched to use:
HttpResponseHeaders::TryToCreate().  We should probably put off
conversion until this successfully makes it to stable.

Bug: 832086
Change-Id: Ib75ac03a6a298238cafb41eaa5f046c082fd0bdf
Reviewed-on: https://chromium-review.googlesource.com/c/1291812
Reviewed-by: Asanka Herath <[email protected]>
Commit-Queue: Matt Menke <[email protected]>
Cr-Commit-Position: refs/heads/master@{#601776}

--

wpt-commits: 89637ce97bb8073a2db5182fc100125acba01481
wpt-pr: 13663
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Nov 8, 2018
…lls in them., a=testonly

Automatic update from web-platform-testsHttpStreamParser: Reject headers with nulls in them.

While the HTTP spec further limits what values are legal, nulls are
particularly concerning, and it's safest just to reject them. See
discussion here: whatwg/xhr#165

Chrome will be the first browser to reject nulls in responses, despite
there being wpt tests for this, so we'll have to keep an eye out for
breakages.

For reference, 0x00 through 0x1F aren't allowed in header values or
fields, (https://tools.ietf.org/html/rfc7230#section-3.2 - VCHAR
excludes those characters).  CRs and LFs are of course needed, and
0x0C and 0x0B are allowed by other specs for particular
header parsers, strangely.

This CL does not affect other code that can generate HTTP response
headers, which still uses the old behavior of just removing nulls.
ServiceWorkers, extensions, WebPackages, Dial (?), and various tests
still inherit the old behavior, since they create headers directly
with a method that can't fail.  It does introduce a new helper method,
however, that they should eventually be switched to use:
HttpResponseHeaders::TryToCreate().  We should probably put off
conversion until this successfully makes it to stable.

Bug: 832086
Change-Id: Ib75ac03a6a298238cafb41eaa5f046c082fd0bdf
Reviewed-on: https://chromium-review.googlesource.com/c/1291812
Reviewed-by: Asanka Herath <[email protected]>
Commit-Queue: Matt Menke <[email protected]>
Cr-Commit-Position: refs/heads/master@{#601776}

--

wpt-commits: 89637ce97bb8073a2db5182fc100125acba01481
wpt-pr: 13663
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 3, 2019
…ntaining 0x00, a=testonly

Automatic update from web-platform-testsFetch/XHR: response header value containing 0x00

As discussed in whatwg/xhr#165 these should turn the response into a network error.
--

wpt-commits: 3d172bc612e03a896b5611d9e9652f860995feef
wpt-pr: 10424

UltraBlame original commit: 0d71b9cb47cea8e9ace98cbffa1585cd9c97c8ca
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 3, 2019
…ntaining 0x00, a=testonly

Automatic update from web-platform-testsFetch/XHR: response header value containing 0x00

As discussed in whatwg/xhr#165 these should turn the response into a network error.
--

wpt-commits: 3d172bc612e03a896b5611d9e9652f860995feef
wpt-pr: 10424

UltraBlame original commit: 0d71b9cb47cea8e9ace98cbffa1585cd9c97c8ca
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 3, 2019
…ntaining 0x00, a=testonly

Automatic update from web-platform-testsFetch/XHR: response header value containing 0x00

As discussed in whatwg/xhr#165 these should turn the response into a network error.
--

wpt-commits: 3d172bc612e03a896b5611d9e9652f860995feef
wpt-pr: 10424

UltraBlame original commit: 0d71b9cb47cea8e9ace98cbffa1585cd9c97c8ca
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 3, 2019
…lls in them., a=testonly

Automatic update from web-platform-testsHttpStreamParser: Reject headers with nulls in them.

While the HTTP spec further limits what values are legal, nulls are
particularly concerning, and it's safest just to reject them. See
discussion here: whatwg/xhr#165

Chrome will be the first browser to reject nulls in responses, despite
there being wpt tests for this, so we'll have to keep an eye out for
breakages.

For reference, 0x00 through 0x1F aren't allowed in header values or
fields, (https://tools.ietf.org/html/rfc7230#section-3.2 - VCHAR
excludes those characters).  CRs and LFs are of course needed, and
0x0C and 0x0B are allowed by other specs for particular
header parsers, strangely.

This CL does not affect other code that can generate HTTP response
headers, which still uses the old behavior of just removing nulls.
ServiceWorkers, extensions, WebPackages, Dial (?), and various tests
still inherit the old behavior, since they create headers directly
with a method that can't fail.  It does introduce a new helper method,
however, that they should eventually be switched to use:
HttpResponseHeaders::TryToCreate().  We should probably put off
conversion until this successfully makes it to stable.

Bug: 832086
Change-Id: Ib75ac03a6a298238cafb41eaa5f046c082fd0bdf
Reviewed-on: https://chromium-review.googlesource.com/c/1291812
Reviewed-by: Asanka Herath <asankachromium.org>
Commit-Queue: Matt Menke <mmenkechromium.org>
Cr-Commit-Position: refs/heads/master{#601776}

--

wpt-commits: 89637ce97bb8073a2db5182fc100125acba01481
wpt-pr: 13663

UltraBlame original commit: 8fc01167cf39bf970ad99953b0776f4338ddca84
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 3, 2019
…lls in them., a=testonly

Automatic update from web-platform-testsHttpStreamParser: Reject headers with nulls in them.

While the HTTP spec further limits what values are legal, nulls are
particularly concerning, and it's safest just to reject them. See
discussion here: whatwg/xhr#165

Chrome will be the first browser to reject nulls in responses, despite
there being wpt tests for this, so we'll have to keep an eye out for
breakages.

For reference, 0x00 through 0x1F aren't allowed in header values or
fields, (https://tools.ietf.org/html/rfc7230#section-3.2 - VCHAR
excludes those characters).  CRs and LFs are of course needed, and
0x0C and 0x0B are allowed by other specs for particular
header parsers, strangely.

This CL does not affect other code that can generate HTTP response
headers, which still uses the old behavior of just removing nulls.
ServiceWorkers, extensions, WebPackages, Dial (?), and various tests
still inherit the old behavior, since they create headers directly
with a method that can't fail.  It does introduce a new helper method,
however, that they should eventually be switched to use:
HttpResponseHeaders::TryToCreate().  We should probably put off
conversion until this successfully makes it to stable.

Bug: 832086
Change-Id: Ib75ac03a6a298238cafb41eaa5f046c082fd0bdf
Reviewed-on: https://chromium-review.googlesource.com/c/1291812
Reviewed-by: Asanka Herath <asankachromium.org>
Commit-Queue: Matt Menke <mmenkechromium.org>
Cr-Commit-Position: refs/heads/master{#601776}

--

wpt-commits: 89637ce97bb8073a2db5182fc100125acba01481
wpt-pr: 13663

UltraBlame original commit: 8fc01167cf39bf970ad99953b0776f4338ddca84
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 3, 2019
…lls in them., a=testonly

Automatic update from web-platform-testsHttpStreamParser: Reject headers with nulls in them.

While the HTTP spec further limits what values are legal, nulls are
particularly concerning, and it's safest just to reject them. See
discussion here: whatwg/xhr#165

Chrome will be the first browser to reject nulls in responses, despite
there being wpt tests for this, so we'll have to keep an eye out for
breakages.

For reference, 0x00 through 0x1F aren't allowed in header values or
fields, (https://tools.ietf.org/html/rfc7230#section-3.2 - VCHAR
excludes those characters).  CRs and LFs are of course needed, and
0x0C and 0x0B are allowed by other specs for particular
header parsers, strangely.

This CL does not affect other code that can generate HTTP response
headers, which still uses the old behavior of just removing nulls.
ServiceWorkers, extensions, WebPackages, Dial (?), and various tests
still inherit the old behavior, since they create headers directly
with a method that can't fail.  It does introduce a new helper method,
however, that they should eventually be switched to use:
HttpResponseHeaders::TryToCreate().  We should probably put off
conversion until this successfully makes it to stable.

Bug: 832086
Change-Id: Ib75ac03a6a298238cafb41eaa5f046c082fd0bdf
Reviewed-on: https://chromium-review.googlesource.com/c/1291812
Reviewed-by: Asanka Herath <asankachromium.org>
Commit-Queue: Matt Menke <mmenkechromium.org>
Cr-Commit-Position: refs/heads/master{#601776}

--

wpt-commits: 89637ce97bb8073a2db5182fc100125acba01481
wpt-pr: 13663

UltraBlame original commit: 8fc01167cf39bf970ad99953b0776f4338ddca84
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

8 participants