Re: preventing shell injection

> On Aug 11, 2026, at 00:04, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> 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
> <v1-0003-pg_upgrade-Fix-improper-shell-escaping-for-pass-t.patch><v1-0001-Fix-improper-shell-escaping-in-various-frontend-u.patch><v1-0005-Fix-improper-shell-escaping-in-code-not-using-lib.patch><v1-0002-pg_createsubscriber-Fix-improper-shell-escaping.patch><v1-0004-stringinfo-Add-helpers-for-shell-quoting.patch><v1-0006-Rationalize-argv-escaping-on-Windows.patch><v1-0008-Remove-postgres_exec_path.patch><v1-0007-Document-that-pg_ctl-o-and-pg_upgrade-o-O-use-the.patch>

The patch overall looks good to me. I just have a few small comments, some of them might only be suspicions.

1 - 0001 - exec.c
```
@@ -416,7 +418,7 @@ check_exec(const char *dir, const char *program, bool check_version)
if (validate_exec(path) != 0)
pg_fatal("check for \"%s\" failed: %m", path);

- snprintf(cmd, sizeof(cmd), "\"%s\" -V", path);
+ snprintf(cmd, sizeof(cmd), "%s -V", quote_shell_arg(path));
```

This is not a concrete comment, but a suspicion. Both cmd and path are char arrays of size MAXPGPATH, and MAXPGPATH is 1024. quote_shell_arg() may add extra characters, for example, an embedded single quote becomes 5 bytes ('\"'\"'), so 4 extra bytes are added per quote. Therefore, potentially, the quoted path might be longer than MAXPGPATH, but we are using snprintf() to build cmd, so the command might be silently truncated.

This is not a new problem introduced by this patch. The current code also adds a few extra characters. This patch just adds more extra characters, making the truncation risk more likely.

2 - 0001 - exec.c, in the same function as comment 1

quote_shell_arg() returns palloc'd memory, but it is not freed. Usually this should not be a problem wrt memory leaks. But in this function, line is freed, which leads to inconsistent memory-freeing patterns. I’m not sure whether this is a concern.

3 - 0004 - stringinfo.c
```
+#ifndef FRONTEND
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("shell command argument contains a newline or carriage return: \"%s\"",
+ s)));
+#else
```

Nit: I think the extra parentheses around errcode and errmsg are no longer needed. The modern style is to omit them.

4 - 0006 - stringinfo.c

The same nit comment as 3.

5 - 0006 - pg_ctl.c - pgwin32_CommandLine()
```
if (post_opts)
{
if (registration)
- appendPQExpBuffer(cmdLine, " -o \"%s\"", post_opts);
+ appendStringInfo(&cmdLine, " -o \"%s\"", post_opts);
else
- appendPQExpBuffer(cmdLine, " %s", post_opts);
+ appendStringInfo(&cmdLine, " %s", post_opts);
}
```

I didn’t test this on Windows, but I wonder whether we should use appendStringInfoWin32Argv() for post_opts here.

I also ran “make check-world” and it passed.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/