Skip site navigation (1) Skip section navigation (2)

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

From: Alex Hunsaker <badalex(at)gmail(dot)com>
To: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>
Subject: Re: Add on_trusted_init and on_untrusted_init to plperl UPDATED [PATCH]
Date: 2010-02-03 17:18:51
Message-ID: 34d269d41002030918v746e5490nef66d21ffb996ae4@mail.gmail.com (view raw or flat)
Thread:
Lists: pgsql-hackers
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?

In response to

Responses

pgsql-hackers by date

Next:From: Alex HunsakerDate: 2010-02-03 17:21:49
Subject: Re: Add on_trusted_init and on_untrusted_init to plperl UPDATED [PATCH]
Previous:From: Simon RiggsDate: 2010-02-03 17:10:11
Subject: Re: Hot Standby and VACUUM FULL

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group