cp: address the remaining toctou issues#12135
Open
sylvestre wants to merge 4 commits intouutils:mainfrom
Open
Conversation
|
GNU testsuite comparison: |
sylvestre
force-pushed
the
toctou-cp
branch
2 times, most recently
from
1f0cb4d to
6ea296f
Compare
) cp previously created the destination with mode 0o666 masked by umask (typically 0o644), then later applied the final permissions via set_permissions. In a shared directory like /tmp this opened an observable window where another user could open the destination with the intermediate broad mode before cp narrowed it, leaking file contents that were intended to stay private. Create dest with 0o600 initially in every non-symlink code path — clone, sparse_copy, sparse_copy_without_hole, fs_copy, the stream path, and the non-Linux fs::copy fallback. The existing set_permissions call in copy_file applies the real final mode after the content is written, so user-visible end state is unchanged; only the intermediate mode is tightened. Matches GNU cp. Extend `util/check-safe-traversal.sh` with a cp strace check that asserts the destination openat carries mode 0600 so a future change that reintroduces 0666 fails the smoke test.
sylvestre
force-pushed
the
toctou-cp
branch
2 times, most recently
from
2cc328b to
37e3636
Compare
…ls#10017) In `-P` / no-dereference mode, cp now opens the source file with `O_NOFOLLOW`, matching GNU cp. This closes a TOCTOU window where an attacker who can swap the source path between cp's `lstat` check and the subsequent open could redirect the read through a symlink to a sensitive file (e.g. /etc/shadow). With `O_NOFOLLOW` the open fails with `ELOOP` instead. The same flag is propagated to `safe_copy::create_dest_restrictive`, so the destination open also refuses to follow a symlink in no-dereference mode. Without that, an attacker who plants the dest path as a symlink between the caller's check and the open could redirect the truncate (and the subsequent write) to any file the caller has permission to write — the symmetric attack to the source side. With `nofollow=true` the dest open returns `ELOOP` and the victim file is left untouched. `copy_on_write` gains a `nofollow` parameter threaded from `copy_helper`, set to `!options.dereference(source_in_command_line)`. In deref mode the flag is false and behavior is unchanged — cp still follows symlinks, matching GNU. Extends `util/check-safe-traversal.sh` with a cp -P strace check so the invariant is locked in: future changes that drop `O_NOFOLLOW` here will fail the smoke test.
GNU `cp -p` preserves mode, ownership, and timestamps. xattrs are
NOT preserved unless the user asks for them via `--preserve=xattr`
or `-a`. uutils's `Attributes::DEFAULT` had xattr set to
`Preserve::Yes { required: true }`, which (1) diverges from GNU and
breaks scripts that expect the stock behavior, (2) leaks security
xattrs like file capabilities and SELinux labels into copies when
run as root, and (3) fails hard on destinations that don't support
xattrs.
Remove the xattr override in `Attributes::DEFAULT` so it inherits
`Preserve::No` from `Attributes::NONE`. `Attributes::ALL` (used by
`-a` and `--preserve=all`) still sets xattr to Yes, and
`--preserve=xattr` still works as before.
When `cp -p` cannot chown the destination to the source's owner (e.g. a non-root user copying a root-owned setuid file), GNU cp strips the setuid and setgid bits from the applied mode so the destination does not give the copying user elevated privileges via the copy. uutils was unconditionally applying the source mode, producing user-owned files with a live setuid bit. Track `ownership_preserved` alongside the existing chown retry logic and, in the subsequent `handle_preserve(mode, ...)` block, mask off `0o6000` from the source's mode when ownership could not be preserved. The sticky bit (01000) is kept, matching GNU.
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
1 participant
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.