sender: resolve --files-from entries under a module subdirectory

[darkguy2008]

Hey! This PR came from an app I'm working on, which is some kind of game console. While using rsync there, I found that when I pull from a subdirectory of a daemon module with --files-from, rsync looks for each file at the module root instead of in that subdirectory. Depending on what's there, the files are reported as vanished, or a same-named file from the module root gets sent instead.

Here's the description generated by my LLM on this. I've also made sure that it ran all the tests to completion, all green, and A/B testing to make sure that, without this fix, the problem actually arises and it's fixed with this PR. Let me know if you need me to do anything else to make sure this PR can be merged. Thank you in advance!

Problem

This is the kind of pull that breaks:

rsync -rt --files-from=list rsync://host/mod/subdir/ dst/

• If no same-named file exists at the module root, every listed file is reported as "file has vanished", and rsync exits with code 24.
• If a same-named file exists at the module root, rsync silently sends that file instead of the one in subdir/. This bypasses the module's filter: with filter = + /subdir/*** - *, the excluded module-root file is sent.
• With --remove-source-files, if that module-root file has the same size and mtime as the listed one, it is deleted, even when the module filter excludes it. Otherwise the sent files are kept with Skipping sender remove for changed file, and rsync exits with code 23.

The same files pulled from the module root work:

rsync -rt --files-from=list rsync://host/mod/ dst/   # entries prefixed with subdir/

Seen with rsync 3.5.1 on Fedora 44, and reproduced on current master.

Root cause

In send_files(), the secure_relpath_active() branch opens each file from the module root with secure_relative_open(). It rebuilds the path as F_PATHNAME + "/" + f_name. secure_sender_parent_fd(), used by --remove-source-files, rebuilds the parent directory the same way.

A --files-from entry has no F_PATHNAME. Like any relative pathname, it is relative to orig_dir, the transfer's source directory. In this case that is the module subdirectory. Leaving out orig_dir makes both look under mod/<entry> instead of mod/subdir/<entry>.

Fix

Prefix both path builds with the module-relative part of orig_dir, through a one-line helper, orig_subdir(). change_pathname() uses the same rule when it chdirs: a relative dir is resolved from orig_dir. An absolute F_PATHNAME (a module with path = /, #897) is left unchanged. orig_dir is no longer static in flist.c, so sender.c can read it.

Verification

• New testsuite/daemon-files-from-subdir_test.py pulls file.txt with --remove-source-files from mod/subdir/ of a module with filter = + /subdir/*** - *. A same-named, same-size, same-mtime file.txt sits at the module root, excluded by the filter. The test checks that the subdirectory's file is sent and removed, and that the excluded file is left in place. On Fedora 44 (aarch64 and x86_64) it fails on master (the excluded file is sent) and passes with the fix. It also fails with only the send_files() half of the fix: the right file is sent, but --remove-source-files keeps it and deletes the excluded module-root file instead.
• make check passes with the fix on Fedora 44 aarch64: 302 passed, 65 skipped, 0 failed.
• Repro script, run with the rsync binary as $1:

#!/bin/sh
rsync=$1
work=$(mktemp -d)
mkdir -p "$work/module/subdir/blobs/sha256"
echo payload > "$work/module/subdir/blobs/sha256/abc"
printf 'use chroot = no\nport = 18873\npid file = %s/pid\nlog file = %s/log\n[mod]\npath = %s/module\nread only = yes\n' "$work" "$work" "$work" > "$work/conf"
"$rsync" --daemon --config="$work/conf"
until [ -s "$work/pid" ]; do :; done
echo blobs/sha256/abc | "$rsync" -rt --files-from=- rsync://127.0.0.1:18873/mod/subdir/ "$work/subdir-out/"; echo "subdir source: exit $?"
echo subdir/blobs/sha256/abc | "$rsync" -rt --files-from=- rsync://127.0.0.1:18873/mod/ "$work/root-out/"; echo "module root source: exit $?"
kill "$(cat "$work/pid")"

Master prints subdir source: exit 24. With the fix it prints exit 0.

Related

• Regression in 3.4.3, "Invalid argument (22)" on all file reads when using native protocol #897 / Security fix in flist.c breaks --delete-missing-args with --files-from #910 / Security fix breaks --link-dest via rsync daemon #915 / secure_relative_open() portable fallback cannot create a new final component (ENOENT vs ENOTDIR). Daemon --inplace fails to create files on kernels without openat2. #1033: other regressions from the same symlink-race hardening. None of them covers this case.
• options.c: Fix --files-from confinement for local and SSH transfers #1094 (merged): fixes --files-from confinement for local and SSH transfers, not daemon module subdirectories.
• No open issue or PR covers this bug.

[steadytao]

Quite unfortunate that this was submitted publicly..... private reporting does exist for a reason :/

[darkguy2008]

Quite unfortunate that this was submitted publicly..... private reporting does exist for a reason :/

Hey! Ouch, I’m sorry about that! It wasn’t my intention honestly, I’d rather close the PR and kill the repo (or create a new one) for submitting this in a cleaner way? I wasn’t aware at all that this caused a security issue 😅 I was more concerned about it returning incorrect results, as I was more focused on data integrity at the time.

Now that you explain the above it makes sense to me. I’ll apply your changes though, but feel free to tell me how else do you want me to proceed, I’ll take this as learning experience and be more careful next time! 🙏🏻

[steadytao]

Nope its all good. This can be kept public. But in the future, I do tend to prefer to lean on the side of caution with security-adjacent work. Can always then move it to public if deemed unnecessary :D -- in this case, its more so that our documentation guarantees certain security guarantees and this bypasses them. Much appreciated for the report and work itself.

[darkguy2008]

Nope its all good. This can be kept public. But in the future, I do tend to prefer to lean on the side of caution with security-adjacent work. Can always then move it to public if deemed unnecessary :D -- in this case, its more so that our documentation guarantees certain security guarantees and this bypasses them. Much appreciated for the report and work itself.

No problem, actually thank you! Really appreciate it! 🙏🏻 and noted! I'll make sure to go through private reporting first if I ever find anything that's security-adjacent 😅 I also pushed the requested changes (filter in the test fixture), so it should be ready for another look whenever you have time.

Thanks again for being cool with it! 🙏🏻

[steadytao]

LGTM. Thanks for the PR.

[steadytao]

@darkguy2008 For future note as well. The primary concern is data integrity however it is technically an authorisation bypass just very narrow and configuration-dependent + the attacker would require access to the module already and I feel you have bigger issues at that point so not worth a CVE but when it comes to edge-cases that are security-adjacent (even data integrity) its always good to submit privately first and then pivot to public if needbe in these cases as it is somewhat common for these edge/corner cases to explode and become worse (and in other projects this could legitimately be worth a low/medium CVE) -- of course dont be overly cautious with things that really aren't CVE-worthy but its always good to be safe.

[darkguy2008]

LGTM. Thanks for the PR.

Awesome!! No problem, glad to have been able to contribute and thank you for being cool with it as well 😁 it feels good to be able to contribute now to great projects like this one!

@darkguy2008 For future note as well. The primary concern is data integrity however it is technically an authorisation bypass just very narrow and configuration-dependent + the attacker would require access to the module already and I feel you have bigger issues at that point so not worth a CVE but when it comes to edge-cases that are security-adjacent (even data integrity) -- its always good to submit privately first and then pivot to public if needbe in these cases as it is somewhat common for these edge/corner cases to explode and become worse (and in other projects this could legitimately be worth a low/medium CVE) -- of course dont be overly cautious with things that really aren't CVE-worthy but its always good to be safe.

Yeah that's what I was thinking too, if you have access to the module then you've got bigger problems to solve 😅, but it was a good catch!

Understood though, if I ever need to do the same again here or maybe in other projects too, I'll take your advice. Appreciate you for this experience!