The security team received a report about misbehavior when a Windows program
executes a PostgreSQL program, passing a wide character on the command line.
Every such character gets translated to something else. Of note, a U+FF02
FULLWIDTH QUOTATION MARK in the caller-provided command line becomes U+0022
QUOTATION MARK in GetCommandLineA(). Since U+0022 is a metacharacter for
splitting the command line into argv, the argv argument boundaries may be
contrary to caller intentions. This isn't a vulnerability in PostgreSQL, but
it may be a vulnerability in Windows programs that construct CreateProcessW()
arguments to execute PostgreSQL programs. We don't know of any affected
programs, but we didn't search diligently.
The report (reporter bcc'd) proposed a fix of using non-ANSI APIs (UNICODE
APIs) to process the command line. That would create the worse problem that
both the caller (runs CreateProcessW() or CreateProcessA()) and callee (runs
GetCommandLineA() or GetCommandLineW()) would need to change simultaneously or
suffer mojibake. Consider the example of the argument to vacuumdb's
"--schema" argument. Under PGCLIENTENCODING=UTF8, a caller wanting YEN SIGN
runs CreateProcessA(... 0xC2 0xA5 ...) or
CreateProcessW(... MultiByteToWideChar(CP_ACP, 0, ... 0xC2 0xA5 ...), ...).
Suppose vacuumdb switched to wmain(int argc, wchar_t *argv[]) and interpreted
argv as UTF-16. The same two CreateProcess calls would then get a
two-character schema name instead, yielding mojibake until callers update to
CreateProcessW(... 0x00A5 ...). That would create as many vulnerabilities as
it solves.
What we can do safely is exit if we could not convert the command line to the
Windows ANSI code page (losslessly). Patch attached.
As I mention in a code comment, I decided not to do anything about the similar
hazard in GetEnvironmentStringsW(). A possible alternative would be to check
each var referenced in PostgreSQL code or each var where the name begins with
"PG". Should we use one of those or another alternative?
The Perl testing calls powershell, which is new for our source tree. The
Internet thinks Windows Server 2008 R1 is the newest Windows where powershell
was optional. The buildfarm's oldest Windows is Windows Server 2008 R2. If
powershell is bad, there's likely a hairy way to write the test in any of
cmd.exe, cscript, Perl Win32::API, or C. My first choice was Perl
Win32::Process or IPC::Run. This test needs CreateProcessW(), but those two
use CreateProcessA().
I subjected the postmaster to program_options_handling_ok() and the other
tests we apply to most executables.
I didn't test a CP_UTF8 Windows system. The new test coverage has a condition
intended to avoid failure there. (My Windows development environments are all
too old, and I stopped short of building a new one for this.) CP_UTF8 Windows
raises broader issues. I'll start a separate thread about them.
I wanted to put the check in some function that most main() instances already
call, instead of adding ~40 new calls for this esoteric topic. Options:
1. Call in pg_logging_init() and the backend. Add pg_logging_init() calls to
pg_config, ecpg (the ecpg preprocessor), and pg_test_timing. I picked
this, for two minor benefits. First, it facilitates the backend
initializing LC_MESSAGES before this check. Second, every frontend may
need pg_logging_init() eventually, so it's fair to add calls where missing.
2. Call in set_pglocale_pgservice() and the three programs that don't call
set_pglocale_pgservice(): oid2name, vacuumlo, pgbench.
Thanks,
nm
Author: Noah Misch <n...@leadboat.com>
Commit: Noah Misch <n...@leadboat.com>
Test postmaster with program_options_handling_ok() et al.
Most executables already get that testing. To occupy the customary
001_basic.pl name, this renumbers the new-in-October tests of
src/test/postmaster/t.
Reviewed by FIXME.
Discussion: https://postgr.es/m/FIXME
diff --git a/src/test/postmaster/meson.build b/src/test/postmaster/meson.build
index 2d89adf..5b909fe 100644
--- a/src/test/postmaster/meson.build
+++ b/src/test/postmaster/meson.build
@@ -6,8 +6,9 @@ tests += {
'bd': meson.current_build_dir(),
'tap': {
'tests': [
- 't/001_connection_limits.pl',
- 't/002_start_stop.pl',
+ 't/001_basic.pl',
+ 't/002_connection_limits.pl',
+ 't/003_start_stop.pl',
],
},
}
diff --git a/src/test/postmaster/t/001_basic.pl
b/src/test/postmaster/t/001_basic.pl
new file mode 100644
index 0000000..13f411d
--- /dev/null
+++ b/src/test/postmaster/t/001_basic.pl
@@ -0,0 +1,13 @@
+
+# Copyright (c) 2021-2024, PostgreSQL Global Development Group
+
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+program_help_ok('postgres');
+program_version_ok('postgres');
+program_options_handling_ok('postgres');
+
+done_testing();
diff --git a/src/test/postmaster/t/001_connection_limits.pl
b/src/test/postmaster/t/001_connection_limits.pl
deleted file mode 100644
index 7e64a82..0000000
--- a/src/test/postmaster/t/001_connection_limits.pl
+++ /dev/null
@@ -1,123 +0,0 @@
-
-# Copyright (c) 2021-2024, PostgreSQL Global Development Group
-
-# Test connection limits, i.e. max_connections, reserved_connections
-# and superuser_reserved_connections.
-
-use strict;
-use warnings FATAL => 'all';
-use PostgreSQL::Test::Cluster;
-use PostgreSQL::Test::Utils;
-use Test::More;
-
-# Initialize the server with specific low connection limits
-my $node = PostgreSQL::Test::Cluster->new('primary');
-$node->init(
- 'auth_extra' => [
- '--create-role',
'regress_regular,regress_reserved,regress_superuser'
- ]);
-$node->append_conf('postgresql.conf', "max_connections = 6");
-$node->append_conf('postgresql.conf', "reserved_connections = 2");
-$node->append_conf('postgresql.conf', "superuser_reserved_connections = 1");
-$node->append_conf('postgresql.conf', "log_connections = on");
-$node->start;
-
-$node->safe_psql(
- 'postgres', qq{
-CREATE USER regress_regular LOGIN;
-CREATE USER regress_reserved LOGIN;
-GRANT pg_use_reserved_connections TO regress_reserved;
-CREATE USER regress_superuser LOGIN SUPERUSER;
-});
-
-# With the limits we set in postgresql.conf, we can establish:
-# - 3 connections for any user with no special privileges
-# - 2 more connections for users belonging to "pg_use_reserved_connections"
-# - 1 more connection for superuser
-
-sub background_psql_as_user
-{
- my $user = shift;
-
- return $node->background_psql(
- 'postgres',
- on_error_die => 1,
- extra_params => [ '-U', $user ]);
-}
-
-my @sessions = ();
-my @raw_connections = ();
-
-push(@sessions, background_psql_as_user('regress_regular'));
-push(@sessions, background_psql_as_user('regress_regular'));
-push(@sessions, background_psql_as_user('regress_regular'));
-$node->connect_fails(
- "dbname=postgres user=regress_regular",
- "reserved_connections limit",
- expected_stderr =>
- qr/FATAL: remaining connection slots are reserved for roles with
privileges of the "pg_use_reserved_connections" role/
-);
-
-push(@sessions, background_psql_as_user('regress_reserved'));
-push(@sessions, background_psql_as_user('regress_reserved'));
-$node->connect_fails(
- "dbname=postgres user=regress_regular",
- "reserved_connections limit",
- expected_stderr =>
- qr/FATAL: remaining connection slots are reserved for roles with the
SUPERUSER attribute/
-);
-
-push(@sessions, background_psql_as_user('regress_superuser'));
-$node->connect_fails(
- "dbname=postgres user=regress_superuser",
- "superuser_reserved_connections limit",
- expected_stderr => qr/FATAL: sorry, too many clients already/);
-
-# We can still open TCP (or Unix domain socket) connections, but
-# beyond a certain number (roughly 2x max_connections), they will be
-# "dead-end backends".
-SKIP:
-{
- skip "this test requires working raw_connect()"
- unless $node->raw_connect_works();
-
- for (my $i = 0; $i <= 20; $i++)
- {
- my $sock = $node->raw_connect();
-
- # On a busy system, the server might reject connections if
- # postmaster cannot accept() them fast enough. The exact limit
- # and behavior depends on the platform. To make this reliable,
- # we attempt SSL negotiation on each connection before opening
- # next one. The server will reject the SSL negotiations, but
- # when it does so, we know that the backend has been launched
- # and we should be able to open another connection.
-
- # SSLRequest packet consists of packet length followed by
- # NEGOTIATE_SSL_CODE.
- my $negotiate_ssl_code = pack("Nnn", 8, 1234, 5679);
- my $sent = $sock->send($negotiate_ssl_code);
-
- # Read reply. We expect the server to reject it with 'N'
- my $reply = "";
- $sock->recv($reply, 1);
- is($reply, "N", "dead-end connection $i");
-
- push(@raw_connections, $sock);
- }
-}
-
-# TODO: test that query cancellation is still possible. A dead-end
-# backend can process a query cancellation packet.
-
-# Clean up
-foreach my $session (@sessions)
-{
- $session->quit;
-}
-foreach my $socket (@raw_connections)
-{
- $socket->close();
-}
-
-done_testing();
diff --git a/src/test/postmaster/t/002_connection_limits.pl
b/src/test/postmaster/t/002_connection_limits.pl
new file mode 100644
index 0000000..7e64a82
--- /dev/null
+++ b/src/test/postmaster/t/002_connection_limits.pl
@@ -0,0 +1,123 @@
+
+# Copyright (c) 2021-2024, PostgreSQL Global Development Group
+
+# Test connection limits, i.e. max_connections, reserved_connections
+# and superuser_reserved_connections.
+
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Initialize the server with specific low connection limits
+my $node = PostgreSQL::Test::Cluster->new('primary');
+$node->init(
+ 'auth_extra' => [
+ '--create-role',
'regress_regular,regress_reserved,regress_superuser'
+ ]);
+$node->append_conf('postgresql.conf', "max_connections = 6");
+$node->append_conf('postgresql.conf', "reserved_connections = 2");
+$node->append_conf('postgresql.conf', "superuser_reserved_connections = 1");
+$node->append_conf('postgresql.conf', "log_connections = on");
+$node->start;
+
+$node->safe_psql(
+ 'postgres', qq{
+CREATE USER regress_regular LOGIN;
+CREATE USER regress_reserved LOGIN;
+GRANT pg_use_reserved_connections TO regress_reserved;
+CREATE USER regress_superuser LOGIN SUPERUSER;
+});
+
+# With the limits we set in postgresql.conf, we can establish:
+# - 3 connections for any user with no special privileges
+# - 2 more connections for users belonging to "pg_use_reserved_connections"
+# - 1 more connection for superuser
+
+sub background_psql_as_user
+{
+ my $user = shift;
+
+ return $node->background_psql(
+ 'postgres',
+ on_error_die => 1,
+ extra_params => [ '-U', $user ]);
+}
+
+my @sessions = ();
+my @raw_connections = ();
+
+push(@sessions, background_psql_as_user('regress_regular'));
+push(@sessions, background_psql_as_user('regress_regular'));
+push(@sessions, background_psql_as_user('regress_regular'));
+$node->connect_fails(
+ "dbname=postgres user=regress_regular",
+ "reserved_connections limit",
+ expected_stderr =>
+ qr/FATAL: remaining connection slots are reserved for roles with
privileges of the "pg_use_reserved_connections" role/
+);
+
+push(@sessions, background_psql_as_user('regress_reserved'));
+push(@sessions, background_psql_as_user('regress_reserved'));
+$node->connect_fails(
+ "dbname=postgres user=regress_regular",
+ "reserved_connections limit",
+ expected_stderr =>
+ qr/FATAL: remaining connection slots are reserved for roles with the
SUPERUSER attribute/
+);
+
+push(@sessions, background_psql_as_user('regress_superuser'));
+$node->connect_fails(
+ "dbname=postgres user=regress_superuser",
+ "superuser_reserved_connections limit",
+ expected_stderr => qr/FATAL: sorry, too many clients already/);
+
+# We can still open TCP (or Unix domain socket) connections, but
+# beyond a certain number (roughly 2x max_connections), they will be
+# "dead-end backends".
+SKIP:
+{
+ skip "this test requires working raw_connect()"
+ unless $node->raw_connect_works();
+
+ for (my $i = 0; $i <= 20; $i++)
+ {
+ my $sock = $node->raw_connect();
+
+ # On a busy system, the server might reject connections if
+ # postmaster cannot accept() them fast enough. The exact limit
+ # and behavior depends on the platform. To make this reliable,
+ # we attempt SSL negotiation on each connection before opening
+ # next one. The server will reject the SSL negotiations, but
+ # when it does so, we know that the backend has been launched
+ # and we should be able to open another connection.
+
+ # SSLRequest packet consists of packet length followed by
+ # NEGOTIATE_SSL_CODE.
+ my $negotiate_ssl_code = pack("Nnn", 8, 1234, 5679);
+ my $sent = $sock->send($negotiate_ssl_code);
+
+ # Read reply. We expect the server to reject it with 'N'
+ my $reply = "";
+ $sock->recv($reply, 1);
+ is($reply, "N", "dead-end connection $i");
+
+ push(@raw_connections, $sock);
+ }
+}
+
+# TODO: test that query cancellation is still possible. A dead-end
+# backend can process a query cancellation packet.
+
+# Clean up
+foreach my $session (@sessions)
+{
+ $session->quit;
+}
+foreach my $socket (@raw_connections)
+{
+ $socket->close();
+}
+
+done_testing();
diff --git a/src/test/postmaster/t/002_start_stop.pl
b/src/test/postmaster/t/002_start_stop.pl
deleted file mode 100644
index 993d855..0000000
--- a/src/test/postmaster/t/002_start_stop.pl
+++ /dev/null
@@ -1,98 +0,0 @@
-
-# Copyright (c) 2021-2024, PostgreSQL Global Development Group
-
-# Test postmaster start and stop state machine.
-
-use strict;
-use warnings FATAL => 'all';
-use PostgreSQL::Test::Cluster;
-use PostgreSQL::Test::Utils;
-use Test::More;
-
-#
-# Test that dead-end backends don't prevent the server from shutting
-# down.
-#
-# Dead-end backends can linger until they reach
-# authentication_timeout. We use a long authentication_timeout and a
-# much shorter timeout for the "pg_ctl stop" operation, to test that
-# if dead-end backends are killed at fast shut down. If they're not,
-# "pg_ctl stop" will error out before the authentication timeout kicks
-# in and cleans up the dead-end backends.
-my $authentication_timeout = $PostgreSQL::Test::Utils::timeout_default;
-my $stop_timeout = $authentication_timeout / 2;
-
-# Initialize the server with low connection limits, to test dead-end backends
-my $node = PostgreSQL::Test::Cluster->new('main');
-$node->init;
-$node->append_conf('postgresql.conf', "max_connections = 5");
-$node->append_conf('postgresql.conf', "max_wal_senders = 0");
-$node->append_conf('postgresql.conf', "autovacuum_max_workers = 1");
-$node->append_conf('postgresql.conf', "max_worker_processes = 1");
-$node->append_conf('postgresql.conf', "log_connections = on");
-$node->append_conf('postgresql.conf', "log_min_messages = debug2");
-$node->append_conf('postgresql.conf',
- "authentication_timeout = '$authentication_timeout s'");
-$node->append_conf('postgresql.conf', 'trace_connection_negotiation=on');
-$node->start;
-
-if (!$node->raw_connect_works())
-{
- plan skip_all => "this test requires working raw_connect()";
-}
-
-my @raw_connections = ();
-
-# Open a lot of TCP (or Unix domain socket) connections to use up all
-# the connection slots. Beyond a certain number (roughly 2x
-# max_connections), they will be "dead-end backends".
-for (my $i = 0; $i <= 20; $i++)
-{
- my $sock = $node->raw_connect();
-
- # On a busy system, the server might reject connections if
- # postmaster cannot accept() them fast enough. The exact limit
- # and behavior depends on the platform. To make this reliable,
- # we attempt SSL negotiation on each connection before opening
- # next one. The server will reject the SSL negotations, but
- # when it does so, we know that the backend has been launched
- # and we should be able to open another connection.
-
- # SSLRequest packet consists of packet length followed by
- # NEGOTIATE_SSL_CODE.
- my $negotiate_ssl_code = pack("Nnn", 8, 1234, 5679);
- my $sent = $sock->send($negotiate_ssl_code);
-
- # Read reply. We expect the server to reject it with 'N'
- my $reply = "";
- $sock->recv($reply, 1);
- is($reply, "N", "dead-end connection $i");
-
- push(@raw_connections, $sock);
-}
-
-# When all the connection slots are in use, new connections will fail
-# before even looking up the user. Hence you now get "sorry, too many
-# clients already" instead of "role does not exist" error. Test that
-# to ensure that we have used up all the slots.
-$node->connect_fails("dbname=postgres user=invalid_user",
- "connect ",
- expected_stderr => qr/FATAL: sorry, too many clients already/);
-
-# Open one more connection, to really ensure that we have at least one
-# dead-end backend.
-my $sock = $node->raw_connect();
-
-# Test that the dead-end backends don't prevent the server from stopping.
-$node->stop('fast', timeout => $stop_timeout);
-
-$node->start();
-$node->connect_ok("dbname=postgres", "works after restart");
-
-# Clean up
-foreach my $socket (@raw_connections)
-{
- $socket->close();
-}
-
-done_testing();
diff --git a/src/test/postmaster/t/003_start_stop.pl
b/src/test/postmaster/t/003_start_stop.pl
new file mode 100644
index 0000000..993d855
--- /dev/null
+++ b/src/test/postmaster/t/003_start_stop.pl
@@ -0,0 +1,98 @@
+
+# Copyright (c) 2021-2024, PostgreSQL Global Development Group
+
+# Test postmaster start and stop state machine.
+
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+#
+# Test that dead-end backends don't prevent the server from shutting
+# down.
+#
+# Dead-end backends can linger until they reach
+# authentication_timeout. We use a long authentication_timeout and a
+# much shorter timeout for the "pg_ctl stop" operation, to test that
+# if dead-end backends are killed at fast shut down. If they're not,
+# "pg_ctl stop" will error out before the authentication timeout kicks
+# in and cleans up the dead-end backends.
+my $authentication_timeout = $PostgreSQL::Test::Utils::timeout_default;
+my $stop_timeout = $authentication_timeout / 2;
+
+# Initialize the server with low connection limits, to test dead-end backends
+my $node = PostgreSQL::Test::Cluster->new('main');
+$node->init;
+$node->append_conf('postgresql.conf', "max_connections = 5");
+$node->append_conf('postgresql.conf', "max_wal_senders = 0");
+$node->append_conf('postgresql.conf', "autovacuum_max_workers = 1");
+$node->append_conf('postgresql.conf', "max_worker_processes = 1");
+$node->append_conf('postgresql.conf', "log_connections = on");
+$node->append_conf('postgresql.conf', "log_min_messages = debug2");
+$node->append_conf('postgresql.conf',
+ "authentication_timeout = '$authentication_timeout s'");
+$node->append_conf('postgresql.conf', 'trace_connection_negotiation=on');
+$node->start;
+
+if (!$node->raw_connect_works())
+{
+ plan skip_all => "this test requires working raw_connect()";
+}
+
+my @raw_connections = ();
+
+# Open a lot of TCP (or Unix domain socket) connections to use up all
+# the connection slots. Beyond a certain number (roughly 2x
+# max_connections), they will be "dead-end backends".
+for (my $i = 0; $i <= 20; $i++)
+{
+ my $sock = $node->raw_connect();
+
+ # On a busy system, the server might reject connections if
+ # postmaster cannot accept() them fast enough. The exact limit
+ # and behavior depends on the platform. To make this reliable,
+ # we attempt SSL negotiation on each connection before opening
+ # next one. The server will reject the SSL negotations, but
+ # when it does so, we know that the backend has been launched
+ # and we should be able to open another connection.
+
+ # SSLRequest packet consists of packet length followed by
+ # NEGOTIATE_SSL_CODE.
+ my $negotiate_ssl_code = pack("Nnn", 8, 1234, 5679);
+ my $sent = $sock->send($negotiate_ssl_code);
+
+ # Read reply. We expect the server to reject it with 'N'
+ my $reply = "";
+ $sock->recv($reply, 1);
+ is($reply, "N", "dead-end connection $i");
+
+ push(@raw_connections, $sock);
+}
+
+# When all the connection slots are in use, new connections will fail
+# before even looking up the user. Hence you now get "sorry, too many
+# clients already" instead of "role does not exist" error. Test that
+# to ensure that we have used up all the slots.
+$node->connect_fails("dbname=postgres user=invalid_user",
+ "connect ",
+ expected_stderr => qr/FATAL: sorry, too many clients already/);
+
+# Open one more connection, to really ensure that we have at least one
+# dead-end backend.
+my $sock = $node->raw_connect();
+
+# Test that the dead-end backends don't prevent the server from stopping.
+$node->stop('fast', timeout => $stop_timeout);
+
+$node->start();
+$node->connect_ok("dbname=postgres", "works after restart");
+
+# Clean up
+foreach my $socket (@raw_connections)
+{
+ $socket->close();
+}
+
+done_testing();
Author: Noah Misch <n...@leadboat.com>
Commit: Noah Misch <n...@leadboat.com>
Reject Windows command lines not convertible to Windows ANSI code page.
PostgreSQL can't interpret them correctly without causing mojibake for
other arguments. This isn't fixing a vulnerability in PostgreSQL, but
Windows programs that form arbitrary CreateProcessW() command lines to
execute PostgreSQL programs may have a vulnerability. If those programs
exist, this change would mitigate their associated vulnerabilities.
A program reaching the new error could use the following steps.
Construct a char[] representing the intended PostgreSQL child program
command line bytes. Filter that through MultiByteToWideChar(CP_ACP, 0,
...), and pass the result as the CreateProcessW() command line argument.
This is both a compatibility break and a bug fix. While PostgreSQL has
been misinterpreting the arguments this rejects, the application may be
compensating. For example, an inconvertible character in the argument
to psql's "--output" has been causing a write to a differently-named
file, determined by Windows "best fit" behavior. If other file accesses
filter the name through "best fit", the application may be functioning
and oblivious to the actual file name written. Hence, no back-patch.
Reported by Splitline Huang. Reviewed by FIXME.
Discussion: https://postgr.es/m/FIXME
diff --git a/src/backend/main/main.c b/src/backend/main/main.c
index e286810..20bad17 100644
--- a/src/backend/main/main.c
+++ b/src/backend/main/main.c
@@ -150,6 +150,8 @@ main(int argc, char *argv[])
*/
unsetenv("LC_ALL");
+ validate_startup(); /* exit if parent called us
incorrectly */
+
/*
* Catch standard options before doing much else, in particular before
we
* insist on not being root.
diff --git a/src/bin/pg_config/pg_config.c b/src/bin/pg_config/pg_config.c
index 504e6c5..f588fdd 100644
--- a/src/bin/pg_config/pg_config.c
+++ b/src/bin/pg_config/pg_config.c
@@ -25,6 +25,7 @@
#include "postgres_fe.h"
#include "common/config_info.h"
+#include "common/logging.h"
static const char *progname;
@@ -134,8 +135,8 @@ main(int argc, char **argv)
int i;
int j;
+ pg_logging_init(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_config"));
-
progname = get_progname(argv[0]);
/* check for --help */
diff --git a/src/bin/pg_test_timing/pg_test_timing.c
b/src/bin/pg_test_timing/pg_test_timing.c
index ce7aad4..eede8dd 100644
--- a/src/bin/pg_test_timing/pg_test_timing.c
+++ b/src/bin/pg_test_timing/pg_test_timing.c
@@ -8,6 +8,7 @@
#include <limits.h>
+#include "common/logging.h"
#include "getopt_long.h"
#include "portability/instr_time.h"
@@ -27,6 +28,7 @@ main(int argc, char *argv[])
{
uint64 loop_count;
+ pg_logging_init(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_test_timing"));
progname = get_progname(argv[0]);
diff --git a/src/common/exec.c b/src/common/exec.c
index 32fd565..7a5b035 100644
--- a/src/common/exec.c
+++ b/src/common/exec.c
@@ -42,6 +42,9 @@
#endif
#endif
+#ifdef FRONTEND
+#include "common/logging.h"
+#endif
#include "common/string.h"
/* Inhibit mingw CRT's auto-globbing of command line arguments */
@@ -466,6 +469,178 @@ set_pglocale_pgservice(const char *argv0, const char *app)
}
}
+/*
+ * Exit if parent process designated unacceptable conditions.
+ *
+ * Every installed executable shall call this, most via pg_logging_init().
+ *
+ * Currently, the only unacceptable condition is a Windows command line
+ * containing characters not surviving conversion to Windows ANSI code page.
+ * Splitting such an unacceptable command line into "char *argv[]" uses "best
+ * fit" behavior
+ * (unicode.org/Public/MAPPINGS/VENDORS/MICSFT/WindowsBestFit/readme.txt).
+ * For example, U+FF02 FULLWIDTH QUOTATION MARK becomes U+0022 QUOTATION MARK.
+ * Since U+0022 is a metacharacter that affects argument boundaries, the
+ * resulting argument boundaries could differ from what they would be if the
+ * executable were to use wmain(int argc, wchar_t *argv[]). That isn't a
+ * vulnerability in PostgreSQL, but it may be a vulnerability in Windows
+ * programs that construct CreateProcessW() arguments to execute PostgreSQL
+ * programs. While most "best fit" substitutes aren't metacharacters, they
+ * still lose parent process intent.
+ *
+ * ==== Background ====
+ *
+ * Windows API functions taking string arguments come in two variants. The
+ * "wide" variant (e.g. CreateProcessW()) takes wchar_t[] strings in UTF-16
+ * encoding. The "ANSI" variant (e.g. CreateProcessA()) takes char[] strings
+ * in the "Windows ANSI code page" encoding. Changing the Control Panel
+ * "system locale" changes the Windows ANSI code page.
+ *
+ * POSIX handles process arguments as an array of encoding-oblivious sequences
+ * of bytes [0x1,0xFF]. Windows handles any number of arguments through a
+ * single wchar_t[] "command line". When both parent and child use ANSI APIs,
+ * process arguments arrive via steps equivalent to the following:
+ *
+ * - Parent starts with the CreateProcessA() lpCommandLine argument.
+ * - Parent's MultiByteToWideChar(CP_ACP, 0, lpCommandLine) produces the
+ * child-accessible command line (the child's GetCommandLineW()).
+ * - Child retrieves GetCommandLineA(), which matches from
+ * WideCharToMultiByte(CP_ACP, 0, GetCommandLineW(), ...).
+ * - Child parses GetCommandLineA() to construct argv (see
+ * https://msdn.microsoft.com/en-us/library/17w5ykft.aspx).
+ *
+ * For code pages other than GetACP()==CP_UTF8, the WideCharToMultiByte()
+ * losslessly reverses the MultiByteToWideChar(). Hence, the double
+ * conversion is imperceptible for those code pages. It also follows that a
+ * CreateProcessW() caller can make arbitrary GetCommandLineA() sequences of
+ * bytes [0x1,0xFF]. It does that by filtering the intended GetCommandLineA()
+ * through MultiByteToWideChar() to get the equivalent CreateProcessW()
+ * lpCommandLine argument.
+ *
+ * With CP_UTF8, GetCommandLineW() will contain U+FFFD REPLACEMENT CHARACTER
+ * for each byte of lpCommandLine not forming valid UTF-8. Hence,
+ * GetCommandLineA() is always valid UTF8. Other GetCommandLineA() byte
+ * sequences are infeasible. An administrator elects CP_UTF8 with non-default
+ * Windows setting "Beta: Use Unicode UTF-8 for worldwide language support".
+ *
+ * ==== Design decisions ====
+ *
+ * Conversion from U+0081 to 0x81 is also a "best fit" conversion in that most
+ * code pages do not have a character named HIGH OCTET PRESET. However, the
+ * conversion is bidirectional, so it loses no parent process intent. This
+ * applies to some other characters in [U+0080,U+00FF]. Allowing these
+ * characters is valuable, because it avoids rejecting any CreateProcessA()
+ * lpCommandLine value. Only CreateProcessW() reaches failure here.
+ * PostgreSQL-supplied programs don't use CreateProcessW().
+ *
+ * Environment variables, too, see "best fit" behavior. In PGOPTIONS, an
+ * unescaped U+FF07 FULLWIDTH APOSTROPHE or U+FF3C FULLWIDTH REVERSE SOLIDUS
+ * would defeat quoting. In PGDATA, any inconvertible character would defeat
+ * finding the directory. Even so, we don't check GetEnvironmentStringsW().
+ * We don't know which variables this process will retrieve. It doesn't help
+ * to reject variables outside those.
+ *
+ * One can always interconvert UTF-8 and UTF-16 without loss. Hence, one
+ * could skip this check for CP_UTF8. Since GetACP()==CP_UTF8 is still in
+ * beta, don't rely on that.
+ */
+void
+validate_startup(void)
+{
+#ifdef WIN32
+ wchar_t *cmd_w = GetCommandLineW();
+ wchar_t *cmd_w_cursor = cmd_w;
+ wchar_t ch;
+ bool all_ascii = true;
+ char *cmd_a;
+ wchar_t *cmd_w_from_a;
+ int result;
+ int n_cmd_w_from_a;
+ int n_cmd_w;
+
+ /* Return early for all-ASCII args. */
+ while ((ch = *cmd_w_cursor++))
+ if (ch > 0x7f)
+ {
+ all_ascii = false;
+ break;
+ }
+ if (all_ascii)
+ return;
+
+ /* non-ASCII: check conversion reversibility */
+ cmd_a = GetCommandLineA();
+ result = MultiByteToWideChar(CP_ACP, 0, cmd_a, -1, NULL, 0);
+ if (result != 0)
+ {
+ n_cmd_w_from_a = result;
+ cmd_w_from_a = palloc(sizeof(wchar_t) * n_cmd_w_from_a);
+ result = MultiByteToWideChar(CP_ACP, 0, cmd_a, -1,
+
cmd_w_from_a, n_cmd_w_from_a);
+ }
+
+ /*
+ * Since the system fills GetCommandLineA() by converting
+ * GetCommandLineW() from UTF-16, there's no known way for those
+ * MultiByteToWideChar() calls to fail. This failure message doesn't
+ * deserve translation, but logging.h has no errmsg_internal()
equivalent
+ * to suppress translation. Once we translate the string for logging.h,
we
+ * may as well use the translation here.
+ */
+ if (result == 0)
+ {
+#ifndef FRONTEND
+ ereport(FATAL,
+ errcode(ERRCODE_INTERNAL_ERROR),
+ errmsg("could not convert command line to
UTF-16: error code %lu",
+ GetLastError()));
+#else
+ pg_log_error("could not convert command line to UTF-16: error
code %lu",
+ GetLastError());
+ exit(1);
+#endif
+ }
+
+ /*
+ * Since the system fills GetCommandLineA() by converting
+ * GetCommandLineW() from UTF-16, there's no known way for the reverse
+ * conversion to yield a different number of code units.
+ */
+ n_cmd_w = 1 /* terminator */ + wcslen(cmd_w);
+ if (n_cmd_w_from_a != n_cmd_w)
+ {
+#ifndef FRONTEND
+ ereport(FATAL,
+ errcode(ERRCODE_INTERNAL_ERROR),
+ errmsg("command line converted length (%d) did
not match wide character command line length (%d)",
+ n_cmd_w_from_a, n_cmd_w));
+#else
+ pg_log_error("command line converted length (%d) did not match
wide character command line length (%d)",
+ n_cmd_w_from_a, n_cmd_w);
+ exit(1);
+#endif
+ }
+ for (int i = 0; i < n_cmd_w; i++)
+ {
+ if (cmd_w[i] == cmd_w_from_a[i])
+ continue;
+#ifndef FRONTEND
+ ereport(FATAL,
+ errcode(ERRCODE_SYSTEM_ERROR),
+ errmsg("could not convert command line to
Windows ANSI code page"),
+ errdetail("First inconvertible UTF-16 code unit
was 0x%04X at offset %d.",
+ cmd_w[i], i));
+#else
+ pg_log_error("could not convert command line to Windows ANSI
code page");
+ pg_log_error_detail("First inconvertible UTF-16 code unit was
0x%04X at offset %d.",
+ cmd_w[i], i);
+ exit(1);
+#endif
+ }
+ /* wide strings were equal, so args are fine */
+#endif
+}
+
#ifdef EXEC_BACKEND
/*
* For the benefit of PostgreSQL developers testing EXEC_BACKEND on Unix
diff --git a/src/common/logging.c b/src/common/logging.c
index 3cf1190..b577d06 100644
--- a/src/common/logging.c
+++ b/src/common/logging.c
@@ -77,7 +77,8 @@ enable_vt_processing(void)
#endif /* WIN32 */
/*
- * This should be called before any output happens.
+ * Every installed frontend should call this before any output happens. Exits
+ * on error.
*/
void
pg_logging_init(const char *argv0)
@@ -157,6 +158,8 @@ pg_logging_init(const char *argv0)
sgr_locus = SGR_LOCUS_DEFAULT;
}
}
+
+ validate_startup(); /* convenient place to affect
all frontends */
}
/*
diff --git a/src/include/port.h b/src/include/port.h
index ba9ab0d..380c46e 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -132,6 +132,9 @@ extern void pgfnames_cleanup(char **filenames);
/* Portable locale initialization (in exec.c) */
extern void set_pglocale_pgservice(const char *argv0, const char *app);
+/* Exit if parent process designated unacceptable conditions (in exec.c) */
+extern void validate_startup(void);
+
/* Portable way to find and execute binaries (in exec.c) */
extern int validate_exec(const char *path);
extern int find_my_exec(const char *argv0, char *retpath);
diff --git a/src/interfaces/ecpg/preproc/ecpg.c
b/src/interfaces/ecpg/preproc/ecpg.c
index 2fcc6f8..36be896 100644
--- a/src/interfaces/ecpg/preproc/ecpg.c
+++ b/src/interfaces/ecpg/preproc/ecpg.c
@@ -7,6 +7,7 @@
#include <unistd.h>
+#include "common/logging.h"
#include "getopt_long.h"
#include "preproc_extern.h"
@@ -143,8 +144,8 @@ main(int argc, char *const argv[])
char my_exec_path[MAXPGPATH];
char include_path[MAXPGPATH];
+ pg_logging_init(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("ecpg"));
-
progname = get_progname(argv[0]);
if (find_my_exec(argv[0], my_exec_path) < 0)
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm
b/src/test/perl/PostgreSQL/Test/Utils.pm
index 022b44b..7266f01 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -936,6 +936,7 @@ sub program_options_handling_ok
{
local $Test::Builder::Level = $Test::Builder::Level + 1;
my ($cmd) = @_;
+
my ($stdout, $stderr);
print("# Running: $cmd --not-a-valid-option\n");
my $result = IPC::Run::run [ $cmd, '--not-a-valid-option' ], '>',
@@ -943,6 +944,42 @@ sub program_options_handling_ok
'2>', \$stderr;
ok(!$result, "$cmd with invalid option nonzero exit code");
isnt($stderr, '', "$cmd with invalid option prints error message");
+
+ # On Windows, confirm (1) rejection of characters we can't convert to
the
+ # Windows ANSI code page and (2) no rejection of environment variables.
+ # The $unicode_only test character, U+1FBCC RAISED SMALL LEFT SQUARE
+ # BRACKET, could have been be any character found only in CP_UTF8 and
code
+ # pages ineligible to be GetACP(). Do accept the wide equivalent of
+ # CreateProcessA(0x80 0x81), per comments at validate_startup().
+ SKIP:
+ {
+ skip "wide characters are a special case to Windows only", 1
+ if !$windows_os;
+ local $ENV{PG_TEST_EXE} = $cmd;
+ my $script = <<'EOPOWERSHELL';
+$want_exit_unicode_only = if ([System.Text.Encoding]::Default -eq
+
[System.Text.Encoding]::UTF8) { 0 } else { 1 }
+$exe = $Env:PG_TEST_EXE
+$any_encoding =
+ [System.Text.Encoding]::Default.GetString([byte[]](0x80, 0x81))
+$unicode_only = [Char]::ConvertFromUtf32(0x1FBCC)
+$Env:PG_TEST_WIDE_ENV = $unicode_only
+& $exe --help ($any_encoding) > $null
+$exit_any_encoding = $LASTEXITCODE
+& $exe --help ($unicode_only) > $null
+$exit_unicode_only = $LASTEXITCODE
+if ($exit_any_encoding -eq 0 -and
+ $exit_unicode_only -eq $want_exit_unicode_only)
+{
+ exit 0
+}
+exit 1
+EOPOWERSHELL
+ $result = IPC::Run::run [ qw(powershell -NoProfile
-NonInteractive),
+ '-Command' => $script ];
+ ok($result, 'command line rejected iff cannot convert to
GetACP()');
+ }
+
return;
}
