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