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

At Fri, 22 Jul 2022 10:00:34 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in
> On Thu, Jul 21, 2022 at 05:39:35PM -0700, Gurjeet Singh wrote:
> > One notable side effect of this change is that
> > process_session_preload_libraries() is now called _before_ we
> > SetProcessingMode(NormalProcessing). Which means any database access
> > performed by _PG_init() of an extension will be doing it in
> > InitProcessing mode. I'm not sure if that's problematic.
>
> I cannot see a reason why on top of my mind. The restrictions of
> InitProcessing apply to two code paths of bgworkers connecting to a
> database, and normal processing is used as a barrier to prevent the
> creation of some objects.
>
> > The patch also adds an assertion in pg_parameter_aclcheck() to ensure
> > that there's a transaction is in progress before it's called.
>
> + /* It's pointless to call this function, unless we're in a transaction. */
> + Assert(IsTransactionState());
>
> This can involve extension code, I think that this should be at least
> an elog(ERROR) so as we have higher chances of knowing if something
> still goes wrong in the wild.

pg_parameter_aclmask involves the same assertion, so the same
backtrace can be obtained without it. I think it is no bad of users
so I'm not sure ERROR is appropriate even if we were to add something
there.

> > The patch now lets the user connect, throws a warning, and does not crash.
> >
> > $ PGOPTIONS="-c plperl.on_plperl_init=" psql -U test
> > WARNING: permission denied to set parameter "plperl.on_plperl_init"
> > Expanded display is used automatically.
> > psql (15beta1)
> > Type "help" for help.
>
> I am wondering whether we'd better have a test on this one with a
> non-superuser. Except for a few tests in the unsafe section,
> session_preload_libraries has a limited amount of coverage.

+1

> + /*
> + * process any libraries that should be preloaded at backend start (this
> + * can't be done until GUC settings are complete). Note that these libraries
> + * can declare new GUC variables.
> + */
> + process_session_preload_libraries();
> There is no point in doing that during bootstrap anyway, no?

This patch makes process_session_preload_libraries called in
autovacuum worker/launcher and background worker in addition to client
backends. It seems to me we also need to prevent that.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center