Implements strict secure cookies as the default behavior in //net

net/cookies/cookie_monster.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Portions of this code based on Mozilla:
 //   (netwerk/cookie/src/nsCookieService.cpp)
 /* ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
  *
  * The contents of this file are subject to the Mozilla Public License Version
@@ skipping 388 matching lines @@
                            const std::string& name,
                            const std::string& value,
                            const std::string& domain,
                            const std::string& path,
                            base::Time creation_time,
                            base::Time expiration_time,
                            base::Time last_access_time,
                            bool secure,
                            bool http_only,
                            CookieSameSite same_site,
-                           bool enforce_strict_secure,
                            CookiePriority priority,
                            const SetCookiesCallback& callback)
       : CookieMonsterTask(cookie_monster),
         url_(url),
         name_(name),
         value_(value),
         domain_(domain),
         path_(path),
         creation_time_(creation_time),
         expiration_time_(expiration_time),
         last_access_time_(last_access_time),
         secure_(secure),
         http_only_(http_only),
         same_site_(same_site),
-        enforce_strict_secure_(enforce_strict_secure),
         priority_(priority),
         callback_(callback) {}
 
   // CookieMonsterTask:
   void Run() override;
 
  protected:
   ~SetCookieWithDetailsTask() override {}
 
  private:
   GURL url_;
   std::string name_;
   std::string value_;
   std::string domain_;
   std::string path_;
   base::Time creation_time_;
   base::Time expiration_time_;
   base::Time last_access_time_;
   bool secure_;
   bool http_only_;
   CookieSameSite same_site_;
-  bool enforce_strict_secure_;
   CookiePriority priority_;
   SetCookiesCallback callback_;
 
   DISALLOW_COPY_AND_ASSIGN(SetCookieWithDetailsTask);
 };
 
 void CookieMonster::SetCookieWithDetailsTask::Run() {
   bool success = this->cookie_monster()->SetCookieWithDetails(
       url_, name_, value_, domain_, path_, creation_time_, expiration_time_,
-      last_access_time_, secure_, http_only_, same_site_,
-      enforce_strict_secure_, priority_);
+      last_access_time_, secure_, http_only_, same_site_, priority_);
   if (!callback_.is_null())
     callback_.Run(success);
 }
 
 // Task class for GetAllCookies call.
 class CookieMonster::GetAllCookiesTask : public CookieMonsterTask {
  public:
   GetAllCookiesTask(CookieMonster* cookie_monster,
                     const GetCookieListCallback& callback)
       : CookieMonsterTask(cookie_monster), callback_(callback) {}
@@ skipping 372 matching lines @@
     const std::string& name,
     const std::string& value,
     const std::string& domain,
     const std::string& path,
     Time creation_time,
     Time expiration_time,
     Time last_access_time,
     bool secure,
     bool http_only,
     CookieSameSite same_site,
-    bool enforce_strict_secure,
     CookiePriority priority,
     const SetCookiesCallback& callback) {
   scoped_refptr<SetCookieWithDetailsTask> task = new SetCookieWithDetailsTask(
       this, url, name, value, domain, path, creation_time, expiration_time,
-      last_access_time, secure, http_only, same_site, enforce_strict_secure,
-      priority, callback);
+      last_access_time, secure, http_only, same_site, priority, callback);
   DoCookieTaskForURL(task, url);
 }
 
 void CookieMonster::FlushStore(const base::Closure& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (initialized_ && store_.get())
     store_->Flush(callback);
   else if (!callback.is_null())
     base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, callback);
@@ skipping 166 matching lines @@
                                          const std::string& name,
                                          const std::string& value,
                                          const std::string& domain,
                                          const std::string& path,
                                          base::Time creation_time,
                                          base::Time expiration_time,
                                          base::Time last_access_time,
                                          bool secure,
                                          bool http_only,
                                          CookieSameSite same_site,
-                                         bool enforce_strict_secure,
                                          CookiePriority priority) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (!HasCookieableScheme(url))
     return false;
 
   // TODO(mmenke): This class assumes each cookie to have a unique creation
   // time. Allowing the caller to set the creation time violates that
   // assumption. Worth fixing? Worth noting that time changes between browser
   // restarts can cause the same issue.
   base::Time actual_creation_time = creation_time;
   if (creation_time.is_null()) {
     actual_creation_time = CurrentTime();
     last_time_seen_ = actual_creation_time;
   }
 
   std::unique_ptr<CanonicalCookie> cc(CanonicalCookie::Create(
       url, name, value, domain, path, actual_creation_time, expiration_time,
-      secure, http_only, same_site, enforce_strict_secure, priority));
+      secure, http_only, same_site, priority));
 
   if (!cc.get())
     return false;
 
   if (!last_access_time.is_null())
     cc->SetLastAccessDate(last_access_time);
 
   CookieOptions options;
   options.set_include_httponly();
   options.set_same_site_cookie_mode(
       CookieOptions::SameSiteCookieMode::INCLUDE_STRICT_AND_LAX);
-  if (enforce_strict_secure)
-    options.set_enforce_strict_secure();
   return SetCanonicalCookie(std::move(cc), url, options);
 }
 
 CookieList CookieMonster::GetAllCookies() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // This function is being called to scrape the cookie list for management UI
   // or similar.  We shouldn't show expired cookies in this list since it will
   // just be confusing to users, and this function is called rarely enough (and
   // is already slow enough) that it's OK to take the time to garbage collect
@@ skipping 515 matching lines @@
       InternalUpdateCookieAccessTime(cc, current);
     }
     cookies->push_back(cc);
   }
 }
 
 bool CookieMonster::DeleteAnyEquivalentCookie(const std::string& key,
                                               const CanonicalCookie& ecc,
                                               const GURL& source_url,
                                               bool skip_httponly,
-                                              bool already_expired,
-                                              bool enforce_strict_secure) {
+                                              bool already_expired) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   bool found_equivalent_cookie = false;
   bool skipped_httponly = false;
   bool skipped_secure_cookie = false;
 
   histogram_cookie_delete_equivalent_->Add(COOKIE_DELETE_EQUIVALENT_ATTEMPT);
 
   for (CookieMapItPair its = cookies_.equal_range(key);
        its.first != its.second;) {
     CookieMap::iterator curit = its.first;
     CanonicalCookie* cc = curit->second.get();
     ++its.first;
 
-    // If strict secure cookies is being enforced, then the equivalency
-    // requirements are looser. 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.
+    // 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.
     //
     // See: https://tools.ietf.org/html/draft-west-leave-secure-cookies-alone

[Mike West, 2017/01/14 06:24:01]

Nit: We should update these references to
https://tools.ietf.org/html/draft-ietf-httpbis-cookie-alone.

[jww, 2017/01/24 18:09:25]

Done.

-    if (enforce_strict_secure && cc->IsSecure() &&
-        !source_url.SchemeIsCryptographic() &&
+    if (cc->IsSecure() && !source_url.SchemeIsCryptographic() &&
         ecc.IsEquivalentForSecureCookieMatching(*cc)) {
       skipped_secure_cookie = true;
       histogram_cookie_delete_equivalent_->Add(
           COOKIE_DELETE_EQUIVALENT_SKIPPING_SECURE);
       // If the cookie is equivalent to the new cookie and wouldn't have been
       // skipped for being HTTP-only, record that it is a skipped secure cookie
       // that would have been deleted otherwise.
       if (ecc.IsEquivalent(*cc)) {
         found_equivalent_cookie = true;
 
@@ skipping 104 matching lines @@
 bool CookieMonster::SetCanonicalCookie(std::unique_ptr<CanonicalCookie> cc,
                                        const GURL& source_url,
                                        const CookieOptions& options) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   Time creation_time = cc->CreationDate();
   const std::string key(GetKey(cc->Domain()));
   bool already_expired = cc->IsExpired(creation_time);
 
   if (DeleteAnyEquivalentCookie(key, *cc, source_url,
-                                options.exclude_httponly(), already_expired,
-                                options.enforce_strict_secure())) {
+                                options.exclude_httponly(), already_expired)) {
     std::string error;
-    if (options.enforce_strict_secure()) {
-      error =
-          "SetCookie() not clobbering httponly cookie or secure cookie for "
-          "insecure scheme";
-    } else {
-      error = "SetCookie() not clobbering httponly cookie";
-    }
+    error =
+        "SetCookie() not clobbering httponly cookie or secure cookie for "
+        "insecure scheme";
 
     VLOG(kVlogSetCookies) << error;
     return false;
   }
 
   VLOG(kVlogSetCookies) << "SetCookie() key: " << key
                         << " cc: " << cc->DebugString();
 
   // Realize that we might be setting an expired cookie, and the only point
   // was to delete the cookie which we've already done.
   if (!already_expired) {
     // See InitializeHistograms() for details.
     if (cc->IsPersistent()) {
       histogram_expiration_duration_minutes_->Add(
           (cc->ExpiryDate() - creation_time).InMinutes());
     }
 
     InternalInsertCookie(key, std::move(cc), source_url, true);
   } else {
     VLOG(kVlogSetCookies) << "SetCookie() not storing already expired cookie.";
   }
 
   // We assume that hopefully setting a cookie will be less common than
   // querying a cookie.  Since setting a cookie can put us over our limits,
   // make sure that we garbage collect...  We can also make the assumption that
   // if a cookie was set, in the common case it will be used soon after,
   // and we will purge the expired cookies in GetCookies().
-  GarbageCollect(creation_time, key, options.enforce_strict_secure());
+  GarbageCollect(creation_time, key);
 
   return true;
 }
 
 bool CookieMonster::SetCanonicalCookies(const CookieList& list) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   CookieOptions options;
   options.set_include_httponly();
 
@@ skipping 52 matching lines @@
   ChangeCausePair mapping = kChangeCauseMapping[deletion_cause];
   if (delegate_.get() && mapping.notify)
     delegate_->OnCookieChanged(*cc, true, mapping.cause);
   RunCookieChangedCallbacks(*cc, mapping.cause);
   cookies_.erase(it);
 }
 
 // Domain expiry behavior is unchanged by key/expiry scheme (the
 // meaning of the key is different, but that's not visible to this routine).
 size_t CookieMonster::GarbageCollect(const Time& current,
-                                     const std::string& key,
-                                     bool enforce_strict_secure) {
+                                     const std::string& key) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   size_t num_deleted = 0;
   Time safe_date(Time::Now() - TimeDelta::FromDays(kSafeFromGlobalPurgeDays));
 
   // Collect garbage for this key, minding cookie priorities.
   if (cookies_.count(key) > kDomainMaxCookies) {
     VLOG(kVlogGarbageCollection) << "GarbageCollect() key: " << key;
 
     CookieItVector* cookie_its;
@@ skipping 37 matching lines @@
           // 4.  High-priority non-secure cookies.
           {COOKIE_PRIORITY_HIGH, true},
           // 5.  Medium-priority secure cookies.
           {COOKIE_PRIORITY_MEDIUM, false},
           // 6.  High-priority secure cookies.
           {COOKIE_PRIORITY_HIGH, false},
       };
 
       size_t quota = 0;
       for (const auto& purge_round : purge_rounds) {
-        // Only observe the non-secure purge rounds if strict secure cookies is
-        // enabled.
-        if (!enforce_strict_secure && purge_round.protect_secure_cookies)
-          continue;
-
         // Adjust quota according to the priority of cookies. Each round should
         // protect certain number of cookies in order to avoid starvation.
         // For example, when each round starts to remove cookies, the number of
         // cookies of that priority are counted and a decision whether they
         // should be deleted or not is made. If yes, some number of cookies of
         // that priority are deleted considering the quota.
         switch (purge_round.priority) {
           case COOKIE_PRIORITY_LOW:
             quota = kDomainCookiesQuotaLow;
             break;
@@ skipping 32 matching lines @@
 
     num_deleted += GarbageCollectExpired(
         current, CookieMapItPair(cookies_.begin(), cookies_.end()),
         &cookie_its);
 
     if (cookie_its.size() > kMaxCookies) {
       VLOG(kVlogGarbageCollection) << "Deep Garbage Collect everything.";
       size_t purge_goal = cookie_its.size() - (kMaxCookies - kPurgeCookies);
       DCHECK(purge_goal > kPurgeCookies);
 
-      if (enforce_strict_secure) {
-        CookieItVector secure_cookie_its;
-        CookieItVector non_secure_cookie_its;
-        SplitCookieVectorIntoSecureAndNonSecure(cookie_its, &secure_cookie_its,
-                                                &non_secure_cookie_its);
-        size_t non_secure_purge_goal =
-            std::min<size_t>(purge_goal, non_secure_cookie_its.size() - 1);
+      CookieItVector secure_cookie_its;
+      CookieItVector non_secure_cookie_its;
+      SplitCookieVectorIntoSecureAndNonSecure(cookie_its, &secure_cookie_its,
+                                              &non_secure_cookie_its);
+      size_t non_secure_purge_goal =
+          std::min<size_t>(purge_goal, non_secure_cookie_its.size() - 1);
 
-        size_t just_deleted = GarbageCollectLeastRecentlyAccessed(
-            current, safe_date, non_secure_purge_goal, non_secure_cookie_its);
-        num_deleted += just_deleted;
+      size_t just_deleted = GarbageCollectLeastRecentlyAccessed(
+          current, safe_date, non_secure_purge_goal, non_secure_cookie_its);
+      num_deleted += just_deleted;
 
-        if (just_deleted < purge_goal) {
-          size_t secure_purge_goal = std::min<size_t>(
-              purge_goal - just_deleted, secure_cookie_its.size() - 1);
-          num_deleted += GarbageCollectLeastRecentlyAccessed(
-              current, safe_date, secure_purge_goal, secure_cookie_its);
-        }
-      } else {
+      if (just_deleted < purge_goal && secure_cookie_its.size() > 0) {
+        size_t secure_purge_goal = std::min<size_t>(
+            purge_goal - just_deleted, secure_cookie_its.size() - 1);
         num_deleted += GarbageCollectLeastRecentlyAccessed(
-            current, safe_date, purge_goal, cookie_its);
+            current, safe_date, secure_purge_goal, secure_cookie_its);

[Mike West, 2017/01/14 06:24:01]

Hrm. I don't think we ever updated
https://tools.ietf.org/html/draft-west-cookie-priority-00 with the implications
for `secure`. I'm not sure it's worth your time to tackle that in the last two
weeks you're here, but maybe you could sketch out a summary in a doc somewhere
so I have something to work from? :)

[jww, 2017/01/24 18:09:25]

Shared a doc with you:
https://docs.google.com/document/d/1fI_vpJ_mF5K_52c5o39wfyVCuPStrswc7E2bFB_o0...

       }
     }
   }
 
   return num_deleted;
 }
 
 size_t CookieMonster::PurgeLeastRecentMatches(CookieItVector* cookies,
                                               CookiePriority priority,
                                               size_t to_protect,
@@ skipping 369 matching lines @@
        it != hook_map_.end(); ++it) {
     std::pair<GURL, std::string> key = it->first;
     if (cookie.IncludeForRequestURL(key.first, opts) &&
         cookie.Name() == key.second) {
       it->second->Notify(cookie, cause);
     }
   }
 }
 
 }  // namespace net