Issue metadata
Sign in to add a comment
|
Reject HTTP response with null character in header value |
||||||||||||||||||||
Issue descriptionSee 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.
,
Apr 16 2018
Agreed. Depending on where we fix this, QUIC might get fixed for free...
,
Apr 17 2018
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.)
,
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.
,
Apr 17 2018
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).
,
Jul 6
,
Jul 6
,
Oct 19
I'll take this. Sounds like H2 and QUIC are likely already covered, so just HTTP/1.x to do.
,
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
,
Oct 22
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.
,
Oct 23
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.)
,
Oct 26
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 |
|||||||||||||||||||||
Comment 1 by mmenke@chromium.org
, Apr 12 2018