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

On Wed, Feb 3, 2010 at 09:31, Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com> wrote:
> 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.)

All I know is I thought hrm... Why cant you have SPI ? It seems useful
and I dont immediately see why its a bad idea. So I dug up the old
threads. Im just afraid say in a year or two we will forget why we
disallow it.

I was thinking something along the lines of:
diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c
index 6f577f0..a19f1da 100644
--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c
@@ -422,6 +422,12 @@ plperl_init_interp(void)

PERL_SET_CONTEXT(plperl);
perl_construct(plperl);
+
+ /*
+ * Allow things like SPI to happen *after* the plperl.*init functions have run
+ * this avoids nasty problems with security definer functions
+ * ...maybe some mail link here or the whole quote from Tom?
+ */
perl_parse(plperl, plperl_init_shared_libs,
nargs, embedding, NULL);

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

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

Much better.

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

Well not quite as with the above there is still no global "on_init".

> 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

Hrm, I think I agree with Tom that we should not have a global
on_init. And instead of two separate GUCs (we still end up with 3
gucs total). Im still thinking through it...

> Since Tom also expressed a preference for .on_plperl_init/.on_plperlu_init

Well I think Magnus and Robert did as well :)

> and you also suggested .on_init earlier, I'll rework the patch with those
> names, plus the docs and test fixes nted above.

OK

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

Well Im not sure. As a user I can probably cause havok just by
passing interesting values to a function. It does seem inconsistent
that you cant create plperl functions but you can potentially modify
SHARED. In-fact if you have a security definer plperl function it
seems quite scary that they could change values in SHARED. But any
plperl function can do that anyway. (do we have/need documentation
that SHARED in a plperl security definer function is unsafe?) So I
dont think its *that* big of deal as long as the GRANT check is in
place.

Thoughts?