Skip to content

Commit 928bca1

Browse files
nmischnmisch
committed
Empty search_path in Autovacuum and non-psql/pgbench clients.
This makes the client programs behave as documented regardless of the connect-time search_path and regardless of user-created objects. Today, a malicious user with CREATE permission on a search_path schema can take control of certain of these clients' queries and invoke arbitrary SQL functions under the client identity, often a superuser. This is exploitable in the default configuration, where all users have CREATE privilege on schema "public". This changes behavior of user-defined code stored in the database, like pg_index.indexprs and pg_extension_config_dump(). If they reach code bearing unqualified names, "does not exist" or "no schema has been selected to create in" errors might appear. Users may fix such errors by schema-qualifying affected names. After upgrading, consider watching server logs for these errors. The --table arguments of src/bin/scripts clients have been lax; for example, "vacuumdb -Zt pg_am\;CHECKPOINT" performed a checkpoint. That now fails, but for now, "vacuumdb -Zt 'pg_am(amname);CHECKPOINT'" still performs a checkpoint. Back-patch to 9.3 (all supported versions). Reviewed by Tom Lane, though this fix strategy was not his first choice. Reported by Arseniy Sharoglazov. Security: CVE-2018-1058
1 parent 461c32b commit 928bca1

File tree

27 files changed

+335
-79
lines changed

27 files changed

+335
-79
lines changed

‎contrib/oid2name/oid2name.c

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
*/
1010
#include "postgres_fe.h"
1111

12+
#include "fe_utils/connect.h"
1213
#include "libpq-fe.h"
1314
#include "pg_getopt.h"
1415

@@ -263,6 +264,7 @@ sql_conn(struct options * my_opts)
263264
PGconn *conn;
264265
char *password = NULL;
265266
bool new_pass;
267+
PGresult *res;
266268

267269
/*
268270
* Start the connection. Loop until we have a password if requested by
@@ -322,6 +324,17 @@ sql_conn(struct options * my_opts)
322324
exit(1);
323325
}
324326

327+
res = PQexec(conn, ALWAYS_SECURE_SEARCH_PATH_SQL);
328+
if (PQresultStatus(res) != PGRES_TUPLES_OK)
329+
{
330+
fprintf(stderr, "oid2name: could not clear search_path: %s\n",
331+
PQerrorMessage(conn));
332+
PQclear(res);
333+
PQfinish(conn);
334+
exit(-1);
335+
}
336+
PQclear(res);
337+
325338
/* return the conn if good */
326339
return conn;
327340
}

‎contrib/pg_upgrade/server.c

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
#include "postgres_fe.h"
1111

12+
#include "fe_utils/connect.h"
1213
#include "pg_upgrade.h"
1314

1415

@@ -39,6 +40,8 @@ connectToServer(ClusterInfo *cluster, const char *db_name)
3940
exit(1);
4041
}
4142

43+
PQclear(executeQueryOrDie(conn, ALWAYS_SECURE_SEARCH_PATH_SQL));
44+
4245
return conn;
4346
}
4447

‎contrib/vacuumlo/vacuumlo.c

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
#include <termios.h>
2222
#endif
2323

24+
#include "fe_utils/connect.h"
2425
#include "libpq-fe.h"
2526
#include "pg_getopt.h"
2627

@@ -135,11 +136,8 @@ vacuumlo(const char *database, const struct _param * param)
135136
fprintf(stdout, "Test run: no large objects will be removed!\n");
136137
}
137138

138-
/*
139-
* Don't get fooled by any non-system catalogs
140-
*/
141-
res = PQexec(conn, "SET search_path = pg_catalog");
142-
if (PQresultStatus(res) != PGRES_COMMAND_OK)
139+
res = PQexec(conn, ALWAYS_SECURE_SEARCH_PATH_SQL);
140+
if (PQresultStatus(res) != PGRES_TUPLES_OK)
143141
{
144142
fprintf(stderr, "Failed to set search_path:\n");
145143
fprintf(stderr, "%s", PQerrorMessage(conn));

‎src/backend/postmaster/autovacuum.c

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -556,6 +556,12 @@ AutoVacLauncherMain(int argc, char *argv[])
556556
/* must unblock signals before calling rebuild_database_list */
557557
PG_SETMASK(&UnBlockSig);
558558

559+
/*
560+
* Set always-secure search path. Launcher doesn't connect to a database,
561+
* so this has no effect.
562+
*/
563+
SetConfigOption("search_path", "", PGC_SUSET, PGC_S_OVERRIDE);
564+
559565
/*
560566
* Force zero_damaged_pages OFF in the autovac process, even if it is set
561567
* in postgresql.conf. We don't really want such a dangerous option being
@@ -1598,6 +1604,14 @@ AutoVacWorkerMain(int argc, char *argv[])
15981604

15991605
PG_SETMASK(&UnBlockSig);
16001606

1607+
/*
1608+
* Set always-secure search path, so malicious users can't redirect user
1609+
* code (e.g. pg_index.indexprs). (That code runs in a
1610+
* SECURITY_RESTRICTED_OPERATION sandbox, so malicious users could not
1611+
* take control of the entire autovacuum worker in any case.)
1612+
*/
1613+
SetConfigOption("search_path", "", PGC_SUSET, PGC_S_OVERRIDE);
1614+
16011615
/*
16021616
* Force zero_damaged_pages OFF in the autovac process, even if it is set
16031617
* in postgresql.conf. We don't really want such a dangerous option being

‎src/bin/pg_basebackup/streamutil.c

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929

3030
#include "common/fe_memutils.h"
3131
#include "datatype/timestamp.h"
32+
#include "fe_utils/connect.h"
3233

3334
const char *progname;
3435
char *connection_string = NULL;
@@ -205,6 +206,23 @@ GetConnection(void)
205206
if (conn_opts)
206207
PQconninfoFree(conn_opts);
207208

209+
/* Set always-secure search path, so malicious users can't get control. */
210+
if (dbname != NULL)
211+
{
212+
PGresult *res;
213+
214+
res = PQexec(tmpconn, ALWAYS_SECURE_SEARCH_PATH_SQL);
215+
if (PQresultStatus(res) != PGRES_TUPLES_OK)
216+
{
217+
fprintf(stderr, _("%s: could not clear search_path: %s\n"),
218+
progname, PQerrorMessage(tmpconn));
219+
PQclear(res);
220+
PQfinish(tmpconn);
221+
exit(1);
222+
}
223+
PQclear(res);
224+
}
225+
208226
/*
209227
* Ensure we have the same value of integer timestamps as the server we
210228
* are connecting to.

‎src/bin/pg_dump/dumputils.c

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1376,8 +1376,9 @@ processSQLNamePattern(PGconn *conn, PQExpBuffer buf, const char *pattern,
13761376
}
13771377

13781378
/*
1379-
* Now decide what we need to emit. Note there will be a leading "^(" in
1380-
* the patterns in any case.
1379+
* Now decide what we need to emit. We may run under a hostile
1380+
* search_path, so qualify EVERY name. Note there will be a leading "^("
1381+
* in the patterns in any case.
13811382
*/
13821383
if (namebuf.len > 2)
13831384
{
@@ -1390,15 +1391,18 @@ processSQLNamePattern(PGconn *conn, PQExpBuffer buf, const char *pattern,
13901391
WHEREAND();
13911392
if (altnamevar)
13921393
{
1393-
appendPQExpBuffer(buf, "(%s ~ ", namevar);
1394+
appendPQExpBuffer(buf,
1395+
"(%s OPERATOR(pg_catalog.~) ", namevar);
13941396
appendStringLiteralConn(buf, namebuf.data, conn);
1395-
appendPQExpBuffer(buf, "\n OR %s ~ ", altnamevar);
1397+
appendPQExpBuffer(buf,
1398+
"\n OR %s OPERATOR(pg_catalog.~) ",
1399+
altnamevar);
13961400
appendStringLiteralConn(buf, namebuf.data, conn);
13971401
appendPQExpBufferStr(buf, ")\n");
13981402
}
13991403
else
14001404
{
1401-
appendPQExpBuffer(buf, "%s ~ ", namevar);
1405+
appendPQExpBuffer(buf, "%s OPERATOR(pg_catalog.~) ", namevar);
14021406
appendStringLiteralConn(buf, namebuf.data, conn);
14031407
appendPQExpBufferChar(buf, '\n');
14041408
}
@@ -1414,7 +1418,7 @@ processSQLNamePattern(PGconn *conn, PQExpBuffer buf, const char *pattern,
14141418
if (strcmp(schemabuf.data, "^(.*)$") != 0 && schemavar)
14151419
{
14161420
WHEREAND();
1417-
appendPQExpBuffer(buf, "%s ~ ", schemavar);
1421+
appendPQExpBuffer(buf, "%s OPERATOR(pg_catalog.~) ", schemavar);
14181422
appendStringLiteralConn(buf, schemabuf.data, conn);
14191423
appendPQExpBufferChar(buf, '\n');
14201424
}

‎src/bin/pg_dump/pg_backup_db.c

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
*-------------------------------------------------------------------------
1111
*/
1212

13+
#include "fe_utils/connect.h"
1314
#include "pg_backup_db.h"
1415
#include "pg_backup_utils.h"
1516
#include "dumputils.h"
@@ -96,6 +97,11 @@ ReconnectToServer(ArchiveHandle *AH, const char *dbname, const char *username)
9697
PQfinish(AH->connection);
9798
AH->connection = newConn;
9899

100+
/* Start strict; later phases may override this. */
101+
if (PQserverVersion(AH->connection) >= 70300)
102+
PQclear(ExecuteSqlQueryForSingleRow((Archive *) AH,
103+
ALWAYS_SECURE_SEARCH_PATH_SQL));
104+
99105
return 1;
100106
}
101107

@@ -304,6 +310,11 @@ ConnectDatabase(Archive *AHX,
304310
PQdb(AH->connection) ? PQdb(AH->connection) : "",
305311
PQerrorMessage(AH->connection));
306312

313+
/* Start strict; later phases may override this. */
314+
if (PQserverVersion(AH->connection) >= 70300)
315+
PQclear(ExecuteSqlQueryForSingleRow((Archive *) AH,
316+
ALWAYS_SECURE_SEARCH_PATH_SQL));
317+
307318
/*
308319
* We want to remember connection's actual password, whether or not we got
309320
* it by prompting. So we don't just store the password variable.

‎src/bin/pg_dump/pg_dump.c

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@
6161
#include "pg_backup_db.h"
6262
#include "pg_backup_utils.h"
6363
#include "dumputils.h"
64+
#include "fe_utils/connect.h"
6465
#include "parallel.h"
6566

6667

@@ -979,6 +980,9 @@ setup_connection(Archive *AH, const char *dumpencoding, char *use_role)
979980
PGconn *conn = GetConnection(AH);
980981
const char *std_strings;
981982

983+
if (AH->remoteVersion >= 70300)
984+
PQclear(ExecuteSqlQueryForSingleRow(AH, ALWAYS_SECURE_SEARCH_PATH_SQL));
985+
982986
/*
983987
* Set the client encoding if requested.
984988
*/
@@ -1245,21 +1249,30 @@ expand_table_name_patterns(Archive *fout,
12451249

12461250
for (cell = patterns->head; cell; cell = cell->next)
12471251
{
1252+
/*
1253+
* Query must remain ABSOLUTELY devoid of unqualified names. This
1254+
* would be unnecessary given a pg_table_is_visible() variant taking a
1255+
* search_path argument.
1256+
*/
12481257
if (cell != patterns->head)
12491258
appendPQExpBufferStr(query, "UNION ALL\n");
12501259
appendPQExpBuffer(query,
12511260
"SELECT c.oid"
12521261
"\nFROM pg_catalog.pg_class c"
1253-
"\n LEFT JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace"
1254-
"\nWHERE c.relkind in ('%c', '%c', '%c', '%c', '%c')\n",
1262+
"\n LEFT JOIN pg_catalog.pg_namespace n"
1263+
"\n ON n.oid OPERATOR(pg_catalog.=) c.relnamespace"
1264+
"\nWHERE c.relkind OPERATOR(pg_catalog.=) ANY"
1265+
"\n (array['%c', '%c', '%c', '%c', '%c'])\n",
12551266
RELKIND_RELATION, RELKIND_SEQUENCE, RELKIND_VIEW,
12561267
RELKIND_MATVIEW, RELKIND_FOREIGN_TABLE);
12571268
processSQLNamePattern(GetConnection(fout), query, cell->val, true,
12581269
false, "n.nspname", "c.relname", NULL,
12591270
"pg_catalog.pg_table_is_visible(c.oid)");
12601271
}
12611272

1273+
ExecuteSqlStatement(fout, "RESET search_path");
12621274
res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
1275+
PQclear(ExecuteSqlQueryForSingleRow(fout, ALWAYS_SECURE_SEARCH_PATH_SQL));
12631276

12641277
for (i = 0; i < PQntuples(res); i++)
12651278
{

‎src/bin/pg_dump/pg_dumpall.c

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626

2727
#include "dumputils.h"
2828
#include "pg_backup.h"
29+
#include "fe_utils/connect.h"
2930

3031
/* version string we expect back from pg_dump */
3132
#define PGDUMP_VERSIONSTR "pg_dump (PostgreSQL) " PG_VERSION "\n"
@@ -1970,12 +1971,8 @@ connectDatabase(const char *dbname, const char *connection_string,
19701971
exit_nicely(1);
19711972
}
19721973

1973-
/*
1974-
* On 7.3 and later, make sure we are not fooled by non-system schemas in
1975-
* the search path.
1976-
*/
19771974
if (server_version >= 70300)
1978-
executeCommand(conn, "SET search_path = pg_catalog");
1975+
PQclear(executeQuery(conn, ALWAYS_SECURE_SEARCH_PATH_SQL));
19791976

19801977
return conn;
19811978
}

‎src/bin/scripts/Makefile

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -25,16 +25,17 @@ all: $(PROGRAMS)
2525
%: %.o $(WIN32RES)
2626
$(CC) $(CFLAGS) $^ $(libpq_pgport) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
2727

28-
createdb: createdb.o common.o dumputils.o kwlookup.o keywords.o | submake-libpq submake-libpgport
29-
createlang: createlang.o common.o print.o mbprint.o | submake-libpq submake-libpgport
30-
createuser: createuser.o common.o dumputils.o kwlookup.o keywords.o | submake-libpq submake-libpgport
31-
dropdb: dropdb.o common.o dumputils.o kwlookup.o keywords.o | submake-libpq submake-libpgport
32-
droplang: droplang.o common.o print.o mbprint.o | submake-libpq submake-libpgport
33-
dropuser: dropuser.o common.o dumputils.o kwlookup.o keywords.o | submake-libpq submake-libpgport
34-
clusterdb: clusterdb.o common.o dumputils.o kwlookup.o keywords.o | submake-libpq submake-libpgport
35-
vacuumdb: vacuumdb.o common.o dumputils.o kwlookup.o keywords.o | submake-libpq submake-libpgport
36-
reindexdb: reindexdb.o common.o dumputils.o kwlookup.o keywords.o | submake-libpq submake-libpgport
37-
pg_isready: pg_isready.o common.o | submake-libpq submake-libpgport
28+
SCRIPTS_COMMON = common.o dumputils.o kwlookup.o keywords.o
29+
createdb: createdb.o $(SCRIPTS_COMMON) | submake-libpq submake-libpgport
30+
createlang: createlang.o $(SCRIPTS_COMMON) print.o mbprint.o | submake-libpq submake-libpgport
31+
createuser: createuser.o $(SCRIPTS_COMMON) | submake-libpq submake-libpgport
32+
dropdb: dropdb.o $(SCRIPTS_COMMON) | submake-libpq submake-libpgport
33+
droplang: droplang.o $(SCRIPTS_COMMON) print.o mbprint.o | submake-libpq submake-libpgport
34+
dropuser: dropuser.o $(SCRIPTS_COMMON) | submake-libpq submake-libpgport
35+
clusterdb: clusterdb.o $(SCRIPTS_COMMON) | submake-libpq submake-libpgport
36+
vacuumdb: vacuumdb.o $(SCRIPTS_COMMON) | submake-libpq submake-libpgport
37+
reindexdb: reindexdb.o $(SCRIPTS_COMMON) | submake-libpq submake-libpgport
38+
pg_isready: pg_isready.o $(SCRIPTS_COMMON) | submake-libpq submake-libpgport
3839

3940
dumputils.c keywords.c: % : $(top_srcdir)/src/bin/pg_dump/%
4041
rm -f $@ && $(LN_S) $< .
@@ -65,7 +66,7 @@ uninstall:
6566

6667
clean distclean maintainer-clean:
6768
rm -f $(addsuffix $(X), $(PROGRAMS)) $(addsuffix .o, $(PROGRAMS))
68-
rm -f common.o dumputils.o kwlookup.o keywords.o print.o mbprint.o $(WIN32RES)
69+
rm -f $(SCRIPTS_COMMON) print.o mbprint.o $(WIN32RES)
6970
rm -f dumputils.c print.c mbprint.c kwlookup.c keywords.c
7071
rm -rf tmp_check
7172

‎src/bin/scripts/clusterdb.c

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -194,17 +194,21 @@ cluster_one_database(const char *dbname, bool verbose, const char *table,
194194

195195
PGconn *conn;
196196

197+
conn = connectDatabase(dbname, host, port, username, prompt_password,
198+
progname, echo, false);
199+
197200
initPQExpBuffer(&sql);
198201

199202
appendPQExpBufferStr(&sql, "CLUSTER");
200203
if (verbose)
201204
appendPQExpBufferStr(&sql, " VERBOSE");
202205
if (table)
203-
appendPQExpBuffer(&sql, " %s", table);
204-
appendPQExpBufferStr(&sql, ";");
206+
{
207+
appendPQExpBufferChar(&sql, ' ');
208+
appendQualifiedRelation(&sql, table, conn, progname, echo);
209+
}
210+
appendPQExpBufferChar(&sql, ';');
205211

206-
conn = connectDatabase(dbname, host, port, username, prompt_password,
207-
progname, false);
208212
if (!executeMaintenanceCommand(conn, sql.data, echo))
209213
{
210214
if (table)
@@ -233,7 +237,7 @@ cluster_all_databases(bool verbose, const char *maintenance_db,
233237
int i;
234238

235239
conn = connectMaintenanceDatabase(maintenance_db, host, port, username,
236-
prompt_password, progname);
240+
prompt_password, progname, echo);
237241
result = executeQuery(conn, "SELECT datname FROM pg_database WHERE datallowconn ORDER BY 1;", progname, echo);
238242
PQfinish(conn);
239243

0 commit comments

Comments
 (0)