Date: Thu, 21 Jul 2022 15:29:00 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Gurjeet Singh <gurjeet@singh.im>,
	Postgres Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Fwd: Unprivileged user can induce crash by using an SUSET param
 in PGOPTIONS
Message-ID: <20220721222900.GA3813377@nathanxps13>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <335170.1658439851@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/20220721222900.GA3813377%40nathanxps13>
Precedence: bulk

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.

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 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