CVE-2025-3071

Low chrome Logic Error 📄 Reporter bug report 🔧 Commit mapped

Overview

Low

Severity

—

CVSS

Exploited ITW

Fixed

Fix Status

ImpactInappropriate implementation in Navigations

DescriptionInappropriate implementation in Navigations

ComponentNavigations

Bug ClassLogic Error

Tracker40051596

Fix commitcdb798347ad7 (chromium/src) +109/-22

CISA KEVNot listed

CreditedDavid Erceg

Disclosed2025-04-01

Changed Functions

Function	Change	Notes
`if` content/browser/renderer_host/navigation_request.cc	modified
`if` content/browser/renderer_host/render_frame_host_impl.cc	modified

Files Changed

content/browser/renderer_host/navigation_controller_impl.cc
content/browser/renderer_host/navigation_controller_impl_browsertest.cc
content/browser/renderer_host/navigation_entry_impl.cc
content/browser/renderer_host/navigation_request.cc
content/browser/renderer_host/render_frame_host_impl.cc

Patch

view commit

From cdb798347ad78bcb1cffdbdc0e06eb1ec1867a57 Mon Sep 17 00:00:00 2001
From: Charlie Reis <[email protected]>
Date: Fri, 14 Feb 2025 14:04:46 -0800
Subject: [PATCH] Do not reuse document sequence number on cross-origin navigations.

Cross-document navigations only reuse document sequence numbers for
"logically related" navigations. This CL ensures that they are not
reused for cross-origin navigations as well (e.g., about:blank with
different owners), by computing this in the browser process and sending
it in the CommitNavigationParams.

Note that an exception is needed for error pages, which should preserve
the document sequence number and other history item state, though they
transition to and from an opaque origin with the same precursor.

Bug: 40051596
Change-Id: I502251ce12ec8b3e613596b914fbe8a63330b4fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6185347
Reviewed-by: Daniel Cheng <[email protected]>
Commit-Queue: Charlie Reis <[email protected]>
Reviewed-by: Rakina Zata Amni <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1420736}
---

diff --git a/content/browser/renderer_host/navigation_controller_impl.cc b/content/browser/renderer_host/navigation_controller_impl.cc
index 597384a..715a22a 100644
--- a/content/browser/renderer_host/navigation_controller_impl.cc
+++ b/content/browser/renderer_host/navigation_controller_impl.cc
@@ -4125,7 +4125,8 @@
           /*visited_link_salt=*/std::nullopt,
           /*local_surface_id=*/std::nullopt,
           node->current_frame_host()->GetCachedPermissionStatuses(),
-          /*should_skip_screentshot=*/false);
+          /*should_skip_screentshot=*/false,
+          /*force_new_document_sequence_number=*/false);
 #if BUILDFLAG(IS_ANDROID)
   if (ValidateDataURLAsString(params.data_url_as_string)) {
     commit_params->data_url_as_string = params.data_url_as_string->as_string();
diff --git a/content/browser/renderer_host/navigation_controller_impl_browsertest.cc b/content/browser/renderer_host/navigation_controller_impl_browsertest.cc
index 7923c93..9a7f5fe60 100644
--- a/content/browser/renderer_host/navigation_controller_impl_browsertest.cc
+++ b/content/browser/renderer_host/navigation_controller_impl_browsertest.cc
@@ -5304,9 +5304,7 @@
   scoped_refptr<FrameNavigationEntry> frame_entry_blank_data =
       controller.GetLastCommittedEntry()->GetFrameEntry(inner_frame);
   int64_t dsn_blank = frame_entry_blank_data->document_sequence_number();
-  //  TODO(crbug.com/40051596): Fix Blink to use a different document sequence
-  //  number for this navigation.
-  EXPECT_EQ(dsn_a1, dsn_blank);
+  EXPECT_NE(dsn_a1, dsn_blank);
 
   // Go back. This should not be treated as same-document, because the origin
   // changed in the previous navigation.
@@ -19169,6 +19167,10 @@
     EXPECT_EQ(previous_frame_entry,
               controller.GetLastCommittedEntry()->GetFrameEntry(child));
     EXPECT_TRUE(capturer.did_replace_entry());
+
+    // We keep the same history.state value, even in the error page, so that it
+    // can be used when the load later succeeds in step 4.
+    EXPECT_EQ("foo", EvalJs(child, "history.state"));
   }
 
   // 4) Test successfully navigating the subframe to the same URL after a failed
diff --git a/content/browser/renderer_host/navigation_entry_impl.cc b/content/browser/renderer_host/navigation_entry_impl.cc
index c1d1d97d..5578f71 100644
--- a/content/browser/renderer_host/navigation_entry_impl.cc
+++ b/content/browser/renderer_host/navigation_entry_impl.cc
@@ -1005,7 +1005,8 @@
           /*visited_link_salt=*/std::nullopt,
           /*local_surface_id=*/std::nullopt,
           /*initial_permission_statuses=*/std::nullopt,
-          /*should_skip_screenshot*/ false);
+          /*should_skip_screenshot*/ false,
+          /*force_new_document_sequence_number=*/false);
 #if BUILDFLAG(IS_ANDROID)
   // `data_url_as_string` is saved in NavigationEntry but should only be used by
   // main frames, because loadData* navigations can only happen on the main
diff --git a/content/browser/renderer_host/navigation_request.cc b/content/browser/renderer_host/navigation_request.cc
index 4921db5..07309925 100644
--- a/content/browser/renderer_host/navigation_request.cc
+++ b/content/browser/renderer_host/navigation_request.cc
@@ -1445,7 +1445,8 @@
           /*visited_link_salt=*/std::nullopt,
           /*local_surface_id=*/std::nullopt,
           frame_tree_node->current_frame_host()->GetCachedPermissionStatuses(),
-          /*should_skip_screenshot=*/false);
+          /*should_skip_screenshot=*/false,
+          /*force_new_document_sequence_number=*/false);
 
   commit_params->navigation_timing->system_entropy_at_navigation_start =
       SystemEntropyUtils::ComputeSystemEntropyForFrameTreeNode(
@@ -1597,7 +1598,8 @@
           /*visited_link_salt=*/std::nullopt,
           /*local_surface_id=*/std::nullopt,
           render_frame_host->GetCachedPermissionStatuses(),
-          /*should_skip_screenshot=*/false);
+          /*should_skip_screenshot=*/false,
+          /*force_new_document_sequence_number=*/false);
   blink::mojom::BeginNavigationParamsPtr begin_params =
       blink::mojom::BeginNavigationParams::New();
   std::unique_ptr<NavigationRequest> navigation_request(new NavigationRequest(
@@ -6061,6 +6063,30 @@
     return;
   }
 
+  // Ensure the renderer does not reuse the document sequence number for
+  // cross-origin navigations (which can lead to later bugs with same-document
+  // navigations appearing to be cross-origin). All error pages have unique
+  // opaque origins and are considered cross-origin.
+  //
+  // The one exception is for transitioning to or from a compatible error page,
+  // which preserves state in case a temporary failure later succeeds. Here, we
+  // must check for any cases where the committing error page has a valid
+  // precursor that agrees with the current document, whether the current
+  // document is already an error page or not. (The renderer process will narrow
+  // DSN reuse further, to cases the URL is a closer match, ignoring fragments.)
+  // See also CommitNavigation for the other direction.
+  // TODO(crbug.com/396645697): Use a different technique for preserving history
+  // item state on error pages, separate from the error page's own state.
+  RenderFrameHostImpl* previous_rfh = frame_tree_node()->current_frame_host();
+  const url::Origin& previous_origin = previous_rfh->GetLastCommittedOrigin();
+  bool is_error_page_with_same_precursor =
+      previous_origin.GetTupleOrPrecursorTupleIfOpaque().IsValid() &&
+      commit_params_->origin_to_commit->GetTupleOrPrecursorTupleIfOpaque() ==
+          previous_origin.GetTupleOrPrecursorTupleIfOpaque();
+  if (!is_error_page_with_same_precursor) {
+    commit_params_->force_new_document_sequence_number = true;
+  }
+
   PopulateDocumentTokenForCrossDocumentNavigation();
   // Use a separate cache shard, and no cookies, for error pages.
   isolation_info_for_subresources_ =
@@ -6375,6 +6401,35 @@
         GetNavigationController()->GetNavigationApiHistoryEntryVectors(
             frame_tree_node_, this);
     PopulateDocumentTokenForCrossDocumentNavigation();
+
+    // Ensure the renderer does not reuse the document sequence number for
+    // cross-origin navigations (which can lead to later bugs with same-document
+    // navigations appearing to be cross-origin).
+    //
+    // The one exception is for transitioning to or from a compatible error
+    // page, which preserves state in case a temporary failure later succeeds.
+    // Here, we must check for the case that the user navigates from an error
+    // page to a non-error page that matches the (valid) precursor origin. See
+    // also CommitErrorPage for the other direction.
+    // TODO(crbug.com/396645697): Use a different technique for preserving
+    // history item state on error pages, separate from the error page's own
+    // state.
+    RenderFrameHostImpl* previous_rfh = frame_tree_node()->current_frame_host();
+    const url::Origin& previous_origin = previous_rfh->GetLastCommittedOrigin();
+    // Skip this check if kUseBrowserCalculatedOrigin is disabled.
+    if (base::FeatureList::IsEnabled(features::kUseBrowserCalculatedOrigin)) {
+      bool is_cross_origin_navigation =
+          !commit_params_->origin_to_commit->IsSameOriginWith(previous_origin);
+      bool compatible_with_error_page =
+          previous_rfh->IsErrorDocument() &&
+          previous_origin.GetTupleOrPrecursorTupleIfOpaque().IsValid() &&
+          commit_params_->origin_to_commit
+                  ->GetTupleOrPrecursorTupleIfOpaque() ==
+              previous_origin.GetTupleOrPrecursorTupleIfOpaque();
+      if (is_cross_origin_navigation && !compatible_with_error_page) {
+        commit_params_->force_new_document_sequence_number = true;
+      }
+    }
   }
 
   if (early_hints_manager_) {
diff --git a/content/browser/renderer_host/render_frame_host_impl.cc b/content/browser/renderer_host/render_frame_host_impl.cc
index 19601b38..69e121b0 100644
--- a/content/browser/renderer_host/render_frame_host_impl.cc
+++ b/content/browser/renderer_host/render_frame_host_impl.cc
@@ -12227,9 +12227,10 @@
   commit_params->should_skip_screenshot =
       NavigationTransitionUtils::ShouldSkipScreenshot(*navigation_request);
 
+  RenderFrameHostImpl* previous_rfh =
+      navigation_request->frame_tree_node()->current_frame_host();
   if (is_same_document) {
-    DCHECK_EQ(navigation_request->frame_tree_node()->current_frame_host(),
-              this);
+    DCHECK_EQ(previous_rfh, this);
     const base::UnguessableToken& navigation_token =
         commit_params->navigation_token;
     commit_params->has_ua_visual_transition =
@@ -12358,8 +12359,7 @@
     // point just before the navigation commits.
     // TODO(altimin, crbug.com/933147): Remove this logic after we are done with
     // implementing back-forward cache.
-    if (!GetParent() &&
-        navigation_request->frame_tree_node()->current_frame_host() == this) {
+    if (!GetParent() && previous_rfh == this) {
       if (NavigationEntryImpl* last_committed_entry =
               NavigationEntryImpl::FromNavigationEntry(
                   navigation_request->frame_tree_node()
@@ -12390,10 +12390,7 @@
     // processes.
     const bool maybe_new_process_is_used =
         GetProcess()->GetRenderFrameHostCount() == 1 &&

Loading diff…

Regression Test / PoC

shipped with the fix

diff --git a/content/browser/renderer_host/navigation_controller_impl_browsertest.cc b/content/browser/renderer_host/navigation_controller_impl_browsertest.cc
index 7923c93..9a7f5fe60 100644
--- a/content/browser/renderer_host/navigation_controller_impl_browsertest.cc
+++ b/content/browser/renderer_host/navigation_controller_impl_browsertest.cc
@@ -5304,9 +5304,7 @@
   scoped_refptr<FrameNavigationEntry> frame_entry_blank_data =
       controller.GetLastCommittedEntry()->GetFrameEntry(inner_frame);
   int64_t dsn_blank = frame_entry_blank_data->document_sequence_number();
-  //  TODO(crbug.com/40051596): Fix Blink to use a different document sequence
-  //  number for this navigation.
-  EXPECT_EQ(dsn_a1, dsn_blank);
+  EXPECT_NE(dsn_a1, dsn_blank);
 
   // Go back. This should not be treated as same-document, because the origin
   // changed in the previous navigation.
@@ -19169,6 +19167,10 @@
     EXPECT_EQ(previous_frame_entry,
               controller.GetLastCommittedEntry()->GetFrameEntry(child));
     EXPECT_TRUE(capturer.did_replace_entry());
+
+    // We keep the same history.state value, even in the error page, so that it
+    // can be used when the load later succeeds in step 4.
+    EXPECT_EQ("foo", EvalJs(child, "history.state"));
   }
 
   // 4) Test successfully navigating the subframe to the same URL after a failed

Loading diff…

Original Bug Report

reported by [email protected]

Security: Possible to cause incorrect origin to be used when performing a same document navigation

VULNERABILITY DETAILS
It’s possible for a window to perform a same document navigation between its original URL (e.g. a http/https URL) and about:blank. If another window within the same namespace navigates the first window to about:blank and then goes back, a same document navigation will be performed, though the origin will now have changed to that of the second window.

This allows, for example, a data: window to control another window whose origin is opaque (the same opaque origin as the data: window), but whose visible URL is the original (http/https) URL.

VERSION
Chrome Version: Tested on 80.0.3987.116 (stable) and 82.0.4067.0 (canary)
Operating System: Windows 10, version 1909

REPRODUCTION CASE

Download the attached files into a directory, then run the following command:

python3 -m http.server 8080

In the browser, navigate to the following location:

http://localhost:8080/index.html

Wait three seconds.
index.html contains two iframes: a data: iframe and an iframe pointing to http://localhost:8080/iframe.html nested within it.

Click within the innermost iframe. This should open a new window with a visible URL of http://localhost:8080/iframe.html and an opaque origin (that matches the origin of the data: iframe on the original page).

The data: iframe controls this window. To demonstrate this, it adds the following content to the new page:

Content set by data: iframe

This seemingly shouldn’t be possible, as the data: frame has an opaque origin, one that’s not the same as the origin represented by a http://localhost:8080 URL.

CREDIT INFORMATION
Reporter credit: David Erceg

View on issue tracker

Links in the report

References

Vendor advisory Fix commit Bug tracker NVD

On This Page