uutils / uutils/coreutils

mv: cross-device leaf chowns by path, contradicting the comment above it (GNU uses fchown)

Open
#14,599 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

U - mv
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_ownership should take the already-open &File and use rustix::fs::fchown();
  • add O_EXCL to the mv destination 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.