New issue
Advanced search Search tips

Issue 832086 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: 2019-02-02
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Reject HTTP response with null character in header value

Project Member Reported by b...@chromium.org, Apr 12 2018

Issue description

See https://github.com/whatwg/xhr/issues/165 for discussion.  Also see
https://bugzilla.mozilla.org/show_bug.cgi?id=1453318
https://bugs.webkit.org/show_bug.cgi?id=184493
https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/16880179/
for bugs filed against other products.  Preemptively adding Internals>Network>HTTP2 label as well, because the best place to implement this for HTTP/2 responses might be different from the best place for older versions.
 

Comment 1 by mmenke@chromium.org, Apr 12 2018

Components: Internals>Network>QUIC
If H2 makes sense as a label here, surely QUIC does as well?

Comment 2 by rch@chromium.org, Apr 16 2018

Agreed. Depending on where we fix this, QUIC might get fixed for free...
Slightly off-topic: I'm not super familiar with H/2 and QUIC. Do they enforce that a header value does not contain 0x0A or 0x0D? And has no leading or trailing 0x09, 0x0A, 0x0D, or 0x20 bytes? Your comments about 0x00 make me wonder if they're even more relaxed about header values than H/1 is.

(Unfortunately there's no web-platform-tests infrastructure beyond H/1 at the moment. We might get H/2 later this year though.)

Comment 4 by b...@chromium.org, Apr 17 2018

h2 header validation is done in HeaderCoalescer, which only allows the following characters in a header value: '\t' (HTAB), x20 (SP), x21-7E, and x80-FF.  It does not check for trailing 0x09.  See https://cs.chromium.org/chromium/src/net/spdy/chromium/header_coalescer.cc?l=111.

Why is trailing 0x20 not allowed?  It should be trivial to ban that in h2.

The best place to fix h1 might be AssembleRawHeaders(), which would unfortunately not buy us either h2 or QUIC for free.
I don't really know how H/2 defines things. In H/1 you can have leading/trailing whitespace, but that whitespace is removed before the value reaches any consumers.

If H/2 header validation is as you say, it seems that 0x00 is already disallowed (and H/2 header values are generally much stricter than H/1).
Components: Internals>Network
Components: -Internals>Network>HTTP
Cc: b...@chromium.org
Owner: mmenke@chromium.org
I'll take this.  Sounds like H2 and QUIC are likely already covered, so just HTTP/1.x to do.
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 22

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/900336ff53907fb0263941689ee734103f0ad352

commit 900336ff53907fb0263941689ee734103f0ad352
Author: Matt Menke <mmenke@chromium.org>
Date: Mon Oct 22 23:26:33 2018

HttpStreamParser: 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: https://github.com/whatwg/xhr/issues/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 <asanka@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601776}
[modify] https://crrev.com/900336ff53907fb0263941689ee734103f0ad352/net/http/http_response_headers.cc
[modify] https://crrev.com/900336ff53907fb0263941689ee734103f0ad352/net/http/http_response_headers.h
[modify] https://crrev.com/900336ff53907fb0263941689ee734103f0ad352/net/http/http_stream_parser.cc
[modify] https://crrev.com/900336ff53907fb0263941689ee734103f0ad352/net/http/http_stream_parser_unittest.cc
[modify] https://crrev.com/900336ff53907fb0263941689ee734103f0ad352/third_party/WebKit/LayoutTests/external/wpt/cookies/http-state/chromium-tests-expected.txt
[modify] https://crrev.com/900336ff53907fb0263941689ee734103f0ad352/third_party/WebKit/LayoutTests/external/wpt/cookies/http-state/chromium-tests.html
[modify] https://crrev.com/900336ff53907fb0263941689ee734103f0ad352/third_party/WebKit/LayoutTests/external/wpt/cookies/http-state/resources/test-files/disabled-chromium0022-test
[delete] https://crrev.com/4213dae85d26ae46cbf108f884113749ee832bb5/third_party/WebKit/LayoutTests/external/wpt/cookies/http-state/resources/test-files/disabled-chromium0023-expected
[delete] https://crrev.com/4213dae85d26ae46cbf108f884113749ee832bb5/third_party/WebKit/LayoutTests/external/wpt/cookies/http-state/resources/test-files/disabled-chromium0023-test
[delete] https://crrev.com/4213dae85d26ae46cbf108f884113749ee832bb5/third_party/WebKit/LayoutTests/external/wpt/cors/allow-headers-expected.txt
[delete] https://crrev.com/4213dae85d26ae46cbf108f884113749ee832bb5/third_party/WebKit/LayoutTests/external/wpt/cors/origin-expected.txt
[delete] https://crrev.com/4213dae85d26ae46cbf108f884113749ee832bb5/third_party/WebKit/LayoutTests/external/wpt/cors/remote-origin-expected.txt
[delete] https://crrev.com/4213dae85d26ae46cbf108f884113749ee832bb5/third_party/WebKit/LayoutTests/external/wpt/fetch/api/basic/header-value-null-byte.any-expected.txt
[delete] https://crrev.com/4213dae85d26ae46cbf108f884113749ee832bb5/third_party/WebKit/LayoutTests/external/wpt/fetch/api/basic/header-value-null-byte.any.serviceworker-expected.txt
[delete] https://crrev.com/4213dae85d26ae46cbf108f884113749ee832bb5/third_party/WebKit/LayoutTests/external/wpt/fetch/api/basic/header-value-null-byte.any.sharedworker-expected.txt
[delete] https://crrev.com/4213dae85d26ae46cbf108f884113749ee832bb5/third_party/WebKit/LayoutTests/external/wpt/fetch/api/basic/header-value-null-byte.any.worker-expected.txt
[delete] https://crrev.com/4213dae85d26ae46cbf108f884113749ee832bb5/third_party/WebKit/LayoutTests/external/wpt/xhr/headers-normalize-response-expected.txt

NextAction: 2019-02-02
Setting next action to update other consumers - per description, this doesn't affect ServiceWorker (Though suspect, in practice, SW's can set such headers, and they can only send such headers to the site that uses them, anyways), or a bunch of other consumers that are even less likely to have issues.

Want to let this bake, and see if there are any major compatibility issues that it causes.
In reaction to your commit message, note https://github.com/httpwg/http-core/issues/19. I'm not at all convinced the field-value ABNF is correct as far as implementations go. Per my testing browsers don't really restrict 0x01-0x1F meaningfully. (I think Safari has also reverted its stance on these bytes to be compatible with others.)
Cc: mmenke@chromium.org
Owner: ----
Status: Available (was: Assigned)
Unassigning from me - I do plan on doing the followup, but too much clutter on my assigned issues page.

Sign in to add a comment