From 79e20bc12d6269f076456c8ae01fa12e398b707e Mon Sep 17 00:00:00 2001 From: Andrew Tridgell Date: Sun, 2 Aug 2026 21:23:58 +1000 Subject: [PATCH] daemon: refuse peer values holding shell syntax in shell hooks 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. (cherry picked from commit 05bd16a469f089c73d4a54a9bdbce816029f5fe9) # Conflicts: # testsuite/daemon-exec-rsync-env-shell-escape_test.py # testsuite/daemon-exec-singlequote-injection_test.py --- loadparm.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 46 insertions(+), 4 deletions(-) diff --git a/loadparm.c b/loadparm.c index cdaea67d..135e6593 100644 --- a/loadparm.c +++ b/loadparm.c @@ -170,10 +170,35 @@ enum shell_quote_context { SHELL_DOUBLE_QUOTED }; +/* Characters that can turn a substituted value into shell syntax rather than + * data, in any quoting context. Quoting alone cannot be relied on here: + * context-aware escaping is correct for exactly one level of shell parsing, + * and a hook such as `sh -c '... %RSYNC_USER_NAME% ...'` re-parses the word in + * a second shell that sees the value bare. Peer-supplied values carrying any + * of these are refused instead. Word-splitting and glob characters are left + * alone: they cannot execute anything, and paths legitimately contain them. */ +static int shell_unsafe_value(const char *val) +{ + const char *s; + + for (s = val; *s; s++) { + if (strchr("'\"`$\\;&|<>()", *s) + || (unsigned char)*s < 0x20 || (unsigned char)*s == 0x7f) + return 1; + } + return 0; +} + static char *expand_vars_shell_escape(const char *val, int quote_context) { const char *s; char *ret, *t; + /* A double-quoted value is deliberately BOTH backslash-escaped and + * wrapped in single quotes. The wrap is redundant for one level of + * shell parsing (and shows up as literal quotes in the value), but a + * hook such as `sh -c "... %RSYNC_USER_NAME% ..."` re-parses the word + * in a second shell, where the backslashes are already gone and only + * the quotes still protect it. */ size_t len = quote_context == SHELL_SINGLE_QUOTED ? 0 : 2; for (s = val; *s; s++) { @@ -233,8 +258,17 @@ static char *expand_vars(const char *str, int shell_escape) * params (path, uid, gid, ...) leave them verbatim -- * quoting there would corrupt the value (e.g. a documented * `path = /home/%RSYNC_USER_NAME%` would become /home/'x'). */ - if (shell_escape && strncmp(t, "RSYNC_", 6) == 0) + if (shell_escape && strncmp(t, "RSYNC_", 6) == 0) { + if (shell_unsafe_value(val)) { + /* Fail closed: the hook may be an access + * check, so skipping it is not an option. */ + rprintf(FLOG, + "refusing to run shell hook: %%%s%% holds a shell metacharacter\n", + t); + exit_cleanup(RERR_UNSUPPORTED); + } val = escaped = expand_vars_shell_escape(val, quote_context); + } len = strlcpy(t, val, bufsize+1); if (escaped) free(escaped); @@ -249,17 +283,25 @@ static char *expand_vars(const char *str, int shell_escape) } if (shell_escape) { if (quote_context == SHELL_SINGLE_QUOTED) { + /* Nothing is special inside '...', not even a backslash; + * only the closing quote ends it. */ if (*f == '\'') quote_context = SHELL_UNQUOTED; } else if (escaped_char) escaped_char = 0; else if (*f == '\\') escaped_char = 1; - else if (*f == '\'') + else if (quote_context == SHELL_DOUBLE_QUOTED) { + /* A single quote inside "..." is literal and must not be + * taken as opening a single-quoted run -- doing so would + * de-sync the tracker and escape a later value for the + * wrong context. */ + if (*f == '"') + quote_context = SHELL_UNQUOTED; + } else if (*f == '\'') quote_context = SHELL_SINGLE_QUOTED; else if (*f == '"') - quote_context = quote_context == SHELL_DOUBLE_QUOTED - ? SHELL_UNQUOTED : SHELL_DOUBLE_QUOTED; + quote_context = SHELL_DOUBLE_QUOTED; } *t++ = *f++; bufsize--;