Reject Windows command lines not convertible to system locale
Noah Misch <noah@leadboat.com>
From: Noah Misch <noah@leadboat.com>
To: pgsql-hackers@postgresql.org
Date: 2024-12-15T02:27:01Z
Lists: pgsql-hackers
Attachments
- bestfit05-postmaster-test-basic-v1.patch (text/plain) patch v1
- bestfit10-cmdline-fatal-v2.patch (text/plain) patch v2
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
Commits
-
Test postmaster with program_options_handling_ok() et al.
- 2f12df7eb46d 18.0 landed