From 62e6fffd6ef05b6c9bd5f652e87e8de8166fc8f3 Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Thu, 23 Jul 2026 18:50:58 -0400 Subject: [PATCH v1 3/8] pg_upgrade: Fix improper shell escaping for pass-through options. -o/--old-options or -O/--new-options are documented as allowing the user to pass through options to the server. Because we run pg_ctl using the shell, this implies that we should apply shell-escaping to the value of these options when building the pg_ctl command line, but instead, we have historically just surrounded the value with double quotes. In the back-branches, we don't want to change that behavior, in case someone is accidentally relying on it, but it seems OK to fix it in the master branch. As an example of what might break, if a user happened to be running a command like pg_upgrade -O '-c search_path=\\\$user', this commit would necessitate changing it to pg_upgrade -O '-c search_path=\$user'. This seems very unlikely, but it's not impossible. We regard this as a robustness fix rather than as a fix for a security vulnerability. See commit XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX for a more detailed rationale. --- src/bin/pg_upgrade/server.c | 53 +++++++++++-------------------------- 1 file changed, 15 insertions(+), 38 deletions(-) diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c index 31b993b43a9..3b81fc8a0dd 100644 --- a/src/bin/pg_upgrade/server.c +++ b/src/bin/pg_upgrade/server.c @@ -184,7 +184,6 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error) PGconn *conn; bool pg_ctl_return = false; PQExpBufferData postgres_opts; - PQExpBufferData socket_opts; static bool exit_hook_registered = false; @@ -216,41 +215,28 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error) add_pg_config_option(&postgres_opts, "full_page_writes", "off"); } - initPQExpBuffer(&socket_opts); + /* + * Pass through user-specified options. + * + * Note that any options added after this point will override whatever the + * user has specified. That may or may not be the behavior that we want, + * but we should be careful not to change it inadvertently. + */ + if (cluster->pgopts) + appendPQExpBuffer(&postgres_opts, " %s", cluster->pgopts); #if !defined(WIN32) /* prevent TCP/IP connections, restrict socket access */ - add_pg_config_option(&socket_opts, "listen_addresses", ""); - add_pg_config_option(&socket_opts, "unix_socket_permissions", "0700"); + add_pg_config_option(&postgres_opts, "listen_addresses", ""); + add_pg_config_option(&postgres_opts, "unix_socket_permissions", "0700"); /* Have a sockdir? Tell the postmaster. */ if (cluster->sockdir) - add_pg_config_option(&socket_opts, "unix_socket_directories", + add_pg_config_option(&postgres_opts, "unix_socket_directories", cluster->sockdir); #endif - /* - * Construct the pg_ctl command. - * - * -o/--old-options or -O/--new-options are documented as allowing the - * user to pass through options to the server. To deliver that behavior, - * we should shell-escape them before passing them to pg_ctl -o, since we - * will use the shell to run pg_ctl. However, the historical behavior of - * these flags is actually that they simply wrap the values of the options - * in double-quotes, and it's possible that there are users including - * shell metacharacters in the values passed to those options and relying - * on the faulty escaping for correct operation. Hence, preserve that - * behavior for now. - * - * We do, however, want to escape the other values that we're passing to - * pg_ctl -o, so that if, for example, the socket directory contains shell - * metacharacters, we nevertheless interpret the value as a literal - * pathname. Since appendShellString can only be applied to an entire - * option value as a unit, we specify -o three times: once for the options - * that precede the user-specified options, once for the user-specified - * options, and once for the options that follow the user-specified - * options. The order matters, since later options override earlier ones. - */ + /* Construct the pg_ctl command. */ initPQExpBuffer(&cmd); appendPQExpBufferStr(&cmd, quote_shell_path_arg(cluster->bindir, "pg_ctl")); appendPQExpBufferStr(&cmd, " -w -l "); @@ -259,20 +245,11 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error) SERVER_LOG_FILE)); appendPQExpBufferStr(&cmd, " -D "); appendPQExpBufferStr(&cmd, quote_shell_arg(cluster->pgconfig)); - appendPQExpBufferStr(&cmd, " -o "); appendShellString(&cmd, postgres_opts.data); - if (cluster->pgopts) - appendPQExpBuffer(&cmd, " -o \"%s\"", cluster->pgopts); - if (socket_opts.len > 0) - { - appendPQExpBufferStr(&cmd, " -o "); - appendShellString(&cmd, socket_opts.data); - } appendPQExpBufferStr(&cmd, " start"); termPQExpBuffer(&postgres_opts); - termPQExpBuffer(&socket_opts); /* * Don't throw an error right away, let connecting throw the error because @@ -363,10 +340,10 @@ stop_postmaster(bool in_atexit) return; /* no cluster running */ exec_prog(SERVER_STOP_LOG_FILE, NULL, !in_atexit, !in_atexit, - "%s -w -D %s -o \"%s\" %s stop", + "%s -w -D %s -o %s %s stop", quote_shell_path_arg(cluster->bindir, "pg_ctl"), quote_shell_arg(cluster->pgconfig), - cluster->pgopts ? cluster->pgopts : "", + quote_shell_arg(cluster->pgopts ? cluster->pgopts : ""), in_atexit ? "-m fast" : "-m smart"); os_info.running_cluster = NULL; -- 2.50.1 (Apple Git-155)