From 1df1a4d9a77f27e6065839562b95fefa12b1def4 Mon Sep 17 00:00:00 2001 From: sbingler Date: Tue, 16 Aug 2022 16:42:16 +0000 Subject: [Backport] CVE-2022-2860: Insufficient policy enforcement in Cookies Cherry-pick of patch originally reviewed on https://chromium-review.googlesource.com/c/chromium/src/+/3803167: Don't allow cookies with hidden cookie prefixes Prevent the creation of any cookies that have an empty name field and whose value impersonates a cookie name prefix. This will also delete any previously stored cookies that meet the conditions by causing them to fail their IsCanonical() check. (cherry picked from commit f9580905b45edb8dfe7da6cd5f26421ab2b5c285) Bug: 1345193 Change-Id: I7e1adef3391bb7caee183204bb609cd63bcdaea7 Commit-Queue: Steven Bingler Cr-Original-Commit-Position: refs/heads/main@{#1028000} Owners-Override: Michael Ershov Commit-Queue: Roger Felipe Zanoni da Silva Reviewed-by: Steven Bingler Reviewed-by: Michael Ershov Cr-Commit-Position: refs/branch-heads/4664@{#1685} Cr-Branched-From: 24dc4ee75e01a29d390d43c9c264372a169273a7-refs/heads/main@{#929512} Reviewed-by: Michal Klocek --- chromium/net/cookies/canonical_cookie.cc | 48 ++++++++++++++++++++++++++++++++ chromium/net/cookies/canonical_cookie.h | 5 ++++ 2 files changed, 53 insertions(+) diff --git a/chromium/net/cookies/canonical_cookie.cc b/chromium/net/cookies/canonical_cookie.cc index e9d0122b6c2..f15d20934f6 100644 --- a/chromium/net/cookies/canonical_cookie.cc +++ b/chromium/net/cookies/canonical_cookie.cc @@ -57,6 +57,7 @@ #include "base/metrics/histogram_macros.h" #include "base/strings/strcat.h" #include "base/strings/string_number_conversions.h" +#include "base/strings/string_piece.h" #include "base/strings/string_util.h" #include "base/strings/stringprintf.h" #include "net/base/features.h" @@ -504,6 +505,11 @@ std::unique_ptr CanonicalCookie::Create( CookiePrefix prefix = GetCookiePrefix(parsed_cookie.Name()); bool is_cookie_prefix_valid = IsCookiePrefixValid(prefix, url, parsed_cookie); RecordCookiePrefixMetrics(prefix, is_cookie_prefix_valid); + + if (parsed_cookie.Name() == "") { + is_cookie_prefix_valid = !HasHiddenPrefixName(parsed_cookie.Value()); + } + if (!is_cookie_prefix_valid) { DVLOG(net::cookie_util::kVlogSetCookies) << "Create() failed because the cookie violated prefix rules."; @@ -657,6 +663,11 @@ std::unique_ptr CanonicalCookie::CreateSanitizedCookie( net::CookieInclusionStatus::EXCLUDE_INVALID_PREFIX); } + if (name == "" && HasHiddenPrefixName(value)) { + status->AddExclusionReason( + net::CookieInclusionStatus::EXCLUDE_INVALID_PREFIX); + } + if (!IsCookieSamePartyValid(same_party, secure, same_site)) { status->AddExclusionReason( net::CookieInclusionStatus::EXCLUDE_INVALID_SAMEPARTY); @@ -1313,6 +1324,9 @@ bool CanonicalCookie::IsCanonical() const { break; } + if (name_ == "" && HasHiddenPrefixName(value_)) + return false; + if (!IsCookieSamePartyValid(same_party_, secure_, same_site_)) return false; @@ -1435,6 +1449,40 @@ CookieEffectiveSameSite CanonicalCookie::GetEffectiveSameSite( } } +// static +bool CanonicalCookie::HasHiddenPrefixName( + const base::StringPiece cookie_value) { + // Skip BWS as defined by HTTPSEM as SP or HTAB (0x20 or 0x9). + base::StringPiece value_without_BWS = + base::TrimString(cookie_value, " \t", base::TRIM_LEADING); + + const base::StringPiece host_prefix = "__Host-"; + + // Compare the value to the host_prefix. + if (base::StartsWith(value_without_BWS, host_prefix)) { + // The prefix matches, now check if the value string contains a subsequent + // '='. + if (value_without_BWS.find_first_of('=', host_prefix.size()) != + base::StringPiece::npos) { + // This value contains a hidden prefix name. + return true; + } + return false; + } + + // Do a similar check for the secure prefix + const base::StringPiece secure_prefix = "__Secure-"; + + if (base::StartsWith(value_without_BWS, secure_prefix)) { + if (value_without_BWS.find_first_of('=', secure_prefix.size()) != + base::StringPiece::npos) { + return true; + } + } + + return false; +} + bool CanonicalCookie::IsRecentlyCreated(base::TimeDelta age_threshold) const { return (base::Time::Now() - creation_date_) <= age_threshold; } diff --git a/chromium/net/cookies/canonical_cookie.h b/chromium/net/cookies/canonical_cookie.h index a8dd4c7a704..be9e7760f74 100644 --- a/chromium/net/cookies/canonical_cookie.h +++ b/chromium/net/cookies/canonical_cookie.h @@ -11,6 +11,7 @@ #include #include "base/gtest_prod_util.h" +#include "base/strings/string_piece.h" #include "base/time/time.h" #include "net/base/features.h" #include "net/base/net_export.h" @@ -394,6 +395,7 @@ class NET_EXPORT CanonicalCookie { private: FRIEND_TEST_ALL_PREFIXES(CanonicalCookieTest, TestPrefixHistograms); + FRIEND_TEST_ALL_PREFIXES(CanonicalCookieTest, TestHasHiddenPrefixName); // This constructor does not validate or canonicalize their inputs; // the resulting CanonicalCookies should not be relied on to be canonical @@ -457,6 +459,9 @@ class NET_EXPORT CanonicalCookie { CookieEffectiveSameSite GetEffectiveSameSite( CookieAccessSemantics access_semantics) const; + // Checks for values that could be misinterpreted as a cookie name prefix. + static bool HasHiddenPrefixName(const base::StringPiece cookie_value); + // Returns whether the cookie was created at most |age_threshold| ago. bool IsRecentlyCreated(base::TimeDelta age_threshold) const; -- cgit v1.2.1