fix(metadata): apply the set-group-ID mode chmod(2) would apply - #7655
Merged
Conversation
A daemon pull of `rsync://host/module/cd/`, where `cd` is a symlink out of the module, enumerated the outside directory into the file list. Measured on the upstream 3.5.0 cell `sender-flist-symlink-leak`: the marker file outside the module appears in the listing, while the cell's own in-module positive control also passes, so the listing machinery is live and the leak is real. The confined enumeration was already routed. `walk.rs` calls `fast_io::pinned_root::read_dir`, which resolves its anchor through `confinement::pinned_root_relative` - the process-global session root. Nothing installs that root for a daemon, so `pinned_root_relative` returned `None`, which is documented to mean "resolve the path the ordinary way", and every directory read silently degraded to an unconfined absolute-path read. Meanwhile `GeneratorContext::confine_root()` was already correct for a daemon (`is_daemon_connection => daemon_module_root`, mirroring upstream's `am_daemon ? module_dir : confine_root`, syscall.c:136) and was consulted only by the CONTENT open. That split is exactly the observed behaviour: the content transfer refuses the escape while the enumeration follows it. The scan now takes the root as a PARAMETER rather than reading ambient state. `read_dir_under(root, path)` performs the same confined walk `read_dir` does for the pinned case, and `GeneratorContext::scan_source_dir` is the single owner of the anchoring decision for both scan sites, so they cannot disagree. Installing the global per connection was rejected deliberately: it is a `static RwLock` and oc serves each daemon connection on a worker thread of one process, so two concurrent connections on different modules would overwrite each other's boundary. Upstream's equivalent global is safe only because upstream forks a child per connection. Passing the root in is what makes the anchor correct under threads. Not-anchorable is not an escape. `strip_prefix` is lexical, so a relative operand - which a restricted shell produces routinely - or a differently spelled absolute path falls back to the ordinary read, matching what `pinned_root_relative` already decides for the ambient pin. Refusing there instead was measured to break `rrsync-pull-arg-shapes` and `rrsync-merge-file-confine` with `opendir "sub" failed: Cross-device link`, directories that were never outside anything. Measured on macOS against a freshly built release binary, whole suite: before 8 fail / 232 pass after 6 fail / 234 pass `sender-flist-symlink-leak` and `daemon-scan-dir-escape` both move fail -> pass; the remaining six are unchanged and all recorded `fail` in the manifest. Both macOS manifest rows are flipped in this commit because the defect is FIXED, not to silence a red - leaving them would report an XPASS. The two Linux manifests already record both cells as `pass` and are untouched. upstream: rsync-3.5.0/flist.c:2028-2059 `secure_opendir()` - the confined open that produces `scan_dirfd` for a daemon's scan. upstream: rsync-3.5.0/syscall.c:136 `confinement_root()` - `am_daemon ? module_dir : confine_root`, the value the new parameter carries.
…a list clippy::doc_lazy_continuation rejected the scan_source_dir doc: a wrapped prose line began with `- was consulted only by ...`, which markdown reads as a list bullet, making the three following lines lazy continuations of a list item. The hyphen-as-aside style is fine mid-line; the defect is purely where the wrap put it. Reworded to parentheses so no line begins with a hyphen unless it is a genuine list item - every other `/// - ` in this file is one. ⚠ This was invisible to a local workspace clippy run: crates/fast_io fails clippy on this host under rust 1.94.0 (7 errors in copy_file_range / sendfile / iocp_stub / confinement, none of them in this change), and a crate that does not compile under clippy means every DEPENDENT crate is never linted at all. "Zero hits in my files" from such a run is vacuous. Verified here by linting transfer with only those inherited lints allowed.
Three sites claimed the daemon's operand resolvers CONFINE, on the strength of collapsing `.` and `..`: "cannot enumerate outside the module root" "the result cannot escape the module root, so there is nothing left to refuse" "the result is confined to the module root by construction" The collapse is a STRING operation. It contains exactly the shapes it folds - a traversing spelling and a host-absolute one - and says nothing about where the path RESOLVES, because a symlink is resolved by the kernel at syscall time and no string operation can see it. Upstream does not rely on the collapse alone either: it enters the module with change_dir(module_chdir, CD_NORMAL) and scans through the confined secure_opendir(). That over-claim is why an out-of-module directory reached by a module-root symlink was enumerated into the file list: a reader had been told this layer already confined, so the downstream scan was left anchored on ambient state. Each site now says what the collapse does prove, and names where resolved containment is actually enforced - the confined source open and the anchored directory scan. Four citations in the same blocks were verified against the pinned 3.5.0 tree and retargeted; two were verified correct and left alone. util1.c:804 -> :881 glob_expand_module() clientserver.c:992 -> :1059 change_dir(module_chdir, CD_NORMAL) flist.c:2338-2349 -> :2610-2621 send_file_list() dir/fn split main.c:1203-1204 -> io.c:1497 the glob_expand_module call site util1.c:1183 unchanged sanitize_path's `..` handling flist.c:2589-2594 unchanged DOTDIR_NAME
Upstream reaches every mode change through do_chmod() -> chmod(2)
(syscall.c:761). oc anchors the same change on a walked parent dirfd for
symlink-race safety, so it issues fchmodat(2) instead - and on macOS the
two syscalls disagree about S_ISGID for an unprivileged caller.
Measured on macOS 26.5, euid 501, caller owns the target, same inode and
same mode word for both calls:
requested chmod(2) fchmodat(2)
0755 - ok
0755 | S_ISUID ok -> 04755 ok -> 04755
0755 | S_ISGID ok -> 00755 EPERM
0755 | both ok -> 04755 EPERM
The kernel silently MASKS a set-group-ID bit it will not grant when the
request arrives through chmod, but REFUSES the identical request through
fchmodat. Linux masks on both, so this is not observable there.
Left unhandled that turned `--chmod=a+s` into a fatal EPERM (exit 23) for
an ordinary user where upstream completes with exit 0. Measured against
the real rsync 3.5.0 binary on the same fixture: upstream exits 0 and
leaves drwsr-xr-x / -rwSr--r--; oc exited 23 and applied nothing.
The refusal is now retried once with S_ISGID cleared - exactly the mode
the platform's own chmod would have applied - so the anchor is kept and
the resulting mode is byte-identical to upstream (verified: 4/4 entries
match after the fix). The retry cannot mask a genuine permission error: a
caller that does not own the target is refused for the setgid-free mode
too, and that error propagates.
macos-setgid-ordinary-mode-regression moves fail -> pass on the macOS
upstream-3.5.0 leg (234/6 -> 235/5); its expect row is flipped in the
same commit because the defect is fixed, not re-baselined.
chmod-setid still fails on macOS and its `fail` row is deliberately
UNCHANGED: that cell asserts both setuid and setgid are set, and the
platform will not grant setgid to this caller at all - the real rsync
3.5.0 binary lands on the same setuid-only mode (-rwSr--r--) on this
host, so the assertion is unsatisfiable here rather than an oc defect.
`u32::from(libc::S_ISGID)` is a real widening on macOS, where `S_ISGID` is `u16`, but a no-op on Linux, where it is already `u32` - so Linux clippy rejects it as `useless_conversion`. An `as` cast fails the other way round, tripping `unnecessary_cast` on Linux. Drop the conversion entirely and name the POSIX-fixed literal, matching the existing idiom in crates/cli/src/frontend/progress/format/list.rs. A unit test pins the literal against `libc::S_ISGID`, widening both sides to `u64` so the comparison itself stays lint-clean on either platform. Value-identical to the previous code, so the measured macOS testsuite result (235 pass / 5 fail) is unchanged.
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.
oc-rsync -rp --chmod=a+s src/ dst/aborted the whole transfer with exit 23 andfailed to preserve permissions '.../dst/': Operation not permittedfor anordinary user on macOS. The real rsync 3.5.0 binary completes the same command
with exit 0 on the same host.
Root cause
Upstream reaches every mode change through
do_chmod()->chmod(2)(
syscall.c:761). oc anchors the same change on a walked parent dirfd forsymlink-race safety, so it issues
fchmodat(2)instead. On macOS the twosyscalls do not agree about
S_ISGIDfor an unprivileged caller.Measured with a standalone C probe (euid 501, caller owns the target, same
inode and same mode word for both calls):
chmod(2)fchmodat(2)07550755 | S_ISUID04755047550755 | S_ISGID007550755 | both04755The kernel silently masks a set-group-ID bit it will not grant when the
request arrives through
chmod, but refuses the identical request throughfchmodat. Linux masks on both, which is exactly why the cell passes there andonly macOS fails.
The
0755row is the control:fchmodatworks normally, so the refusalisolates to one bit rather than to the probe being broken.
Oracle
Same fixture, same argv, both binaries, this host:
drwsr-xr-x-rwSr--r--drwxr-xr-x-rw-r--r--drwsr-xr-x-rwSr--r--After the fix oc is byte-identical to upstream on all four entries.
The fix
secure_chmod_atroutes through a helper that retries the refusal once withS_ISGIDcleared - exactly the mode the platform's ownchmodwould haveapplied. This keeps the dirfd anchor (the confinement property) while restoring
upstream's acceptance semantics.
openat+fchmodwas considered and rejected: opening the leaf would block ona FIFO and fail on an unreadable file, hazards
chmoddoes not have.The retry cannot mask a genuine permission error - a caller that does not own
the target is refused for the setgid-free mode too, and that error propagates.
It is also naturally inert on Linux, where
fchmodatdoes not refuse whatchmodmasks, so no platformcfgis needed.Testsuite
macOS upstream-3.5.0 nonroot leg, freshly built release binary:
macos-setgid-ordinary-mode-regressionmoves fail -> pass; its expect row isflipped in the same commit because the defect is fixed, not re-baselined.
⚠ chmod-setid is deliberately left failing
That cell asserts
--chmod=a+ssets both setuid and setgid, and the resulthere is mode
4644- setuid only. The platform will not grant setgid to thiscaller at all, and the measured oracle above shows the real rsync 3.5.0 binary
landing on the same setuid-only mode (
-rwSr--r--) on this host. The assertionis unsatisfiable in this environment rather than an oc defect, so its
failrowis unchanged.
Verification
cargo fmt --all -- --checkclean.cargo clippy -p fast_io --all-targets --no-deps -D warningsexits 0 with thetwo lints this toolchain already fails on in untouched files allowed
(
collapsible_if,derivable_impls); zero hits in the changed file, checkedby name after confirming the crate actually compiled - an unlinted crate
reports no hits either.
fmtrebuild: 235/5/105,same failing set.