Fix "Leave Secure Cookies Alone" implementation

"Leave Secure Cookies Alone" refers to step 12 in Section 5.4 of RFC
6265bis:
https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-05#section-5.4

If an insecure URL attempts to set a cookie that is "equivalent" to a
preexisting Secure cookie, then that set attempt is rejected.

This CL:
 - Fixes a bug in our calculation of "equivalence" for these purposes,
   which was not correctly computing domain-matching described in
   Section 5.1.3 of the RFC. (See example below.)
 - Adds (perhaps excessively) exhaustive tests for this behavior.
 - Adds comments clarifying the situation.
 - Renames variables to make the direction of the comparison clearer,
   as this is an asymmetric comparison.

Example:

Before this CL: If https://foo.com/ has set a secure host cookie

  cookie=secure; secure

then we allowed an insecure host cookie to be set by http://a.foo.com/:

  cookie=insecure

This cookie should be rejected, according to the spec, because
a.foo.com and foo.com are domain-matching.

Bug: 1069804
Change-Id: I002f9ffd7cf8523a2f4291525db2939306028d7a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2143865
Commit-Queue: Lily Chen <chlily@chromium.org>
Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#762005}

net/base/url_util.cc

@@ -266,6 +266,23 @@ std::string GetSuperdomain(base::StringPiece domain) {
   return domain.substr(dot_pos + 1).as_string();
 }
 
+bool IsSubdomainOf(base::StringPiece subdomain, base::StringPiece superdomain) {
+  if (subdomain.empty() || superdomain.empty())
+    return false;
+
+  // Subdomain must be identical or have strictly more labels than the
+  // superdomain.
+  if (subdomain.length() <= superdomain.length())
+    return subdomain == superdomain;
+
+  // Superdomain must be suffix of subdomain, and the last character not
+  // included in the matching substring must be a dot.
+  if (!subdomain.ends_with(superdomain))
+    return false;
+  subdomain.remove_suffix(superdomain.length());
+  return subdomain.back() == '.';
+}
+
 std::string CanonicalizeHost(base::StringPiece host,
                              url::CanonHostInfo* host_info) {
   // Try to canonicalize the host.

net/base/url_util.h

@@ -131,6 +131,12 @@ NET_EXPORT std::string GetHostOrSpecFromURL(const GURL& url);
 // GetSuperdomain("127.0.0.1") -> "0.0.1"
 NET_EXPORT std::string GetSuperdomain(base::StringPiece domain);
 
+// Returns whether |subdomain| is a subdomain of (or identical to)
+// |superdomain|, if both are hostnames (not IP addresses -- for which this
+// function is nonsensical). Does not consider the Public Suffix List.
+NET_EXPORT bool IsSubdomainOf(base::StringPiece subdomain,
+                              base::StringPiece superdomain);
+
 // Canonicalizes |host| and returns it.  Also fills |host_info| with
 // IP address information.  |host_info| must not be NULL.
 NET_EXPORT std::string CanonicalizeHost(base::StringPiece host,

net/base/url_util_unittest.cc

@@ -297,6 +297,34 @@ TEST(UrlUtilTest, GetSuperdomain) {
   }
 }
 
+TEST(UrlUtilTest, IsSubdomainOf) {
+  struct {
+    const char* subdomain;
+    const char* superdomain;
+    bool is_subdomain;
+  } tests[] = {
+      {"bar.foo.com", "foo.com", true},
+      {"barfoo.com", "foo.com", false},
+      {"bar.foo.com", "com", true},
+      {"bar.foo.com", "other.com", false},
+      {"bar.foo.com", "bar.foo.com", true},
+      {"bar.foo.com", "baz.foo.com", false},
+      {"bar.foo.com", "baz.bar.foo.com", false},
+      {"bar.foo.com", "ar.foo.com", false},
+      {"foo.com", "foo.com.", false},
+      {"bar.foo.com", "foo.com.", false},
+      {"", "", false},
+      {"a", "", false},
+      {"", "a", false},
+      {"127.0.0.1", "0.0.1", true},  // Don't do this...
+  };
+
+  for (const auto& test : tests) {
+    EXPECT_EQ(test.is_subdomain,
+              IsSubdomainOf(test.subdomain, test.superdomain));
+  }
+}
+
 TEST(UrlUtilTest, CompliantHost) {
   struct {
     const char* const host;

net/cookies/canonical_cookie.cc

@@ -403,10 +403,27 @@ std::string CanonicalCookie::DomainWithoutDot() const {
 }
 
 bool CanonicalCookie::IsEquivalentForSecureCookieMatching(
-    const CanonicalCookie& ecc) const {
-  return (name_ == ecc.Name() && (ecc.IsDomainMatch(DomainWithoutDot()) ||
-                                  IsDomainMatch(ecc.DomainWithoutDot())) &&
-          ecc.IsOnPath(Path()));
+    const CanonicalCookie& secure_cookie) const {
+  // Names must be the same
+  bool same_name = name_ == secure_cookie.Name();
+
+  // They should domain-match in one direction or the other. (See RFC 6265bis
+  // section 5.1.3.)
+  // TODO(chlily): This does not check for the IP address case. This is bad due
+  // to https://crbug.com/1069935.
+  bool domain_match =
+      IsSubdomainOf(DomainWithoutDot(), secure_cookie.DomainWithoutDot()) ||
+      IsSubdomainOf(secure_cookie.DomainWithoutDot(), DomainWithoutDot());
+
+  bool path_match = secure_cookie.IsOnPath(Path());
+
+  bool equivalent_for_secure_cookie_matching =
+      same_name && domain_match && path_match;
+
+  // IsEquivalent() is a stricter check than this.
+  DCHECK(!IsEquivalent(secure_cookie) || equivalent_for_secure_cookie_matching);
+
+  return equivalent_for_secure_cookie_matching;
 }
 
 bool CanonicalCookie::IsOnPath(const std::string& url_path) const {

net/cookies/canonical_cookie.h

@@ -145,7 +145,7 @@ class NET_EXPORT CanonicalCookie {
   bool IsEquivalent(const CanonicalCookie& ecc) const {
     // It seems like it would make sense to take secure, httponly, and samesite
     // into account, but the RFC doesn't specify this.
-    // NOTE: Keep this logic in-sync with TrimDuplicateCookiesForHost().
+    // NOTE: Keep this logic in-sync with TrimDuplicateCookiesForKey().
     return (name_ == ecc.Name() && domain_ == ecc.Domain()
             && path_ == ecc.Path());
   }
@@ -157,18 +157,40 @@ class NET_EXPORT CanonicalCookie {
   }
 
   // Checks a looser set of equivalency rules than 'IsEquivalent()' in order
-  // to support the stricter 'Secure' behaviors specified in
+  // to support the stricter 'Secure' behaviors specified in Step 12 of
+  // https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-05#section-5.4
+  // which originated from the proposal in
   // https://tools.ietf.org/html/draft-ietf-httpbis-cookie-alone#section-3
   //
-  // Returns 'true' if this cookie's name matches |ecc|, and this cookie is
-  // a domain-match for |ecc| (or vice versa), and |ecc|'s path is "on" this
-  // cookie's path (as per 'IsOnPath()').
+  // Returns 'true' if this cookie's name matches |secure_cookie|, and this
+  // cookie is a domain-match for |secure_cookie| (or vice versa), and
+  // |secure_cookie|'s path is "on" this cookie's path (as per 'IsOnPath()').
   //
   // Note that while the domain-match cuts both ways (e.g. 'example.com'
   // matches 'www.example.com' in either direction), the path-match is
   // unidirectional (e.g. '/login/en' matches '/login' and '/', but
   // '/login' and '/' do not match '/login/en').
-  bool IsEquivalentForSecureCookieMatching(const CanonicalCookie& ecc) const;
+  //
+  // Conceptually:
+  // If new_cookie.IsEquivalentForSecureCookieMatching(secure_cookie) is true,
+  // this means that new_cookie would "shadow" secure_cookie: they would would
+  // be indistinguishable when serialized into a Cookie header. This is
+  // important because, if an attacker is attempting to set new_cookie, it
+  // should not be allowed to mislead the server into using new_cookie's value
+  // instead of secure_cookie's.
+  //
+  // The reason for the asymmetric path comparison ("cookie1=bad; path=/a/b"
+  // from an insecure source is not allowed if "cookie1=good; secure; path=/a"
+  // exists, but "cookie2=bad; path=/a" from an insecure source is allowed if
+  // "cookie2=good; secure; path=/a/b" exists) is because cookies in the Cookie
+  // header are serialized with longer path first. (See CookieSorter in
+  // cookie_monster.cc.) That is, they would be serialized as "Cookie:
+  // cookie1=bad; cookie1=good" in one case, and "Cookie: cookie2=good;
+  // cookie2=bad" in the other case. The first scenario is not allowed because
+  // the attacker injects the bad value, whereas the second scenario is ok
+  // because the good value is still listed first.
+  bool IsEquivalentForSecureCookieMatching(
+      const CanonicalCookie& secure_cookie) const;
 
   void SetSourceScheme(CookieSourceScheme source_scheme) {
     source_scheme_ = source_scheme;
@@ -178,12 +200,28 @@ class NET_EXPORT CanonicalCookie {
   }
   void SetCreationDate(const base::Time& date) { creation_date_ = date; }
 
-  // Returns true if the given |url_path| path-matches the cookie-path as
-  // described in section 5.1.4 in RFC 6265.
+  // Returns true if the given |url_path| path-matches this cookie's cookie-path
+  // as described in section 5.1.4 in RFC 6265. This returns true if |path_| and
+  // |url_path| are identical, or if |url_path| is a subdirectory of |path_|.
   bool IsOnPath(const std::string& url_path) const;
 
-  // Returns true if the cookie domain matches the given |host| as described in
-  // section 5.1.3 of RFC 6265.
+  // This returns true if this cookie's |domain_| indicates that it can be
+  // accessed by |host|.
+  //
+  // In the case where |domain_| has no leading dot, this is a host cookie and
+  // will only domain match if |host| is identical to |domain_|.
+  //
+  // In the case where |domain_| has a leading dot, this is a domain cookie. It
+  // will match |host| if |domain_| is a suffix of |host|, or if |domain_| is
+  // exactly equal to |host| plus a leading dot.
+  //
+  // Note that this isn't quite the same as the "domain-match" algorithm in RFC
+  // 6265bis, since our implementation uses the presence of a leading dot in the
+  // |domain_| string in place of the spec's host-only-flag. That is, if
+  // |domain_| has no leading dot, then we only consider it matching if |host|
+  // is identical (which reflects the intended behavior when the cookie has a
+  // host-only-flag), whereas the RFC also treats them as domain-matching if
+  // |domain_| is a subdomain of |host|.
   bool IsDomainMatch(const std::string& host) const;
 
   // Returns if the cookie should be included (and if not, why) for the given

net/cookies/canonical_cookie_unittest.cc

@@ -396,6 +396,7 @@ TEST(CanonicalCookieTest, IsEquivalent) {
       creation_time, expiration_time, base::Time(), secure, httponly, same_site,
       COOKIE_PRIORITY_MEDIUM);
   EXPECT_FALSE(cookie->IsEquivalent(*other_cookie));
+  // The path comparison is asymmetric
   EXPECT_FALSE(cookie->IsEquivalentForSecureCookieMatching(*other_cookie));
   EXPECT_TRUE(other_cookie->IsEquivalentForSecureCookieMatching(*cookie));
 
@@ -408,6 +409,56 @@ TEST(CanonicalCookieTest, IsEquivalent) {
   EXPECT_FALSE(other_cookie->IsEquivalentForSecureCookieMatching(*cookie));
 }
 
+TEST(CanonicalCookieTest, IsEquivalentForSecureCookieMatching) {
+  struct {
+    struct {
+      const char* name;
+      const char* domain;
+      const char* path;
+    } cookie, secure_cookie;
+    bool equivalent;
+    bool is_symmetric;  // Whether the reverse comparison has the same result.
+  } kTests[] = {
+      // Equivalent to itself
+      {{"A", "a.foo.com", "/"}, {"A", "a.foo.com", "/"}, true, true},
+      {{"A", ".a.foo.com", "/"}, {"A", ".a.foo.com", "/"}, true, true},
+      // Names are different
+      {{"A", "a.foo.com", "/"}, {"B", "a.foo.com", "/"}, false, true},
+      // Host cookie and domain cookie with same hostname match
+      {{"A", "a.foo.com", "/"}, {"A", ".a.foo.com", "/"}, true, true},
+      // Subdomains and superdomains match
+      {{"A", "a.foo.com", "/"}, {"A", ".foo.com", "/"}, true, true},
+      {{"A", ".a.foo.com", "/"}, {"A", ".foo.com", "/"}, true, true},
+      {{"A", "a.foo.com", "/"}, {"A", "foo.com", "/"}, true, true},
+      {{"A", ".a.foo.com", "/"}, {"A", "foo.com", "/"}, true, true},
+      // Different domains don't match
+      {{"A", "a.foo.com", "/"}, {"A", "b.foo.com", "/"}, false, true},
+      {{"A", "a.foo.com", "/"}, {"A", "ba.foo.com", "/"}, false, true},
+      // Path attribute matches if it is a subdomain, but not vice versa.
+      {{"A", "a.foo.com", "/sub"}, {"A", "a.foo.com", "/"}, true, false},
+      // Different paths don't match
+      {{"A", "a.foo.com", "/sub"}, {"A", "a.foo.com", "/other"}, false, true},
+      {{"A", "a.foo.com", "/a/b"}, {"A", "a.foo.com", "/a/c"}, false, true},
+  };
+
+  for (auto test : kTests) {
+    auto cookie = std::make_unique<CanonicalCookie>(
+        test.cookie.name, "value1", test.cookie.domain, test.cookie.path,
+        base::Time(), base::Time(), base::Time(), false /* secure */, false,
+        CookieSameSite::LAX_MODE, COOKIE_PRIORITY_MEDIUM);
+    auto secure_cookie = std::make_unique<CanonicalCookie>(
+        test.secure_cookie.name, "value2", test.secure_cookie.domain,
+        test.secure_cookie.path, base::Time(), base::Time(), base::Time(),
+        true /* secure */, false, CookieSameSite::LAX_MODE,
+        COOKIE_PRIORITY_MEDIUM);
+
+    EXPECT_EQ(test.equivalent,
+              cookie->IsEquivalentForSecureCookieMatching(*secure_cookie));
+    EXPECT_EQ(test.equivalent == test.is_symmetric,
+              secure_cookie->IsEquivalentForSecureCookieMatching(*cookie));
+  }
+}
+
 TEST(CanonicalCookieTest, IsDomainMatch) {
   GURL url("http://www.example.com/test/foo.html");
   base::Time creation_time = base::Time::Now();

net/cookies/cookie_monster.cc

@@ -904,7 +904,7 @@ void CookieMonster::TrimDuplicateCookiesForKey(const std::string& key,
     dupes.erase(dupes.begin());
 
     LOG(ERROR) << base::StringPrintf(
-        "Found %d duplicate cookies for host='%s', "
+        "Found %d duplicate cookies for key='%s', "
         "with {name='%s', domain='%s', path='%s'}",
         static_cast<int>(dupes.size()), key.c_str(),
         std::get<0>(signature).c_str(), std::get<1>(signature).c_str(),
@@ -998,68 +998,72 @@ void CookieMonster::MaybeDeleteEquivalentCookieAndUpdateStatus(
       CanonicalCookie::CookieInclusionStatus::EXCLUDE_OVERWRITE_HTTP_ONLY));
 
   bool found_equivalent_cookie = false;
-  CookieMap::iterator maybe_delete_it = cookies_.end();
-  CanonicalCookie* cc_skipped_secure = nullptr;
+  CookieMap::iterator deletion_candidate_it = cookies_.end();
+  CanonicalCookie* skipped_secure_cookie = nullptr;
 
   // Check every cookie matching this domain key for equivalence.
   CookieMapItPair range_its = cookies_.equal_range(key);
   for (auto cur_it = range_its.first; cur_it != range_its.second; ++cur_it) {
-    CanonicalCookie* cc = cur_it->second.get();
+    CanonicalCookie* cur_existing_cookie = cur_it->second.get();
 
     // Evaluate "Leave Secure Cookies Alone":
-    // If the cookie is being set from an insecure scheme, then if a cookie
-    // already exists with the same name and it is Secure, then the cookie
-    // should *not* be updated if they domain-match and ignoring the path
-    // attribute. This notion of equivalence is slightly more inclusive than the
-    // usual IsEquivalent() check.
+    // If the cookie is being set from an insecure source, then if an
+    // "equivalent" Secure cookie already exists, then the cookie should *not*
+    // be updated.
+    //
+    // "Equivalent" means they are the same by
+    // IsEquivalentForSecureCookieMatching(). See the comment there for
+    // details. (Note this is not a symmetric comparison.) This notion of
+    // equivalence is slightly more inclusive than the usual IsEquivalent() one.
     //
     // See: https://tools.ietf.org/html/draft-ietf-httpbis-cookie-alone
-    if (cc->IsSecure() && !source_secure &&
-        cookie_being_set.IsEquivalentForSecureCookieMatching(*cc)) {
+    if (cur_existing_cookie->IsSecure() && !source_secure &&
+        cookie_being_set.IsEquivalentForSecureCookieMatching(
+            *cur_existing_cookie)) {
       // Hold onto this for additional Netlogging later if we end up preserving
       // a would-have-been-deleted cookie because of this.
-      cc_skipped_secure = cc;
+      skipped_secure_cookie = cur_existing_cookie;
       net_log_.AddEvent(NetLogEventType::COOKIE_STORE_COOKIE_REJECTED_SECURE,
                         [&](NetLogCaptureMode capture_mode) {
                           return NetLogCookieMonsterCookieRejectedSecure(
-                              cc_skipped_secure, &cookie_being_set,
+                              skipped_secure_cookie, &cookie_being_set,
                               capture_mode);
                         });
       status->AddExclusionReason(
           CanonicalCookie::CookieInclusionStatus::EXCLUDE_OVERWRITE_SECURE);
     }
 
-    if (cookie_being_set.IsEquivalent(*cc)) {
+    if (cookie_being_set.IsEquivalent(*cur_existing_cookie)) {
       // We should never have more than one equivalent cookie, since they should
       // overwrite each other.
       CHECK(!found_equivalent_cookie)
           << "Duplicate equivalent cookies found, cookie store is corrupted.";
-      DCHECK(maybe_delete_it == cookies_.end());
+      DCHECK(deletion_candidate_it == cookies_.end());
       found_equivalent_cookie = true;
 
       // The |cookie_being_set| is rejected for trying to overwrite an httponly
       // cookie when it should not be able to.
-      if (skip_httponly && cc->IsHttpOnly()) {
+      if (skip_httponly && cur_existing_cookie->IsHttpOnly()) {
         net_log_.AddEvent(
             NetLogEventType::COOKIE_STORE_COOKIE_REJECTED_HTTPONLY,
             [&](NetLogCaptureMode capture_mode) {
               return NetLogCookieMonsterCookieRejectedHttponly(
-                  cc, &cookie_being_set, capture_mode);
+                  cur_existing_cookie, &cookie_being_set, capture_mode);
             });
         status->AddExclusionReason(CanonicalCookie::CookieInclusionStatus::
                                        EXCLUDE_OVERWRITE_HTTP_ONLY);
       } else {
-        maybe_delete_it = cur_it;
+        deletion_candidate_it = cur_it;
       }
     }
   }
 
-  if (maybe_delete_it != cookies_.end()) {
-    CanonicalCookie* maybe_delete_cc = maybe_delete_it->second.get();
-    if (maybe_delete_cc->Value() == cookie_being_set.Value())
-      *creation_date_to_inherit = maybe_delete_cc->CreationDate();
+  if (deletion_candidate_it != cookies_.end()) {
+    CanonicalCookie* deletion_candidate = deletion_candidate_it->second.get();
+    if (deletion_candidate->Value() == cookie_being_set.Value())
+      *creation_date_to_inherit = deletion_candidate->CreationDate();
     if (status->IsInclude()) {
-      InternalDeleteCookie(maybe_delete_it, true,
+      InternalDeleteCookie(deletion_candidate_it, true /* sync_to_store */,
                            already_expired ? DELETE_COOKIE_EXPIRED_OVERWRITE
                                            : DELETE_COOKIE_OVERWRITE);
     } else if (status->HasExclusionReason(
@@ -1067,14 +1071,14 @@ void CookieMonster::MaybeDeleteEquivalentCookieAndUpdateStatus(
                        EXCLUDE_OVERWRITE_SECURE)) {
       // Log that we preserved a cookie that would have been deleted due to
       // Leave Secure Cookies Alone. This arbitrarily only logs the last
-      // |cc_skipped_secure| that we were left with after the for loop, even if
-      // there were multiple matching Secure cookies that were left alone.
-      DCHECK(cc_skipped_secure);
+      // |skipped_secure_cookie| that we were left with after the for loop, even
+      // if there were multiple matching Secure cookies that were left alone.
+      DCHECK(skipped_secure_cookie);
       net_log_.AddEvent(
           NetLogEventType::COOKIE_STORE_COOKIE_PRESERVED_SKIPPED_SECURE,
           [&](NetLogCaptureMode capture_mode) {
             return NetLogCookieMonsterCookiePreservedSkippedSecure(
-                cc_skipped_secure, maybe_delete_cc, &cookie_being_set,
+                skipped_secure_cookie, deletion_candidate, &cookie_being_set,
                 capture_mode);
           });
     }

net/cookies/cookie_util.h

@@ -73,8 +73,9 @@ NET_EXPORT GURL CookieOriginToURL(const std::string& domain, bool is_https);
 NET_EXPORT GURL SimulatedCookieSource(const CanonicalCookie& cookie,
                                       const std::string& source_scheme);
 
-// Returns true if the cookie |domain| matches the given |host| as described
-// in section 5.1.3 of RFC 6265.
+// |domain| is the output of cookie.Domain() for some cookie. This returns true
+// if a |domain| indicates that the cookie can be accessed by |host|.
+// See comment on CanonicalCookie::IsDomainMatch().
 NET_EXPORT bool IsDomainMatch(const std::string& domain,
                               const std::string& host);