Don't send SameSite=Lax cookies on cross-site, non-top-level requests

This fixes a bug which was sending SameSite=Lax cookies on cross-site,
*non*-top-level requests when the site-for-cookies was same-site with
the request URL. If the request is not strictly same-site, we should
only be sending Lax cookies when the site-for-cookies is same-site with
the request URL, *and* the request is a top-level navigation. Similarly
for accepting cookies set on responses.

This implements the fix behind a flag (default enabled) to allow
reverting to the old behavior if there is too much site breakage as a
result of the fix.

Bug: 1166211
Change-Id: I2cebf8011010903cd016d7d7c1a32bf84aa325ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2653663
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Commit-Queue: Lily Chen <chlily@chromium.org>
Cr-Commit-Position: refs/heads/master@{#851323}

content/browser/devtools/devtools_url_loader_interceptor.cc

@@ -1117,6 +1117,7 @@ void InterceptionJob::ProcessSetCookies(const net::HttpResponseHeaders& headers,
           create_loader_params_->request.url,
           create_loader_params_->request.site_for_cookies,
           create_loader_params_->request.request_initiator,
+          create_loader_params_->request.is_main_frame,
           should_treat_as_first_party));
 
   // |this| might be deleted here if |cookies| is empty!
@@ -1265,7 +1266,8 @@ void InterceptionJob::FetchCookies(
   options.set_same_site_cookie_context(
       net::cookie_util::ComputeSameSiteContextForRequest(
           request.method, request.url, request.site_for_cookies,
-          request.request_initiator, should_treat_as_first_party));
+          request.request_initiator, request.is_main_frame,
+          should_treat_as_first_party));
 
   cookie_manager_->GetCookieList(request.url, options, std::move(callback));
 }

net/base/features.cc

@@ -199,5 +199,8 @@ constexpr base::Feature kFirstPartySets{"FirstPartySets",
 const base::FeatureParam<bool> kFirstPartySetsIsDogfooder{
     &kFirstPartySets, "FirstPartySetsIsDogfooder", false};
 
+const base::Feature kSameSiteCookiesBugfix1166211{
+    "SameSiteCookiesBugfix1166211", base::FEATURE_ENABLED_BY_DEFAULT};
+
 }  // namespace features
 }  // namespace net

net/base/features.h

@@ -294,6 +294,15 @@ NET_EXPORT extern const base::Feature kFirstPartySets;
 // feature.
 NET_EXPORT extern const base::FeatureParam<bool> kFirstPartySetsIsDogfooder;
 
+// Controls whether the fix for crbug.com/1166211 is enabled. When this is
+// enabled, SameSite=Lax cookies may only be accessed for cross-site requests if
+// they are top-level navigations. When it is disabled, the (incorrect) previous
+// behavior that allows SameSite=Lax cookies on cross-site, non-top-level
+// requests if all frame ancestors are same-site with the request URL is used
+// instead. This fix is implemented behind a flag (kill switch) due to potential
+// compatibility risk.
+NET_EXPORT extern const base::Feature kSameSiteCookiesBugfix1166211;
+
 }  // namespace features
 }  // namespace net
 

net/cookies/cookie_util.cc

@@ -83,45 +83,76 @@ bool SaturatedTimeFromUTCExploded(const base::Time::Exploded& exploded,
   return false;
 }
 
+// This function consolidates the common logic for computing SameSite cookie
+// access context in various situations (HTTP vs JS; get vs set).
+//
+// `is_http` is whether the current cookie access request is associated with a
+// network request (as opposed to a non-HTTP API, i.e., JavaScript).
+//
+// `compute_schemefully` is whether the current computation is for a
+// schemeful_context, i.e. whether scheme should be considered when comparing
+// two sites.
+//
+// See documentation of `ComputeSameSiteContextForRequest` for explanations of
+// other parameters.
 ContextType ComputeSameSiteContext(const GURL& url,
                                    const SiteForCookies& site_for_cookies,
                                    const base::Optional<url::Origin>& initiator,
+                                   bool is_http,
+                                   bool is_main_frame_navigation,
                                    bool compute_schemefully) {
-  if (site_for_cookies.IsFirstPartyWithSchemefulMode(url,
-                                                     compute_schemefully)) {
+  bool site_for_cookies_is_same_site =
+      site_for_cookies.IsFirstPartyWithSchemefulMode(url, compute_schemefully);
+
+  // If the request is a main frame navigation, site_for_cookies must either be
+  // null (for opaque origins, e.g., data: origins) or same-site with the
+  // request URL (both schemefully and schemelessly), and the URL cannot be
+  // ws/wss (these schemes are not navigable).
+  DCHECK(!is_main_frame_navigation || site_for_cookies_is_same_site ||
+         site_for_cookies.IsNull());
+  DCHECK(!is_main_frame_navigation || !url.SchemeIsWSOrWSS());
+
+  if (site_for_cookies_is_same_site) {
     // Create a SiteForCookies object from the initiator so that we can reuse
     // IsFirstPartyWithSchemefulMode().
     if (!initiator ||
         SiteForCookies::FromOrigin(initiator.value())
             .IsFirstPartyWithSchemefulMode(url, compute_schemefully)) {
       return ContextType::SAME_SITE_STRICT;
-    } else {
-      return ContextType::SAME_SITE_LAX;
     }
+    // Preserve old behavior if the bugfix is disabled.
+    if (!base::FeatureList::IsEnabled(features::kSameSiteCookiesBugfix1166211))
+      return ContextType::SAME_SITE_LAX;
+
+    if (!is_http || is_main_frame_navigation)
+      return ContextType::SAME_SITE_LAX;
   }
   return ContextType::CROSS_SITE;
 }
 
 CookieOptions::SameSiteCookieContext ComputeSameSiteContextForSet(
     const GURL& url,
     const SiteForCookies& site_for_cookies,
-    bool force_ignore_site_for_cookies) {
-  if (force_ignore_site_for_cookies)
-    return CookieOptions::SameSiteCookieContext::MakeInclusiveForSet();
+    const base::Optional<url::Origin>& initiator,
+    bool is_http,
+    bool is_main_frame_navigation) {
+  CookieOptions::SameSiteCookieContext same_site_context;
 
-  // Schemeless check
-  if (!site_for_cookies.IsFirstPartyWithSchemefulMode(url, false)) {
-    return CookieOptions::SameSiteCookieContext(ContextType::CROSS_SITE,
-                                                ContextType::CROSS_SITE);
-  }
+  same_site_context.set_context(ComputeSameSiteContext(
+      url, site_for_cookies, initiator, is_http, is_main_frame_navigation,
+      false /* compute_schemefully */));
+  same_site_context.set_schemeful_context(ComputeSameSiteContext(
+      url, site_for_cookies, initiator, is_http, is_main_frame_navigation,
+      true /* compute_schemefully */));
 
-  // Schemeful check
-  if (!site_for_cookies.IsFirstPartyWithSchemefulMode(url, true)) {
-    return CookieOptions::SameSiteCookieContext(ContextType::SAME_SITE_LAX,
-                                                ContextType::CROSS_SITE);
-  }
+  // Setting any SameSite={Strict,Lax} cookie only requires a LAX context, so
+  // normalize any strictly same-site contexts to Lax for cookie writes.
+  if (same_site_context.context() == ContextType::SAME_SITE_STRICT)
+    same_site_context.set_context(ContextType::SAME_SITE_LAX);
+  if (same_site_context.schemeful_context() == ContextType::SAME_SITE_STRICT)
+    same_site_context.set_schemeful_context(ContextType::SAME_SITE_LAX);
 
-  return CookieOptions::SameSiteCookieContext::MakeInclusiveForSet();
+  return same_site_context;
 }
 
 }  // namespace
@@ -470,22 +501,24 @@ CookieOptions::SameSiteCookieContext ComputeSameSiteContextForRequest(
     const GURL& url,
     const SiteForCookies& site_for_cookies,
     const base::Optional<url::Origin>& initiator,
+    bool is_main_frame_navigation,
     bool force_ignore_site_for_cookies) {
-  // Set SameSiteCookieMode according to the rules laid out in
-  // https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-02:
+  // Set SameSiteCookieContext according to the rules laid out in
+  // https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis:
   //
   // * Include both "strict" and "lax" same-site cookies if the request's
   //   |url|, |initiator|, and |site_for_cookies| all have the same
   //   registrable domain. Note: this also covers the case of a request
   //   without an initiator (only happens for browser-initiated main frame
-  //   navigations).
+  //   navigations). If computing schemefully, the schemes must also match.
   //
   // * Include only "lax" same-site cookies if the request's |URL| and
   //   |site_for_cookies| have the same registrable domain, _and_ the
-  //   request's |http_method| is "safe" ("GET" or "HEAD").
+  //   request's |http_method| is "safe" ("GET" or "HEAD"), and the request
+  //   is a main frame navigation.
   //
-  //   This case should generally occur only for cross-site requests which
-  //   target a top-level browsing context.
+  //   This case should occur only for cross-site requests which
+  //   target a top-level browsing context, with a "safe" method.
   //
   // * Include both "strict" and "lax" same-site cookies if the request is
   //   tagged with a flag allowing it.
@@ -501,10 +534,12 @@ CookieOptions::SameSiteCookieContext ComputeSameSiteContextForRequest(
 
   CookieOptions::SameSiteCookieContext same_site_context;
 
-  same_site_context.set_context(
-      ComputeSameSiteContext(url, site_for_cookies, initiator, false));
-  same_site_context.set_schemeful_context(
-      ComputeSameSiteContext(url, site_for_cookies, initiator, true));
+  same_site_context.set_context(ComputeSameSiteContext(
+      url, site_for_cookies, initiator, true /* is_http */,
+      is_main_frame_navigation, false /* compute_schemefully */));
+  same_site_context.set_schemeful_context(ComputeSameSiteContext(
+      url, site_for_cookies, initiator, true /* is_http */,
+      is_main_frame_navigation, true /* compute_schemefully */));
 
   // If the method is safe, the context is Lax. Otherwise, make a note that
   // the method is unsafe.
@@ -531,10 +566,12 @@ ComputeSameSiteContextForScriptGet(const GURL& url,
 
   CookieOptions::SameSiteCookieContext same_site_context;
 
-  same_site_context.set_context(
-      ComputeSameSiteContext(url, site_for_cookies, initiator, false));
-  same_site_context.set_schemeful_context(
-      ComputeSameSiteContext(url, site_for_cookies, initiator, true));
+  same_site_context.set_context(ComputeSameSiteContext(
+      url, site_for_cookies, initiator, false /* is_http */,
+      false /* is_main_frame_navigation */, false /* compute_schemefully */));
+  same_site_context.set_schemeful_context(ComputeSameSiteContext(
+      url, site_for_cookies, initiator, false /* is_http */,
+      false /* is_main_frame_navigation */, true /* compute_schemefully */));
 
   return same_site_context;
 }
@@ -543,20 +580,39 @@ CookieOptions::SameSiteCookieContext ComputeSameSiteContextForResponse(
     const GURL& url,
     const SiteForCookies& site_for_cookies,
     const base::Optional<url::Origin>& initiator,
+    bool is_main_frame_navigation,
     bool force_ignore_site_for_cookies) {
-  // |initiator| is here in case it'll be decided to ignore |site_for_cookies|
-  // for entirely browser-side requests (see https://crbug.com/958335).
+  if (force_ignore_site_for_cookies)
+    return CookieOptions::SameSiteCookieContext::MakeInclusiveForSet();
+
+  if (is_main_frame_navigation && !site_for_cookies.IsNull()) {
+    // If the request is a main frame navigation, site_for_cookies must either
+    // be null (for opaque origins, e.g., data: origins) or same-site with the
+    // request URL (both schemefully and schemelessly), and the URL cannot be
+    // ws/wss (these schemes are not navigable).
+    DCHECK(site_for_cookies.IsFirstPartyWithSchemefulMode(url, true));
+    DCHECK(!url.SchemeIsWSOrWSS());
+    return CookieOptions::SameSiteCookieContext::MakeInclusiveForSet();
+  }
 
-  return ComputeSameSiteContextForSet(url, site_for_cookies,
-                                      force_ignore_site_for_cookies);
+  return ComputeSameSiteContextForSet(url, site_for_cookies, initiator,
+                                      true /* is_http */,
+                                      is_main_frame_navigation);
 }
 
 CookieOptions::SameSiteCookieContext ComputeSameSiteContextForScriptSet(
     const GURL& url,
     const SiteForCookies& site_for_cookies,
     bool force_ignore_site_for_cookies) {
-  return ComputeSameSiteContextForSet(url, site_for_cookies,
-                                      force_ignore_site_for_cookies);
+  if (force_ignore_site_for_cookies)
+    return CookieOptions::SameSiteCookieContext::MakeInclusiveForSet();
+
+  // It doesn't matter what initiator origin we pass here. Either way, the
+  // context will be considered same-site iff the site_for_cookies is same-site
+  // with the url.
+  return ComputeSameSiteContextForSet(
+      url, site_for_cookies, base::nullopt /* initiator */, false /* is_http */,
+      false /* is_main_frame_navigation */);
 }
 
 CookieOptions::SameSiteCookieContext ComputeSameSiteContextForSubresource(

net/cookies/cookie_util.h

@@ -140,67 +140,76 @@ NET_EXPORT void ParseRequestCookieLine(const std::string& header_value,
 NET_EXPORT std::string SerializeRequestCookieLine(
     const ParsedRequestCookies& parsed_cookies);
 
-// Determines which of the cookies for |url| can be accessed, with respect to
-// the SameSite attribute. This applies to looking up existing cookies; for
-// setting new ones, see ComputeSameSiteContextForResponse and
-// ComputeSameSiteContextForScriptSet.
+// Determines which of the cookies for `url` can be accessed, with respect to
+// the SameSite attribute. This applies to looking up existing cookies for HTTP
+// requests. For looking up cookies for non-HTTP APIs (i.e., JavaScript), see
+// ComputeSameSiteContextForScriptGet. For setting new cookies, see
+// ComputeSameSiteContextForResponse and ComputeSameSiteContextForScriptSet.
 //
-// |site_for_cookies| is the currently navigated to site that should be
+// `site_for_cookies` is the currently navigated to site that should be
 // considered "first-party" for cookies.
 //
-// |initiator| is the origin ultimately responsible for getting the request
-// issued; it may be different from |site_for_cookies| in that it may be some
-// other website that caused the navigation to |site_for_cookies| to occur.
+// `initiator` is the origin ultimately responsible for getting the request
+// issued. It may be different from `site_for_cookies`.
 //
-// base::nullopt for |initiator| denotes that the navigation was initiated by
+// base::nullopt for `initiator` denotes that the navigation was initiated by
 // the user directly interacting with the browser UI, e.g. entering a URL
 // or selecting a bookmark.
 //
-// If |force_ignore_site_for_cookies| is specified, all SameSite cookies will be
+// `is_main_frame_navigation` is whether the request is for a navigation that
+// targets the main frame or top-level browsing context. These requests may
+// sometimes send SameSite=Lax cookies but not SameSite=Strict cookies.
+//
+// If `force_ignore_site_for_cookies` is specified, all SameSite cookies will be
 // attached, i.e. this will return SAME_SITE_STRICT. This flag is set to true
-// when the |site_for_cookies| is a chrome:// URL embedding a secure origin,
+// when the `site_for_cookies` is a chrome:// URL embedding a secure origin,
 // among other scenarios.
 // This is *not* set when the *initiator* is chrome-extension://,
 // which is intentional, since it would be bad to let an extension arbitrarily
 // redirect anywhere and bypass SameSite=Strict rules.
 //
 // See also documentation for corresponding methods on net::URLRequest.
 //
-// |http_method| is used to enforce the requirement that, in a context that's
+// `http_method` is used to enforce the requirement that, in a context that's
 // lax same-site but not strict same-site, SameSite=lax cookies be only sent
 // when the method is "safe" in the RFC7231 section 4.2.1 sense.
 NET_EXPORT CookieOptions::SameSiteCookieContext
 ComputeSameSiteContextForRequest(const std::string& http_method,
                                  const GURL& url,
                                  const SiteForCookies& site_for_cookies,
                                  const base::Optional<url::Origin>& initiator,
+                                 bool is_main_frame_navigation,
                                  bool force_ignore_site_for_cookies);
 
-// As above, but applying for scripts. |initiator| here should be the initiator
+// As above, but applying for scripts. `initiator` here should be the initiator
 // used when fetching the document.
-// If |force_ignore_site_for_cookies| is true, this returns SAME_SITE_STRICT.
+// If `force_ignore_site_for_cookies` is true, this returns SAME_SITE_STRICT.
 NET_EXPORT CookieOptions::SameSiteCookieContext
 ComputeSameSiteContextForScriptGet(const GURL& url,
                                    const SiteForCookies& site_for_cookies,
                                    const base::Optional<url::Origin>& initiator,
                                    bool force_ignore_site_for_cookies);
 
-// Determines which of the cookies for |url| can be set from a network response,
+// Determines which of the cookies for `url` can be set from a network response,
 // with respect to the SameSite attribute. This will only return CROSS_SITE or
 // SAME_SITE_LAX (cookie sets of SameSite=strict cookies are permitted in same
 // contexts that sets of SameSite=lax cookies are).
-// If |force_ignore_site_for_cookies| is true, this returns SAME_SITE_LAX.
+// `is_main_frame_navigation` is whether the request was for a navigation that
+// targets the main frame or top-level browsing context. Both SameSite=Lax and
+// SameSite=Strict cookies may be set by any main frame navigation.
+// If `force_ignore_site_for_cookies` is true, this returns SAME_SITE_LAX.
 NET_EXPORT CookieOptions::SameSiteCookieContext
 ComputeSameSiteContextForResponse(const GURL& url,
                                   const SiteForCookies& site_for_cookies,
                                   const base::Optional<url::Origin>& initiator,
+                                  bool is_main_frame_navigation,
                                   bool force_ignore_site_for_cookies);
 
-// Determines which of the cookies for |url| can be set from a script context,
+// Determines which of the cookies for `url` can be set from a script context,
 // with respect to the SameSite attribute. This will only return CROSS_SITE or
 // SAME_SITE_LAX (cookie sets of SameSite=strict cookies are permitted in same
 // contexts that sets of SameSite=lax cookies are).
-// If |force_ignore_site_for_cookies| is true, this returns SAME_SITE_LAX.
+// If `force_ignore_site_for_cookies` is true, this returns SAME_SITE_LAX.
 NET_EXPORT CookieOptions::SameSiteCookieContext
 ComputeSameSiteContextForScriptSet(const GURL& url,
                                    const SiteForCookies& site_for_cookies,