Low firefox Logic Error 🔧 Commit mapped

Overview

Low
Severity
CVSS
No
Exploited ITW
Fixed
Fix Status
Impactlow
DescriptionRepeated writes to history interface attributes could have been used to cause a Denial of Service condition in the browser. This was addressed by introducing rate-limiting to this API.
ComponentDOM
Bug ClassLogic Error
Tracker1913000
Fix commit795dad3b68cb (firefox) +49/-37
CISA KEVNot listed
CreditedAndrei Enache
Disclosed2024-10-29

Changed Functions

FunctionChangeNotes
if
docshell/base/BrowsingContext.cpp
modified
for
docshell/test/navigation/test_rate_limit_location_change.html
modified

Files Changed

  • docshell/base/BrowsingContext.cpp
  • docshell/base/BrowsingContext.h
  • docshell/shistory/ChildSHistory.cpp
  • docshell/test/navigation/test_rate_limit_location_change.html
  • dom/base/Location.cpp
  • dom/base/LocationBase.cpp
  • dom/base/nsHistory.cpp
  • dom/base/nsHistory.h
  • dom/chrome-webidl/BrowsingContext.webidl
  • dom/webidl/History.webidl
  • modules/libpref/init/StaticPrefList.yaml
  • remote/shared/RecommendedPreferences.sys.mjs
  • testing/marionette/client/marionette_driver/geckoinstance.py
  • tools/@types/lib.gecko.dom.d.ts
diff --git a/docshell/base/BrowsingContext.cpp b/docshell/base/BrowsingContext.cpp
index cd4d788c654..9b5c8143cb7 100644
--- a/docshell/base/BrowsingContext.cpp
+++ b/docshell/base/BrowsingContext.cpp
@@ -3830,17 +3830,16 @@ bool BrowsingContext::ShouldUpdateSessionHistory(uint32_t aLoadType) {
           (IsForceReloadType(aLoadType) && IsSubframe()));
 }
 
-nsresult BrowsingContext::CheckLocationChangeRateLimit(CallerType aCallerType) {
+nsresult BrowsingContext::CheckNavigationRateLimit(CallerType aCallerType) {
   // We only rate limit non system callers
   if (aCallerType == CallerType::System) {
     return NS_OK;
   }
 
   // Fetch rate limiting preferences
-  uint32_t limitCount =
-      StaticPrefs::dom_navigation_locationChangeRateLimit_count();
+  uint32_t limitCount = StaticPrefs::dom_navigation_navigationRateLimit_count();
   uint32_t timeSpanSeconds =
-      StaticPrefs::dom_navigation_locationChangeRateLimit_timespan();
+      StaticPrefs::dom_navigation_navigationRateLimit_timespan();
 
   // Disable throttling if either of the preferences is set to 0.
   if (limitCount == 0 || timeSpanSeconds == 0) {
@@ -3849,15 +3848,15 @@ nsresult BrowsingContext::CheckLocationChangeRateLimit(CallerType aCallerType) {
 
   TimeDuration throttleSpan = TimeDuration::FromSeconds(timeSpanSeconds);
 
-  if (mLocationChangeRateLimitSpanStart.IsNull() ||
-      ((TimeStamp::Now() - mLocationChangeRateLimitSpanStart) > throttleSpan)) {
+  if (mNavigationRateLimitSpanStart.IsNull() ||
+      ((TimeStamp::Now() - mNavigationRateLimitSpanStart) > throttleSpan)) {
     // Initial call or timespan exceeded, reset counter and timespan.
-    mLocationChangeRateLimitSpanStart = TimeStamp::Now();
-    mLocationChangeRateLimitCount = 1;
+    mNavigationRateLimitSpanStart = TimeStamp::Now();
+    mNavigationRateLimitCount = 1;
     return NS_OK;
   }
 
-  if (mLocationChangeRateLimitCount >= limitCount) {
+  if (mNavigationRateLimitCount >= limitCount) {
     // Rate limit reached
 
     Document* doc = GetDocument();
@@ -3870,14 +3869,14 @@ nsresult BrowsingContext::CheckLocationChangeRateLimit(CallerType aCallerType) {
     return NS_ERROR_DOM_SECURITY_ERR;
   }
 
-  mLocationChangeRateLimitCount++;
+  mNavigationRateLimitCount++;
   return NS_OK;
 }
 
-void BrowsingContext::ResetLocationChangeRateLimit() {
+void BrowsingContext::ResetNavigationRateLimit() {
   // Resetting the timestamp object will cause the check function to
   // init again and reset the rate limit.
-  mLocationChangeRateLimitSpanStart = TimeStamp();
+  mNavigationRateLimitSpanStart = TimeStamp();
 }
 
 void BrowsingContext::LocationCreated(dom::Location* aLocation) {
diff --git a/docshell/base/BrowsingContext.h b/docshell/base/BrowsingContext.h
index e566841106c..98a34cab1ff 100644
--- a/docshell/base/BrowsingContext.h
+++ b/docshell/base/BrowsingContext.h
@@ -900,13 +900,13 @@ class BrowsingContext : public nsILoadContext, public nsWrapperCache {
 
   // Checks if we reached the rate limit for calls to Location and History API.
   // The rate limit is controlled by the
-  // "dom.navigation.locationChangeRateLimit" prefs.
+  // "dom.navigation.navigationRateLimit" prefs.
   // Rate limit applies per BrowsingContext.
   // Returns NS_OK if we are below the rate limit and increments the counter.
   // Returns NS_ERROR_DOM_SECURITY_ERR if limit is reached.
-  nsresult CheckLocationChangeRateLimit(CallerType aCallerType);
+  nsresult CheckNavigationRateLimit(CallerType aCallerType);
 
-  void ResetLocationChangeRateLimit();
+  void ResetNavigationRateLimit();
 
   mozilla::dom::DisplayMode DisplayMode() { return Top()->GetDisplayMode(); }
 
@@ -1453,9 +1453,9 @@ class BrowsingContext : public nsILoadContext, public nsWrapperCache {
   nsTArray<std::function<void(uint64_t)>> mDiscardListeners;
 
   // Counter and time span for rate limiting Location and History API calls.
-  // Used by CheckLocationChangeRateLimit. Do not apply cross-process.
-  uint32_t mLocationChangeRateLimitCount;
-  mozilla::TimeStamp mLocationChangeRateLimitSpanStart;
+  // Used by CheckNavigationRateLimit. Do not apply cross-process.
+  uint32_t mNavigationRateLimitCount;
+  mozilla::TimeStamp mNavigationRateLimitSpanStart;
 
   mozilla::LinkedList<dom::Location> mLocations;
 };
diff --git a/docshell/shistory/ChildSHistory.cpp b/docshell/shistory/ChildSHistory.cpp
index 3e3cf09493d..855d7646177 100644
--- a/docshell/shistory/ChildSHistory.cpp
+++ b/docshell/shistory/ChildSHistory.cpp
@@ -177,7 +177,7 @@ void ChildSHistory::AsyncGo(int32_t aOffset, bool aRequireUserInteraction,
   MOZ_LOG(gSHLog, LogLevel::Debug,
           ("ChildSHistory::AsyncGo(%d), current index = %d", aOffset,
            index.value()));
-  nsresult rv = mBrowsingContext->CheckLocationChangeRateLimit(aCallerType);
+  nsresult rv = mBrowsingContext->CheckNavigationRateLimit(aCallerType);
   if (NS_FAILED(rv)) {
     MOZ_LOG(gSHLog, LogLevel::Debug, ("Rejected"));
     aRv.Throw(rv);
diff --git a/docshell/test/navigation/test_rate_limit_location_change.html b/docshell/test/navigation/test_rate_limit_location_change.html
index b1b51b92dde..9fefce12af8 100644
--- a/docshell/test/navigation/test_rate_limit_location_change.html
+++ b/docshell/test/navigation/test_rate_limit_location_change.html
@@ -17,8 +17,8 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=1314912
 
   async function setup() {
     await SpecialPowers.pushPrefEnv({set: [
-      ["dom.navigation.locationChangeRateLimit.count", RATE_LIMIT_COUNT],
-      ["dom.navigation.locationChangeRateLimit.timespan", RATE_LIMIT_TIME_SPAN]]});
+      ["dom.navigation.navigationRateLimit.count", RATE_LIMIT_COUNT],
+      ["dom.navigation.navigationRateLimit.timespan", RATE_LIMIT_TIME_SPAN]]});
   }
 
   let inc = 0;
@@ -26,6 +26,7 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=1314912
   const rateLimitedFunctions = (win) => ({
     "history.replaceState": () => win.history.replaceState(null, "test", `${win.location.href}#${inc++}`),
     "history.pushState":  () => win.history.pushState(null, "test", `${win.location.href}#${inc++}`),
+    "history.SetScrollRestoration": () => win.history.scrollRestoration = "auto",
     "history.back": () => win.history.back(),
     "history.forward": () => win.history.forward(),
     "history.go": () => win.history.go(-1),
@@ -53,7 +54,7 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=1314912
     Object.entries(rateLimitedFunctions(win)).forEach(([name, fn]) => {
       // Reset the rate limit for the next run.
       info("Reset rate limit.");
-      SpecialPowers.wrap(win).browsingContext.resetLocationChangeRateLimit();
+      SpecialPowers.wrap(win).browsingContext.resetNavigationRateLimit();
 
       info(`Calling ${name} ${RATE_LIMIT_COUNT} times to reach the rate limit.`);
       for(let i = 0; i< RATE_LIMIT_COUNT; i++) {
@@ -83,7 +84,7 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=1314912
 
     // Cleanup
     win.close();
-    SpecialPowers.wrap(win).browsingContext.resetLocationChangeRateLimit();
+    SpecialPowers.wrap(win).browsingContext.resetNavigationRateLimit();
     SimpleTest.finish();
   }
 
diff --git a/dom/base/Location.cpp b/dom/base/Location.cpp
index 9edb9e0b6f8..11603ce493e 100644
--- a/dom/base/Location.cpp
+++ b/dom/base/Location.cpp
@@ -562,7 +562,7 @@ void Location::Reload(bool aForceget, nsIPrincipal& aSubjectPrincipal,
                               ? CallerType::System
                               : CallerType::NonSystem;
 
-  nsresult rv = bc->CheckLocationChangeRateLimit(callerType);
+  nsresult rv = bc->CheckNavigationRateLimit(callerType);
   if (NS_FAILED(rv)) {
     aRv.Throw(rv);
     return;
diff --git a/dom/base/LocationBase.cpp b/dom/base/LocationBase.cpp
index 7ca3cf83a42..94ab819c94c 100644
--- a/dom/base/LocationBase.cpp
+++ b/dom/base/LocationBase.cpp
@@ -128,7 +128,7 @@ void LocationBase::SetURI(nsIURI* aURI, nsIPrincipal& aSubjectPrincipal,
                               ? CallerType::System
                               : CallerType::NonSystem;
 
-  nsresult rv = bc->CheckLocationChangeRateLimit(callerType);
+  nsresult rv = bc->CheckNavigationRateLimit(callerType);
   if (NS_FAILED(rv)) {
     aRv.Throw(rv);
     return;
diff --git a/dom/base/nsHistory.cpp b/dom/base/nsHistory.cpp
index 99994a73ccd..39d2f12df3f 100644
--- a/dom/base/nsHistory.cpp
+++ b/dom/base/nsHistory.cpp
@@ -72,7 +72,8 @@ uint32_t nsHistory::GetLength(ErrorResult& aRv) const {
   return len >= 0 ? len : 0;
 }
 
-ScrollRestoration nsHistory::GetScrollRestoration(mozilla::ErrorResult& aRv) {
+ScrollRestoration nsHistory::GetScrollRestoration(
+    mozilla::dom::CallerType aCallerType, mozilla::ErrorResult& aRv) {
   nsCOMPtr<nsPIDOMWindowInner> win(do_QueryReferent(mInnerWindow));
   if (!win || !win->HasActiveDocument() || !win->GetDocShell()) {
     aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
@@ -88,6 +89,7 @@ ScrollRestoration nsHistory::GetScrollRestoration(mozilla::ErrorResult& aRv) {
 }
 
 void nsHistory::SetScrollRestoration(mozilla::dom::ScrollRestoration aMode,
+                                     mozilla::dom::CallerType aCallerType,
                                      mozilla::ErrorResult& aRv) {
   nsCOMPtr<nsPIDOMWindowInner> win(do_QueryReferent(mInnerWindow));
   if (!win || !win->HasActiveDocument() || !win->GetDocShell()) {
Loading diff…

Regression Test / PoC

shipped with the fix
diff --git a/docshell/test/navigation/test_rate_limit_location_change.html b/docshell/test/navigation/test_rate_limit_location_change.html
index b1b51b92dde..9fefce12af8 100644
--- a/docshell/test/navigation/test_rate_limit_location_change.html
+++ b/docshell/test/navigation/test_rate_limit_location_change.html
@@ -17,8 +17,8 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=1314912
 
   async function setup() {
     await SpecialPowers.pushPrefEnv({set: [
-      ["dom.navigation.locationChangeRateLimit.count", RATE_LIMIT_COUNT],
-      ["dom.navigation.locationChangeRateLimit.timespan", RATE_LIMIT_TIME_SPAN]]});
+      ["dom.navigation.navigationRateLimit.count", RATE_LIMIT_COUNT],
+      ["dom.navigation.navigationRateLimit.timespan", RATE_LIMIT_TIME_SPAN]]});
   }
 
   let inc = 0;
@@ -26,6 +26,7 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=1314912
   const rateLimitedFunctions = (win) => ({
     "history.replaceState": () => win.history.replaceState(null, "test", `${win.location.href}#${inc++}`),
     "history.pushState":  () => win.history.pushState(null, "test", `${win.location.href}#${inc++}`),
+    "history.SetScrollRestoration": () => win.history.scrollRestoration = "auto",
     "history.back": () => win.history.back(),
     "history.forward": () => win.history.forward(),
     "history.go": () => win.history.go(-1),
@@ -53,7 +54,7 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=1314912
     Object.entries(rateLimitedFunctions(win)).forEach(([name, fn]) => {
       // Reset the rate limit for the next run.
       info("Reset rate limit.");
-      SpecialPowers.wrap(win).browsingContext.resetLocationChangeRateLimit();
+      SpecialPowers.wrap(win).browsingContext.resetNavigationRateLimit();
 
       info(`Calling ${name} ${RATE_LIMIT_COUNT} times to reach the rate limit.`);
       for(let i = 0; i< RATE_LIMIT_COUNT; i++) {
@@ -83,7 +84,7 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=1314912
 
     // Cleanup
     win.close();
-    SpecialPowers.wrap(win).browsingContext.resetLocationChangeRateLimit();
+    SpecialPowers.wrap(win).browsingContext.resetNavigationRateLimit();
     SimpleTest.finish();
   }
diff --git a/testing/marionette/client/marionette_driver/geckoinstance.py b/testing/marionette/client/marionette_driver/geckoinstance.py
index 1957d77f158..56a69260518 100644
--- a/testing/marionette/client/marionette_driver/geckoinstance.py
+++ b/testing/marionette/client/marionette_driver/geckoinstance.py
@@ -83,8 +83,8 @@ class GeckoInstance(object):
         # No slow script dialogs
         "dom.max_chrome_script_run_time": 0,
         "dom.max_script_run_time": 0,
-        # Disable location change rate limitation
-        "dom.navigation.locationChangeRateLimit.count": 0,
+        # Disable navigation change rate limitation
+        "dom.navigation.navigationRateLimit.count": 0,
         # DOM Push
         "dom.push.connection.enabled": False,
         # Screen Orientation API
Loading diff…