Bug 1828126 - Add a mechanism to fix cookies with invalid future createdAt timestamp r=pbz,edgul,cookie-reviewers

Differential Revision: https://phabricator.services.mozilla.com/D175589

modules/libpref/init/StaticPrefList.yaml

@@ -11298,6 +11298,14 @@
   value: false
   mirror: always
 
+# If true cookies loaded from the sqlite DB that have a creation or
+# last accessed time that is in the future will be fixed and the
+# timestamps will be set to the current time.
+- name: network.cookie.fixup_on_db_load
+  type: RelaxedAtomicBool
+  value: true
+  mirror: always
+
 # If we should attempt to race the cache and network.
 - name: network.http.rcwn.enabled
   type: bool

netwerk/cookie/Cookie.cpp

@@ -7,6 +7,7 @@
 #include "CookieStorage.h"
 #include "mozilla/Encoding.h"
 #include "mozilla/dom/ToJSValue.h"
+#include "mozilla/glean/GleanMetrics.h"
 #include "mozilla/StaticPrefs_network.h"
 #include "nsIURLParser.h"
 #include "nsURLHelper.h"
@@ -44,12 +45,8 @@ int64_t Cookie::GenerateUniqueCreationTime(int64_t aCreationTime) {
 already_AddRefed<Cookie> Cookie::Create(
     const CookieStruct& aCookieData,
     const OriginAttributes& aOriginAttributes) {
-  RefPtr<Cookie> cookie = new Cookie(aCookieData, aOriginAttributes);
-
-  // Ensure mValue contains a valid UTF-8 sequence. Otherwise XPConnect will
-  // truncate the string after the first invalid octet.
-  UTF_8_ENCODING->DecodeWithoutBOMHandling(aCookieData.value(),
-                                           cookie->mData.value());
+  RefPtr<Cookie> cookie =
+      Cookie::FromCookieStruct(aCookieData, aOriginAttributes);
 
   // If the creationTime given to us is higher than the running maximum,
   // update our maximum.
@@ -57,6 +54,19 @@ already_AddRefed<Cookie> Cookie::Create(
     gLastCreationTime = cookie->mData.creationTime();
   }
 
+  return cookie.forget();
+}
+
+already_AddRefed<Cookie> Cookie::FromCookieStruct(
+    const CookieStruct& aCookieData,
+    const OriginAttributes& aOriginAttributes) {
+  RefPtr<Cookie> cookie = new Cookie(aCookieData, aOriginAttributes);
+
+  // Ensure mValue contains a valid UTF-8 sequence. Otherwise XPConnect will
+  // truncate the string after the first invalid octet.
+  UTF_8_ENCODING->DecodeWithoutBOMHandling(aCookieData.value(),
+                                           cookie->mData.value());
+
   // If sameSite/rawSameSite values aren't sensible reset to Default
   // cf. 5.4.7 in draft-ietf-httpbis-rfc6265bis-09
   if (!Cookie::ValidateSameSite(cookie->mData)) {
@@ -67,6 +77,52 @@ already_AddRefed<Cookie> Cookie::Create(
   return cookie.forget();
 }
 
+already_AddRefed<Cookie> Cookie::CreateValidated(
+    const CookieStruct& aCookieData,
+    const OriginAttributes& aOriginAttributes) {
+  if (!StaticPrefs::network_cookie_fixup_on_db_load()) {
+    return Cookie::Create(aCookieData, aOriginAttributes);
+  }
+
+  RefPtr<Cookie> cookie =
+      Cookie::FromCookieStruct(aCookieData, aOriginAttributes);
+
+  int64_t currentTimeInUsec = PR_Now();
+  // Assert that the last creation time is not higher than the current time.
+  // The 10000 wiggle room accounts for the fact that calling
+  // GenerateUniqueCreationTime might go over the value of PR_Now(), but we'd
+  // most likely not add 10000 cookies in a row.
+  MOZ_ASSERT(gLastCreationTime < currentTimeInUsec + 10000,
+             "Last creation time must not be higher than NOW");
+
+  // If the creationTime given to us is higher than the current time then
+  // update the creation time to now.
+  if (cookie->mData.creationTime() > currentTimeInUsec) {
+    uint64_t diffInSeconds =
+        (cookie->mData.creationTime() - currentTimeInUsec) / PR_USEC_PER_SEC;
+    mozilla::glean::networking::cookie_creation_fixup_diff.AccumulateSamples(
+        {diffInSeconds});
+    glean::networking::cookie_timestamp_fixed_count.Get("creationTime"_ns)
+        .Add(1);
+
+    cookie->mData.creationTime() =
+        GenerateUniqueCreationTime(currentTimeInUsec);
+  }
+
+  if (cookie->mData.lastAccessed() > currentTimeInUsec) {
+    uint64_t diffInSeconds =
+        (cookie->mData.lastAccessed() - currentTimeInUsec) / PR_USEC_PER_SEC;
+    mozilla::glean::networking::cookie_access_fixup_diff.AccumulateSamples(
+        {diffInSeconds});
+    glean::networking::cookie_timestamp_fixed_count.Get("lastAccessed"_ns)
+        .Add(1);
+
+    cookie->mData.lastAccessed() = currentTimeInUsec;
+  }
+
+  return cookie.forget();
+}
+
 size_t Cookie::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const {
   return aMallocSizeOf(this) +
          mData.name().SizeOfExcludingThisIfUnshared(MallocSizeOf) +

netwerk/cookie/Cookie.h

@@ -44,6 +44,10 @@ class Cookie final : public nsICookie {
          const OriginAttributes& aOriginAttributes)
       : mData(aCookieData), mOriginAttributes(aOriginAttributes) {}
 
+  static already_AddRefed<Cookie> FromCookieStruct(
+      const CookieStruct& aCookieData,
+      const OriginAttributes& aOriginAttributes);
+
  public:
   // Returns false if rawSameSite has an invalid value, compared to sameSite.
   static bool ValidateSameSite(const CookieStruct& aCookieData);
@@ -57,6 +61,13 @@ class Cookie final : public nsICookie {
       const CookieStruct& aCookieData,
       const OriginAttributes& aOriginAttributes);
 
+  // Same as Cookie::Create but fixes the lastAccessed and creationDates
+  // if they are set in the future.
+  // Should only get called from CookiePersistentStorage::InitDBConn
+  static already_AddRefed<Cookie> CreateValidated(
+      const CookieStruct& aCookieData,
+      const OriginAttributes& aOriginAttributes);
+
   size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const;
 
   // fast (inline, non-xpcom) getters

netwerk/cookie/CookieCommons.cpp

@@ -411,6 +411,7 @@ already_AddRefed<Cookie> CookieCommons::CreateCookieFromDocument(
   nsCString cookieString(aCookieString);
 
   CookieStruct cookieData;
+  MOZ_ASSERT(cookieData.creationTime() == 0, "Must be initialized to 0");
   bool canSetCookie = false;
   CookieService::CanSetCookie(principalURI, baseDomain, cookieData,
                               requireHostMatch, cookieStatus, cookieString,

netwerk/cookie/CookiePersistentStorage.cpp

@@ -1751,8 +1751,17 @@ void CookiePersistentStorage::InitDBConn() {
     CookieDomainTuple& tuple = mReadArray[i];
     MOZ_ASSERT(!tuple.cookie->isSession());
 
+    // CreateValidated fixes up the creation and lastAccessed times.
+    // If the DB is corrupted and the timestaps are far away in the future
+    // we don't want the creation timestamp to update gLastCreationTime
+    // as that would contaminate all the next creation times.
+    // We fix up these dates to not be later than the current time.
+    // The downside is that if the user sets the date far away in the past
+    // then back to the current date, those cookies will be stale,
+    // but if we don't fix their dates, those cookies might never be
+    // evicted.
     RefPtr<Cookie> cookie =
-        Cookie::Create(*tuple.cookie, tuple.originAttributes);
+        Cookie::CreateValidated(*tuple.cookie, tuple.originAttributes);
     AddCookieToList(tuple.key.mBaseDomain, tuple.key.mOriginAttributes, cookie);
   }
 

netwerk/metrics.yaml

@@ -31,3 +31,64 @@ networking:
       - aborted_socket_fail
       - aborted_https_not_enabled
     telemetry_mirror: NETWORKING_SPECULATIVE_CONNECT_OUTCOME
+
+  cookie_timestamp_fixed_count:
+    type: labeled_counter
+    description: >
+      Counts the number of times a cookie's invalid timestamp was fixed when
+      reading it from the DB.
+    bugs:
+      - https://bugzilla.mozilla.org/show_bug.cgi?id=1828126
+    data_reviews:
+      - https://bugzilla.mozilla.org/show_bug.cgi?id=1828126#c5
+    data_sensitivity:
+      - technical
+    notification_emails:
+      - necko@mozilla.com
+      - vgosu@mozilla.com
+    labels:
+      - creationTime
+      - lastAccessed
+    expires: never
+
+  cookie_creation_fixup_diff:
+    type: custom_distribution
+    unit: second
+    description: >
+      If we fix up a cookie creation timestamp that is in the future this
+      metric records the number of seconds that timestamp was off from NOW.
+    bugs:
+      - https://bugzilla.mozilla.org/show_bug.cgi?id=1828126
+    data_reviews:
+      - https://bugzilla.mozilla.org/show_bug.cgi?id=1828126#c5
+    data_sensitivity:
+      - technical
+    notification_emails:
+      - necko@mozilla.com
+      - vgosu@mozilla.com
+    expires: never
+    range_min: 0
+    range_max: 315360000000
+    bucket_count: 100
+    histogram_type: exponential
+
+  cookie_access_fixup_diff:
+    type: custom_distribution
+    unit: second
+    description: >
+      If we fix up a cookie lastAccessed timestamp that is in the future this
+      metric records the number of seconds that timestamp was off from NOW.
+    bugs:
+      - https://bugzilla.mozilla.org/show_bug.cgi?id=1828126
+    data_reviews:
+      - https://bugzilla.mozilla.org/show_bug.cgi?id=1828126#c5
+    data_sensitivity:
+      - technical
+    notification_emails:
+      - necko@mozilla.com
+      - vgosu@mozilla.com
+    expires: never
+    range_min: 0
+    range_max: 315360000000
+    bucket_count: 100
+    histogram_type: exponential