From 085efc288be9fb6f1b243a2e790b49db34d208d4 Mon Sep 17 00:00:00 2001 From: Takashi Toyoshima Date: Fri, 21 Oct 2022 08:30:12 +0000 Subject: [Backport] CVE-2023-0141: Insufficient policy enforcement in CORS (1/2) Cherry-pick of patch originally reviewed on https://chromium-review.googlesource.com/c/chromium/src/+/3963942: Net: API change to take a value to evaluate safe headers This patch changes the net::HttpUtil::IsSafeHeader() API to take a header value in addition to the name. This API change is needed in a coming change, and this CL is a preparation to avoid mixing the core change with mechanical changes spread around the code base. This also affects blink::cors::IsForbiddenHeaderName(). Bug: 1362331 Change-Id: I517799b96c3a045c336d2a509691bb8cc1f173e0 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3963942 Reviewed-by: Scott Violet Reviewed-by: Mike West Commit-Queue: Takashi Toyoshima Reviewed-by: Matt Menke Cr-Commit-Position: refs/heads/main@{#1062009} Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/454295 Reviewed-by: Michal Klocek --- .../browser/extensions/api/downloads/downloads_api.cc | 10 +++++----- chromium/net/android/android_http_util.cc | 6 +++--- chromium/net/http/http_util.cc | 2 +- chromium/net/http/http_util.h | 10 +++++----- chromium/services/network/cors/cors_util.cc | 2 +- chromium/services/network/websocket.cc | 2 +- chromium/third_party/blink/renderer/core/fetch/headers.cc | 14 +++++++------- .../renderer/core/loader/web_associated_url_loader_impl.cc | 4 ++-- .../blink/renderer/core/xmlhttprequest/xml_http_request.cc | 5 +++-- .../blink/renderer/platform/loader/cors/cors.cc | 4 ++-- .../third_party/blink/renderer/platform/loader/cors/cors.h | 3 ++- 11 files changed, 32 insertions(+), 30 deletions(-) diff --git a/chromium/chrome/browser/extensions/api/downloads/downloads_api.cc b/chromium/chrome/browser/extensions/api/downloads/downloads_api.cc index bdbd9a08659..22a466a984d 100644 --- a/chromium/chrome/browser/extensions/api/downloads/downloads_api.cc +++ b/chromium/chrome/browser/extensions/api/downloads/downloads_api.cc @@ -1032,19 +1032,19 @@ ExtensionFunction::ResponseAction DownloadsDownloadFunction::Run() { download_params->set_prompt(*options.save_as); if (options.headers.get()) { - for (const downloads::HeaderNameValuePair& name_value : *options.headers) { - if (!net::HttpUtil::IsValidHeaderName(name_value.name)) { + for (const downloads::HeaderNameValuePair& header : *options.headers) { + if (!net::HttpUtil::IsValidHeaderName(header.name)) { return RespondNow(Error(download_extension_errors::kInvalidHeaderName)); } - if (!net::HttpUtil::IsSafeHeader(name_value.name)) { + if (!net::HttpUtil::IsSafeHeader(header.name, header.value)) { return RespondNow( Error(download_extension_errors::kInvalidHeaderUnsafe)); } - if (!net::HttpUtil::IsValidHeaderValue(name_value.value)) { + if (!net::HttpUtil::IsValidHeaderValue(header.value)) { return RespondNow( Error(download_extension_errors::kInvalidHeaderValue)); } - download_params->add_request_header(name_value.name, name_value.value); + download_params->add_request_header(header.name, header.value); } } diff --git a/chromium/net/android/android_http_util.cc b/chromium/net/android/android_http_util.cc index 83f8831548f..0a632c4ddb2 100644 --- a/chromium/net/android/android_http_util.cc +++ b/chromium/net/android/android_http_util.cc @@ -20,9 +20,9 @@ jboolean JNI_HttpUtil_IsAllowedHeader( std::string header_name(ConvertJavaStringToUTF8(env, j_header_name)); std::string header_value(ConvertJavaStringToUTF8(env, j_header_value)); - return HttpUtil::IsValidHeaderName(header_name) - && HttpUtil::IsSafeHeader(header_name) - && HttpUtil::IsValidHeaderValue(header_value); + return HttpUtil::IsValidHeaderName(header_name) && + HttpUtil::IsSafeHeader(header_name, header_value) && + HttpUtil::IsValidHeaderValue(header_value); } } // namespace net diff --git a/chromium/net/http/http_util.cc b/chromium/net/http/http_util.cc index 0978147da37..40ebbb4a3e9 100644 --- a/chromium/net/http/http_util.cc +++ b/chromium/net/http/http_util.cc @@ -326,7 +326,7 @@ bool HttpUtil::IsMethodIdempotent(base::StringPiece method) { } // static -bool HttpUtil::IsSafeHeader(base::StringPiece name) { +bool HttpUtil::IsSafeHeader(base::StringPiece name, base::StringPiece value) { if (base::StartsWith(name, "proxy-", base::CompareCase::INSENSITIVE_ASCII) || base::StartsWith(name, "sec-", base::CompareCase::INSENSITIVE_ASCII)) return false; diff --git a/chromium/net/http/http_util.h b/chromium/net/http/http_util.h index 65f64279020..86bf5c82583 100644 --- a/chromium/net/http/http_util.h +++ b/chromium/net/http/http_util.h @@ -96,11 +96,11 @@ class NET_EXPORT HttpUtil { // RFC 7231). static bool IsMethodIdempotent(base::StringPiece method); - // Returns true if it is safe to allow users and scripts to specify the header - // named |name|. Returns true for headers not in the list at - // https://fetch.spec.whatwg.org/#forbidden-header-name. Does not check header - // validity. - static bool IsSafeHeader(base::StringPiece name); + // Returns true if it is safe to allow users and scripts to specify a header + // with a given |name| and |value|. + // See https://fetch.spec.whatwg.org/#forbidden-request-header. + // Does not check header validity. + static bool IsSafeHeader(base::StringPiece name, base::StringPiece value); // Returns true if |name| is a valid HTTP header name. static bool IsValidHeaderName(base::StringPiece name); diff --git a/chromium/services/network/cors/cors_util.cc b/chromium/services/network/cors/cors_util.cc index 14fb9d81897..761c6a0de1a 100644 --- a/chromium/services/network/cors/cors_util.cc +++ b/chromium/services/network/cors/cors_util.cc @@ -21,7 +21,7 @@ std::vector CorsUnsafeNotForbiddenRequestHeaderNames( size_t safe_list_value_size = 0; for (const auto& header : headers) { - if (!net::HttpUtil::IsSafeHeader(header.key)) + if (!net::HttpUtil::IsSafeHeader(header.key, header.value)) continue; const std::string name = base::ToLowerASCII(header.key); diff --git a/chromium/services/network/websocket.cc b/chromium/services/network/websocket.cc index 1b4da1a289b..06b041638c4 100644 --- a/chromium/services/network/websocket.cc +++ b/chromium/services/network/websocket.cc @@ -621,7 +621,7 @@ void WebSocket::AddChannel( for (const auto& header : additional_headers) { if (net::HttpUtil::IsValidHeaderName(header->name) && net::HttpUtil::IsValidHeaderValue(header->value) && - (net::HttpUtil::IsSafeHeader(header->name) || + (net::HttpUtil::IsSafeHeader(header->name, header->value) || base::EqualsCaseInsensitiveASCII( header->name, net::HttpRequestHeaders::kUserAgent) || base::EqualsCaseInsensitiveASCII(header->name, diff --git a/chromium/third_party/blink/renderer/core/fetch/headers.cc b/chromium/third_party/blink/renderer/core/fetch/headers.cc index 3f4291baad2..b336fbd815e 100644 --- a/chromium/third_party/blink/renderer/core/fetch/headers.cc +++ b/chromium/third_party/blink/renderer/core/fetch/headers.cc @@ -107,7 +107,7 @@ void Headers::append(ScriptState* script_state, } // "4. Otherwise, if guard is |request| and |name| is a forbidden header // name, return." - if (guard_ == kRequestGuard && cors::IsForbiddenHeaderName(name)) + if (guard_ == kRequestGuard && cors::IsForbiddenRequestHeader(name, value)) return; // 5. Otherwise, if guard is |request-no-cors|: if (guard_ == kRequestNoCorsGuard) { @@ -164,9 +164,9 @@ void Headers::remove(ScriptState* script_state, UseCounter::Count(execution_context, WebFeature::kFetchSetCookieInRequestGuardedHeaders); } - // "3. Otherwise, if guard is |request| and |name| is a forbidden header - // name, return." - if (guard_ == kRequestGuard && cors::IsForbiddenHeaderName(name)) + // "3. Otherwise, if guard is |request| and (|name|, '') is a forbidden + // request header, return." + if (guard_ == kRequestGuard && cors::IsForbiddenRequestHeader(name, "")) return; // "4. Otherwise, if the context object’s guard is |request-no-cors|, |name| // is not a no-CORS-safelisted request-header name, and |name| is not a @@ -248,9 +248,9 @@ void Headers::set(ScriptState* script_state, UseCounter::Count(execution_context, WebFeature::kFetchSetCookieInRequestGuardedHeaders); } - // "4. Otherwise, if guard is |request| and |name| is a forbidden header - // name, return." - if (guard_ == kRequestGuard && cors::IsForbiddenHeaderName(name)) + // "4. Otherwise, if guard is |request| and (|name|, |value|) is a forbidden + // request header, return." + if (guard_ == kRequestGuard && cors::IsForbiddenRequestHeader(name, value)) return; // "5. Otherwise, if guard is |request-no-CORS| and |name|/|value| is not a // no-CORS-safelisted header, return." diff --git a/chromium/third_party/blink/renderer/core/loader/web_associated_url_loader_impl.cc b/chromium/third_party/blink/renderer/core/loader/web_associated_url_loader_impl.cc index 4de2d8c49cb..21f11a9a76f 100644 --- a/chromium/third_party/blink/renderer/core/loader/web_associated_url_loader_impl.cc +++ b/chromium/third_party/blink/renderer/core/loader/web_associated_url_loader_impl.cc @@ -88,7 +88,7 @@ class HTTPRequestHeaderValidator : public WebHTTPHeaderVisitor { void HTTPRequestHeaderValidator::VisitHeader(const WebString& name, const WebString& value) { is_safe_ = is_safe_ && IsValidHTTPToken(name) && - !cors::IsForbiddenHeaderName(name) && + !cors::IsForbiddenRequestHeader(name, value) && IsValidHTTPHeaderValue(value); } @@ -377,7 +377,7 @@ void WebAssociatedURLLoaderImpl::LoadAsynchronously( // consult it separately, if set. if (request.ReferrerString() != blink::WebString(Referrer::ClientReferrerString())) { - DCHECK(cors::IsForbiddenHeaderName("Referer")); + DCHECK(cors::IsForbiddenRequestHeader("Referer", "")); // `Referer` is a forbidden header name, so we must disallow this to // load. allow_load = false; diff --git a/chromium/third_party/blink/renderer/core/xmlhttprequest/xml_http_request.cc b/chromium/third_party/blink/renderer/core/xmlhttprequest/xml_http_request.cc index 69c35483a78..32b4a71dbb5 100644 --- a/chromium/third_party/blink/renderer/core/xmlhttprequest/xml_http_request.cc +++ b/chromium/third_party/blink/renderer/core/xmlhttprequest/xml_http_request.cc @@ -1391,9 +1391,10 @@ void XMLHttpRequest::setRequestHeader(const AtomicString& name, return; } - // "5. Terminate these steps if |name| is a forbidden header name." + // "5. Terminate these steps if (|name|, |value|) is a forbidden request + // header." // No script (privileged or not) can set unsafe headers. - if (cors::IsForbiddenHeaderName(name)) { + if (cors::IsForbiddenRequestHeader(name, value)) { LogConsoleError(GetExecutionContext(), "Refused to set unsafe header \"" + name + "\""); return; diff --git a/chromium/third_party/blink/renderer/platform/loader/cors/cors.cc b/chromium/third_party/blink/renderer/platform/loader/cors/cors.cc index 4e3c4c8017c..5f705e406c2 100644 --- a/chromium/third_party/blink/renderer/platform/loader/cors/cors.cc +++ b/chromium/third_party/blink/renderer/platform/loader/cors/cors.cc @@ -148,8 +148,8 @@ PLATFORM_EXPORT Vector PrivilegedNoCorsHeaderNames() { return header_names; } -bool IsForbiddenHeaderName(const String& name) { - return !net::HttpUtil::IsSafeHeader(name.Latin1()); +bool IsForbiddenRequestHeader(const String& name, const String& value) { + return !net::HttpUtil::IsSafeHeader(name.Latin1(), value.Latin1()); } bool ContainsOnlyCorsSafelistedHeaders(const HTTPHeaderMap& header_map) { diff --git a/chromium/third_party/blink/renderer/platform/loader/cors/cors.h b/chromium/third_party/blink/renderer/platform/loader/cors/cors.h index 1f1bdbc079d..c4eb638e741 100644 --- a/chromium/third_party/blink/renderer/platform/loader/cors/cors.h +++ b/chromium/third_party/blink/renderer/platform/loader/cors/cors.h @@ -39,7 +39,8 @@ PLATFORM_EXPORT bool IsNoCorsSafelistedHeader(const String& name, PLATFORM_EXPORT bool IsPrivilegedNoCorsHeaderName(const String& name); PLATFORM_EXPORT bool IsNoCorsSafelistedHeaderName(const String& name); PLATFORM_EXPORT Vector PrivilegedNoCorsHeaderNames(); -PLATFORM_EXPORT bool IsForbiddenHeaderName(const String& name); +PLATFORM_EXPORT bool IsForbiddenRequestHeader(const String& name, + const String& value); PLATFORM_EXPORT bool ContainsOnlyCorsSafelistedHeaders(const HTTPHeaderMap&); PLATFORM_EXPORT bool IsOkStatus(int status); -- cgit v1.2.1