From a12e651f6f62614ab4a0c0eed6d686fec8414682 Mon Sep 17 00:00:00 2001
From: Alex Moshchuk <alexmos@chromium.org>
Date: Sat, 28 Feb 2026 11:18:02 -0800
Subject: [PATCH] Add browser-side validation for allow-popups sandboxed frame attribute

Sandboxed frames created without an allow-popups attribute should not
be allowed to create popups. Previously, this attribute was only
checked in the renderer. This CL adds additional validation in
RenderFrameHostImpl::CreateNewWindow(), the browser process's
entrypoint for creating popups via web APIs like window.open().

Bug: 487471101
Change-Id: I0776f9d6df4200ecf20b29e8ecd18efa2edcd45a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7617636
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Auto-Submit: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Mark Pearson <mpearson@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1592057}
---

diff --git a/content/browser/bad_message.h b/content/browser/bad_message.h
index 6873983..372417d 100644
--- a/content/browser/bad_message.h
+++ b/content/browser/bad_message.h
@@ -360,6 +360,7 @@
   RFH_INVALID_DOCUMENT_SEQUENCE_NUMBER = 332,
   RFH_NEW_ISOLATED_WEB_APP_PERMISSION_POLICIES = 333,
   RFH_CREATE_NEW_WINDOW_INVALID_DISPOSITION = 334,
+  RFH_CREATE_NEW_WINDOW_FROM_SANDBOXED_FRAME = 335,
 
   // Please add new elements here. The naming convention is abbreviated class
   // name (e.g. RenderFrameHost becomes RFH) plus a unique description of the
diff --git a/content/browser/renderer_host/render_frame_host_impl.cc b/content/browser/renderer_host/render_frame_host_impl.cc
index a71222b..b5cd09d 100644
--- a/content/browser/renderer_host/render_frame_host_impl.cc
+++ b/content/browser/renderer_host/render_frame_host_impl.cc
@@ -9968,6 +9968,16 @@
     return;
   }
 
+  // Sandboxed frames should only be allowed to create a popup when they have
+  // the "allow-popups" attribute. This should have already been checked by the
+  // renderer process (see blink::CreateNewWindow()), and this browser-side
+  // check defends against compromised renderers.
+  if (IsSandboxed(network::mojom::WebSandboxFlags::kPopups)) {
+    bad_message::ReceivedBadMessage(
+        GetProcess(), bad_message::RFH_CREATE_NEW_WINDOW_FROM_SANDBOXED_FRAME);
+    return;
+  }
+
   // Fenced frames that have revoked network access can't open popups.
   if (base::FeatureList::IsEnabled(
           blink::features::kFencedFramesLocalUnpartitionedDataAccess)) {
diff --git a/content/browser/renderer_host/render_frame_host_impl.h b/content/browser/renderer_host/render_frame_host_impl.h
index e7821c9b..81c5099 100644
--- a/content/browser/renderer_host/render_frame_host_impl.h
+++ b/content/browser/renderer_host/render_frame_host_impl.h
@@ -3466,6 +3466,8 @@
                            AttemptDuplicateRenderWidgetHost);
   FRIEND_TEST_ALL_PREFIXES(SecurityExploitBrowserTest,
                            BindToWebUIFromWebViaMojo);
+  FRIEND_TEST_ALL_PREFIXES(SecurityExploitBrowserTest,
+                           WindowOpenDisallowedFromSandboxedFrame);
   FRIEND_TEST_ALL_PREFIXES(SitePerProcessBrowserTest,
                            RenderViewHostIsNotReusedAfterDelayedUnloadACK);
   FRIEND_TEST_ALL_PREFIXES(SitePerProcessBrowserTest,
diff --git a/content/browser/security_exploit_browsertest.cc b/content/browser/security_exploit_browsertest.cc
index 4d0c4617..802f0ad 100644
--- a/content/browser/security_exploit_browsertest.cc
+++ b/content/browser/security_exploit_browsertest.cc
@@ -1111,6 +1111,55 @@
             kill_waiter.Wait());
 }
 
+// Regression test for browser-side validation of the allow-popups sandbox
+// attribute. See https://crbug.com/487471101.
+IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest,
+                       WindowOpenDisallowedFromSandboxedFrame) {
+  // Explicitly isolating b.com helps establish consistent expectations for the
+  // sandboxed frame across platforms.
+  IsolateOrigin("b.com");
+
+  GURL main_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), main_url));
+
+  WebContentsImpl* web_contents =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+  FrameTreeNode* root = web_contents->GetPrimaryFrameTree().root();
+  RenderFrameHostImpl* main_frame = root->current_frame_host();
+
+  // Create cross-site sandboxed child frame. The frame lacks the allow-popups
+  // attribute, so it should not be allowed to create popups.
+  GURL child_url(embedded_test_server()->GetURL("b.com", "/title2.html"));
+  {
+    std::string js_str = base::StringPrintf(
+        "var frame = document.createElement('iframe'); "
+        "frame.sandbox = 'allow-scripts'; "
+        "frame.src = '%s'; "
+        "document.body.appendChild(frame);",
+        child_url.spec().c_str());
+    EXPECT_TRUE(ExecJs(main_frame, js_str));
+    ASSERT_TRUE(WaitForLoadStop(web_contents));
+  }
+
+  RenderFrameHostImpl* subframe = root->child_at(0)->current_frame_host();
+  EXPECT_TRUE(
+      subframe->GetSiteInstance()->GetSecurityPrincipal().IsSandboxed());
+  EXPECT_TRUE(subframe->IsSandboxed(network::mojom::WebSandboxFlags::kPopups));
+
+  // Simulate that the b.com renderer is compromised and sends an IPC to open a
+  // popup, bypassing renderer-side checks in blink::CreateNewWindow(). The
+  // browser process should detect this and terminate the renderer.
+  RenderProcessHostBadIpcMessageWaiter kill_waiter(subframe->GetProcess());
+  mojom::CreateNewWindowParamsPtr params = mojom::CreateNewWindowParams::New();
+  params->target_url = GURL("about:blank");
+  params->disposition = WindowOpenDisposition::NEW_FOREGROUND_TAB;
+  subframe->CreateNewWindow(std::move(params), base::DoNothing());
+  EXPECT_EQ(bad_message::RFH_CREATE_NEW_WINDOW_FROM_SANDBOXED_FRAME,
+            kill_waiter.Wait());
+  EXPECT_FALSE(subframe->IsRenderFrameLive());
+  EXPECT_EQ(1u, Shell::windows().size());
+}
+
 // Test verifying that a compromised renderer can't lie about the source_origin
 // passed along with the RouteMessageEvent() mojo message.  Similar to the test
 // above, but exercises a scenario where the source origin is opaque and the
diff --git a/tools/metrics/histograms/metadata/stability/enums.xml b/tools/metrics/histograms/metadata/stability/enums.xml
index bfaf4c1..de97da0 100644
--- a/tools/metrics/histograms/metadata/stability/enums.xml
+++ b/tools/metrics/histograms/metadata/stability/enums.xml
@@ -500,6 +500,7 @@
   <int value="332" label="RFH_INVALID_DOCUMENT_SEQUENCE_NUMBER"/>
   <int value="333" label="RFH_NEW_ISOLATED_WEB_APP_PERMISSION_POLICIES"/>
   <int value="334" label="RFH_CREATE_NEW_WINDOW_INVALID_DISPOSITION"/>
+  <int value="335" label="RFH_CREATE_NEW_WINDOW_FROM_SANDBOXED_FRAME"/>
 </enum>
 
 <enum name="BadMessageReasonExtensions">

diff --git a/content/browser/security_exploit_browsertest.cc b/content/browser/security_exploit_browsertest.cc
index 4d0c4617..802f0ad 100644
--- a/content/browser/security_exploit_browsertest.cc
+++ b/content/browser/security_exploit_browsertest.cc
@@ -1111,6 +1111,55 @@
             kill_waiter.Wait());
 }
 
+// Regression test for browser-side validation of the allow-popups sandbox
+// attribute. See https://crbug.com/487471101.
+IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest,
+                       WindowOpenDisallowedFromSandboxedFrame) {
+  // Explicitly isolating b.com helps establish consistent expectations for the
+  // sandboxed frame across platforms.
+  IsolateOrigin("b.com");
+
+  GURL main_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), main_url));
+
+  WebContentsImpl* web_contents =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+  FrameTreeNode* root = web_contents->GetPrimaryFrameTree().root();
+  RenderFrameHostImpl* main_frame = root->current_frame_host();
+
+  // Create cross-site sandboxed child frame. The frame lacks the allow-popups
+  // attribute, so it should not be allowed to create popups.
+  GURL child_url(embedded_test_server()->GetURL("b.com", "/title2.html"));
+  {
+    std::string js_str = base::StringPrintf(
+        "var frame = document.createElement('iframe'); "
+        "frame.sandbox = 'allow-scripts'; "
+        "frame.src = '%s'; "
+        "document.body.appendChild(frame);",
+        child_url.spec().c_str());
+    EXPECT_TRUE(ExecJs(main_frame, js_str));
+    ASSERT_TRUE(WaitForLoadStop(web_contents));
+  }
+
+  RenderFrameHostImpl* subframe = root->child_at(0)->current_frame_host();
+  EXPECT_TRUE(
+      subframe->GetSiteInstance()->GetSecurityPrincipal().IsSandboxed());
+  EXPECT_TRUE(subframe->IsSandboxed(network::mojom::WebSandboxFlags::kPopups));
+
+  // Simulate that the b.com renderer is compromised and sends an IPC to open a
+  // popup, bypassing renderer-side checks in blink::CreateNewWindow(). The
+  // browser process should detect this and terminate the renderer.
+  RenderProcessHostBadIpcMessageWaiter kill_waiter(subframe->GetProcess());
+  mojom::CreateNewWindowParamsPtr params = mojom::CreateNewWindowParams::New();
+  params->target_url = GURL("about:blank");
+  params->disposition = WindowOpenDisposition::NEW_FOREGROUND_TAB;
+  subframe->CreateNewWindow(std::move(params), base::DoNothing());
+  EXPECT_EQ(bad_message::RFH_CREATE_NEW_WINDOW_FROM_SANDBOXED_FRAME,
+            kill_waiter.Wait());
+  EXPECT_FALSE(subframe->IsRenderFrameLive());
+  EXPECT_EQ(1u, Shell::windows().size());
+}
+
 // Test verifying that a compromised renderer can't lie about the source_origin
 // passed along with the RouteMessageEvent() mojo message.  Similar to the test
 // above, but exercises a scenario where the source origin is opaque and the