From: Andrew Tridgell Date: Wed, 3 Jun 2026 10:48:10 +0000 (+1000) Subject: syscall/receiver: honour a relative alt-basis dir on a daemon receiver (#915) X-Git-Url: http://git.ipfire.org/gitweb/index.cgi?a=commitdiff_plain;h=5972ebdaf83b7e4ed2dbdbd696ba4b35318f5932;p=thirdparty%2Frsync.git syscall/receiver: honour a relative alt-basis dir on a daemon receiver (#915) The symlink-race hardening routed the receiver's basis open through secure_relative_open(), which rejects any '..' -- so a sibling --link-dest=../01 on a use-chroot=no daemon was silently ignored and every file re-transferred (#915/#928, a regression from 3.4.1). Narrow the confinement to the sanitizing daemon (am_daemon && !am_chrooted) and re-anchor it at the module root, the real trust boundary: secure_relative_open() prefixes the cwd's module-relative path (from rsync's logical curr_dir[], a guaranteed lexical prefix of module_dir) and resolves beneath module_dir, so RESOLVE_BENEATH permits an in-module '..' climb while still rejecting one that escapes the module. secure_basis_open() opens with a bare do_open() in the non-sanitizing cases. t_stub.c gains weak curr_dir[]/curr_dir_len for the helpers (via #pragma weak on non-GNU compilers, where rsync.h erases __attribute__). Two tests: link-dest-relative-basis asserts the in-module '..' is honoured; link-dest-module-escape asserts a --link-dest=../../OUTSIDE climb that leaves the module is refused (not hard-linked to an outside file). See upstream PR #930. --- diff --git a/.github/workflows/cygwin-build.yml b/.github/workflows/cygwin-build.yml index 87fbe901..a63a3f26 100644 --- a/.github/workflows/cygwin-build.yml +++ b/.github/workflows/cygwin-build.yml @@ -43,8 +43,10 @@ jobs: # (rsyncfns.py drives xattrs via getfattr/setfattr from the `attr` # package installed above), verified on a real Cygwin host. The real # chown/devices tests still skip (need root/mknod), as do the - # RESOLVE_BENEATH symlink-race tests. - 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,symlink-dirlink-basis make check' + # 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' - 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/receiver.c b/receiver.c index 7d429fe8..cb797841 100644 --- a/receiver.c +++ b/receiver.c @@ -99,6 +99,27 @@ static int updating_basis_or_equiv; * Anything else is a straight pass-through that preserves the strict contract. */ static int secure_basis_open(const char *basedir, const char *relpath, int flags, mode_t mode) { + extern int am_daemon, am_chrooted; + + /* The confined resolver is only needed for the sanitizing daemon + * (am_daemon && !am_chrooted, i.e. use_secure_symlinks). Local / + * remote-shell mode has no module boundary, and "use chroot = yes" makes + * the kernel root the boundary, so there an alt-dest basis like + * --link-dest=../01 must resolve against the cwd as a bare open did before + * the hardening (confining it would reject the legitimate sibling "..", + * #915). */ + if (!am_daemon || am_chrooted) { + if (basedir) { + char fullpath[MAXPATHLEN]; + if (pathjoin(fullpath, sizeof fullpath, basedir, relpath) >= sizeof fullpath) { + errno = ENAMETOOLONG; + return -1; + } + return do_open(fullpath, flags, mode); + } + return do_open(relpath, flags, mode); + } + if (!basedir && relpath && *relpath == '/') { const char *slash = strrchr(relpath, '/'); const char *leaf = slash + 1; @@ -859,7 +880,7 @@ int recv_files(int f_in, int f_out, char *local_name) basedir = basis_dir[0]; fnamecmp = fname; fnamecmp_type = FNAMECMP_BASIS_DIR_LOW; - fd1 = secure_relative_open(basedir, fnamecmp, O_RDONLY, 0); + fd1 = secure_basis_open(basedir, fnamecmp, O_RDONLY, 0); } } diff --git a/syscall.c b/syscall.c index 0748d998..b402ebf8 100644 --- a/syscall.c +++ b/syscall.c @@ -1780,13 +1780,68 @@ static int secure_relative_open_resolve_beneath(const char *basedir, const char } #endif +/* The logical current directory (maintained by change_dir() in util1.c). + * Defined here -- rather than in util1.c -- so the test helpers that link + * syscall.o but not util1.o (tls, trimslash) get the definition without a + * weak-symbol fallback, which is not portable to PE/COFF targets (Cygwin). */ +char curr_dir[MAXPATHLEN]; +unsigned int curr_dir_len; + int secure_relative_open(const char *basedir, const char *relpath, int flags, mode_t mode) { + extern int am_daemon, am_chrooted; + extern char *module_dir; + extern unsigned int module_dirlen; + char modrel_buf[MAXPATHLEN]; + int reanchored = 0; + if (!relpath || relpath[0] == '/') { // must be a relative path errno = EINVAL; return -1; } + + /* Sanitizing daemon only (am_daemon && !am_chrooted). Here we have chdir'd + * into a sub-dir of the module (the transfer destination), so a relative + * alt-dest like "../01" may legitimately climb to a sibling that is still + * inside the module (#915). Confining beneath the cwd would reject that + * climb. Re-anchor at the module root -- the real trust boundary -- by + * prefixing the cwd's module-relative path (from rsync's logical curr_dir[], + * a guaranteed lexical prefix of module_dir, unlike getcwd()) and resolving + * beneath module_dir; RESOLVE_BENEATH then allows in-module climbs and still + * rejects escapes. Only for paths that contain "..". module_dirlen is 0 for + * a `path = /` module (clientserver.c), so we gate on module_dir, not its + * length, to cover that case too -- the prefix check below treats + * module_dirlen 0 as "module root is /". */ + if (am_daemon && !am_chrooted + && module_dir && module_dir[0] == '/' + && (basedir == NULL || basedir[0] != '/') + && (path_has_dotdot_component(relpath) + || (basedir && path_has_dotdot_component(basedir)))) { + const char *p; + int n; + if (curr_dir_len >= module_dirlen + && strncmp(curr_dir, module_dir, module_dirlen) == 0 + && (curr_dir[module_dirlen] == '\0' || curr_dir[module_dirlen] == '/')) { + for (p = curr_dir + module_dirlen; *p == '/'; p++) {} + if (basedir) + n = snprintf(modrel_buf, sizeof modrel_buf, "%s%s%s/%s", + p, *p ? "/" : "", basedir, relpath); + else + n = snprintf(modrel_buf, sizeof modrel_buf, "%s%s%s", + p, *p ? "/" : "", relpath); + if (n < 0 || n >= (int)sizeof modrel_buf) { + errno = ENAMETOOLONG; + return -1; + } + basedir = module_dir; /* absolute, operator-trusted anchor */ + relpath = modrel_buf; + reanchored = 1; + } + /* else: cwd not under module root as expected -- fall through to the + * front-door rejection below (fail safe). */ + } + /* Reject any path with a literal ".." component (bare "..", * "../foo", "foo/..", "foo/../bar", "subdir/.."). The previous * substring-based check caught only "../" prefix and "/../" @@ -1795,14 +1850,19 @@ int secure_relative_open(const char *basedir, const char *relpath, int flags, mo * and pre-5.6 Linux. RESOLVE_BENEATH on Linux/FreeBSD/macOS * catches some of these in-kernel with EXDEV, but the front * door must reject them consistently with EINVAL across all - * platforms so callers can rely on the validation. */ - if (path_has_dotdot_component(relpath)) { - errno = EINVAL; - return -1; - } - if (basedir && basedir[0] != '/' && path_has_dotdot_component(basedir)) { - errno = EINVAL; - return -1; + * platforms so callers can rely on the validation. Skipped for a + * re-anchored path: its ".." is deliberate, stays within the module, + * and is adjudicated by RESOLVE_BENEATH below (the portable fallback + * re-rejects it -- see there). */ + if (!reanchored) { + if (path_has_dotdot_component(relpath)) { + errno = EINVAL; + return -1; + } + if (basedir && basedir[0] != '/' && path_has_dotdot_component(basedir)) { + errno = EINVAL; + return -1; + } } #if defined(__linux__) && defined(HAVE_OPENAT2) @@ -1821,6 +1881,21 @@ int secure_relative_open(const char *basedir, const char *relpath, int flags, mo return secure_relative_open_resolve_beneath(basedir, relpath, flags, mode); #endif + /* Portable fallback only (no kernel RESOLVE_BENEATH): the per-component + * O_NOFOLLOW walk below can't adjudicate ".." safely, so reject it here -- + * even for a re-anchored path. This re-breaks --link-dest=../01 on + * openat2/O_RESOLVE_BENEATH-less platforms (NetBSD/OpenBSD/Solaris/Cygwin/ + * pre-5.6 Linux), trading function for safety; on the kernel paths above + * RESOLVE_BENEATH already allowed the in-module climb. */ + if (path_has_dotdot_component(relpath)) { + errno = EINVAL; + return -1; + } + if (basedir && basedir[0] != '/' && path_has_dotdot_component(basedir)) { + errno = EINVAL; + return -1; + } + #if !defined(O_NOFOLLOW) || !defined(O_DIRECTORY) || !defined(AT_FDCWD) // really old system, all we can do is live with the risks if (!basedir) { diff --git a/t_stub.c b/t_stub.c index d6c4c133..2b99e74d 100644 --- a/t_stub.c +++ b/t_stub.c @@ -45,6 +45,8 @@ size_t max_alloc = (size_t)-1; /* test helpers are not memory-constrained; * hits at its first my_strdup() call. */ char *partial_dir; char *module_dir; +/* curr_dir[]/curr_dir_len (read by secure_relative_open) are defined in + * syscall.c, which every helper links -- no stub needed here. */ filter_rule_list daemon_filter_list; void rprintf(UNUSED(enum logcode code), const char *format, ...) diff --git a/testsuite/link-dest-module-escape_test.py b/testsuite/link-dest-module-escape_test.py new file mode 100644 index 00000000..dfa4ad0b --- /dev/null +++ b/testsuite/link-dest-module-escape_test.py @@ -0,0 +1,67 @@ +#!/usr/bin/env python3 +# Security guard for the #915 re-anchor: a daemon receiver must NOT honour an +# alt-basis dir whose `..` climbs OUT of the module. +# +# Honouring a relative --link-dest=../01 again (#915) deliberately re-permits an +# in-module `..` climb (dest 00 -> sibling basis 01). This test pins the other +# side of that boundary: a client-supplied --link-dest=../../OUTSIDE that points +# at a file OUTSIDE the module root must be refused, so the basis is never used +# and the dest file is re-transferred rather than hard-linked to the outside +# file (which would be an info-leak / cross-module hard-link). +# +# The re-anchor confines resolution beneath module_dir with RESOLVE_BENEATH, so +# the escaping climb is rejected in-kernel; on platforms without +# openat2/O_RESOLVE_BENEATH the portable resolver rejects the `..` outright. +# Either way the escape is blocked, so this test must PASS on every platform. +# Runs at any uid. + +import shutil +import subprocess + +from rsyncfns import ( + SCRATCHDIR, make_data_file, makepath, rmtree, rsync_argv, start_test_daemon, + test_fail, write_daemon_conf, +) + +DAEMON_PORT = 12916 +DATA_SIZE = 40000 + +mod = SCRATCHDIR / 'escmod' # daemon module root (holds dest 00) +src = SCRATCHDIR / 'escsrc' +outside = SCRATCHDIR / 'OUTSIDE' # sibling of the module root -- OUTSIDE it +for d in (mod, src, outside): + rmtree(d) +makepath(mod / '00', src, outside) + +# Source file, plus a byte-identical secret OUTSIDE the module with the same +# name/size/mtime (so a followed basis would quick-check as a match). +make_data_file(src / 'f.dat', DATA_SIZE) +shutil.copy2(src / 'f.dat', outside / 'f.dat') + +conf = write_daemon_conf([ + ('bak', {'path': str(mod), 'read only': 'no'}), +]) +url = start_test_daemon(conf, DAEMON_PORT) + +# Dest is bak/00 (cwd = module/00). --link-dest=../../OUTSIDE climbs +# module/00 -> module -> SCRATCHDIR/OUTSIDE, i.e. out of the module. +proc = subprocess.run( + rsync_argv('-a', '--link-dest=../../OUTSIDE', f'{src}/', f'{url}bak/00/'), + stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True) +out = proc.stdout or '' +if proc.returncode not in (0, 23): # 23: a basis rejection is non-fatal here + test_fail(f"escape push failed unexpectedly (rc={proc.returncode}):\n{out}") + +dest = mod / '00' / 'f.dat' +secret = outside / 'f.dat' +if not dest.is_file(): + test_fail(f"destination file missing ({dest})") + +ds, ss = dest.stat(), secret.stat() +if (ds.st_dev, ds.st_ino) == (ss.st_dev, ss.st_ino): + test_fail( + "MODULE ESCAPE: the dest was hard-linked to a file OUTSIDE the module " + f"via --link-dest=../../OUTSIDE -- the confined resolver let a `..` " + f"climb escape the module root.\n{out}") +# Escape blocked: the basis was refused, so the file was re-transferred and the +# dest is its own inode, not the outside secret's. diff --git a/testsuite/link-dest-pathroot_test.py b/testsuite/link-dest-pathroot_test.py new file mode 100644 index 00000000..fa63fcf1 --- /dev/null +++ b/testsuite/link-dest-pathroot_test.py @@ -0,0 +1,63 @@ +#!/usr/bin/env python3 +# Functional regression: a relative --link-dest=../sibling against a daemon +# module with `path = /` (the intersection of #897 and #915). +# +# #915 re-anchors the receiver's basis open at the module root so an in-module +# "../01" climb is honoured. The gate keyed on a nonzero module_dirlen, but a +# `path = /` module has module_dirlen == 0 (clientserver.c), so the re-anchor +# was skipped there and --link-dest=../01 was silently ignored (every file +# re-transferred) even though plain #915 modules were fixed. +# +# Like link-dest-relative-basis this XFAILs on platforms without +# openat2/O_RESOLVE_BENEATH (the portable resolver rejects the '..' for safety); +# it flips to PASS where the kernel can adjudicate the in-module climb. Runs at +# any uid. + +import shutil +import subprocess + +from rsyncfns import ( + SCRATCHDIR, make_data_file, makepath, rmtree, rsync_argv, start_test_daemon, + test_fail, test_xfail, write_daemon_conf, +) + +DAEMON_PORT = 12931 +DATA_SIZE = 40000 + +# dest 00 and basis 01 live side by side under `base`; the module is rooted at +# "/", so the served subtree is addressed by its absolute path minus the leading +# slash, and --link-dest=../01 climbs dest 00 -> sibling 01 (both inside /). +base = SCRATCHDIR / 'bakroot' +src = SCRATCHDIR / 'srcroot' +rmtree(base) +rmtree(src) +makepath(base / '01', src) +make_data_file(src / 'f.dat', DATA_SIZE) +shutil.copy2(src / 'f.dat', base / '01' / 'f.dat') + +conf = write_daemon_conf([ + ('root', {'path': '/', 'read only': 'no'}), +]) +url = start_test_daemon(conf, DAEMON_PORT) + +base_rel = str(base).lstrip('/') # address `base` via the path=/ module +rmtree(base / '00') +proc = subprocess.run( + rsync_argv('-a', '--link-dest=../01', f'{src}/', f'{url}root/{base_rel}/00/'), + stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True) +out = proc.stdout or '' +if proc.returncode not in (0, 23): # 23: no-RESOLVE_BENEATH platforms reject the basis + test_fail(f"path=/ --link-dest push failed unexpectedly (rc={proc.returncode}):\n{out}") + +dest = base / '00' / 'f.dat' +basis = base / '01' / 'f.dat' +if not dest.is_file(): + test_fail(f"destination file missing ({dest})") + +ds, bs = dest.stat(), basis.stat() +if (ds.st_dev, ds.st_ino) != (bs.st_dev, bs.st_ino): + test_xfail( + "#915 (path=/ case): a `path = /` daemon module ignored --link-dest=../01 " + "(module_dirlen==0 skipped the re-anchor) -- the file was re-transferred " + "instead of hard-linked. Honoured once the re-anchor covers path=/.") +# Honoured: the dest is hard-linked to the in-module sibling basis. diff --git a/testsuite/link-dest-relative-basis_test.py b/testsuite/link-dest-relative-basis_test.py new file mode 100644 index 00000000..88510007 --- /dev/null +++ b/testsuite/link-dest-relative-basis_test.py @@ -0,0 +1,121 @@ +#!/usr/bin/env python3 +# Functional regression: a RELATIVE alt-basis dir (--link-dest / --copy-dest / +# --compare-dest = ../sibling) is silently ignored by a daemon receiver, so the +# basis is never used -- every file is re-transferred instead of hard-linked / +# copied / skipped. No error is printed; backups silently stop de-duplicating. +# +# Reported as #915 ("Security fix breaks --link-dest via rsync daemon": a +# `use chroot = no` daemon with `--link-dest=../01` re-transfers everything and +# fills the backup disk). The closely-related #928 is the same family over a +# remote shell with a relative `--link-dest=../snap.1`. +# +# Root cause: the 3.4.x symlink-race hardening resolves the receiver's basis +# through the confined resolver, which rejects the `..` that climbs from the +# destination (00) to its sibling basis (01); no basis is found, so the file is +# treated as new. Works in 3.4.1 (basis honoured). +# +# We exercise all three alt-basis forms because they are NOT obviously identical +# even though they share check_alt_basis_dirs(): +# * --link-dest=../01 : the matched file must be HARD-LINKED to the basis. +# * --copy-dest=../01 : the matched file is COPIED from the basis, so its +# data is NOT sent over the wire (literal data ~ 0). +# * --compare-dest=../01 : a matched file is skipped entirely -- NOT created +# in the destination at all. +# Each signal cleanly separates "basis honoured" (fixed/3.4.1) from "basis +# ignored" (the regression). +# +# XFAIL until a relative alt-basis dir is honoured by a sanitize_paths receiver +# again (the accompanying syscall.c/receiver.c fix; cf. upstream PR #930). On +# platforms without openat2/O_RESOLVE_BENEATH the portable resolver still +# rejects the '..' for safety, so this stays XFAIL there. Runs at any uid. + +import re +import subprocess + +from rsyncfns import ( + SCRATCHDIR, make_data_file, makepath, rmtree, rsync_argv, start_test_daemon, + test_fail, test_xfail, write_daemon_conf, +) + +DAEMON_PORT = 12915 +DATA_SIZE = 40000 + +mod = SCRATCHDIR / 'bakmod' # daemon module root: holds basis 01 and dest 00 +src = SCRATCHDIR / 'src915' +rmtree(mod) +rmtree(src) +makepath(mod / '01', src) +make_data_file(src / 'f.dat', DATA_SIZE) +# Basis 01 holds a byte-identical copy of the file (same name/size/mtime so the +# quick-check treats it as a match and the basis is eligible). +import shutil +shutil.copy2(src / 'f.dat', mod / '01' / 'f.dat') + +conf = write_daemon_conf([ + ('bak', {'path': str(mod), 'read only': 'no'}), +]) +url = start_test_daemon(conf, DAEMON_PORT) + + +def push(opt): + """Fresh dest 00, push src/ into bak/00/ with the given alt-basis option. + Returns (rc, stdout).""" + rmtree(mod / '00') + proc = subprocess.run( + rsync_argv('-a', '--stats', opt, f'{src}/', f'{url}bak/00/'), + stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True) + return proc.returncode, (proc.stdout or '') + + +def same_inode(a, b): + sa, sb = a.stat(), b.stat() + return (sa.st_dev, sa.st_ino) == (sb.st_dev, sb.st_ino) + + +def literal_bytes(out): + m = re.search(r'Literal data:\s*([\d,]+)', out) + return int(m.group(1).replace(',', '')) if m else -1 + + +regressions = [] +basis = mod / '01' / 'f.dat' + +# --- 1. --link-dest=../01 : matched file must be hard-linked to the basis ---- +rc, out = push('--link-dest=../01') +if rc not in (0, 23): # 23: no-RESOLVE_BENEATH platforms reject the basis + test_fail(f"--link-dest push failed unexpectedly (rc={rc}):\n{out}") +dest = mod / '00' / 'f.dat' +if not dest.is_file(): + test_fail(f"--link-dest: destination file missing ({dest})") +if not same_inode(dest, basis): + regressions.append("--link-dest=../01 did not hard-link to the basis " + "(file re-transferred)") + +# --- 2. --copy-dest=../01 : matched file copied locally, NOT sent on the wire - +rc, out = push('--copy-dest=../01') +if rc not in (0, 23): # 23: no-RESOLVE_BENEATH platforms reject the basis + test_fail(f"--copy-dest push failed unexpectedly (rc={rc}):\n{out}") +dest = mod / '00' / 'f.dat' +if not dest.is_file(): + test_fail(f"--copy-dest: destination file missing ({dest})") +lit = literal_bytes(out) +if lit > DATA_SIZE // 2: + regressions.append(f"--copy-dest=../01 re-sent the data over the wire " + f"(Literal data={lit}, basis not used)") + +# --- 3. --compare-dest=../01 : matched file skipped, NOT created in dest ------ +rc, out = push('--compare-dest=../01') +if rc not in (0, 23): # 23: no-RESOLVE_BENEATH platforms reject the basis + test_fail(f"--compare-dest push failed unexpectedly (rc={rc}):\n{out}") +if (mod / '00' / 'f.dat').is_file(): + regressions.append("--compare-dest=../01 created the file in the dest " + "(basis not matched, so the file was transferred)") + +if regressions: + test_xfail( + "#915: a daemon receiver ignored a RELATIVE alt-basis dir (../01); the " + "confined path resolver rejects the `..` climb to the sibling basis so " + "the basis is never used:\n - " + "\n - ".join(regressions) + + "\nTo be closed by honouring a relative alt-basis dir on a " + "sanitize_paths receiver again (cf. PR #930).") +# No regressions -> all three relative alt-basis forms honoured the basis. diff --git a/util1.c b/util1.c index 36c1b68c..12361057 100644 --- a/util1.c +++ b/util1.c @@ -41,8 +41,8 @@ extern filter_rule_list daemon_filter_list; int sanitize_paths = 0; -char curr_dir[MAXPATHLEN]; -unsigned int curr_dir_len; +extern char curr_dir[MAXPATHLEN]; /* defined in syscall.c */ +extern unsigned int curr_dir_len; int curr_dir_depth; /* This is only set for a sanitizing daemon. */ /* Set a fd into nonblocking mode. */