Skip to content

Commit b3f6b14

Browse files
author
Amit Kapila
committed
Fixups for commit 93db6cb.
Ensure to set always-secure search path for both local and remote connections during slot synchronization, so that malicious users can't redirect user code (e.g. operators). In the passing, improve the name of define, remove spurious return statement, and a minor change in one of the comments. Author: Bertrand Drouvot and Shveta Malik Reviewed-by: Amit Kapila, Peter Smith Discussion: https://postgr.es/m/514f6f2f-6833-4539-39f1-96cd1e011f23@enterprisedb.com Discussion: https://postgr.es/m/ZdcejBDCr+wlVGnO@ip-10-97-1-34.eu-west-3.compute.internal Discussion: https://postgr.es/m/CAJpy0uBNP=nrkNJkJSfF=jSocEh8vU2Owa8Rtpi=63fG=SvfVQ@mail.gmail.com
1 parent ada87a4 commit b3f6b14

File tree

4 files changed

+82
-16
lines changed

4 files changed

+82
-16
lines changed

‎src/backend/access/transam/xlogrecovery.c

+8-7
Original file line numberDiff line numberDiff line change
@@ -1472,13 +1472,14 @@ FinishWalRecovery(void)
14721472
* Shutdown the slot sync worker to drop any temporary slots acquired by
14731473
* it and to prevent it from keep trying to fetch the failover slots.
14741474
*
1475-
* We do not update the 'synced' column from true to false here, as any
1476-
* failed update could leave 'synced' column false for some slots. This
1477-
* could cause issues during slot sync after restarting the server as a
1478-
* standby. While updating the 'synced' column after switching to the new
1479-
* timeline is an option, it does not simplify the handling for the
1480-
* 'synced' column. Therefore, we retain the 'synced' column as true after
1481-
* promotion as it may provide useful information about the slot origin.
1475+
* We do not update the 'synced' column in 'pg_replication_slots' system
1476+
* view from true to false here, as any failed update could leave 'synced'
1477+
* column false for some slots. This could cause issues during slot sync
1478+
* after restarting the server as a standby. While updating the 'synced'
1479+
* column after switching to the new timeline is an option, it does not
1480+
* simplify the handling for the 'synced' column. Therefore, we retain the
1481+
* 'synced' column as true after promotion as it may provide useful
1482+
* information about the slot origin.
14821483
*/
14831484
ShutDownSlotSync();
14841485

‎src/backend/replication/libpqwalreceiver/libpqwalreceiver.c

+5-1
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,11 @@ libpqrcv_connect(const char *conninfo, bool replication, bool logical,
271271
errhint("Target server's authentication method must be changed, or set password_required=false in the subscription parameters.")));
272272
}
273273

274-
if (logical)
274+
/*
275+
* Set always-secure search path for the cases where the connection is
276+
* used to run SQL queries, so malicious users can't get control.
277+
*/
278+
if (!replication || logical)
275279
{
276280
PGresult *res;
277281

‎src/backend/replication/logical/slotsync.c

+15-8
Original file line numberDiff line numberDiff line change
@@ -105,10 +105,10 @@ bool sync_replication_slots = false;
105105
* (within a MIN/MAX range) according to slot activity. See
106106
* wait_for_slot_activity() for details.
107107
*/
108-
#define MIN_WORKER_NAPTIME_MS 200
109-
#define MAX_WORKER_NAPTIME_MS 30000 /* 30s */
108+
#define MIN_SLOTSYNC_WORKER_NAPTIME_MS 200
109+
#define MAX_SLOTSYNC_WORKER_NAPTIME_MS 30000 /* 30s */
110110

111-
static long sleep_ms = MIN_WORKER_NAPTIME_MS;
111+
static long sleep_ms = MIN_SLOTSYNC_WORKER_NAPTIME_MS;
112112

113113
/* The restart interval for slot sync work used by postmaster */
114114
#define SLOTSYNC_RESTART_INTERVAL_SEC 10
@@ -924,12 +924,9 @@ ValidateSlotSyncParams(int elevel)
924924
* in this case regardless of elevel provided by caller.
925925
*/
926926
if (wal_level < WAL_LEVEL_LOGICAL)
927-
{
928927
ereport(ERROR,
929928
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
930929
errmsg("slot synchronization requires wal_level >= \"logical\""));
931-
return false;
932-
}
933930

934931
/*
935932
* A physical replication slot(primary_slot_name) is required on the
@@ -1082,15 +1079,15 @@ wait_for_slot_activity(bool some_slot_updated)
10821079
* No slots were updated, so double the sleep time, but not beyond the
10831080
* maximum allowable value.
10841081
*/
1085-
sleep_ms = Min(sleep_ms * 2, MAX_WORKER_NAPTIME_MS);
1082+
sleep_ms = Min(sleep_ms * 2, MAX_SLOTSYNC_WORKER_NAPTIME_MS);
10861083
}
10871084
else
10881085
{
10891086
/*
10901087
* Some slots were updated since the last sleep, so reset the sleep
10911088
* time.
10921089
*/
1093-
sleep_ms = MIN_WORKER_NAPTIME_MS;
1090+
sleep_ms = MIN_SLOTSYNC_WORKER_NAPTIME_MS;
10941091
}
10951092

10961093
rc = WaitLatch(MyLatch,
@@ -1215,6 +1212,16 @@ ReplSlotSyncWorkerMain(int argc, char *argv[])
12151212
*/
12161213
sigprocmask(SIG_SETMASK, &UnBlockSig, NULL);
12171214

1215+
/*
1216+
* Set always-secure search path, so malicious users can't redirect user
1217+
* code (e.g. operators).
1218+
*
1219+
* It's not strictly necessary since we won't be scanning or writing to
1220+
* any user table locally, but it's good to retain it here for added
1221+
* precaution.
1222+
*/
1223+
SetConfigOption("search_path", "", PGC_SUSET, PGC_S_OVERRIDE);
1224+
12181225
dbname = CheckAndGetDbnameFromConninfo();
12191226

12201227
/*

‎src/test/recovery/t/040_standby_failover_slots_sync.pl

+54
Original file line numberDiff line numberDiff line change
@@ -361,6 +361,60 @@
361361

362362
$cascading_standby->stop;
363363

364+
##################################################
365+
# Test to confirm that the slot synchronization is protected from malicious
366+
# users.
367+
##################################################
368+
369+
$primary->psql('postgres', "CREATE DATABASE slotsync_test_db");
370+
$primary->wait_for_replay_catchup($standby1);
371+
372+
$standby1->stop;
373+
374+
# On the primary server, create '=' operator in another schema mapped to
375+
# inequality function and redirect the queries to use new operator by setting
376+
# search_path. The new '=' operator is created with leftarg as 'bigint' and
377+
# right arg as 'int' to redirect 'count(*) = 1' in slot sync's query to use
378+
# new '=' operator.
379+
$primary->safe_psql(
380+
'slotsync_test_db', q{
381+
382+
CREATE ROLE repl_role REPLICATION LOGIN;
383+
CREATE SCHEMA myschema;
384+
385+
CREATE FUNCTION myschema.myintne(bigint, int) RETURNS bool as $$
386+
BEGIN
387+
RETURN $1 <> $2;
388+
END;
389+
$$ LANGUAGE plpgsql immutable;
390+
391+
CREATE OPERATOR myschema.= (
392+
leftarg = bigint,
393+
rightarg = int,
394+
procedure = myschema.myintne);
395+
396+
ALTER DATABASE slotsync_test_db SET SEARCH_PATH TO myschema,pg_catalog;
397+
GRANT USAGE on SCHEMA myschema TO repl_role;
398+
});
399+
400+
# Start the standby with changed primary_conninfo.
401+
$standby1->append_conf('postgresql.conf', "primary_conninfo = '$connstr_1 dbname=slotsync_test_db user=repl_role'");
402+
$standby1->start;
403+
404+
# Run the synchronization function. If the sync flow was not prepared
405+
# to handle such attacks, it would have failed during the validation
406+
# of the primary_slot_name itself resulting in
407+
# ERROR: slot synchronization requires valid primary_slot_name
408+
$standby1->safe_psql('slotsync_test_db', "SELECT pg_sync_replication_slots();");
409+
410+
# Reset the dbname and user in primary_conninfo to the earlier values.
411+
$standby1->append_conf('postgresql.conf', "primary_conninfo = '$connstr_1 dbname=postgres'");
412+
$standby1->reload;
413+
414+
# Drop the newly created database.
415+
$primary->psql('postgres',
416+
q{DROP DATABASE slotsync_test_db;});
417+
364418
##################################################
365419
# Test to confirm that the slot sync worker exits on invalid GUC(s) and
366420
# get started again on valid GUC(s).

0 commit comments

Comments
 (0)