On Wed, Feb 3, 2010 at 09:31, Tim Bunce <tim.bu...@pobox.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.bu...@pobox.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?
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
