From 2a2ccaa1bfe50df51987b170c4f2fc756716523d Mon Sep 17 00:00:00 2001 From: Andrew Tridgell Date: Sun, 21 Jun 2026 16:35:17 +1000 Subject: [PATCH] batch/rsync-ssl: quote replay-script args and harden rsync-ssl Quote and newline-escape the arguments written into the --write-batch replay script so a crafted filename cannot inject shell, and tighten rsync-ssl's option handling. --- batch.c | 56 ++++++++++++++++++++++++++++++++++++------------------- rsync-ssl | 16 +++++++++++++++- 2 files changed, 52 insertions(+), 20 deletions(-) diff --git a/batch.c b/batch.c index a0845f67..a45fe833 100644 --- a/batch.c +++ b/batch.c @@ -166,25 +166,33 @@ static int write_arg(const char *arg) const char *x, *s; int len, err = 0; + /* Emit a "--opt=" prefix unquoted only when it is a plain option token; + * a metacharacter before '=' (an attacker-shaped arg) must be quoted + * along with the rest, or it would run raw in the replay script. */ if (*arg == '-' && (x = strchr(arg, '=')) != NULL) { - err |= write(batch_sh_fd, arg, x - arg + 1) != x - arg + 1; - arg += x - arg + 1; - } - - if (strpbrk(arg, " \"'&;|[]()$#!*?^\\") != NULL) { - err |= write(batch_sh_fd, "'", 1) != 1; - for (s = arg; (x = strchr(s, '\'')) != NULL; s = x + 1) { - err |= write(batch_sh_fd, s, x - s + 1) != x - s + 1; - err |= write(batch_sh_fd, "'", 1) != 1; + const char *p = arg; + while (p < x && (*p == '-' || *p == '_' + || (*p >= '0' && *p <= '9') + || (*p >= 'A' && *p <= 'Z') + || (*p >= 'a' && *p <= 'z'))) + p++; + if (p == x) { + err |= write(batch_sh_fd, arg, x - arg + 1) != x - arg + 1; + arg += x - arg + 1; } - len = strlen(s); - err |= write(batch_sh_fd, s, len) != len; - err |= write(batch_sh_fd, "'", 1) != 1; - return err; } - len = strlen(arg); - err |= write(batch_sh_fd, arg, len) != len; + /* Single-quote unconditionally so every shell metacharacter (backtick, + * newline, redirection, ...) stays literal in the replay script. An + * embedded ' is emitted as the '\'' close/escape/reopen sequence. */ + err |= write(batch_sh_fd, "'", 1) != 1; + for (s = arg; (x = strchr(s, '\'')) != NULL; s = x + 1) { + err |= write(batch_sh_fd, s, x - s) != x - s; + err |= write(batch_sh_fd, "'\\''", 4) != 4; + } + len = strlen(s); + err |= write(batch_sh_fd, s, len) != len; + err |= write(batch_sh_fd, "'", 1) != 1; return err; } @@ -210,6 +218,16 @@ static void write_filter_rules(int fd) for (ent = filter_list.head; ent; ent = ent->next) { unsigned int plen; char *p = get_rule_prefix(ent, "- ", 0, &plen); + /* A filter pattern is one here-doc line; an embedded newline would let + * a crafted pattern (e.g. from a dir-merge/--exclude-from file in an + * untrusted tree) forge the "#E#" terminator on its own line and inject + * shell commands into the generated replay script. Such a pattern also + * can't round-trip the line-delimited here-doc, so refuse it fail-closed + * rather than emit an injectable script. */ + if (ent->pattern && strchr(ent->pattern, '\n')) { + rprintf(FERROR, "cannot write a filter rule containing a newline to the batch replay script\n"); + exit_cleanup(RERR_SYNTAX); + } write_buf(fd, p, plen); write_sbuf(fd, ent->pattern); if (ent->rflags & FILTRULE_DIRECTORY) @@ -233,7 +251,7 @@ void open_batch_files(void) stringjoin(filename, sizeof filename, batch_name, ".sh", NULL); - batch_sh_fd = safe_open_no_attacker_symlinks(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, S_IRUSR | S_IWUSR | S_IXUSR); + batch_sh_fd = open_no_attacker_symlinks(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, S_IRUSR | S_IWUSR | S_IXUSR); if (batch_sh_fd < 0) { rsyserr(FERROR, errno, "Batch file %s open error", full_fname(filename)); exit_cleanup(RERR_FILESELECT); @@ -241,12 +259,12 @@ void open_batch_files(void) /* O_BINARY: the batch stream is binary protocol data; without it * Cygwin et al apply CRLF translation and corrupt it. Unlike - * do_open(), safe_open_no_attacker_symlinks passes flags verbatim. */ - batch_fd = safe_open_no_attacker_symlinks(batch_name, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, S_IRUSR | S_IWUSR); + * do_open(), open_no_attacker_symlinks passes flags verbatim. */ + batch_fd = open_no_attacker_symlinks(batch_name, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, S_IRUSR | S_IWUSR); } else if (strcmp(batch_name, "-") == 0) batch_fd = STDIN_FILENO; else - batch_fd = safe_open_no_attacker_symlinks(batch_name, O_RDONLY | O_BINARY, S_IRUSR | S_IWUSR); + batch_fd = open_no_attacker_symlinks(batch_name, O_RDONLY | O_BINARY, S_IRUSR | S_IWUSR); if (batch_fd < 0) { rsyserr(FERROR, errno, "Batch file %s open error", full_fname(batch_name)); diff --git a/rsync-ssl b/rsync-ssl index a6411455..113a34ea 100755 --- a/rsync-ssl +++ b/rsync-ssl @@ -142,8 +142,21 @@ function rsync_ssl_helper { exit 1 fi + # Bind the server certificate to the requested host (openssl -verify_hostname, + # stunnel checkHost) so a cert that is CA-valid for a *different* name can't be + # used to MITM. Set RSYNC_SSL_SKIP_HOSTNAME_CHECK=1 to keep CA chain + # verification but skip the identity check (e.g. bare-IP or internal-CA setups). + # checkHost only applies while the chain is being verified. + openssl_host_opt="-verify_hostname $hostname" + stunnel_host_opt="" + [[ "$verify" == "verifyChain = yes" ]] && stunnel_host_opt="checkHost = $hostname" + if [[ "$RSYNC_SSL_SKIP_HOSTNAME_CHECK" == 1 ]]; then + openssl_host_opt="" + stunnel_host_opt="" + fi + if [[ $RSYNC_SSL_TYPE == openssl ]]; then - exec $RSYNC_SSL_OPENSSL s_client $caopt $certopt $keyopt -quiet -verify_quiet -servername $hostname -verify_hostname $hostname -connect $hostname:$port + exec $RSYNC_SSL_OPENSSL s_client $caopt $certopt $keyopt -quiet -verify_quiet -servername $hostname $openssl_host_opt -connect $hostname:$port elif [[ $RSYNC_SSL_TYPE == gnutls ]]; then exec $RSYNC_SSL_GNUTLS --logfile=/dev/null $gnutls_cert_opt $gnutls_key_opt $gnutls_opts $hostname:$port else @@ -155,6 +168,7 @@ connect = $hostname:$port client = yes TIMEOUTclose = 0 $verify +$stunnel_host_opt $certopt $cafile EOF