8000 Fix insecure parsing of server command-line switches. · commandprompt/postgres@3d21a0f · GitHub
[go: up one dir, main page]
Skip to content

Commit 3d21a0f

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 315af69 commit 3d21a0f

File tree

5 files changed

+29
-33
lines changed

5 files changed

+29
-33
lines changed

‎src/backend/main/main.c

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

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

188188
exit(PostmasterMain(argc, argv));
189189
}

‎src/backend/postmaster/postmaster.c

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3525,7 +3525,7 @@ BackendRun(Port *port)
35253525
* from ExtraOptions is (strlen(ExtraOptions) + 1) / 2; see
35263526
* pg_split_opts().
35273527
*/
3528-
maxac = 5; /* for fixed args supplied below */
3528+
maxac = 2; /* for fixed args supplied below */
35293529
maxac += (strlen(ExtraOptions) + 1) / 2;
35303530

35313531
av = (char **) MemoryContextAlloc(TopMemoryContext,
@@ -3541,11 +3541,6 @@ BackendRun(Port *port)
35413541
*/
35423542
pg_split_opts(av, &ac, ExtraOptions);
35433543

3544-
/*
3545-
* Tell the backend which database to use.
3546-
*/
3547-
av[ac++] = port->database_name;
3548-
35493544
av[ac] = NULL;
35503545

35513546
Assert(ac < maxac);
@@ -3568,7 +3563,7 @@ BackendRun(Port *port)
35683563
*/
35693564
MemoryContextSwitchTo(TopMemoryContext);
35703565

3571-
return (PostgresMain(ac, av, port->user_name));
3566+
return PostgresMain(ac, av, port->database_name, port->user_name);
35723567
}
35733568

35743569

‎src/backend/tcop/postgres.c

Lines changed: 21 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -3271,13 +3271,14 @@ get_stats_option_name(const char *arg)
32713271
* coming from the client, or PGC_SUSET for insecure options coming from
32723272
* a superuser client.
32733273
*
3274-
* Returns the database name extracted from the command line, if any.
3274+
* If a database name is present in the command line arguments, it's
3275+
* returned into *dbname (this is allowed only if *dbname is initially NULL).
32753276
* ----------------------------------------------------------------
32763277
*/
3277-
const char *
3278-
process_postgres_switches(int argc, char *argv[], GucContext ctx)
3278+
void
3279+
process_postgres_switches(int argc, char *argv[], GucContext ctx,
3280+
const char **dbname)
32793281
{
3280-
const char *dbname;
32813282
bool secure = (ctx == PGC_POSTMASTER);
32823283
int errs = 0;
32833284
GucSource gucsource;
@@ -3326,7 +3327,8 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx)
33263327
break;
33273328

33283329
case 'E':
3329-
EchoQuery = true;
3330+
if (secure)
3331+
EchoQuery = true;
33303332
break;
33313333

33323334
case 'e':
@@ -3351,7 +3353,8 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx)
33513353
break;
33523354

33533355
case 'j':
3354-
UseNewLine = 0;
3356+
if (secure)
3357+
UseNewLine = 0;
33553358
break;
33563359

33573360
case 'k':
@@ -3466,10 +3469,12 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx)
34663469
}
34673470

34683471
/*
3469-
* Should be no more arguments except an optional database name, and
3470-
* that's only in the secure case.
3472+
* Optional database name should be there only if *dbname is NULL.
34713473
*/
3472-
if (errs || argc - optind > 1 || (argc != optind && !secure))
3474+
if (!errs && dbname && *dbname == NULL && argc - optind >= 1)
3475+
*dbname = strdup(argv[optind++]);
3476+
3477+
if (errs || argc != optind)
34733478
{
34743479
/* spell the error message a bit differently depending on context */
34753480
if (IsUnderPostmaster)
@@ -3485,11 +3490,6 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx)
34853490
errhint("Try \"%s --help\" for more information.", progname)));
34863491
}
34873492

3488-
if (argc - optind == 1)
3489-
dbname = strdup(argv[optind]);
3490-
else
3491-
dbname = NULL;
3492-
34933493
/*
34943494
* Reset getopt(3) library so that it will work correctly in subprocesses
34953495
* or when this function is called a second time with another array.
@@ -3498,8 +3498,6 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx)
34983498
#ifdef HAVE_INT_OPTRESET
34993499
optreset = 1; /* some systems need this too */
35003500
#endif
3501-
3502-
return dbname;
35033501
}
35043502

35053503

@@ -3509,14 +3507,16 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx)
35093507
*
35103508
* argc/argv are the command line arguments to be used. (When being forked
35113509
* by the postmaster, these are not the original argv array of the process.)
3512-
* username is the (possibly authenticated) PostgreSQL user name to be used
3513-
* for the session.
3510+
* dbname is the name of the database to connect to, or NULL if the database
3511+
* name should be extracted from the command line arguments or defaulted.
3512+
* username is the PostgreSQL user name to be used for the session.
35143513
* ----------------------------------------------------------------
35153514
*/
35163515
int
3517-
PostgresMain(int argc, char *argv[], const char *username)
3516+
PostgresMain(int argc, char *argv[],
3517+
const char *dbname,
3518+
const char *username)
35183519
{
3519-
const char *dbname;
35203520
int firstchar;
35213521
StringInfoData input_message;
35223522
sigjmp_buf local_sigjmp_buf;
@@ -3563,7 +3563,7 @@ PostgresMain(int argc, char *argv[], const char *username)
35633563
/*
35643564
* Parse command-line options.
35653565
*/
3566-
dbname = process_postgres_switches(argc, argv, PGC_POSTMASTER);
3566+
process_postgres_switches(argc, argv, PGC_POSTMASTER, &dbname);
35673567

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

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

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

905905
Assert(ac < maxac);
906906

907-
(void) process_postgres_switches(ac, av, gucctx);
907+
(void) process_postgres_switches(ac, av, gucctx, NULL);
908908
}
909909

910910
/*

‎src/include/tcop/tcopprot.h

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

0 commit comments

Comments
 (0)
0