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

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.

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

+ /*
+ * 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?
--
Michael