From 0894e0422e4b9ceddfe1fc7bbf6dba5ef09f0dc6 Mon Sep 17 00:00:00 2001 From: Andrew Tridgell Date: Tue, 28 Jul 2026 08:54:29 +1000 Subject: [PATCH] rrsync: pin a sender argument where rsync will actually resolve it 88cee089 rewrote every validated argument to /proc/self/fd/N so the spawned rsync re-resolves it to the pinned inode. That is right for a receiver, which open()s its destination, but a sender never opens its source argument: send_file_list() lstat()s it first, and lstat of a procfs magic link is always S_IFLNK. So the sender described the argument as a symlink and sent no data -- silent data loss on "rsync -a user@host:file dest/", the most ordinary command there is. Of the argument shapes now covered, only a trailing-slash directory survived. Leonid Bugaev reported the regression, diagnosed the lstat-vs-magic-link mechanism, and supplied the first regression test. Which pin is usable depends on what rsync does with the argument: * a trailing "/" or "/." directory is opened, not lstat()ed, and rsync does follow a symlink there, so it keeps the leaf pin -- verified: without it a flipped leaf transfers the outside directory's content; * anything else pins one level up and passes the leaf by name. rsync will not follow a symlink at that position (it sends the symlink itself), -L/-k/--copy-unsafe-links are already disabled for a restricted dir, and rsync's own leaf open is O_NOFOLLOW. The directory pin resolves normally, including a symlink at its last component, which is legitimate and which 3.4.4 accepts; the readlink check afterwards is what proves the held inode is in-tree. It uses O_PATH because reaching a known name beneath a directory needs only search permission, and a mode 0111 parent is an ordinary way to publish a file without letting it be listed. Under --relative the transmitted name is the whole argument rather than its basename, so the pin moves up to where that name starts and the rest is spelled after a /./ marker. The client's own first marker wins if it supplied one, including when nothing follows it; -R is parsed out of the short-option cluster rather than sniffed for the letter, so the trailing capability blob (-e.iLsfxC) cannot be mistaken for it. Directory pins are keyed by (st_dev, st_ino), so a glob whose matches share a parent inherits one descriptor rather than one per argument. Every shape now delivers what a pristine 3.4.4 delivers, which is what rrsync-pull-arg-shapes asserts -- it passes against 3.4.4 itself, so the expectations are that behaviour and not this implementation's. The "-R --no-implied-dirs" case is gated on protocol >= 30: at protocol 29 the receiver rejects it with "invalid path from sender" and transfers nothing, which 3.4.4 does identically. Moving the sender's pin off the leaf invalidates rrsync-symlink's oracle, so it is reworked here rather than left failing. It patches rsync to a stub that open()s its last argument, which is a faithful model for an intermediate path component -- whatever rsync does with the final name, it must not reach it through a flipped parent -- but not for the leaf: a sender lstat()s its source and transmits a symlink there rather than reading through it. So it now flips an intermediate directory, and the leaf is covered against the real binary by rrsync-sender-leaf-flip, which races both a plain file argument and a trailing-slash directory and asserts no outside CONTENT is delivered rather than requiring a symptom from a race that may not be won on a given run. Its trailing-slash half is RED against a pristine 3.4.4 rrsync, which delivers outside/dir/loot. rrsync-symlink is now sender-only. Measured over a 5s race, the stub reached the outside marker 28 times in 98 runs as a sender and 25 in 97 as a receiver against 3.4.4; with the pin it is 0 as a sender but still 7-9 as a receiver, on the branch base as well as here. That residual is the receiver's not-yet-existing-destination fallback, which predates this work and is tracked in #139 rather than folded in. Clearing FD_CLOEXEC goes through F_SETFD rather than os.set_inheritable(), which prefers ioctl(FIONCLEX) and gets EBADF from an O_PATH descriptor on older kernels -- every sender pull on Ubuntu 18.04 aborted with "Bad file descriptor" the moment a directory pin was taken. Co-authored-by: Leonid Bugaev (cherry picked from commit 6edb7dea2a77d998ad30355b9cc1d7c0aac5c370) --- support/rrsync | 181 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 177 insertions(+), 4 deletions(-) diff --git a/support/rrsync b/support/rrsync index 4c571f0f..5136fc24 100755 --- a/support/rrsync +++ b/support/rrsync @@ -142,6 +142,14 @@ from argparse import RawTextHelpFormatter # TOCTOU. pinned_fds = [] +# Directory pins, keyed by (st_dev, st_ino), so a glob or a multi-arg command +# whose args share a parent inherits one fd rather than one per arg. +pinned_dirs = {} + +# Whether the client asked for --relative/-R, which decides how much of a +# sender arg rsync transmits as the file's name (see sender_pinned_arg). +client_relative = False + # The inode-pin trick needs /proc/self/fd/N to be a Linux-style magic symlink # whose readlink yields the open file's real path. macOS/BSD lack the directory # entirely; Solaris HAS /proc/self/fd but its entries are not such symlinks (its @@ -172,6 +180,134 @@ HAS_DOT_DOT_RE = re.compile(r'(^|/)\.\.(/|$)') LONG_OPT_RE = re.compile(r'^--([^=]+)(?:=(.*))?$') DE_BACKSLASH_RE = re.compile(r'\\(.)') +def make_inheritable(fd): + """Clear FD_CLOEXEC so the exec'd rsync inherits `fd`. + + os.set_inheritable() prefers ioctl(FIONCLEX), which an O_PATH descriptor + rejects with EBADF on older kernels; F_SETFD is one of the few operations + O_PATH always allows. + """ + try: + os.set_inheritable(fd, True) + except OSError: + import fcntl + fcntl.fcntl(fd, fcntl.F_SETFD, + fcntl.fcntl(fd, fcntl.F_GETFD) & ~fcntl.FD_CLOEXEC) + +def pin_dir(path, orig_arg): + """Inode-pin a directory and return an fd rsync will inherit. + + The open resolves `path` normally -- including a symlink at its last + component, which is legitimate and which 3.4.4 accepts -- so a component + could be flipped first. The readlink check afterwards is what makes that + safe: it proves the inode we ended up holding is inside the restricted + tree. From then on the fd names that inode, not the path, so nothing above + it can be flipped again. + + O_PATH, not O_RDONLY: reaching a known name beneath a directory needs only + search permission, and a mode 0111 parent is a perfectly ordinary way to + publish a file without letting it be listed. An O_PATH directory fd is + just as firmly pinned when used as a /proc/self/fd/N/... prefix (the O_PATH + caveat in validated_arg() is about reopening the magic link as the file + itself, which is not what happens here). + """ + flags = os.O_DIRECTORY | getattr(os, 'O_PATH', 0) + if not flags & getattr(os, 'O_PATH', 0): + flags |= os.O_RDONLY + try: + fd = os.open(path or '.', flags) + except OSError as e: + die('unable to pin sender path:', orig_arg, e.strerror) + try: + st = os.fstat(fd) + pinned_path = os.readlink('/proc/self/fd/%d' % fd) + except OSError as e: + os.close(fd) + die('post-pin readlink failed (race?):', orig_arg, e.strerror) + if pinned_path != args.dir and not pinned_path.startswith(args.dir_slash): + os.close(fd) + die('post-pin path escaped tree (race?):', orig_arg, pinned_path) + key = (st.st_dev, st.st_ino) + if key in pinned_dirs: + os.close(fd) + return pinned_dirs[key] + make_inheritable(fd) + pinned_fds.append(fd) + pinned_dirs[key] = fd + return fd + +# sender_pinned_arg() verdicts that are not a rewritten argument. +KEEP_LEAF_PIN = 'keep' # hand rsync the leaf's own /proc/self/fd/N +LEAF_PIN_UNUSABLE = 'unusable' # no pin for this shape; keep the plain name + +def sender_pinned_arg(fd, arg, orig_arg, has_slash, has_slash_dot): + """Return a source name rsync will both resolve safely and name correctly. + + The obvious rewrite -- hand rsync /proc/self/fd/N for the leaf itself -- + only works where rsync open()s the argument. A sender lstat()s it first, + and lstat of a procfs magic link is always S_IFLNK, so rsync describes the + argument as a symlink instead of sending the file. Which pin is usable + therefore depends on what rsync does with the argument: + + * a trailing "/" or "/." directory is opened, not lstat()ed, and rsync + DOES follow a symlink there, so it keeps the leaf pin; + * anything else keeps the pin one level up and passes the leaf by name. + rsync will not follow a symlink at that position (it sends the symlink + itself), the follow options that would change that are already disabled + for a restricted dir, and its own leaf open is O_NOFOLLOW. + + Under --relative the transmitted name is the whole argument rather than + its basename, so the pin has to move up to wherever that name starts and + the rest is spelled after a /./ marker, which is how rsync is told where + the transmitted portion begins. + """ + if client_relative: + # Look for the marker in the argument as the client spelled it: the + # caller has already split off a trailing "/" or "/.", which is exactly + # what turns "sub/./" into a terminal marker. + full = arg + ('/' if has_slash else '/.' if has_slash_dot else '') + # rsync honours the FIRST /./, so split there and keep the client's + # marker rather than inserting a second, earlier one. + head, marker, tail = full.partition('/./') + if marker: + anchor, suffix = head, tail.rstrip('/') + else: + anchor, suffix = '', arg + if suffix.startswith('/'): + return LEAF_PIN_UNUSABLE + if suffix in ('', '.'): + # A terminal marker: everything the client wants transmitted starts + # at the argument itself, which is the directory we already pinned. + # The caller re-appends the trailing "/" or "/.". + dfd = pin_dir(anchor, orig_arg) + check = '.' + pinned = '/proc/self/fd/%d/.' % dfd + else: + dfd = pin_dir(anchor, orig_arg) + pinned = '/proc/self/fd/%d/./%s' % (dfd, suffix) + check = suffix + else: + if has_slash or has_slash_dot: + return KEEP_LEAF_PIN + anchor, _, leaf = arg.rpartition('/') + if not leaf or leaf in ('.', '..'): + return LEAF_PIN_UNUSABLE + dfd = pin_dir(anchor, orig_arg) + pinned = '/proc/self/fd/%d/%s' % (dfd, leaf) + check = leaf + + # Tie the pinned directory to the inode realpath() validated: resolving + # `check` beneath the held fd cannot be redirected above the leaf, so if it + # does not reach the same file, something was flipped -- fail closed. + try: + st = os.stat(check, dir_fd=dfd) + except OSError as e: + die('post-pin stat failed (race?):', orig_arg, e.strerror) + leaf_st = os.fstat(fd) + if (st.st_dev, st.st_ino) != (leaf_st.st_dev, leaf_st.st_ino): + die('post-pin path changed (race?):', orig_arg, check) + return pinned + def safe_open_logfile(): nofollow = getattr(os, 'O_NOFOLLOW', 0) try: @@ -253,6 +389,7 @@ def main(): except OSError as e: die('unable to chdir to restricted dir:', str(e)) + global client_relative rsync_opts = [ '--server' ] rsync_args = [ ] saw_the_dot_arg = False @@ -275,7 +412,16 @@ def main(): saw_the_dot_arg = True continue rsync_opts.append(arg) - if short_no_arg_re.match(arg) or short_with_num_re.match(arg): + sm = short_no_arg_re.match(arg) + if sm or short_with_num_re.match(arg): + if sm: + # Scan the cluster's own letters only: the trailing + # capability blob (-e.iLsfxC) is not a set of options. + letters = arg[1:] + if sm.group(1): + letters = letters[:-len(sm.group(1))] + if 'R' in letters: + client_relative = True continue disabled = False m = LONG_OPT_RE.match(arg) @@ -285,6 +431,11 @@ def main(): ct = long_opts.get(opt, None) if ct is None: break # Generate generic failure due to unfinished arg parsing + # Last one wins, matching rsync's own option handling. + if opt == 'relative': + client_relative = True + elif opt == 'no-relative': + client_relative = False if ct == 0: continue opt = '--' + opt @@ -384,6 +535,7 @@ def validated_arg(opt, arg, typ=3, wild=False): for arg in got: if args.dir != '/' and arg != '.' and (typ == 3 or (typ == 2 and not am_sender)): arg_has_trailing_slash = arg.endswith('/') + arg_has_trailing_slash_dot = False if arg_has_trailing_slash: arg = arg[:-1] else: @@ -498,9 +650,30 @@ def validated_arg(opt, arg, typ=3, wild=False): os.close(fd) die('post-pin path escaped tree (race?):', orig_arg, pinned_path) - os.set_inheritable(fd, True) - pinned_fds.append(fd) - arg = '/proc/self/fd/%d' % fd + if am_sender and opt == 'arg': + logical = arg + if is_absolute_arg: + if logical == args.dir: + logical = '' + elif logical.startswith(args.dir_slash): + logical = logical[args.dir_slash_len:] + pinned = sender_pinned_arg(fd, logical, orig_arg, + arg_has_trailing_slash, + arg_has_trailing_slash_dot) + else: + pinned = KEEP_LEAF_PIN + if pinned == KEEP_LEAF_PIN: + os.set_inheritable(fd, True) + pinned_fds.append(fd) + arg = '/proc/self/fd/%d' % fd + elif pinned == LEAF_PIN_UNUSABLE: + # Nothing to spell beneath a held directory (a bare "." + # or the tree root): keep the realpath-validated name, + # which is what 3.4.4 passes. + os.close(fd) + else: + os.close(fd) # only needed to validate the pin + arg = pinned if arg_has_trailing_slash: arg += '/' elif arg_has_trailing_slash_dot: