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

# Conflicts:
#	testsuite/connect-prog-nested-singlequote-host-injection_test.py
This commit is contained in:
Andrew Tridgell committed 2026-08-02 21:30:22 +10:00
1 parent 10c3b361f6
commit c1d8cc1c04
1 file changed
+33 -1
+33 -1
View File
@@ -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;