fix: consider a Windows resource untrusted if security information could not be retrieved (#2128) #2147
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…uld 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>\\wsl$\<em>distro</em></code> or <code>\\wsl.localhost\<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]> Co-authored-by: copilot-swe-agent[bot] <[email protected]>
EliahKagan
approved these changes
Aug 31, 2025
There was a problem hiding this comment.
Per #2128 (comment), this looks fine to me. I've proposed a change to comments below that I think makes them clearer and more accurate to the condition under which this workaround applies.
It may be good to have a test that this workaround is not triggered on conditions that are decisively other errors, such as on a path where no file or directory exists at all. However, I don't think that needs to block this, and I can add it later.
(More extensive tests than that, such as those involving an actual 9P share, could also eventually be added but I'd personally hold off on doing those until next time I'm working on gix-sec, where I anticipate I'll be able to add them while augmenting the test suite in other ways.)
Co-authored-by: Eliah Kagan <[email protected]>
|
I think this doesn't require a re-amend, but I just noticed that a couple of the paths in my analysis (that you copied into the commit message) are not really readable in the commit message, because I had used explicit HTML for custom formatting internal to the code spans: Support for 9P shares was added to Windows to implement shares that access files in installed WSL distributions. These are shares named like <code>\\wsl$\<em>distro</em></code> or <code>\\wsl.localhost\<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).Specifically: These are shares named like <code>\\wsl$\<em>distro</em></code> or <code>\\wsl.localhost\<em>distro</em></code>, where *`distro`* isThis is probably fine and still adequately informative, and it may even render properly in automatically generated changelog entries, or it can be fixed up there if not. Therefore, I suggest ignoring this unless you already need to amend the commit between now and when the PR merges. That is, noticing this has not changed my judgment that the PR looks good to me. :) |
|
I also just realised that this working hinges on the error-check to be correct, and admittedly I am not sure as I didn't validate this at all.
|
It seems to be working. In the following, I've replaced some long output with |
|
Fantastic, thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2128 .
Related to gitbutlerapp/gitbutler#9913