fix: listing sandbox escaping symlinks - #6731
Conversation
|
I’m not sure For the sandbox symlink issue we fixed earlier, the bug was that follow-style operations could resolve through an “evil” symlink and escape the mount/preopen boundary. The intended model after that work was:
With this PR, There’s also a coverage gap here: the new host policy downcasts mount entries directly to Note that when I delivered symbolc links to Wasmer I have ran these tests: and from https://github.com/wasmerio/anti-regression-shield these specifically: I think would be great to run these tests with your changes. |
Arshia001
left a comment
There was a problem hiding this comment.
The change is correct in principle. However, we're adding a lot of #[cfg]ed code into an already huge file, and even doing downcasts that depend on exactly how the FS tree is set up right now AFAICT. This makes the implementation brittle. We should move the host-related logic into host FS. The WASIX FS doesn't really need to understand any of this.
Arshia001
left a comment
There was a problem hiding this comment.
Still not a fan of the implementation. Not every VirtualFS implementer needs to know what a "host" is.
Arshia001
left a comment
There was a problem hiding this comment.
Still architecturally unsound IMO. Filtering must be done at the FileSystem impl level, not at the WASIX level. This also avoids the need for the whole symlink_policy method on FileSystem.
|
|
||
| let metadata = fs::symlink_metadata(&path)?; | ||
|
|
||
| if !metadata.file_type().is_symlink() { |
There was a problem hiding this comment.
this is technically a wrong input, not a success. I'd use InvalidInput here.
| return Ok(SymlinkPolicy::Visible); | ||
| } | ||
|
|
||
| let target = match fs::read_link(&path) { |
There was a problem hiding this comment.
Can we unify the file_type().is_symlink() above and this read_link() into one call? I'm guessing read_link must report an error containing "this is not a symlink" if you call it on the wrong thing?
| @@ -0,0 +1,51 @@ | |||
| use std::path::{Component, Path, PathBuf}; | |||
|
|
|||
| pub(crate) fn resolve_path_within( | |||
There was a problem hiding this comment.
This probably belongs in the host_fs module, since that's the only module that does handling of host paths (i.e. std::path). Note that WASIX uses unix-style paths regardless of host OS, including on Windows.
| )) | ||
| } | ||
|
|
||
| pub(crate) fn readdir_entry_visible( |
There was a problem hiding this comment.
I don't see why we're reporting entries from the FS impls, and then filtering out at this layer. Each FS impl should be responsible for making sure they only report visible entries.
ad70b04 to
3094118
Compare
Summary
fd_readdirwas exposing symlink entries that should not have been visible inside the WASIX sandbox, including host-backed symlinks that could resolve outside the mounted or preopened directory tree.Changes
fd_readdirto filter directory entries through a symlink visibility check instead of listing all symlinks blindly.readdirbehavior and symlink traversal with new WASIX and Rust tests for visible vs hidden symlinks.