Conversation
traverse() classified the operand from pathname metadata -- is it a directory, does --from match, is it '/' -- and then re-resolved the same pathname twice: once with DirFd::open(path, Follow) for the fchown, and again in safe_dive_into() for the descent. Neither open was checked against the metadata the decisions were made on. An attacker able to rename inside the operand's parent could therefore swap the classified directory for a symlink after the checks, and every fd-anchored mutation below ran inside the symlink target: fchown on the root, chown_at/chmod_at on the descendants. --from and --preserve-root were evaluated against the decoy and never re-checked, so the filter was bypassed rather than merely the traversal. Open the operand once, fstat the descriptor and compare (dev, ino) against that metadata, and fail closed if they differ. The descent reuses that same descriptor, so the pathname is resolved exactly once, and whether to descend at all is now decided from the metadata rather than from fresh is_symlink()/is_dir() lookups. b286558 fixed the descent open (open_subdir with the caller's symlink policy); this is the same fail-closed pattern one level up, at the acquisition of the operand itself. Measured with a rename-exchange harness racing chgrp -R against a symlink to an off-tree directory: before, the off-tree victim was regrouped within 1-4 invocations on every run; after, 1500 invocations left it untouched. GNU chgrp resists the same harness. The static flag matrix (-P/-H/-L, symlink and directory operands, --from, --preserve-root, missing operand) is byte-identical before and after.
walk_dir_with_context checked is_symlink() and is_dir() on the operand pathname and then opened that pathname again with a hardcoded SymlinkBehavior::Follow. An attacker able to rename inside the operand's parent could swap the directory just checked for a symlink in between, and the whole descent ran inside the symlink target -- under -P, which asks for no symlink to be traversed at all. Open with should_follow_symlink instead, the policy the checks were made under: O_NOFOLLOW makes the open fail closed when the operand became a symlink, while -H/-L, where following the operand is what was asked for, are unchanged. This mirrors what the descent already does for entries met inside the tree. Measured with a rename-exchange harness racing chmod -R -P against a symlink to an off-tree directory: before, an off-tree file was re-moded within 1-8 invocations on every run; after, 1500 invocations left it untouched. GNU chmod resists the same harness under -P. Under -H and -L both GNU and uutils follow, as intended. Still path-based, and not addressed here: the operand's own chmod_file call resolves the pathname a third time, so a swap landing in that narrower window re-modes the symlink target itself (not its contents). Closing it needs the operand's mode change to go through the descriptor.
The pre-existing "cannot access" messages in the recursive walk were still hardcoded English. Route them through translate!() with the perms-cannot-access and perms-too-many-symlink-levels keys.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate issues remain in recursive operand binding, symlink traversal, and regression coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens recursive permission and ownership operations against pathname replacement, updates localized diagnostics, and aligns chmod traversal with symlink policies.
Changes:
- Verifies descriptors for recursive
chown/chgrp. - Updates shared permission diagnostics.
- Adjusts
chmodrecursive root handling.
File summaries
| File | Summary | Final findings |
|---|---|---|
src/uucore/src/lib/features/perms.rs |
Descriptor verification and recursive ownership traversal. | Critical (1 vote): recursive file operands remain path-based. Moderate (2 votes): root traversal mishandles -H/-L --no-dereference. Moderate (2 votes): replacement test does not exercise same-path recursive traversal. |
src/uucore/locales/en-US.ftl |
Shared recursive permission diagnostics. | No final review comments. |
src/uu/chmod/src/chmod.rs |
Symlink-policy-aware recursive directory opening. | Critical (3 votes): opened descriptor is not verified against checked metadata. Nit (1 vote): comment misstates default symlink behavior. |
Review details
Suppressed comments (1)
src/uu/chmod/src/chmod.rs:655
- The default passed to
configure_symlink_and_recursionfor chmod isTraverseSymlinks::First(chmod.rs:188-189), and the existing tests label bare-Ras-H;-Pis not the recursive default. This comment therefore misstates the symlink behavior being explained and could lead future changes to applyNoFollowto default root traversal.
// above used: the pathname is resolved again here, so under `-P` (the `-R`
// default) O_NOFOLLOW fails the open rather than redirecting the descent into a
// swapped-in symlink. `-H`/`-L` still follow, which is what they ask for.
- Files reviewed: 3/3 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // swapped-in symlink. `-H`/`-L` still follow, which is what they ask for. | ||
| if (!file_path.is_symlink() || should_follow_symlink) && file_path.is_dir() { | ||
| match DirFd::open(file_path, SymlinkBehavior::Follow) { | ||
| match DirFd::open(file_path, should_follow_symlink.into()) { |
Comment on lines
+323
to
+324
| let operand_fd = if meta.is_dir() { | ||
| match DirFd::open(path, SymlinkBehavior::Follow) { |
Comment on lines
+489
to
491
| if !meta.is_dir() { | ||
| // No children to visit, matching WalkDir's min_depth(1). | ||
| return 0; |
Comment on lines
+1111
to
+1114
| // Same pathname, different object underneath: the descriptor no longer matches | ||
| // the metadata every decision was made on. | ||
| let other_fd = DirFd::open(&other, SymlinkBehavior::Follow).unwrap(); | ||
| assert!(!fd_is(&other_fd, &meta).unwrap()); |
Merging this PR will improve performance by 13.18%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | true_consecutive_calls |
350.2 ns | 294.7 ns | +18.85% |
| ⚡ | Simulation | hostname_ip_lookup[100000] |
178 µs | 165.2 µs | +7.78% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing sylvestre:chown-verify-operand-fd (a891e99) with main (da3bb21)
Footnotes
-
50 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
|
GNU testsuite comparison: |
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
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.
No description provided.