diff --git a/.github/workflows/almalinux-8-build.yml b/.github/workflows/almalinux-8-build.yml index 1a0b0e157..f3ba969f4 100644 --- a/.github/workflows/almalinux-8-build.yml +++ b/.github/workflows/almalinux-8-build.yml @@ -62,7 +62,7 @@ jobs: # crtimes-not-supported skip matches the other Linux jobs; # daemon-chroot-acl and proxy-response-line-too-long skip because # the default (secure) transport opens no listening socket. - run: RSYNC_EXPECT_SKIPPED=crtimes,daemon-access-ip,daemon-chroot-acl,proxy-response-line-too-long make check + run: RSYNC_EXPECT_SKIPPED=crtimes,daemon-access-ip,daemon-chroot-acl,proxy-response-line-too-long,recv-discard-nullderef make check - name: check (TCP daemon transport) # Second run exercising the real loopback-TCP daemon path. run: ./runtests.py --rsync-bin="$PWD/rsync" --use-tcp -j 8 diff --git a/.github/workflows/cygwin-build.yml b/.github/workflows/cygwin-build.yml index a63a3f26b..a14e9d069 100644 --- a/.github/workflows/cygwin-build.yml +++ b/.github/workflows/cygwin-build.yml @@ -46,7 +46,7 @@ jobs: # RESOLVE_BENEATH symlink-race tests. symlink-dirlink-basis also now # RUNS (the #915 non-daemon basis open uses a plain do_open, restoring # following an in-tree dir-symlink basis without RESOLVE_BENEATH). - run: bash -c 'RSYNC_EXPECT_SKIPPED=acls-default,acls-depth,acls,bare-do-open-symlink-race,chdir-symlink-race,chown,daemon-access-ip,daemon-chroot-acl,devices,dir-sgid,open-noatime,protected-regular,proxy-response-line-too-long,sender-flist-symlink-leak,simd-checksum make check' + run: bash -c 'RSYNC_EXPECT_SKIPPED=acls-default,acls-depth,acls,bare-do-open-symlink-race,chdir-symlink-race,chown,daemon-access-ip,daemon-chroot-acl,devices,dir-sgid,open-noatime,protected-regular,proxy-response-line-too-long,recv-discard-nullderef,sender-flist-symlink-leak,simd-checksum make check' - name: check (TCP daemon transport) # Second run with daemon tests over a real loopback rsyncd; the default # 'make check' above uses the secure stdio-pipe transport. diff --git a/.github/workflows/macos-build.yml b/.github/workflows/macos-build.yml index 067671ce0..971389fb6 100644 --- a/.github/workflows/macos-build.yml +++ b/.github/workflows/macos-build.yml @@ -44,7 +44,7 @@ jobs: # chown-fake / devices-fake / xattrs / xattrs-hlink now RUN on macOS # (rsyncfns.py drives xattrs via the `xattr` command), verified on a # real macOS host, so they're no longer in the skip set. - run: sudo RSYNC_EXPECT_SKIPPED=acls-default,acls-depth,chmod-temp-dir,daemon-access-ip,daemon-chroot-acl,dir-sgid,open-noatime,preallocate,protected-regular,proxy-response-line-too-long,simd-checksum,sparse make check + run: sudo RSYNC_EXPECT_SKIPPED=acls-default,acls-depth,chmod-temp-dir,daemon-access-ip,daemon-chroot-acl,dir-sgid,open-noatime,preallocate,protected-regular,proxy-response-line-too-long,recv-discard-nullderef,simd-checksum,sparse make check - name: check (TCP daemon transport) # Second run with daemon tests over a real loopback rsyncd; the default # 'make check' above uses the secure stdio-pipe transport. diff --git a/.github/workflows/ubuntu-22.04-build.yml b/.github/workflows/ubuntu-22.04-build.yml index 392b5e738..af14ce4bb 100644 --- a/.github/workflows/ubuntu-22.04-build.yml +++ b/.github/workflows/ubuntu-22.04-build.yml @@ -39,11 +39,11 @@ jobs: - name: info run: rsync --version - name: check - run: sudo RSYNC_EXPECT_SKIPPED=crtimes,daemon-access-ip,daemon-chroot-acl,proxy-response-line-too-long make check + run: sudo RSYNC_EXPECT_SKIPPED=crtimes,daemon-access-ip,daemon-chroot-acl,proxy-response-line-too-long,recv-discard-nullderef make check - name: check30 - run: sudo RSYNC_EXPECT_SKIPPED=crtimes,daemon-access-ip,daemon-chroot-acl,proxy-response-line-too-long make check30 + run: sudo RSYNC_EXPECT_SKIPPED=crtimes,daemon-access-ip,daemon-chroot-acl,proxy-response-line-too-long,recv-discard-nullderef make check30 - name: check29 - run: sudo RSYNC_EXPECT_SKIPPED=crtimes,daemon-access-ip,daemon-chroot-acl,proxy-response-line-too-long make check29 + run: sudo RSYNC_EXPECT_SKIPPED=crtimes,daemon-access-ip,daemon-chroot-acl,proxy-response-line-too-long,recv-discard-nullderef make check29 - name: check (TCP daemon transport) # Second run with daemon tests over a real loopback rsyncd; the default # 'make check' above uses the secure stdio-pipe transport. diff --git a/.github/workflows/ubuntu-build.yml b/.github/workflows/ubuntu-build.yml index c142d693f..72f19e3fd 100644 --- a/.github/workflows/ubuntu-build.yml +++ b/.github/workflows/ubuntu-build.yml @@ -63,11 +63,11 @@ jobs: - name: info run: rsync --version - name: check - run: sudo RSYNC_EXPECT_SKIPPED=crtimes,daemon-access-ip,daemon-chroot-acl,proxy-response-line-too-long make check + run: sudo RSYNC_EXPECT_SKIPPED=crtimes,daemon-access-ip,daemon-chroot-acl,proxy-response-line-too-long,recv-discard-nullderef make check - name: check30 - run: sudo RSYNC_EXPECT_SKIPPED=crtimes,daemon-access-ip,daemon-chroot-acl,proxy-response-line-too-long make check30 + run: sudo RSYNC_EXPECT_SKIPPED=crtimes,daemon-access-ip,daemon-chroot-acl,proxy-response-line-too-long,recv-discard-nullderef make check30 - name: check29 - run: sudo RSYNC_EXPECT_SKIPPED=crtimes,daemon-access-ip,daemon-chroot-acl,proxy-response-line-too-long make check29 + run: sudo RSYNC_EXPECT_SKIPPED=crtimes,daemon-access-ip,daemon-chroot-acl,proxy-response-line-too-long,recv-discard-nullderef make check29 - name: check (TCP daemon transport) # Second run with daemon tests over a real loopback rsyncd. The default # 'make check' above uses the secure stdio-pipe transport (no listening diff --git a/receiver.c b/receiver.c index 84bb151aa..2b263cea0 100644 --- a/receiver.c +++ b/receiver.c @@ -423,16 +423,32 @@ static int receive_data(int f_in, char *fname_r, int fd_r, OFF_T size_r, stats.matched_data += len; - /* A block match can only be honored if we actually mapped the - * basis. If we didn't (basis open failed), the sender should - * never have been told a basis existed -- treat it as a protocol - * inconsistency rather than silently omitting these bytes from - * the verification checksum (which yields a spurious failure) or - * leaving a hole in the output. */ + /* A block match with no mapped basis is a protocol inconsistency + * ONLY when we are actually producing output (fd != -1): the + * generator told the sender a basis existed but the receiver could + * not open it, so honoring the match would silently omit these + * bytes from the verification checksum (a spurious failure) or + * leave a hole in the output. Fail cleanly in that case. + * + * On the DISCARD path (fd == -1, fname == NULL) there is no output + * and no verification: discard_receive_data() deliberately drains a + * delta the receiver never intends to write (basis fstat failed, + * basis is a directory, output open failed, batch skip, ...). The + * sender does not know the data is being discarded and streams an + * ordinary delta, so a match token here is NORMAL protocol, not + * malformed. Absorb it benignly (advance the offset and continue), + * as the pre-existing "if (mapbuf)" guards did before this check was + * added in 31fbb17d -- erroring would wrongly break legitimate + * transfers, and full_fname(fname) with fname==NULL would + * dereference NULL (a receiver crash on a normal transfer). */ if (!mapbuf) { - rprintf(FERROR, "got a block match with no basis file for %s [%s]\n", - full_fname(fname), who_am_i()); - exit_cleanup(RERR_PROTOCOL); + if (fd != -1) { + rprintf(FERROR, "got a block match with no basis file for %s [%s]\n", + full_fname(fname), who_am_i()); + exit_cleanup(RERR_PROTOCOL); + } + offset += len; + continue; } if (DEBUG_GTE(DELTASUM, 3)) { diff --git a/testsuite/recv-discard-nullderef_test.py b/testsuite/recv-discard-nullderef_test.py new file mode 100755 index 000000000..e130203f8 --- /dev/null +++ b/testsuite/recv-discard-nullderef_test.py @@ -0,0 +1,126 @@ +#!/usr/bin/env python3 +# Regression test for a receiver NULL-deref on the delta DISCARD path. +# +# In receiver.c receive_data(), a block-MATCH token that arrives while the +# receiver is DISCARDING a file (discard_receive_data() -> receive_data() with +# fname==NULL, fd==-1, hence mapbuf==NULL) reached +# rprintf(FERROR, "...%s...", full_fname(fname), ...) +# with fname==NULL. full_fname() dereferences its argument unconditionally +# (util1.c: `if (*fn == '/')`), so the receiver SIGSEGVs. The faulty error +# branch was added in 31fbb17d ("receiver: fix absolute --partial-dir delta +# resume"); the fix discriminates on fd (not mapbuf) and, on the discard path +# (fd==-1), absorbs the matched bytes benignly instead of erroring. +# +# This is a NORMAL-operation crash, not adversarial: a stock cooperating sender +# triggers it. The generator sends real block sums (basis readable, delta mode); +# the receiver then has to discard because its output mkstemp() fails -- here +# because the destination directory is not writable. A block MATCH against the +# shared leading block reaches the discard path and crashes the pre-fix binary. +# +# We drive a real sender<->receiver pair (client sender -> daemon receiver) so +# the receiver actually takes the recv_files discard path; a local `rsync a b` +# does not. In the default (pipe) daemon transport both ends are the binary +# under test. +# +# Skipped (exit 77) when running as root (root bypasses DAC), or when the +# directory mode is not enforced (e.g. a non-root process holding +# CAP_DAC_OVERRIDE in an unprivileged container): in both cases the receiver's +# mkstemp() would succeed despite chmod 0555, the discard path would not be +# taken, and the test would silently pass against a buggy binary. The +# post-chmod writability probe converts that silent false-pass into an honest +# skip and subsumes the root check. + +import os +import shlex +import subprocess +import tempfile + +from rsyncfns import ( + SCRATCHDIR, RSYNC, TMPDIR, + get_testuid, get_rootuid, makepath, start_test_daemon, write_daemon_conf, + test_fail, test_skipped, +) + +DAEMON_PORT = 12895 + +if get_testuid() == get_rootuid(): + test_skipped("root bypasses DAC: the unwritable dest dir wouldn't make " + "the receiver's mkstemp fail, so the discard path (and the " + "bug) is never reached") + +os.chdir(TMPDIR) + +MODDIR = SCRATCHDIR / 'recvdiscard-mod' # daemon module root (writable) +BASISDIR = MODDIR / 'd' # made read-only -> mkstemp fails +SRCDIR_ = SCRATCHDIR / 'recvdiscard-src' # client source tree +makepath(MODDIR, BASISDIR, SRCDIR_) + +# Basis and source share a leading block (2000 'A's) so the generator emits +# real sums and the receiver gets a block MATCH; the tails differ and the +# source is larger so a delta (not a no-op) is sent. +basis = BASISDIR / 'f' +basis.write_bytes(b'A' * 2000 + b'C' * 1000) +src = SRCDIR_ / 'f' +src.write_bytes(b'A' * 2000 + b'B' * 3000) + +# A read/write daemon module rooted at MODDIR. +conf = write_daemon_conf([('recvdiscard', {'path': str(MODDIR), + 'read only': 'no'})]) +url = start_test_daemon(conf, DAEMON_PORT, rsync_cmd=RSYNC) + +# Make the destination directory unwritable so the receiver's output mkstemp() +# fails and it falls back to discarding the delta stream. Restore in finally so +# the per-test scratch tree can be cleaned up. +os.chmod(BASISDIR, 0o555) + +# Probe that the chmod actually denies writes for *this* process. A non-root +# user holding CAP_DAC_OVERRIDE bypasses the directory write bit, so mkstemp +# would succeed in the daemon receiver too, the discard path would never be +# taken, and the test would silently pass on a buggy binary. Better to skip +# explicitly. (Root takes this path too: its probe succeeds → skip, which +# subsumes the uid==0 check.) +try: + _fd, _probe = tempfile.mkstemp(dir=BASISDIR) + os.close(_fd) + os.unlink(_probe) + os.chmod(BASISDIR, 0o755) + test_skipped("destination dir is writable despite chmod 0555 " + "(CAP_DAC_OVERRIDE?); cannot force the receiver discard path") +except OSError: + pass # EACCES -- good, the precondition is enforced + +try: + argv = shlex.split(RSYNC) + [ + '--no-whole-file', '-a', + str(src), f'{url}recvdiscard/d/f', + ] + print('Running:', ' '.join(argv)) + proc = subprocess.run(argv, stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, text=True) + print(proc.stdout, end='') +finally: + os.chmod(BASISDIR, 0o755) + +rc = proc.returncode + +# A receiver SIGSEGV manifests to the client as a protocol error (the daemon's +# receiver child crashes mid-stream and the connection drops): exit code 12. +# With the fix the receiver drains the delta and, because the forced-unwritable +# destination leaves the file untransferred, the run reports the benign "some +# files were not transferred" -- exit code 23. +# +# 23 is the ONLY non-crash outcome here: the writability probe above guarantees +# the receiver's mkstemp() fails, so the file is always discarded. An exit 0 +# would mean the file actually transferred -- the discard path was NOT exercised +# and the run proves nothing -- so require exactly 23 (and call out 12 as the +# pre-fix crash). +if rc == 12: + test_fail(f"receiver crashed on the discard path (rsync exited {rc}: " + "error in rsync protocol data stream -- the receiver child " + "SIGSEGV'd in full_fname(NULL))") +if rc != 23: + test_fail(f"expected rsync exit 23 (the forced discard leaves the file " + f"untransferred); got {rc} -- the discard path was not exercised, " + "so this run validates nothing (12 would be the pre-fix crash)") + +print(f"OK: receiver discarded the delta without crashing (rsync exit {rc})")