From c1d8cc1c04f746a3d4922e028f917543c7417247 Mon Sep 17 00:00:00 2001 From: Andrew Tridgell Date: Sun, 2 Aug 2026 21:30:22 +1000 Subject: [PATCH] socket: refuse a meaning-changing first character, and stop refusing aliases The %H allow-list was wrong in both directions. Several accepted characters change an argument's MEANING rather than its text when they lead the value, which quoting cannot prevent because the word stays intact -- that IS the problem: '-' and '+' introduce options to plenty of programs; with RSYNC_CONNECT_PROG="prog %H", hosts "-c" and "+x" arrive as options, and "sh +x" is as real as "sh -x"; '~' is tilde-expanded by the nested shell, turning ~root into /root; '%' is expanded by a nested fish, where %self becomes its pid. An empty host has the same shape from the other end: it survives a direct exec as an empty argument but disappears when a nested shell re-splits the command, shifting everything after it. rsync://:873/m/ and ::m/ both produce one. None of these can begin a real hostname, so refuse them in first position only -- mid-word each is literal, which matters because an IPv6 zone id carries its '%' mid-word. The other direction: '+' and '~' were refused outright. They execute nothing, and RSYNC_CONNECT_PROG exists for custom transports where %H is often an alias the program resolves itself rather than a name the resolver sees. Refusing them mid-word breaks that use case for no gain. A non-ASCII host stays refused. That is a policy choice rather than a free one: a custom connect program never calls getaddrinfo, so a Unicode alias would otherwise work, and this does exclude it. A punycode A-label is unaffected. What this cannot do is bound what the named program makes of the value. "host:-rf" arrives intact, and a program that splits on ':' may reinterpret the tail; that boundary belongs to whoever writes the command. The test asserted only that a marker file was absent -- equally true when rsync failed to parse its arguments, when socketpair_tcp is blocked, or when touch was missing. Worse, the marker path was absolute, and a URL authority ends at the first '/', so the injected `touch` never received an operand and the check could not fail even with the guard gone. It now runs with cwd set to the scratch directory and injects a bare name, so the marker is genuinely reachable; requires the specific refusal message; checks the exit status; and checks that ordinary hosts still arrive at the connect program with their text intact, which is the part an absence-only test can never show. Fault-injected separately: dropping the guard, dropping just the first-character check, and narrowing the set back each fail the test on their own, in both the default and --use-tcp transports. (cherry picked from commit dcc3c9c51ad91156fd2995bad3f0e6f9a187d58b) # Conflicts: # testsuite/connect-prog-nested-singlequote-host-injection_test.py --- socket.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/socket.c b/socket.c index 4fad270b..54015150 100644 --- a/socket.c +++ b/socket.c @@ -172,13 +172,45 @@ static char *shell_quote_connect_host(const char *host) return quoted; } +/* %H is substituted into a free-form command string that a shell then runs, and + * a hook of the form `sh -c '... %H ...'` re-parses the word in a nested shell + * where the quoting we added has already been consumed. Restrict the host to + * characters that stay data through such a pass. ('Any shell' is too strong a + * claim -- see the leading-'%' note below.) + * + * The set is deliberately a little wider than a DNS name: RSYNC_CONNECT_PROG + * exists for custom transports, where %H may be an alias the program resolves + * itself rather than a name the resolver ever sees, so '+' and '~' are allowed + * inside the string. '%' is required for an IPv6 zone id (fe80::1%eth0) and is + * not special to a POSIX shell. + * + * The first character is checked separately, because several of the allowed + * ones change an argument's MEANING rather than its text, which no amount of + * quoting prevents. Mid-word each is literal, which is why the set still + * allows them: + * '-' and '+' both introduce options to plenty of programs -- `sh +x` is as + * real as `sh -x`; + * '~' is tilde-expanded by the nested shell, turning ~root into /root; + * '%' is expanded by a nested fish, where %self becomes its pid (an IPv6 + * zone id has its '%' mid-word, so nothing legitimate is lost). + * An empty host is refused too: it survives a direct exec as an empty argument + * but vanishes entirely when a nested shell re-splits the command, shifting + * every argument after it. None of these can begin a real hostname. + * + * This bounds what a SHELL can do with the value. It cannot bound what the + * named program does with it -- something like "host:-rf" arrives intact, and a + * program that splits on ':' may reinterpret the tail. That boundary belongs + * to whoever writes RSYNC_CONNECT_PROG. */ static int shell_unsafe_connect_host(const char *host) { const unsigned char *s; + if (!*host || strchr("-+~%", *host)) + return 1; + for (s = (const unsigned char *)host; *s; s++) { if (!((*s >= 'a' && *s <= 'z') || (*s >= 'A' && *s <= 'Z') - || (*s >= '0' && *s <= '9') || strchr("._:-%", *s))) + || (*s >= '0' && *s <= '9') || strchr("._:-%+~", *s))) return 1; } return 0;