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

On Thu, Jul 21, 2022 at 2:44 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Gurjeet Singh <gurjeet@singh.im> writes:
> > While poking at plperl's GUC in an internal discussion, I was able to
> > induce a crash (or an assertion failure in assert-enabled builds) as
> > an unprivileged user.
> > My investigation so far has revealed that the code path for the
> > following condition has never been tested, and because of this, when a
> > user tries to override an SUSET param via PGOPTIONS, Postgres tries to
> > perform a table lookup during process initialization. Because there's
> > no transaction in progress, and because this table is not in the
> > primed caches, we end up with code trying to dereference an
> > uninitialized  CurrentResourceOwner.
>
> 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 had debated just wrapping the process_session_preload_libraries()
call with a transaction, like Nathan's patch posted ealier on this
thread does. But I hesitated because of the sensitivity around the
order of operations/call during process initialization.

I like the idea of performing library initialization in
InitPostgres(), as it performs the first transaction of the
connection, and because of the libraries' ability to gin up new GUC
variables that might need special handling, and also if it allows them
to do database access.

I think anywhere after the 'PostAuthDelay' check in InitPostgres()
would be a good place to perform process_session_preload_libraries().
I'm inclined to invoke it as late as possible, before we commit the
transaction.

As for making set_config_option() throw an error if not in
transaction, I'm not a big fan of checks  that break the flow, and of
unrelated code showing up when reading a function. For a casual
reader, such a check for transaction would make for a jarring
experience; "why are we checking for active transaction in  the guts
of guc.c?", they might think. If anything, such an error should be
thrown from or below pg_parameter_aclcheck().

But I am not sure if it should be exposed as an error. A user
encountering that error is not at fault. Hence I believe an assertion
check is more suitable for catching code that invokes
set_config_option() outside a transaction.

Best regards,
Gurjeet
http://Gurje.et