Re: Add on_trusted_init and on_untrusted_init to plperl UPDATED [PATCH]

On Tue, Feb 02, 2010 at 08:42:21PM -0700, Alex Hunsaker wrote:
> On Sat, Jan 30, 2010 at 08:49, Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com> wrote:
>
> >    SPI functions are not available when the code is run.
>
> Hrm, we might want to stick why in the docs or as a comment somewhere.
> I think this was the main concern?
>
> * We call a plperl function for the first time in a session, causing
> plperl.so to be loaded. This happens in the context of a superuser
> calling a non-superuser security definer function, or perhaps vice
> versa. Whose permissions apply to whatever the on_load code tries
> to do? (Hint: every answer is wrong.)

It's hard to convey the underlying issues in a sentence or two. I tried.
I'm not sure it's worth it. (Though if anyone thinks it is I'd be happy
to get some specific suggestions for the wording to use.)

> > - The utf8fix code has been greatly simplified.
>
> Yeah to the point that it makes me wonder if the old code had some
> reason to spin up the FunctionCall stuff. Do you happen to know?

Before my refactoring led me to add safe_eval(), FunctionCall was
'the natural way' to invoke code inside the Safe compartment.

> The tests dont seem to pass :( this is from a make installcheck-world

> + ERROR: unrecognized configuration parameter "plperl.on_trusted_init"

> If I throw a LOAD 'plperl'; at the top of those sql files it works...

Ah. That's be because I've got custom_variable_classes = 'plperl' in my
postgresql.conf. I'll add LOAD 'plperl'; to the top of those tests.

> The only quibble I have with the docs is:
> + If the code fails with an error it will abort the initialization and
> + propagate out to the calling query, causing the current transaction or
> + subtransaction to be aborted. Any changes within the perl won't be
> + undone. If the <literal>plperl</> language is used again the
> + initialization will be repeated.
>
> Instead of "Any changes within the perl won't be undone". Maybe
> "Changes to the perl interpreter will not be undone" ?

In an earlier patch I'd used the word interpreter quite often. When
polishing up the doc changes in response to Tom's feedback I opted to
avoid that word when describing on_*trusted_init. This looks like a case
where I removed 'interpreter' but didn't fixup the rest of the sentence.

I'd prefer to simplify the sentence further, so I've changed it to
"Any changes within perl won't be undone".

On Wed, Feb 03, 2010 at 01:06:03AM -0700, Alex Hunsaker wrote:
>
> plperl.on_trusted_init runs *inside* of the safe. So you cant do
> unsafe things like use this or that module.

Yes. It's effectively the same as having a DO '...' language plperl;
that's guaranteed to run before any other use of plperl.

> plperl.on_init runs on
> init *outside* of the safe so you can use modules and what not. So now
> I can use say Digest::SHA without tossing the baby out with the bath
> water (just using plperlu). Gaping security whole? Maybe, no more so
> than installing an insecure C/plperlu function as you have to edit
> postgresql.conf to change it. Right?

Right.

I'll emphasise the point that only plperlu code has access to anything
loaded by plperl.on_init (aka .on_perl_init). Currently plperl code doesn't.

There seemed to be some confusion upthread about how the GUCs work
together so I'll recap here (using the original GUC names):

When plperl.so is loaded (possibly in the postmaster due to
shared_preload_libraries) a perl interpreter is created using
whatever version of perl the server was configured with.
That perl is initialized as if it had been started using:
perl -e $(cat plc_perlboot.pl)
If on_perl_init is set then the the initialization is effectively:
perl -e $(cat plc_perlboot.pl) -e $on_perl_init

That perl interpreter is now in a virgin 'unspecialized' state.
>From that state the interpreter may become either a plperl or plperlu
interpreter depending on whichever language is first used.

When an interpreter transitions from the initial state to become
specialized for plperlu for a particular user then
plperl.on_untrusted_init is eval'd.

When an interpreter transitions from the initial state to become
specialized for plperl then plc_safe_ok.pl is executed to create the
Safe compartment and then plperl.on_trusted_init is eval'd *inside* the
compartment.

So, if all GUCs were set then plperl code would run in a perl
initialized with on_perl_init + on_trusted_init, and plperlu code would
run in a perl initialized with on_perl_init + on_untrusted_init.

To add some context, I envisage plperl.on_perl_init being a stable value
that typically pre-loads a set of modules etc., while the on_*trusted_init
GUCs will typically be used for short-term debug/testing. Which is why
on_untrusted_init (SUSET) is still useful with on_perl_init (SUSET).

> Maybe we should have:
> plperl.on_plperl_safe_init (runs inside of the safe, PGC_USERSET)
> plperl.on_plperl_init (runs outside safe, PGC_SUSET)
> plperl.on_plpleru_init (PGC_SUSET)

Which, except for the names, is essentially what the patches implement.

If we're going to bikeshed the names, I'd suggest:

plperl.on_init - run on interpreter creation
plperl.on_plperl_init - run when then specialized for plperl
plperl.on_plperlu_init - run when then specialized for plperlu

as being the most natural fit with the user and DBA perspective.

Since Tom also expressed a preference for .on_plperl_init/.on_plperlu_init
and you also suggested .on_init earlier, I'll rework the patch with those
names, plus the docs and test fixes nted above.

> There does seem to be the risk that I may not have plperl GRANTed but
> I can make any plperl function elog(ERROR) as long as they have not
> loaded plperl via a plperl_safe_init. We can probably fix that if
> people think its a valid dos/attack vector.

Interesting thought. If this is a valid concern (as it seems to be)
then I could add some logic to check if the language has been GRANTed.
(I.e. ignore on_plperl_init if the user can't write plperl code, and
ignore on_plperlu_init if the user can't write plperlu code.)

Tim.