Thread

Add on_trusted_init and on_untrusted_init to plperl UPDATED [PATCH]

Tim Bunce <tim.bunce@pobox.com> — 2010-01-30T15:49:43Z

This is an update the fourth of the patches to be split out from the
former 'plperl feature patch 1'.

Changes in this patch:

- Adds plperl.on_trusted_init and plperl.on_untrusted_init GUCs
    on_trusted_init is PGC_USERSET, on_untrusted_init is PGC_SUSET
    SPI functions are not available when the code is run.
    Errors are detected and reported as ereport(ERROR, ...)
    Corresponding documentation.

- select_perl_context() state management improved
    An error during interpreter initialization will leave
    the state (interp_state etc) unchanged.

- The utf8fix code has been greatly simplified.

Tim.

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

Alex Hunsaker <badalex@gmail.com> — 2010-02-03T03:42:21Z

On Sat, Jan 30, 2010 at 08:49, Tim Bunce <Tim.Bunce@pobox.com> wrote:
> This is an update the fourth of the patches to be split out from the
> former 'plperl feature patch 1'.
>
> Changes in this patch:
>
> - Adds plperl.on_trusted_init and plperl.on_untrusted_init GUCs
>    on_trusted_init is PGC_USERSET, on_untrusted_init is PGC_SUSET

Im not a fan of the names (I think everyone gets trusted vs untrusted
confused). May I humbly suggest:
plperl.on_init
plperlu.on_init
plperl.both_on_init <- this one is the one that throws the scheme off :(

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

> - select_perl_context() state management improved
>    An error during interpreter initialization will leave
>    the state (interp_state etc) unchanged.

This looked good.

> - 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?
Looks good to me otherwise.

The tests dont seem to pass :( this is from a make installcheck-world
test plperl_shared        ... FAILED
...
test plperl_init          ... FAILED

with:
  SET plperl.on_trusted_init = '$_SHARED{on_init} = 42';
+ ERROR:  unrecognized configuration parameter "plperl.on_trusted_init"
  -- test the shared hash

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

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" ?

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

Robert Haas <robertmhaas@gmail.com> — 2010-02-03T03:46:13Z

On Tue, Feb 2, 2010 at 10:42 PM, Alex Hunsaker <badalex@gmail.com> wrote:
> On Sat, Jan 30, 2010 at 08:49, Tim Bunce <Tim.Bunce@pobox.com> wrote:
>> This is an update the fourth of the patches to be split out from the
>> former 'plperl feature patch 1'.
>>
>> Changes in this patch:
>>
>> - Adds plperl.on_trusted_init and plperl.on_untrusted_init GUCs
>>    on_trusted_init is PGC_USERSET, on_untrusted_init is PGC_SUSET
>
> Im not a fan of the names (I think everyone gets trusted vs untrusted
> confused). May I humbly suggest:
> plperl.on_init
> plperlu.on_init
> plperl.both_on_init <- this one is the one that throws the scheme off :(
>
>>    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.)
>
>> - select_perl_context() state management improved
>>    An error during interpreter initialization will leave
>>    the state (interp_state etc) unchanged.
>
> This looked good.
>
>> - 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?
> Looks good to me otherwise.
>
> The tests dont seem to pass :( this is from a make installcheck-world
> test plperl_shared        ... FAILED
> ...
> test plperl_init          ... FAILED
>
> with:
>  SET plperl.on_trusted_init = '$_SHARED{on_init} = 42';
> + ERROR:  unrecognized configuration parameter "plperl.on_trusted_init"
>  -- test the shared hash
>
> If I throw a LOAD 'plperl'; at the top of those sql files it works...
>
> 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" ?

With all due respect.... yuck.

...Robert

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

Robert Haas <robertmhaas@gmail.com> — 2010-02-03T03:46:54Z

On Tue, Feb 2, 2010 at 10:46 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Feb 2, 2010 at 10:42 PM, Alex Hunsaker <badalex@gmail.com> wrote:
>> On Sat, Jan 30, 2010 at 08:49, Tim Bunce <Tim.Bunce@pobox.com> wrote:
>>> This is an update the fourth of the patches to be split out from the
>>> former 'plperl feature patch 1'.
>>>
>>> Changes in this patch:
>>>
>>> - Adds plperl.on_trusted_init and plperl.on_untrusted_init GUCs
>>>    on_trusted_init is PGC_USERSET, on_untrusted_init is PGC_SUSET
>>
>> Im not a fan of the names (I think everyone gets trusted vs untrusted
>> confused). May I humbly suggest:
>> plperl.on_init
>> plperlu.on_init
>> plperl.both_on_init <- this one is the one that throws the scheme off :(
>>
>>>    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.)
>>
>>> - select_perl_context() state management improved
>>>    An error during interpreter initialization will leave
>>>    the state (interp_state etc) unchanged.
>>
>> This looked good.
>>
>>> - 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?
>> Looks good to me otherwise.
>>
>> The tests dont seem to pass :( this is from a make installcheck-world
>> test plperl_shared        ... FAILED
>> ...
>> test plperl_init          ... FAILED
>>
>> with:
>>  SET plperl.on_trusted_init = '$_SHARED{on_init} = 42';
>> + ERROR:  unrecognized configuration parameter "plperl.on_trusted_init"
>>  -- test the shared hash
>>
>> If I throw a LOAD 'plperl'; at the top of those sql files it works...
>>
>> 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" ?
>
> With all due respect.... yuck.

OK, third time is the charm.  Sigh.  The "yuck" was in reference
specifically to the proposed GUC names.

I like the original ones better.

...Robert

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

Alvaro Herrera <alvherre@commandprompt.com> — 2010-02-03T03:51:15Z

Robert Haas escribió:
> On Tue, Feb 2, 2010 at 10:46 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> > On Tue, Feb 2, 2010 at 10:42 PM, Alex Hunsaker <badalex@gmail.com> wrote:
> >> On Sat, Jan 30, 2010 at 08:49, Tim Bunce <Tim.Bunce@pobox.com> wrote:
> >>> This is an update the fourth of the patches to be split out from the
> >>> former 'plperl feature patch 1'.
> >>>
> >>> Changes in this patch:
> >>>
> >>> - Adds plperl.on_trusted_init and plperl.on_untrusted_init GUCs
> >>>    on_trusted_init is PGC_USERSET, on_untrusted_init is PGC_SUSET
> >>
> >> Im not a fan of the names (I think everyone gets trusted vs untrusted
> >> confused). May I humbly suggest:
> >> plperl.on_init
> >> plperlu.on_init
> >> plperl.both_on_init <- this one is the one that throws the scheme off :(

> > With all due respect.... yuck.
> 
> OK, third time is the charm.  Sigh.  The "yuck" was in reference
> specifically to the proposed GUC names.
> 
> I like the original ones better.

With all due respect, I think you should get more used to trimming the
message you're replying to, so that your reply makes more sense to the
readership at large.  I fully realize that Gmail is not the best
platform for that :-(

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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

Robert Haas <robertmhaas@gmail.com> — 2010-02-03T03:54:32Z

On Tue, Feb 2, 2010 at 10:51 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
> Robert Haas escribió:
>> On Tue, Feb 2, 2010 at 10:46 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> > On Tue, Feb 2, 2010 at 10:42 PM, Alex Hunsaker <badalex@gmail.com> wrote:
>> >> On Sat, Jan 30, 2010 at 08:49, Tim Bunce <Tim.Bunce@pobox.com> wrote:
>> >>> This is an update the fourth of the patches to be split out from the
>> >>> former 'plperl feature patch 1'.
>> >>>
>> >>> Changes in this patch:
>> >>>
>> >>> - Adds plperl.on_trusted_init and plperl.on_untrusted_init GUCs
>> >>>    on_trusted_init is PGC_USERSET, on_untrusted_init is PGC_SUSET
>> >>
>> >> Im not a fan of the names (I think everyone gets trusted vs untrusted
>> >> confused). May I humbly suggest:
>> >> plperl.on_init
>> >> plperlu.on_init
>> >> plperl.both_on_init <- this one is the one that throws the scheme off :(
>
>> > With all due respect.... yuck.
>>
>> OK, third time is the charm.  Sigh.  The "yuck" was in reference
>> specifically to the proposed GUC names.
>>
>> I like the original ones better.
>
> With all due respect, I think you should get more used to trimming the
> message you're replying to, so that your reply makes more sense to the
> readership at large.  I fully realize that Gmail is not the best
> platform for that :-(

Well, actually, I did that.  Except I replied only to Alex.  And then
I hit send.

Then I immediately realized my mistake, and did it over, trimming it
wrong the second time.  And then I hit send again.

Actually, I find Gmail to be pretty good for this - it's just, I
shouldn't try to answer my email when I'm exhausted and half-asleep.

So... I'm going to bed now.

...Robert

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

Alex Hunsaker <badalex@gmail.com> — 2010-02-03T04:30:31Z

On Tue, Feb 2, 2010 at 20:46, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Feb 2, 2010 at 10:46 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Tue, Feb 2, 2010 at 10:42 PM, Alex Hunsaker <badalex@gmail.com> wrote:
>>> On Sat, Jan 30, 2010 at 08:49, Tim Bunce <Tim.Bunce@pobox.com> wrote:
>>>> This is an update the fourth of the patches to be split out from the
>>>> former 'plperl feature patch 1'.
>>>>
>>>> Changes in this patch:
>>>>
>>>> - Adds plperl.on_trusted_init and plperl.on_untrusted_init GUCs
>>>>    on_trusted_init is PGC_USERSET, on_untrusted_init is PGC_SUSET
>>>
>>> Im not a fan of the names (I think everyone gets trusted vs untrusted
>>> confused). May I humbly suggest:
>>> plperl.on_init
>>> plperlu.on_init
>>> plperl.both_on_init <- this one is the one that throws the scheme off :(

>> With all due respect.... yuck.

heh, well I feel as reviewer its my job to solicit feed back from the
community.  If I have to do it by suggesting gross names, so be it.

> OK, third time is the charm.  Sigh.  The "yuck" was in reference
> specifically to the proposed GUC names.

Yeah the both is gross.  How about:
plperl.on_plperl_init
plperl.on_plperlu_init
plperl.on_init ?

> I like the original ones better.

I think they are OK.

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

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-03T04:38:33Z

Alex Hunsaker <badalex@gmail.com> writes:
> Yeah the both is gross.  How about:
> plperl.on_plperl_init
> plperl.on_plperlu_init
> plperl.on_init ?

I like the first two.  The problem of selecting a good name for the
third one is easily solved: don't have it.  What would it be except
a headache and a likely security problem?

			regards, tom lane

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

Alex Hunsaker <badalex@gmail.com> — 2010-02-03T05:13:06Z

On Tue, Feb 2, 2010 at 21:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alex Hunsaker <badalex@gmail.com> writes:
>> Yeah the both is gross.  How about:
>> plperl.on_plperl_init
>> plperl.on_plperlu_init
>> plperl.on_init ?
>
> I like the first two.  The problem of selecting a good name for the
> third one is easily solved: don't have it.  What would it be except
> a headache and a likely security problem?

Well its already in.  (FYI its also PGC_SIGHUP)  I included it here
because I wanted to make sure it made sense in context of the other
new plperl GUCs.

I *think* its there so people can:
-"use" the same modules in both
-profile both plperl and plperlu code easily (which is really the same point...)

The main difference between on_init and the other two is the later get
eval()ed in while the former is treated as "perl -e". (Did I get that
right Tim? I did not really look over the first patch).  Im not sure
if there are different consequences for that style...  But I would
venture its done that way so we can profile any perl interpreter
startup stuff we do in plperl.c or the src/pl/plperl/*.pl files.

So there might be a reason for it...

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

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-03T05:50:35Z

Alex Hunsaker <badalex@gmail.com> writes:
> On Tue, Feb 2, 2010 at 21:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Alex Hunsaker <badalex@gmail.com> writes:
>>> Yeah the both is gross. How about:
>>> plperl.on_plperl_init
>>> plperl.on_plperlu_init
>>> plperl.on_init ?
>> 
>> I like the first two. The problem of selecting a good name for the
>> third one is easily solved: don't have it. What would it be except
>> a headache and a likely security problem?

> Well its already in.

Well *that's* easily fixed.  I think it's a bad idea, because it's
unclear what you should put there and what the security implications
are.  Two entirely separate init strings seems much easier to understand
and administer.

			regards, tom lane

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

Magnus Hagander <magnus@hagander.net> — 2010-02-03T07:11:16Z

On Wednesday, February 3, 2010, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alex Hunsaker <badalex@gmail.com> writes:
>> On Tue, Feb 2, 2010 at 21:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Alex Hunsaker <badalex@gmail.com> writes:
>>>> Yeah the both is gross.  How about:
>>>> plperl.on_plperl_init
>>>> plperl.on_plperlu_init
>>>> plperl.on_init ?
>>>
>>> I like the first two.  The problem of selecting a good name for the
>>> third one is easily solved: don't have it.  What would it be except
>>> a headache and a likely security problem?
>
>> Well its already in.
>
> Well *that's* easily fixed.  I think it's a bad idea, because it's
> unclear what you should put there and what the security implications
> are.  Two entirely separate init strings seems much easier to understand
> and administer.
>

+1.

It's a simple copy/paste in the config file if you want them the same
anyway, right?

/Magnus


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

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

Alex Hunsaker <badalex@gmail.com> — 2010-02-03T07:46:38Z

On Tue, Feb 2, 2010 at 22:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alex Hunsaker <badalex@gmail.com> writes:
>> On Tue, Feb 2, 2010 at 21:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Alex Hunsaker <badalex@gmail.com> writes:
>>>> Yeah the both is gross.  How about:
>>>> plperl.on_plperl_init
>>>> plperl.on_plperlu_init
>>>> plperl.on_init ?

>> Well its already in.
>
> Well *that's* easily fixed.  I think it's a bad idea, because it's
> unclear what you should put there and what the security implications
> are.

 I can't speak for its virtue, maybe Tim, Andrew?

> Two entirely separate init strings seems much easier to understand
> and administer.

I think people might quibble with you on that...

But I do agree that it seems redundant.

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

Alex Hunsaker <badalex@gmail.com> — 2010-02-03T08:06:03Z

On Wed, Feb 3, 2010 at 00:46, Alex Hunsaker <badalex@gmail.com> wrote:
> On Tue, Feb 2, 2010 at 22:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Alex Hunsaker <badalex@gmail.com> writes:
>>> On Tue, Feb 2, 2010 at 21:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> Alex Hunsaker <badalex@gmail.com> writes:
>>>>> Yeah the both is gross.  How about:
>>>>> plperl.on_plperl_init
>>>>> plperl.on_plperlu_init
>>>>> plperl.on_init ?
>
>>> Well its already in.
>>
>> Well *that's* easily fixed.  I think it's a bad idea, because it's
>> unclear what you should put there and what the security implications
>> are.
>
>  I can't speak for its virtue, maybe Tim, Andrew?

Ahh I think i figured it out.

plperl.on_trusted_init runs *inside* of the safe.  So you cant do
unsafe things like use this or that module.  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?

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)

All of the above have no SPI/database access.

I think we can gt away with PGC_USERSET on safe_init as it wont allow
you to do anything "scary" like play with security definer functions
or redefine functions etc...  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.

Comments?

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

Andrew Dunstan <andrew@dunslane.net> — 2010-02-03T13:41:36Z

Alex Hunsaker wrote:
>>>> Well its already in.
>>>>         
>>> Well *that's* easily fixed.  I think it's a bad idea, because it's
>>> unclear what you should put there and what the security implications
>>> are.
>>>       
>>  I can't speak for its virtue, maybe Tim, Andrew?
>>     
>
>
>   

plperl.on_perl_init runs when the library is loaded. That makes it 
useful for preloading perl modules and similar tasks. The other two in 
the patch under disccussion run when the relevant interpreter is first 
used in the current session. That makes them appropriate for doing 
things like loading specific initial settings (e.g. in the interpreter's 
%_SHARED).

I'm not going to be pleased if, having had a substantial debate on the 
patch that contained on_perl_init a week or so ago there are now 
attempts to rip it out. As I commented when I committed it:

> The final thing that persuaded me that no great damage would be done 
> by on_perl_init was the realization that we already have the ability 
> to do more or less the same thing anyway via standard Perl mechanisms, 
> and I'd be very surprised if enterprising Perl users hadn't made use 
> of it. 

Regarding the naming of the params, I'm not keen to have more than one 
custom_variable_class for plperl. Within that, maybe we can bikeshed the 
names a bit. I don't have terribly strong feelings.

cheers

andrew

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

Alex Hunsaker <badalex@gmail.com> — 2010-02-03T14:51:35Z

On Wed, Feb 3, 2010 at 06:41, Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> Alex Hunsaker wrote:
>>>>>
>>>>> Well its already in.
>>>>>
>>>>
>>>> Well *that's* easily fixed.  I think it's a bad idea, because it's
>>>> unclear what you should put there and what the security implications
>>>> are.
>>>
>>>  I can't speak for its virtue, maybe Tim, Andrew?

> Regarding the naming of the params, I'm not keen to have more than one
> custom_variable_class for plperl. Within that, maybe we can bikeshed the
> names a bit. I don't have terribly strong feelings.

Hey! I don't think were quite to that nasty B word yet :)  I would
argue that treating plperl and plperlu as the same language just
because it shares the same code is a mistake.  But I hate the idea of
two custom_variable_classes for plperl(u) as well.  Which is why I
quickly switched to plperl.on_plperl(u)_init.  Any thoughts on those?
Again maybe people think the original names are fine... *shrug*.

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

Robert Haas <robertmhaas@gmail.com> — 2010-02-03T14:55:11Z

On Wed, Feb 3, 2010 at 9:51 AM, Alex Hunsaker <badalex@gmail.com> wrote:
> Hey! I don't think were quite to that nasty B word yet :)  I would
> argue that treating plperl and plperlu as the same language just
> because it shares the same code is a mistake.  But I hate the idea of
> two custom_variable_classes for plperl(u) as well.  Which is why I
> quickly switched to plperl.on_plperl(u)_init.  Any thoughts on those?
> Again maybe people think the original names are fine... *shrug*.

I like plperl.on_plperl(u)_init better than plperl.on_(un)trusted_init.

...Robert

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

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-03T15:34:04Z

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Feb 3, 2010 at 9:51 AM, Alex Hunsaker <badalex@gmail.com> wrote:
>> Hey! I don't think were quite to that nasty B word yet :) I would
>> argue that treating plperl and plperlu as the same language just
>> because it shares the same code is a mistake. But I hate the idea of
>> two custom_variable_classes for plperl(u) as well. Which is why I
>> quickly switched to plperl.on_plperl(u)_init. Any thoughts on those?
>> Again maybe people think the original names are fine... *shrug*.

> I like plperl.on_plperl(u)_init better than plperl.on_(un)trusted_init.

I agree.  But the question in my mind is the relationship between plperl
and plperlu.  I agree with the upthread comment that it would be better
if the init strings for them were entirely separate.  ISTM we have got
three categories here:

	plperl init done outside Safe (must be SUSET)
	plperl init done inside Safe (can be USERSET)
	plperlu init (must be SUSET)

and there is no good reason to conflate the first and third, nor to
insist that one must be a subset of the other, which AFAICS is the
effect of the current design.  So we need a naming scheme that takes
some account of this.  Perhaps

	plperl.plperl_init
	plperl.plperl_safe_init
	plperl.plperlu_init

?

			regards, tom lane

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

Tim Bunce <tim.bunce@pobox.com> — 2010-02-03T16:31:30Z

On Tue, Feb 02, 2010 at 08:42:21PM -0700, Alex Hunsaker wrote:
> On Sat, Jan 30, 2010 at 08:49, Tim Bunce <Tim.Bunce@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.)

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

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

Alex Hunsaker <badalex@gmail.com> — 2010-02-03T17:18:51Z

On Wed, Feb 3, 2010 at 09:31, Tim Bunce <Tim.Bunce@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.Bunce@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?

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

Alex Hunsaker <badalex@gmail.com> — 2010-02-03T17:21:49Z

On Wed, Feb 3, 2010 at 10:18, Alex Hunsaker <badalex@gmail.com> wrote:
> On Wed, Feb 3, 2010 at 09:31, Tim Bunce <Tim.Bunce@pobox.com> wrote:
>> On Tue, Feb 02, 2010 at 08:42:21PM -0700, Alex Hunsaker wrote:

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

Err
"And instead have two separate GUCs (3 in total)" not
"And Instead of two...".

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

David E. Wheeler <david@kineticode.com> — 2010-02-03T17:27:27Z

On Feb 3, 2010, at 9:21 AM, Alex Hunsaker wrote:

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

I completely agree on using "plperl" and "plperlu" instead of "trusted" and "untrusted" in the GUC names. The latter are just too confusing (even Tom mixed them up in a post last week). They are among the worst names in the system, IMHO.

Best,

David

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

Tim Bunce <tim.bunce@pobox.com> — 2010-02-03T17:56:53Z

On Wed, Feb 03, 2010 at 10:18:51AM -0700, Alex Hunsaker wrote:
> On Wed, Feb 3, 2010 at 09:31, Tim Bunce <Tim.Bunce@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.Bunce@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.

Ah, yes, a comment is certainly easier to write up. I'll add one
and include a link to the relevant thread in the archives.
Thanks for the example.


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

I don't see a significant issue in security definer and %_SHARED from
what you've said above. Authors of security definer functions should
naturally take care in how they use their argument values and %_SHARED.

I do see a need for a GRANT check and I'm adding one now (based on
the code in CreateFunction() in functioncmds.c - thanks to RhodiumToad
on IRC for the pointer).

Tim.

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

Alex Hunsaker <badalex@gmail.com> — 2010-02-03T18:27:24Z

On Wed, Feb 3, 2010 at 10:56, Tim Bunce <Tim.Bunce@pobox.com> wrote:
> On Wed, Feb 03, 2010 at 10:18:51AM -0700, Alex Hunsaker wrote:
>> On Wed, Feb 3, 2010 at 09:31, Tim Bunce <Tim.Bunce@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.Bunce@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.

> Ah, yes, a comment is certainly easier to write up. I'll add one
> and include a link to the relevant thread in the archives.
> Thanks for the example.

BTW I (obviously?) stuck the example in the wrong spot in plperl.c. I
did not have a tree with your patches applied handy :)

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

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-03T18:28:02Z

Tim Bunce <Tim.Bunce@pobox.com> writes:
> I do see a need for a GRANT check and I'm adding one now (based on
> the code in CreateFunction() in functioncmds.c - thanks to RhodiumToad
> on IRC for the pointer).

What exactly are you proposing to check, and where, and what do you
think that will fix?

If the concern is that someone could sabotage the behavior of a plperl
function by changing things around in the perl_init script, then I think
we have to forget about making it USERSET.  Whether someone has been
granted permission to use plperl seems to me to have little to do with
whether it's okay to mess up a function (possibly a SECURITY DEFINER
one) belonging to someone else.

			regards, tom lane

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

Alex Hunsaker <badalex@gmail.com> — 2010-02-03T18:43:40Z

On Wed, Feb 3, 2010 at 11:28, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Tim Bunce <Tim.Bunce@pobox.com> writes:
>> I do see a need for a GRANT check and I'm adding one now (based on
>> the code in CreateFunction() in functioncmds.c - thanks to RhodiumToad
>> on IRC for the pointer).
>
> What exactly are you proposing to check, and where, and what do you
> think that will fix?

Non plperl GRANTed people could modify the global $_SHARED variable.
Currently anyone that can make a plperl function can do anything they
want with $_SHARED.  So In my mind disallowing them to set
plperl.plperl_safe_init would make the permission model of $_SHARED
consistent.  No?  Now im not saying its a good permission model...

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

Alex Hunsaker <badalex@gmail.com> — 2010-02-03T18:49:01Z

On Wed, Feb 3, 2010 at 11:43, Alex Hunsaker <badalex@gmail.com> wrote:
> On Wed, Feb 3, 2010 at 11:28, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Tim Bunce <Tim.Bunce@pobox.com> writes:
>>> I do see a need for a GRANT check and I'm adding one now (based on
>>> the code in CreateFunction() in functioncmds.c - thanks to RhodiumToad
>>> on IRC for the pointer).
>>
>> What exactly are you proposing to check, and where, and what do you
>> think that will fix?
>
> Non plperl ...

Put another way:
HEAD: only people with plperl granted can make functions to manipulate $_SHARED
PATCH: anyone can set plperl.plperl_safe_init (but note not
plperlu_init as its SUSER) and manipulate $_SHARED
Proposed fix: only people with plperl granted can set
plperl.plplerl_safe_init and hence restore HEAD behavior

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

Andrew Dunstan <andrew@dunslane.net> — 2010-02-03T18:51:47Z

Tom Lane wrote:
> Tim Bunce <Tim.Bunce@pobox.com> writes:
>   
>> I do see a need for a GRANT check and I'm adding one now (based on
>> the code in CreateFunction() in functioncmds.c - thanks to RhodiumToad
>> on IRC for the pointer).
>>     
>
> What exactly are you proposing to check, and where, and what do you
> think that will fix?
>
> If the concern is that someone could sabotage the behavior of a plperl
> function by changing things around in the perl_init script, then I think
> we have to forget about making it USERSET.  Whether someone has been
> granted permission to use plperl seems to me to have little to do with
> whether it's okay to mess up a function (possibly a SECURITY DEFINER
> one) belonging to someone else.
>
> 			
>   

If we are seriously worried about use of %_SHARED in security definer 
functions then ISTM the right thing might be to have more than one and 
swap in the one for the effective user in a security definer function. 
Or possibly even disable it altogether in security definer functions.

%_SHARED has been around for several years now, and if there are genuine 
security concerns about it ISTM they would apply today, regardless of 
these patches.

cheers

andrew

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

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-03T19:04:47Z

Andrew Dunstan <andrew@dunslane.net> writes:
> %_SHARED has been around for several years now, and if there are genuine 
> security concerns about it ISTM they would apply today, regardless of 
> these patches.

Yes.  I am not at all happy about inserting nonstandard permissions
checks into GUC assign hooks --- they are not really meant for that
and I think there could be unexpected consequences.  Without a serious
demonstration of a real problem that didn't exist before, I'm not in
favor of it.

I think a more reasonable answer is just to add a documentation note
pointing out that %_SHARED should be considered insecure in a multi-user
database.

What I was actually wondering about, however, is the extent to which
the semantics of Perl code could be changed from an on_init hook ---
is there any equivalent of changing search_path or otherwise creating
trojan-horse code that might be executed unexpectedly?  And if so is
there any point in trying to guard against it?  AIUI there isn't
anything that can be done in on_init that couldn't be done in somebody
else's function anyhow.

			regards, tom lane

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

David E. Wheeler <david@kineticode.com> — 2010-02-03T19:15:56Z

On Feb 3, 2010, at 11:04 AM, Tom Lane wrote:

> What I was actually wondering about, however, is the extent to which
> the semantics of Perl code could be changed from an on_init hook ---
> is there any equivalent of changing search_path or otherwise creating
> trojan-horse code that might be executed unexpectedly?

Yes.

> And if so is
> there any point in trying to guard against it?

No. This is Perl we're talking about. The DBA should vet code before putting it into on_perl_init.

> AIUI there isn't
> anything that can be done in on_init that couldn't be done in somebody
> else's function anyhow.

Correct.

Best,

David

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

Robert Haas <robertmhaas@gmail.com> — 2010-02-03T19:20:04Z

On Wed, Feb 3, 2010 at 1:49 PM, Alex Hunsaker <badalex@gmail.com> wrote:
> On Wed, Feb 3, 2010 at 11:43, Alex Hunsaker <badalex@gmail.com> wrote:
>> On Wed, Feb 3, 2010 at 11:28, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Tim Bunce <Tim.Bunce@pobox.com> writes:
>>>> I do see a need for a GRANT check and I'm adding one now (based on
>>>> the code in CreateFunction() in functioncmds.c - thanks to RhodiumToad
>>>> on IRC for the pointer).
>>>
>>> What exactly are you proposing to check, and where, and what do you
>>> think that will fix?
>>
>> Non plperl ...
>
> Put another way:
> HEAD: only people with plperl granted can make functions to manipulate $_SHARED
> PATCH: anyone can set plperl.plperl_safe_init (but note not
> plperlu_init as its SUSER) and manipulate $_SHARED
> Proposed fix: only people with plperl granted can set
> plperl.plplerl_safe_init and hence restore HEAD behavior

I think we should just not have any unprivileged-user-settable GUCs
and then this problem goes away.  Trying to test for GRANT privilege
on the appropriate language seems like a big kludge, and I am doubtful
that it's worth it.

...Robert

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

Alex Hunsaker <badalex@gmail.com> — 2010-02-03T19:22:03Z

On Wed, Feb 3, 2010 at 12:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> %_SHARED has been around for several years now, and if there are genuine
>> security concerns about it ISTM they would apply today, regardless of
>> these patches.
>
> Yes.  I am not at all happy about inserting nonstandard permissions
> checks into GUC assign hooks

I think Tims solution is just to check in plperl.c right before we
eval it so not at SET time.

> I think a more reasonable answer is just to add a documentation note
> pointing out that %_SHARED should be considered insecure in a multi-user
> database.

Works for me. We probably want to do that anyway.

> is there any equivalent of changing search_path or otherwise creating
> trojan-horse code that might be executed unexpectedly?

Yes but not in the plperl variant.  Only with plperlu or the
plperl.init GUCs marked SUSER could you do any of the above. Which
makes me think maybe the plperl.plperlu_init function could just have
a similar permission check to plperl.plperl_safe_init and be USERSET ?
Too gross?

> And if so is
> there any point in trying to guard against it?  AIUI there isn't
> anything that can be done in on_init that couldn't be done in somebody
> else's function anyhow.

Right, the point is you can only do that if you can make those
functions (or if someone prepared a nice function for you to use).

Maybe im just being paranoid.  Leaving it the way it is now (USERSET)
is certainly easier. =)

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

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-03T19:29:14Z

Alex Hunsaker <badalex@gmail.com> writes:
> On Wed, Feb 3, 2010 at 12:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Yes. I am not at all happy about inserting nonstandard permissions
>> checks into GUC assign hooks

> I think Tims solution is just to check in plperl.c right before we
> eval it so not at SET time.

Well, that would be *completely* wrong/useless.  What you would find out
is the ID of the user who directly called the function, which would have
nothing at all to do with the privileges of whoever set the GUC.

I'm leaning in the same direction as Robert: let's just make all three
of these SUSET and stop worrying.  It's not real clear that there's much
of a use-case for letting unprivileged users set on_plperl_init anyway.
Also, we can always back it off later if we decide it's safer than it
looks.

			regards, tom lane

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

Tim Bunce <tim.bunce@pobox.com> — 2010-02-03T19:38:38Z

On Wed, Feb 03, 2010 at 02:04:47PM -0500, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
> > %_SHARED has been around for several years now, and if there are genuine 
> > security concerns about it ISTM they would apply today, regardless of 
> > these patches.
> 
> Yes.  I am not at all happy about inserting nonstandard permissions
> checks into GUC assign hooks --- they are not really meant for that
> and I think there could be unexpected consequences.  Without a serious
> demonstration of a real problem that didn't exist before, I'm not in
> favor of it.

I wasn't thinking of using GUC assign hooks (as that simply hadn't
occured to me). I was thinking of just ignoring on_plperl_init if
the user wasn't allowed to use the plperl language. Something like:

    if (user_is_su_or_has_usage_of('plperl')) {
        ... eval the on_plperl_init code ..
    }

> I think a more reasonable answer is just to add a documentation note
> pointing out that %_SHARED should be considered insecure in a multi-user
> database.

That's seems worth anyway. I'll add a note along those lines.

> What I was actually wondering about, however, is the extent to which
> the semantics of Perl code could be changed from an on_init hook ---
> is there any equivalent of changing search_path or otherwise creating
> trojan-horse code that might be executed unexpectedly?

This seems like a reasonable 'vector of first choice':

    SET plperl.on_plperl_init = '$SIG{__WARN__} = sub { ... }';

and then do something to trigger a warning from some existing plperl
function. So I think the answer is yes.

> And if so is there any point in trying to guard against it?
> AIUI there isn't anything that can be done in on_init that couldn't be
> done in somebody else's function anyhow.

(The issue here is with on_plperl_init, not on_init or on_plperlu_init as they're SUSET).

There isn't anything that can be done in on_plperl_init that can't be
done in plperl in terms of what perl code can be compiled.
It seems there is a plausable vector for trojan-horse code though, so I
think the key issue if this could be exploited in a security definer
scenario.

Tim.

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

Robert Haas <robertmhaas@gmail.com> — 2010-02-03T19:53:00Z

On Wed, Feb 3, 2010 at 2:38 PM, Tim Bunce <Tim.Bunce@pobox.com> wrote:
>> What I was actually wondering about, however, is the extent to which
>> the semantics of Perl code could be changed from an on_init hook ---
>> is there any equivalent of changing search_path or otherwise creating
>> trojan-horse code that might be executed unexpectedly?
>
> This seems like a reasonable 'vector of first choice':
>
>    SET plperl.on_plperl_init = '$SIG{__WARN__} = sub { ... }';
>
> and then do something to trigger a warning from some existing plperl
> function. So I think the answer is yes.

Perl is actually full of places where you can do things like this,
like exporting things into CORE::GLOBAL, or just polluting the package
namespace in which the code will run.  Not sure if any of this is
prevented by Safe.

...Robert

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

Andrew Dunstan <andrew@dunslane.net> — 2010-02-03T23:41:43Z

Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>   
>> %_SHARED has been around for several years now, and if there are genuine 
>> security concerns about it ISTM they would apply today, regardless of 
>> these patches.
>>     
>
> Yes.  I am not at all happy about inserting nonstandard permissions
> checks into GUC assign hooks --- they are not really meant for that
> and I think there could be unexpected consequences.  Without a serious
> demonstration of a real problem that didn't exist before, I'm not in
> favor of it.
>   

Agreed.

> I think a more reasonable answer is just to add a documentation note
> pointing out that %_SHARED should be considered insecure in a multi-user
> database.
>   

Seems fair.

> What I was actually wondering about, however, is the extent to which
> the semantics of Perl code could be changed from an on_init hook ---
> is there any equivalent of changing search_path or otherwise creating
> trojan-horse code that might be executed unexpectedly?  And if so is
> there any point in trying to guard against it?  AIUI there isn't
> anything that can be done in on_init that couldn't be done in somebody
> else's function anyhow.
>
> 			
>   

The user won't be able to do anything in the on_init hook that they 
could not do from a plperl function anyway. In fact less, as SPI is not 
being made available.

Plperl functions can't load arbitrary modules at all, and can't modify 
perl's equivalent of search_path. So I think the plain answer to your 
wonder ins "no, it can't happen."

There has been discussion in the past about allowing plperl functions 
access to certain modules. There is some sense in doing this, as it 
would help to avoid having to write plperlu for perfectly innocuous 
operations. But the list of allowed modules would have to be carefully 
controlled in something like a SIGHUP setting. We're certainly not going 
to allow such decisions to be made by anyone but the DBA.

cheers

andrew

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

Robert Haas <robertmhaas@gmail.com> — 2010-02-04T02:13:11Z

On Wed, Feb 3, 2010 at 6:41 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
>> What I was actually wondering about, however, is the extent to which
>> the semantics of Perl code could be changed from an on_init hook ---
>> is there any equivalent of changing search_path or otherwise creating
>> trojan-horse code that might be executed unexpectedly?  And if so is
>> there any point in trying to guard against it?  AIUI there isn't
>> anything that can be done in on_init that couldn't be done in somebody
>> else's function anyhow.
>>
> The user won't be able to do anything in the on_init hook that they could
> not do from a plperl function anyway. In fact less, as SPI is not being made
> available.

But suppose the user doesn't have privileges to create a plperl
function, but they can set the GUC...

...Robert

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

Robert Haas <robertmhaas@gmail.com> — 2010-02-12T05:22:28Z

On Wed, Feb 3, 2010 at 6:41 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> Tom Lane wrote:
>>
>> Andrew Dunstan <andrew@dunslane.net> writes:
>>
>>>
>>> %_SHARED has been around for several years now, and if there are genuine
>>> security concerns about it ISTM they would apply today, regardless of these
>>> patches.
>>>
>>
>> Yes.  I am not at all happy about inserting nonstandard permissions
>> checks into GUC assign hooks --- they are not really meant for that
>> and I think there could be unexpected consequences.  Without a serious
>> demonstration of a real problem that didn't exist before, I'm not in
>> favor of it.
>>
>
> Agreed.
>
>> I think a more reasonable answer is just to add a documentation note
>> pointing out that %_SHARED should be considered insecure in a multi-user
>> database.
>>
>
>
> Seems fair.
>
>> What I was actually wondering about, however, is the extent to which
>> the semantics of Perl code could be changed from an on_init hook ---
>> is there any equivalent of changing search_path or otherwise creating
>> trojan-horse code that might be executed unexpectedly?  And if so is
>> there any point in trying to guard against it?  AIUI there isn't
>> anything that can be done in on_init that couldn't be done in somebody
>> else's function anyhow.
>>
>>
>>
>
> The user won't be able to do anything in the on_init hook that they could
> not do from a plperl function anyway. In fact less, as SPI is not being made
> available.
>
> Plperl functions can't load arbitrary modules at all, and can't modify
> perl's equivalent of search_path. So I think the plain answer to your wonder
> ins "no, it can't happen."
>
> There has been discussion in the past about allowing plperl functions access
> to certain modules. There is some sense in doing this, as it would help to
> avoid having to write plperlu for perfectly innocuous operations. But the
> list of allowed modules would have to be carefully controlled in something
> like a SIGHUP setting. We're certainly not going to allow such decisions to
> be made by anyone but the DBA.

The discussion on this seems to have died off.  Andrew, do you have
something you're planning to commit for this?  I think we're kind of
down to the level of bikeshedding here...

...Robert

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

Tim Bunce <tim.bunce@pobox.com> — 2010-02-12T10:40:13Z

On Fri, Feb 12, 2010 at 12:22:28AM -0500, Robert Haas wrote:
> On Wed, Feb 3, 2010 at 6:41 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
> > Tom Lane wrote:
> >> Andrew Dunstan <andrew@dunslane.net> writes:
> >>
> >>> %_SHARED has been around for several years now, and if there are genuine
> >>> security concerns about it ISTM they would apply today, regardless of these
> >>> patches.
> >>
> >> Yes.  I am not at all happy about inserting nonstandard permissions
> >> checks into GUC assign hooks --- they are not really meant for that
> >> and I think there could be unexpected consequences.  Without a serious
> >> demonstration of a real problem that didn't exist before, I'm not in
> >> favor of it.
> >
> > Agreed.
> >
> >> I think a more reasonable answer is just to add a documentation note
> >> pointing out that %_SHARED should be considered insecure in a multi-user
> >> database.
> >
> > Seems fair.
> >
> >> What I was actually wondering about, however, is the extent to which
> >> the semantics of Perl code could be changed from an on_init hook ---
> >> is there any equivalent of changing search_path or otherwise creating
> >> trojan-horse code that might be executed unexpectedly?  And if so is
> >> there any point in trying to guard against it?  AIUI there isn't
> >> anything that can be done in on_init that couldn't be done in somebody
> >> else's function anyhow.
> >
> > The user won't be able to do anything in the on_init hook that they could
> > not do from a plperl function anyway. In fact less, as SPI is not being made
> > available.
> >
> > Plperl functions can't load arbitrary modules at all, and can't modify
> > perl's equivalent of search_path. So I think the plain answer to your wonder
> > ins "no, it can't happen."
> >
> > There has been discussion in the past about allowing plperl functions access
> > to certain modules. There is some sense in doing this, as it would help to
> > avoid having to write plperlu for perfectly innocuous operations. But the
> > list of allowed modules would have to be carefully controlled in something
> > like a SIGHUP setting. We're certainly not going to allow such decisions to
> > be made by anyone but the DBA.
> 
> The discussion on this seems to have died off.  Andrew, do you have
> something you're planning to commit for this?  I think we're kind of
> down to the level of bikeshedding here...

Following this thread I posted an updated patch:
http://archives.postgresql.org/message-id/20100205134044.GO52427@timac.local

which Alex Hunsaker reviewed (and marked Ready For Committer) in
http://archives.postgresql.org/pgsql-hackers/2010-02/msg00391.php

Andrew Dunstan also reviewed it and highlighted some docs issues in
http://archives.postgresql.org/pgsql-hackers/2010-02/msg00488.php

I replied in http://archives.postgresql.org/pgsql-hackers/2010-02/msg00515.php
and posted a further patch update to reflect the needed doc changes in
http://archives.postgresql.org/message-id/20100208204213.GH1618@timac.local

That updated patch is in the commitfest
https://commitfest.postgresql.org/action/patch_view?id=271

From talking to Andrew via IM I understand he's very busy, and also that
he'll be traveling on Sunday and Monday.

I certainly hope he can commit this patch today (along with the next
patch, which is also marked ready for committer) but if not, is there
someone else who could?

What happens to patches marked 'ready for committer' after the
commitfest ends?

Tim.

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

Andrew Dunstan <andrew@dunslane.net> — 2010-02-12T13:53:01Z


Tim Bunce wrote:
> That updated patch is in the commitfest
> https://commitfest.postgresql.org/action/patch_view?id=271
>
> >From talking to Andrew via IM I understand he's very busy, and also that
> he'll be traveling on Sunday and Monday.
>
> I certainly hope he can commit this patch today (along with the next
> patch, which is also marked ready for committer) but if not, is there
> someone else who could?
>   

Working on it now.

> What happens to patches marked 'ready for committer' after the
> commitfest ends?
>
>
>   

It's not the end of the world. But anyway, this will make the cut, and 
possibly your other patch too.

cheers

andrew

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

Andrew Dunstan <andrew@dunslane.net> — 2010-02-12T13:56:48Z

Robert Haas wrote:
> The discussion on this seems to have died off.  Andrew, do you have
> something you're planning to commit for this?  I think we're kind of
> down to the level of bikeshedding here...
>
>
>   

There is documentation in Tim's patch I am working on right now. I don't 
think anything else is needed.

cheers

andrew

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

Robert Haas <robertmhaas@gmail.com> — 2010-02-12T15:16:42Z

On Fri, Feb 12, 2010 at 5:40 AM, Tim Bunce <Tim.Bunce@pobox.com> wrote:
> What happens to patches marked 'ready for committer' after the
> commitfest ends?

We talk about it and figure out what to do.  It'll be some combination
of (1) last-minute commits,  (2) postponing things to 9.1, and (3)
rejecting things we don't ever want.

...Robert

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

Robert Haas <robertmhaas@gmail.com> — 2010-02-12T15:24:40Z

On Fri, Feb 12, 2010 at 8:56 AM, Andrew Dunstan <andrew@dunslane.net> wrote:
> There is documentation in Tim's patch I am working on right now. I don't
> think anything else is needed.

Cool.

...Robert