preventing shell injection

Hi,

The PostgreSQL security team has received complaints about various
parts of the PostgreSQL code that allow for shell injection. A typical
pattern is writing something like \"%s\" inside of a format string
where %s is an external input, and then passing the result to the
shell. This fairly obviously overlooks a number of ways in which
things could go wrong, such as the possibility that the value
interpolated via %s may itself contain double quotes. After some
study, the security team does not believe that these issues should be
treated as security vulnerabilities. However, that doesn't mean we
shouldn't improve the code or the documentation, so here are a bunch
of patches to do that.

The rationale for not treating these as vulnerabilities is outlined in
the commit message for 0001. I quoted the relevant part here for those
not wanting to plow through the attachments: For any currently-known
shell-injection opportunity to constitute a security vulnerability, an
attacker would need to lack direct access to the shell but be able to
control either (A) some but not all of the arguments being passed to
frontend utilities, or (B) pathnames to important filesystem locations
such as the PostgreSQL installation directory or the locations of
database clusters, or (C) the contents of environment variables passed
to affected PostgreSQL binaries. While such scenarios can't be
completely ruled out, they seem unlikely in practice. Users who may
have such scenarios are advised to carefully consider how inputs to
PostgreSQL-provided binaries are sanitized.

Said differently, and perhaps in plainer language, it's unlikely that
an attacker can arrange for you to find the PostgreSQL binaries at a
pathname of their choice. If they can do that, they can probably also
substitute the binaries you expect to find for others that do bad
things. It's also unlikely that an attacker can arrange for the path
to the data directory to be one which contains shell metacharacters;
if they can do that, they probably have local filesystem access
already. What is maybe slightly more likely is that an attacker might
lack direct shell access but be able to induce you to run PostgreSQL
frontend tools with arguments partially chosen by them. Of particular
concern is pg_ctl -o. The argument to -o is interpreted by the shell
by design: the documentation offers suggestions such as

pg_ctl -o "-F -p 5433" restart

which can't work unless the shell is going to parse "-F -p 5433" to
split it apart into words. But if the shell is going to parse that,
then you could also include other shell metacharacters in there. My
guess is that some users are doing exactly that, so I don't think we
can get away with changing the behavior here in a minor release, even
we all agreed on how to change it, which I'm almost 100% sure we would
not. This means that it's a bad idea to let your adversaries choose
values for you to pass via pg_ctl's -o option, or via the
closely-related pg_upgrade options -o/--old-options or
-O/--new-options, so 0007 in this patch series documents that you
should not do that. The preceding patches, 0001-0006, add proper
escaping in various places that lack it, and are grouped by the type
of escaping required and by how far I think we can back-patch without
risk of breaking existing installations. Finally, 0008 removes some
escaping-adjacent code that is mostly redundant; see the commit
message for why the code in question is not entirely redundant.

This patch set is not comprehensive. Here are some other, similar
things that we might also want to fix:

- pg_upgrade writes out delete_old_cluster.{sh,bat} without proper
shell escaping. Fixing this on non-Windows looks easy, but fixing it
on Windows appears more complicated because Windows escaping in batch
files apparently follows slightly different rules than in some other
contexts. I am not even sure this can be made 100% robust on Windows.

- The on-disk format postmaster.opts is poorly chosen. The file is
written by CreateOptsFile() and later read by read_post_opts(), but
CreateOptsFile() just writes the full program name (without escaping)
and then, for each argument, writes a space, a double-quote, the
argument value, and another double quote. This is not robust against
embedded double quotes in either the program name or the arguments,
and it's possible to construct cases where the server is started with
one set of options and then after a restart ends up running with a
different set of options. read_post_opts() isn't making anything
better: it looks for space-double-quote to identify the place where
exec_path ends and post_opts begin, so this is not as simple as just
changing the file-writing code to use appendShellString() or the new
appendStringInfoShell. The file format needs to change on disk, and
it's not clear exactly what to pick.

- I think the choice to design pg_ctl with a -o option and pg_upgrade
with -o and -O options whose arguments are passed through the shell by
design is questionable. A better design would be one that doesn't rely
on the shell in the first place, and doesn't use it. We could cover an
awful lot of the practical use cases here by inventing pg_ctl --set,
similar to what initdb already supports; unfortunately, we can't allow
-c as a short option there, as postgres and initdb do, because pg_ctl
-c means --core-files, though maybe it would be worth introducing an
incompatibility and changing the short form of --core-files to -C or
removing the short form altogether. However, just having --set, with
or without a -c shorthand, doesn't cover everything; if we want to
have a comprehensive replacement for pg_ctl -o, we need something like
pg_ctl --program-arg (name to be workshopped), that is defined as
passing exactly one argument through to whichever of postgres and
initdb it ends up executing. (Note that you can't fix this issue in a
satisfying way without fixing the preceding issue first.)

- pg_regress is not touched by these patches but is naive about
escaping, in psql_start_command and psql_end_command among other
places.

--
Robert Haas
EDB: http://www.enterprisedb.com