Skip to content

Commit

Permalink
Revert "Strengthen requirements on CORS-safelisted request-headers"
Browse files Browse the repository at this point in the history 
This reverts commit 074455d.

Reason for revert: Suspected of causing check failures across a variety of tests on the Android CFI bot. See https://crbug.com/881538 for more details.

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 whatwg/fetch#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}

TBR=toyoshim@chromium.org,yhirano@chromium.org

Change-Id: I9952df291ff0aeaab0b50c6cff3de418b2272e71
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 824130, 881538 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Reviewed-on: https://chromium-review.googlesource.com/1211958
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589323}
  • Loading branch information
@justindonnelly
justindonnelly authored and Commit Bot committed Sep 6, 2018
1 parent 3541fbe commit b39baca
Show file tree
Hide file tree
Showing 17 changed files with 158 additions and 438 deletions.
10 changes: 7 additions & 3 deletions services/network/cors/cors_url_loader.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,13 @@ bool NeedsPreflight(const ResourceRequest& request) {
if (!IsCORSSafelistedMethod(request.method))
return true;

return !CORSUnsafeNotForbiddenRequestHeaderNames(
request.headers.GetHeaderVector())
.empty();
for (const auto& header : request.headers.GetHeaderVector()) {
if (!IsCORSSafelistedHeader(header.key, header.value) &&
!IsForbiddenHeader(header.key)) {
return true;
}
}
return false;
}

} // namespace
Expand Down
17 changes: 12 additions & 5 deletions services/network/cors/preflight_controller.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,18 @@ base::Optional<std::string> GetHeaderString(
// - byte-lowercased
std::string CreateAccessControlRequestHeadersHeader(
const net::HttpRequestHeaders& headers) {
// 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());
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));
}
if (filtered_headers.empty())
return std::string();

Expand Down
81 changes: 0 additions & 81 deletions services/network/public/cpp/cors/cors.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -354,10 +354,6 @@ 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
Expand Down Expand Up @@ -399,89 +395,12 @@ 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"};
Expand Down
22 changes: 0 additions & 22 deletions services/network/public/cpp/cors/cors.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,9 @@
#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"
Expand Down Expand Up @@ -119,26 +117,6 @@ 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.
Expand Down
Loading

0 comments on commit b39baca

Please sign in to comment.