diff --git a/log.c b/log.c index bf30df98d..8b02b98e9 100644 --- a/log.c +++ b/log.c @@ -173,8 +173,7 @@ static void logfile_open(void) * attacker-writable dirs; a planted symlink could redirect root's log * into e.g. /root/.ssh/authorized_keys. Refuse symlinks not owned by * uid 0 or our euid. */ - int fd = open_no_attacker_symlinks(logfile_name, - O_WRONLY | O_APPEND | O_CREAT, 0644); + int fd = open_log_file(logfile_name, O_WRONLY | O_APPEND | O_CREAT, 0644); logfile_fp = fd >= 0 ? fdopen(fd, "a") : NULL; if (!logfile_fp && fd >= 0) close(fd); diff --git a/support/rrsync b/support/rrsync index a29889a1b..7e8c707b6 100755 --- a/support/rrsync +++ b/support/rrsync @@ -489,6 +489,53 @@ def safe_open_logfile(): return None return os.fdopen(fd, 'a') +def resolve_log_path(path, dir_st): + """Resolve symlinks before .. and reject paths through DIR.""" + todo = [c for c in path.split('/') if c] + if path.endswith('/'): + todo.append('.') + done = '//' if path.startswith('//') and not path.startswith('///') else '/' + hops = 0 + while todo: + comp = todo.pop(0) + if comp == '.': + continue + if comp == '..': + done = os.path.dirname(done) + continue + cand = os.path.join(done, comp) + try: + st = os.lstat(cand) + except FileNotFoundError as e: + if not todo: + return cand + die("cannot resolve the -log FILE", cand + ':', e.strerror) + except OSError as e: + die("cannot resolve the -log FILE", cand + ':', e.strerror) + if stat.S_ISLNK(st.st_mode): + if st.st_uid not in (0, os.geteuid()): + die("untrusted symlink in -log FILE:", cand) + hops += 1 + if hops > 40: + die("too many symlinks in the -log FILE path") + try: + target = os.readlink(cand) + except OSError as e: + die("cannot resolve the -log FILE", cand + ':', e.strerror) + if target.startswith('/'): + done = '//' if target.startswith('//') and not target.startswith('///') else '/' + todo = [c for c in target.split('/') if c] + (['.'] if target.endswith('/') else []) + todo + continue + if os.path.samestat(st, dir_st): + die("the -log FILE must be outside the restricted directory") + if todo: + try: + os.stat(os.path.join(cand, '.')) + except OSError as e: + die("cannot resolve the -log FILE", cand + ':', e.strerror) + done = cand + return done + def main(): if not os.path.isdir(args.dir): die("Restricted directory does not exist!") @@ -527,7 +574,7 @@ def main(): for opt in long_opts: if opt.startswith(('remove', 'delete')): long_opts[opt] = -1 - if args.ro: + if args.ro or args.log: long_opts['log-file'] = -1 global short_disabled @@ -672,6 +719,23 @@ def main(): if args.no_overwrite: rsync_opts.append('--ignore-existing') + if args.log: + if args.dir == '/': + die("-log cannot be used when DIR is /") + resolved_log = resolve_log_path(args.log, os.stat(args.dir)) + fd = None + try: + flags = os.O_WRONLY | os.O_APPEND | os.O_CREAT | os.O_NONBLOCK | getattr(os, 'O_NOFOLLOW', 0) + fd = os.open(resolved_log, flags, 0o644) + if not stat.S_ISREG(os.fstat(fd).st_mode): + die("-log FILE must be a regular file") + except OSError as e: + die("cannot open the -log FILE", args.log + ':', e.strerror) + finally: + if fd is not None: + os.close(fd) + rsync_opts.append('--log-file=' + args.log) + if not rsync_args: rsync_args = [ '.' ] @@ -1023,12 +1087,17 @@ if __name__ == '__main__': arg_parser.add_argument('-no-del', action='store_true', help="Disable rsync's --delete* and --remove* options.") arg_parser.add_argument('-no-lock', action='store_true', help="Avoid the single-run (per-user) lock check.") arg_parser.add_argument('-no-overwrite', action='store_true', help="Prevent overwriting existing files by enforcing --ignore-existing") + arg_parser.add_argument('-log', metavar='FILE', help="Log each transfer to FILE via rsync's --log-file.") arg_parser.add_argument('-help', '-h', action='help', help="Output this help message and exit.") arg_parser.add_argument('dir', metavar='DIR', help="The restricted directory to use.") args = arg_parser.parse_args() args.dir = os.path.realpath(args.dir) args.dir_slash = args.dir + '/' args.dir_slash_len = len(args.dir_slash) + if args.log is not None: + if not args.log: + die("empty -log FILE") + args.log = os.path.join(os.getcwd(), args.log) if args.ro: args.no_del = True elif not args.no_lock: diff --git a/support/rrsync.1.md b/support/rrsync.1.md index 2ab0fd17d..58c61145f 100644 --- a/support/rrsync.1.md +++ b/support/rrsync.1.md @@ -5,7 +5,7 @@ rrsync - a script to setup restricted rsync users via ssh logins ## SYNOPSIS ``` -rrsync [-ro|-wo] [-munge] [-absolute] [-no-del] [-no-lock] [-no-overwrite] DIR +rrsync [-ro|-wo] [-munge] [-absolute] [-no-del] [-no-lock] [-no-overwrite] [-log FILE] DIR ``` The single non-option argument specifies the restricted _DIR_ to use. It can be @@ -103,8 +103,29 @@ The remainder of this manpage is dedicated to using the rrsync script. replacing whatever has appeared at that name since the check), and backup mode (`-b`, `--backup-dir`, whose published backup replaces whatever already occupies the backup name). Resumable uploads with an explicit - `--partial-dir`, `--delay-updates`, `--delay-symlinks`, and server-side - logging are therefore unavailable under this option. + `--partial-dir`, `--delay-updates`, `--delay-symlinks` and client-selected + log files are therefore unavailable under this option. Administrator logging + with [`-log FILE`](#opt) remains available. + +0. `-log FILE` + + Write server-side transfer records to _FILE_ using rsync's + [`--log-file`](rsync.1#opt). The default "%i %n%L" format records files sent, + received or deleted, with timestamps and process IDs. The client's own + `--log-file` is refused. This works with `-ro`, `-wo` and `-no-overwrite`. + + _FILE_ must be a regular file outside _DIR_. It can be absolute or relative + to the user's home directory. rrsync resolves symlinks before `..` and + refuses paths that pass through _DIR_. `-log` is refused when _DIR_ is `/`. + Symlinks must be owned by root or the user running rrsync. Keep the log and + its parent directories out of untrusted users' reach, including + through hard links or bind mounts; rrsync cannot detect every such alias. + + rrsync checks that _FILE_ can be opened before starting the transfer. + Rsync then opens the file itself. Logging is best-effort: a later open or + write failure can leave a successful transfer with incomplete or missing + records. Use rsync's exit status to determine whether a transfer succeeded. + This option does not provide a tamper-resistant audit trail. 0. `-help`, `-h` diff --git a/syscall.c b/syscall.c index e0bae3807..339995d8b 100644 --- a/syscall.c +++ b/syscall.c @@ -421,7 +421,7 @@ static int ona_open_absolute_root(char *remaining, char *abspath, size_t abspath /* Core walk. When out_abs is non-NULL and the path resolves to a directory * (O_DIRECTORY), the resolved absolute path is copied there -- owner_walk_parent * uses it to filter-check the (otherwise unchecked) leaf basename. */ -static int ona_open(const char *path, int flags, mode_t mode, char *out_abs, size_t out_cap) +static int ona_open(const char *path, int flags, mode_t mode, char *out_abs, size_t out_cap, int enforce_root) { #if defined AT_FDCWD && defined O_NOFOLLOW && defined O_DIRECTORY const int dir_traverse_flags = directory_traverse_flags() | O_CLOEXEC; @@ -446,7 +446,7 @@ static int ona_open(const char *path, int flags, mode_t mode, char *out_abs, siz char abspath[MAXPATHLEN] = {0}; if (am_daemon && module_dir && module_dir[0] == '/') strlcpy(abspath, module_dir, sizeof abspath); /* "/" for a path=/ module */ - else if (confine_root) { + else if (enforce_root && confine_root) { /* Unlike a daemon's, this cwd is not pinned to the root -- the receiver * chdir's into the destination -- so it has to be read, not assumed. * It must be the PHYSICAL cwd: curr_dir is the lexical name change_dir() @@ -472,7 +472,7 @@ static int ona_open(const char *path, int flags, mode_t mode, char *out_abs, siz * following the link restarts the walk at its absolute target, and every * component of THAT is checked, so a pin aimed outside is still refused. */ const char *ptail = fd_pin_tail(path); - int pin_transit = !am_daemon && confine_root && ptail != NULL; + int pin_transit = enforce_root && !am_daemon && confine_root && ptail != NULL; /* Path-walk state. `remaining` is the unconsumed tail; we splice * symlink targets back into it as we go. Sized 2x MAXPATHLEN so a @@ -534,7 +534,7 @@ static int ona_open(const char *path, int flags, mode_t mode, char *out_abs, siz saved_errno = errno; goto out; } - if (!pin_transit && abspath_outside_confinement(abspath, 1)) { + if (enforce_root && !pin_transit && abspath_outside_confinement(abspath, 1)) { saved_errno = ELOOP; goto out; } @@ -664,7 +664,7 @@ static int ona_open(const char *path, int flags, mode_t mode, char *out_abs, siz saved_errno = errno; goto out; } - if (!pin_transit && abspath_outside_confinement(abspath, 1)) { + if (enforce_root && !pin_transit && abspath_outside_confinement(abspath, 1)) { saved_errno = ELOOP; goto out; } @@ -688,7 +688,7 @@ static int ona_open(const char *path, int flags, mode_t mode, char *out_abs, siz saved_errno = errno; goto out; } - if (!pin_transit && abspath_outside_confinement(abspath, 0)) { + if (enforce_root && !pin_transit && abspath_outside_confinement(abspath, 0)) { saved_errno = ELOOP; goto out; } @@ -716,7 +716,7 @@ static int ona_open(const char *path, int flags, mode_t mode, char *out_abs, siz * an O_PATH fd is sufficient for traversal and fchdir but not operations * such as fchmod. */ if (flags & O_DIRECTORY) { - if (!pin_transit && abspath_outside_confinement(abspath, 1)) { + if (enforce_root && !pin_transit && abspath_outside_confinement(abspath, 1)) { saved_errno = ELOOP; goto out; } @@ -737,13 +737,20 @@ static int ona_open(const char *path, int flags, mode_t mode, char *out_abs, siz #else /* Pre-AT_FDCWD / no O_NOFOLLOW systems: best-effort fallback. */ (void)out_abs; (void)out_cap; + (void)enforce_root; return open(path, flags, mode); #endif } int open_no_attacker_symlinks(const char *path, int flags, mode_t mode) { - return ona_open(path, flags, mode, NULL, 0); + return ona_open(path, flags, mode, NULL, 0, 1); +} + +/* A log's ownership check must not inherit file-list confinement. */ +int open_log_file(const char *path, int flags, mode_t mode) +{ + return ona_open(path, flags, mode, NULL, 0, 0); } /* Open a directory for traversal or as *at()/fchdir() authority. Unlike an @@ -751,7 +758,7 @@ int open_no_attacker_symlinks(const char *path, int flags, mode_t mode) * directory on Linux. */ int open_no_attacker_symlinks_dirfd(const char *path) { - return ona_open(path, directory_traverse_flags(), 0, NULL, 0); + return ona_open(path, directory_traverse_flags(), 0, NULL, 0, 1); } /* When set, the do_*_at() wrappers resolve their path as an OPERATOR-supplied @@ -780,7 +787,7 @@ int owner_walk_parent(const char *path, const char **bname) *bname = slash ? slash + 1 : path; pabs[0] = '\0'; if (!slash) - dfd = ona_open(".", directory_traverse_flags(), 0, pabs, sizeof pabs); + dfd = ona_open(".", directory_traverse_flags(), 0, pabs, sizeof pabs, 1); else { dlen = slash == path ? 1 : (size_t)(slash - path); /* "/x" -> parent "/" */ if (dlen >= sizeof dir) { @@ -789,7 +796,7 @@ int owner_walk_parent(const char *path, const char **bname) } memcpy(dir, path, dlen); dir[dlen] = '\0'; - dfd = ona_open(dir, directory_traverse_flags(), 0, pabs, sizeof pabs); + dfd = ona_open(dir, directory_traverse_flags(), 0, pabs, sizeof pabs, 1); } if (dfd < 0) return -1; diff --git a/testsuite/rrsync-log_test.py b/testsuite/rrsync-log_test.py new file mode 100644 index 000000000..fdb4ea74d --- /dev/null +++ b/testsuite/rrsync-log_test.py @@ -0,0 +1,245 @@ +#!/usr/bin/env python3 + +import os +import shlex +import shutil +import signal +import stat +import subprocess +import sys + +signal.signal(signal.SIGUSR1, signal.SIG_IGN) +signal.signal(signal.SIGUSR2, signal.SIG_IGN) +if '--shell' in sys.argv: + i = sys.argv.index('--shell') + 2 + env = {**os.environ, 'SSH_ORIGINAL_COMMAND': ' '.join(sys.argv[i:])} + signal.signal(signal.SIGUSR1, signal.SIG_DFL) + signal.signal(signal.SIGUSR2, signal.SIG_DFL) + flags = shlex.split(env['RRSYNC_FLAGS']) + os.execve(env['RRSYNC_WRAPPER'], [env['RRSYNC_WRAPPER'], *flags, env['RRSYNC_ROOT']], env) + +from rsyncfns import RSYNC, SCRATCHDIR, makepath, patched_rrsync, rsync_argv, rsync_path_arg, test_fail + +base = SCRATCHDIR / 'rrsync-log' +src, root = base / 'src', base / 'root' +makepath(src / 'sub', root) +(src / 'a.txt').write_bytes(b'AAA') +(src / 'sub' / 'b.txt').write_bytes(b'BB') +log = base / 'transfer.log' +shim = base / 'rsync-shim' +shim.write_text('#!/bin/sh\nexec ' + rsync_path_arg(RSYNC) + ' "$@"\n') +shim.chmod(0o755) +wrapper = patched_rrsync(base, rsync_path=str(shim)) +rsh = f'{shlex.quote(sys.executable)} {shlex.quote(os.path.abspath(__file__))} --shell' + +def transfer(*options, flags=('-wo', '-no-lock'), logfile=log, sender=f'{src}/', receiver='ignored:', script=wrapper): + flags = [*flags] + (['-log', str(logfile)] if logfile is not None else []) + env = {**os.environ, 'RRSYNC_WRAPPER': str(script), 'RRSYNC_ROOT': str(root), 'RRSYNC_FLAGS': shlex.join(flags)} + return subprocess.run(rsync_argv('-rI', *options, '-e', rsh, sender, receiver), + cwd=base, env=env, capture_output=True, text=True, timeout=20) + +def check(result, error=None): + if error is None: + if result.returncode != 0: + test_fail(f'transfer failed: {result.returncode}: {result.stderr}') + elif result.returncode == 0 or error not in result.stderr: + test_fail(f'expected {error!r}: {result.returncode}: {result.stderr}') + +def recorded(path, name, marker): + lines = path.read_text().splitlines() + if not any(marker in line and line.endswith(' ' + name) for line in lines): + test_fail(f'{name} missing from {path}: {lines!r}') + if not any('sent ' in line and ' received ' in line and ' total size ' in line for line in lines): + test_fail(f'transfer summary missing: {lines!r}') + +log.write_text('previous transfer\n') +log.chmod(0o600) +check(transfer(flags=('-wo',))) +if (root / 'sub' / 'b.txt').read_bytes() != b'BB': + test_fail('upload contents differ') +recorded(log, 'a.txt', '>f') +recorded(log, 'sub/b.txt', '>f') +downloaded = base / 'download' +check(transfer(flags=('-ro',), sender='ignored:', receiver=f'{downloaded}/')) +if (downloaded / 'sub' / 'b.txt').read_bytes() != b'BB': + test_fail('download contents differ') +recorded(log, 'a.txt', 'f') +selected_log = base / 'log with spaces' +check(transfer(logfile=selected_log)) +recorded(selected_log, 'a.txt', '>f') +file_list = base / 'files-from' +file_list.write_text('a.txt\n') +selected_log = base / 'files-from.log' +check(transfer(f'--files-from={file_list}', flags=('-ro',), logfile=selected_log, + sender='ignored:', receiver=f'{base}/listed/')) +recorded(selected_log, 'a.txt', 'f') + +makepath(base / 'real' / 'inner') +os.symlink(base / 'real' / 'inner', base / 'alias') +check(transfer(logfile='alias/../relative.log')) +if not (base / 'real' / 'relative.log').is_file() or (base / 'relative.log').exists(): + test_fail('symlink followed by .. resolved to the wrong log') +recorded(base / 'real' / 'relative.log', 'a.txt', '>f') +check(transfer(logfile=base / 'alias' / 'absolute.log')) +if not (base / 'real' / 'inner' / 'absolute.log').is_file(): + test_fail('external symlink log path failed') +recorded(base / 'real' / 'inner' / 'absolute.log', 'a.txt', '>f') +leaf = base / 'leaf' +leaf.symlink_to(base / 'real' / 'leaf.log') +check(transfer(logfile=leaf)) +recorded(base / 'real' / 'leaf.log', 'a.txt', '>f') +double_slash = base / 'double-slash.log' +check(transfer(logfile='/' + str(double_slash))) +recorded(double_slash, 'a.txt', '>f') + +os.symlink(root, base / 'into') +os.symlink(base / 'stolen', root / 'out') +os.symlink(root / 'out', base / 'through') +for path in (root / 'transfer.log', base / 'into' / 'transfer.log', base / 'through', root / '..' / 'escaped.log'): + check(transfer(logfile=path), 'must be outside the restricted directory') +if (root / 'transfer.log').exists() or (base / 'stolen').exists() or (base / 'escaped.log').exists(): + test_fail('a rejected log was created') + +if os.geteuid() == 0: + real = base / 'foreign-real' + makepath(real) + foreign_dir = base / 'foreign-dir' + foreign_dir.symlink_to(real) + foreign_leaf = base / 'foreign-leaf' + foreign_leaf.symlink_to(real / 'leaf.log') + for link, path, target in ((foreign_leaf, foreign_leaf, real / 'leaf.log'), + (foreign_dir, foreign_dir / 'parent.log', real / 'parent.log')): + os.lchown(link, 65534, 65534) + target.write_text('unchanged\n') + result = subprocess.run(rsync_argv('-rI', f'--log-file={path}', f'{src}/', f'{root}/'), + capture_output=True, text=True, timeout=20) + if ('refusing to follow a symlink owned by an untrusted user' not in result.stderr + or target.read_text() != 'unchanged\n'): + test_fail('ordinary --log-file ownership control failed') + result = transfer(logfile=path) + if target.read_text() != 'unchanged\n': + test_fail('untrusted -log symlink changed its target') + check(result, 'untrusted symlink in -log FILE') + +regular = base / 'regular' +regular.write_text('not a directory') +os.symlink('regular/../bad.log', base / 'bad-link') +os.symlink('regular/', base / 'bad-slash') +for path in ('regular/../bad.log', 'regular/./bad.log', str(regular) + '/', base / 'bad-link', base / 'bad-slash'): + check(transfer(logfile=path), 'cannot resolve the -log FILE') +if (base / 'bad.log').exists(): + test_fail('an invalid log path was normalised and opened') +os.symlink('loop', base / 'loop') +check(transfer(logfile=base / 'loop'), 'too many symlinks') +check(transfer(logfile=''), 'empty -log FILE') +check(transfer(logfile=base / 'real'), 'cannot open the -log FILE') +check(transfer(logfile='missing/../bad.log'), 'cannot resolve the -log FILE') +if os.geteuid() != 0: + denied = base / 'no-search' + denied.mkdir(mode=0o600) + try: + check(transfer(logfile='no-search/../bad.log'), 'cannot resolve the -log FILE') + finally: + denied.chmod(0o700) + readonly = base / 'readonly.log' + readonly.touch(mode=0o400) + check(transfer(logfile=readonly), 'cannot open the -log FILE') + +(root / 'sub' / 'b.txt').unlink() +check(transfer(logfile=base / 'missing-dir' / 'transfer.log'), 'cannot resolve the -log FILE') +if (root / 'sub' / 'b.txt').exists(): + test_fail('transfer ran despite a failed initial log open') + +if hasattr(os, 'mkfifo'): + fifo = base / 'fifo' + os.mkfifo(fifo) + fd = os.open(fifo, os.O_RDONLY | os.O_NONBLOCK) + try: + check(transfer(logfile=fifo), '-log FILE must be a regular file') + finally: + os.close(fd) + +for command in ('true', 'not-rsync'): + unused = base / 'unused.log' + result = subprocess.run([str(wrapper), '-no-lock', '-log', str(unused), str(root)], + env={**os.environ, 'SSH_ORIGINAL_COMMAND': command}, + stdin=subprocess.DEVNULL, capture_output=True, text=True, timeout=20) + if result.returncode != (0 if command == 'true' else 1) or unused.exists(): + test_fail(f'non-transfer command opened the log: {result.stderr}') + +result = subprocess.run([str(wrapper), '-no-lock', '-log', str(log), '/'], + env={**os.environ, 'SSH_ORIGINAL_COMMAND': 'rsync --server -r . .'}, + stdin=subprocess.DEVNULL, capture_output=True, text=True, timeout=20) +check(result, '-log cannot be used when DIR is /') + +for fault in ('directory', 'full', 'foreign'): + if fault == 'full' and not os.path.exists('/dev/full'): + continue + if fault == 'foreign' and os.geteuid() != 0: + continue + directory = base / fault + makepath(directory) + fault_log = directory / 'transfer.log' + fault_shim = directory / 'rsync-shim' + quoted = shlex.quote(str(fault_log)) + change = f'mkdir {quoted}' if fault == 'directory' else f'ln -s /dev/full {quoted}' + if fault == 'foreign': + victim = directory / 'victim' + victim.write_text('unchanged\n') + change = f'ln -s {shlex.quote(str(victim))} {quoted}\nchown -h 65534 {quoted}' + fault_shim.write_text(f'#!/bin/sh\nset -e\nmv {quoted} {quoted}.old\n{change}\nexec ' + + rsync_path_arg(RSYNC) + ' "$@"\n') + fault_shim.chmod(0o755) + fault_wrapper = patched_rrsync(directory, rsync_path=str(fault_shim)) + (src / f'after-{fault}').write_text('transferred') + result = transfer(script=fault_wrapper, logfile=fault_log) + check(result) + if (root / f'after-{fault}').read_text() != 'transferred': + test_fail('best-effort logging stopped the transfer') + if fault == 'directory' and 'failed to open log-file' not in result.stderr: + test_fail(f'log reopen failure was not reported: {result.stderr}') + if (fault == 'foreign' and (victim.read_text() != 'unchanged\n' + or 'refusing to follow a symlink owned by an untrusted user' not in result.stderr)): + test_fail('the log reopen bypassed symlink ownership checks') + +unshare = shutil.which('unshare') +userns = [unshare, '--user', '--map-root-user', '--mount'] if unshare else None +if userns and subprocess.run(userns + ['true'], capture_output=True, timeout=10).returncode == 0: + bind = base / 'bind' + makepath(bind) + command = 'mount --bind "$1" "$2" && exec "$3" -wo -no-lock -log "$2/transfer.log" "$1"' + result = subprocess.run(userns + ['sh', '-c', command, 'sh', str(root), str(bind), str(wrapper)], + env={**os.environ, 'SSH_ORIGINAL_COMMAND': 'rsync --server -r . .'}, + stdin=subprocess.DEVNULL, capture_output=True, text=True, timeout=20) + check(result, 'must be outside the restricted directory')