Re: Package namespace and Safe init cleanup for plperl [PATCH]

Lists: pgsql-hackers
From: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>
To: pgsql-hackers(at)postgresql(dot)org
Cc: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>
Subject: Package namespace and Safe init cleanup for plperl [PATCH]
Date: 2010-01-15 04:02:02
Message-ID: 20100115040202.GN8024@timac.local
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

This is the final plperl patch in the series from me.

Changes in this patch:

- Moved internal functions out of main:: namespace
into PostgreSQL::InServer and PostgreSQL::InServer::safe

- Restructured Safe compartment setup code
to generalize and separate the data from the logic.

Neither change has any user visible effects.

This patch will apply cleanly over the 'Add on_trusted_init and
on_untrusted_init to plperl' patch:
https://commitfest.postgresql.org/action/patch_view?id=271

Tim.

Attachment Content-Type Size
plperl-nscleanup.patch text/x-patch 6.9 KB

From: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>
To: pgsql-hackers(at)postgresql(dot)org
Cc: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>
Subject: Re: Package namespace and Safe init cleanup for plperl [PATCH]
Date: 2010-01-25 19:53:26
Message-ID: 20100125195326.GD78082@timac.local
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Fri, Jan 15, 2010 at 04:02:02AM +0000, Tim Bunce wrote:
> This is the final plperl patch in the series from me.
>
> Changes in this patch:
>
> - Moved internal functions out of main:: namespace
> into PostgreSQL::InServer and PostgreSQL::InServer::safe
>
> - Restructured Safe compartment setup code
> to generalize and separate the data from the logic.
>
> Neither change has any user visible effects.

This is a revised version of the patch with the following additional
changes:

- Further generalized the 'what to load into Safe compartment' logic.

- Added the 'warnings' pragma to the list of modules to load into Safe.
So plperl functions can now "use warnings;" - added test for that.

- Added 'use 5.008001;' to plc_perlboot.pl as a run-time check to
complement the configure-time check added by Tom Lane recently.

Tim.

Attachment Content-Type Size
plperl-nscleanup2.patch text/x-patch 9.1 KB

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
Subject: Re: Package namespace and Safe init cleanup for plperl [PATCH]
Date: 2010-01-30 03:07:30
Message-ID: 34d269d41001291907x16b70b4ftbffb31258ca92f23@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Mon, Jan 25, 2010 at 12:53, Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com> wrote:
> - Added the 'warnings' pragma to the list of modules to load into Safe.
>  So plperl functions can now "use warnings;" - added test for that.

*yay*

> - Added 'use 5.008001;' to plc_perlboot.pl as a run-time check to
>  complement the configure-time check added by Tom Lane recently.

For the curious the above works quite nicely.:
=> do $$ 1; $$ language plperl;
ERROR: Perl v5.20.1 required--this is only v5.10.1, stopped at line 1.
BEGIN failed--compilation aborted at line 1.

A couple of comments. *note* I have not tested this as a whole yet
(due to rejects).

in plc_perboot.pl
+$funcsrc .= qq[ package main; undef *{'$name'}; *{'$name'} = sub {
$BEGIN $prolog $src } ];

Any thoughts on using a package other than main? Maybe something like
package PlPerl or what not? Not that I think it really matters...
But it might be nicer to see in say NYTprof ?

in plc_safe_ok.pl
+use vars qw($PLContainer $SafeClass @EvalInSafe @ShareIntoSafe);

Is there some reason not to use my here? The only reason I can think
of is you want the *_init gucs to be able to stick things into
@ShareIntoSafe and friends? And if so should we document that
somewhere (or was that in an earlier patch that i missed?)? Or does
vars do some other magic that im not seeing that we need here?

Also whats the use case for $SafeClass? Multiple versions of Safe.pm?
Other modules that are like Safe.pm? Im just curious...

Hrm I also dont see the point of the new "use Safe;" as we still eval
it in in plperl_safe_init() care to enlighten a lost soul?

Other than those really quite minor questions that are arguably me
nitpicking... It looks great to me.


From: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>
To: Alex Hunsaker <badalex(at)gmail(dot)com>
Cc: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Package namespace and Safe init cleanup for plperl [PATCH]
Date: 2010-01-30 14:51:22
Message-ID: 20100130145122.GF1141@timac.local
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Fri, Jan 29, 2010 at 08:07:30PM -0700, Alex Hunsaker wrote:
> A couple of comments. *note* I have not tested this as a whole yet
> (due to rejects).
>
> in plc_perboot.pl
> +$funcsrc .= qq[ package main; undef *{'$name'}; *{'$name'} = sub {
> $BEGIN $prolog $src } ];
>
> Any thoughts on using a package other than main? Maybe something like
> package PlPerl or what not? Not that I think it really matters...
> But it might be nicer to see in say NYTprof ?

The only interface Safe provides for sharing things with the compartment
shares them with the root package of the compartment which, from within
the compartment, is 'main::'.

It's an unfortunate limitation of Safe. I could have added more code to
wordaround that but opted to keep it simple for this commitfest.

> in plc_safe_ok.pl
> +use vars qw($PLContainer $SafeClass @EvalInSafe @ShareIntoSafe);
>
> Is there some reason not to use my here? The only reason I can think
> of is you want the *_init gucs to be able to stick things into
> @ShareIntoSafe and friends? And if so should we document that
> somewhere (or was that in an earlier patch that i missed?)?

It's a step towards providing an interface to give the DBA control over
what gets loaded into the Safe compartment and what gets shared with it.

I opted to not try to define a formal documented interface because I
felt it was likely to be a source of controversy and/or bikeshedding.
This late in the game I didn't want to take that chance.

I'd rather a few brave souls get some experience with it as-as, and collect
some good use-cases, before proposing a formal documented API.

> Also whats the use case for $SafeClass? Multiple versions of Safe.pm?
> Other modules that are like Safe.pm? Im just curious...

It's possible someone might want to use an alternative module.
Most likely their own local subclass of Safe that adds extra features.
I'd explored that when trying to integrate NYTProf. It seemed worth
at least making possible.

> Hrm I also dont see the point of the new "use Safe;" as we still eval
> it in in plperl_safe_init() care to enlighten a lost soul?

It just makes the file more self-contained for syntax checking:
perl -cw plc_safeboot.pl

> Other than those really quite minor questions that are arguably me
> nitpicking... It looks great to me.

Great, thanks Alex!

Tim.


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
Subject: Re: Package namespace and Safe init cleanup for plperl [PATCH]
Date: 2010-01-30 18:08:26
Message-ID: 34d269d41001301008r4c17edaehd75379547496d60e@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Sat, Jan 30, 2010 at 07:51, Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com> wrote:
> On Fri, Jan 29, 2010 at 08:07:30PM -0700, Alex Hunsaker wrote:
>> A couple of comments. *note* I have not tested this as a whole yet
>> (due to rejects).
>>
>> in plc_perboot.pl
>> +$funcsrc .= qq[ package main; undef *{'$name'}; *{'$name'} = sub {
>> $BEGIN $prolog $src } ];
>>
>> Any thoughts on using a package other than main?  Maybe something like
>> package PlPerl or what not?  Not that I think it really matters...
>> But it might be nicer to see in say NYTprof ?
>
> The only interface Safe provides for sharing things with the compartment
> shares them with the root package of the compartment which, from within
> the compartment, is 'main::'.
>
> It's an unfortunate limitation of Safe. I could have added more code to
> wordaround that but opted to keep it simple for this commitfest.

I thought it might be something like that.

>> in plc_safe_ok.pl
>> +use vars qw($PLContainer $SafeClass @EvalInSafe @ShareIntoSafe);
>>
>> Is there some reason not to use my here?  The only reason I can think
>> of is you want the *_init gucs to be able to stick things into
>> @ShareIntoSafe and friends?  And if so should we document that
>> somewhere (or was that in an earlier patch that i missed?)?
>
> It's a step towards providing an interface to give the DBA control over
> what gets loaded into the Safe compartment and what gets shared with it.
>
> I opted to not try to define a formal documented interface because I
> felt it was likely to be a source of controversy and/or bikeshedding.
> This late in the game I didn't want to take that chance.
>
> I'd rather a few brave souls get some experience with it as-as, and collect
> some good use-cases, before proposing a formal documented API.

Fine with me.

>> Also whats the use case for $SafeClass?  Multiple versions of Safe.pm?
>>  Other modules that are like Safe.pm? Im just curious...
>
> It's possible someone might want to use an alternative module.
> Most likely their own local subclass of Safe that adds extra features.
> I'd explored that when trying to integrate NYTProf.  It seemed worth
> at least making possible.
>
>> Hrm I also dont see the point of the new "use Safe;"  as we still eval
>> it in in plperl_safe_init() care to enlighten a lost soul?
>
> It just makes the file more self-contained for syntax checking:
>    perl -cw plc_safeboot.pl
>
>> Other than those really quite minor questions that are arguably me
>> nitpicking...  It looks great to me.
>
> Great, thanks Alex!

I marked it as ready, though ill add a comment that it depends on the
other patch to apply cleanly (and if that one gets rebased...
obviously this one probably will need to as well).


From: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>
To: Alex Hunsaker <badalex(at)gmail(dot)com>
Cc: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Package namespace and Safe init cleanup for plperl [PATCH]
Date: 2010-01-30 18:37:07
Message-ID: 20100130183707.GH1141@timac.local
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Sat, Jan 30, 2010 at 11:08:26AM -0700, Alex Hunsaker wrote:
> On Sat, Jan 30, 2010 at 07:51, Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com> wrote:
> > On Fri, Jan 29, 2010 at 08:07:30PM -0700, Alex Hunsaker wrote:
> >
> >> Other than those really quite minor questions that are arguably me
> >> nitpicking...  It looks great to me.
> >
> > Great, thanks Alex!
>
> I marked it as ready, though ill add a comment that it depends on the
> other patch to apply cleanly (and if that one gets rebased...
> obviously this one probably will need to as well).

I've rebased and reposted the other patch ("Add on_trusted_init and
on_untrusted_init to plperl") and added it to the commitfest.

I've rebasing this one now and making one minor change: I'm adding an
if (not our $_init++) {
...
}
around the code that adds items to @EvalInSafe and @ShareIntoSafe to
avoid any problems with on_trusted_init code that causes exceptions.

Currently an exception from on_trusted_init will leave the plperl
interpreter in the 'not yet setup' state. So the next use runs
the plc_safe_ok code and the on_trusted_init code again.
This change makes the plc_safe_ok code idempotent so the re-execution
won't cause any problems (except for harmless warnings about the
safe_eval and mksafefunc subs being redefined).

Patch to follow (after getting the kids to bed).

Tim.


From: Andrew Dunstan <andrew(at)dunslane(dot)net>
To: Alex Hunsaker <badalex(at)gmail(dot)com>
Cc: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Package namespace and Safe init cleanup for plperl [PATCH]
Date: 2010-02-12 20:42:54
Message-ID: 4B75BD4E.7070603@dunslane.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Alex Hunsaker wrote:
>>> in plc_safe_ok.pl
>>> +use vars qw($PLContainer $SafeClass @EvalInSafe @ShareIntoSafe);
>>>
>>> Is there some reason not to use my here? The only reason I can think
>>> of is you want the *_init gucs to be able to stick things into
>>> @ShareIntoSafe and friends? And if so should we document that
>>> somewhere (or was that in an earlier patch that i missed?)?
>>>
>> It's a step towards providing an interface to give the DBA control over
>> what gets loaded into the Safe compartment and what gets shared with it.
>>
>> I opted to not try to define a formal documented interface because I
>> felt it was likely to be a source of controversy and/or bikeshedding.
>> This late in the game I didn't want to take that chance.
>>
>> I'd rather a few brave souls get some experience with it as-as, and collect
>> some good use-cases, before proposing a formal documented API.
>>
>
> Fine with me.
>
>

I'm not sure it's fine with me.

I'm a bit inclined to commit the piece of this patch that concerns the
warnings pragma, and leave the rest for the next release, unless you can
convince me real fast that we're not opening a back door here. If we're
going to allow DBAs to add things to the Safe container, it's going to
be up front or not at all, as far as I'm concerned.

I hope I haven't misunderstood what's going on here.

cheers

andrew


From: Alex Hunsaker <badalex(at)gmail(dot)com>
To: Andrew Dunstan <andrew(at)dunslane(dot)net>
Cc: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>, pgsql-hackers(at)postgresql(dot)org, "David E(dot) Wheeler" <david(at)kineticode(dot)com>
Subject: Re: Package namespace and Safe init cleanup for plperl [PATCH]
Date: 2010-02-12 21:30:37
Message-ID: 34d269d41002121330x6ad3ab6bi89a257f3de3de582@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Fri, Feb 12, 2010 at 13:42, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
>
> Alex Hunsaker wrote:
>>>>
>>>> in plc_safe_ok.pl
>>>> +use vars qw($PLContainer $SafeClass @EvalInSafe @ShareIntoSafe);
>>>> ...the *_init gucs to be able to stick things into
>>>> @ShareIntoSafe and friends?
>
> I'm not sure it's fine with me.
>
> I'm a bit inclined to commit the piece of this patch that concerns the
> warnings pragma,

I think a sizable portion of the patch can be dropped if you do the
above. Namely the whole double init protection that got added in the
last round.

> and leave the rest for the next release, unless you can
> convince me real fast that we're not opening a back door here. If we're
> going to allow DBAs to add things to the Safe container, it's going to be up
> front or not at all, as far as I'm concerned.

I think backdoor is a bit extreme. Yes it could allow people who
can set the plperl.*_init functions to muck with the safe. As an
admin I could also do that by setting plperl.on_init and overloading
subs in the Safe namespace or switching out Safe.pm.

Anyway reasons I can come up for it are:
-its general so we can add other modules/pragmas easily in the future
-helps with the plperl/plperlu all or nothing situation
-starts to flesh out how an actual exposed (read documented) interface
should look for 9.1

... I know Tim mentioned David as having some use cases (cc'ed)

So my $0.2 is I don't have any strong feelings for/against it. I
think if we could expose *something* (even if you can only get to it
in a C contrib module) that would be great. But I realize it might be
impractical at this stage in the game.

*shrug*


From: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>
To: Alex Hunsaker <badalex(at)gmail(dot)com>
Cc: Andrew Dunstan <andrew(at)dunslane(dot)net>, Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>, pgsql-hackers(at)postgresql(dot)org, "David E(dot) Wheeler" <david(at)kineticode(dot)com>
Subject: Re: Package namespace and Safe init cleanup for plperl [PATCH]
Date: 2010-02-12 23:45:54
Message-ID: 20100212234554.GU373@timac.local
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Fri, Feb 12, 2010 at 02:30:37PM -0700, Alex Hunsaker wrote:
> On Fri, Feb 12, 2010 at 13:42, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> > Alex Hunsaker wrote:
> >>>>
> > and leave the rest for the next release, unless you can
> > convince me real fast that we're not opening a back door here. If we're
> > going to allow DBAs to add things to the Safe container, it's going to be up
> > front or not at all, as far as I'm concerned.
>
> I think backdoor is a bit extreme. Yes it could allow people who
> can set the plperl.*_init functions to muck with the safe. As an
> admin I could also do that by setting plperl.on_init and overloading
> subs in the Safe namespace or switching out Safe.pm.

Exactly.

[As I mentioned before, the docs for Devel::NYTProf::PgPLPerl
http://code.google.com/p/perl-devel-nytprof/source/browse/trunk/lib/Devel/NYTProf/PgPLPerl.pm
talk about the need to _hack_ perl standard library modules
in order to be able to run NYTProf on plperl code. The PERL5OPT
environment variable could be used as an alternative vector.]

Is there any kind of threat model (http://en.wikipedia.org/wiki/Threat_model)
that PostgreSQL developers use when making design decisions?

Clearly the PostgreSQL developers take security very seriously, and
rightly so. An explicit threat model document would provide a solid
framework to assess potential security issues when their raised.

The key issue here seems to be the trust, or lack of it, placed in DBAs.

> Anyway reasons I can come up for it are:
> -its general so we can add other modules/pragmas easily in the future
> -helps with the plperl/plperlu all or nothing situation
> -starts to flesh out how an actual exposed (read documented) interface
> should look for 9.1
>
> ... I know Tim mentioned David as having some use cases (cc'ed)

I've just posted the docs for a module that's a good example of the
kind of module a DBA might want to allow plperl code to use
http://archives.postgresql.org/pgsql-hackers/2010-02/msg01059.php

I'd be happy to add a large scary warning in the plc_safe_ok.pl
file saying that any manipulation of the (undocumented) variables
could cause a security risk and is not supported in any way.

Tim.

> So my $0.2 is I don't have any strong feelings for/against it. I
> think if we could expose *something* (even if you can only get to it
> in a C contrib module) that would be great. But I realize it might be
> impractical at this stage in the game.
>
> *shrug*
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>


From: Andrew Dunstan <andrew(at)dunslane(dot)net>
To: Alex Hunsaker <badalex(at)gmail(dot)com>
Cc: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>, pgsql-hackers(at)postgresql(dot)org, "David E(dot) Wheeler" <david(at)kineticode(dot)com>
Subject: Re: Package namespace and Safe init cleanup for plperl [PATCH]
Date: 2010-02-13 00:57:15
Message-ID: 4B75F8EB.40800@dunslane.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Alex Hunsaker wrote:
> Yes it could allow people who
> can set the plperl.*_init functions to muck with the safe. As an
> admin I could also do that by setting plperl.on_init and overloading
> subs in the Safe namespace or switching out Safe.pm.
>

It's quite easy to subvert Safe.pm today, sadly. You don't need on*init,
nor do you need to replace the System's Safe.pm. I'm not going to tell
people how here, but it took me all of a few minutes to discover and
verify when I went looking a few weeks ago, once I thought about it.

But that's quite different from us providing an undocumented way to
expose arbitrary objects to the Safe container. In that case *we* become
responsible for any insecure uses, and we don't even have the luxury of
having put large warnings in the docs, because there aren't any docs.

Another objection is that discussion on this facility has been pretty
scant. I think that's putting it mildly. Maybe it's been apparent to a
few people what the implications are, but I doubt it's been apparent to
many. That makes me quite nervous, which is why I'm raising it now.

Tim mentioned in his reply that he'd be happy to put warnings in
plc_safe_ok.pl. But that file only exists in the source - it's turned
into header file data as part of the build process, so warnings there
will be seen by roughly nobody.

I still think if we do this at all it needs to be documented and
surrounded with appropriate warnings.

cheers

andrew


From: Alex Hunsaker <badalex(at)gmail(dot)com>
To: Andrew Dunstan <andrew(at)dunslane(dot)net>
Cc: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>, pgsql-hackers(at)postgresql(dot)org, "David E(dot) Wheeler" <david(at)kineticode(dot)com>
Subject: Re: Package namespace and Safe init cleanup for plperl [PATCH]
Date: 2010-02-13 01:17:47
Message-ID: 34d269d41002121717u7dccf542ra3946971780d599@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Fri, Feb 12, 2010 at 17:57, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> Another objection is that discussion on this facility has been pretty scant.
> I think that's putting it mildly.

Well I can certainly attest to that seeing as how I spotted it purely
by review...


From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Andrew Dunstan <andrew(at)dunslane(dot)net>
Cc: Alex Hunsaker <badalex(at)gmail(dot)com>, Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Package namespace and Safe init cleanup for plperl [PATCH]
Date: 2010-02-13 02:14:47
Message-ID: 603c8f071002121814v3848a896x9265504b8ffd345f@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Fri, Feb 12, 2010 at 3:42 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> I'm a bit inclined to commit the piece of this patch that concerns the
> warnings pragma, and leave the rest for the next release,

Based on the subsequent discussion on this thread, +1 for this
approach. Allowing use warnings sounds good; opening a potential
security hole sounds bad. Sounds like it would considerably simplify
the patch, too.

...Robert


From: Alex Hunsaker <badalex(at)gmail(dot)com>
To: Andrew Dunstan <andrew(at)dunslane(dot)net>
Cc: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>, pgsql-hackers(at)postgresql(dot)org, "David E(dot) Wheeler" <david(at)kineticode(dot)com>
Subject: Re: Package namespace and Safe init cleanup for plperl [PATCH]
Date: 2010-02-13 04:08:38
Message-ID: 34d269d41002122008rb7009b8rf078c7649afc72e6@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Fri, Feb 12, 2010 at 17:57, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>r
>
> Alex Hunsaker wrote:
>>
>>  Yes it could allow people who
>> can set the plperl.*_init functions to muck with the safe.
>
> It's quite easy to subvert Safe.pm today, sadly. ...

If anything that sounds like an argument for it =)

> But that's quite different from us providing an undocumented way to expose
> arbitrary objects to the Safe container. In that case *we* become
> responsible for any insecure uses, and we don't even have the luxury of
> having put large warnings in the docs, because there aren't any docs.

Hrm... Not sure I agree with this point. If you are saying there is
some way to subvert safe by using these new vars thats not a bug (or
feature) of upstream safe. Then I agree. But if what you are saying
is as a (super)user I muck with these (internal) vars in my on_init
function and things become insecure. Then I disagree, its just a less
ugly (uniform and perhaps more secure?) way of doing the below:

NYTProf/ PgPLPerl.pm
# hack to make DB::finish_profile available within PL/Perl
use Safe;
my $orig_share_from = \&Safe::share_from;
*Safe::share_from = sub {
my $obj = shift;
$obj->$orig_share_from('DB', [ 'finish_profile' ]);
return $obj->$orig_share_from(@_);
};

> I still think if we do this at all it needs to be documented and surrounded
> with appropriate warnings.

This is really what I think the issue comes down to. I think the
feeling is if we document it then we have to support it in the future.
And we dont have a clear proposal, only a need. The attitude seems
to be, well its an implementation artifact that might disappear in the
future. Lets use it to help figure out what that future api should
like like.

I agree with Robert. At this point in the commit feast its not the
time to be discussing things like this (sorry I could not get to it
sooner Tim!) :( Though If a patch with good documentation does show
up Ill be happy to review it :)


From: Alex Hunsaker <badalex(at)gmail(dot)com>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Andrew Dunstan <andrew(at)dunslane(dot)net>, Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Package namespace and Safe init cleanup for plperl [PATCH]
Date: 2010-02-13 04:14:07
Message-ID: 34d269d41002122014v795ca10ek12f24fc27692d6e9@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Fri, Feb 12, 2010 at 19:14, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Sounds like it would considerably simplify
> the patch, too.

I overstated that. The way its done now we could just change the
decelerations to 'my' and drop an if block. Id be in favor of more or
less keeping the internals as implemented in the latest patch the same
just not exposing them.


From: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>
To: Andrew Dunstan <andrew(at)dunslane(dot)net>
Cc: Alex Hunsaker <badalex(at)gmail(dot)com>, Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>, pgsql-hackers(at)postgresql(dot)org, "David E(dot) Wheeler" <david(at)kineticode(dot)com>
Subject: Re: Package namespace and Safe init cleanup for plperl [PATCH]
Date: 2010-02-13 10:17:55
Message-ID: 20100213101755.GV373@timac.local
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Fri, Feb 12, 2010 at 07:57:15PM -0500, Andrew Dunstan wrote:
> Alex Hunsaker wrote:
> > Yes it could allow people who
> >can set the plperl.*_init functions to muck with the safe. As an
> >admin I could also do that by setting plperl.on_init and overloading
> >subs in the Safe namespace or switching out Safe.pm.
>
> It's quite easy to subvert Safe.pm today, sadly. You don't need
> on*init, nor do you need to replace the System's Safe.pm. I'm not
> going to tell people how here, but it took me all of a few minutes
> to discover and verify when I went looking a few weeks ago, once I
> thought about it.
>
> But that's quite different from us providing an undocumented way to
> expose arbitrary objects to the Safe container. In that case *we*
> become responsible for any insecure uses, and we don't even have the
> luxury of having put large warnings in the docs, because there
> aren't any docs.

I wish I understood better how PostgreSQL developers would be
responsible for a DBA using an undocumented hack. In my view the DBA is
clearly responsible in that case.

If it's not documented it's not supported.

> Another objection is that discussion on this facility has been
> pretty scant. I think that's putting it mildly. Maybe it's been
> apparent to a few people what the implications are, but I doubt it's
> been apparent to many. That makes me quite nervous, which is why I'm
> raising it now.
>
> Tim mentioned in his reply that he'd be happy to put warnings in
> plc_safe_ok.pl. But that file only exists in the source - it's
> turned into header file data as part of the build process, so
> warnings there will be seen by roughly nobody.
>
> I still think if we do this at all it needs to be documented and
> surrounded with appropriate warnings.

I'm away for a day or so (for my wedding anniversary) but I'll have an
updated patch, with a clean well-documented API, for consideration by
Monday morning.

Tim.


From: Andrew Dunstan <andrew(at)dunslane(dot)net>
To: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>
Cc: Alex Hunsaker <badalex(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org, "David E(dot) Wheeler" <david(at)kineticode(dot)com>
Subject: Re: Package namespace and Safe init cleanup for plperl [PATCH]
Date: 2010-02-13 14:32:31
Message-ID: 4B76B7FF.10704@dunslane.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Tim Bunce wrote:
>> But that's quite different from us providing an undocumented way to
>> expose arbitrary objects to the Safe container. In that case *we*
>> become responsible for any insecure uses, and we don't even have the
>> luxury of having put large warnings in the docs, because there
>> aren't any docs.
>>
>
> I wish I understood better how PostgreSQL developers would be
> responsible for a DBA using an undocumented hack. In my view the DBA is
> clearly responsible in that case.
>
> If it's not documented it's not supported.
>

My feeling is if we provide something we are responsible for it,
documented or not. Undocumented features with security implications
raise big red flags in my head. Maybe the difference in perspective
comes from working on a database as opposed to working on a language.

>>
>> I still think if we do this at all it needs to be documented and
>> surrounded with appropriate warnings.
>>
>
> I'm away for a day or so (for my wedding anniversary) but I'll have an
> updated patch, with a clean well-documented API, for consideration by
> Monday morning.
>
>
>

Happy anniversary! But on reflection I think it's too late for this. We
are a month into the commitfest. Alex's suggestion on how to proceed
(change the declarations on the relevant items to make them
inaccessible) makes sense to me.

We've always been fighting time on these PLPerl patches and we're
moderately lucky to have got as much in as we have.

In case I haven't said it before, your work on this is very much
appreciated.

cheers

andrew


From: "David E(dot) Wheeler" <david(at)kineticode(dot)com>
To: Andrew Dunstan <andrew(at)dunslane(dot)net>
Cc: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>, Alex Hunsaker <badalex(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Package namespace and Safe init cleanup for plperl [PATCH]
Date: 2010-02-13 22:38:29
Message-ID: E8D769F2-26BF-4F01-9688-A2C2C921B287@kineticode.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Feb 13, 2010, at 6:32 AM, Andrew Dunstan wrote:

> My feeling is if we provide something we are responsible for it, documented or not. Undocumented features with security implications raise big red flags in my head. Maybe the difference in perspective comes from working on a database as opposed to working on a language.

I'm confused. Doesn't on_plperl_init already give us this? Isn't any of the stuff loaded by that GUC then available from inside the PLPerl Safe compartment?

Best,

David


From: Andrew Dunstan <andrew(at)dunslane(dot)net>
To: "David E(dot) Wheeler" <david(at)kineticode(dot)com>
Cc: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>, Alex Hunsaker <badalex(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Package namespace and Safe init cleanup for plperl [PATCH]
Date: 2010-02-13 22:46:05
Message-ID: 4B772BAD.7000002@dunslane.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

David E. Wheeler wrote:
> On Feb 13, 2010, at 6:32 AM, Andrew Dunstan wrote:
>
>
>> My feeling is if we provide something we are responsible for it, documented or not. Undocumented features with security implications raise big red flags in my head. Maybe the difference in perspective comes from working on a database as opposed to working on a language.
>>
>
> I'm confused. Doesn't on_plperl_init already give us this? Isn't any of the stuff loaded by that GUC then available from inside the PLPerl Safe compartment?
>
>
>

No (and if it does it's a bug). Try it and see.

cheers

andrew


From: "David E(dot) Wheeler" <david(at)kineticode(dot)com>
To: Andrew Dunstan <andrew(at)dunslane(dot)net>
Cc: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>, Alex Hunsaker <badalex(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Package namespace and Safe init cleanup for plperl [PATCH]
Date: 2010-02-13 23:32:41
Message-ID: E5C2BC1D-E8F7-40D5-9E1F-F53832EDFEFA@kineticode.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Feb 13, 2010, at 2:46 PM, Andrew Dunstan wrote:

>> I'm confused. Doesn't on_plperl_init already give us this? Isn't any of the stuff loaded by that GUC then available from inside the PLPerl Safe compartment?
>
> No (and if it does it's a bug). Try it and see.

Then what's the point of it?

David


From: Andrew Dunstan <andrew(at)dunslane(dot)net>
To: "David E(dot) Wheeler" <david(at)kineticode(dot)com>
Cc: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>, Alex Hunsaker <badalex(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Package namespace and Safe init cleanup for plperl [PATCH]
Date: 2010-02-13 23:35:42
Message-ID: 4B77374E.1070703@dunslane.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

David E. Wheeler wrote:
> On Feb 13, 2010, at 2:46 PM, Andrew Dunstan wrote:
>
>
>>> I'm confused. Doesn't on_plperl_init already give us this? Isn't any of the stuff loaded by that GUC then available from inside the PLPerl Safe compartment?
>>>
>> No (and if it does it's a bug). Try it and see.
>>
>
> Then what's the point of it?
>
>
>

To perform initialisation, such as setting a value in %_SHARED.

cheers

andrew


From: "David E(dot) Wheeler" <david(at)kineticode(dot)com>
To: Andrew Dunstan <andrew(at)dunslane(dot)net>
Cc: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>, Alex Hunsaker <badalex(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Package namespace and Safe init cleanup for plperl [PATCH]
Date: 2010-02-14 00:00:25
Message-ID: 06EF65A7-F3D0-4B08-81AE-171DB1F206E7@kineticode.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Feb 13, 2010, at 3:35 PM, Andrew Dunstan wrote:

> To perform initialisation, such as setting a value in %_SHARED.

Hrm. Well, as a DBA, I'd *really* like to be able to make some things available from within a Safe container, such as Devel::NYTProf::PgPLPerl or the PostgreSQL::PLPerl::Call module that Tim's working on. Right now I can do that by hacking warnings.pm directly or by the method you figured out a few weeks ago, which isn't really all that nasty.

I'm not sure that Tim's interface is the best approach to giving DBAs the ability to do this from within PostgreSQL, but I'm hard-pressed to come up with a better interface. But I do think it should be allowed.

Best,

David


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: "David E(dot) Wheeler" <david(at)kineticode(dot)com>
Cc: Andrew Dunstan <andrew(at)dunslane(dot)net>, Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>, Alex Hunsaker <badalex(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Package namespace and Safe init cleanup for plperl [PATCH]
Date: 2010-02-14 00:25:48
Message-ID: 10348.1266107148@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
> I'm not sure that Tim's interface is the best approach to giving DBAs
> the ability to do this from within PostgreSQL, but I'm hard-pressed to
> come up with a better interface. But I do think it should be allowed.

I think the fact that security worries have been raised is enough reason
to go slow on this. Let's plan to think about it some more, and put it
into 9.1 if no real problems emerge.

regards, tom lane


From: Andrew Dunstan <andrew(at)dunslane(dot)net>
To: "David E(dot) Wheeler" <david(at)kineticode(dot)com>
Cc: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>, Alex Hunsaker <badalex(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Package namespace and Safe init cleanup for plperl [PATCH]
Date: 2010-02-14 00:38:20
Message-ID: 4B7745FC.4080907@dunslane.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

David E. Wheeler wrote:
> On Feb 13, 2010, at 3:35 PM, Andrew Dunstan wrote:
>
>
>> To perform initialisation, such as setting a value in %_SHARED.
>>
>
> Hrm. Well, as a DBA, I'd *really* like to be able to make some things available from within a Safe container, such as Devel::NYTProf::PgPLPerl or the PostgreSQL::PLPerl::Call module that Tim's working on. Right now I can do that by hacking warnings.pm directly or by the method you figured out a few weeks ago, which isn't really all that nasty.
>
> I'm not sure that Tim's interface is the best approach to giving DBAs the ability to do this from within PostgreSQL, but I'm hard-pressed to come up with a better interface. But I do think it should be allowed.
>

I am not saying we shouldn't. But that's a new feature, and we do have a
process to follow. I think it's long past the time when we can accept
new features which have had almost no discussion.

And I am saying that any facility we provide for it has to be known and
documented.

Clearly there are modules which could probably be used with reasonable
safety. Math::BigInt and MIME::Base64 are examples that come to mind as
likely candidates.

But I am not prepared to commit a patch providing this facility based on
just having a variable declared using "use vars" rather than "my",
without a word in the docs, and with almost no discussion on -hackers,
and that discussion starting two weeks after the start of the final
commitfest. That's just not the way we do things round here, AIUI.

cheers

andrew