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@paquier.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