From 209b855110299955cbfee344270cc7a6b28b8a8c Mon Sep 17 00:00:00 2001 From: seks99x Date: Thu, 3 Sep 2026 14:38:35 +0300 Subject: [PATCH 1/4] Tighten alt-dest path resolution and partial-dir state validation This introduces architectural best-practices to harden the receiver's state machine and harmonize symlink handling across the delta-basis engine, addressing protocol edge-cases reported by Fyyre (James). - receiver.c (recv_files): Added explicit validation to ensure one_inplace is only triggered when partial_dir is configured and the FNAMECMP_PARTIAL_DIR token is legitimate. - receiver.c (secure_basis_open): Enforced O_NOFOLLOW on leaf components when resolving operator-supplied paths, aligning it on operator-path behavior. Co-authored-by: Fyyre --- receiver.c | 54 +++++--- testsuite/strict-basis_test.py | 238 +++++++++++++++++++++++++++++++++ 2 files changed, 271 insertions(+), 21 deletions(-) create mode 100644 testsuite/strict-basis_test.py diff --git a/receiver.c b/receiver.c index 28d663acc..2f53183ab 100644 --- a/receiver.c +++ b/receiver.c @@ -119,24 +119,35 @@ static int secure_basis_open(const char *basedir, const char *relpath, int flags return do_open(relpath, flags, mode); } - /* A peer-supplied --partial-dir basis/staging path (operator_path_resolve set - * by recv_files) may be absolute (module_dir-prefixed on a non-chroot daemon) - * and traverse a symlink the secure_relative_open path can't confine: resolve - * it with the ownership walk, which follows a uid0/euid-owned symlink but - * refuses a foreign one AND (via abspath_excluded_by_module) refuses a target - * the module's exclude hides -- closing the partial-dir exclude bypass. */ - if (operator_path_resolve) { - char fullpath[MAXPATHLEN]; - const char *p = relpath; - if (basedir) { - if (pathjoin(fullpath, sizeof fullpath, basedir, relpath) >= sizeof fullpath) { - errno = ENAMETOOLONG; - return -1; - } + /* A path with operator_path_resolve set (e.g. an absolute --partial-dir or alt-dest + * basis) may traverse a symlink the secure_relative_open path can't confine. We + * resolve the operator path with the ownership walk, which safely follows + * subdirectory and parent components if they are trusted (uid0/euid-owned) symlinks, + * but refuses a foreign one AND (via abspath_excluded_by_module) refuses a target + * the module's exclude hides -- closing the partial-dir exclude bypass. + * However, we strictly refuse to follow a leaf symlink coming from an operator + * path by enforcing O_NOFOLLOW, aligning it on operator-path behavior. */ + if (operator_path_resolve) { + char fullpath[MAXPATHLEN]; + const char* p = relpath; + int dfd, fd, e; + const char *leaf; + if (basedir) { + if (pathjoin(fullpath, sizeof fullpath, basedir, relpath) >= sizeof fullpath) { + errno = ENAMETOOLONG; + return -1; + } p = fullpath; - } - return open_no_attacker_symlinks(p, flags, mode); - } + } + dfd = owner_walk_parent(p, &leaf); + if (dfd < 0) + return -1; + fd = openat(dfd, leaf, flags | O_NOFOLLOW, mode); + e = errno; + close(dfd); + errno = e; + return fd; + } /* The confined resolver is needed for the sanitizing daemon * (am_daemon && !am_chrooted) and for a /./ inner-module chroot @@ -1085,7 +1096,7 @@ int recv_files(int f_in, int f_out, char *local_name) * stronger confinement branch in secure_basis_open(), so only * route the alt-dest basedir read through the walk off-daemon. */ if ((basedir && !am_daemon) || fnamecmp_type == FNAMECMP_PARTIAL_DIR) - operator_path_resolve = 1; + operator_path_resolve = 1; fd1 = secure_basis_open(basedir, fnamecmp, O_RDONLY, 0); operator_path_resolve = 0; } @@ -1133,9 +1144,10 @@ int recv_files(int f_in, int f_out, char *local_name) } /* A peer's basis selector cannot enable direct output through a path - * that the confined basis open did not validate. */ - one_inplace = inplace_partial && fnamecmp_type == FNAMECMP_PARTIAL_DIR - && fd1 != -1; + * that the confined basis open did not validate. We also explicitly + * require partial_dir to be configured by the client */ + one_inplace = inplace_partial && partial_dir + && fnamecmp_type == FNAMECMP_PARTIAL_DIR && fd1 != -1; updating_basis_or_equiv = one_inplace || (inplace && (fnamecmp == fname || fnamecmp_type == FNAMECMP_BACKUP)); diff --git a/testsuite/strict-basis_test.py b/testsuite/strict-basis_test.py new file mode 100644 index 000000000..93ebda6a1 --- /dev/null +++ b/testsuite/strict-basis_test.py @@ -0,0 +1,238 @@ +#!/usr/bin/env python3 +"""TCP receiver edge-cases. + +Test 1: Ensures O_NOFOLLOW correctly blocks leaf symlinks in operator paths. +Test 2: Ensures unexpected FNAMECMP_PARTIAL_DIR tokens cannot cause an + unintended in-place overwrite. +""" +import hashlib +import os +import socket +import struct +import subprocess +from pathlib import Path + +from rsyncfns import ( + TMPDIR, TODIR, hands_setup, makepath, rmtree, rsync_argv, test_fail +) +import rsync_proto as rp + +hands_setup() + +CF_INPLACE_PARTIAL_DIR = 1 << 6 +FNAMECMP_BASIS_DIR_LOW = 0x00 +FNAMECMP_PARTIAL_DIR = 0x81 +ITEM_BASIS_TYPE_FOLLOWS = 1 << 11 +ITEM_XNAME_FOLLOWS = 1 << 12 + +TEST_DATA = b"STRICT_NOFOLLOW_REQUIRED\n" +ORIGINAL_DATA = b"ORIGINAL_DEST_BYTES_KEEP_ME\n" +MODIFIED_DATA = b"MODIFIED_INPLACE_DESPITE_BAD_CHECKSUM\n" +INVALID_MD5 = b"\x00" * 16 +MODTIME = 1_700_000_000 + +# --- Protocol Helpers --- +def drain_argv(peer): + nul_run = 0 + while nul_run < 2: + b = peer._recv_exact(1) + nul_run = nul_run + 1 if b == b"\0" else 0 + +def read_mux_bytes(peer, buf, n): + while len(buf) < n: + word = struct.unpack("> 24) - rp.MPLEX_BASE == rp.MSG_DATA: + buf.extend(payload) + out = bytes(buf[:n]) + del buf[:n] + return out + +def read_mux_int(peer, buf): return struct.unpack(" Date: Sat, 19 Sep 2026 21:24:54 +1000 Subject: [PATCH 2/4] receiver: validate partial-dir basis state --- receiver.c | 55 +++--- testsuite/strict-basis_test.py | 302 +++++++++++++-------------------- 2 files changed, 140 insertions(+), 217 deletions(-) diff --git a/receiver.c b/receiver.c index 2f53183ab..c8c8a10ef 100644 --- a/receiver.c +++ b/receiver.c @@ -119,35 +119,24 @@ static int secure_basis_open(const char *basedir, const char *relpath, int flags return do_open(relpath, flags, mode); } - /* A path with operator_path_resolve set (e.g. an absolute --partial-dir or alt-dest - * basis) may traverse a symlink the secure_relative_open path can't confine. We - * resolve the operator path with the ownership walk, which safely follows - * subdirectory and parent components if they are trusted (uid0/euid-owned) symlinks, - * but refuses a foreign one AND (via abspath_excluded_by_module) refuses a target - * the module's exclude hides -- closing the partial-dir exclude bypass. - * However, we strictly refuse to follow a leaf symlink coming from an operator - * path by enforcing O_NOFOLLOW, aligning it on operator-path behavior. */ - if (operator_path_resolve) { - char fullpath[MAXPATHLEN]; - const char* p = relpath; - int dfd, fd, e; - const char *leaf; - if (basedir) { - if (pathjoin(fullpath, sizeof fullpath, basedir, relpath) >= sizeof fullpath) { - errno = ENAMETOOLONG; - return -1; - } + /* A peer-supplied --partial-dir basis/staging path (operator_path_resolve set + * by recv_files) may be absolute (module_dir-prefixed on a non-chroot daemon) + * and traverse a symlink the secure_relative_open path can't confine: resolve + * it with the ownership walk, which follows a uid0/euid-owned symlink but + * refuses a foreign one AND (via abspath_excluded_by_module) refuses a target + * the module's exclude hides -- closing the partial-dir exclude bypass. */ + if (operator_path_resolve) { + char fullpath[MAXPATHLEN]; + const char *p = relpath; + if (basedir) { + if (pathjoin(fullpath, sizeof fullpath, basedir, relpath) >= sizeof fullpath) { + errno = ENAMETOOLONG; + return -1; + } p = fullpath; - } - dfd = owner_walk_parent(p, &leaf); - if (dfd < 0) - return -1; - fd = openat(dfd, leaf, flags | O_NOFOLLOW, mode); - e = errno; - close(dfd); - errno = e; - return fd; - } + } + return open_no_attacker_symlinks(p, flags, mode); + } /* The confined resolver is needed for the sanitizing daemon * (am_daemon && !am_chrooted) and for a /./ inner-module chroot @@ -1096,7 +1085,7 @@ int recv_files(int f_in, int f_out, char *local_name) * stronger confinement branch in secure_basis_open(), so only * route the alt-dest basedir read through the walk off-daemon. */ if ((basedir && !am_daemon) || fnamecmp_type == FNAMECMP_PARTIAL_DIR) - operator_path_resolve = 1; + operator_path_resolve = 1; fd1 = secure_basis_open(basedir, fnamecmp, O_RDONLY, 0); operator_path_resolve = 0; } @@ -1144,10 +1133,10 @@ int recv_files(int f_in, int f_out, char *local_name) } /* A peer's basis selector cannot enable direct output through a path - * that the confined basis open did not validate. We also explicitly - * require partial_dir to be configured by the client */ - one_inplace = inplace_partial && partial_dir - && fnamecmp_type == FNAMECMP_PARTIAL_DIR && fd1 != -1; + * that the confined basis open did not validate. */ + one_inplace = inplace_partial && partial_dir + && fnamecmp_type == FNAMECMP_PARTIAL_DIR + && fd1 != -1; updating_basis_or_equiv = one_inplace || (inplace && (fnamecmp == fname || fnamecmp_type == FNAMECMP_BACKUP)); diff --git a/testsuite/strict-basis_test.py b/testsuite/strict-basis_test.py index 93ebda6a1..a9a806a4e 100644 --- a/testsuite/strict-basis_test.py +++ b/testsuite/strict-basis_test.py @@ -1,238 +1,172 @@ #!/usr/bin/env python3 -"""TCP receiver edge-cases. +"""Ensure forged partial-dir basis tokens cannot enable in-place writes.""" -Test 1: Ensures O_NOFOLLOW correctly blocks leaf symlinks in operator paths. -Test 2: Ensures unexpected FNAMECMP_PARTIAL_DIR tokens cannot cause an - unintended in-place overwrite. -""" import hashlib -import os import socket import struct import subprocess -from pathlib import Path -from rsyncfns import ( - TMPDIR, TODIR, hands_setup, makepath, rmtree, rsync_argv, test_fail -) +from rsyncfns import TODIR, hands_setup, makepath, rmtree, rsync_argv, test_fail import rsync_proto as rp hands_setup() CF_INPLACE_PARTIAL_DIR = 1 << 6 -FNAMECMP_BASIS_DIR_LOW = 0x00 FNAMECMP_PARTIAL_DIR = 0x81 ITEM_BASIS_TYPE_FOLLOWS = 1 << 11 ITEM_XNAME_FOLLOWS = 1 << 12 -TEST_DATA = b"STRICT_NOFOLLOW_REQUIRED\n" ORIGINAL_DATA = b"ORIGINAL_DEST_BYTES_KEEP_ME\n" MODIFIED_DATA = b"MODIFIED_INPLACE_DESPITE_BAD_CHECKSUM\n" INVALID_MD5 = b"\x00" * 16 MODTIME = 1_700_000_000 +EXPECTED_PROTOCOL_ERROR = "error in rsync protocol data stream" + -# --- Protocol Helpers --- def drain_argv(peer): nul_run = 0 while nul_run < 2: - b = peer._recv_exact(1) - nul_run = nul_run + 1 if b == b"\0" else 0 + byte = peer._recv_exact(1) + nul_run = nul_run + 1 if byte == b"\0" else 0 + -def read_mux_bytes(peer, buf, n): - while len(buf) < n: +def read_mux_bytes(peer, buf, count): + while len(buf) < count: word = struct.unpack("> 24) - rp.MPLEX_BASE == rp.MSG_DATA: buf.extend(payload) - out = bytes(buf[:n]) - del buf[:n] - return out + result = bytes(buf[:count]) + del buf[:count] + return result + + +def read_mux_int(peer, buf): + return struct.unpack(" Date: Sun, 20 Sep 2026 15:10:17 +1000 Subject: [PATCH 3/4] fix: reject alt-dest leaf symlinks --- receiver.c | 17 ++++++- testsuite/strict-basis_test.py | 83 +++++++++++++++++++++++++++++++++- 2 files changed, 97 insertions(+), 3 deletions(-) diff --git a/receiver.c b/receiver.c index c8c8a10ef..c2cec18dd 100644 --- a/receiver.c +++ b/receiver.c @@ -124,10 +124,15 @@ static int secure_basis_open(const char *basedir, const char *relpath, int flags * and traverse a symlink the secure_relative_open path can't confine: resolve * it with the ownership walk, which follows a uid0/euid-owned symlink but * refuses a foreign one AND (via abspath_excluded_by_module) refuses a target - * the module's exclude hides -- closing the partial-dir exclude bypass. */ + * the module's exclude hides -- closing the partial-dir exclude bypass. The + * final component is opened with O_NOFOLLOW so an operator-path leaf symlink + * cannot be selected as an alternate basis. */ +#if defined AT_FDCWD && defined O_NOFOLLOW && defined O_DIRECTORY if (operator_path_resolve) { char fullpath[MAXPATHLEN]; const char *p = relpath; + const char *leaf; + int dfd, fd, saved_errno; if (basedir) { if (pathjoin(fullpath, sizeof fullpath, basedir, relpath) >= sizeof fullpath) { errno = ENAMETOOLONG; @@ -135,8 +140,16 @@ static int secure_basis_open(const char *basedir, const char *relpath, int flags } p = fullpath; } - return open_no_attacker_symlinks(p, flags, mode); + dfd = owner_walk_parent(p, &leaf); + if (dfd < 0) + return -1; + fd = openat(dfd, leaf, flags | O_NOFOLLOW, mode); + saved_errno = errno; + close(dfd); + errno = saved_errno; + return fd; } +#endif /* The confined resolver is needed for the sanitizing daemon * (am_daemon && !am_chrooted) and for a /./ inner-module chroot diff --git a/testsuite/strict-basis_test.py b/testsuite/strict-basis_test.py index a9a806a4e..100a5673b 100644 --- a/testsuite/strict-basis_test.py +++ b/testsuite/strict-basis_test.py @@ -2,16 +2,26 @@ """Ensure forged partial-dir basis tokens cannot enable in-place writes.""" import hashlib +import os import socket import struct import subprocess -from rsyncfns import TODIR, hands_setup, makepath, rmtree, rsync_argv, test_fail +from rsyncfns import ( + TODIR, + hands_setup, + makepath, + rmtree, + rsync_argv, + test_fail, + test_skipped, +) import rsync_proto as rp hands_setup() CF_INPLACE_PARTIAL_DIR = 1 << 6 +FNAMECMP_BASIS_DIR_LOW = 0x00 FNAMECMP_PARTIAL_DIR = 0x81 ITEM_BASIS_TYPE_FOLLOWS = 1 << 11 ITEM_XNAME_FOLLOWS = 1 << 12 @@ -21,6 +31,7 @@ INVALID_MD5 = b"\x00" * 16 MODTIME = 1_700_000_000 EXPECTED_PROTOCOL_ERROR = "error in rsync protocol data stream" +TEST_DATA = b"STRICT_NOFOLLOW_REQUIRED\n" def drain_argv(peer): @@ -139,6 +150,76 @@ def run_synthetic_sender(client_args, uri_path, dest_path, inject_fault): return process.returncode, stdout +def run_basis_sender(client_args, dest_path, inject_fault): + listener = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + listener.bind(("127.0.0.1", 0)) + listener.listen(1) + port = listener.getsockname()[1] + command = rsync_argv("--protocol=30", "-r", "--no-whole-file") + client_args + command += [f"rsync://127.0.0.1:{port}/mod/", f"{dest_path}/"] + process = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True) + client, _ = listener.accept() + client.settimeout(15) + peer = rp.DaemonReceiver(client) + + try: + peer.handshake(compat_flags=CF_INPLACE_PARTIAL_DIR, seed=0x12345678) + drain_argv(peer) + entry = rp.FileEntry("f", mode=rp.S_IFREG | 0o644, + length=len(TEST_DATA), modtime=MODTIME) + peer.send_data(entry.encode() + rp.end_of_flist()) + buf = bytearray() + index = drain_generator_request(peer, buf) + + body = bytearray() + body += rp.w_shortint(rp.ITEM_TRANSFER | ITEM_BASIS_TYPE_FOLLOWS) + body += rp.w_byte(FNAMECMP_BASIS_DIR_LOW) + body += rp.w_sum_head(1, len(TEST_DATA), 16, len(TEST_DATA)) + body += rp.w_int(-1) + rp.w_int(0) + body += INVALID_MD5 if inject_fault else hashlib.md5(TEST_DATA).digest() + + response = bytearray(peer.w_ndx(index)) + response += body + response += peer.w_ndx(rp.NDX_DONE) * 4 + peer.send_data(bytes(response)) + finally: + peer.drain(timeout=3) + peer.close() + listener.close() + + stdout, _ = process.communicate(timeout=10) + return process.returncode, stdout + + +print("=== Test 1: Operator-path leaf symlink rejection ===", flush=True) +rmtree(TODIR) +dest = TODIR / "dest" +linkdest = TODIR / "linkdest" +makepath(dest, linkdest) +(linkdest / "f").write_bytes(TEST_DATA) + +_, output = run_basis_sender([f"--link-dest={linkdest}", "--partial"], dest, False) +if not (dest / "f").is_file() or (dest / "f").read_bytes() != TEST_DATA: + test_fail("regular alt-dest basis control failed:\n" + output) + +rmtree(TODIR) +dest = TODIR / "dest" +linkdest = TODIR / "linkdest" +target = TODIR / "outside-target" +makepath(dest, linkdest) +target.write_bytes(TEST_DATA) +try: + os.symlink(str(target), linkdest / "f") +except (OSError, NotImplementedError): + test_skipped("leaf-symlink basis test requires symlink support") + +ret, output = run_basis_sender([f"--link-dest={linkdest}", "--partial"], dest, True) +if ret == 0 or "got a block match with no basis file" not in output: + test_fail("leaf symlink basis was not rejected:\n" + output) +if (dest / "f").is_file() and (dest / "f").read_bytes() == TEST_DATA: + test_fail("receiver followed an alt-dest leaf symlink:\n" + output) + + print("=== Test 1: Legitimate partial-dir transfer ===", flush=True) rmtree(TODIR) dest = TODIR / "dest" From ffad43e0b915dfa6432514fb6a8c682466b320dd Mon Sep 17 00:00:00 2001 From: Zen Dodd Date: Sun, 20 Sep 2026 20:17:35 +1000 Subject: [PATCH 4/4] fix: preserve basis open flags --- receiver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/receiver.c b/receiver.c index c2cec18dd..a39108704 100644 --- a/receiver.c +++ b/receiver.c @@ -143,8 +143,8 @@ static int secure_basis_open(const char *basedir, const char *relpath, int flags dfd = owner_walk_parent(p, &leaf); if (dfd < 0) return -1; - fd = openat(dfd, leaf, flags | O_NOFOLLOW, mode); - saved_errno = errno; + fd = do_open_atfd(dfd, leaf, flags, mode); + saved_errno = fd < 0 ? errno : 0; close(dfd); errno = saved_errno; return fd;