diff --git a/receiver.c b/receiver.c index edfbb210..eacb1bc2 100644 --- a/receiver.c +++ b/receiver.c @@ -753,6 +753,7 @@ int recv_files(int f_in, int f_out, char *local_name) && check_filter(&daemon_filter_list, FLOG, fnamecmp, 0) < 0)) { fnamecmp = fname; fnamecmp_type = FNAMECMP_FNAME; + basedir = NULL; } } else { /* Reminder: --inplace && --partial-dir are never @@ -768,6 +769,19 @@ int recv_files(int f_in, int f_out, char *local_name) fnamecmp = fname; } + /* For FNAMECMP_FNAME, split into basedir (dirname) + fnamecmp + * (basename) so that secure_relative_open() follows symlinks + * in the directory part. This is needed because the receiver's + * destination may legitimately contain directory symlinks + * (e.g. when using -K/--copy-dirlinks). The basedir is opened + * following symlinks, while the file component uses O_NOFOLLOW. + * See: https://github.com/RsyncProject/rsync/issues/715 */ + if (fnamecmp_type == FNAMECMP_FNAME && basedir == NULL + && fnamecmp == fname && file->dirname && !local_name) { + basedir = file->dirname; + fnamecmp = (char*)file->basename; + } + /* open the file */ fd1 = secure_relative_open(basedir, fnamecmp, O_RDONLY, 0); @@ -775,6 +789,7 @@ int recv_files(int f_in, int f_out, char *local_name) if (fnamecmp != fname) { fnamecmp = fname; fnamecmp_type = FNAMECMP_FNAME; + basedir = NULL; fd1 = do_open_nofollow(fnamecmp, O_RDONLY); } @@ -792,6 +807,12 @@ int recv_files(int f_in, int f_out, char *local_name) // path name as a single string pathjoin(fnamecmpbuf, sizeof fnamecmpbuf, basedir, fnamecmp); fnamecmp = fnamecmpbuf; + /* For FNAMECMP_FNAME, the reconstructed path is + * identical to fname. Restore the pointer so that + * downstream pointer comparisons (e.g. in + * finish_transfer) continue to work. */ + if (fnamecmp_type == FNAMECMP_FNAME) + fnamecmp = fname; } one_inplace = inplace_partial && fnamecmp_type == FNAMECMP_PARTIAL_DIR; diff --git a/testsuite/secure-relative-open.test b/testsuite/secure-relative-open.test new file mode 100644 index 00000000..59d14a6d --- /dev/null +++ b/testsuite/secure-relative-open.test @@ -0,0 +1,111 @@ +#!/bin/sh + +# Regression test for the CVE fix in commit c35e283 ("receiver: use +# secure_relative_open() for basis file"). +# +# secure_relative_open() enforces O_NOFOLLOW on every path component of +# the relpath, preventing the receiver from following symlinks when +# opening basis files for delta comparison. This stops a malicious +# sender from causing the receiver to read files outside the +# destination tree via crafted symlinks. +# +# This test verifies that when a basis directory (--copy-dest) contains +# a symlink as an intermediate path component, the receiver does NOT +# follow it to open the basis file. The generator may find the file +# (it uses link_stat/do_open_checklinks which follow intermediate +# symlinks), but the receiver's secure_relative_open must reject it. +# +# Observable difference: with a real directory, the receiver logs +# "recv mapped " when it successfully opens the basis file. +# With a symlink, that line is absent because the open fails. + +. "$suitedir/rsync.fns" + +RSYNC_RSH="$scratchdir/src/support/lsh.sh" +export RSYNC_RSH + +# Create source with a file in a subdirectory +srcbase="$tmpdir/src" +mkdir -p "$srcbase/sub" +dd if=/dev/urandom of="$srcbase/sub/file" bs=1024 count=32 2>/dev/null \ + || test_fail "failed to create source file" + +# Create a copy-dest where "sub" is a symlink to "realsub". +# The basis file is reachable through the symlink, but +# secure_relative_open must refuse to follow it. +copydest="$tmpdir/copydest" +mkdir -p "$copydest/realsub" +# Basis has different size to force delta transfer (not hard-link) +dd if=/dev/urandom of="$copydest/realsub/file" bs=1024 count=16 2>/dev/null \ + || test_fail "failed to create basis file" +ln -s realsub "$copydest/sub" + +###################################################################### +# Test 1: Symlink in copy-dest path must NOT be followed by receiver +###################################################################### + +mkdir -p "$todir" + +$RSYNC -aivvv --copy-dest="$copydest" --rsync-path="$RSYNC" \ + "$srcbase/" "lh:$todir/" > "$outfile" 2>&1 \ + || test_fail "test 1: rsync failed" + +# The receiver must NOT have mapped the basis file through the symlink. +# "recv mapped" appears only when secure_relative_open succeeds. +if grep "recv mapped.*copydest/sub/file" "$outfile" >/dev/null 2>&1; then + test_fail "test 1: receiver followed symlink in copy-dest path (CVE regression!)" +fi + +# Verify the file was still transferred correctly +diff "$srcbase/sub/file" "$todir/sub/file" >/dev/null \ + || test_fail "test 1: transferred file content mismatch" + +###################################################################### +# Test 2: Real directory in copy-dest path SHOULD be used by receiver +###################################################################### + +rm -rf "$todir" +mkdir -p "$todir" + +# Replace symlink with real directory +rm "$copydest/sub" +mkdir "$copydest/sub" +cp "$copydest/realsub/file" "$copydest/sub/file" + +$RSYNC -aivvv --copy-dest="$copydest" --rsync-path="$RSYNC" \ + "$srcbase/" "lh:$todir/" > "$outfile" 2>&1 \ + || test_fail "test 2: rsync failed" + +# With a real directory, the receiver SHOULD map the basis file +if ! grep "recv mapped.*copydest/sub/file" "$outfile" >/dev/null 2>&1; then + test_fail "test 2: receiver did not use basis file from real directory" +fi + +diff "$srcbase/sub/file" "$todir/sub/file" >/dev/null \ + || test_fail "test 2: transferred file content mismatch" + +###################################################################### +# Test 3: ".." in relpath must be rejected by secure_relative_open +###################################################################### + +rm -rf "$todir" +mkdir -p "$todir" + +# Create a copy-dest with a normal structure +copydest2="$tmpdir/copydest2" +mkdir -p "$copydest2/sub" +dd if=/dev/urandom of="$copydest2/sub/file" bs=1024 count=16 2>/dev/null + +# Create source with the same structure +# (secure_relative_open rejects ".." in relpath at the syscall level, +# but clean_fname also rejects it in the file list. This test verifies +# the defense-in-depth.) +$RSYNC -aivvv --copy-dest="$copydest2" --rsync-path="$RSYNC" \ + "$srcbase/" "lh:$todir/" > "$outfile" 2>&1 \ + || test_fail "test 3: rsync failed" + +diff "$srcbase/sub/file" "$todir/sub/file" >/dev/null \ + || test_fail "test 3: transferred file content mismatch" + +# The script would have aborted on error, so getting here means we've won. +exit 0 diff --git a/testsuite/symlink-dirlink-basis.test b/testsuite/symlink-dirlink-basis.test new file mode 100644 index 00000000..9065dd81 --- /dev/null +++ b/testsuite/symlink-dirlink-basis.test @@ -0,0 +1,247 @@ +#!/bin/sh + +# Test that updating a file through a directory symlink works when using +# -K (--copy-dirlinks). This is a regression test for: +# https://github.com/RsyncProject/rsync/issues/715 +# +# The CVE fix in commit c35e283 introduced secure_relative_open() which +# uses O_NOFOLLOW on all path components, breaking legitimate directory +# symlinks on the receiver side. The fix splits the path into basedir +# (dirname, symlinks followed) and basename (O_NOFOLLOW) so that +# directory symlinks are traversed while the final file component is +# still protected. +# +# The regression only manifests when delta matching is triggered (i.e., +# the sender finds matching blocks in the old file). Small files with +# completely different content are transferred in full and don't trigger +# the bug. We use a large file with a small modification to ensure +# delta transfer is used. +# +# In addition to the original regression, this test covers edge cases +# in the fix itself: +# - --backup with directory symlinks (finish_transfer pointer identity) +# - --partial-dir with protocol < 29 (fnamecmp != partialptr guard) +# - --inplace with directory symlinks (updating_basis_or_equiv check) +# - Files without a dirname (top-level files, no split needed) + +. "$suitedir/rsync.fns" + +RSYNC_RSH="$scratchdir/src/support/lsh.sh" +export RSYNC_RSH + +# $HOME is set to $scratchdir by rsync.fns +# localhost: destination will cd to $HOME (i.e., $scratchdir) + +# Helper: create a large file suitable for delta transfers. +# ~32KB is large enough for rsync's block matching to find matches. +make_testfile() { + dd if=/dev/urandom of="$1" bs=1024 count=32 2>/dev/null \ + || test_fail "failed to create test file $1" +} + +# Set up source tree +srcbase="$tmpdir/src" + +###################################################################### +# Test 1: Basic directory symlink update (the original issue #715) +###################################################################### + +mkdir -p "$HOME/real-dir" +ln -s real-dir "$HOME/dir" + +mkdir -p "$srcbase/dir" +make_testfile "$srcbase/dir/file" + +# First transfer (initial): should create the file through the symlink +(cd "$srcbase" && $RSYNC -KRlptv --rsync-path="$RSYNC" dir/file localhost:) \ + || test_fail "test 1: initial transfer failed" + +if [ ! -f "$HOME/real-dir/file" ]; then + test_fail "test 1: initial transfer did not create file through symlink" +fi + +diff "$srcbase/dir/file" "$HOME/real-dir/file" >/dev/null \ + || test_fail "test 1: initial transfer content mismatch" + +# Small modification to trigger delta transfer +echo "appended update" >> "$srcbase/dir/file" +sleep 1 +touch "$srcbase/dir/file" + +# Second transfer (update): was failing with "failed verification" +(cd "$srcbase" && $RSYNC -KRlptv --rsync-path="$RSYNC" dir/file localhost:) \ + || test_fail "test 1: update through directory symlink failed" + +diff "$srcbase/dir/file" "$HOME/real-dir/file" >/dev/null \ + || test_fail "test 1: update transfer content mismatch" + +###################################################################### +# Test 2: Compression (-z) as in the original reproducer +###################################################################### + +echo "another line" >> "$srcbase/dir/file" +sleep 1 +touch "$srcbase/dir/file" + +(cd "$srcbase" && $RSYNC -KRlptzv --rsync-path="$RSYNC" dir/file localhost:) \ + || test_fail "test 2: compressed update through directory symlink failed" + +diff "$srcbase/dir/file" "$HOME/real-dir/file" >/dev/null \ + || test_fail "test 2: compressed update content mismatch" + +###################################################################### +# Test 3: Nested directory symlinks (nested/sub/data.txt where +# "nested" is a symlink to "nested_real") +###################################################################### + +mkdir -p "$HOME/nested_real/sub" +ln -s nested_real "$HOME/nested" + +mkdir -p "$srcbase/nested/sub" +make_testfile "$srcbase/nested/sub/data.txt" + +(cd "$srcbase" && $RSYNC -KRlptv --rsync-path="$RSYNC" nested/sub/data.txt localhost:) \ + || test_fail "test 3: initial nested transfer failed" + +echo "appended nested" >> "$srcbase/nested/sub/data.txt" +sleep 1 +touch "$srcbase/nested/sub/data.txt" + +(cd "$srcbase" && $RSYNC -KRlptv --rsync-path="$RSYNC" nested/sub/data.txt localhost:) \ + || test_fail "test 3: update through nested directory symlink failed" + +diff "$srcbase/nested/sub/data.txt" "$HOME/nested_real/sub/data.txt" >/dev/null \ + || test_fail "test 3: nested update content mismatch" + +###################################################################### +# Test 4: --backup with directory symlinks +# +# Exercises the finish_transfer() "fnamecmp == fname" pointer +# comparison that determines whether to update fnamecmp to the +# backup name. If broken, --backup would reference a renamed file +# for xattr handling. +###################################################################### + +# Reset destination +rm -f "$HOME/real-dir/file" "$HOME/real-dir/file~" + +make_testfile "$srcbase/dir/file" + +(cd "$srcbase" && $RSYNC -KRlptv --rsync-path="$RSYNC" dir/file localhost:) \ + || test_fail "test 4: initial transfer for backup test failed" + +echo "backup update" >> "$srcbase/dir/file" +sleep 1 +touch "$srcbase/dir/file" + +(cd "$srcbase" && $RSYNC -KRlptv --backup --rsync-path="$RSYNC" dir/file localhost:) \ + || test_fail "test 4: update with --backup through directory symlink failed" + +diff "$srcbase/dir/file" "$HOME/real-dir/file" >/dev/null \ + || test_fail "test 4: backup update content mismatch" + +if [ ! -f "$HOME/real-dir/file~" ]; then + test_fail "test 4: backup file was not created" +fi + +###################################################################### +# Test 5: --inplace with directory symlinks +# +# Exercises the updating_basis_or_equiv check which uses +# "fnamecmp == fname". With --inplace, rsync writes directly to +# the destination file instead of a temp file. +###################################################################### + +rm -f "$HOME/real-dir/file" "$HOME/real-dir/file~" + +make_testfile "$srcbase/dir/file" + +(cd "$srcbase" && $RSYNC -KRlptv --inplace --rsync-path="$RSYNC" dir/file localhost:) \ + || test_fail "test 5: initial inplace transfer failed" + +echo "inplace update" >> "$srcbase/dir/file" +sleep 1 +touch "$srcbase/dir/file" + +(cd "$srcbase" && $RSYNC -KRlptv --inplace --rsync-path="$RSYNC" dir/file localhost:) \ + || test_fail "test 5: inplace update through directory symlink failed" + +diff "$srcbase/dir/file" "$HOME/real-dir/file" >/dev/null \ + || test_fail "test 5: inplace update content mismatch" + +###################################################################### +# Test 6: Top-level file (no dirname, no split needed) +# +# Ensures the dirname/basename split is not attempted for files +# at the top level (file->dirname is NULL). +###################################################################### + +make_testfile "$srcbase/topfile" +mkdir -p "$HOME" + +(cd "$srcbase" && $RSYNC -Rlptv --rsync-path="$RSYNC" topfile localhost:) \ + || test_fail "test 6: initial top-level transfer failed" + +echo "toplevel update" >> "$srcbase/topfile" +sleep 1 +touch "$srcbase/topfile" + +(cd "$srcbase" && $RSYNC -Rlptv --rsync-path="$RSYNC" topfile localhost:) \ + || test_fail "test 6: top-level update failed" + +diff "$srcbase/topfile" "$HOME/topfile" >/dev/null \ + || test_fail "test 6: top-level update content mismatch" + +###################################################################### +# Test 7: --partial-dir with protocol < 29 +# +# For protocol < 29, fnamecmp_type stays FNAMECMP_FNAME even when +# fnamecmp is set to partialptr. The dirname/basename split must +# NOT trigger in this case (guarded by "fnamecmp == fname"). +###################################################################### + +rm -f "$HOME/real-dir/file" +make_testfile "$srcbase/dir/file" + +(cd "$srcbase" && $RSYNC -KRlptv --protocol=28 --partial-dir=.rsync-partial \ + --rsync-path="$RSYNC" dir/file localhost:) \ + || test_fail "test 7: initial proto28 partial-dir transfer failed" + +echo "partial-dir update" >> "$srcbase/dir/file" +sleep 1 +touch "$srcbase/dir/file" + +(cd "$srcbase" && $RSYNC -KRlptv --protocol=28 --partial-dir=.rsync-partial \ + --rsync-path="$RSYNC" dir/file localhost:) \ + || test_fail "test 7: proto28 partial-dir update through dirlink failed" + +diff "$srcbase/dir/file" "$HOME/real-dir/file" >/dev/null \ + || test_fail "test 7: proto28 partial-dir update content mismatch" + +###################################################################### +# Test 8: Protocol < 29 basic directory symlink update +# +# Exercises the protocol < 29 code path and its fallback logic +# (clearing basedir on retry). +###################################################################### + +rm -f "$HOME/real-dir/file" +make_testfile "$srcbase/dir/file" + +(cd "$srcbase" && $RSYNC -KRlptv --protocol=28 \ + --rsync-path="$RSYNC" dir/file localhost:) \ + || test_fail "test 8: initial proto28 transfer failed" + +echo "proto28 update" >> "$srcbase/dir/file" +sleep 1 +touch "$srcbase/dir/file" + +(cd "$srcbase" && $RSYNC -KRlptv --protocol=28 \ + --rsync-path="$RSYNC" dir/file localhost:) \ + || test_fail "test 8: proto28 update through directory symlink failed" + +diff "$srcbase/dir/file" "$HOME/real-dir/file" >/dev/null \ + || test_fail "test 8: proto28 update content mismatch" + +# The script would have aborted on error, so getting here means we've won. +exit 0