From: Andrew Tridgell Date: Sun, 24 May 2026 22:08:42 +0000 (+1000) Subject: testsuite: add positive controls to the symlink-race security tests X-Git-Url: http://git.ipfire.org/gitweb/index.cgi?a=commitdiff_plain;h=f196279c2916cabebea9aad1354fed78ffb127cf;p=thirdparty%2Frsync.git testsuite: add positive controls to the symlink-race security tests The symlink-race tests only asserted that an outside sentinel was unchanged or unlisted while ignoring rsync's exit status, so an attack transfer/listing that failed before reaching the vulnerable receiver/sender path would pass without the security property ever being exercised. Add a positive control to each -- an ordinary in-module write (bare-do-open, chdir) or an in-module listing (sender-flist-leak) that must succeed -- so a globally broken/refusing daemon can no longer make the sentinel checks vacuous, and assert the attack run did not die from a signal. clean-fname-underflow now also enforces a non-zero exit: clean_fname() collapses "a/../test" to "test", whose merge file is absent, so rsync must reject it; accepting it (rc 0) would mean the crafted name was mis-collapsed. Co-Authored-By: Claude Opus 4.7 (1M context) --- diff --git a/testsuite/bare-do-open-symlink-race_test.py b/testsuite/bare-do-open-symlink-race_test.py index deff0731..6ad70a40 100644 --- a/testsuite/bare-do-open-symlink-race_test.py +++ b/testsuite/bare-do-open-symlink-race_test.py @@ -92,10 +92,31 @@ daemon_url = start_test_daemon(conf, DAEMON_PORT).rstrip('/') def run_attack(args): - subprocess.run( + """Run the (expected-to-be-contained) attack transfer and return its exit + code; output is discarded since the security assertion is on the filesystem.""" + return subprocess.run( rsync_argv(*args), stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, - ) + ).returncode + + +def positive_control(): + """Confirm the upload module accepts an ordinary write. Without this, every + scenario below would also pass if the daemon refused (or failed) before the + vulnerable code ran -- the sentinel-unchanged check alone can't tell a + securely-contained attack from one that never reached the receiver path.""" + for d in (mod, src): + rmtree(d) + d.mkdir(parents=True) + (src / 'legit.txt').write_text("LEGIT_IN_MODULE\n") + rc = run_attack(['-t', f'{src}/legit.txt', f'{daemon_url}/upload/legit.txt']) + if rc != 0 or not (mod / 'legit.txt').is_file() \ + or (mod / 'legit.txt').read_text() != "LEGIT_IN_MODULE\n": + test_fail(f"positive control: upload module did not accept a normal " + f"write (rc={rc}); attack scenarios would be vacuous") + + +positive_control() # Scenario 3b: --inplace --backup --backup-dir=cd @@ -105,10 +126,12 @@ os.chmod(mod / 'target.txt', 0o666) (src / 'target.txt').write_text("NEW_DATA_FROM_SENDER\n") os.chmod(src / 'target.txt', 0o644) -run_attack([ +rc = run_attack([ '--inplace', '--backup', '--backup-dir=cd', f'{src}/target.txt', f'{daemon_url}/upload/target.txt', ]) +if rc >= 128: + test_fail(f"3b inplace+backup-dir=cd: rsync died from a signal (rc={rc})") verify_outside_unchanged("3b inplace+backup-dir=cd") @@ -117,7 +140,9 @@ setup() (src / 'cd').mkdir() os.symlink('/etc/passwd', src / 'cd' / 'sym') -run_attack(['-rl', f'{src}/', f'{daemon_url}/upload_fake/']) +rc = run_attack(['-rl', f'{src}/', f'{daemon_url}/upload_fake/']) +if rc >= 128: + test_fail(f"3c-symlink: rsync died from a signal (rc={rc})") verify_outside_unchanged_or_absent("3c-symlink fake-super symlink push", "sym") @@ -132,5 +157,7 @@ except OSError: if not stat.S_ISFIFO((src / 'cd' / 'fifo').stat().st_mode): test_skipped("mkfifo unavailable; cannot exercise 3c-mknod") -run_attack(['-rD', f'{src}/', f'{daemon_url}/upload_fake/']) +rc = run_attack(['-rD', f'{src}/', f'{daemon_url}/upload_fake/']) +if rc >= 128: + test_fail(f"3c-mknod: rsync died from a signal (rc={rc})") verify_outside_unchanged_or_absent("3c-mknod fake-super FIFO push", "fifo") diff --git a/testsuite/chdir-symlink-race_test.py b/testsuite/chdir-symlink-race_test.py index 80b314c0..623793c9 100644 --- a/testsuite/chdir-symlink-race_test.py +++ b/testsuite/chdir-symlink-race_test.py @@ -87,13 +87,41 @@ url = start_test_daemon(conf, DAEMON_PORT) def run_attack(label: str, *args) -> None: reset_outside() - subprocess.run( + rc = subprocess.run( rsync_argv(*args), stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, - ) + ).returncode + if rc >= 128: + test_fail(f"{label}: rsync died from a signal (rc={rc})") verify_unchanged(label) +def positive_control() -> None: + """Confirm the receiver writes into an ordinary in-module subdirectory, so + the symlink-escape scenarios below genuinely exercise the chdir path rather + than passing because the daemon refused (or failed) before reaching it.""" + real = mod / 'realdir' + rmtree(real) + real.mkdir() + # When this test runs as root the daemon serves as 'nobody' (the module + # sets no uid), so make the control target world-writable; push a single + # file with no attribute preservation so the write never needs to own/chmod + # the dir -- it should land purely on the receiver's normal write path. + os.chmod(real, 0o777) + rc = subprocess.run( + rsync_argv(f'{src}/subdir/target.txt', f'{url}upload/realdir/'), + stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, + ).returncode + landed = real / 'target.txt' + if rc != 0 or not landed.is_file() \ + or not filecmp.cmp(landed, src / 'subdir' / 'target.txt', shallow=False): + test_fail(f"positive control: receiver did not write into an ordinary " + f"in-module subdir (rc={rc}); attack scenarios would be vacuous") + + +positive_control() + + # 1. Single file with --size-only -- receiver normally skips basis open and # goes straight to chmod; only the chdir-escape blocks it. run_attack("single-file --size-only", diff --git a/testsuite/clean-fname-underflow_test.py b/testsuite/clean-fname-underflow_test.py index 4b30155c..638bf3ae 100644 --- a/testsuite/clean-fname-underflow_test.py +++ b/testsuite/clean-fname-underflow_test.py @@ -3,8 +3,10 @@ # # Ensure clean_fname() does not read-before-buffer when collapsing "..". # Exercises the --server path where a crafted merge filename hits -# clean_fname(); a non-zero exit is expected (the input is bogus), but -# the test fails if rsync dies from a signal (status >= 128). +# clean_fname(); the test fails if rsync dies from a signal (status >= 128) +# AND if rsync exits 0 -- the bogus input must be rejected (clean_fname +# collapses "a/../test" to "test", whose merge file does not exist, so rsync +# errors out non-zero). Accepting it would mean the name was mis-collapsed. import os import shlex @@ -29,5 +31,8 @@ proc = subprocess.run( if proc.returncode >= 128: test_fail(f"rsync exited due to a signal (status={proc.returncode})") +if proc.returncode == 0: + test_fail("rsync accepted the bogus 'a/../test' merge filter (expected a " + "non-zero rejection); clean_fname() may have mis-collapsed it") -print("OK: clean_fname() handled 'a/../test' without crashing") +print("OK: clean_fname() handled 'a/../test' without crashing, and rejected it") diff --git a/testsuite/sender-flist-symlink-leak_test.py b/testsuite/sender-flist-symlink-leak_test.py index 67fc2f11..97c1f602 100644 --- a/testsuite/sender-flist-symlink-leak_test.py +++ b/testsuite/sender-flist-symlink-leak_test.py @@ -49,6 +49,11 @@ os.chmod(outside / 'leak_marker.txt', 0o644) os.symlink(str(outside), mod / 'cd') +# A legitimate in-module file, used as a positive control so the leak check +# below can't pass simply because the daemon's listing machinery is broken. +(mod / 'realdir').mkdir() +(mod / 'realdir' / 'in_module.txt').write_text("INSIDE_THE_MODULE\n") + my_uid = get_testuid() root_uid = get_rootuid() root_gid = get_rootgid() @@ -71,10 +76,24 @@ log file = {SCRATCHDIR}/rsyncd.log url = start_test_daemon(conf, DAEMON_PORT) +# Positive control: a normal recursive listing of an in-module path must +# enumerate the in-module file. If this fails, the daemon's flist generation is +# broken and the leak check below would be vacuously satisfied. +ctl = subprocess.run( + rsync_argv('-nrv', f'{url}upload/realdir/', f'{SCRATCHDIR}/dst/'), + capture_output=True, text=True, +) +if ctl.returncode != 0 or 'in_module.txt' not in ctl.stdout: + test_fail("positive control: listing an in-module path did not enumerate " + f"in_module.txt (rc={ctl.returncode}); leak check would be vacuous" + f"\n{ctl.stdout}{ctl.stderr}") + proc = subprocess.run( rsync_argv('-nrv', f'{url}upload/cd/', f'{SCRATCHDIR}/dst/'), capture_output=True, text=True, ) +if proc.returncode >= 128: + test_fail(f"leak pull: rsync died from a signal (rc={proc.returncode})") listfile.write_text(proc.stdout + proc.stderr) if 'leak_marker.txt' in listfile.read_text():