Thread: Add on_trusted_init and on_untrusted_init to plperl UPDATED [PATCH]
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.
Attachment
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
contextof a superuser calling a non-superuser security definer function, or perhaps vice versa. Whose permissions
applyto 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" ?
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
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
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
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
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.
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
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...
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
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 HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
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.
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?
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
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*.
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
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_initplperl.plperl_safe_initplperl.plperlu_init
?
regards, tom lane
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 plperlplperl.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.
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?
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]
From
"David E. Wheeler"
Date:
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 arejust too confusing (even Tom mixed them up in a post last week). They are among the worst names in the system, IMHO. Best, David
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.
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 :)
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
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...
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
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
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]
From
"David E. Wheeler"
Date:
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
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
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. =)
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
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.
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
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
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
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
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.
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
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
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
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