Have net::CanonicalCookie::Create return nullptr if the resulting cookie

is non-canonical.

The function documentation already states this is what the function
should do, but the current code only DCHECKs isCanonical. In production
it is still possible for non-canonical cookies to be sent to the browser
process this way since the DCHECK is not enabled.

Bug: 1162915
Change-Id: I6046477eeb3d042fee65c3e1996fc487795b3cc7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2614366Reviewed-by: Martin Šrámek <msramek@chromium.org>
Reviewed-by: Lily Chen <chlily@chromium.org>
Reviewed-by: Denis Kuznetsov [CET] <antrim@chromium.org>
Commit-Queue: Dylan Cutler <dylancutler@google.com>
Cr-Commit-Position: refs/heads/master@{#841642}

chrome/browser/ui/hats/hats_survey_status_checker.cc

@@ -40,6 +40,7 @@ HatsSurveyStatusChecker::HatsSurveyStatusChecker(Profile* profile) {
   GURL cookie_url = GURL("https://www.google.com");
   auto survey_cookie = net::CanonicalCookie::Create(
       cookie_url, "PAIDCONTENT=0", base::Time::Now(), base::nullopt);
+  DCHECK(survey_cookie);
   network::mojom::CookieManager* cookie_manager =
       GetStoragePartition()->GetCookieManagerForBrowserProcess();
   cookie_manager->SetCanonicalCookie(*survey_cookie, cookie_url,

chrome/browser/ui/webui/chromeos/login/online_login_helper.cc

@@ -74,6 +74,8 @@ void SetCookieForPartition(
   std::unique_ptr<net::CanonicalCookie> cc(net::CanonicalCookie::Create(
       gaia_url, gaps_cookie_value, base::Time::Now(),
       base::nullopt /* server_time */));
+  if (!cc)
+    return;
 
   const net::CookieOptions options = net::CookieOptions::MakeAllInclusive();
   partition->GetCookieManagerForBrowserProcess()->SetCanonicalCookie(
@@ -244,4 +246,4 @@ void OnlineLoginHelper::OnGetCookiesForCompleteAuthentication(
   std::move(complete_login_callback_).Run(user_context);
 }
 
-}  // namespace chromeos
\ No newline at end of file
+}  // namespace chromeos

net/cookies/canonical_cookie.cc

@@ -428,14 +428,11 @@ std::unique_ptr<CanonicalCookie> CanonicalCookie::Create(
   if (parsed_cookie.IsSameParty())
     base::UmaHistogramBoolean("Cookie.IsSamePartyValid", is_same_party_valid);
 
-  // TODO(chlily): Log metrics.
   if (!status->IsInclude())
     return nullptr;
 
   CookieSameSiteString samesite_string = CookieSameSiteString::kUnspecified;
   CookieSameSite samesite = parsed_cookie.SameSite(&samesite_string);
-  RecordCookieSameSiteAttributeValueHistogram(samesite_string,
-                                              parsed_cookie.IsSameParty());
 
   CookieSourceScheme source_scheme = url.SchemeIsCryptographic()
                                          ? CookieSourceScheme::kSecure
@@ -449,9 +446,16 @@ std::unique_ptr<CanonicalCookie> CanonicalCookie::Create(
       parsed_cookie.IsHttpOnly(), samesite, parsed_cookie.Priority(),
       parsed_cookie.IsSameParty(), source_scheme, source_port));
 
-  DCHECK(cc->IsCanonical());
-
   // TODO(chlily): Log metrics.
+  if (!cc->IsCanonical()) {
+    status->AddExclusionReason(
+        net::CookieInclusionStatus::EXCLUDE_FAILURE_TO_STORE);
+    return nullptr;
+  }
+
+  RecordCookieSameSiteAttributeValueHistogram(samesite_string,
+                                              parsed_cookie.IsSameParty());
+
   return cc;
 }
 

net/cookies/canonical_cookie_unittest.cc

@@ -194,6 +194,21 @@ TEST(CanonicalCookie, CreationCornerCases) {
   cookie = CanonicalCookie::Create(GURL("http://fool/;/"), "*", creation_time,
                                    server_time);
   EXPECT_TRUE(cookie.get());
+
+  // Control characters in name or value.
+  CookieInclusionStatus status;
+  cookie =
+      CanonicalCookie::Create(GURL("http://www.example.com/test/foo.html"),
+                              "\b=foo", creation_time, server_time, &status);
+  EXPECT_FALSE(cookie.get());
+  EXPECT_TRUE(status.HasExclusionReason(
+      CookieInclusionStatus::ExclusionReason::EXCLUDE_FAILURE_TO_STORE));
+  cookie =
+      CanonicalCookie::Create(GURL("http://www.example.com/test/foo.html"),
+                              "bar=\b", creation_time, server_time, &status);
+  EXPECT_FALSE(cookie.get());
+  EXPECT_TRUE(status.HasExclusionReason(
+      CookieInclusionStatus::ExclusionReason::EXCLUDE_FAILURE_TO_STORE));
 }
 
 TEST(CanonicalCookieTest, Create) {