mirror of
https://github.com/RsyncProject/rsync.git
synced 2026-09-16 23:27:54 -04:00
Andrew Tridgell
05bd16a469
Context-aware quoting is only correct for one level of shell parsing. A
hook may re-parse the substituted word in a nested shell:
pre-xfer exec = sh -c 'printf %s %RSYNC_USER_NAME% >out'
The level-1 quotes are removed before the inner shell sees the value, so
an authenticated peer's username still reaches it as syntax however
carefully it was escaped. Escaping cannot fix this; refuse instead.
A %RSYNC_*% value substituted into a shell-executed hook (early exec,
name converter, pre-/post-xfer exec) is now rejected if it holds any
character that can become shell syntax in any context: quote, backtick,
dollar, backslash, semicolon, ampersand, pipe, redirection, parenthesis,
or a control character. Word-splitting and glob characters are left
alone -- they cannot execute anything and paths legitimately contain
them. The refusal is fail-closed and logged: a hook may be an access
check, so silently skipping it is not an option.
Also fix the quote tracker itself, which moved to SHELL_SINGLE_QUOTED on
an apostrophe even inside "...", where it is an ordinary character. That
made a value in `printf %s "it's %RSYNC_USER_NAME%"` escape for the wrong
context. With the refusal above this is defence in depth, and it matters
if the refused set is ever narrowed.
The two existing hook-injection tests asserted that a metacharacter value
was quoted and the transfer still succeeded; both now expect the refusal.
85 lines
2.6 KiB
Python
85 lines
2.6 KiB
Python
#!/usr/bin/env python3
|
|
"""PoC: shell-escaped %RSYNC_USER_NAME% is unsafe inside single quotes.
|
|
|
|
expand_vars_shell_escape() produces a shell word of the form 'value'. That is
|
|
safe when substituted into an unquoted command, but a common hook such as
|
|
|
|
pre-xfer exec = printf '%RSYNC_USER_NAME%' >/dev/null
|
|
|
|
already supplies single quotes. The generated quotes cancel those quotes and
|
|
leave an authenticated client's username in shell syntax context.
|
|
"""
|
|
|
|
import os
|
|
import subprocess
|
|
from pathlib import Path
|
|
|
|
from rsyncfns import (
|
|
FROMDIR, SCRATCHDIR, make_tree, makepath, rmtree, rsync_argv,
|
|
start_test_daemon, test_fail, write_daemon_conf,
|
|
)
|
|
|
|
PORT = 12939
|
|
|
|
src = FROMDIR
|
|
rmtree(src)
|
|
make_tree(src, depth=1)
|
|
dest = SCRATCHDIR / 'exec-singlequote-dest'
|
|
rmtree(dest)
|
|
makepath(dest)
|
|
|
|
# A slash would be interpreted as the URL's module separator before the client
|
|
# sends the username, so create the marker in the daemon's inherited cwd.
|
|
sentinel = Path.cwd() / 'exec-singlequote-pwned'
|
|
if sentinel.exists():
|
|
sentinel.unlink()
|
|
|
|
# No spaces or ':' are needed in the wire username. ${IFS} becomes a shell
|
|
# separator only after the broken quote composition exposes the value.
|
|
user = f';touch${{IFS}}{sentinel.name};true'
|
|
password = 'known-password'
|
|
|
|
secrets = SCRATCHDIR / 'exec-singlequote.secrets'
|
|
secrets.write_text(f'{user}:{password}\n')
|
|
secrets.chmod(0o600)
|
|
pwfile = SCRATCHDIR / 'exec-singlequote.password'
|
|
pwfile.write_text(password + '\n')
|
|
pwfile.chmod(0o600)
|
|
|
|
conf = write_daemon_conf([
|
|
('hook', {
|
|
'path': str(dest),
|
|
'read only': 'no',
|
|
'use chroot': 'no',
|
|
'auth users': '*',
|
|
'secrets file': str(secrets),
|
|
'pre-xfer exec': "printf '%RSYNC_USER_NAME%' >/dev/null",
|
|
}),
|
|
], name='exec-singlequote.conf')
|
|
url = start_test_daemon(conf, PORT)
|
|
|
|
os.environ['RSYNC_PASSWORD'] = 'wrong-environment-fallback'
|
|
user_url = url.replace('rsync://', f'rsync://{user}@', 1) + 'hook/'
|
|
proc = subprocess.run(
|
|
rsync_argv('-r', f'--password-file={pwfile}', f'{src}/', user_url),
|
|
stdout=subprocess.PIPE,
|
|
stderr=subprocess.PIPE,
|
|
text=True,
|
|
)
|
|
|
|
if sentinel.exists():
|
|
test_fail(
|
|
"authenticated metacharacter username executed through a "
|
|
f"single-quoted %RSYNC_USER_NAME% hook (rc={proc.returncode}):\n"
|
|
f"{proc.stderr}"
|
|
)
|
|
# The username carries shell syntax, so the daemon refuses it rather than
|
|
# trying to quote it; the transfer must fail closed.
|
|
if proc.returncode == 0:
|
|
test_fail(
|
|
"daemon accepted an authenticated username holding shell syntax "
|
|
f"(rc={proc.returncode}):\n{proc.stderr}"
|
|
)
|
|
|
|
print("single-quoted daemon hook refused a username holding shell syntax")
|