Hi,
On Fri, Feb 23, 2024 at 09:30:58AM +0000, Zhijie Hou (Fujitsu) wrote:
> On Friday, February 23, 2024 5:07 PM Bertrand Drouvot
> <bertranddrouvot...@gmail.com> wrote:
> > On Fri, Feb 23, 2024 at 02:15:11PM +0530, shveta malik wrote:
> > >
> > > Thanks for the details. I understand it now. We do not use '=' in our
> > > main slots-fetch query but we do use '=' in remote-validation query.
> > > See validate_remote_info().
> >
> > Oh, right, I missed it during the review.
> >
> > > Do you think instead of doing the above, we can override search-path
> > > with empty string in the slot-sync case.
> > > SImilar to logical apply worker and autovacuum worker case (see
> > > InitializeLogRepWorker(), AutoVacWorkerMain()).
> >
> > Yeah, we should definitively ensure that any operators being used in the
> > query
> > is coming from the pg_catalog schema (could be by setting the search path or
> > using the up-thread proposal).
> >
> > Setting the search path would prevent any risks in case the query is changed
> > later on, so I'd vote for changing the search path in
> > validate_remote_info() and
> > in synchronize_slots() to be on the safe side.
>
> I think to set secure search path for remote connection, the standard approach
> could be to extend the code in libpqrcv_connect[1], so that we don't need to
> schema
> qualify all the operators in the queries.
>
> And for local connection, I agree it's also needed to add a
> SetConfigOption("search_path", "" call in the slotsync worker.
>
> [1]
> libpqrcv_connect
> ...
> if (logical)
> ...
> res = libpqrcv_PQexec(conn->streamConn,
>
> ALWAYS_SECURE_SEARCH_PATH_SQL);
>
Agree, something like in the attached? (it's .txt to not disturb the CF bot).
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
>From abd3e8a7131621251b5d4628f4cb0979911159ac Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot...@gmail.com>
Date: Fri, 23 Feb 2024 13:58:31 +0000
Subject: [PATCH v1] reset search_path for slot synchronization.
Ensure that search_path is reset for slot synchronization, within the BGW and
"local" connections.
---
src/backend/replication/libpqwalreceiver/libpqwalreceiver.c | 2 +-
src/backend/replication/logical/slotsync.c | 6 ++++++
2 files changed, 7 insertions(+), 1 deletion(-)
20.0% src/backend/replication/libpqwalreceiver/
79.9% src/backend/replication/logical/
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 04271ee703..a30528a5f6 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -271,7 +271,7 @@ libpqrcv_connect(const char *conninfo, bool replication,
bool logical,
errhint("Target server's authentication method
must be changed, or set password_required=false in the subscription
parameters.")));
}
- if (logical)
+ if (logical || !replication)
{
PGresult *res;
diff --git a/src/backend/replication/logical/slotsync.c
b/src/backend/replication/logical/slotsync.c
index 36773cfe73..0ee08c3976 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -1215,6 +1215,12 @@ ReplSlotSyncWorkerMain(int argc, char *argv[])
*/
sigprocmask(SIG_SETMASK, &UnBlockSig, NULL);
+ /*
+ * Set always-secure search path, so malicious users can't redirect user
+ * code (e.g. operators).
+ */
+ SetConfigOption("search_path", "", PGC_SUSET, PGC_S_OVERRIDE);
+
dbname = CheckAndGetDbnameFromConninfo();
/*
--
2.34.1
