Skip to content

Commit 286c8bc

Browse files
nmischnmisch
committed
Obstruct shell, SQL, and conninfo injection via database and role names.
Due to simplistic quoting and confusion of database names with conninfo strings, roles with the CREATEDB or CREATEROLE option could escalate to superuser privileges when a superuser next ran certain maintenance commands. The new coding rule for PQconnectdbParams() calls, documented at conninfo_array_parse(), is to pass expand_dbname=true and wrap literal database names in a trivial connection string. Escape zero-length values in appendConnStrVal(). Back-patch to 9.1 (all supported versions). Nathan Bossart, Michael Paquier, and Noah Misch. Reviewed by Peter Eisentraut. Reported by Nathan Bossart. Security: CVE-2016-5424
1 parent 8adff37 commit 286c8bc

File tree

21 files changed

+656
-207
lines changed

21 files changed

+656
-207
lines changed

‎src/bin/pg_basebackup/streamutil.c‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,9 +64,15 @@ GetConnection(void)
6464
PQconninfoOption *conn_opt;
6565
char *err_msg = NULL;
6666

67+
/* pg_recvlogical uses dbname only; others use connection_string only. */
68+
Assert(dbname == NULL || connection_string == NULL);
69+
6770
/*
6871
* Merge the connection info inputs given in form of connection string,
6972
* options and default values (dbname=replication, replication=true, etc.)
73+
* Explicitly discard any dbname value in the connection string;
74+
* otherwise, PQconnectdbParams() would interpret that value as being
75+
* itself a connection string.
7076
*/
7177
i = 0;
7278
if (connection_string)
@@ -80,7 +86,8 @@ GetConnection(void)
8086

8187
for (conn_opt = conn_opts; conn_opt->keyword != NULL; conn_opt++)
8288
{
83-
if (conn_opt->val != NULL && conn_opt->val[0] != '\0')
89+
if (conn_opt->val != NULL && conn_opt->val[0] != '\0' &&
90+
strcmp(conn_opt->keyword, "dbname") != 0)
8491
argcount++;
8592
}
8693

@@ -89,7 +96,8 @@ GetConnection(void)
8996

9097
for (conn_opt = conn_opts; conn_opt->keyword != NULL; conn_opt++)
9198
{
92-
if (conn_opt->val != NULL && conn_opt->val[0] != '\0')
99+
if (conn_opt->val != NULL && conn_opt->val[0] != '\0' &&
100+
strcmp(conn_opt->keyword, "dbname") != 0)
93101
{
94102
keywords[i] = conn_opt->keyword;
95103
values[i] = conn_opt->val;

‎src/bin/pg_dump/dumputils.c‎

Lines changed: 204 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -339,6 +339,210 @@ appendStringLiteralDQ(PQExpBuffer buf, const char *str, const char *dqprefix)
339339
}
340340

341341

342+
/*
343+
* Append the given string to the shell command being built in the buffer,
344+
* with suitable shell-style quoting to create exactly one argument.
345+
*
346+
* Forbid LF or CR characters, which have scant practical use beyond designing
347+
* security breaches. The Windows command shell is unusable as a conduit for
348+
* arguments containing LF or CR characters. A future major release should
349+
* reject those characters in CREATE ROLE and CREATE DATABASE, because use
350+
* there eventually leads to errors here.
351+
*/
352+
void
353+
appendShellString(PQExpBuffer buf, const char *str)
354+
{
355+
const char *p;
356+
357+
#ifndef WIN32
358+
appendPQExpBufferChar(buf, '\'');
359+
for (p = str; *p; p++)
360+
{
361+
if (*p == '\n' || *p == '\r')
362+
{
363+
fprintf(stderr,
364+
_("shell command argument contains a newline or carriage return: \"%s\"\n"),
365+
str);
366+
exit(EXIT_FAILURE);
367+
}
368+
369+
if (*p == '\'')
370+
appendPQExpBufferStr(buf, "'\"'\"'");
371+
else
372+
appendPQExpBufferChar(buf, *p);
373+
}
374+
appendPQExpBufferChar(buf, '\'');
375+
#else /* WIN32 */
376+
int backslash_run_length = 0;
377+
378+
/*
379+
* A Windows system() argument experiences two layers of interpretation.
380+
* First, cmd.exe interprets the string. Its behavior is undocumented,
381+
* but a caret escapes any byte except LF or CR that would otherwise have
382+
* special meaning. Handling of a caret before LF or CR differs between
383+
* "cmd.exe /c" and other modes, and it is unusable here.
384+
*
385+
* Second, the new process parses its command line to construct argv (see
386+
* https://msdn.microsoft.com/en-us/library/17w5ykft.aspx). This treats
387+
* backslash-double quote sequences specially.
388+
*/
389+
appendPQExpBufferStr(buf, "^\"");
390+
for (p = str; *p; p++)
391+
{
392+
if (*p == '\n' || *p == '\r')
393+
{
394+
fprintf(stderr,
395+
_("shell command argument contains a newline or carriage return: \"%s\"\n"),
396+
str);
397+
exit(EXIT_FAILURE);
398+
}
399+
400+
/* Change N backslashes before a double quote to 2N+1 backslashes. */
401+
if (*p == '"')
402+
{
403+
while (backslash_run_length)
404+
{
405+
appendPQExpBufferStr(buf, "^\\");
406+
backslash_run_length--;
407+
}
408+
appendPQExpBufferStr(buf, "^\\");
409+
}
410+
else if (*p == '\\')
411+
backslash_run_length++;
412+
else
413+
backslash_run_length = 0;
414+
415+
/*
416+
* Decline to caret-escape the most mundane characters, to ease
417+
* debugging and lest we approach the command length limit.
418+
*/
419+
if (!((*p >= 'a' && *p <= 'z') ||
420+
(*p >= 'A' && *p <= 'Z') ||
421+
(*p >= '0' && *p <= '9')))
422+
appendPQExpBufferChar(buf, '^');
423+
appendPQExpBufferChar(buf, *p);
424+
}
425+
426+
/*
427+
* Change N backslashes at end of argument to 2N backslashes, because they
428+
* precede the double quote that terminates the argument.
429+
*/
430+
while (backslash_run_length)
431+
{
432+
appendPQExpBufferStr(buf, "^\\");
433+
backslash_run_length--;
434+
}
435+
appendPQExpBufferStr(buf, "^\"");
436+
#endif /* WIN32 */
437+
}
438+
439+
440+
/*
441+
* Append the given string to the buffer, with suitable quoting for passing
442+
* the string as a value, in a keyword/pair value in a libpq connection
443+
* string
444+
*/
445+
void
446+
appendConnStrVal(PQExpBuffer buf, const char *str)
447+
{
448+
const char *s;
449+
bool needquotes;
450+
451+
/*
452+
* If the string is one or more plain ASCII characters, no need to quote
453+
* it. This is quite conservative, but better safe than sorry.
454+
*/
455+
needquotes = true;
456+
for (s = str; *s; s++)
457+
{
458+
if (!((*s >= 'a' && *s <= 'z') || (*s >= 'A' && *s <= 'Z') ||
459+
(*s >= '0' && *s <= '9') || *s == '_' || *s == '.'))
460+
{
461+
needquotes = true;
462+
break;
463+
}
464+
needquotes = false;
465+
}
466+
467+
if (needquotes)
468+
{
469+
appendPQExpBufferChar(buf, '\'');
470+
while (*str)
471+
{
472+
/* ' and \ must be escaped by to \' and \\ */
473+
if (*str == '\'' || *str == '\\')
474+
appendPQExpBufferChar(buf, '\\');
475+
476+
appendPQExpBufferChar(buf, *str);
477+
str++;
478+
}
479+
appendPQExpBufferChar(buf, '\'');
480+
}
481+
else
482+
appendPQExpBufferStr(buf, str);
483+
}
484+
485+
486+
/*
487+
* Append a psql meta-command that connects to the given database with the
488+
* then-current connection's user, host and port.
489+
*/
490+
void
491+
appendPsqlMetaConnect(PQExpBuffer buf, const char *dbname)
492+
{
493+
const char *s;
494+
bool complex;
495+
496+
/*
497+
* If the name is plain ASCII characters, emit a trivial "\connect "foo"".
498+
* For other names, even many not technically requiring it, skip to the
499+
* general case. No database has a zero-length name.
500+
*/
501+
complex = false;
502+
for (s = dbname; *s; s++)
503+
{
504+
if (*s == '\n' || *s == '\r')
505+
{
506+
fprintf(stderr,
507+
_("database name contains a newline or carriage return: \"%s\"\n"),
508+
dbname);
509+
exit(EXIT_FAILURE);
510+
}
511+
512+
if (!((*s >= 'a' && *s <= 'z') || (*s >= 'A' && *s <= 'Z') ||
513+
(*s >= '0' && *s <= '9') || *s == '_' || *s == '.'))
514+
{
515+
complex = true;
516+
}
517+
}
518+
519+
appendPQExpBufferStr(buf, "\\connect ");
520+
if (complex)
521+
{
522+
PQExpBufferData connstr;
523+
524+
initPQExpBuffer(&connstr);
525+
appendPQExpBuffer(&connstr, "dbname=");
526+
appendConnStrVal(&connstr, dbname);
527+
528+
appendPQExpBuffer(buf, "-reuse-previous=on ");
529+
530+
/*
531+
* As long as the name does not contain a newline, SQL identifier
532+
* quoting satisfies the psql meta-command parser. Prefer not to
533+
* involve psql-interpreted single quotes, which behaved differently
534+
* before PostgreSQL 9.2.
535+
*/
536+
appendPQExpBufferStr(buf, fmtId(connstr.data));
537+
538+
termPQExpBuffer(&connstr);
539+
}
540+
else
541+
appendPQExpBufferStr(buf, fmtId(dbname));
542+
appendPQExpBufferChar(buf, '\n');
543+
}
544+
545+
342546
/*
343547
* Convert a bytea value (presented as raw bytes) to an SQL string literal
344548
* and append it to the given buffer. We assume the specified

‎src/bin/pg_dump/dumputils.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,9 @@ extern void appendStringLiteralDQ(PQExpBuffer buf, const char *str,
8181
extern void appendByteaLiteral(PQExpBuffer buf,
8282
const unsigned char *str, size_t length,
8383
bool std_strings);
84+
extern void appendShellString(PQExpBuffer buf, const char *str);
85+
extern void appendConnStrVal(PQExpBuffer buf, const char *str);
86+
extern void appendPsqlMetaConnect(PQExpBuffer buf, const char *dbname);
8487
extern bool parsePGArray(const char *atext, char ***itemarray, int *nitems);
8588
extern bool buildACLCommands(const char *name, const char *subname,
8689
const char *type, const char *acls, const char *owner,

‎src/bin/pg_dump/pg_backup.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ typedef struct _restoreOptions
102102
SimpleStringList tableNames;
103103

104104
int useDB;
105-
char *dbname;
105+
char *dbname; /* subject to expand_dbname */
106106
char *pgport;
107107
char *pghost;
108108
char *username;
@@ -120,7 +120,7 @@ typedef struct _restoreOptions
120120

121121
typedef struct _dumpOptions
122122
{
123-
const char *dbname;
123+
const char *dbname; /* subject to expand_dbname */
124124
const char *pghost;
125125
const char *pgport;
126126
const char *username;

‎src/bin/pg_dump/pg_backup_archiver.c‎

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -762,9 +762,16 @@ restore_toc_entry(ArchiveHandle *AH, TocEntry *te, bool is_parallel)
762762
/* If we created a DB, connect to it... */
763763
if (strcmp(te->desc, "DATABASE") == 0)
764764
{
765+
PQExpBufferData connstr;
766+
767+
initPQExpBuffer(&connstr);
768+
appendPQExpBufferStr(&connstr, "dbname=");
769+
appendConnStrVal(&connstr, te->tag);
770+
/* Abandon struct, but keep its buffer until process exit. */
771+
765772
ahlog(AH, 1, "connecting to new database \"%s\"\n", te->tag);
766773
_reconnectToDB(AH, te->tag);
767-
ropt->dbname = pg_strdup(te->tag);
774+
ropt->dbname = connstr.data;
768775
}
769776
}
770777

@@ -2921,12 +2928,17 @@ _reconnectToDB(ArchiveHandle *AH, const char *dbname)
29212928
ReconnectToServer(AH, dbname, NULL);
29222929
else
29232930
{
2924-
PQExpBuffer qry = createPQExpBuffer();
2931+
if (dbname)
2932+
{
2933+
PQExpBufferData connectbuf;
29252934

2926-
appendPQExpBuffer(qry, "\\connect %s\n\n",
2927-
dbname ? fmtId(dbname) : "-");
2928-
ahprintf(AH, "%s", qry->data);
2929-
destroyPQExpBuffer(qry);
2935+
initPQExpBuffer(&connectbuf);
2936+
appendPsqlMetaConnect(&connectbuf, dbname);
2937+
ahprintf(AH, "%s\n", connectbuf.data);
2938+
termPQExpBuffer(&connectbuf);
2939+
}
2940+
else
2941+
ahprintf(AH, "%s\n", "\\connect -\n");
29302942
}
29312943

29322944
/*
@@ -4400,7 +4412,7 @@ CloneArchive(ArchiveHandle *AH)
44004412
}
44014413
else
44024414
{
4403-
char *dbname;
4415+
PQExpBufferData connstr;
44044416
char *pghost;
44054417
char *pgport;
44064418
char *username;
@@ -4413,14 +4425,18 @@ CloneArchive(ArchiveHandle *AH)
44134425
* because all just return a pointer and do not actually send/receive
44144426
* any data to/from the database.
44154427
*/
4416-
dbname = PQdb(AH->connection);
4428+
initPQExpBuffer(&connstr);
4429+
appendPQExpBuffer(&connstr, "dbname=");
4430+
appendConnStrVal(&connstr, PQdb(AH->connection));
44174431
pghost = PQhost(AH->connection);
44184432
pgport = PQport(AH->connection);
44194433
username = PQuser(AH->connection);
44204434

44214435
/* this also sets clone->connection */
4422-
ConnectDatabase((Archive *) clone, dbname, pghost, pgport, username, TRI_NO);
4436+
ConnectDatabase((Archive *) clone, connstr.data,
4437+
pghost, pgport, username, TRI_NO);
44234438

4439+
termPQExpBuffer(&connstr);
44244440
/* setupDumpWorker will fix up connection state */
44254441
}
44264442

‎src/bin/pg_dump/pg_backup_db.c‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@ ReconnectToServer(ArchiveHandle *AH, const char *dbname, const char *username)
128128
static PGconn *
129129
_connectDB(ArchiveHandle *AH, const char *reqdb, const char *requser)
130130
{
131+
PQExpBufferData connstr;
131132
PGconn *newConn;
132133
const char *newdb;
133134
const char *newuser;
@@ -156,6 +157,10 @@ _connectDB(ArchiveHandle *AH, const char *reqdb, const char *requser)
156157
exit_horribly(modulename, "out of memory\n");
157158
}
158159

160+
initPQExpBuffer(&connstr);
161+
appendPQExpBuffer(&connstr, "dbname=");
162+
appendConnStrVal(&connstr, newdb);
163+
159164
do
160165
{
161166
const char *keywords[7];
@@ -170,7 +175,7 @@ _connectDB(ArchiveHandle *AH, const char *reqdb, const char *requser)
170175
keywords[3] = "password";
171176
values[3] = password;
172177
keywords[4] = "dbname";
173-
values[4] = newdb;
178+
values[4] = connstr.data;
174179
keywords[5] = "fallback_application_name";
175180
values[5] = progname;
176181
keywords[6] = NULL;
@@ -222,6 +227,8 @@ _connectDB(ArchiveHandle *AH, const char *reqdb, const char *requser)
222227
if (password)
223228
free(password);
224229

230+
termPQExpBuffer(&connstr);
231+
225232
/* check for version mismatch */
226233
_check_database_version(AH);
227234

0 commit comments

Comments
 (0)