From a7c6783bdfd6bb99111eb98da491550b3f5970a0 Mon Sep 17 00:00:00 2001
From: Alex Moshchuk <alexmos@chromium.org>
Date: Thu, 30 Jul 2026 09:54:58 -0700
Subject: [PATCH] Validate opaque-origin precursors for MHTML subframe commits

CanCommitOriginAndUrl() returns CAN_COMMIT_ORIGIN_AND_URL early for any
opaque origin reported by an MHTML subframe in the main frame's
SiteInstance, without checking the origin's precursor tuple. For commits
that arrive without a browser-side NavigationRequest (the synchronous
about:blank commit, or a renderer-initiated same-document navigation)
this allows the renderer to commit an opaque origin whose precursor is
unrelated to the archive or the frame's inheritance chain, and have it
recorded in ChildProcessSecurityPolicy via AddCommittedOrigin().

Require the precursor (when present) to either match the tuple of the
URL being committed, or be inherited from an origin of another MHTML
frame that had already been admitted into the MHTML process (e.g.,
inherited from the parent frame when creating a new iframe).

Add a regression test that intercepts an MHTML subframe's synchronous
about:blank commit, rewrites its origin to an opaque origin with an
unrelated precursor, and verifies the renderer is terminated and the
precursor is not recorded as hosted by the process.

Bug: 517215407
Change-Id: I323064a68de0b3d86ddfc320fa56c0a357f59917
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8168022
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1671169}
---

diff --git a/content/browser/navigation_mhtml_browsertest.cc b/content/browser/navigation_mhtml_browsertest.cc
index 7126186..1684fa6e 100644
--- a/content/browser/navigation_mhtml_browsertest.cc
+++ b/content/browser/navigation_mhtml_browsertest.cc
@@ -18,6 +18,7 @@
 #include "content/browser/bad_message.h"
 #include "content/browser/renderer_host/navigation_request.h"
 #include "content/browser/renderer_host/render_frame_host_impl.h"
+#include "content/browser/security/cpsp/child_process_security_policy_impl.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/common/content_navigation_policy.h"
 #include "content/common/frame.mojom.h"
@@ -30,6 +31,7 @@
 #include "content/public/test/test_utils.h"
 #include "content/shell/browser/shell.h"
 #include "content/test/content_browser_test_utils_internal.h"
+#include "content/test/did_commit_navigation_interceptor.h"
 #include "content/test/frame_host_interceptor.h"
 #include "mojo/public/c/system/trap.h"
 #include "mojo/public/c/system/types.h"
@@ -516,6 +518,85 @@
   EXPECT_EQ(bad_message::RFH_INVALID_ORIGIN_ON_COMMIT, kill_waiter.Wait());
 }
 
+namespace {
+
+// Intercepts the synchronous about:blank commit of a child frame and replaces
+// the reported origin to test error handling for unexpected precursor origins.
+class SubframeSyncCommitOriginReplacer : public DidCommitNavigationInterceptor {
+ public:
+  SubframeSyncCommitOriginReplacer(WebContents* web_contents,
+                                   const url::Origin& origin)
+      : DidCommitNavigationInterceptor(web_contents), origin_(origin) {}
+
+  SubframeSyncCommitOriginReplacer(const SubframeSyncCommitOriginReplacer&) =
+      delete;
+  SubframeSyncCommitOriginReplacer& operator=(
+      const SubframeSyncCommitOriginReplacer&) = delete;
+
+  std::optional<bad_message::BadMessageReason> WaitForKill() {
+    return kill_waiter_->Wait();
+  }
+
+  bool did_intercept() const { return did_intercept_; }
+  int process_id() const { return process_id_; }
+
+ private:
+  bool WillProcessDidCommitNavigation(
+      RenderFrameHost* render_frame_host,
+      NavigationRequest* navigation_request,
+      mojom::DidCommitProvisionalLoadParamsPtr* params,
+      mojom::DidCommitProvisionalLoadInterfaceParamsPtr* interface_params)
+      override {
+    // Only target the synchronous about:blank commit of a child frame, which
+    // arrives without a browser-side NavigationRequest.
+    if (navigation_request || !render_frame_host->GetParent() ||
+        did_intercept_) {
+      return true;
+    }
+    did_intercept_ = true;
+    (*params)->origin = origin_;
+    process_id_ = render_frame_host->GetProcess()->GetDeprecatedID();
+    kill_waiter_ = std::make_unique<RenderProcessHostBadIpcMessageWaiter>(
+        render_frame_host->GetProcess());
+    return true;
+  }
+
+  const url::Origin origin_;
+  bool did_intercept_ = false;
+  int process_id_ = -1;
+  std::unique_ptr<RenderProcessHostBadIpcMessageWaiter> kill_waiter_;
+};
+
+}  // namespace
+
+// An MHTML subframe's synchronous about:blank commit inherits an opaque origin
+// derived from the main document. Ensure the browser rejects an opaque origin
+// whose precursor tuple does not match what the subframe could have inherited.
+IN_PROC_BROWSER_TEST_F(NavigationMhtmlBrowserTest,
+                       MhtmlSubframeOpaquePrecursorValidation) {
+  MhtmlArchive mhtml_archive;
+  mhtml_archive.AddHtmlDocument(GURL("http://example.com"),
+                                "<iframe src=\"about:blank\"></iframe>");
+  GURL mhtml_url = mhtml_archive.Write("index.mhtml");
+
+  url::Origin mismatched_origin =
+      url::Origin::Create(GURL("https://unrelated.example"))
+          .DeriveNewOpaqueOrigin();
+  SubframeSyncCommitOriginReplacer replacer(web_contents(), mismatched_origin);
+
+  // The result of NavigateToURL is not checked, since the main frame commits
+  // before the subframe's invalid commit terminates the renderer.
+  std::ignore = NavigateToURL(shell(), mhtml_url);
+  EXPECT_TRUE(replacer.did_intercept());
+
+  // The process must not be recorded as hosting the opaque origin with the
+  // mismatched precursor.
+  EXPECT_FALSE(ChildProcessSecurityPolicyImpl::GetInstance()->HostsOrigin(
+      replacer.process_id(), mismatched_origin));
+
+  EXPECT_EQ(bad_message::RFH_INVALID_ORIGIN_ON_COMMIT, replacer.WaitForKill());
+}
+
 // Load iframe with the content-ID scheme. The resource is found in the MHTML
 // archive.
 IN_PROC_BROWSER_TEST_F(NavigationMhtmlBrowserTest, IframeContentIdFound) {
diff --git a/content/browser/renderer_host/render_frame_host_impl.cc b/content/browser/renderer_host/render_frame_host_impl.cc
index d5f8e0b..38e2a7ee 100644
--- a/content/browser/renderer_host/render_frame_host_impl.cc
+++ b/content/browser/renderer_host/render_frame_host_impl.cc
@@ -12031,6 +12031,8 @@
     return CanCommitStatus::CANNOT_COMMIT_ORIGIN;
   }
 
+  auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
+
   // MHTML subframes can supply URLs at commit time that do not match the
   // process lock. For example, it can be either "cid:..." or arbitrary URL at
   // which the frame was at the time of generating the MHTML
@@ -12038,15 +12040,29 @@
   // the URL to commit in the process of the main frame.
   if (IsMhtmlSubframe()) {
     // Documents derived from an MHTML archive are behind sandbox flags, so
-    // their origin is opaque. The early-return below validates neither URL
-    // nor origin, so a compromised renderer could otherwise launder an
-    // arbitrary non-opaque origin past this point via
-    // DidCommitSameDocumentNavigation.
+    // their origin must be opaque.
     if (!origin.opaque()) {
       LogCanCommitOriginAndUrlFailureReason("mhtml_subframe_non_opaque_origin");
       return CanCommitStatus::CANNOT_COMMIT_ORIGIN;
     }
+
+    // Additionally, ensure the opaque origin's precursor is something this
+    // subframe could have legitimately produced: either derived from the URL
+    // being committed, or inherited from an existing frame's origin which had
+    // already been allowed into this process. This is important because this
+    // path skips the ChildProcessSecurityPolicy validation further down
+    // below.
     RenderFrameHostImpl* main_frame = GetMainFrame();
+    const url::SchemeHostPort precursor =
+        origin.GetTupleOrPrecursorTupleIfOpaque();
+    if (precursor.IsValid() && precursor != url::SchemeHostPort(url) &&
+        !policy->HostsOrigin(GetProcess()->GetDeprecatedID(), origin)) {
+      LogCanCommitOriginAndUrlFailureReason(
+          "mhtml_subframe_invalid_precursor_origin");
+      return CanCommitStatus::CANNOT_COMMIT_ORIGIN;
+    }
+
+    // Require the URL to commit in the process of the main frame.
     if (IsSameSiteInstance(main_frame)) {
       return CanCommitStatus::CAN_COMMIT_ORIGIN_AND_URL;
     }
@@ -12085,7 +12101,6 @@
   }
 
   // Check with ChildProcessSecurityPolicy, which enforces Site Isolation, etc.
-  auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
   const CanCommitStatus can_commit_status = policy->CanCommitOriginAndUrl(
       GetProcess()->GetDeprecatedID(), GetSiteInstance()->GetIsolationContext(),
       url_info);

diff --git a/content/browser/navigation_mhtml_browsertest.cc b/content/browser/navigation_mhtml_browsertest.cc
index 7126186..1684fa6e 100644
--- a/content/browser/navigation_mhtml_browsertest.cc
+++ b/content/browser/navigation_mhtml_browsertest.cc
@@ -18,6 +18,7 @@
 #include "content/browser/bad_message.h"
 #include "content/browser/renderer_host/navigation_request.h"
 #include "content/browser/renderer_host/render_frame_host_impl.h"
+#include "content/browser/security/cpsp/child_process_security_policy_impl.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/common/content_navigation_policy.h"
 #include "content/common/frame.mojom.h"
@@ -30,6 +31,7 @@
 #include "content/public/test/test_utils.h"
 #include "content/shell/browser/shell.h"
 #include "content/test/content_browser_test_utils_internal.h"
+#include "content/test/did_commit_navigation_interceptor.h"
 #include "content/test/frame_host_interceptor.h"
 #include "mojo/public/c/system/trap.h"
 #include "mojo/public/c/system/types.h"
@@ -516,6 +518,85 @@
   EXPECT_EQ(bad_message::RFH_INVALID_ORIGIN_ON_COMMIT, kill_waiter.Wait());
 }
 
+namespace {
+
+// Intercepts the synchronous about:blank commit of a child frame and replaces
+// the reported origin to test error handling for unexpected precursor origins.
+class SubframeSyncCommitOriginReplacer : public DidCommitNavigationInterceptor {
+ public:
+  SubframeSyncCommitOriginReplacer(WebContents* web_contents,
+                                   const url::Origin& origin)
+      : DidCommitNavigationInterceptor(web_contents), origin_(origin) {}
+
+  SubframeSyncCommitOriginReplacer(const SubframeSyncCommitOriginReplacer&) =
+      delete;
+  SubframeSyncCommitOriginReplacer& operator=(
+      const SubframeSyncCommitOriginReplacer&) = delete;
+
+  std::optional<bad_message::BadMessageReason> WaitForKill() {
+    return kill_waiter_->Wait();
+  }
+
+  bool did_intercept() const { return did_intercept_; }
+  int process_id() const { return process_id_; }
+
+ private:
+  bool WillProcessDidCommitNavigation(
+      RenderFrameHost* render_frame_host,
+      NavigationRequest* navigation_request,
+      mojom::DidCommitProvisionalLoadParamsPtr* params,
+      mojom::DidCommitProvisionalLoadInterfaceParamsPtr* interface_params)
+      override {
+    // Only target the synchronous about:blank commit of a child frame, which
+    // arrives without a browser-side NavigationRequest.
+    if (navigation_request || !render_frame_host->GetParent() ||
+        did_intercept_) {
+      return true;
+    }
+    did_intercept_ = true;
+    (*params)->origin = origin_;
+    process_id_ = render_frame_host->GetProcess()->GetDeprecatedID();
+    kill_waiter_ = std::make_unique<RenderProcessHostBadIpcMessageWaiter>(
+        render_frame_host->GetProcess());
+    return true;
+  }
+
+  const url::Origin origin_;
+  bool did_intercept_ = false;
+  int process_id_ = -1;
+  std::unique_ptr<RenderProcessHostBadIpcMessageWaiter> kill_waiter_;
+};
+
+}  // namespace
+
+// An MHTML subframe's synchronous about:blank commit inherits an opaque origin
+// derived from the main document. Ensure the browser rejects an opaque origin
+// whose precursor tuple does not match what the subframe could have inherited.
+IN_PROC_BROWSER_TEST_F(NavigationMhtmlBrowserTest,
+                       MhtmlSubframeOpaquePrecursorValidation) {
+  MhtmlArchive mhtml_archive;
+  mhtml_archive.AddHtmlDocument(GURL("http://example.com"),
+                                "<iframe src=\"about:blank\"></iframe>");
+  GURL mhtml_url = mhtml_archive.Write("index.mhtml");
+
+  url::Origin mismatched_origin =
+      url::Origin::Create(GURL("https://unrelated.example"))
+          .DeriveNewOpaqueOrigin();
+  SubframeSyncCommitOriginReplacer replacer(web_contents(), mismatched_origin);
+
+  // The result of NavigateToURL is not checked, since the main frame commits
+  // before the subframe's invalid commit terminates the renderer.
+  std::ignore = NavigateToURL(shell(), mhtml_url);
+  EXPECT_TRUE(replacer.did_intercept());
+
+  // The process must not be recorded as hosting the opaque origin with the
+  // mismatched precursor.
+  EXPECT_FALSE(ChildProcessSecurityPolicyImpl::GetInstance()->HostsOrigin(
+      replacer.process_id(), mismatched_origin));
+
+  EXPECT_EQ(bad_message::RFH_INVALID_ORIGIN_ON_COMMIT, replacer.WaitForKill());
+}
+
 // Load iframe with the content-ID scheme. The resource is found in the MHTML
 // archive.
 IN_PROC_BROWSER_TEST_F(NavigationMhtmlBrowserTest, IframeContentIdFound) {