From: Tom Lane <tgl@sss.pgh.pa.us>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
cc: michael@paquier.xyz, gurjeet@singh.im, pgsql-hackers@postgresql.org
Subject: Re: Unprivileged user can induce crash by using an SUSET param in
 PGOPTIONS
In-reply-to: <20220722.151600.1094104349120879204.horikyota.ntt@gmail.com>
References: 
 <CABwTF4U9v4u3L2GWU6ZVxK-6NtvEMarrgAj2Vq8=wE_g1Tkncw@mail.gmail.com>
 <CABwTF4WXCbjEpVRq+G+MZeJEXU=2evYMb_DVdzXpWap5cczkcQ@mail.gmail.com>
 <Ytn2ssEKdzPFM3Vf@paquier.xyz>
 <20220722.151600.1094104349120879204.horikyota.ntt@gmail.com>
Comments: In-reply-to Kyotaro Horiguchi <horikyota.ntt@gmail.com>
	message dated "Fri, 22 Jul 2022 15:16:00 +0900"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <729233.1658516159.0@sss.pgh.pa.us>
Date: Fri, 22 Jul 2022 14:56:22 -0400
Message-ID: <729330.1658516182@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/729330.1658516182%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <729233.1658516159.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable

Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Fri, 22 Jul 2022 10:00:34 +0900, Michael Paquier <michael@paquier.xyz=
> wrote in =

>> On Thu, Jul 21, 2022 at 05:39:35PM -0700, Gurjeet Singh wrote:
>>> The patch also adds an assertion in pg_parameter_aclcheck() to ensure
>>> that there's a transaction is in progress before it's called.

>> This can involve extension code, I think that this should be at least
>> an elog(ERROR) so as we have higher chances of knowing if something
>> still goes wrong in the wild.

That assert strikes me as having been inserted with the advice of a
dartboard.  Why pg_parameter_aclcheck, and not any other aclchk.c
functions?  Why in the callers at all, rather than somewhere down
inside the syscache code?  And why isn't the existing Assert that
you started the thread with plenty sufficient for that already?

> This patch makes process_session_preload_libraries called in
> autovacuum worker/launcher and background worker in addition to client
> backends.  It seems to me we also need to prevent that.

Yeah.  I think the definition of session/local_preload_libraries
is that it loads libraries into *interactive* sessions.  The
existing coding seems already buggy in that regard, because it
will load such libraries into walsenders as well; isn't that
a POLA violation?

So I propose the attached.  I tested this to the extent of checking
that all our contrib modules can be loaded via session_preload_libraries.
That doesn't prove a whole lot about what outside extensions might do,
but it's some comfort.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name="move-process_session_preload_libraries-2.patch";
	charset="us-ascii"
Content-ID: <729233.1658516159.2@sss.pgh.pa.us>
Content-Description: move-process_session_preload_libraries-2.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 8ba1c170f0..67098aa204 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -4112,12 +4112,6 @@ PostgresMain(const char *dbname, const char *userna=
me)
 	if (am_walsender)
 		InitWalSender();
 =

-	/*
-	 * process any libraries that should be preloaded at backend start (this
-	 * likewise can't be done until GUC settings are complete)
-	 */
-	process_session_preload_libraries();
-
 	/*
 	 * Send this backend's cancellation info to the frontend.
 	 */
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/po=
stinit.c
index a5c208a20a..47605b9d59 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -1108,6 +1108,19 @@ InitPostgres(const char *in_dbname, Oid dboid, cons=
t char *username,
 	/* Initialize this backend's session state. */
 	InitializeSession();
 =

+	/*
+	 * If this is an interactive session, load any libraries that should be
+	 * preloaded at backend start.  Since those are determined by GUCs, this
+	 * can't happen until GUC settings are complete, but we want it to happe=
n
+	 * during the initial transaction in case anything that requires databas=
e
+	 * access needs to be done.
+	 */
+	if (!bootstrap &&
+		!IsAutoVacuumWorkerProcess() &&
+		!IsBackgroundWorker &&
+		!am_walsender)
+		process_session_preload_libraries();
+
 	/* report this backend in the PgBackendStatus array */
 	if (!bootstrap)
 		pgstat_bestart();

------- =_aaaaaaaaaa0--