Skip to content

Commit ddf1772

Browse files
tglsfdctglsfdc
committed
Fix insecure parsing of server command-line switches.
An oversight in commit e710b65 allowed database names beginning with "-" to be treated as though they were secure command-line switches; and this switch processing occurs before client authentication, so that even an unprivileged remote attacker could exploit the bug, needing only connectivity to the postmaster's port. Assorted exploits for this are possible, some requiring a valid database login, some not. The worst known problem is that the "-r" switch can be invoked to redirect the process's stderr output, so that subsequent error messages will be appended to any file the server can write. This can for example be used to corrupt the server's configuration files, so that it will fail when next restarted. Complete destruction of database tables is also possible. Fix by keeping the database name extracted from a startup packet fully separate from command-line switches, as had already been done with the user name field. The Postgres project thanks Mitsumasa Kondo for discovering this bug, Kyotaro Horiguchi for drafting the fix, and Noah Misch for recognizing the full extent of the danger. Security: CVE-2013-1899
1 parent b403f41 commit ddf1772

File tree

5 files changed

+31
-34
lines changed

5 files changed

+31
-34
lines changed

‎src/backend/main/main.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,7 @@ main(int argc, char *argv[])
194194
exit(GucInfoMain());
195195

196196
if (argc > 1 && strcmp(argv[1], "--single") == 0)
197-
exit(PostgresMain(argc, argv, get_current_username(progname)));
197+
exit(PostgresMain(argc, argv, NULL, get_current_username(progname)));
198198

199199
exit(PostmasterMain(argc, argv));
200200
}

‎src/backend/postmaster/postmaster.c

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3571,7 +3571,7 @@ BackendRun(Port *port)
35713571
* from ExtraOptions is (strlen(ExtraOptions) + 1) / 2; see
35723572
* pg_split_opts().
35733573
*/
3574-
maxac = 5; /* for fixed args supplied below */
3574+
maxac = 2; /* for fixed args supplied below */
35753575
maxac += (strlen(ExtraOptions) + 1) / 2;
35763576

35773577
av = (char **) MemoryContextAlloc(TopMemoryContext,
@@ -3587,11 +3587,6 @@ BackendRun(Port *port)
35873587
*/
35883588
pg_split_opts(av, &ac, ExtraOptions);
35893589

3590-
/*
3591-
* Tell the backend which database to use.
3592-
*/
3593-
av[ac++] = port->database_name;
3594-
35953590
av[ac] = NULL;
35963591

35973592
Assert(ac < maxac);
@@ -3614,7 +3609,7 @@ BackendRun(Port *port)
36143609
*/
36153610
MemoryContextSwitchTo(TopMemoryContext);
36163611

3617-
return (PostgresMain(ac, av, port->user_name));
3612+
return PostgresMain(ac, av, port->database_name, port->user_name);
36183613
}
36193614

36203615

‎src/backend/tcop/postgres.c

Lines changed: 23 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -3256,13 +3256,14 @@ get_stats_option_name(const char *arg)
32563256
* coming from the client, or PGC_SUSET for insecure options coming from
32573257
* a superuser client.
32583258
*
3259-
* Returns the database name extracted from the command line, if any.
3259+
* If a database name is present in the command line arguments, it's
3260+
* returned into *dbname (this is allowed only if *dbname is initially NULL).
32603261
* ----------------------------------------------------------------
32613262
*/
3262-
const char *
3263-
process_postgres_switches(int argc, char *argv[], GucContext ctx)
3263+
void
3264+
process_postgres_switches(int argc, char *argv[], GucContext ctx,
3265+
const char **dbname)
32643266
{
3265-
const char *dbname;
32663267
bool secure = (ctx == PGC_POSTMASTER);
32673268
int errs = 0;
32683269
GucSource gucsource;
@@ -3303,7 +3304,8 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx)
33033304

33043305
case 'b':
33053306
/* Undocumented flag used for binary upgrades */
3306-
IsBinaryUpgrade = true;
3307+
if (secure)
3308+
IsBinaryUpgrade = true;
33073309
break;
33083310

33093311
case 'D':
@@ -3316,7 +3318,8 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx)
33163318
break;
33173319

33183320
case 'E':
3319-
EchoQuery = true;
3321+
if (secure)
3322+
EchoQuery = true;
33203323
break;
33213324

33223325
case 'e':
@@ -3341,7 +3344,8 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx)
33413344
break;
33423345

33433346
case 'j':
3344-
UseNewLine = 0;
3347+
if (secure)
3348+
UseNewLine = 0;
33453349
break;
33463350

33473351
case 'k':
@@ -3456,10 +3460,12 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx)
34563460
}
34573461

34583462
/*
3459-
* Should be no more arguments except an optional database name, and
3460-
* that's only in the secure case.
3463+
* Optional database name should be there only if *dbname is NULL.
34613464
*/
3462-
if (errs || argc - optind > 1 || (argc != optind && !secure))
3465+
if (!errs && dbname && *dbname == NULL && argc - optind >= 1)
3466+
*dbname = strdup(argv[optind++]);
3467+
3468+
if (errs || argc != optind)
34633469
{
34643470
/* spell the error message a bit differently depending on context */
34653471
if (IsUnderPostmaster)
@@ -3475,11 +3481,6 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx)
34753481
errhint("Try \"%s --help\" for more information.", progname)));
34763482
}
34773483

3478-
if (argc - optind == 1)
3479-
dbname = strdup(argv[optind]);
3480-
else
3481-
dbname = NULL;
3482-
34833484
/*
34843485
* Reset getopt(3) library so that it will work correctly in subprocesses
34853486
* or when this function is called a second time with another array.
@@ -3488,8 +3489,6 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx)
34883489
#ifdef HAVE_INT_OPTRESET
34893490
optreset = 1; /* some systems need this too */
34903491
#endif
3491-
3492-
return dbname;
34933492
}
34943493

34953494

@@ -3499,14 +3498,16 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx)
34993498
*
35003499
* argc/argv are the command line arguments to be used. (When being forked
35013500
* by the postmaster, these are not the original argv array of the process.)
3502-
* username is the (possibly authenticated) PostgreSQL user name to be used
3503-
* for the session.
3501+
* dbname is the name of the database to connect to, or NULL if the database
3502+
* name should be extracted from the command line arguments or defaulted.
3503+
* username is the PostgreSQL user name to be used for the session.
35043504
* ----------------------------------------------------------------
35053505
*/
35063506
int
3507-
PostgresMain(int argc, char *argv[], const char *username)
3507+
PostgresMain(int argc, char *argv[],
3508+
const char *dbname,
3509+
const char *username)
35083510
{
3509-
const char *dbname;
35103511
int firstchar;
35113512
StringInfoData input_message;
35123513
sigjmp_buf local_sigjmp_buf;
@@ -3553,7 +3554,7 @@ PostgresMain(int argc, char *argv[], const char *username)
35533554
/*
35543555
* Parse command-line options.
35553556
*/
3556-
dbname = process_postgres_switches(argc, argv, PGC_POSTMASTER);
3557+
process_postgres_switches(argc, argv, PGC_POSTMASTER, &dbname);
35573558

35583559
/* Must have gotten a database name, or have a default (the username) */
35593560
if (dbname == NULL)

‎src/backend/utils/init/postinit.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -912,7 +912,7 @@ process_startup_options(Port *port, bool am_superuser)
912912

913913
Assert(ac < maxac);
914914

915-
(void) process_postgres_switches(ac, av, gucctx);
915+
(void) process_postgres_switches(ac, av, gucctx, NULL);
916916
}
917917

918918
/*

‎src/include/tcop/tcopprot.h

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,9 +68,10 @@ extern void RecoveryConflictInterrupt(ProcSignalReason reason); /* called from S
6868
* handler */
6969
extern void prepare_for_client_read(void);
7070
extern void client_read_ended(void);
71-
extern const char *process_postgres_switches(int argc, char *argv[],
72-
GucContext ctx);
73-
extern int PostgresMain(int argc, char *argv[], const char *username);
71+
extern void process_postgres_switches(int argc, char *argv[],
72+
GucContext ctx, const char **dbname);
73+
extern int PostgresMain(int argc, char *argv[],
74+
const char *dbname, const char *username);
7475
extern long get_stack_depth_rlimit(void);
7576
extern void ResetUsage(void);
7677
extern void ShowUsage(const char *title);

0 commit comments

Comments
 (0)