Re: Fwd: Unprivileged user can induce crash by using an SUSET param in PGOPTIONS - Mailing list pgsql-hackers

On Thu, Jul 21, 2022 at 3:29 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> 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.

(none of the following is your patch's fault)

I don't think that is a good call-site for
process_session_preload_libraries(), because a library being loaded
can declare its own GUCs, hence I believe this should be called at
least before the call to BeginReportingGUCOptions().

If an extension creates a GUC with GUC_REPORT flag, it is violating
expectations. But since the DefineCustomXVariable() stack does not
prevent the callers from doing so, we must still honor the protocol
followed for all params with GUC_REPORT. And hence the

I think it'd be a good idea to ban the callers of
DefineCustomXVariable() from declaring their variable GUC_REPORT, to
ensure that only core code can define such variables.

Best regards,
Gurjeet
http://Gurje.et