Skip to content

fix(receiver): confine the --delay-updates staging path to the module - #7659

Merged
oferchen merged 1 commit into
masterfrom
fix/partial-dir-staging-confinement
Sep 4, 2026
Merged

fix(receiver): confine the --delay-updates staging path to the module#7659
oferchen merged 1 commit into
masterfrom
fix/partial-dir-staging-confinement

Conversation

@oferchen

@oferchen oferchen commented Sep 4, 2026

Copy link
Copy Markdown
Owner

Closes the operator-path-partial-dir-daemon divergence by refusing the escaping operand, which is what upstream does and what the Linux baseline already encodes. Supersedes #7657, whose shape (clear the symlink and proceed) was the operand-ignored outcome the cell rejects.

The defect

On a daemon receiver --partial-dir is peer-supplied. --delay-updates stages every file through <--partial-dir>/<basename> and later renames it onto the destination. Both of those renames walked the ownership walk in its Ancillary spelling, and AbsPathTracker::start returns Disabled for anything that is not PathKind::Confined — so the session confinement root was never consulted on this path at all.

A symlink standing at the partial-dir name therefore sent the staged file — a complete copy of the source — outside the served module, and the delayed sweep then renamed it back over the destination.

Measured with the cell's own fixture (module mod with blink -> <outside>/secret planted inside, -a --delay-updates --partial-dir=/blink src/ <url>mod/):

exit mod/f0 outside secret/f0
before 23 replaced destroyed
after 11 untouched intact

How it was localised

create_partial_dir and partial_dir_rename were both excluded by instrumentation first — neither is on this path. Probing every operator_* entry point named the two real sinks:

ENTRY operator_rename        path=".../mod/blink/f0"     <- receiver/transfer.rs sweep_rename
ENTRY owner_trusted_parent   path=".../mod/blink"        <- Ancillary, tracker DISABLED

Both halves are load-bearing

Measured in isolation, not assumed:

exit dest victim
neither confined 23 REPLACED DESTROYED
sweep only 23 untouched DESTROYED
both 11 untouched intact

Confining the sweep protects the destination; confining the staging rename protects the victim. Neither alone is sufficient.

Why not rename_config_sandboxed

The staging endpoint deliberately does not reuse it. Its renameat_via_sandbox_or_fallback helper drops to a path-based rename when the anchored open fails — the drop-to-unconfined that upstream names as wrong. syscall.c open_dir_secure() signals policy with errno == 0 precisely so a runtime errno cannot select the unconfined arm.

Upstream

  • util1.c:1518-1530 handle_partial_dir(..., PDIR_CREATE) runs the whole retention under operator_path_resolve
  • syscall.c:1891 do_rename_at() under that flag
  • receiver.c:546 do_rename(partialptr, fname) — the sweep rename

Verification

  • macOS upstream 3.5.0 testsuite on this exact base: 237 passed / 3 failed (was 235/5). operator-path-partial-dir-daemon passes.
  • The three residual rows are the ones the manifest header already classifies: chmod-setid and partial-protected-regular-retry-policy are unsatisfiable on this host (real rsync 3.5.0 lands on the same result), filter-merge-content-echo is owned separately.
  • macOS nonroot expect manifest flips the row fail -> pass in the same commit; the header's count and classification list are reconciled with it.
  • cargo fmt --all -- --check and cargo clippy --workspace --all-targets --all-features clean on the pinned 1.88.0 toolchain.
  • The relative-operand control is unchanged (exit 11, dest untouched, victim intact) before and after.

On a daemon receiver `--partial-dir` is peer-supplied, and `--delay-updates`
stages every file through `<--partial-dir>/<basename>` before renaming it onto
the destination. Both of those renames walked the ownership walk in its
`Ancillary` spelling, which leaves `AbsPathTracker` disabled and so never
consults the session confinement root at all. A symlink standing at the
partial-dir name therefore sent the staged file - a complete copy of the
source - outside the served module, and the delayed sweep then renamed it back
over the destination.

Measured against the cell's own fixture (module `mod` with
`blink -> <outside>/secret` planted inside it,
`-a --delay-updates --partial-dir=/blink src/ <url>mod/`), macOS exited 23 with
`mod/f0` replaced and the outside `secret/f0` destroyed. Both endpoints now
take `operator_rename_confined`, and the run exits 11 with the destination
untouched and the victim intact - the same refusal the Linux baseline already
encodes.

Each half is load-bearing, measured in isolation:

  neither confined     exit 23, dest REPLACED,  victim DESTROYED
  sweep only           exit 23, dest untouched, victim DESTROYED
  both                 exit 11, dest untouched, victim intact

The staging endpoint deliberately does not reuse `rename_config_sandboxed`:
its `renameat_via_sandbox_or_fallback` helper drops to a path-based rename when
the anchored open fails, which is the drop-to-unconfined upstream names as
wrong - `syscall.c` `open_dir_secure()` signals policy with `errno == 0` rather
than letting a runtime errno select the unconfined arm.

upstream: util1.c:1518-1530 `handle_partial_dir(..., PDIR_CREATE)` runs the
whole retention under `operator_path_resolve`; syscall.c:1891 `do_rename_at()`
and receiver.c:546 `do_rename(partialptr, fname)` are the two renames.

The macOS nonroot expect manifest flips `operator-path-partial-dir-daemon`
fail -> pass in the same commit. macOS leg 235/5 -> 237/3; the three residual
rows are the ones the manifest header already classifies (two unsatisfiable on
this host with real rsync 3.5.0 landing on the same result, one owned).
@github-actions github-actions Bot added the bug Something isn't working label Sep 4, 2026
@oferchen
oferchen merged commit 14c7c6d into master Sep 4, 2026
75 checks passed
@oferchen
oferchen deleted the fix/partial-dir-staging-confinement branch September 4, 2026 06:28
oferchen added a commit that referenced this pull request Sep 4, 2026
… them (#7661)

`README.md` ships the exact `awk` command that reads a leg's expected-outcome
manifest and prints its pass/fail/skip counts. Running it disagrees with every
row of the table directly beneath it.

| row | documented | re-derived |
|---|---|---|
| non-root, pipe | 257 / 3 / 85 | **259 / 1 / 85** |
| root, pipe | 286 / 3 / 56 | **288 / 1 / 56** |
| non-root, tcp | 101 / 21 / 33 | **108 / 14 / 33** |
| root, tcp | 113 / 27 / 15 | **120 / 20 / 15** |

`SECURITY.md` carries the same four rows and the same two summary figures, so
both files drifted together. Corrected throughout, along with:

- **"3 of 345 tests currently diverge"** across the full-corpus legs. It is
  **one** - `filter-merge-content-echo` - and the README names it now rather
  than leaving the reader to count.
- **"29 across all four"** distinct failures. It is **23**, and across
  **five** manifests, not four: the README's own glob
  `tools/ci/upstream-3.5.0-expect.*.txt` has always matched the macOS file its
  prose excluded.
- **The macOS leg was missing from both tables.** It runs on every PR as
  `upstream-testsuite-macos` (`ci.yml`), on the full 345-cell corpus, with its
  own committed manifest. It is the only leg that can observe a
  platform-conditional divergence - one of its three remaining failures *skips*
  on Linux, so it had never executed in this repository's CI before the leg
  existed. Added as a fifth row, with its non-required status stated.
- **"the required checks being the two stdio-pipe legs."** All four Linux legs
  have been required contexts since #7408 wired the TCP pair into PR CI; the
  ruleset returns ten contexts, not eight.
- **`proxy protocol hosts` described as "not yet implemented"** and "still
  under audit" in two places. It shipped in #7648: parsed into a
  `ProxyProtocolPolicy` that mirrors upstream's `allow_proxy_protocol_peer()`,
  rejecting every peer when the trusted list is empty or unset, and warning at
  startup on the combination upstream warns about.
- **The `MAX_PROXY_LINE_BYTES` citation** pointed at `connect/proxy.rs:344`.
  The constant moved and, more usefully, stopped being a typed literal: it is
  now `PROXY_BUF_SIZE - 1`, so the doc records the derivation rather than a
  line number that will drift again.

The two macOS-leg rationale comments in the workflows carried the same pre-fix
counts and a failure list six entries out of date. Both are recounted from the
manifests.

`CHANGELOG.md` stopped at #7632, leaving the 26 PRs merged since then
unrecorded. Added under Security / Fixed / Testing and CI / Documentation.

## Why the numbers were wrong in a way reading could not catch

Every figure here is the outcome column of a committed manifest. The previous
values were transcribed once and then maintained by hand, so they decayed as
fixes landed - and a reader checking the table against the prose beside it
would find them perfectly consistent with each other.

The check that finds this class is re-running the derivation, not re-reading
the text, so that is what was done: a script recomputes all five legs and both
summary figures from `tools/ci/upstream-3.5.0-expect*.txt` and asserts the two
documents contain the results, with the superseded strings blacklisted so a
partial edit cannot pass. It also caught a live error in this changeset -
#7659 landed mid-review and flipped `operator-path-partial-dir-daemon`, taking
macOS from 236/4 to 237/3 and the distinct-failure count from 24 to 23. The
figures here are derived from master with that merge in place.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant