Skip to content

Commit 386c6a6

Browse files
committed
fix: consider a Windows resource untrusted if security information could not be retrieved (#2128)
Here is the full text of the analysis by Eliah Kagan: ---- Although I'm signed in to Discord, I'm not able to access the linked Discord chat. But from the task list in #2129, it looks like the problem underlying this issue is that, unlike files and directories accessed through SMB shares, Windows does not make security information available on files and directories accessed (on the Windows side) through 9P shares. Support for 9P shares was added to Windows to implement shares that access files in installed WSL distributions. These are shares named like <code>&bsol;&bsol;wsl$&bsol;<em>distro</em></code> or <code>&bsol;&bsol;wsl.localhost&bsol;<em>distro</em></code>, where *`distro`* is the distribution. Currently 9P is supported on Windows only for such WSL shares (though I don't know if being intended only for this WSL-related use is the reason security information isn't available through them). This is the case whether the 9P share is accessed through a `\\` path (e.g., `\\share\name`, `\\?\UNC\share\name`) or mapped to a drive letter. It is not specific to mapped drive letters, nor to any particular technique of mapping a share (or other directory) to a drive letter. So the claim that this depends on how the path is mounted is either accurate or inaccurate depending on exactly what is meant by it. Consider my Windows 10 system, on which the drive letters `Y:` and `Z:` (ignore `X:`) are currently mapped as: ```text C:\Users\ek> net use New connections will be remembered. Status Local Remote Network ------------------------------------------------------------------------------- Disconnected X: \\localhost\C$ Microsoft Windows Network OK Y: \\kip\ek Microsoft Windows Network Z: \\wsl$\Ubuntu Plan 9 Network Provider The command completed successfully. ``` In this experimental setup, subdirectories `\\kip\ek\temporary` and `\\wsl$\Ubuntu\home\ek\temporary` both exist, and they are both accessible. (These are separate directories on separate systems. They both have same name `temporary` in connection with more systematic testing I've been doing; I hope that's not too confusing.) Each is accessible through the `\\` paths or mounted drive letters. The SMB ("Microsoft Windows Network") share is accessible: ```text C:\Users\ek> Get-Item \\kip\ek\temporary Directory: \\kip\ek Mode LastWriteTime Length Name ---- ------------- ------ ---- d---- 6/28/2025 2:51 AM temporary C:\Users\ek> Get-Item Y:\temporary Directory: Y:\ Mode LastWriteTime Length Name ---- ------------- ------ ---- d---- 6/28/2025 2:51 AM temporary ``` And the 9P ("Plan 9 Network Provider") share is accessible: ```text C:\Users\ek> Get-Item \\wsl$\Ubuntu\home\ek\temporary Directory: \\wsl$\Ubuntu\home\ek Mode LastWriteTime Length Name ---- ------------- ------ ---- d---- 7/5/2025 8:28 PM temporary C:\Users\ek> Get-Item Z:\home\ek\temporary Directory: Z:\home\ek Mode LastWriteTime Length Name ---- ------------- ------ ---- d---- 7/5/2025 8:28 PM temporary ``` With the SMB share, security information is made available to Windows (this is the case even though `kip` happens to be a GNU/Linux system running a Samba server): ```text C:\Users\ek> Get-Acl \\kip\ek\temporary Directory: \\kip\ek Path Owner Access ---- ----- ------ temporary O:S-1-5-21-??????????-?????????-??????????-1000 Everyone Allow ReadAndExecute, Synchronize… C:\Users\ek> Get-Acl Y:\temporary Directory: Y:\ Path Owner Access ---- ----- ------ temporary O:S-1-5-21-??????????-?????????-??????????-1000 Everyone Allow ReadAndExecute, Synchronize… ``` (I've modified the output shown above by replacing most digits from the non-wellknown SID corresponding to the user `ek` on the machine `kip` with `?` characters. The output is otherwise unmodified.) In contrast, with the 9P share, security information is not available to Windows: ```text C:\Users\ek> Get-Acl \\wsl$\Ubuntu\home\ek\temporary Get-Acl: Method failed with unexpected error code 1. C:\Users\ek> Get-Acl Z:\home\ek\temporary Get-Acl: Method failed with unexpected error code 1. ``` To further confirm the cause, note that when the item doesn't exist, `Get-Acl` reports that: ```text C:\Users\ek> Get-Acl \\wsl$\Ubuntu\home\ek\nonexistent Get-Acl: Cannot find path '\\wsl$\Ubuntu\home\ek\nonexistent' because it does not exist. C:\Users\ek> Get-Acl Z:\home\ek\nonexistent Get-Acl: Cannot find path 'Z:\home\ek\nonexistent' because it does not exist. ``` Thus, it's not that somehow `Get-Item` is able to access the directory but `Get-Acl` is not, but rather than `Get-Acl` accesses the directory but is not able to obtain its security information. Based on the above, I think the approach in #2129--of treating the error that occurs when security information is unavailable as equivalent to finding that the location should not be trusted, rather than as an error--is reasonable. Once CI is allowed to run in full on #2129, I expect it to work (either already or with minor refinements). However, also in view of the above, when the feature branch in #2129 is rebased to add a conventional prefix `fix:` to a7d35d4, I recommend that the commit message also be changed. The current message claims that this is fixing the trust check "for mounted paths," which I would consider inaccurate: it's fixing it for paths where Windows cannot access security information. (Since Copilot was used to open that PR, I think Copilot will actually perform the relevant rebase if asked to do so.) This situation with 9P shares is also responsible for most of what has been reported in #2067. This issue is *almost* a duplicate of #2067, but not quite. That issue claims the trust check performed by `is_path_owned_by_current_user` doesn't work for: > UNC paths - repos located in WSL, or on network drives, or >255 character long paths That is accurate for repos located in WSL (provided that is taken to mean repos accessed on the Windows side through 9P shares). It is otherwise mostly inaccurate: - UNC paths of all styles work fine, network drives in general work fine, and long paths work fine in the `\\?\` (including `\\?\UNC\host\share\`) and `\??\` styles, which are the styles that are supposed to support long paths. (These work regardless of whether greater than usual long path support is enabled in the registry and the executable's application manifest.) - Whether long paths also work in styles whose semantics do not entail long path support--e.g., paths starting like `C:\` or `\\host\share`--is according to the usual behavior of Windows applications. If such support is enabled in the Windows registry and the executable attempting to access them declares compatibility with such support in its app manifest, then they work. Otherwise, they do not. However, it seems to me that it nonetheless *is* a bug that `is_path_owned_by_current_user` doesn't work when passed a long path of a style other than those that are supposed to support long paths. The reason is that, while these do not usually work in Windows applications, they *do* usually work in Windows applications implemented in Rust that use the Rust standard library. This is because functions in `std` automatically turn long paths of forms other than `\\?\` and `\??\` into `\\?\` paths before passing them to Windows API functions that could otherwise fail due to the path being unexpectedly long. Consequently, `is_path_owned_by_current_user` is one of the few--maybe the only--functions in any `gix-*` crate that doesn't support long paths in all styles regardless of app manifest or registry settings. Since this is effectively part of what #2067 reports, I suggest keeping that open even if merging #2129 fixes the bug here. If no one else gets to fixing that long-paths aspect of #2067, I expect that I will eventually, though there are some other improvements in `gix-sec` that I would want to finish working on first. (#2067 also seems like it is describing that allowlisting paths--or some styles of paths?--in `safe.directory` is not honored. Maybe this falls under #1912, but I'm not sure. I haven't been researching that aspect of it, only the aspects that pertain to `is_path_owned_by_current_user` behavior. I think this will become clear once the information you requested in #2067 (comment) has been provided.) A separate question is whether it would be acceptable to return `Ok(true)` rather than `Ok(false)` when accessing a 9P share for a WSL distribution that is itself owned by the current user. I am unsure, both whether that would be reasonably safe and whether there is an acceptably simple way to achieve that safely. I would not rush to attempt that. My research on #2067, including experiments that support most of what I've said above--and that verify each of the cases with various path styles, calling both `std` functions and `is_path_owned_by_current_user`--can be seen in [this gist of currently incomplete notes](https://gist.github.com/EliahKagan/fc8319485af33edbde38db6e7438f2d3). The claims I haven't, as yet, shown experimental evidence for in that gist are about a long-path-aware configuration, i.e., experiments about the effects of app manifests and registry settings. I've carried out those long-path-aware experiments too, but I've not made a convenient way to verify them, nor written them up. (The experiments shown there were with `evcxr`. The long-path-aware experiments were with `evcxr` modified with an app manifest as in EliahKagan/evcxr@d4f90e3.) ---- Co-authored-by: Eliah Kagan <[email protected]
1 parent 8d8dba2 commit 386c6a6

File tree

1 file changed

+6
-1
lines changed

1 file changed

+6
-1
lines changed

gix-sec/src/identity.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ mod impl_ {
8282

8383
pub fn is_path_owned_by_current_user(path: &Path) -> io::Result<bool> {
8484
use windows_sys::Win32::{
85-
Foundation::{GetLastError, LocalFree, ERROR_INSUFFICIENT_BUFFER, ERROR_SUCCESS},
85+
Foundation::{GetLastError, LocalFree, ERROR_INSUFFICIENT_BUFFER, ERROR_INVALID_FUNCTION, ERROR_SUCCESS},
8686
Security::{
8787
Authorization::{GetNamedSecurityInfoW, SE_FILE_OBJECT},
8888
CheckTokenMembership, EqualSid, GetTokenInformation, IsWellKnownSid, TokenOwner,
@@ -123,6 +123,11 @@ mod impl_ {
123123
);
124124

125125
if result != ERROR_SUCCESS {
126+
// We cannot determine ownership, so we default to reduced trust (false) rather
127+
// than failing completely.
128+
if result == ERROR_INVALID_FUNCTION {
129+
return Ok(false);
130+
}
126131
let inner = io::Error::from_raw_os_error(result as _);
127132
error!(
128133
inner,

0 commit comments

Comments
 (0)