Re: Fwd: Unprivileged user can induce crash by using an SUSET param in PGOPTIONS

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