mv: cross-device leaf chowns by path, contradicting the comment above it (GNU uses fchown)
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 24.1k
- Forks
- 2k
- Avg merge
- 1d 5h
- Merged PRs (30d)
- 365
Description
src/uu/mv/src/mv.rs:1466 documents a guarantee the code does not fully provide:
// Open src/dst with O_NOFOLLOW and keep the fds alive across copy,
// chown, xattr, and chmod so a concurrent path-swap can't redirect any
// step to a different inode.
Copy, xattr and chmod do honour it. chown does not. preserve_ownership (mv.rs:1527) takes two &Path, re-symlink_metadata()s them, and calls wrap_chown(to, …) (:1544) — a path-based lchown, not an operation on the dst_file fd that is open and in scope right there.
Line references are against main at be00b4c4e.
Trace
Cross-device move as root, source owned by uid 1000:
uutils: open("…/dst/k", O_WRONLY|O_CREAT|O_TRUNC|O_NOFOLLOW|O_CLOEXEC, 0600) = 4
lchown("…/dst/k", 1000, 1000) <- path-based
fchmod(4, 0664) <- fd-based
GNU: openat(AT_FDCWD, "…/dst/k2", O_WRONLY|O_CREAT|O_EXCL, 0600) = 4
fchown(4, 1000, 1000) <- fd-based
fchmod(4, 0100664)
Control: GNU coreutils 9.11.130 (master).
Two divergences: GNU chowns the descriptor, and GNU's create carries O_EXCL where create_dest_restrictive(to, nofollow = true) (mv.rs:1481) does not.
Suggested fix
preserve_ownershipshould take the already-open&Fileand userustix::fs::fchown();- add
O_EXCLto themvdestination create; - correct the comment at
mv.rs:1466— as written it asserts a guarantee that does not hold, which is the part most likely to stop the next reviewer from looking.
Reported by @RelunSec (InsiteTech.jp). Triaged as a correctness/robustness bug: no primitive was found that reaches a file the attacker does not already control (lchown does not follow symlinks; fs.protected_hardlinks=1 blocks the hard-link route), but the divergence from GNU is real and the false comment should go.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in src/uu/mv/src/mv.rs at the comment around line 1466, then trace preserve_ownership around lines 1527-1544 and create_dest_restrictive around line 1481. Check the existing cross-device move flow and filesystem APIs before changing ownership and destination creation. Done means chown uses the open destination file, creation includes O_EXCL, and the comment no longer promises more than the code guarantees.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- cli, operating-systems
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100