daemon: refuse '!', '~' and braces in a hook expansion too

The refused set was built from the characters that obviously execute
something, and missed three that a SECOND shell acts on:

  '!' negates in command position.  A hook written as an access check --
      `pre-xfer exec = sh -c '%RSYNC_USER_NAME% false'` -- becomes
      `! false`, reports success, and serves the transfer.  An
      authenticated user named "!" turns a denial into an approval, which
      is precisely the case the fail-closed comment above exists for.
  '~' is tilde-expanded, so ~root becomes /root.
  '{' and '}' brace-expand in bash and zsh.

None of them execute anything on their own, which is how a set built from
the obvious metacharacters came to miss them.  That is also the standing
weakness of the approach: this is a deny-list, and the two rounds of
review it took to find '!' are the argument for eventually inverting it.

The documentation is corrected with it -- it claimed every shell-active
character was refused, which this disproves -- and now lists the set.
Each listed character is pinned by the test, which needed its module
paths to EXIST first: a missing path fails the transfer on its own, so
checking the exit status alone passed whether or not the character was
refused.  Removing any single character from the set now fails the test.

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;
 	}

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

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')

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 `<dir>/touc?` to `<dir>/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 `<dir>/touc?` to `<dir>/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 '