From eb0b9aba64dbe619399fb2e4e1be6a784524c809 Mon Sep 17 00:00:00 2001
From: Ming-Ying Chung <mych@chromium.org>
Date: Thu, 28 May 2026 00:41:24 -0700
Subject: [PATCH] [FSA] Fix UAF in ShowFilePickerOnUIThread.

The `ShowFilePickerOnUIThread()` function in the browser process was
caching raw pointers to `RenderFrameHost` and `WebContents` which could
become dangling if the frame was destroyed during nested message loops
spun by intermediate operations like `CanShowFilePicker()` or
`ForSecurityDropFullscreen()`.

Th CL re-resolves the RenderFrameHost and WebContents pointers from
the stored `GlobalRenderFrameHostId` after these operations and aborts
safely if they are no longer active or valid.

Bug: 516677924
Change-Id: I50046d8f72f139f06f885309847b5179a32b7b37
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7871715
Reviewed-by: Fergal Daly <fergal@chromium.org>
Commit-Queue: Ming-Ying Chung <mych@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1637522}
---

diff --git a/content/browser/file_system_access/file_system_access_manager_impl.cc b/content/browser/file_system_access/file_system_access_manager_impl.cc
index e40ce0c..51049e7 100644
--- a/content/browser/file_system_access/file_system_access_manager_impl.cc
+++ b/content/browser/file_system_access/file_system_access_manager_impl.cc
@@ -19,6 +19,7 @@
 #include "base/functional/bind.h"
 #include "base/functional/callback_helpers.h"
 #include "base/i18n/file_util_icu.h"
+#include "base/memory/raw_ptr.h"
 #include "base/notreached.h"
 #include "base/strings/string_util.h"
 #include "base/strings/string_view_util.h"
@@ -96,6 +97,40 @@
 constexpr char kThirdPartyIframesNotAllowedToShowFilePicker[] =
     "Third party iframes are not allowed to show a file picker.";
 
+// Holds resolved and validated frame objects. All pointers are guaranteed to be
+// non-null and active if this struct is returned.
+struct ResolvedFrame {
+  raw_ptr<RenderFrameHost> rfh;
+  raw_ptr<WebContents> web_contents;
+  raw_ptr<RenderFrameHost> outermost_rfh;
+};
+
+// Resolves `frame_id` to its corresponding `RenderFrameHost`, `WebContents`,
+// and outermost `RenderFrameHost`, and validates that they are all non-null
+// and active.
+// Returns a `ResolvedFrame` struct if ALL resolved objects are valid and
+// active, i.e. non-null; otherwise, returns `std::nullopt`.
+//
+// This check is critical because intermediate operations, like permission
+// checks or security prompts, can run nested message loops during which the
+// calling frame or WebContents can be destroyed or navigated.
+std::optional<ResolvedFrame> ResolveAndValidateFrame(
+    GlobalRenderFrameHostId frame_id) {
+  RenderFrameHost* rfh = RenderFrameHost::FromID(frame_id);
+  if (!rfh || !rfh->IsActive()) {
+    return std::nullopt;
+  }
+  WebContents* web_contents = WebContents::FromRenderFrameHost(rfh);
+  if (!web_contents) {
+    return std::nullopt;
+  }
+  RenderFrameHost* outermost_rfh = rfh->GetOutermostMainFrame();
+  if (!outermost_rfh || !outermost_rfh->IsActive()) {
+    return std::nullopt;
+  }
+  return ResolvedFrame{rfh, web_contents, outermost_rfh};
+}
+
 #if BUILDFLAG(IS_ANDROID)
 // Adaptor between FileSystemChooser::ResultCallback and FileSelectListener
 // used when delegating file choosing to WebContentsDelegate.
@@ -207,6 +242,18 @@
                           {});
                       return;
                     });
+    // `CanShowFilePicker()` runs nested message loops during which the frame
+    // could be destroyed or navigated. Re-resolve and re-validate the frame.
+    auto re_resolved = ResolveAndValidateFrame(frame_id);
+    if (!re_resolved) {
+      std::move(callback).Run(file_system_access_error::FromStatus(
+                                  FileSystemAccessStatus::kOperationAborted),
+                              {});
+      return;
+    }
+    rfh = re_resolved->rfh;
+    web_contents = re_resolved->web_contents;
+    outermost_rfh = re_resolved->outermost_rfh;
   }
 
 #if BUILDFLAG(IS_ANDROID)
@@ -247,12 +294,20 @@
 
   auto blocker =
       web_contents->ForSecurityDropFullscreen(display::kInvalidDisplayId);
-  if (!blocker) {
+
+  // `ForSecurityDropFullscreen()` can also run nested message loops. Re-resolve
+  // and re-validate the frame, and ensure the fullscreen blocker was
+  // successfully acquired.
+  auto post_fullscreen_resolved = ResolveAndValidateFrame(frame_id);
+  if (!post_fullscreen_resolved || !blocker) {
     std::move(callback).Run(file_system_access_error::FromStatus(
                                 FileSystemAccessStatus::kOperationAborted),
                             {});
     return;
   }
+  rfh = post_fullscreen_resolved->rfh;
+  web_contents = post_fullscreen_resolved->web_contents;
+  outermost_rfh = post_fullscreen_resolved->outermost_rfh;
 
   FileSystemChooser::ScopedObjects scoped_objects(
       /*fullscreen_block=*/std::move(*blocker),
diff --git a/content/browser/file_system_access/file_system_access_manager_impl_unittest.cc b/content/browser/file_system_access/file_system_access_manager_impl_unittest.cc
index 904349e..b6037546 100644
--- a/content/browser/file_system_access/file_system_access_manager_impl_unittest.cc
+++ b/content/browser/file_system_access/file_system_access_manager_impl_unittest.cc
@@ -1608,6 +1608,80 @@
   ASSERT_TRUE(future.Wait());
 }
 
+// Covers the re-entrancy and lifetime safety of `RenderFrameHost` and
+// `WebContents` in `ShowFilePickerOnUIThread`. Specifically, it covers the
+// scenario where the frame is detached/destroyed during the
+// `CanShowFilePicker` permission check.
+//
+// Without the fix, this scenario triggers a crash when the code attempts to
+// access the destroyed `WebContents` to drop fullscreen
+// `web_contents->ForSecurityDropFullscreen(...)`.
+//
+// Note: If destruction occurred during the fullscreen exit loop instead, the
+// crash would occur in `FileSystemChooser::CreateAndShow()` when dereferencing
+// the dangling RenderFrameHost.
+TEST_F(FileSystemAccessManagerImplTest,
+       ChooseEntries_CanShowFilePickerDestroysFrameUAF) {
+  static_cast<TestRenderFrameHost*>(web_contents_->GetPrimaryMainFrame())
+      ->SimulateUserActivation();
+
+  mojo::Remote<blink::mojom::FileSystemAccessManager> manager_remote;
+  FileSystemAccessManagerImpl::BindingContext binding_context = {
+      kTestStorageKey, kTestURL,
+      web_contents_->GetPrimaryMainFrame()->GetGlobalId()};
+  manager_->BindReceiver(binding_context,
+                         manager_remote.BindNewPipeAndPassReceiver());
+
+  EXPECT_CALL(permission_context_,
+              CanObtainReadPermission(kTestStorageKey.origin()))
+      .WillOnce(testing::Return(true));
+
+  EXPECT_CALL(
+      permission_context_,
+      GetWellKnownDirectoryPath(blink::mojom::WellKnownDirectory::kDirDocuments,
+                                kTestStorageKey.origin()))
+      .WillOnce(testing::Return(base::FilePath()));
+  EXPECT_CALL(permission_context_,
+              GetLastPickedDirectory(kTestStorageKey.origin(), std::string()))
+      .WillOnce(testing::Return(PathInfo()));
+  EXPECT_CALL(permission_context_, GetPickerTitle(testing::_))
+      .WillOnce(testing::Return(std::u16string()));
+
+  // Mock CanShowFilePicker to synchronously destroy the WebContents, which
+  // destroys the RFH. This simulates the frame being detached/destroyed
+  // during the yielding permission check.
+  EXPECT_CALL(permission_context_, CanShowFilePicker(testing::_))
+      .WillOnce([&](content::RenderFrameHost* rfh) {
+        auto* temp_wc = web_contents_.get();
+        web_contents_ = nullptr;
+        web_contents_factory_.DestroyWebContents(temp_wc);
+        return base::ok();
+      });
+
+  auto open_file_picker_options = blink::mojom::OpenFilePickerOptions::New(
+      blink::mojom::AcceptsTypesInfo::New(
+          std::vector<blink::mojom::ChooseFileSystemEntryAcceptsOptionPtr>(),
+          /*include_accepts_all=*/true),
+      /*can_select_multiple_files=*/false);
+  auto picker_options = blink::mojom::FilePickerOptions::New(
+      blink::mojom::TypeSpecificFilePickerOptionsUnion::
+          NewOpenFilePickerOptions(std::move(open_file_picker_options)),
+      /*starting_directory_id=*/std::string(),
+      blink::mojom::FilePickerStartInOptionsUnionPtr());
+
+  base::test::TestFuture<blink::mojom::FileSystemAccessErrorPtr,
+                         std::vector<blink::mojom::FileSystemAccessEntryPtr>>
+      future;
+  manager_remote->ChooseEntries(std::move(picker_options),
+                                future.GetCallback());
+  ASSERT_TRUE(future.Wait());
+
+  // The call should be aborted gracefully since the frame was destroyed.
+  // Without the fix, this would have crashed during the picker call.
+  EXPECT_EQ(blink::mojom::FileSystemAccessStatus::kOperationAborted,
+            future.Get<0>()->status);
+}
+
 TEST_F(FileSystemAccessManagerImplTest,

diff --git a/content/browser/file_system_access/file_system_access_manager_impl_unittest.cc b/content/browser/file_system_access/file_system_access_manager_impl_unittest.cc
index 904349e..b6037546 100644
--- a/content/browser/file_system_access/file_system_access_manager_impl_unittest.cc
+++ b/content/browser/file_system_access/file_system_access_manager_impl_unittest.cc
@@ -1608,6 +1608,80 @@
   ASSERT_TRUE(future.Wait());
 }
 
+// Covers the re-entrancy and lifetime safety of `RenderFrameHost` and
+// `WebContents` in `ShowFilePickerOnUIThread`. Specifically, it covers the
+// scenario where the frame is detached/destroyed during the
+// `CanShowFilePicker` permission check.
+//
+// Without the fix, this scenario triggers a crash when the code attempts to
+// access the destroyed `WebContents` to drop fullscreen
+// `web_contents->ForSecurityDropFullscreen(...)`.
+//
+// Note: If destruction occurred during the fullscreen exit loop instead, the
+// crash would occur in `FileSystemChooser::CreateAndShow()` when dereferencing
+// the dangling RenderFrameHost.
+TEST_F(FileSystemAccessManagerImplTest,
+       ChooseEntries_CanShowFilePickerDestroysFrameUAF) {
+  static_cast<TestRenderFrameHost*>(web_contents_->GetPrimaryMainFrame())
+      ->SimulateUserActivation();
+
+  mojo::Remote<blink::mojom::FileSystemAccessManager> manager_remote;
+  FileSystemAccessManagerImpl::BindingContext binding_context = {
+      kTestStorageKey, kTestURL,
+      web_contents_->GetPrimaryMainFrame()->GetGlobalId()};
+  manager_->BindReceiver(binding_context,
+                         manager_remote.BindNewPipeAndPassReceiver());
+
+  EXPECT_CALL(permission_context_,
+              CanObtainReadPermission(kTestStorageKey.origin()))
+      .WillOnce(testing::Return(true));
+
+  EXPECT_CALL(
+      permission_context_,
+      GetWellKnownDirectoryPath(blink::mojom::WellKnownDirectory::kDirDocuments,
+                                kTestStorageKey.origin()))
+      .WillOnce(testing::Return(base::FilePath()));
+  EXPECT_CALL(permission_context_,
+              GetLastPickedDirectory(kTestStorageKey.origin(), std::string()))
+      .WillOnce(testing::Return(PathInfo()));
+  EXPECT_CALL(permission_context_, GetPickerTitle(testing::_))
+      .WillOnce(testing::Return(std::u16string()));
+
+  // Mock CanShowFilePicker to synchronously destroy the WebContents, which
+  // destroys the RFH. This simulates the frame being detached/destroyed
+  // during the yielding permission check.
+  EXPECT_CALL(permission_context_, CanShowFilePicker(testing::_))
+      .WillOnce([&](content::RenderFrameHost* rfh) {
+        auto* temp_wc = web_contents_.get();
+        web_contents_ = nullptr;
+        web_contents_factory_.DestroyWebContents(temp_wc);
+        return base::ok();
+      });
+
+  auto open_file_picker_options = blink::mojom::OpenFilePickerOptions::New(
+      blink::mojom::AcceptsTypesInfo::New(
+          std::vector<blink::mojom::ChooseFileSystemEntryAcceptsOptionPtr>(),
+          /*include_accepts_all=*/true),
+      /*can_select_multiple_files=*/false);
+  auto picker_options = blink::mojom::FilePickerOptions::New(
+      blink::mojom::TypeSpecificFilePickerOptionsUnion::
+          NewOpenFilePickerOptions(std::move(open_file_picker_options)),
+      /*starting_directory_id=*/std::string(),
+      blink::mojom::FilePickerStartInOptionsUnionPtr());
+
+  base::test::TestFuture<blink::mojom::FileSystemAccessErrorPtr,
+                         std::vector<blink::mojom::FileSystemAccessEntryPtr>>
+      future;
+  manager_remote->ChooseEntries(std::move(picker_options),
+                                future.GetCallback());
+  ASSERT_TRUE(future.Wait());
+
+  // The call should be aborted gracefully since the frame was destroyed.
+  // Without the fix, this would have crashed during the picker call.
+  EXPECT_EQ(blink::mojom::FileSystemAccessStatus::kOperationAborted,
+            future.Get<0>()->status);
+}
+
 TEST_F(FileSystemAccessManagerImplTest,
        ChooseEntries_CrossOriginDenialDoesNotConsumeActivation) {
   auto* rfh =