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

On Thu, Jul 21, 2022 at 4:35 PM Gurjeet Singh <gurjeet(at)singh(dot)im> wrote:
> 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.

Please see attached the patch that implements the above proposal.

The process_session_preload_libraries() call has been moved to the end
of InitPostgres(), just before we report the backend to
PgBackendStatus and commit the first transaction.

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.

The patch also adds an assertion in pg_parameter_aclcheck() to ensure
that there's a transaction is in progress before it's called.

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.

postgres(at)B:694512=>

Best regards,
Gurjeet
http://Gurje.et