Commit 3301bab5 authored by Yutaka Hirano's avatar Yutaka Hirano Committed by Commit Bot

Reland "Strengthen requirements on CORS-safelisted request-headers"

This is a reland of 074455de

Original change's description:
> Strengthen requirements on CORS-safelisted request-headers
>
> With this CL, some request headers that used to be treated as
> CORS-safelisted are not CORS-safelisted any more. Specifically,
>
>  - "accept", "accept-language" and "content-language" have a stronger
>    check on its value
>  - All headers whose value exceeds 128 bytes are treated as not
>    CORS-safelisted
>  - If the sum of value length of CORS-safelisted headers exceeds 1024,
>    then all of them are treated as not CORS-safelisted.
>
> This CL also implements
> https://fetch.spec.whatwg.org/#no-cors-safelisted-request-header.
>
> This is for https://github.com/whatwg/fetch/pull/736.
>
> Bug: 824130
> Cq-Include-Trybots: luci.chromium.try:linux_mojo
> Change-Id: Ib12a7dbff6367717a43130ae59304dca55b7bf4e
> Reviewed-on: https://chromium-review.googlesource.com/1196563
> Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#589153}

Bug: 824130
Change-Id: Ia5caad12a51ee44713cf4cf11f42b1fc9ab831a9
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Tbr: toyoshim@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/1212425
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: default avatarYutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589434}
parent 299669f9
......@@ -46,13 +46,9 @@ bool NeedsPreflight(const ResourceRequest& request) {
if (!IsCORSSafelistedMethod(request.method))
return true;
for (const auto& header : request.headers.GetHeaderVector()) {
if (!IsCORSSafelistedHeader(header.key, header.value) &&
!IsForbiddenHeader(header.key)) {
return true;
}
}
return false;
return !CORSUnsafeNotForbiddenRequestHeaderNames(
request.headers.GetHeaderVector())
.empty();
}
} // namespace
......
......@@ -41,18 +41,11 @@ base::Optional<std::string> GetHeaderString(
// - byte-lowercased
std::string CreateAccessControlRequestHeadersHeader(
const net::HttpRequestHeaders& headers) {
std::vector<std::string> filtered_headers;
for (const auto& header : headers.GetHeaderVector()) {
// Exclude CORS-safelisted headers.
if (cors::IsCORSSafelistedHeader(header.key, header.value))
continue;
// Exclude the forbidden headers because they may be added by the user
// agent. They must be checked separately and rejected for
// JavaScript-initiated requests.
if (cors::IsForbiddenHeader(header.key))
continue;
filtered_headers.push_back(base::ToLowerASCII(header.key));
}
// Exclude the forbidden headers because they may be added by the user
// agent. They must be checked separately and rejected for
// JavaScript-initiated requests.
std::vector<std::string> filtered_headers =
CORSUnsafeNotForbiddenRequestHeaderNames(headers.GetHeaderVector());
if (filtered_headers.empty())
return std::string();
......
......@@ -354,6 +354,10 @@ bool IsCORSSafelistedContentType(const std::string& media_type) {
}
bool IsCORSSafelistedHeader(const std::string& name, const std::string& value) {
// If |value|’s length is greater than 128, then return false.
if (value.size() > 128)
return false;
// https://fetch.spec.whatwg.org/#cors-safelisted-request-header
// "A CORS-safelisted header is a header whose name is either one of `Accept`,
// `Accept-Language`, and `Content-Language`, or whose name is
......@@ -395,12 +399,89 @@ bool IsCORSSafelistedHeader(const std::string& name, const std::string& value) {
if (lower_name == "save-data")
return lower_value == "on";
if (lower_name == "accept") {
return (value.end() == std::find_if(value.begin(), value.end(), [](char c) {
return (c < 0x20 && c != 0x09) || c == 0x22 || c == 0x28 ||
c == 0x29 || c == 0x3a || c == 0x3c || c == 0x3e ||
c == 0x3f || c == 0x40 || c == 0x5b || c == 0x5c ||
c == 0x5d || c == 0x7b || c == 0x7d || c >= 0x7f;
}));
}
if (lower_name == "accept-language" || lower_name == "content-language") {
return (value.end() == std::find_if(value.begin(), value.end(), [](char c) {
return !isalnum(c) && c != 0x20 && c != 0x2a && c != 0x2c &&
c != 0x2d && c != 0x2e && c != 0x3b && c != 0x3d;
}));
}
if (lower_name == "content-type")
return IsCORSSafelistedLowerCaseContentType(lower_value);
return true;
}
bool IsNoCORSSafelistedHeader(const std::string& name,
const std::string& value) {
const std::string lower_name = base::ToLowerASCII(name);
if (lower_name != "accept" && lower_name != "accept-language" &&
lower_name != "content-language" && lower_name != "content-type") {
return false;
}
return IsCORSSafelistedHeader(lower_name, value);
}
std::vector<std::string> CORSUnsafeRequestHeaderNames(
const net::HttpRequestHeaders::HeaderVector& headers) {
std::vector<std::string> potentially_unsafe_names;
std::vector<std::string> header_names;
constexpr size_t kSafeListValueSizeMax = 1024;
size_t safe_list_value_size = 0;
for (const auto& header : headers) {
if (!IsCORSSafelistedHeader(header.key, header.value)) {
header_names.push_back(base::ToLowerASCII(header.key));
} else {
potentially_unsafe_names.push_back(base::ToLowerASCII(header.key));
safe_list_value_size += header.value.size();
}
}
if (safe_list_value_size > kSafeListValueSizeMax) {
header_names.insert(header_names.end(), potentially_unsafe_names.begin(),
potentially_unsafe_names.end());
}
return header_names;
}
std::vector<std::string> CORSUnsafeNotForbiddenRequestHeaderNames(
const net::HttpRequestHeaders::HeaderVector& headers) {
std::vector<std::string> header_names;
std::vector<std::string> potentially_unsafe_names;
constexpr size_t kSafeListValueSizeMax = 1024;
size_t safe_list_value_size = 0;
for (const auto& header : headers) {
if (IsForbiddenHeader(header.key))
continue;
if (!IsCORSSafelistedHeader(header.key, header.value)) {
header_names.push_back(base::ToLowerASCII(header.key));
} else {
potentially_unsafe_names.push_back(base::ToLowerASCII(header.key));
safe_list_value_size += header.value.size();
}
}
if (safe_list_value_size > kSafeListValueSizeMax) {
header_names.insert(header_names.end(), potentially_unsafe_names.begin(),
potentially_unsafe_names.end());
}
return header_names;
}
bool IsForbiddenMethod(const std::string& method) {
static const std::vector<std::string> forbidden_methods = {"trace", "track",
"connect"};
......
......@@ -6,9 +6,11 @@
#define SERVICES_NETWORK_PUBLIC_CPP_CORS_CORS_H_
#include <string>
#include <vector>
#include "base/component_export.h"
#include "base/optional.h"
#include "net/http/http_request_headers.h"
#include "services/network/public/cpp/cors/cors_error_status.h"
#include "services/network/public/mojom/cors.mojom-shared.h"
#include "services/network/public/mojom/fetch_api.mojom-shared.h"
......@@ -117,6 +119,26 @@ COMPONENT_EXPORT(NETWORK_CPP)
bool IsCORSSafelistedContentType(const std::string& name);
COMPONENT_EXPORT(NETWORK_CPP)
bool IsCORSSafelistedHeader(const std::string& name, const std::string& value);
COMPONENT_EXPORT(NETWORK_CPP)
bool IsNoCORSSafelistedHeader(const std::string& name,
const std::string& value);
// https://fetch.spec.whatwg.org/#cors-unsafe-request-header-names
// |headers| must not contain multiple headers for the same name.
// The returned list is NOT sorted.
// The returned list consists of lower-cased names.
COMPONENT_EXPORT(NETWORK_CPP)
std::vector<std::string> CORSUnsafeRequestHeaderNames(
const net::HttpRequestHeaders::HeaderVector& headers);
// https://fetch.spec.whatwg.org/#cors-unsafe-request-header-names
// Returns header names which are not CORS-safelisted AND not forbidden.
// |headers| must not contain multiple headers for the same name.
// The returned list is NOT sorted.
// The returned list consists of lower-cased names.
COMPONENT_EXPORT(NETWORK_CPP)
std::vector<std::string> CORSUnsafeNotForbiddenRequestHeaderNames(
const net::HttpRequestHeaders::HeaderVector& headers);
// Checks forbidden method in the fetch spec.
// See https://fetch.spec.whatwg.org/#forbidden-method.
......
......@@ -136,20 +136,17 @@ PreflightResult::EnsureAllowedCrossOriginHeaders(
if (!credentials_ && headers_.find("*") != headers_.end())
return base::nullopt;
for (const auto& header : headers.GetHeaderVector()) {
// Forbidden headers are forbidden to be used by JavaScript, and checked
// beforehand. But user-agents may add these headers internally, and it's
// fine.
for (const auto& name :
CORSUnsafeNotForbiddenRequestHeaderNames(headers.GetHeaderVector())) {
// Header list check is performed in case-insensitive way. Here, we have a
// parsed header list set in lower case, and search each header in lower
// case.
const std::string key = base::ToLowerASCII(header.key);
if (headers_.find(key) == headers_.end() &&
!IsCORSSafelistedHeader(key, header.value)) {
// Forbidden headers are forbidden to be used by JavaScript, and checked
// beforehand. But user-agents may add these headers internally, and it's
// fine.
if (IsForbiddenHeader(key))
continue;
if (headers_.find(name) == headers_.end()) {
return CORSErrorStatus(
mojom::CORSError::kHeaderDisallowedByPreflightResponse, header.key);
mojom::CORSError::kHeaderDisallowedByPreflightResponse, name);
}
}
return base::nullopt;
......
......@@ -135,15 +135,15 @@ const TestCase header_cases[] = {
{"GET", "", mojom::FetchCredentialsMode::kOmit, "GET", "X-MY-HEADER:t",
mojom::FetchCredentialsMode::kOmit,
CORSErrorStatus(mojom::CORSError::kHeaderDisallowedByPreflightResponse,
"X-MY-HEADER")},
"x-my-header")},
{"GET", "X-SOME-OTHER-HEADER", mojom::FetchCredentialsMode::kOmit, "GET",
"X-MY-HEADER:t", mojom::FetchCredentialsMode::kOmit,
CORSErrorStatus(mojom::CORSError::kHeaderDisallowedByPreflightResponse,
"X-MY-HEADER")},
"x-my-header")},
{"GET", "X-MY-HEADER", mojom::FetchCredentialsMode::kOmit, "GET",
"X-MY-HEADER:t\r\nY-MY-HEADER:t", mojom::FetchCredentialsMode::kOmit,
CORSErrorStatus(mojom::CORSError::kHeaderDisallowedByPreflightResponse,
"Y-MY-HEADER")},
"y-my-header")},
};
TEST_F(PreflightResultTest, MaxAge) {
......
This is a testharness.js-based test.
PASS Loading data…
FAIL Need CORS-preflight for accept/" header assert_equals: Preflight request has been made expected "1" but got "0"
FAIL Need CORS-preflight for accept/012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678 header assert_equals: Preflight request has been made expected "1" but got "0"
FAIL Need CORS-preflight for accept-language/ header assert_equals: Preflight request has been made expected "1" but got "0"
FAIL Need CORS-preflight for accept-language/@ header assert_equals: Preflight request has been made expected "1" but got "0"
FAIL Need CORS-preflight for content-language/ header assert_equals: Preflight request has been made expected "1" but got "0"
FAIL Need CORS-preflight for content-language/@ header assert_equals: Preflight request has been made expected "1" but got "0"
PASS Need CORS-preflight for content-type/text/html header
FAIL Need CORS-preflight for content-type/text/plain; long=0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901 header assert_equals: Preflight request has been made expected "1" but got "0"
PASS Need CORS-preflight for test/hi header
Harness: the test ran to completion.
This is a testharness.js-based test.
PASS Loading data…
FAIL Need CORS-preflight for accept/" header assert_equals: Preflight request has been made expected "1" but got "0"
FAIL Need CORS-preflight for accept/012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678 header assert_equals: Preflight request has been made expected "1" but got "0"
FAIL Need CORS-preflight for accept-language/ header assert_equals: Preflight request has been made expected "1" but got "0"
FAIL Need CORS-preflight for accept-language/@ header assert_equals: Preflight request has been made expected "1" but got "0"
FAIL Need CORS-preflight for content-language/ header assert_equals: Preflight request has been made expected "1" but got "0"
FAIL Need CORS-preflight for content-language/@ header assert_equals: Preflight request has been made expected "1" but got "0"
PASS Need CORS-preflight for content-type/text/html header
FAIL Need CORS-preflight for content-type/text/plain; long=0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901 header assert_equals: Preflight request has been made expected "1" but got "0"
PASS Need CORS-preflight for test/hi header
Harness: the test ran to completion.
This is a testharness.js-based test.
PASS Loading data…
FAIL "no-cors" Headers object cannot have accept/" as header assert_false: expected false got true
FAIL "no-cors" Headers object cannot have accept/012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678 as header assert_false: expected false got true
FAIL "no-cors" Headers object cannot have accept-language/ as header assert_false: expected false got true
FAIL "no-cors" Headers object cannot have accept-language/@ as header assert_false: expected false got true
FAIL "no-cors" Headers object cannot have content-language/ as header assert_false: expected false got true
FAIL "no-cors" Headers object cannot have content-language/@ as header assert_false: expected false got true
PASS "no-cors" Headers object cannot have content-type/text/html as header
FAIL "no-cors" Headers object cannot have content-type/text/plain; long=0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901 as header assert_false: expected false got true
PASS "no-cors" Headers object cannot have test/hi as header
FAIL "no-cors" Headers object cannot have dpr/2 as header assert_false: expected false got true
PASS "no-cors" Headers object cannot have downlink/1 as header
FAIL "no-cors" Headers object cannot have save-data/on as header assert_false: expected false got true
FAIL "no-cors" Headers object cannot have viewport-width/100 as header assert_false: expected false got true
FAIL "no-cors" Headers object cannot have width/100 as header assert_false: expected false got true
Harness: the test ran to completion.
......@@ -128,13 +128,6 @@ void FetchHeaderList::ClearList() {
header_list_.clear();
}
bool FetchHeaderList::ContainsNonCORSSafelistedHeader() const {
return std::any_of(
header_list_.cbegin(), header_list_.cend(), [](const Header& header) {
return !CORS::IsCORSSafelistedHeader(header.first, header.second);
});
}
Vector<FetchHeaderList::Header> FetchHeaderList::SortAndCombine() const {
// https://fetch.spec.whatwg.org/#concept-header-list-sort-and-combine
// "To sort and combine a header list (|list|), run these steps:
......
......@@ -41,7 +41,6 @@ class CORE_EXPORT FetchHeaderList final
bool Has(const String&) const;
void ClearList();
bool ContainsNonCORSSafelistedHeader() const;
Vector<Header> SortAndCombine() const;
const std::multimap<String, String, ByteCaseInsensitiveCompare>& List()
......@@ -67,8 +66,7 @@ class CORE_EXPORT FetchHeaderList final
// This would cause FetchHeaderList::size() to have to manually
// iterate through all keys and vectors in the HashMap. Similarly,
// list() would require callers to manually iterate through the
// HashMap's keys and value vector, and so would
// ContainsNonCORSSafelistedHeader().
// HashMap's keys and value vector.
std::multimap<String, String, ByteCaseInsensitiveCompare> header_list_;
};
......
......@@ -117,26 +117,6 @@ TEST(FetchHeaderListTest, Contains) {
EXPECT_FALSE(headerList->Has("X-Bar"));
}
TEST(FetchHeaderListTest, ContainsNonCORSSafelistedHeader) {
FetchHeaderList* headerList = FetchHeaderList::Create();
EXPECT_FALSE(headerList->ContainsNonCORSSafelistedHeader());
headerList->Append("Host", "foobar");
headerList->Append("X-Foo", "bar");
EXPECT_TRUE(headerList->ContainsNonCORSSafelistedHeader());
headerList->ClearList();
headerList->Append("ConTenT-TyPe", "text/plain");
headerList->Append("content-type", "application/xml");
headerList->Append("X-Foo", "bar");
EXPECT_TRUE(headerList->ContainsNonCORSSafelistedHeader());
headerList->ClearList();
headerList->Append("ConTenT-TyPe", "multipart/form-data");
headerList->Append("Accept", "xyz");
EXPECT_FALSE(headerList->ContainsNonCORSSafelistedHeader());
}
TEST(FetchHeaderListTest, SortAndCombine) {
FetchHeaderList* headerList = FetchHeaderList::Create();
EXPECT_TRUE(headerList->SortAndCombine().IsEmpty());
......
......@@ -98,9 +98,9 @@ void Headers::append(const String& name,
if (guard_ == kRequestGuard && CORS::IsForbiddenHeaderName(name))
return;
// "5. Otherwise, if guard is |request-no-CORS| and |name|/|value| is not a
// CORS-safelisted header, return."
// no-CORS-safelisted header, return."
if (guard_ == kRequestNoCORSGuard &&
!CORS::IsCORSSafelistedHeader(name, normalized_value)) {
!CORS::IsNoCORSSafelistedHeader(name, normalized_value)) {
return;
}
// "6. Otherwise, if guard is |response| and |name| is a forbidden response
......@@ -130,9 +130,9 @@ void Headers::remove(const String& name, ExceptionState& exception_state) {
if (guard_ == kRequestGuard && CORS::IsForbiddenHeaderName(name))
return;
// "4. Otherwise, if guard is |request-no-CORS| and |name|/`invalid` is not
// a CORS-safelisted header, return."
// a no-CORS-safelisted header, return."
if (guard_ == kRequestNoCORSGuard &&
!CORS::IsCORSSafelistedHeader(name, "invalid")) {
!CORS::IsNoCORSSafelistedHeader(name, "invalid")) {
return;
}
// "5. Otherwise, if guard is |response| and |name| is a forbidden response
......@@ -198,9 +198,9 @@ void Headers::set(const String& name,
if (guard_ == kRequestGuard && CORS::IsForbiddenHeaderName(name))
return;
// "5. Otherwise, if guard is |request-no-CORS| and |name|/|value| is not a
// CORS-safelisted header, return."
// no-CORS-safelisted header, return."
if (guard_ == kRequestNoCORSGuard &&
!CORS::IsCORSSafelistedHeader(name, normalized_value)) {
!CORS::IsNoCORSSafelistedHeader(name, normalized_value)) {
return;
}
// "6. Otherwise, if guard is |response| and |name| is a forbidden response
......
......@@ -79,20 +79,14 @@ namespace {
// Fetch API Spec: https://fetch.spec.whatwg.org/#cors-preflight-fetch-0
AtomicString CreateAccessControlRequestHeadersHeader(
const HTTPHeaderMap& headers) {
Vector<String> filtered_headers;
for (const auto& header : headers) {
// Exclude CORS-safelisted headers.
if (CORS::IsCORSSafelistedHeader(header.key, header.value))
continue;
// Calling a deprecated function, but eventually this function,
// |CreateAccessControlRequestHeadersHeader| will be removed.
// When the request is from a Worker, referrer header was added by
// WorkerThreadableLoader. But it should not be added to
// Access-Control-Request-Headers header.
if (DeprecatedEqualIgnoringCase(header.key, "referer"))
continue;
filtered_headers.push_back(header.key.DeprecatedLower());
}
Vector<String> filtered_headers = CORS::CORSUnsafeRequestHeaderNames(headers);
// FetchManager may add a "referer" header.
// TODO(yhirano): Remove this.
auto it = filtered_headers.Find("referer");
if (it != kNotFound)
filtered_headers.EraseAt(it);
if (!filtered_headers.size())
return g_null_atom;
......
......@@ -232,6 +232,26 @@ bool IsCORSSafelistedHeader(const String& name, const String& value) {
std::string(utf8_value.data(), utf8_value.length()));
}
bool IsNoCORSSafelistedHeader(const String& name, const String& value) {
DCHECK(!name.IsNull());
DCHECK(!value.IsNull());
return network::cors::IsNoCORSSafelistedHeader(WebString(name).Latin1(),
WebString(value).Latin1());
}
Vector<String> CORSUnsafeRequestHeaderNames(const HTTPHeaderMap& headers) {
net::HttpRequestHeaders::HeaderVector in;
for (const auto& entry : headers) {
in.push_back(net::HttpRequestHeaders::HeaderKeyValuePair(
WebString(entry.key).Latin1(), WebString(entry.value).Latin1()));
}
Vector<String> header_names;
for (const auto& name : network::cors::CORSUnsafeRequestHeaderNames(in))
header_names.push_back(WebString::FromLatin1(name));
return header_names;
}
bool IsForbiddenHeaderName(const String& name) {
CString utf8_name = name.Utf8();
return network::cors::IsForbiddenHeader(
......@@ -239,21 +259,20 @@ bool IsForbiddenHeaderName(const String& name) {
}
bool ContainsOnlyCORSSafelistedHeaders(const HTTPHeaderMap& header_map) {
for (const auto& header : header_map) {
if (!IsCORSSafelistedHeader(header.key, header.value))
return false;
}
return true;
Vector<String> header_names = CORSUnsafeRequestHeaderNames(header_map);
return header_names.IsEmpty();
}
bool ContainsOnlyCORSSafelistedOrForbiddenHeaders(
const HTTPHeaderMap& header_map) {
for (const auto& header : header_map) {
if (!IsCORSSafelistedHeader(header.key, header.value) &&
!IsForbiddenHeaderName(header.key))
return false;
const HTTPHeaderMap& headers) {
Vector<String> header_names;
net::HttpRequestHeaders::HeaderVector in;
for (const auto& entry : headers) {
in.push_back(net::HttpRequestHeaders::HeaderKeyValuePair(
WebString(entry.key).Latin1(), WebString(entry.value).Latin1()));
}
return true;
return network::cors::CORSUnsafeNotForbiddenRequestHeaderNames(in).empty();
}
bool IsOkStatus(int status) {
......
......@@ -11,6 +11,7 @@
#include "services/network/public/mojom/fetch_api.mojom-shared.h"
#include "third_party/blink/renderer/platform/platform_export.h"
#include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
#include "third_party/blink/renderer/platform/wtf/vector.h"
namespace blink {
......@@ -89,6 +90,10 @@ PLATFORM_EXPORT bool IsCORSSafelistedMethod(const String& method);
PLATFORM_EXPORT bool IsCORSSafelistedContentType(const String&);
PLATFORM_EXPORT bool IsCORSSafelistedHeader(const String& name,
const String& value);
PLATFORM_EXPORT bool IsNoCORSSafelistedHeader(const String& name,
const String& value);
PLATFORM_EXPORT Vector<String> CORSUnsafeRequestHeaderNames(
const HTTPHeaderMap& headers);
PLATFORM_EXPORT bool IsForbiddenHeaderName(const String& name);
PLATFORM_EXPORT bool ContainsOnlyCORSSafelistedHeaders(const HTTPHeaderMap&);
PLATFORM_EXPORT bool ContainsOnlyCORSSafelistedOrForbiddenHeaders(
......
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