10000 Obstruct shell, SQL, and conninfo injection via database and role names. · s-monk/postgres@e8f4922 · GitHub
[go: up one dir, main page]
Skip to content

Commit e8f4922

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 f1d0b09 commit e8f4922

19 files changed

+622
-192
lines changed

‎contrib/pg_upgrade/pg_upgrade.h

Copy file name to clipboard
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
#include <sys/time.h>
1313

1414
#include "libpq-fe.h"
15+
#include "pqexpbuffer.h"
1516

1617
/* Use port in the private/dynamic port number range */
1718
#define DEF_PGUPORT 50432
@@ -427,6 +428,9 @@ void check_pghost_envvar(void);
427428
/* util.c */
428429

429430
char *quote_identifier(const char *s);
431+
extern void appendShellString(PQExpBuffer buf, const char *str);
432+
extern void appendConnStrVal(PQExpBuffer buf, const char *str);
433+
extern void appendPsqlMetaConnect(PQExpBuffer buf, const char *dbname);
430434
int get_user_info(char **user_name);
431435
void check_ok(void);
432436
void

‎contrib/pg_upgrade/server.c

Lines changed: 31 additions & 19 deletions
88
Original file line numberDiff line numberDiff line change
@@ -51,18 +51,25 @@ connectToServer(ClusterInfo *cluster, const char *db_name)
5151
static PGconn *
5252
get_db_conn(ClusterInfo *cluster, const char *db_name)
5353
{
54-
char conn_opts[2 * NAMEDATALEN + MAXPGPATH + 100];
54+
PQExpBufferData conn_opts;
55+
PGconn *conn;
5556

57+
/* Build connection string with proper quoting */
58+
initPQExpBuffer(&conn_opts);
59+
appendPQExpBufferStr(&conn_opts, "dbname=");
60+
appendConnStrVal(&conn_opts, db_name);
61+
appendPQExpBufferStr(&conn_opts, " user=");
62+
appendConnStrVal(&conn_opts, os_info.user);
63+
appendPQExpBuffer(&conn_opts, " port=%d", cluster->port);
5664
if (cluster->sockdir)
57-
snprintf(conn_opts, sizeof(conn_opts),
58-
"dbname = '%s' user = '%s' host = '%s' port = %d",
59-
db_name, os_info.user, cluster->sockdir, cluster->port);
60-
else
61-
snprintf(conn_opts, sizeof(conn_opts),
62-
"dbname = '%s' user = '%s' port = %d",
63-
db_name, os_info.user, cluster->port);
65+
{
66+
appendPQExpBufferStr(&conn_opts, " host=");
67+
appendConnStrVal(&conn_opts, cluster->sockdir);
68+
}
6469

65-
return PQconnectdb(conn_opts);
70+
conn = PQconnectdb(conn_opts.data);
71+
termPQExpBuffer(&conn_opts);
72+
return conn;
6673
}
6774

6875

@@ -74,23 +81,28 @@ get_db_conn(ClusterInfo *cluster, const char *db_name)
7481
* sets, but the utilities we need aren't very consistent about the treatment
7582
* of database name options, so we leave that out.
7683
*
77-
* Note result is in static storage, so use it right away.
84+
* Result is valid until the next call to this function.
7885
*/
7986
char *
8087
cluster_conn_opts(ClusterInfo *cluster)
81
{
82-
static char conn_opts[MAXPGPATH + NAMEDATALEN + 100];
89+
static PQExpBuffer buf;
8390

84-
if (cluster->sockdir)
85-
snprintf(conn_opts, sizeof(conn_opts),
86-
"--host \"%s\" --port %d --username \"%s\"",
87-
cluster->sockdir, cluster->port, os_info.user);
91+
if (buf == NULL)
92+
buf = createPQExpBuffer();
8893
else
89-
snprintf(conn_opts, sizeof(conn_opts),
90-
"--port %d --username \"%s\"",
91-
cluster->port, os_info.user);
94+
resetPQExpBuffer(buf);
95+
96+
if (cluster->sockdir)
97+
{
98+
appendPQExpBufferStr(buf, "--host ");
99+
appendShellString(buf, cluster->sockdir);
100+
appendPQExpBufferChar(buf, ' ');
101+
}
102+
appendPQExpBuffer(buf, "--port %d --username ", cluster->port);
103+
appendShellString(buf, os_info.user);
92104

93-
return conn_opts;
105+
return buf->data;
94106
}
95107

96108

‎contrib/pg_upgrade/test.sh

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,20 @@ set -x
131131

132132
standard_initdb "$oldbindir"/initdb
133133
$oldbindir/pg_ctl start -l "$logdir/postmaster1.log" -o "$POSTMASTER_OPTS" -w
134+
135+
# Create databases with names covering the ASCII bytes other than NUL, BEL,
136+
# LF, or CR. BEL would ring the terminal bell in the course of this test, and
137+
# it is not otherwise a special case. PostgreSQL doesn't support the rest.
138+
dbname1=`awk 'BEGIN { for (i= 1; i < 46; i++)
139+
if (i != 7 && i != 10 && i != 13) printf "%c", i }' </dev/null`
140+
# Exercise backslashes adjacent to double quotes, a Windows special case.
141+
dbname1='\"\'$dbname1'\\"\\\'
142+
dbname2=`awk 'BEGIN { for (i = 46; i < 91; i++) printf "%c", i }' </dev/null`
143+
dbname3=`awk 'BEGIN { for (i = 91; i < 128; i++) printf "%c", i }' </dev/null`
144+
createdb "$dbname1" || createdb_status=$?
145+
createdb "$dbname2" || createdb_status=$?
146+
createdb "$dbname3" || createdb_status=$?
147+
134148
if "$MAKE" -C "$oldsrc" installcheck; then
135149
pg_dumpall -f "$temp_root"/dump1.sql || pg_dumpall1_status=$?
136150
if [ "$newsrc" != "$oldsrc" ]; then
@@ -156,6 +170,9 @@ else
156170
make_installcheck_status=$?
157171
fi
158172
$oldbindir/pg_ctl -m fast stop
173+
if [ -n "$createdb_status" ]; then
174+
exit 1
175+
fi
159176
if [ -n "$make_installcheck_status" ]; then
160177
exit 1
161178
fi

‎contrib/pg_upgrade/util.c

Lines changed: 204 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,210 @@ quote_identifier(const char *s)
158158
}
159159

160160

161+
/*
162+
* Append the given string to the shell command being built in the buffer,
163+
* with suitable shell-style quoting to create exactly one argument.
164+
*
165+
* Forbid LF or CR characters, which have scant practical use beyond designing
166+
* security breaches. The Windows command shell is unusable as a conduit for
167+
* arguments containing LF or CR characters. A future major release should
168+
* reject those characters in CREATE ROLE and CREATE DATABASE, because use
169+
* there eventually leads to errors here.
170+
*/
171+
void
172+
appendShellString(PQExpBuffer buf, const char *str)
173+
{
174+
const char *p;
175+
176+
#ifndef WIN32
177+
appendPQExpBufferChar(buf, '\'');
178+
for (p = str; *p; p++)
179+
{
180+
if (*p == '\n' || *p == '\r')
181+
{
182+
fprintf(stderr,
183+
_("shell command argument contains a newline or carriage return: \"%s\"\n"),
184+
str);
185+
exit(EXIT_FAILURE);
186+
}
187+
188+
if (*p == '\'')
189+
appendPQExpBufferStr(buf, "'\"'\"'");
190+
else
191+
appendPQExpBufferChar(buf, *p);
192+
}
193+
appendPQExpBufferChar(buf, '\'');
194+
#else /* WIN32 */
195+
int backslash_run_length = 0;
196+
197+
/*
198+
* A Windows system() argument experiences two layers of interpretation.
199+
* First, cmd.exe interprets the string. Its behavior is undocumented,
200+
* but a caret escapes any byte except LF or CR that would otherwise have
201+
* special meaning. Handling of a caret before LF or CR differs between
202+
* "cmd.exe /c" and other modes, and it is unusable here.
203+
*
204+
* Second, the new process parses its command line to construct argv (see
205+
* https://msdn.microsoft.com/en-us/library/17w5ykft.aspx). This treats
206+
* backslash-double quote sequences specially.
207+
*/
208+
appendPQExpBufferStr(buf, "^\"");
209+
for (p = str; *p; p++)
210+
{
211+
if (*p == '\n' || *p == '\r')
212+
{
213+
fprintf(stderr,
214+
_("shell command argument contains a newline or carriage return: \"%s\"\n"),
215+
str);
216+
exit(EXIT_FAILURE);
217+
}
218+
219+
/* Change N backslashes before a double quote to 2N+1 backslashes. */
220+
if (*p == '"')
221+
{
222+
while (backslash_run_length)
223+
{
224+
appendPQExpBufferStr(buf, "^\\");
225+
backslash_run_length--;
226+
}
227+
appendPQExpBufferStr(buf, "^\\");
228+
}
229+
else if (*p == '\\')
230+
backslash_run_length++;
231+
else
232+
backslash_run_length = 0;
233+
234+
/*
235+
* Decline to caret-escape the most mundane characters, to ease
236+
* debugging and lest we approach the command length limit.
237+
*/
238+
if (!((*p >= 'a' && *p <= 'z') ||
239+
(*p >= 'A' && *p <= 'Z') ||
240+
(*p >= '0' && *p <= '9')))
241+
appendPQExpBufferChar(buf, '^');
242+
appendPQExpBufferChar(buf, *p);
243+
}
244+
245+
/*
246+
* Change N backslashes at end of argument to 2N backslashes, because they
247+
* precede the double quote that terminates the argument.
248+
*/
249+
while (backslash_run_length)
250+
{
251+
appendPQExpBufferStr(buf, "^\\");
252+
backslash_run_length--;
253+
}
254+
appendPQExpBufferStr(buf, "^\"");
255+
#endif /* WIN32 */
256+
}
257+
258+
259+
/*
260+
* Append the given string to the buffer, with suitable quoting for passing
261+
* the string as a value, in a keyword/pair value in a libpq connection
262+
* string
263+
*/
264+
void
265+
appendConnStrVal(PQExpBuffer buf, const char *str)
266+
{
267+
const char *s;
268+
bool needquotes;
269+
270+
/*
271+
* If the string is one or more plain ASCII characters, no need to quote
272+
* it. This is quite conservative, but better safe than sorry.
273+
*/
274+
needquotes = true;
275+
for (s = str; *s; s++)
276+
{
277+
if (!((*s >= 'a' && *s <= 'z') || (*s >= 'A' && *s <= 'Z') ||
278+
(*s >= '0' && *s <= '9') || *s == '_' || *s == '.'))
279+
{
280+
needquotes = true;
281+
break;
282+
}
283+
needquotes = false;
284+
}
285+
286+
if (needquotes)
287+
{
288+
appendPQExpBufferChar(buf, '\'');
289+
while (*str)
290+
{
291+
/* ' and \ must be escaped by to \' and \\ */
292+
if (*str == '\'' || *str == '\\')
293+
appendPQExpBufferChar(buf, '\\');
294+
295+
appendPQExpBufferChar(buf, *str);
296+
str++;
297+
}
298+
appendPQExpBufferChar(buf, '\'');
299+
}
300+
else
301+
appendPQExpBufferStr(buf, str);
302+
}
303+
304+
305+
/*
306+
* Append a psql meta-command that connects to the given database with the
307+
* then-current connection's user, host and port.
308+
*/
309+
void
310+
appendPsqlMetaConnect(PQExpBuffer buf, const char *dbname)
311+
{
312+
const char *s;
313+
bool complex;
314+
315+
/*
316+
* If the name is plain ASCII characters, emit a trivial "\connect "foo"".
317+
* For other names, even many not technically requiring it, skip to the
318+
* general case. No database has a zero-length name.
319+
*/
320+
complex = false;
321+
for (s = dbname; *s; s++)
322+
{
323+
if (*s == '\n' || *s == '\r')
324+
{
325+
fprintf(stderr,
326+
_("database name contains a newline or carriage return: \"%s\"\n"),
327+
dbname);
328+
exit(EXIT_FAILURE);
329+
}
330+
331+
if (!((*s >= 'a' && *s <= 'z') || (*s >= 'A' && *s <= 'Z') ||
332+
(*s >= '0' && *s <= '9') || *s == '_' || *s == '.'))
333+
{
334+
complex = true;
335+
}
336+
}
337+
338+
appendPQExpBufferStr(buf, "\\connect ");
339+
if (complex)
340+
{
341+
PQExpBufferData connstr;
342+
343+
initPQExpBuffer(&connstr);
344+
appendPQExpBuffer(&connstr, "dbname=");
345+
appendConnStrVal(&connstr, dbname);
346+
347+
appendPQExpBuffer(buf, "-reuse-previous=on ");
348+
349+
/*
350+
* As long as the name does not contain a newline, SQL identifier
351+
* quoting satisfies the psql meta-command 10000 parser. Prefer not to
352+
* involve psql-interpreted single quotes, which behaved differently
353+
* before PostgreSQL 9.2.
354+
*/
355+
appendPQExpBufferStr(buf, quote_identifier(connstr.data));
356+
357+
termPQExpBuffer(&connstr);
358+
}
359+
else
360+
appendPQExpBufferStr(buf, quote_identifier(dbname));
361+
appendPQExpBufferChar(buf, '\n');
362+
}
363+
364+
161365
/*
162366
* get_user_info()
163367
* (copied from initdb.c) find the current user

‎contrib/pg_upgrade/version.c

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,10 +48,16 @@ new_9_0_populate_pg_largeobject_metadata(ClusterInfo *cluster, bool check_mode)
4848
found = true;
4949
if (!check_mode)
5050
{
51+
PQExpBufferData connectbuf;
52+
5153
if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
5254
pg_log(PG_FATAL, "could not open file \"%s\": %s\n", output_path, getErrorText(errno));
53-
fprintf(script, "\\connect %s\n",
54-
quote_identifier(active_db->db_name));
55+
56+
initPQExpBuffer(&connectbuf);
57+
appendPsqlMetaConnect(&connectbuf, active_db->db_name);
58+
fputs(connectbuf.data, script);
59+
termPQExpBuffer(&connectbuf);
60+
5561
fprintf(script,
5662
"SELECT pg_catalog.lo_create(t.loid)\n"
5763
"FROM (SELECT DISTINCT loid FROM pg_catalog.pg_largeobject) AS t;\n");

0 commit comments

Comments
 (0)
0