fix(receiver): clear a symlink standing at the --partial-dir name - #7657
fix(receiver): clear a symlink standing at the --partial-dir name#7657oferchen wants to merge 2 commits into
Conversation
A peer-supplied `--partial-dir` naming a symlink out of the served tree was accepted as "the directory is already there", after which the staging rename followed the link and wrote outside the daemon module. MEASURED against a daemon push of `-a --delay-updates --partial-dir=/blink` where `mod/blink` was a symlink to a directory outside the module, with real rsync 3.5.0 as the control on the same host: | | exit | dest mod/f0 | outside victim | |---|---|---|---| | upstream 3.5.0 | 2, `refusing unconfined partial basis for f0` | OLD | intact | | oc before | 23 | NEW | DESTROYED | | oc after | 0 | NEW | intact | The escaping site is the `--delay-updates` staging create, not the basis open and not `rename_to_partial_dir`. `clear_partial_dir_obstruction` deliberately left a symlink standing, on the stated grounds that "the confined rename makes the refusal". That premise is refuted by the run above: the staging rename follows the link into the outside directory, and the delayed-updates sweep then renames the file back out, which is why the victim's directory ends up empty. Leaving the link could not work in any case, because `mkdirat` reports `EEXIST` for a symlink and every caller on this path maps `EEXIST` to success - `fast_io::operator_mkdir` and `create_dir_all_sandboxed` both do. So the obstruction was never merely tolerated; it was read as a usable directory. upstream util1.c:1516-1527 `handle_partial_dir(..., PDIR_CREATE)` is the rule: under `operator_path_resolve`, `do_lstat_at` finds a non-directory at the name - `!S_ISDIR` covers a symlink and a regular file alike - `do_unlink_at` clears it, and a failure to clear returns 0, at which point receiver.c:1302-1306 discards the received temp rather than staging around the obstruction. Clearing is what makes it safe: the walk resolves the parent chain and hands `unlinkat` a single leaf component, so the link is removed as the link it is and never followed. `fast_io::operator_unlink` is added as the sibling of `operator_mkdir` that this needs - the same ownership walk, the same shape, mirroring upstream's pairing of `do_unlink_at` with `do_mkdir_at` inside one `operator_path_resolve` region. Also fixes a second, non-security divergence measured on the way: with a RELATIVE `--partial-dir=blink` upstream clears the obstruction and completes (exit 0, destination updated) while oc exited 11 with the destination untouched. oc now matches upstream on that shape too. Mutation-proven: restoring only the symlink arm reproduces the original escape end to end (victim destroyed, exit 23) and reddens `clearing_the_partial_dir_removes_a_symlink_without_touching_its_target` with "the symlink standing at the partial-dir name must be removed". The pin asserts BOTH that the link is gone AND that its target survives - "the link is gone" alone would also hold for an implementation that followed the link and deleted the target, which is the opposite of this fix. DELIBERATELY NOT CHANGED, and the reason the upstream-testsuite cell `operator-path-partial-dir-daemon` still fails: upstream never sanitizes `partial_dir`. options.c:2402-2430's `if (sanitize_paths)` block covers `argv`, `tmpdir` and `backup_dir` and NOT `partial_dir`, so on a daemon `/blink` stays absolute, names the filesystem root, and cannot be created - which is where upstream's non-zero exit comes from. oc re-roots it under the module instead (the hardening added for the daemon partial-dir), so oc completes the transfer where upstream refuses. That is a policy question about whether a peer may name an absolute staging path outside the served module, not something to flip silently in a security fix; the cell's row is not re-baselined. Verified on the pinned 1.88.0 toolchain: `cargo fmt --all -- --check` clean, `cargo clippy --workspace --all-targets --all-features --no-deps -- -D warnings` clean, engine 4719/4719.
…patchable bug The row's one-line classification read "real oc defect", which invites the wrong fix. Its security oracle now passes; the residual is the first oracle's non-zero exit, and upstream's non-zero exit comes from options.c:2402-2430 never sanitizing partial_dir - an absolute daemon --partial-dir stays absolute and cannot be created. oc re-roots it under the served module, so greening the row means letting a peer name a staging path outside the module.
|
Converting to draft: this change regresses four Linux legs and greens nothing. My own design was wrong, and the CI measured it. What CI reportedAll four with the cell's own message: Root cause — the design, not a detail
This branch's commit is Why this greens nothingPer-platform baselines on master:
Linux already refuses correctly — that is what the What the fix has to be insteadRefuse, don't clear. A peer-supplied Method noteThe macOS-only verification I ran (236/4, "zero regression") was measured on |
|
Correcting two claims in my earlier comment, both refuted by measurement. The conclusion that this branch must not land is unchanged; the reasons behind it were partly wrong. The fixture, run head-to-head on both platformsSame commit on both hosts (
Linux refuses. macOS follows the peer's escaping Correction 1: macOS is the same defect, not a different oneMy earlier comment said "the macOS divergence needs its own root-cause pass; it is not the same Correction 2: Landlock is not what makes Linux refuseThe obvious hypothesis was that Linux refuses via Landlock and macOS has no equivalent - the daemon Refuted by A/B. With So the refusal is userspace - an What is NOT yet establishedI have localised the Linux refusal to an Why this branch still must not landUnchanged from before, and now better supported: clearing the symlink and proceeding is the |
|
Narrowing follow-up. The platform split is not in the symlink policy — it is entirely in the handling of an absolute
Three of the four cells agree. macOS refuses the relative escaping symlink exactly as Linux does, which rules out the ownership walk's symlink-trust test and the So the defect is on the route an absolute operand takes before or instead of that walk. Whoever picks this up should start there rather than in The harness is |
|
Root-caused. It is not a macOS defect, and I have to correct my own previous comment again: Landlock is the masker after all — my refutation two comments up was measured on a fixture shape that cannot show it. The defect
macOS hits it in the testsuite only because the harness scratch dir is under The measurementsSame tree on both hosts. Fixture is the cell's own: module
The last row is the one that matters: the userspace confinement fails on Linux too, and only Landlock stops the escape there. Take Landlock away — a kernel too old for it, Correction to my previous commentI wrote that the Where it is notTwo candidates measured and excluded, so nobody re-treads them:
What the fix has to doCarry one spelling of the module root and the staging path, and compare those. Whether that is canonicalising the root to match, or refusing to canonicalise the operand, is the design call — but the guard cannot compare two different spellings of the same directory and be sound. And it must not rely on Landlock: the table above shows Landlock is currently load-bearing for a userspace guard that is supposed to stand on its own. |
|
Superseded by #7659, which takes the opposite shape: refuse the escaping operand rather than clear the obstacle and proceed. The root cause is now named and measured. Both Two corrections to my own earlier comments on this PR:
#7659 routes both onto Closing this branch unmerged. |
A peer-supplied
--partial-dirnaming a symlink out of the served tree wasaccepted as "the directory is already there", after which the
--delay-updatesstaging rename followed the link and wrote outside the daemon module. The
delayed-updates sweep then renamed the file back out, leaving the victim's
directory empty.
Measured
Daemon push,
-a --delay-updates,mod/blinka symlink to a directory outsidethe module, with real rsync 3.5.0 as the control on the same host:
--partial-dir/blinkabsrefusing unconfined partial basis for f0, dest OLD, victim INTACT/blinkabsUnable to create partial-dir ... discarding completed file., dest OLDblinkrelThe third row is a second, non-security divergence fixed by the same change:
with a relative
--partial-dirupstream cleared the obstruction and completedwhere oc failed the file.
Root cause
clear_partial_dir_obstructionhad an explicit symlink pass-through arm,justified by "the confined rename makes the refusal". The run above refutes
that premise - the staging rename follows the link.
Leaving the link could not have worked in any case:
mkdiratreportsEEXISTfor a symlink, and both
fast_io::operator_mkdirandcreate_dir_all_sandboxedmap
EEXISTto success. The obstruction was never merely tolerated; it was readas a usable directory at every caller on this path. That also rules out the
tempting DRY fix of routing the staging create onto the shared
engine::create_partial_dir- it swallowsEEXISTtoo, so the routing wouldhave been inert.
The fix
util1.c:1516-1527handle_partial_dir(.., PDIR_CREATE)is the rule: underoperator_path_resolve,do_lstat_atfinds a non-directory at the name -!S_ISDIRcovers a symlink and a regular file alike -do_unlink_atclears it,and a failure to clear returns 0, at which point
receiver.c:1302-1306discardsthe received temp rather than staging around the obstruction.
Clearing is what makes it safe: the ownership walk resolves the parent chain and
hands
unlinkata single leaf component, so the link is removed as the link itis and never followed.
fast_io::operator_unlinkis added as the sibling ofoperator_mkdirthat this needs, mirroring upstream's pairing ofdo_unlink_atwith
do_mkdir_atinside oneoperator_path_resolveregion.Verification
Mutation-proven in both directions: restoring only the symlink arm reproduces
the original escape end to end (victim destroyed, exit 23) and reddens
clearing_the_partial_dir_removes_a_symlink_without_touching_its_target. Thatpin asserts both that the link is gone and that its target survives - "the link
is gone" alone would also hold for an implementation that followed the link and
deleted the target, which is the opposite of this fix.
clearing_the_partial_dir_removes_a_regular_file_and_reuses_a_directoryis thenon-vacuity companion and stays green throughout.
On the pinned 1.88.0 toolchain:
cargo fmt --all -- --checkclean,cargo clippy --workspace --all-targets --all-features --no-deps -- -D warningsclean, engine 4719/4719. Full macOS upstream-3.5.0 leg run locally: 236 passed /
4 failed / 105 skipped, matching the committed manifest exactly - the four
failrows are the four already classified there, and no passing cell regressed.
Deliberately not changed
The upstream-testsuite cell
operator-path-partial-dir-daemonstill fails, andits manifest row is not re-baselined. Its security oracle now passes. What
remains is the cell's first oracle, which wants a non-zero exit - and upstream's
non-zero exit comes from somewhere else entirely:
options.c:2402-2430'sif (sanitize_paths)block coversargv,tmpdirandbackup_dirand notpartial_dir, so on a daemon/blinkstays absolute, names the filesystem root,and cannot be created.
oc re-roots it under the served module instead (the hardening from the earlier
daemon
--partial-dirwork). Matching upstream there means letting a peer namean absolute staging path outside the served module - a question about oc's
confinement policy, not something to flip silently inside a security fix. The
second commit writes that reasoning into the manifest header, because the row's
previous one-line classification ("real oc defect") invites exactly the wrong
fix.
Residual, filed not fixed
engine::create_partial_dir's ownOk(_) => {}arm still falls through to theEEXIST-swallowingoperator_mkdir- the same defect class on thefinalize_partial/rename_to_partial_dirpath. It is unmeasured as reachable,so it is recorded rather than speculatively changed here.