fix(hardening): Look for TOCTOU issues in the code base - #2420
Open
hunger wants to merge 5 commits into
Open
Conversation
Collaborator
Author
|
Looks like I need to apply more windows-love :-) |
pavelzw
reviewed
May 11, 2026
pavelzw
left a comment
pavelzw
left a comment
Member
There was a problem hiding this comment.
haven't looked at everything so far. only a small review from my phone
Comment on lines
-194
to
+199
| let mut writer = HashingWriter::<_, Sha256>::new(File::create(path)?); | ||
| writer.write_all(bytes)?; | ||
| let (_, hash) = writer.finalize(); | ||
| atomic_write(path, bytes, mode)?; | ||
| let hash = compute_bytes_digest::<Sha256>(bytes); |
Member
There was a problem hiding this comment.
Won't this result in performance degradation?
Collaborator
Author
There was a problem hiding this comment.
Am I missing a trick here? I thought it would be the same in this case where we write all data in one go anyway.
baszalmstra
marked this pull request as draft
May 12, 2026 11:46
added 5 commits
May 13, 2026 14:24
A small workspace crate of symlink-resistant filesystem helpers, built on cap-std. The shared theme is that every operation routes through a `cap_std::fs::Dir` capability opened on a trusted parent, so path-component traversal can never escape that parent and the only remaining attack surface — the final component — is handled with explicit symlink rejection. Three helpers, covering the common shapes that came out of the TOCTOU audit: * `open_no_follow(parent, name, opts)` — opens or creates `name` inside `parent`, returning `PermissionDenied` if `name` is a symlink. For `.lock` files and similar short-lived metadata files in shared cache roots, where a co-tenant could otherwise redirect the open via a same-directory symlink swap. * `atomic_write_in_dir(parent, name, bytes, mode)` — writes via `tempfile::NamedTempFile` in the same parent, applies `mode` (Unix only) via `fchmod` on the still-open temp- file fd, then renames over `name`. The final path never exists with the wrong permissions, the chmod can't race a path-symlink swap, and a concurrent reader never sees a half-written file. * `validate_relative_inside(root, candidate)` — lexical normaliser that refuses absolute paths, `..` escapes, and empty/curdir-only inputs. For pre-validating attacker- influenced relative paths (archive `paths.json::relative_path`, menuinst manifest dest, …) before they're handed to a higher-trust opener. On Windows the path-based primitives leave small TOCTOU windows reachable by a co-tenant with write access to the parent of the cache root, so a `windows_sec` module replaces them with fd-anchored NT-level equivalents: * `create_owner_only_subdir(parent, name)` and `create_owner_only_file(parent, name)` — both call `NtCreateFile` with `OBJECT_ATTRIBUTES.RootDirectory = parent.HANDLE`, `FILE_CREATE`, and an explicit `PROTECTED` owner-only `SECURITY_DESCRIPTOR` carrying a single allow-ACE for the current user's SID. Placement is fd-anchored and the DACL is applied atomically at create — no inherit-then-harden window. * `rename_via_handle(src, dest_dir, dest_name)` — the publish step. Uses `SetFileInformationByHandle(FileRenameInfoEx)` with `RootDirectory = dest_dir.HANDLE` and POSIX semantics. The destination is identified entirely by the caller's handle plus a single-component name; no path resolution. * `verify_owner(dir)` — defense-in-depth check that the kernel object's owner SID matches the process token's user SID. Catches filesystems that ignore security descriptors (FAT) and any future ownership-semantics regression. * `is_symlink_refusal` matches `ERROR_STOPPED_ON_SYMLINK` and `ERROR_CANT_ACCESS_FILE`, so `open_no_follow_in` returns `PermissionDenied` on Windows the same way it does on Unix. `TempDir::new_in` dispatches to `create_owner_only_subdir` on Windows; `atomic_write_in` opens the payload via `create_owner_only_file` and publishes via `rename_via_handle`. cap-std stays the type vocabulary (`Dir`, `File`) — handles returned by NtCreateFile are wrapped via `Dir::from_std_file`. A Windows-only `windows-sys` dependency adds eight features covering the Wdk and Win32 namespaces used. cap-std covers every rattler CI target — Linux (incl. musl/ aarch64/arm/riscv64/ppc64), macOS x86_64+aarch64, Windows x86_64+aarch64. The helpers re-export `cap_std::fs::Dir`, `File`, `OpenOptions`, `Permissions`, etc. so callers don't take a direct cap-std dep. 13 unit tests cover symlink refusal, atomic-replace semantics, mode application via `fchmod`, and the normaliser's edge cases (absolute, escaping `..`, empty, curdir-only, balanced `..`). Three Windows-only tests verify the tempdir is owner-owned and `SE_DACL_PROTECTED`, the published file inherits the same properties, and the reparse-point error codes map to `PermissionDenied`. This commit only adds the crate; the drop-in callers that exercise these helpers land in the follow-up commit.
Open `<prefix>/.guard` via `rattler_fs_safety::open_no_follow` instead of plain `OpenOptions::new().open(&guard_path)`. The new opener resolves the file through a `cap_std::fs::Dir` capability rooted at the prefix directory and refuses to follow a symlink at the final component, so a co-tenant on a shared install root who plants `.guard` as a symlink to e.g. `~/.bashrc` can no longer steer the locking opener at that target. The guard struct now stores the prefix directory rather than the assembled `.guard` path, and `AsyncPrefixGuard::new` ensures the prefix exists rather than the guard's parent (same effect — the prefix *is* the guard's parent — but keeps the path-construction local to `write`).
`LockFile::to_path` now serializes to bytes and routes the write through `rattler_fs_safety::atomic_write_in_dir`, which opens a `tempfile::NamedTempFile` in the same parent directory, fsyncs it, and renames over the final path. Two benefits over the previous `File::create` + `to_writer`: * A concurrent reader never sees a torn YAML document — the lockfile either has the previous version or the new one, not a half-written mix. * The final-path open isn't redirected by a final-component symlink swap, since the rename target is resolved through `tempfile::persist` and the kernel fails the rename rather than overwriting the symlink target.
…_path A package shipping `paths.json::relative_path = "../../etc/cron.d/x"` could previously have `link_file` hardlink (or write through) into the user's `cron.d`, since `relative_path` is joined to the package directory without lexical checking and `destination_relative_path` is similarly attacker-controllable through the clobber registry's renaming. `link_file` now lexically validates both paths via `rattler_fs_safety::validate_relative_inside` before any filesystem operation, returning a new `LinkFileError::RelativePathEscapesPrefix` variant. The check runs on both the source side (against the package directory) and the destination side (against the target prefix); each rejects `..`-escapes, absolute paths, and prefix components ahead of any `File::open` / `hard_link` / `reflink_copy` call, so a malicious channel can't even probe the host filesystem through error-shape side channels. A new `test_link_file_rejects_relative_path_escape` test pins both directions of the rejection.
…try-point scripts `write_and_hash` no longer truncates the final path with `File::create` and then chmods it via path. Both moves are now done atomically: * The bytes go to a `tempfile::NamedTempFile` in the same parent directory via `rattler_fs_safety::atomic_write_in_dir`, which fsyncs and renames over the final path. A concurrent reader never sees a half-written script. * On Unix, the `0o775` mode is applied via `fchmod` on the still-open temp-file fd before the rename, so the script never exists at the final path with the wrong permissions and the chmod can't race a path-symlink swap. The previous `std::fs::set_permissions(script_path, ...)` call left a small window where a co-tenant could redirect the chmod via a symlink at `script_path`. The Windows `.exe` launcher write (an unconditional `std::fs::write`) is now atomic too, closing the same torn-write window for that path. `write_and_hash` no longer constructs a `HashingWriter`; the digest is computed from the in-memory bytes via `compute_bytes_digest::<Sha256>` after the atomic write, which is equivalent for a single `write_all` call and avoids keeping the file fd alive past the rename.
hunger
marked this pull request as ready for review
May 20, 2026 09:08
Collaborator
Author
|
Most of this has been covered upstream and I need to merge the two somehow. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
I want to get to a point where I can share package caches between users. For that the backend code needs to be more hardened to all kinds of mischief people can come up with.
Here is a first round of TOCTOU and similar issues in Claude found in rattler, trying to close the window where an attacker could potentially sneak in a symlink into unexpected places or get a file descriptor to something that will then get more restrictive permissions applied.
The biggest addition is a rattler_fs_safety crate that tries to have some common and safe filesystem helpers.
The rest is pretty small and and self-contained changes, often using rattler_safe_fs.
How Has This Been Tested?
Tests
AI Disclosure
Tools: Claude
Checklist: