diff --git a/loadparm.c b/loadparm.c index cc619a9e..875951c7 100644 --- a/loadparm.c +++ b/loadparm.c @@ -181,7 +181,13 @@ static int shell_unsafe_value(const char *val) const char *s; for (s = val; *s; s++) { - if (strchr("'\"`$\\;&|<>()*?[]# ", *s) + /* '!' negates in command position (a hook `sh -c '%VAR% false'` + * becomes `! false` and reports success, inverting an access + * check); '~' is tilde-expanded; '{' and '}' brace-expand in + * bash and zsh. None of them execute anything on their own, + * which is why a set built from the obvious metacharacters + * missed them. */ + if (strchr("'\"`$\\;&|<>()*?[]# !~{}", *s) || (unsigned char)*s < 0x20 || (unsigned char)*s == 0x7f) return 1; } diff --git a/rsyncd.conf.5.md b/rsyncd.conf.5.md index 3d0c821e..338abe71 100644 --- a/rsyncd.conf.5.md +++ b/rsyncd.conf.5.md @@ -1169,15 +1169,22 @@ in the values of parameters. See that section for details. data any more -- it chooses the command. Because rsync cannot escape for an unknown number of shell passes, a value - carrying any character that is active in a shell is refused outright, and + carrying any character that a shell could act on is refused outright, and the transfer is aborted with > refusing to run shell hook: %VAR% holds a shell metacharacter + The refused characters are whitespace, the quoting and expansion characters + `'` `"` `` ` `` `$` `\`, the separators `;` `&` `|`, redirections `<` `>`, + parentheses, the pattern characters `*` `?` `[` `]`, `#`, `!`, `~`, `{` `}`, + and any control character. Some of those are harmless on their own and are + refused for what a *second* shell would do with them -- `!` negates in + command position, so a hook written as an access check can be turned from a + denial into an approval, and `~` is tilde-expanded. + This applies to EVERY `%RSYNC_*%` value, including ones you supplied - yourself. In particular a module whose `path` contains a space, a `*`, a - `?`, a `[`, a `]` or a `#` cannot be interpolated into one of these - commands: `path = /srv/My Backups` with a command mentioning + yourself. In particular a module whose `path` contains any of them cannot + be interpolated into one of these commands: `path = /srv/My Backups` with a command mentioning `%RSYNC_MODULE_PATH%` will refuse every transfer of that module, not just a hostile one. The restriction is deliberate -- rsync cannot tell your space from an attacker's once both are inside the same string, and `path` itself @@ -1185,7 +1192,9 @@ in the values of parameters. See that section for details. If you need such a value in a hook, pass it through the environment instead of interpolating it: the same names are exported to the command, so - `$RSYNC_MODULE_PATH` inside your script is both safe and unrestricted. + `"$RSYNC_MODULE_PATH"` inside your script is unrestricted by this check. + Quote it there as shown -- rsync no longer has any say in how your script + splits the value. Even though the commands can be associated with a particular module, they are run using the permissions of the user that started the daemon (not the diff --git a/testsuite/daemon-exec-metachar-documented-limit_test.py b/testsuite/daemon-exec-metachar-documented-limit_test.py index a11bc9e3..1c6cbe94 100644 --- a/testsuite/daemon-exec-metachar-documented-limit_test.py +++ b/testsuite/daemon-exec-metachar-documented-limit_test.py @@ -14,6 +14,7 @@ It is pinned here because it is a real usability cost, and if it ever changes the manual has to change with it. """ +import os import subprocess from rsyncfns import ( @@ -23,6 +24,12 @@ from rsyncfns import ( REFUSAL = 'holds a shell metacharacter' +# The daemon inherits this process's environment. If RSYNC_MODULE_PATH happens +# to be set already, %RSYNC_MODULE_PATH% expands from the inherited value and +# the workaround half passes even when rsync never exported it -- the check +# would prove nothing. Poison it, so a pass means rsync overwrote it. +os.environ['RSYNC_MODULE_PATH'] = '/inherited-value-that-rsync-must-overwrite' + base = SCRATCHDIR / 'exec-metachar-documented-limit' rmtree(base) module = base / 'My Backups' # an ordinary directory name with a space @@ -83,5 +90,37 @@ if got != str(module): test_fail(f'the hook received {got!r} from the environment, not the module ' f'path {str(module)!r} -- the space did not survive') +# The manual lists the refused characters; only the space is exercised above, +# so the rest would silently become wrong if the set were narrowed. Each module +# path must EXIST and each daemon needs its own log: a path that is merely +# missing makes the transfer fail on its own, and checking only the exit status +# then passes whether or not the character was refused. +port = 12995 +for ch in ('*', '?', '[', ']', '#', '!', '~', '{', '}'): + mod = base / f'ch{ord(ch)}' / f'x{ch}y' + makepath(mod) + (mod / 'f.txt').write_text('DATA\n') + chlog = base / f'log{ord(ch)}' + c = write_daemon_conf([ + ('m', {'path': str(mod), 'read only': 'yes', + 'pre-xfer exec': "sh -c 'printf %s \"%RSYNC_MODULE_PATH%\" >/dev/null'"}), + ], globals={'pid file': str(base / f'pid{ord(ch)}'), 'log file': str(chlog)}, + name=f'exec-metachar-{ord(ch)}.conf') + u = start_test_daemon(c, port) + port += 1 + out = base / f'out{ord(ch)}' + makepath(out) + r = subprocess.run(rsync_argv('-r', f'{u}m/', f'{out}/'), + stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True) + if r.returncode == 0: + test_fail(f'a module path containing {ch!r} was interpolated into a hook ' + 'and served; rsyncd.conf(5) lists it as refused') + chlog_text = chlog.read_text(errors='replace') if chlog.is_file() else '' + if REFUSAL not in chlog_text: + test_fail(f'the transfer for {ch!r} failed, but not with the documented ' + f'refusal -- the module path exists, so something else went ' + f'wrong and this check proves nothing: {r.stdout.strip()[:200]!r}') + print('an operator value with a space is refused when interpolated, and works ' - 'when read from the environment, exactly as rsyncd.conf(5) says') + 'when read from the environment, exactly as rsyncd.conf(5) says; every ' + 'documented character is refused too') diff --git a/testsuite/daemon-exec-second-shell-argument-injection_test.py b/testsuite/daemon-exec-second-shell-argument-injection_test.py index d68b08e3..38342a83 100755 --- a/testsuite/daemon-exec-second-shell-argument-injection_test.py +++ b/testsuite/daemon-exec-second-shell-argument-injection_test.py @@ -1,12 +1,13 @@ #!/usr/bin/env python3 -"""A metacharacter-free peer value still selects a command in a nested shell. +"""A peer value with no EXECUTING character still selects a command. `pre-xfer exec = sh -c '.../%RSYNC_USER_NAME% ...'` re-parses the substituted word in a SECOND shell. rsync escapes for the quoting context it sees, which is correct for the outer shell only; the inner one gets the value bare. A username -of `touc?` therefore carries no character that can execute in one level of -parsing, yet the inner shell glob-expands `/touc?` to `/touch` and -runs it. The substitution has stopped being data and has chosen the command. +of `touc?` carries nothing that can execute in one level of parsing -- which +is why the original character set let it through -- yet the inner shell +glob-expands `/touc?` to `/touch` and runs it. ('?' is refused now, +which is what this test pins; it was not when the hole was found.) The substitution has stopped being data and has chosen the command. Two things this test has to avoid. @@ -23,6 +24,7 @@ than /usr/bin/touch -- a hardcoded system path would make the whole thing hinge on a binary the build never guaranteed. """ +import os import subprocess from rsyncfns import ( @@ -47,8 +49,9 @@ victim = bindir / 'touch' victim.write_text('#!/bin/sh\n: > "$1"\n') victim.chmod(0o755) -# No character from shell_unsafe_value()'s set appears in this username: it is -# data to the outer shell and a pathname expansion in the nested one. +# Data to the outer shell, a pathname expansion in the nested one. '?' IS in +# shell_unsafe_value()'s set today -- that is the point: this asserts it stays +# there. user = 'touc?' password = 'known-password' secrets = base / 'secrets' @@ -60,7 +63,11 @@ pwfile.chmod(0o600) # --- positive control: the attack must be live in this environment --------- control = base / 'control-marker' -subprocess.run(['sh', '-c', f"sh -c '{bindir}/touc? {control}'"], +# The OUTER shell must be the one rsync would use -- shell_exec() honours +# RSYNC_SHELL -- or the control can succeed on /bin/sh while rsync's hook path +# could not have run at all, and "the attack is live here" would be false. +outer = os.environ.get('RSYNC_SHELL') or '/bin/sh' +subprocess.run([outer, '-c', f"sh -c '{bindir}/touc? {control}'"], stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True) if not control.exists(): test_fail(f'the nested-shell glob did not select {victim} in this '