From 209b855110299955cbfee344270cc7a6b28b8a8c Mon Sep 17 00:00:00 2001 From: seks99x Date: Thu, 3 Sep 2026 14:38:35 +0300 Subject: [PATCH 1/2] 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/2] 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("