Re: Controlling changes in plpgsql variable resolution

On Wed, Oct 21, 2009 at 4:28 PM, Merlin Moncure <mmoncure@gmail.com> wrote:
> On Wed, Oct 21, 2009 at 3:09 PM, Josh Berkus <josh@agliodbs.com> wrote:
>> Tom has proposed some kind of odd special "options" syntax to get around
>> this, but I think that's unnecessary.  So far on this thread, I haven't
>> seen anyone engineer an actual function exploit by using this setting; I
>> personally can't come up with one off the top of my head which doesn't
>> require the attacker to be the table owner, the function owner, or the
>> superuser themselves.  Also keep in mind what we're patching here is an
>> unmaintanable and insecure practice anyway, which is the ambiguous use
>> of variable names which match column names.
>>
>> So, I'm saying: make it a userset.
>
> I couldn't disagree more strongly.  .conf entries that adjust how
> plpgsql funtions operate in a very central way will 'fork'  plpgsql
> develoeprs so that you have one group of people using method 'a' and
> another using method 'b'.  Nobody bothers to fix legacy code and now
> we have a first class mess.  All code intended to run on servers you
> don't control (like library code) now needs to be decorated with 'set
> local...' which defeats the whole purpose.  IMO, guc settings that
> control how sql behaves should be avoided at all costs.  You should be
> able to look at a piece of code and explicitly determine what it does.
>  At least with #option, knowing the server version and the function
> body is enough.  if you want to support multiple behaviors but don't
> like #option, i think the only alternative is to version the plpgsql
> language somehow and decorate 'create function' with the version.  Tom
> didn't like that idea,  but it still beats GUC imo.

I agree.  We already have one case (search_path) where you potentially
can't accurately predict the semantics of the function without knowing
something about the calling environment.  That means every
security-definer function, to be secure, has to explicitly control the
search path.  That's bad.

I actually think that we should not have a GUC for this at all.  We
should have a compiled-in default, and it should be error.  If you
want some other behavior, decorate your functions with #option.  The
only way this is not a train wreck is if the semantics are set *from
within the function*.  Having any portion of the semantics, no matter
how seemingly trivial, imported from the outside is just asking to get
whacked on the head with a stream of difficult-to-diagnose
misbehavior.  Even if we assume, with a heavy dose of unjustified
optimism, that no security vulnerabilities will result, it's not going
to be fun or pleasant.

...Robert