From 27e39e926cdb0492188053e5dfb918ae5c796ea4 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Mon, 26 Oct 2026 16:57:02 -0400 Subject: [PATCH] fix: use upstream's File System Access path matcher (#55328) * fix: use upstream's File System Access path matcher Electron kept its own copy of ShouldBlockAccessToPath() and the Windows MaybeIsLocalUNCPath() helpers. That copy never evaluated the suffix block rules, so the kBlockWrite rules for .git/hooks, .git/config and .git/modules were not enforced, and it lacked the hypervisor host-share servers (mac, psf, vmware-host, .host, vboxsvr, vboxsrv) and trailing-dot handling that upstream added in 158.0.8120.0. Extend refactor_expose_file_system_access_blocklist.patch to move upstream's ShouldBlockAccessToPath() and its UNC helpers verbatim into chrome_file_system_access_blocklist.cc, exposed as a static member of ChromeFileSystemAccessPermissionContext, and call it from Electron with the access trigger so kBlockWrite rules only apply to save and programmatic write operations, matching Chrome. Future upstream changes to the matcher now conflict on roll instead of drifting. This replaces Electron's stricter UNC checks with upstream's: '_' is now allowed in UNC server names, and only drive (X$) and ADMIN$/IPC$/PRINT$/FAX$ admin shares are blocked rather than every '$'-suffixed share. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01MtuMgvwAcw7t9Q6HEB31Vx * test: cover Windows UNC classification in the File System Access blocklist Shares a fixture directory over loopback SMB and pastes it under different UNC server and share names, checking that local server names (localhost, 127.0.0.1, and a trailing-dot localhost when it resolves) and drive admin shares are blocked while other servers and non-admin hidden shares are not. The hidden share and trailing-dot cases pin down the intentional changes from Electron's previous matcher. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01MtuMgvwAcw7t9Q6HEB31Vx * test: fix the UNC spec's localhost case and share names A file URL drops a "localhost" host, so pasting \\localhost\share reached Chromium as a path with no server. Drop that case; 127.0.0.1 and the trailing-dot localhost still cover local server names. Keep share names to 12 characters so net share doesn't stop to ask about MS-DOS clients, and answer that prompt anyway in case it appears. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01MtuMgvwAcw7t9Q6HEB31Vx --------- Co-authored-by: Claude --- ..._expose_file_system_access_blocklist.patch | 535 ++++++++++++++++-- .../file_system_access_permission_context.cc | 149 +---- .../file_system_access_permission_context.h | 2 + spec/chromium.spec.ts | 155 +++++ 4 files changed, 675 insertions(+), 166 deletions(-) diff --git a/patches/chromium/refactor_expose_file_system_access_blocklist.patch b/patches/chromium/refactor_expose_file_system_access_blocklist.patch index 2aa028bf91..dd2e4c9c14 100644 --- a/patches/chromium/refactor_expose_file_system_access_blocklist.patch +++ b/patches/chromium/refactor_expose_file_system_access_blocklist.patch @@ -7,15 +7,20 @@ This CL exposes the file system access blocklist publicly so that we can leverag it in Electron and prevent drift from Chrome's blocklist. We should look for a way to upstream this change to Chrome. -NormalizeFilePathForBlocklist() and GenerateBlockPaths() are declared as static -members in the header, and their definitions (along with the NormalizeFilePath() -and MaybeStripMacDataVolumePrefix() helpers they use) are moved verbatim out of +NormalizeFilePathForBlocklist(), GenerateBlockPaths() and +ShouldBlockAccessToPath() are declared as static members in the header, and +their definitions (along with the NormalizeFilePath() and +MaybeStripMacDataVolumePrefix() helpers, and the Windows-only +ContainsInvalidDNSCharacter(), IsLocalUNCServer() and MaybeIsLocalUNCPath() +helpers they use) are moved verbatim out of chrome_file_system_access_permission_context.cc into a new -chrome_file_system_access_blocklist.cc, which Electron compiles. Upstream changes -to the blocklist therefore conflict on the hunk that removes them from -chrome_file_system_access_permission_context.cc instead of being silently -dropped; when resolving such a conflict, carry the change over into -chrome_file_system_access_blocklist.cc. +chrome_file_system_access_blocklist.cc, which Electron compiles. Electron +therefore uses Chrome's own path matcher as well as its rules, including suffix +rules such as .git/hooks, kBlockWrite handling and the local UNC path checks. +Upstream changes to the blocklist or the matcher conflict on the hunks that +remove them from chrome_file_system_access_permission_context.cc instead of +being silently dropped; when resolving such a conflict, carry the change over +into chrome_file_system_access_blocklist.cc. As of 158.0.8118.0, chrome_file_system_access_blocklist.cc also carries the upstream kBlockPaths additions from that version (~/.config on all platforms, @@ -36,10 +41,10 @@ index e5146cc3ed80f558fe729e76158463a4d6fed518..cf3a3a321286972783b8bb3e85fc50b6 "file_system_access_permission_context_factory.cc", diff --git a/chrome/browser/file_system_access/chrome_file_system_access_blocklist.cc b/chrome/browser/file_system_access/chrome_file_system_access_blocklist.cc new file mode 100644 -index 0000000000000000000000000000000000000000..7cec4f2dd24f5c376293b62aad51994c7601814c +index 0000000000000000000000000000000000000000..c26eb34183aa46fd2d5886df768ab909e7484fde --- /dev/null +++ b/chrome/browser/file_system_access/chrome_file_system_access_blocklist.cc -@@ -0,0 +1,330 @@ +@@ -0,0 +1,567 @@ +// Copyright 2026 The Chromium Authors +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. @@ -50,13 +55,16 @@ index 0000000000000000000000000000000000000000..7cec4f2dd24f5c376293b62aad51994c +// chrome_file_system_access_permission_context.cc can still use Chrome's +// blocklist rules. + ++#include +#include ++#include + +#include "base/base_paths.h" +#include "base/check.h" +#include "base/check_op.h" +#include "base/files/file_path.h" +#include "base/files/file_util.h" ++#include "base/logging.h" +#include "base/path_service.h" +#include "build/build_config.h" +#include "chrome/browser/file_system_access/chrome_file_system_access_permission_context.h" @@ -64,6 +72,99 @@ index 0000000000000000000000000000000000000000..7cec4f2dd24f5c376293b62aad51994c + +namespace { + ++#if BUILDFLAG(IS_WIN) ++bool ContainsInvalidDNSCharacter(base::FilePath::StringType hostname) { ++ for (base::FilePath::CharType c : hostname) { ++ if (!((c >= L'A' && c <= L'Z') || (c >= L'a' && c <= L'z') || ++ (c >= L'0' && c <= L'9') || (c == L'.') || (c == L'-') || ++ (c == L'_'))) { ++ return true; ++ } ++ } ++ return false; ++} ++ ++bool IsLocalUNCServer(base::FilePath::StringViewType server) { ++ if (server.size() > 1 && server.back() == FILE_PATH_LITERAL('.')) { ++ server.remove_suffix(1); ++ } ++ static constexpr base::FilePath::StringViewType kLocalServers[] = { ++ FILE_PATH_LITERAL("localhost"), ++ FILE_PATH_LITERAL("wsl.localhost"), ++ // Hypervisor host-share redirectors (Parallels, VMware, VirtualBox). ++ FILE_PATH_LITERAL("mac"), ++ FILE_PATH_LITERAL("psf"), ++ FILE_PATH_LITERAL("vmware-host"), ++ FILE_PATH_LITERAL(".host"), ++ FILE_PATH_LITERAL("vboxsvr"), ++ FILE_PATH_LITERAL("vboxsrv"), ++ }; ++ for (const auto& candidate : kLocalServers) { ++ if (base::FilePath::CompareEqualIgnoreCase(server, candidate)) { ++ return true; ++ } ++ } ++ return false; ++} ++ ++// Returns true if the path is a Universal Naming Convention (UNC) path pointing ++// to a local system path, device namespace, WSL loopback redirector, or ++// hypervisor host-share namespace. ++bool MaybeIsLocalUNCPath(const base::FilePath& path) { ++ if (!path.IsNetwork()) { ++ return false; ++ } ++ ++ const std::vector components = ++ path.GetComponents(); ++ ++ // Check for server name that could represent a local or host system. We only ++ // check for a very short list, as it is impossible to cover all different ++ // variants on Windows. ++ if (components.size() >= 2 && ++ (IsLocalUNCServer(components[1]) || ++ components[1] == FILE_PATH_LITERAL("127.0.0.1") || ++ components[1] == FILE_PATH_LITERAL(".") || ++ components[1] == FILE_PATH_LITERAL("?") || ++ ContainsInvalidDNSCharacter(components[1]))) { ++ return true; ++ } ++ ++ // Check *admin* shares only (drive admin like "C$" and named admin). ++ // Note: the share component is typically components[2], but we scan all ++ // components defensively in case the structure changes. ++ for (size_t i = 2; i < components.size(); ++i) { ++ const auto& component = components[i]; ++ ++ // component ends with "$" ++ if (!component.empty() && component.back() == L'$') { ++ // Drive admin share: "C$".."Z$" (case-insensitive on the letter). ++ if (component.size() == 2 && ++ ((component[0] >= L'A' && component[0] <= L'Z') || ++ (component[0] >= L'a' && component[0] <= L'z'))) { ++ return true; ++ } ++ ++ // Named admin shares: "ADMIN$", "IPC$", "PRINT$", and "FAX$" ++ if (base::FilePath::CompareEqualIgnoreCase(component, ++ FILE_PATH_LITERAL("ADMIN$")) || ++ base::FilePath::CompareEqualIgnoreCase(component, ++ FILE_PATH_LITERAL("IPC$")) || ++ base::FilePath::CompareEqualIgnoreCase(component, ++ FILE_PATH_LITERAL("PRINT$")) || ++ base::FilePath::CompareEqualIgnoreCase(component, ++ FILE_PATH_LITERAL("FAX$"))) { ++ return true; ++ } ++ ++ // Otherwise, it is just a hidden share (e.g. "Share$")—do not block. ++ } ++ } ++ ++ return false; ++} ++#endif ++ +#if BUILDFLAG(IS_MAC) +// Returns a file path with the "/System/Volumes/Data" firmlink prefix stripped, +// mapping paths on the macOS Data volume back to the root "/" namespace. @@ -370,14 +471,248 @@ index 0000000000000000000000000000000000000000..7cec4f2dd24f5c376293b62aad51994c + + return block_path_rules; +} ++ ++// Checks if `path` should be blocked by the `rules`. ++// The BlockType of the nearest ancestor of a path to check is what ++// ultimately determines if a path is blocked or not. If a blocked path is a ++// descendent of another blocked path, then it may override the ++// child-blocking policy of its ancestor. For example, if /home blocks all ++// children, but /home/downloads does not, then /home/downloads/file.ext ++// will *not* be blocked. ++// static ++bool ChromeFileSystemAccessPermissionContext::ShouldBlockAccessToPath( ++ bool should_normalize_file_path, ++ base::FilePath path, ++ HandleType handle_type, ++ AccessTrigger access_trigger, ++ std::vector ++ extra_rules, ++ ChromeFileSystemAccessPermissionContext::BlockPathRules block_path_rules, ++ base::FilePath profile_path) { ++ CHECK(!path.empty(), base::NotFatalUntil::M161); ++ CHECK(path.IsAbsolute(), base::NotFatalUntil::M161); ++ ++ path = NormalizeFilePathForBlocklist(path, should_normalize_file_path); ++ profile_path = ++ NormalizeFilePathForBlocklist(profile_path, should_normalize_file_path); ++ for (auto& rule : extra_rules) { ++ rule.path = ++ NormalizeFilePathForBlocklist(rule.path, should_normalize_file_path); ++ } ++ ++#if BUILDFLAG(IS_WIN) ++ // On Windows, local UNC paths are rejected, as UNC path can be written in a ++ // way that can bypass the blocklist. ++ if (MaybeIsLocalUNCPath(path)) { ++ return true; ++ } ++#endif ++ ++ base::FilePath nearest_ancestor; ++ BlockType nearest_ancestor_block_type = BlockType::kDontBlockChildren; ++ auto should_block_with_rule = [&](const base::FilePath& block_path, ++ BlockType block_type) -> bool { ++ if (block_type == BlockType::kBlockWrite && ++ access_trigger != AccessTrigger::kSave && ++ access_trigger != AccessTrigger::kProgrammaticWrite) { ++ return false; ++ } ++ ++ if (path == block_path || path.IsParent(block_path)) { ++ LOG(ERROR) << "Blocking access to " << path ++ << " because it is a parent of " << block_path; ++ return true; ++ } ++ ++ if (block_path.IsParent(path) && ++ (nearest_ancestor.empty() || nearest_ancestor.IsParent(block_path))) { ++ nearest_ancestor = block_path; ++ nearest_ancestor_block_type = block_type; ++ } ++ return false; ++ }; ++ ++ for (const auto* block_rules_ptr : ++ {&extra_rules, &block_path_rules.block_path_rules_}) { ++ for (const auto& block : *block_rules_ptr) { ++ if (should_block_with_rule(block.path, block.type)) { ++ return true; ++ } ++ } ++ } ++ ++ for (const auto& rule : block_path_rules.profile_based_block_path_rules_) { ++ if (should_block_with_rule( ++ rule.path ? profile_path.Append(rule.path) : profile_path, ++ rule.type)) { ++ return true; ++ } ++ } ++ ++ std::vector path_components = ++ path.GetComponents(); ++ ++ // Checks if the path components contain the components of the suffix rule. ++ // For example, if the rule is `.git/hooks`, it will block paths like ++ // `/foo/bar/.git/hooks`. The `std::search` identifies the matching subrange ++ // using case-insensitive comparison and constructs a `current_path` from the ++ // root up to the end of the matched subrange (e.g., `/foo/bar/.git/hooks`). ++ // This path is then evaluated against the regular block rules. ++ for (const auto& rule : block_path_rules.suffix_block_path_rules_) { ++ base::FilePath rule_path(rule.path); ++ std::vector rule_components = ++ rule_path.GetComponents(); ++ if (rule_components.empty()) { ++ continue; ++ } ++ ++ auto it = path_components.begin(); ++ while (true) { ++ it = std::search(it, path_components.end(), rule_components.begin(), ++ rule_components.end(), ++ base::FilePath::CompareEqualIgnoreCase); ++ if (it == path_components.end()) { ++ break; ++ } ++ ++ base::FilePath current_path = base::FilePath(path_components[0]); ++ for (auto path_it = path_components.begin() + 1; ++ path_it != it + rule_components.size(); ++path_it) { ++ current_path = current_path.Append(*path_it); ++ } ++ ++ if (should_block_with_rule(current_path, rule.type)) { ++ return true; ++ } ++ ++ ++it; ++ } ++ } ++ ++ // The path we're checking is not in a potentially blocked directory, or the ++ // nearest ancestor does not block access to its children. Grant access. ++ if (nearest_ancestor.empty() || ++ nearest_ancestor_block_type == BlockType::kDontBlockChildren) { ++ VLOG(1) << "Not blocking access to " << path << " because it is inside " ++ << nearest_ancestor << " and it's kDontBlockChildren"; ++ return false; ++ } ++ ++ // The path we're checking is a file, and the nearest ancestor only blocks ++ // access to directories. Grant access. ++ if (handle_type == HandleType::kFile && ++ nearest_ancestor_block_type == BlockType::kBlockNestedDirectories) { ++ VLOG(1) << "Not blocking access to " << path << " because it is inside " ++ << nearest_ancestor << " and it's kBlockNestedDirectories"; ++ return false; ++ } ++ ++ // The nearest ancestor blocks access to its children, so block access. ++ VLOG(1) << "Blocking access to " << path << " because it is inside " ++ << nearest_ancestor << " and it's kBlockAllChildren"; ++ return true; ++} diff --git a/chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc b/chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc -index b32c79be552991c5414e5f7317edaee10a2e5193..1a5135f1a06d8f3f4db8477b3e9f25ea0c86ea73 100644 +index b32c79be552991c5414e5f7317edaee10a2e5193..779dd39a4227b61ad2f122eeb5668fe3b7ed03d6 100644 --- a/chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc +++ b/chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc -@@ -361,308 +361,8 @@ bool MaybeIsLocalUNCPath(const base::FilePath& path) { +@@ -268,541 +268,8 @@ void ShowFileSystemAccessDangerousFileDialogOnUIThread( } #endif +-#if BUILDFLAG(IS_WIN) +-bool ContainsInvalidDNSCharacter(base::FilePath::StringType hostname) { +- for (base::FilePath::CharType c : hostname) { +- if (!((c >= L'A' && c <= L'Z') || (c >= L'a' && c <= L'z') || +- (c >= L'0' && c <= L'9') || (c == L'.') || (c == L'-') || +- (c == L'_'))) { +- return true; +- } +- } +- return false; +-} +- +-bool IsLocalUNCServer(base::FilePath::StringViewType server) { +- if (server.size() > 1 && server.back() == FILE_PATH_LITERAL('.')) { +- server.remove_suffix(1); +- } +- static constexpr base::FilePath::StringViewType kLocalServers[] = { +- FILE_PATH_LITERAL("localhost"), +- FILE_PATH_LITERAL("wsl.localhost"), +- // Hypervisor host-share redirectors (Parallels, VMware, VirtualBox). +- FILE_PATH_LITERAL("mac"), +- FILE_PATH_LITERAL("psf"), +- FILE_PATH_LITERAL("vmware-host"), +- FILE_PATH_LITERAL(".host"), +- FILE_PATH_LITERAL("vboxsvr"), +- FILE_PATH_LITERAL("vboxsrv"), +- }; +- for (const auto& candidate : kLocalServers) { +- if (base::FilePath::CompareEqualIgnoreCase(server, candidate)) { +- return true; +- } +- } +- return false; +-} +- +-// Returns true if the path is a Universal Naming Convention (UNC) path pointing +-// to a local system path, device namespace, WSL loopback redirector, or +-// hypervisor host-share namespace. +-bool MaybeIsLocalUNCPath(const base::FilePath& path) { +- if (!path.IsNetwork()) { +- return false; +- } +- +- const std::vector components = +- path.GetComponents(); +- +- // Check for server name that could represent a local or host system. We only +- // check for a very short list, as it is impossible to cover all different +- // variants on Windows. +- if (components.size() >= 2 && +- (IsLocalUNCServer(components[1]) || +- components[1] == FILE_PATH_LITERAL("127.0.0.1") || +- components[1] == FILE_PATH_LITERAL(".") || +- components[1] == FILE_PATH_LITERAL("?") || +- ContainsInvalidDNSCharacter(components[1]))) { +- return true; +- } +- +- // Check *admin* shares only (drive admin like "C$" and named admin). +- // Note: the share component is typically components[2], but we scan all +- // components defensively in case the structure changes. +- for (size_t i = 2; i < components.size(); ++i) { +- const auto& component = components[i]; +- +- // component ends with "$" +- if (!component.empty() && component.back() == L'$') { +- // Drive admin share: "C$".."Z$" (case-insensitive on the letter). +- if (component.size() == 2 && +- ((component[0] >= L'A' && component[0] <= L'Z') || +- (component[0] >= L'a' && component[0] <= L'z'))) { +- return true; +- } +- +- // Named admin shares: "ADMIN$", "IPC$", "PRINT$", and "FAX$" +- if (base::FilePath::CompareEqualIgnoreCase(component, +- FILE_PATH_LITERAL("ADMIN$")) || +- base::FilePath::CompareEqualIgnoreCase(component, +- FILE_PATH_LITERAL("IPC$")) || +- base::FilePath::CompareEqualIgnoreCase(component, +- FILE_PATH_LITERAL("PRINT$")) || +- base::FilePath::CompareEqualIgnoreCase(component, +- FILE_PATH_LITERAL("FAX$"))) { +- return true; +- } +- +- // Otherwise, it is just a hidden share (e.g. "Share$")—do not block. +- } +- } +- +- return false; +-} +-#endif +- -#if BUILDFLAG(IS_MAC) -// Returns a file path with the "/System/Volumes/Data" firmlink prefix stripped, -// mapping paths on the macOS Data volume back to the root "/" namespace. @@ -680,33 +1015,154 @@ index b32c79be552991c5414e5f7317edaee10a2e5193..1a5135f1a06d8f3f4db8477b3e9f25ea - return block_path_rules; -} - - // Checks if `path` should be blocked by the `rules`. - // The BlockType of the nearest ancestor of a path to check is what - // ultimately determines if a path is blocked or not. If a blocked path is a -@@ -682,12 +382,15 @@ bool ShouldBlockAccessToPath( - CHECK(!path.empty(), base::NotFatalUntil::M161); - CHECK(path.IsAbsolute(), base::NotFatalUntil::M161); - +-// Checks if `path` should be blocked by the `rules`. +-// The BlockType of the nearest ancestor of a path to check is what +-// ultimately determines if a path is blocked or not. If a blocked path is a +-// descendent of another blocked path, then it may override the +-// child-blocking policy of its ancestor. For example, if /home blocks all +-// children, but /home/downloads does not, then /home/downloads/file.ext +-// will *not* be blocked. +-bool ShouldBlockAccessToPath( +- bool should_normalize_file_path, +- base::FilePath path, +- HandleType handle_type, +- AccessTrigger access_trigger, +- std::vector +- extra_rules, +- ChromeFileSystemAccessPermissionContext::BlockPathRules block_path_rules, +- base::FilePath profile_path) { +- CHECK(!path.empty(), base::NotFatalUntil::M161); +- CHECK(path.IsAbsolute(), base::NotFatalUntil::M161); +- - path = NormalizeFilePathForBlocklist(path, should_normalize_file_path); -+ path = ChromeFileSystemAccessPermissionContext::NormalizeFilePathForBlocklist( -+ path, should_normalize_file_path); - profile_path = +- profile_path = - NormalizeFilePathForBlocklist(profile_path, should_normalize_file_path); -+ ChromeFileSystemAccessPermissionContext::NormalizeFilePathForBlocklist( -+ profile_path, should_normalize_file_path); - for (auto& rule : extra_rules) { - rule.path = +- for (auto& rule : extra_rules) { +- rule.path = - NormalizeFilePathForBlocklist(rule.path, should_normalize_file_path); -+ ChromeFileSystemAccessPermissionContext::NormalizeFilePathForBlocklist( -+ rule.path, should_normalize_file_path); - } - - #if BUILDFLAG(IS_WIN) +- } +- +-#if BUILDFLAG(IS_WIN) +- // On Windows, local UNC paths are rejected, as UNC path can be written in a +- // way that can bypass the blocklist. +- if (MaybeIsLocalUNCPath(path)) { +- return true; +- } +-#endif +- +- base::FilePath nearest_ancestor; +- BlockType nearest_ancestor_block_type = BlockType::kDontBlockChildren; +- auto should_block_with_rule = [&](const base::FilePath& block_path, +- BlockType block_type) -> bool { +- if (block_type == BlockType::kBlockWrite && +- access_trigger != AccessTrigger::kSave && +- access_trigger != AccessTrigger::kProgrammaticWrite) { +- return false; +- } +- +- if (path == block_path || path.IsParent(block_path)) { +- LOG(ERROR) << "Blocking access to " << path +- << " because it is a parent of " << block_path; +- return true; +- } +- +- if (block_path.IsParent(path) && +- (nearest_ancestor.empty() || nearest_ancestor.IsParent(block_path))) { +- nearest_ancestor = block_path; +- nearest_ancestor_block_type = block_type; +- } +- return false; +- }; +- +- for (const auto* block_rules_ptr : +- {&extra_rules, &block_path_rules.block_path_rules_}) { +- for (const auto& block : *block_rules_ptr) { +- if (should_block_with_rule(block.path, block.type)) { +- return true; +- } +- } +- } +- +- for (const auto& rule : block_path_rules.profile_based_block_path_rules_) { +- if (should_block_with_rule( +- rule.path ? profile_path.Append(rule.path) : profile_path, +- rule.type)) { +- return true; +- } +- } +- +- std::vector path_components = +- path.GetComponents(); +- +- // Checks if the path components contain the components of the suffix rule. +- // For example, if the rule is `.git/hooks`, it will block paths like +- // `/foo/bar/.git/hooks`. The `std::search` identifies the matching subrange +- // using case-insensitive comparison and constructs a `current_path` from the +- // root up to the end of the matched subrange (e.g., `/foo/bar/.git/hooks`). +- // This path is then evaluated against the regular block rules. +- for (const auto& rule : block_path_rules.suffix_block_path_rules_) { +- base::FilePath rule_path(rule.path); +- std::vector rule_components = +- rule_path.GetComponents(); +- if (rule_components.empty()) { +- continue; +- } +- +- auto it = path_components.begin(); +- while (true) { +- it = std::search(it, path_components.end(), rule_components.begin(), +- rule_components.end(), +- base::FilePath::CompareEqualIgnoreCase); +- if (it == path_components.end()) { +- break; +- } +- +- base::FilePath current_path = base::FilePath(path_components[0]); +- for (auto path_it = path_components.begin() + 1; +- path_it != it + rule_components.size(); ++path_it) { +- current_path = current_path.Append(*path_it); +- } +- +- if (should_block_with_rule(current_path, rule.type)) { +- return true; +- } +- +- ++it; +- } +- } +- +- // The path we're checking is not in a potentially blocked directory, or the +- // nearest ancestor does not block access to its children. Grant access. +- if (nearest_ancestor.empty() || +- nearest_ancestor_block_type == BlockType::kDontBlockChildren) { +- VLOG(1) << "Not blocking access to " << path << " because it is inside " +- << nearest_ancestor << " and it's kDontBlockChildren"; +- return false; +- } +- +- // The path we're checking is a file, and the nearest ancestor only blocks +- // access to directories. Grant access. +- if (handle_type == HandleType::kFile && +- nearest_ancestor_block_type == BlockType::kBlockNestedDirectories) { +- VLOG(1) << "Not blocking access to " << path << " because it is inside " +- << nearest_ancestor << " and it's kBlockNestedDirectories"; +- return false; +- } +- +- // The nearest ancestor blocks access to its children, so block access. +- VLOG(1) << "Blocking access to " << path << " because it is inside " +- << nearest_ancestor << " and it's kBlockAllChildren"; +- return true; +-} +- + // Returns true if `child_path` is the same as or a descendant of + // `parent_path`, ignoring case differences. Unlike + // `base::FilePath::IsParent()`, this handles case-variant paths returned by diff --git a/chrome/browser/file_system_access/chrome_file_system_access_permission_context.h b/chrome/browser/file_system_access/chrome_file_system_access_permission_context.h -index 05ee182cf840bc925697a2bfa3d1c281579296c4..33595aaa73cf1301f1361bd07deb39ac30cc9201 100644 +index 05ee182cf840bc925697a2bfa3d1c281579296c4..e201e4d82b7298d6f90d78490aa342d915251a56 100644 --- a/chrome/browser/file_system_access/chrome_file_system_access_permission_context.h +++ b/chrome/browser/file_system_access/chrome_file_system_access_permission_context.h -@@ -242,6 +242,18 @@ class ChromeFileSystemAccessPermissionContext +@@ -242,6 +242,31 @@ class ChromeFileSystemAccessPermissionContext } }; @@ -721,6 +1177,19 @@ index 05ee182cf840bc925697a2bfa3d1c281579296c4..33595aaa73cf1301f1361bd07deb39ac + // chrome_file_system_access_blocklist.cc. + static std::unique_ptr GenerateBlockPaths( + bool should_normalize_file_path); ++ ++ // Returns true if access to `path` should be blocked by `extra_rules` or ++ // `block_path_rules`. Rules of type `BlockType::kBlockWrite` only apply when ++ // `access_trigger` is `AccessTrigger::kSave` or ++ // `AccessTrigger::kProgrammaticWrite`. May block, so must be called on a ++ // sequence that allows blocking. See chrome_file_system_access_blocklist.cc. ++ static bool ShouldBlockAccessToPath(bool should_normalize_file_path, ++ base::FilePath path, ++ HandleType handle_type, ++ AccessTrigger access_trigger, ++ std::vector extra_rules, ++ BlockPathRules block_path_rules, ++ base::FilePath profile_path); + explicit ChromeFileSystemAccessPermissionContext( content::BrowserContext* context, diff --git a/shell/browser/file_system_access/file_system_access_permission_context.cc b/shell/browser/file_system_access/file_system_access_permission_context.cc index 4c83f6eb76..c7d515754d 100644 --- a/shell/browser/file_system_access/file_system_access_permission_context.cc +++ b/shell/browser/file_system_access/file_system_access_permission_context.cc @@ -84,7 +84,6 @@ struct Converter< namespace { -using BlockType = ChromeFileSystemAccessPermissionContext::BlockType; using HandleType = content::FileSystemAccessPermissionContext::HandleType; using GrantType = electron::FileSystemAccessPermissionContext::GrantType; using SensitiveEntryResult = @@ -112,131 +111,6 @@ constexpr std::string_view kTimestampKey = "timestamp"; constexpr base::TimeDelta kPermissionRevocationTimeout = base::Seconds(5); -#if BUILDFLAG(IS_WIN) -[[nodiscard]] constexpr bool ContainsInvalidDNSCharacter( - base::FilePath::StringType hostname) { - return !std::ranges::all_of(hostname, [](base::FilePath::CharType c) { - return (c >= L'A' && c <= L'Z') || (c >= L'a' && c <= L'z') || - (c >= L'0' && c <= L'9') || (c == L'.') || (c == L'-'); - }); -} - -// Returns true if the path is a Universal Naming Convention (UNC) path pointing -// to a local system path, device namespace, or WSL loopback redirector. -bool MaybeIsLocalUNCPath(const base::FilePath& path) { - if (!path.IsNetwork()) { - return false; - } - - const std::vector components = - path.GetComponents(); - - // Check for server name that could represent a local system. We only - // check for a very short list, as it is impossible to cover all different - // variants on Windows. - if (components.size() >= 2 && - (base::FilePath::CompareEqualIgnoreCase(components[1], - FILE_PATH_LITERAL("localhost")) || - base::FilePath::CompareEqualIgnoreCase( - components[1], FILE_PATH_LITERAL("wsl.localhost")) || - base::FilePath::CompareEqualIgnoreCase( - components[1], FILE_PATH_LITERAL("wsl.localhost.")) || - components[1] == FILE_PATH_LITERAL("127.0.0.1") || - components[1] == FILE_PATH_LITERAL(".") || - components[1] == FILE_PATH_LITERAL("?") || - ContainsInvalidDNSCharacter(components[1]))) { - return true; - } - - // In case we missed the server name check above, we also check for shares - // ending with '$' as they represent pre-defined shares, including the local - // drives. - for (size_t i = 2; i < components.size(); ++i) { - if (components[i].back() == L'$') { - return true; - } - } - - return false; -} -#endif // BUILDFLAG(IS_WIN) - -bool ShouldBlockAccessToPath( - base::FilePath path, - HandleType handle_type, - std::vector - extra_rules, - ChromeFileSystemAccessPermissionContext::BlockPathRules block_path_rules) { - DCHECK(!path.empty()); - DCHECK(path.IsAbsolute()); - - path = ChromeFileSystemAccessPermissionContext::NormalizeFilePathForBlocklist( - path, /*should_normalize_file_path=*/true); - for (auto& rule : extra_rules) { - rule.path = - ChromeFileSystemAccessPermissionContext::NormalizeFilePathForBlocklist( - rule.path, /*should_normalize_file_path=*/true); - } - -#if BUILDFLAG(IS_WIN) - // On Windows, local UNC paths are rejected, as UNC path can be written in a - // way that can bypass the blocklist. - if (MaybeIsLocalUNCPath(path)) { - return true; - } -#endif - - base::FilePath nearest_ancestor; - BlockType nearest_ancestor_block_type = BlockType::kDontBlockChildren; - auto should_block_with_rule = [&](const base::FilePath& block_path, - BlockType block_type) -> bool { - if (path == block_path || path.IsParent(block_path)) { - VLOG(1) << "Blocking access to " << path << " because it is a parent of " - << block_path; - return true; - } - - if (block_path.IsParent(path) && - (nearest_ancestor.empty() || nearest_ancestor.IsParent(block_path))) { - nearest_ancestor = block_path; - nearest_ancestor_block_type = block_type; - } - return false; - }; - - for (const auto* block_rules_ptr : - {&extra_rules, &block_path_rules.block_path_rules_}) { - for (const auto& block : *block_rules_ptr) { - if (should_block_with_rule(block.path, block.type)) { - return true; - } - } - } - - // The path we're checking is not in a potentially blocked directory, or the - // nearest ancestor does not block access to its children. Grant access. - if (nearest_ancestor.empty() || - nearest_ancestor_block_type == BlockType::kDontBlockChildren) { - VLOG(1) << "Not blocking access to " << path << " because it is inside " - << nearest_ancestor << " and it's kDontBlockChildren"; - return false; - } - - // The path we're checking is a file, and the nearest ancestor only blocks - // access to directories. Grant access. - if (handle_type == HandleType::kFile && - nearest_ancestor_block_type == BlockType::kBlockNestedDirectories) { - VLOG(1) << "Not blocking access to " << path << " because it is inside " - << nearest_ancestor << " and it's kBlockNestedDirectories"; - return false; - } - - // The nearest ancestor blocks access to its children, so block access. - VLOG(1) << "Blocking access to " << path << " because it is inside " - << nearest_ancestor << " and it's kBlockAllChildren"; - return true; -} - std::string GenerateLastPickedDirectoryKey(const std::string& id) { return id.empty() ? kDefaultLastPickedDirectoryKey : base::StrCat({kCustomLastPickedDirectoryKey, "-", id}); @@ -722,27 +596,36 @@ void FileSystemAccessPermissionContext::ConfirmSensitiveEntryAccess( &FileSystemAccessPermissionContext::DidCheckPathAgainstBlocklist, GetWeakPtr(), request_id, origin, path_info, handle_type, access_trigger, frame_id); - CheckPathAgainstBlocklist(path_info, handle_type, + CheckPathAgainstBlocklist(path_info, handle_type, access_trigger, std::move(after_blocklist_check_callback)); } void FileSystemAccessPermissionContext::CheckShouldBlockAccessToPathAndReply( base::FilePath path, HandleType handle_type, + AccessTrigger access_trigger, std::vector extra_rules, base::OnceCallback callback, ChromeFileSystemAccessPermissionContext::BlockPathRules block_path_rules) { + // Uses Chrome's own matcher so that suffix rules, kBlockWrite rules (which + // only apply to AccessTrigger::kSave and AccessTrigger::kProgrammaticWrite) + // and the Windows local UNC path checks stay in sync with upstream. The + // profile path is only consulted for ChromeOS profile-based rules. base::ThreadPool::PostTaskAndReplyWithResult( FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_VISIBLE}, - base::BindOnce(&ShouldBlockAccessToPath, path, handle_type, extra_rules, - block_path_rules), + base::BindOnce( + &ChromeFileSystemAccessPermissionContext::ShouldBlockAccessToPath, + /*should_normalize_file_path=*/true, std::move(path), handle_type, + access_trigger, std::move(extra_rules), std::move(block_path_rules), + browser_context()->GetPath()), std::move(callback)); } void FileSystemAccessPermissionContext::CheckPathAgainstBlocklist( const content::PathInfo& path_info, HandleType handle_type, + AccessTrigger access_trigger, base::OnceCallback callback) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); // TODO(crbug.com/40101272): Figure out what external paths should be @@ -765,9 +648,9 @@ void FileSystemAccessPermissionContext::CheckPathAgainstBlocklist( if (is_block_path_rules_init_complete_) { // The rules initialization is completed, we can just post the task to a // anonymous blocking traits. - CheckShouldBlockAccessToPathAndReply(path_info.path, handle_type, - extra_rules, std::move(callback), - *block_path_rules_.get()); + CheckShouldBlockAccessToPathAndReply( + path_info.path, handle_type, access_trigger, extra_rules, + std::move(callback), *block_path_rules_.get()); return; } // The check must be performed after the rules initialization is done. @@ -775,7 +658,7 @@ void FileSystemAccessPermissionContext::CheckPathAgainstBlocklist( base::BindOnce(&FileSystemAccessPermissionContext:: CheckShouldBlockAccessToPathAndReply, weak_factory_.GetWeakPtr(), path_info.path, handle_type, - extra_rules, std::move(callback)))); + access_trigger, extra_rules, std::move(callback)))); } void FileSystemAccessPermissionContext::PerformAfterWriteChecks( diff --git a/shell/browser/file_system_access/file_system_access_permission_context.h b/shell/browser/file_system_access/file_system_access_permission_context.h index 1c2aa4cefa..6e01bfdb53 100644 --- a/shell/browser/file_system_access/file_system_access_permission_context.h +++ b/shell/browser/file_system_access/file_system_access_permission_context.h @@ -138,6 +138,7 @@ class FileSystemAccessPermissionContext void CheckShouldBlockAccessToPathAndReply( base::FilePath path, HandleType handle_type, + AccessTrigger access_trigger, std::vector extra_rules, base::OnceCallback callback, @@ -145,6 +146,7 @@ class FileSystemAccessPermissionContext void CheckPathAgainstBlocklist(const content::PathInfo& path, HandleType handle_type, + AccessTrigger access_trigger, base::OnceCallback callback); void DidCheckPathAgainstBlocklist(int request_id, const url::Origin& origin, diff --git a/spec/chromium.spec.ts b/spec/chromium.spec.ts index 2c70336f58..890cf8d633 100644 --- a/spec/chromium.spec.ts +++ b/spec/chromium.spec.ts @@ -2028,6 +2028,161 @@ describe('chromium features', () => { expect(restricted).toHaveLength(2); }); + it('blocks writes to .git/hooks, .git/config and .git/modules but still allows reading them', async () => { + const ses = session.fromPartition(`fsa-scope-${Math.random()}`); + ses.setPermissionRequestHandler((_wc, _permission, callback) => callback(true)); + const restricted: string[] = []; + ses.on('file-system-access-restricted', (_e, details, callback) => { + restricted.push(details.path); + callback('deny'); + }); + + // On Windows the temp dir lives under %LOCALAPPDATA%, which the blocklist + // blocks entirely, so use a directory the blocklist leaves alone there. + const base = process.platform === 'win32' ? path.join(fixturesPath, 'file-system') : os.tmpdir(); + const dir = fs.realpathSync(fs.mkdtempSync(path.join(base, 'fsa-git-'))); + defer(() => fs.rmSync(dir, { recursive: true, force: true })); + const hooks = path.join(dir, '.git', 'hooks'); + fs.mkdirSync(hooks, { recursive: true }); + fs.writeFileSync(path.join(hooks, 'pre-commit.sample'), 'sample'); + fs.writeFileSync(path.join(dir, '.git', 'config'), '[core]\n'); + + const w = new BrowserWindow({ show: true, webPreferences: { session: ses } }); + await w.loadURL(urlA); + const frame = w.webContents.mainFrame; + await pasteHandle(w, frame, dir); + await waitForHandle(frame); + expect(await frame.executeJavaScript('window.handle && window.handle.kind')).toBe('directory'); + expect(await frame.executeJavaScript("window.handle.requestPermission({ mode: 'readwrite' })", true)).toBe( + 'granted' + ); + + // Matches Chrome: these are kBlockWrite suffix rules, so reads are allowed + // and only operations that write are rejected. + const result = await frame.executeJavaScript(`(async () => { + const attempt = (p) => p.then(() => 'ok', (e) => e.name); + const git = await window.handle.getDirectoryHandle('.git'); + const hooks = await git.getDirectoryHandle('hooks'); + const sample = await hooks.getFileHandle('pre-commit.sample'); + const config = await git.getFileHandle('config'); + return { + readHook: await (await sample.getFile()).text(), + readConfig: await (await config.getFile()).text(), + createHook: await attempt(hooks.getFileHandle('post-checkout', { create: true })), + writeHook: await attempt(sample.createWritable()), + writeConfig: await attempt(config.createWritable()), + createModules: await attempt(git.getDirectoryHandle('modules', { create: true })), + createAllowed: await attempt(window.handle.getFileHandle('allowed.txt', { create: true })) + }; + })()`); + expect(result).toEqual({ + readHook: 'sample', + readConfig: '[core]\n', + createHook: 'SecurityError', + writeHook: 'SecurityError', + writeConfig: 'SecurityError', + createModules: 'SecurityError', + createAllowed: 'ok' + }); + expect(fs.existsSync(path.join(hooks, 'post-checkout'))).toBe(false); + expect(fs.existsSync(path.join(dir, '.git', 'modules'))).toBe(false); + expect(fs.readFileSync(path.join(dir, '.git', 'config'), 'utf8')).toBe('[core]\n'); + expect(fs.existsSync(path.join(dir, 'allowed.txt'))).toBe(true); + // Programmatic writes are rejected outright rather than asking the app. + expect(restricted).toEqual([]); + }); + + // A pasted directory only reaches the blocklist if it exists, so this + // shares a fixture directory over loopback SMB and reaches it under + // different UNC server and share names. Creating a share needs an + // elevated administrator. + ifit(process.platform === 'win32')("applies Chrome's local UNC path rules", { timeout: 120000 }, async (ctx) => { + const dir = fs.realpathSync(fs.mkdtempSync(path.join(fixturesPath, 'file-system', 'fsa-unc-'))); + defer(() => fs.rmSync(dir, { recursive: true, force: true })); + // At most 12 characters, including the hidden share's '$', or net share + // asks whether MS-DOS clients matter. + const share = `efsa${Math.floor(Math.random() * 0x1000000) + .toString(16) + .padStart(6, '0')}`; + const hiddenShare = `${share}$`; + try { + for (const name of [share, hiddenShare]) { + ChildProcess.execFileSync('net', ['share', `${name}=${dir}`], { stdio: 'pipe', input: 'y\r\n' }); + defer(() => { + try { + ChildProcess.execFileSync('net', ['share', name, '/delete', '/y'], { stdio: 'pipe' }); + } catch {} + }); + } + } catch (error) { + // On CI this must not turn into a silent skip. + if (process.env.CI) throw error; + console.warn(`Skipping UNC blocklist spec, could not create an SMB share: ${error}`); + return ctx.skip(); + } + + const host = os.hostname(); + const unc = (server: string, ...rest: string[]) => `\\\\${server}\\${rest.join('\\')}`; + const isReachable = (p: string) => + Promise.race([ + fs.promises.stat(p).then( + (s) => s.isDirectory(), + () => false + ), + setTimeout(20000, false, { ref: false }) + ]); + + const ses = session.fromPartition(`fsa-scope-${Math.random()}`); + const restricted: string[] = []; + ses.on('file-system-access-restricted', (_e, details, callback) => { + restricted.push(details.path); + callback('deny'); + }); + const w = new BrowserWindow({ show: true, webPreferences: { session: ses } }); + await w.loadURL(urlA); + const frame = w.webContents.mainFrame; + const classify = async (p: string) => { + // Also opens the SMB session up front, which can be slow. + expect(await isReachable(p), `${p} should be reachable`).toBe(true); + const before = restricted.length; + await frame.executeJavaScript('window.handle = null; true'); + await pasteHandle(w, frame, p); + await waitForHandle(frame); + const kind = await frame.executeJavaScript('window.handle && window.handle.kind'); + if (restricted.length === before) { + expect(kind, p).toBe('directory'); + return 'allowed'; + } + // File URLs lowercase the server name. + expect(restricted.slice(before).map((r) => r.toLowerCase())).toEqual([p.toLowerCase()]); + expect(kind, p).toBe(null); + return 'blocked'; + }; + + // A server name that isn't known to be local is treated as remote. + expect(await classify(unc(host, share))).toBe('allowed'); + // Intentional change: Electron used to block every share ending in '$'; + // like Chrome, only drive (X$) and ADMIN$/IPC$/PRINT$/FAX$ shares are. + expect(await classify(unc(host, hiddenShare))).toBe('allowed'); + // Drive admin shares are blocked whatever the server is called. + const [drive, ...dirComponents] = dir.split(path.sep); + expect(await classify(unc(host, `${drive.replace(/:$/, '')}$`, ...dirComponents))).toBe('blocked'); + // Local server names are blocked whatever the share is. A bare + // "localhost" can't be pasted: file URLs drop that host, so it would + // arrive as a path with no server. + expect(await classify(unc('127.0.0.1', share))).toBe('blocked'); + + // Intentional change: a trailing dot no longer hides a local server name. + // Electron used to allow this. It needs "localhost." to resolve, so it is + // skipped where it doesn't. + const trailingDot = unc('localhost.', share); + if (await isReachable(trailingDot)) { + expect(await classify(trailingDot)).toBe('blocked'); + } else { + console.warn(`Skipping ${trailingDot}, it does not resolve here`); + } + }); + it('revokes active grants once no top-level document of the origin remains', { timeout: 60000 }, async () => { const ses = session.fromPartition(`fsa-scope-${Math.random()}`); ses.setPermissionRequestHandler((_wc, _permission, callback) => callback(true));