Skip to content

GitLab

T
tangled
Commit 900336ff authored by Matt Menke's avatar Matt Menke Committed by Commit Bot
Browse files
Options

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/1291812Reviewed-by: default avatarAsanka Herath <asanka@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601776}
parent 4213dae8
Hide whitespace changes
Inline Side-by-side
Showing with 62 additions and 202 deletions
net/http/http_response_headers.cc
View file @ 900336ff
......@@ -10,6 +10,7 @@
#include "net/http/http_response_headers.h"
#include <algorithm>
#include <limits>
#include <memory>
#include <unordered_map>
#include <utility>
......@@ -116,11 +117,19 @@ bool ShouldUpdateHeader(base::StringPiece name) {
return true;
}
void CheckDoesNotHaveEmbededNulls(const std::string& str) {
bool HasEmbeddedNulls(base::StringPiece str) {
for (char c : str) {
if (c == '\0')
return true;
}
return false;
}
void CheckDoesNotHaveEmbeddedNulls(base::StringPiece str) {
// Care needs to be taken when adding values to the raw headers string to
// make sure it does not contain embeded NULLs. Any embeded '\0' may be
// understood as line terminators and change how header lines get tokenized.
CHECK(str.find('\0') == std::string::npos);
CHECK(!HasEmbeddedNulls(str));
}
} // namespace
......@@ -170,6 +179,18 @@ HttpResponseHeaders::HttpResponseHeaders(base::PickleIterator* iter)
Parse(raw_input);
}
scoped_refptr<HttpResponseHeaders> HttpResponseHeaders::TryToCreate(
base::StringPiece headers) {
// Reject strings with nulls.
if (HasEmbeddedNulls(headers) ||
headers.size() > std::numeric_limits<int>::max()) {
return nullptr;
}
return base::MakeRefCounted<HttpResponseHeaders>(
HttpUtil::AssembleRawHeaders(headers.data(), headers.size()));
}
void HttpResponseHeaders::Persist(base::Pickle* pickle,
PersistOptions options) {
if (options == PERSIST_RAW) {
......@@ -357,7 +378,7 @@ void HttpResponseHeaders::RemoveHeaderLine(const std::string& name,
}
void HttpResponseHeaders::AddHeader(const std::string& header) {
CheckDoesNotHaveEmbededNulls(header);
CheckDoesNotHaveEmbeddedNulls(header);
DCHECK_EQ('\0', raw_headers_[raw_headers_.size() - 2]);
DCHECK_EQ('\0', raw_headers_[raw_headers_.size() - 1]);
// Don't copy the last null.
......@@ -377,7 +398,7 @@ void HttpResponseHeaders::AddCookie(const std::string& cookie_string) {
}
void HttpResponseHeaders::ReplaceStatusLine(const std::string& new_status) {
CheckDoesNotHaveEmbededNulls(new_status);
CheckDoesNotHaveEmbeddedNulls(new_status);
// Copy up to the null byte. This just copies the status line.
std::string new_raw_headers(new_status);
new_raw_headers.push_back('\0');
......
net/http/http_response_headers.h
View file @ 900336ff
......@@ -80,6 +80,13 @@ class NET_EXPORT HttpResponseHeaders
// be passed to the pickle's various Read* methods.
explicit HttpResponseHeaders(base::PickleIterator* pickle_iter);
// Takes headers as an ASCII string and tries to parse them as HTTP response
// headers. returns nullptr on failure. Unlike the HttpResponseHeaders
// constructor that takes a std::string, HttpUtil::AssembleRawHeaders should
// not be called on |headers| before calling this method.
static scoped_refptr<HttpResponseHeaders> TryToCreate(
base::StringPiece headers);
// Appends a representation of this object to the given pickle.
// The options argument can be a combination of PersistOptions.
void Persist(base::Pickle* pickle, PersistOptions options);
......
net/http/http_stream_parser.cc
View file @ 900336ff
......@@ -971,8 +971,10 @@ int HttpStreamParser::ParseResponseHeaders(int end_offset) {
if (response_header_start_offset_ >= 0) {
received_bytes_ += end_offset;
headers = new HttpResponseHeaders(
HttpUtil::AssembleRawHeaders(read_buf_->StartOfBuffer(), end_offset));
headers = HttpResponseHeaders::TryToCreate(
base::StringPiece(read_buf_->StartOfBuffer(), end_offset));
if (!headers)
return net::ERR_INVALID_HTTP_RESPONSE;
} else {
// Enough data was read -- there is no status line, so this is HTTP/0.9, or
// the server is broken / doesn't speak HTTP.
......
net/http/http_stream_parser_unittest.cc
View file @ 900336ff
......@@ -1435,6 +1435,31 @@ TEST(HttpStreamParser, Http09PortTests) {
}
}
TEST(HttpStreamParser, NullFails) {
const char kTestHeaders[] =
"HTTP/1.1 200 OK\r\n"
"Foo: Bar\r\n"
"Content-Length: 4\r\n\r\n";
// Try inserting a null at each position in kTestHeaders. Every location
// should result in an error.
//
// Need to start at 4 because HttpStreamParser will treat the response as
// HTTP/0.9 if it doesn't see "HTTP", and need to end at -1 because "\r\n\r"
// is currently treated as a valid end of header marker.
for (size_t i = 4; i < base::size(kTestHeaders) - 1; ++i) {
std::string read_data(kTestHeaders);
read_data.insert(i, 1, '\0');
read_data.append("body");
SimpleGetRunner get_runner;
get_runner.set_url(GURL("http://foo.test/"));
get_runner.AddRead(read_data);
get_runner.SetupParserAndSendRequest();
get_runner.ReadHeadersExpectingError(ERR_INVALID_HTTP_RESPONSE);
}
}
// Make sure that Shoutcast is recognized when receiving one byte at a time.
TEST(HttpStreamParser, ShoutcastSingleByteReads) {
SimpleGetRunner get_runner;
......
third_party/WebKit/LayoutTests/external/wpt/cookies/http-state/chromium-tests-expected.txt
View file @ 900336ff
......@@ -20,7 +20,6 @@ PASS chromium0018 - chromium0018
PASS chromium0019 - chromium0019
PASS disabled-chromium0020 - disabled-chromium0020
PASS chromium0021 - chromium0021
FAIL disabled-chromium0022 - disabled-chromium0022 assert_equals: expected "AAA=BB" but got "AAA=BBZYX"
PASS disabled-chromium0023 - disabled-chromium0023
PASS disabled-chromium0022 - disabled-chromium0022
Harness: the test ran to completion.
third_party/WebKit/LayoutTests/external/wpt/cookies/http-state/chromium-tests.html
View file @ 900336ff
......@@ -43,7 +43,6 @@
{file: "disabled-chromium0020", name: "disabled-chromium0020"},
{file: "chromium0021", name: "chromium0021"},
{file: "disabled-chromium0022", name: "disabled-chromium0022"},
{file: "disabled-chromium0023", name: "disabled-chromium0023"},
];
for (const i in TEST_CASES) {
......
third_party/WebKit/LayoutTests/external/wpt/cors/allow-headers-expected.txt deleted 100644 → 0
View file @ 4213dae8
This is a testharness.js-based test.
PASS Allow origin: *
PASS Allow origin: _*__
PASS Allow origin: [tab]*
PASS Allow origin: http://web-platform.test:8001
PASS Allow origin: _http://web-platform.test:8001
PASS Allow origin: _http://web-platform.test:8001___[tab]_
PASS Allow origin: [tab]http://web-platform.test:8001
PASS Disallow origin: http://www1.web-platform.test:8001
PASS Disallow origin: //web-platform.test:8001
PASS Disallow origin: ://web-platform.test:8001
PASS Disallow origin: ftp://web-platform.test:8001
PASS Disallow origin: http:://web-platform.test:8001
PASS Disallow origin: http:/web-platform.test:8001
PASS Disallow origin: http:web-platform.test:8001
PASS Disallow origin: web-platform.test:8001
PASS Disallow origin: http://web-platform.test:8001?
PASS Disallow origin: http://web-platform.test:8001/
PASS Disallow origin: http://web-platform.test:8001 /
PASS Disallow origin: http://web-platform.test:8001#
PASS Disallow origin: http://web-platform.test:8001%23
PASS Disallow origin: http://web-platform.test:8001:80
PASS Disallow origin: http://web-platform.test:8001, *
FAIL Disallow origin: http://web-platform.test:8001\0 assert_throws: send function "function() { client.send() }" did not throw
PASS Disallow origin: HTTP://WEB-PLATFORM.TEST:8001
PASS Disallow origin: HTTP://web-platform.test:8001
PASS Disallow origin: -
PASS Disallow origin: **
FAIL Disallow origin: \0* assert_throws: send function "function() { client.send() }" did not throw
FAIL Disallow origin: *\0 assert_throws: send function "function() { client.send() }" did not throw
PASS Disallow origin: '*'
PASS Disallow origin: "*"
PASS Disallow origin: * *
PASS Disallow origin: *http://*
PASS Disallow origin: *http://web-platform.test:8001
PASS Disallow origin: * http://web-platform.test:8001
PASS Disallow origin: *, http://web-platform.test:8001
FAIL Disallow origin: \0http://web-platform.test:8001 assert_throws: send function "function() { client.send() }" did not throw
PASS Disallow origin: null http://web-platform.test:8001
PASS Disallow origin: http://example.net
PASS Disallow origin: null
PASS Disallow origin:
PASS Disallow origin: http://web-platform.test:8001/cors/allow-headers.htm
PASS Disallow origin: http://web-platform.test:8001/cors/
PASS Disallow origin: http://www1.web-platform.test:8001/cors/
Harness: the test ran to completion.
third_party/WebKit/LayoutTests/external/wpt/cors/origin-expected.txt deleted 100644 → 0
View file @ 4213dae8
This is a testharness.js-based test.
Found 58 tests; 54 PASS, 4 FAIL, 0 TIMEOUT, 0 NOTRUN.
PASS Allow origin: *
PASS Allow origin: _*__
PASS Allow origin: [tab]*
PASS Allow origin: http://web-platform.test:8001
PASS Allow origin: _http://web-platform.test:8001
PASS Allow origin: _http://web-platform.test:8001___[tab]_
PASS Allow origin: [tab]http://web-platform.test:8001
PASS Disallow origin: http://www1.web-platform.test:8001
PASS Disallow origin: //web-platform.test:8001
PASS Disallow origin: ://web-platform.test:8001
PASS Disallow origin: ftp://web-platform.test:8001
PASS Disallow origin: http:://web-platform.test:8001
PASS Disallow origin: http:/web-platform.test:8001
PASS Disallow origin: http:web-platform.test:8001
PASS Disallow origin: web-platform.test:8001
PASS Disallow origin: http://web-platform.test:8001?
PASS Disallow origin: http://web-platform.test:8001/
PASS Disallow origin: http://web-platform.test:8001 /
PASS Disallow origin: http://web-platform.test:8001#
PASS Disallow origin: http://web-platform.test:8001%23
PASS Disallow origin: http://web-platform.test:8001:80
PASS Disallow origin: http://web-platform.test:8001, *
FAIL Disallow origin: http://web-platform.test:8001\0 assert_throws: send function "function() { client.send() }" did not throw
PASS Disallow origin: HTTP://WEB-PLATFORM.TEST:8001
PASS Disallow origin: HTTP://web-platform.test:8001
PASS Disallow origin: -
PASS Disallow origin: **
FAIL Disallow origin: \0* assert_throws: send function "function() { client.send() }" did not throw
FAIL Disallow origin: *\0 assert_throws: send function "function() { client.send() }" did not throw
PASS Disallow origin: '*'
PASS Disallow origin: "*"
PASS Disallow origin: * *
PASS Disallow origin: * null
PASS Disallow origin: *http://*
PASS Disallow origin: *http://web-platform.test:8001
PASS Disallow origin: * http://web-platform.test:8001
PASS Disallow origin: *, http://web-platform.test:8001
FAIL Disallow origin: \0http://web-platform.test:8001 assert_throws: send function "function() { client.send() }" did not throw
PASS Disallow origin: null http://web-platform.test:8001
PASS Disallow origin: http://example.net
PASS Disallow origin: null
PASS Disallow origin: null *
PASS Disallow origin:
PASS Disallow origin: http://web-platform.test:8001/cors/origin.htm
PASS Disallow origin: http://web-platform.test:8001/cors/
PASS Disallow origin: http://www1.web-platform.test:8001/cors/
PASS Disallow origin: test:8001
PASS Disallow origin: .test:8001
PASS Disallow origin: *.test:8001
PASS Disallow origin: http://test:8001
PASS Disallow origin: http://.test:8001
PASS Disallow origin: http://*.test:8001
PASS Disallow multiple headers (, *)
PASS Disallow multiple headers (*, )
PASS Disallow multiple headers (*, *)
PASS Disallow multiple headers (, http://web-platform.test:8001)
PASS Disallow multiple headers (*, http://web-platform.test:8001)
PASS Disallow multiple headers (http://web-platform.test:8001, http://web-platform.test:8001)
Harness: the test ran to completion.
third_party/WebKit/LayoutTests/external/wpt/cors/remote-origin-expected.txt deleted 100644 → 0
View file @ 4213dae8
This is a testharness.js-based test.
PASS Allow origin: *
PASS Allow origin: _*__
PASS Allow origin: [tab]*
PASS Allow origin: http://www1.web-platform.test:8001
PASS Allow origin: _http://www1.web-platform.test:8001
PASS Allow origin: _http://www1.web-platform.test:8001___[tab]_
PASS Allow origin: [tab]http://www1.web-platform.test:8001
PASS Disallow origin: http://web-platform.test:8001
PASS Disallow origin: //www1.web-platform.test:8001
PASS Disallow origin: ://www1.web-platform.test:8001
PASS Disallow origin: ftp://www1.web-platform.test:8001
PASS Disallow origin: http:://www1.web-platform.test:8001
PASS Disallow origin: http:/www1.web-platform.test:8001
PASS Disallow origin: http:www1.web-platform.test:8001
PASS Disallow origin: www1.web-platform.test:8001
PASS Disallow origin: http://www1.web-platform.test:8001?
PASS Disallow origin: http://www1.web-platform.test:8001/
PASS Disallow origin: http://www1.web-platform.test:8001_/
PASS Disallow origin: http://www1.web-platform.test:8001#
PASS Disallow origin: http://www1.web-platform.test:8001%23
PASS Disallow origin: http://www1.web-platform.test:8001:80
PASS Disallow origin: http://www1.web-platform.test:8001,_*
FAIL Disallow origin: http://www1.web-platform.test:8001\0 assert_equals: expected "error" but got "load"
PASS Disallow origin: HTTP://WWW1.WEB-PLATFORM.TEST:8001
PASS Disallow origin: HTTP://www1.web-platform.test:8001
PASS Disallow origin: http://WWW1.WEB-PLATFORM.TEST:8001
PASS Disallow origin: -
PASS Disallow origin: **
FAIL Disallow origin: \0* assert_equals: expected "error" but got "load"
FAIL Disallow origin: *\0 assert_equals: expected "error" but got "load"
PASS Disallow origin: '*'
PASS Disallow origin: "*"
PASS Disallow origin: *_*
PASS Disallow origin: *http://*
PASS Disallow origin: *http://www1.web-platform.test:8001
PASS Disallow origin: *_http://www1.web-platform.test:8001
PASS Disallow origin: *,_http://www1.web-platform.test:8001
FAIL Disallow origin: \0http://www1.web-platform.test:8001 assert_equals: expected "error" but got "load"
PASS Disallow origin: null_http://www1.web-platform.test:8001
PASS Disallow origin: http://example.net
PASS Disallow origin: null
PASS Disallow origin:
PASS Disallow origin: http://web-platform.test:8001/cors/remote-origin.htm
PASS Disallow origin: http://web-platform.test:8001/cors/
PASS Disallow origin: http://www1.web-platform.test:8001/cors/
Harness: the test ran to completion.
third_party/WebKit/LayoutTests/external/wpt/fetch/api/basic/header-value-null-byte.any-expected.txt deleted 100644 → 0
View file @ 4213dae8
This is a testharness.js-based test.
FAIL Ensure fetch() rejects null bytes in headers assert_unreached: Should have rejected: undefined Reached unreachable code
Harness: the test ran to completion.
third_party/WebKit/LayoutTests/external/wpt/fetch/api/basic/header-value-null-byte.any.serviceworker-expected.txt deleted 100644 → 0
View file @ 4213dae8
This is a testharness.js-based test.
FAIL Ensure fetch() rejects null bytes in headers assert_unreached: Should have rejected: undefined Reached unreachable code
Harness: the test ran to completion.
third_party/WebKit/LayoutTests/external/wpt/fetch/api/basic/header-value-null-byte.any.sharedworker-expected.txt deleted 100644 → 0
View file @ 4213dae8
This is a testharness.js-based test.
FAIL Ensure fetch() rejects null bytes in headers assert_unreached: Should have rejected: undefined Reached unreachable code
Harness: the test ran to completion.
third_party/WebKit/LayoutTests/external/wpt/fetch/api/basic/header-value-null-byte.any.worker-expected.txt deleted 100644 → 0
View file @ 4213dae8
This is a testharness.js-based test.
FAIL Ensure fetch() rejects null bytes in headers assert_unreached: Should have rejected: undefined Reached unreachable code
Harness: the test ran to completion.
third_party/WebKit/LayoutTests/external/wpt/xhr/headers-normalize-response-expected.txt deleted 100644 → 0
View file @ 4213dae8
This is a testharness.js-based test.
FAIL Header value: hello world\0 assert_throws: function "() => client.send()" did not throw
FAIL Header value: \0hello world assert_throws: function "() => client.send()" did not throw
FAIL Header value: hello\0world assert_throws: function "() => client.send()" did not throw
PASS Header value: __hello_world
PASS Header value: hello_world__
PASS Header value: __hello_world__
PASS Header value: [tab]hello_world
PASS Header value: hello_world[tab]
PASS Header value: [tab]hello_world[tab]
PASS Header value: hello______world
PASS Header value: hello[tab]world
FAIL Header value: \0 assert_throws: function "() => client.send()" did not throw
PASS Header value: ___
PASS Header value: [tab]
PASS Header value:
Harness: the test ran to completion.
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment