From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Gurjeet Singh <gurjeet@singh.im>
Cc: Postgres Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Fwd: Unprivileged user can induce crash by using an SUSET param in PGOPTIONS
Date: Thu, 21 Jul 2022 15:29:00 -0700
Message-ID: <20220721222900.GA3813377@nathanxps13> (raw)
In-Reply-To: <335170.1658439851@sss.pgh.pa.us>
References: <CABwTF4XZvL4Mneo49MQZDBcRF+=8e2GOENViU3E-S9NGa8b90A@mail.gmail.com>
<20220705174938.GB2566600@rfd.leadboat.com>
<CABwTF4VEpwTHhRQ+q5MiC5ucngN-whN-PdcKeufX7eLSoAfbZA@mail.gmail.com>
<335170.1658439851@sss.pgh.pa.us>
On Thu, Jul 21, 2022 at 05:44:11PM -0400, Tom Lane wrote:
> Right. So there are basically two things we could do about this:
>
> 1. set_config_option could decline to call pg_parameter_aclcheck
> if not IsTransactionState(), instead failing the assignment.
> This isn't a great answer because it would result in disallowing
> GUC assignments that users might expect to work.
>
> 2. We could move process_session_preload_libraries() to someplace
> where a transaction is in progress -- probably, relocate it to
> inside InitPostgres().
>
> I'm inclined to think that #2 is a better long-term solution,
> because it'd allow you to session-preload libraries that expect
> to be able to do database access during _PG_init. (Right now
> that'd fail with the same sort of symptoms seen here.) But
> there's no denying that this might have surprising side-effects
> for extensions that weren't expecting such a change.
>
> It could also be reasonable to do both #1 and #2, with the idea
> that #1 might save us from crashing if there are any other
> code paths where we can reach that pg_parameter_aclcheck call
> outside a transaction.
>
> Thoughts?
I wrote up a small patch along the same lines as #2 before seeing this
message. It simply ensures that process_session_preload_libraries() is
called within a transaction. I don't have a strong opinion about doing it
this way versus moving this call somewhere else as you proposed, but I'd
agree that #2 is a better long-term solution than #1. AFAICT
shared_preload_libraries, even with EXEC_BACKEND, should not have the same
problem.
I'm not sure whether we should be worried about libraries that are already
creating transactions in their _PG_init() functions. Off the top of my
head, I don't recall seeing anything like that. Even if it does impact
some extensions, it doesn't seem like it'd be too much trouble to fix.
inline patch 1: src/backend/tcop/postgres.c (+7/-0 lines) download inline-1.patchdiff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.cindex 8ba1c170f0..fd471d74a3 100644--- a/src/backend/tcop/postgres.c+++ b/src/backend/tcop/postgres.c@@ -4115,8 +4115,15 @@ PostgresMain(const char *dbname, const char *username)
/*
* process any libraries that should be preloaded at backend start (this
* likewise can't be done until GUC settings are complete)
+ *+ * If the user provided a setting at session startup for a custom GUC+ * defined by one of these libraries, we might need syscache access when+ * evaluating whether she has permission to set it, so do this step within+ * a transaction.
*/
+ StartTransactionCommand();
process_session_preload_libraries();
+ CommitTransactionCommand();
/*
* Send this backend's cancellation info to the frontend.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, tgl@sss.pgh.pa.us, gurjeet@singh.im
Subject: Re: Fwd: Unprivileged user can induce crash by using an SUSET param in PGOPTIONS
In-Reply-To: <20220721222900.GA3813377@nathanxps13>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox