receiver: fix basis file open for directory symlinks (issue #715)#864
Open
samueloph wants to merge 2 commits intoRsyncProject:masterfrom
Open
receiver: fix basis file open for directory symlinks (issue #715)#864samueloph wants to merge 2 commits intoRsyncProject:masterfrom
samueloph wants to merge 2 commits intoRsyncProject:masterfrom
Conversation
…ect#715) The secure_relative_open() change (c35e283) passes the full destination path as relpath with basedir=NULL for FNAMECMP_FNAME, applying O_NOFOLLOW to every component including intermediate directories. This breaks legitimate directory symlinks on the receiver (e.g. -K), causing "failed verification -- update discarded" on delta transfers when the basis file can't be opened. Fix by splitting file->dirname into basedir and file->basename into fnamecmp for the FNAMECMP_FNAME case. secure_relative_open() already follows symlinks in basedir, so directory symlinks are traversed while O_NOFOLLOW still protects the final component. After reconstruction, restore fnamecmp = fname to preserve pointer identity for downstream checks (finish_transfer's backup handling, updating_basis_or_equiv, directory-is-basis). The split guard includes "fnamecmp == fname" because protocol < 29 may set fnamecmp to partialptr without updating fnamecmp_type. Also clear basedir in the daemon filter fallback and the protocol < 29 retry path to prevent stale values from prior cases. The CVE fix is preserved: FNAMECMP_FUZZY and basis_dir paths still use secure_relative_open() with O_NOFOLLOW on all relpath components. For FNAMECMP_FNAME, the generator and receiver write path already follow directory symlinks in the same way, so no new information channel is created. file->dirname is validated against ".." by clean_fname(). Fixes: RsyncProject#715 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add a test that verifies the CVE fix in commit c35e283 is working: secure_relative_open() must reject symlinks in relpath components when the receiver opens basis files. The test uses --copy-dest with a basis directory where an intermediate path component is a symlink. The generator finds the file through the symlink (it uses link_stat/do_open_checklinks which follow intermediate symlinks), but the receiver's secure_relative_open must refuse to open it. This is detected by checking for the absence of the "recv mapped" log line, which only appears when the receiver successfully opens and maps a basis file. A control test with a real directory (no symlink) verifies the basis file IS used when the path contains no symlinks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Was this supposed to fix and ends with |
Contributor
Author
|
@mcepl thanks for testing, can you confirm that you do have a patched rsync both on the sender and receiver side (although I believe you just need the receiver to be patched)? The patch is not there in https://build.opensuse.org/package/show/home:mcepl/rsync |
Correct, the fixed |
Contributor
Author
|
Awesome, thanks for checking! |
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
2 participants
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.
(The patch on this PR was generated with Claude)
I've noticed that the latest two commits were done with the use of an AI agent, I took that as a signal that it would be ok to submit this PR.
The regression from #715 has been bothering users for a roughly one year now, and since I don't have enough time to investigate the source code and handwrite a patch myself, I though I would give Claude a chance.
I've reviewed everything in this PR myself, but I didn't go too deep in understanding the internals of rsync and that's where there's room for issues, I've focused heavily on unit tests to attest behaviors.
For something sensitive like this, I guess the only way to be sure is to have @tridge double check it, but I will understand if you don't want to review it. In the worst case, I suppose this PR is better than nothing.
These is a summary of the things I did to lower the chances of hallucinations and mistakes:
(1) Ground the agent in primary sources - fetch the issue, read the CVE fix commit and full function before proposing changes.
(2) Require build + test at every step - the agent compiled, wrote a regression test, verified it fails without the fix, and ran the full suite after each change.
(3) Explicitly ask the agent to verify security properties - without prompting, the agent hadn't analyzed whether the fix reintroduces the CVE.
(4) Explicitly ask about flag interactions - this uncovered two bugs (--partial-dir with protocol < 29, stale basedir in fallback path) the agent missed on first pass.
(5) Demand thoroughness after mistakes - after the agent missed those two, pushing harder uncovered a third bug (--backup xattr handling via pointer identity).
(6) Add test coverage for each discovered bug - the final test has 8 scenarios covering every edge case found.
(7) Test the absence of a test - the original CVE fix had no regression test, so one was added and validated by temporarily reverting the fix. The core lesson: the agent's first-pass fix was correct for the reported bug but had three latent issues causing silent data loss in edge cases; each was found only after explicit human direction to look harder.
(8) Explicitly ask it to verify how this patch interacts with the fixes for CVE-2024-1208{4..8}, which is where the regression came from.