receiver.c: Tighten alt-dest path resolution and partial-dir state validation - #1077
receiver.c: Tighten alt-dest path resolution and partial-dir state validation#1077seks99x wants to merge 1 commit into
Conversation
a68e9b6 to
90b5f7f
Compare
This introduces architectural best-practices to harden the receiver's state machine and harmonize symlink handling across the delta-basis engine, addressing protocol edge-cases reported by Fyyre (James). - receiver.c (recv_files): Added explicit validation to ensure one_inplace is only triggered when partial_dir is configured and the FNAMECMP_PARTIAL_DIR token is legitimate. - receiver.c (secure_basis_open): Enforced O_NOFOLLOW on leaf components when resolving operator-supplied paths, aligning it on operator-path behavior. Co-authored-by: Fyyre <fyyre@fyyre.net>
steadytao
left a comment
There was a problem hiding this comment.
The partial_dir state guard looks reasonable, but the leaf-opening rewrite changes established operator-path and --insecure-links behaviour without tests. Please preserve the explicit compatibility opt-out and add focused positive and negative cases before this merges. The diff also needs normal rsync indentation and comment cleanup.
| dfd = owner_walk_parent(p, &leaf); | ||
| if (dfd < 0) | ||
| return -1; | ||
| fd = openat(dfd, leaf, flags | O_NOFOLLOW, mode); |
There was a problem hiding this comment.
This bypasses the existing operator-path symlink policy and unconditionally rejects the leaf. In particular, --insecure-links can no longer restore legacy following for an operator-owned leaf symlink while open_no_attacker_symlinks() deliberately follows trusted leaves or honours the explicit opt-out. Please preserve that contract and add regressions for a trusted leaf, an untrusted leaf and --insecure-links.
There was a problem hiding this comment.
I spent a lot of time yesterday tracing these exact code paths, and I'd like to clarify a few mechanics regarding how the leaf is handled here. --insecure-links is already preserved, the legacy following behavior for --insecure-links is not broken by this patch. There is an early return check for symlink_optout_allowed() at the top of the function that falls back to the raw do_open() syscall, which bypasses this O_NOFOLLOW block entirely. open_no_attacker_symlinks() function appears to be designed for non-operator paths (like --exclude-from, --read-batch, or --files-from), which typically target specific files. Operator paths behave differently, and the existing comments indicate they are not intended to follow leaf symlinks ( check copy_file() also ).
What "leaf" actually means here for operator paths is that the path is usually concatenated (e.g., basedir + fname). When owner_walk_parent separates the leaf, it is rejecting the destination file itself (e.g., file.txt in /home/Omar/leaf/file.txt), not the parent directory. If the parent directory (leaf/) is a symlink, it is still followed normally if trusted. I've written regression tests that confirm this directory-level leaf symlink traversal works exactly as expected.
Using open_no_attacker_symlinks() here for the leaf file would actually introduce inconsistency. Even if it allowed us to successfully open a symlinked file.txt, subsequent operations (like do_rename_at) would still fail on it, because those downstream syscalls strictly refuse to follow leaf symlinks anyway. Also standard transfers already refuse to follow operator-path fname leaf symlinks, a legitimately 'trusted' symlink is one explicitly configured by the operator as part of the base path itself, not a leaf link planted during the transfer.
74615f2 to
aefcad2
Compare
This introduces a couple of architectural best-practices to harden the receiver's state machine and harmonize symlink handling across the delta-basis engine, addressing protocol edge-cases reported by Fyyre (fyyre@fyyre.net).
Strict State Validation for partial_dir (receiver.c)
Added explicit validation to ensure
one_inplaceis only triggered whenpartial_diris actually configured by the client and theFNAMECMP_PARTIAL_DIRtoken is legitimate. This prevents any potential protocol state confusion where a peer sending unexpected/forged tokens could bypass temp-file cleanup or misroute in-place writes.Harmonized Leaf Symlink Handling (receiver.c)
When an operator path was sent,
secure_basis_open()calledopen_no_attacker_symlinks()for partial-dir/link-dest/fuzzy-dest path resolving, which followed leaf symlinks. Since standard operator-path behavior dictates that leaves should not be followed (consistent with standard do_*_at style, copy_file()) I've updated this logic to enforce O_NOFOLLOW on the final component and walk the parent withowner_walk_parent, bringing our path resolution into perfect alignment with standard operator-path behavior.