From bce48856b0af8ec2d4e019f9ca9f22ae72b58743 Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Fri, 24 Jul 2026 08:53:54 -0400 Subject: [PATCH v1 5/8] Fix improper shell-escaping in code not using libpq. Commit XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX added shell-escaping functions for StringInfo, which, because it is part of src/common, is available practically everywhere. This allows us to fix shell-escaping deficiencies that can't be cleanly fixed using PQExpBuffer. This commit fixes the following problems: (1) find_other_exec(), which is used by assorted frontend utilities, and also by postgres itself on Windows systems, ran the other binary with the -V option without properly escaping the path to that binary; (2) pg_verifybackup ran pg_waldump without properly escaping the path to pg_waldump and the path to the pg_wal directory; and (3) pg_ctl ran initdb and postgres without properly escaping the path to the corresponding binary, the data directory location specified via -D, or the log file location specified via -l. We regard this as a robustness fix, defending against pathnames containing unusual characters, rather than as a fix for a security vulnerability. See commit XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX for a more detailed rationale. Backpatch-through: 14 --- src/bin/pg_ctl/pg_ctl.c | 80 ++++++++++++++--------- src/bin/pg_verifybackup/pg_verifybackup.c | 20 ++++-- src/common/exec.c | 11 +++- 3 files changed, 70 insertions(+), 41 deletions(-) diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c index 6c604e2d962..d99f3c93ca1 100644 --- a/src/bin/pg_ctl/pg_ctl.c +++ b/src/bin/pg_ctl/pg_ctl.c @@ -28,6 +28,7 @@ #include "common/string.h" #include "datatype/timestamp.h" #include "getopt_long.h" +#include "lib/stringinfo.h" #include "utils/pidfile.h" #ifdef WIN32 /* on Unix, we don't need libpq */ @@ -442,7 +443,7 @@ free_readfile(char **optlines) static pid_t start_postmaster(void) { - char *cmd; + StringInfoData cmd; #ifndef WIN32 pid_t pm_pid; @@ -489,15 +490,18 @@ start_postmaster(void) * everything to a shell to process them. Use exec so that the postmaster * has the same PID as the current child process. */ + initStringInfo(&cmd); + appendStringInfoString(&cmd, "exec "); + appendStringInfoShell(&cmd, exec_path); + appendStringInfo(&cmd, " %s%s < \"%s\"", pgdata_opt, post_opts, DEVNULL); if (log_file != NULL) - cmd = psprintf("exec \"%s\" %s%s < \"%s\" >> \"%s\" 2>&1", - exec_path, pgdata_opt, post_opts, - DEVNULL, log_file); - else - cmd = psprintf("exec \"%s\" %s%s < \"%s\" 2>&1", - exec_path, pgdata_opt, post_opts, DEVNULL); + { + appendStringInfoString(&cmd, " >> "); + appendStringInfoShell(&cmd, log_file); + } + appendStringInfoString(&cmd, " 2>&1"); - (void) execl("/bin/sh", "/bin/sh", "-c", cmd, (char *) NULL); + (void) execl("/bin/sh", "/bin/sh", "-c", cmd.data, (char *) NULL); /* exec failed */ write_stderr(_("%s: could not start server: %m\n"), @@ -555,15 +559,20 @@ start_postmaster(void) } else close(fd); + } - cmd = psprintf("\"%s\" /C \"\"%s\" %s%s < \"%s\" >> \"%s\" 2>&1\"", - comspec, exec_path, pgdata_opt, post_opts, DEVNULL, log_file); + initStringInfo(&cmd); + appendStringInfo(&cmd, "\"%s\" /C ", comspec); + appendStringInfoShell(&cmd, exec_path); + appendStringInfo(&cmd, " %s%s < \"%s\"", pgdata_opt, post_opts, DEVNULL); + if (log_file != NULL) + { + appendStringInfoString(&cmd, " >> "); + appendStringInfoShell(&cmd, log_file); } - else - cmd = psprintf("\"%s\" /C \"\"%s\" %s%s < \"%s\" 2>&1\"", - comspec, exec_path, pgdata_opt, post_opts, DEVNULL); + appendStringInfoString(&cmd, " 2>&1"); - if (!CreateRestrictedProcess(cmd, &pi, false)) + if (!CreateRestrictedProcess(cmd.data, &pi, false)) { write_stderr(_("%s: could not start server: error code %lu\n"), progname, GetLastError()); @@ -904,7 +913,7 @@ find_other_exec_or_die(const char *argv0, const char *target, const char *versio static void do_init(void) { - char *cmd; + StringInfoData cmd; if (exec_path == NULL) exec_path = find_other_exec_or_die(argv0, "initdb", "initdb (PostgreSQL) " PG_VERSION "\n"); @@ -915,15 +924,14 @@ do_init(void) if (post_opts == NULL) post_opts = ""; - if (!silent_mode) - cmd = psprintf("\"%s\" %s%s", - exec_path, pgdata_opt, post_opts); - else - cmd = psprintf("\"%s\" %s%s > \"%s\"", - exec_path, pgdata_opt, post_opts, DEVNULL); + initStringInfo(&cmd); + appendStringInfoShell(&cmd, exec_path); + appendStringInfo(&cmd, " %s%s", pgdata_opt, post_opts); + if (silent_mode) + appendStringInfo(&cmd, " > \"%s\"", DEVNULL); fflush(NULL); - if (system(cmd) != 0) + if (system(cmd.data) != 0) { write_stderr(_("%s: database system initialization failed\n"), progname); exit(1); @@ -2125,9 +2133,9 @@ static void adjust_data_dir(void) { char filename[MAXPGPATH]; - char *my_exec_path, - *cmd; + char *my_exec_path; FILE *fd; + StringInfoData cmd; /* do nothing if we're working without knowledge of data dir */ if (pg_config == NULL) @@ -2155,17 +2163,22 @@ adjust_data_dir(void) else my_exec_path = pg_strdup(exec_path); + initStringInfo(&cmd); + appendStringInfoShell(&cmd, my_exec_path); + /* it's important for -C to be the first option, see main.c */ - cmd = psprintf("\"%s\" -C data_directory %s%s", - my_exec_path, - pgdata_opt ? pgdata_opt : "", - post_opts ? post_opts : ""); + appendStringInfoString(&cmd, " -C data_directory "); + if (pgdata_opt) + appendStringInfoString(&cmd, pgdata_opt); + if (post_opts) + appendStringInfoString(&cmd, post_opts); + fflush(NULL); - fd = popen(cmd, "r"); + fd = popen(cmd.data, "r"); if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL || pclose(fd) != 0) { - write_stderr(_("%s: could not determine the data directory using command \"%s\"\n"), progname, cmd); + write_stderr(_("%s: could not determine the data directory using command \"%s\"\n"), progname, cmd.data); exit(1); } pg_free(my_exec_path); @@ -2278,6 +2291,7 @@ main(int argc, char **argv) case 'D': { char *pgdata_D; + StringInfoData buf; pgdata_D = pg_strdup(optarg); canonicalize_path(pgdata_D); @@ -2287,7 +2301,11 @@ main(int argc, char **argv) * We could pass PGDATA just in an environment variable * but we do -D too for clearer postmaster 'ps' display */ - pgdata_opt = psprintf("-D \"%s\" ", pgdata_D); + initStringInfo(&buf); + appendStringInfoString(&buf, "-D "); + appendStringInfoShell(&buf, pgdata_D); + appendStringInfoChar(&buf, ' '); + pgdata_opt = buf.data; pg_free(pgdata_D); break; } diff --git a/src/bin/pg_verifybackup/pg_verifybackup.c b/src/bin/pg_verifybackup/pg_verifybackup.c index 81694144b46..c8f8f703a18 100644 --- a/src/bin/pg_verifybackup/pg_verifybackup.c +++ b/src/bin/pg_verifybackup/pg_verifybackup.c @@ -24,6 +24,7 @@ #include "common/parse_manifest.h" #include "fe_utils/simple_list.h" #include "getopt_long.h" +#include "lib/stringinfo.h" #include "pg_verifybackup.h" #include "pgtime.h" @@ -1235,17 +1236,22 @@ parse_required_wal(verifier_context *context, char *pg_waldump_path, while (this_wal_range != NULL) { - char *pg_waldump_cmd; - - pg_waldump_cmd = psprintf("\"%s\" --quiet --path=\"%s\" --timeline=%u --start=%X/%08X --end=%X/%08X\n", - pg_waldump_path, wal_path, this_wal_range->tli, - LSN_FORMAT_ARGS(this_wal_range->start_lsn), - LSN_FORMAT_ARGS(this_wal_range->end_lsn)); + StringInfoData buf; + + initStringInfo(&buf); + appendStringInfoShell(&buf, pg_waldump_path); + appendStringInfoString(&buf, " --quiet --path="); + appendStringInfoShell(&buf, wal_path); + appendStringInfo(&buf, " --timeline=%u --start=%X/%08X --end=%X/%08X", + this_wal_range->tli, + LSN_FORMAT_ARGS(this_wal_range->start_lsn), + LSN_FORMAT_ARGS(this_wal_range->end_lsn)); fflush(NULL); - if (system(pg_waldump_cmd) != 0) + if (system(buf.data) != 0) report_backup_error(context, "WAL parsing failed for timeline %u", this_wal_range->tli); + pfree(buf.data); this_wal_range = this_wal_range->next; } diff --git a/src/common/exec.c b/src/common/exec.c index 2881aa92ca6..dd25783d262 100644 --- a/src/common/exec.c +++ b/src/common/exec.c @@ -43,6 +43,7 @@ #endif #include "common/string.h" +#include "lib/stringinfo.h" /* Inhibit mingw CRT's auto-globbing of command line arguments */ #if defined(WIN32) && !defined(_MSC_VER) @@ -311,7 +312,7 @@ int find_other_exec(const char *argv0, const char *target, const char *versionstr, char *retpath) { - char cmd[MAXPGPATH]; + StringInfoData cmd; char *line; if (find_my_exec(argv0, retpath) < 0) @@ -328,9 +329,13 @@ find_other_exec(const char *argv0, const char *target, if (validate_exec(retpath) != 0) return -1; - snprintf(cmd, sizeof(cmd), "\"%s\" -V", retpath); + initStringInfo(&cmd); + appendStringInfoShell(&cmd, retpath); + appendStringInfoString(&cmd, " -V"); - if ((line = pipe_read_line(cmd)) == NULL) + line = pipe_read_line(cmd.data); + pfree(cmd.data); + if (line == NULL) return -1; if (strcmp(line, versionstr) != 0) -- 2.50.1 (Apple Git-155)