Re: Facility for detecting insecure object naming

> On Aug 14, 2018, at 8:00 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Wed, Aug 8, 2018 at 1:58 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I'm not sold on #2 either. That path leads to, for example,
>> s/=/OPERATOR(pg_catalog.=)/g everywhere, which is utterly catastrophic
>> to both readability and portability of your SQL code. We *must* find
>> a way to do better, not tell people that's what to do.
>>
>> When the security team was discussing this issue before, we speculated
>> about ideas like inventing a function trust mechanism, so that attacks
>> based on search path manipulations would fail even if they managed to
>> capture an operator reference. I'd rather go down that path than
>> encourage people to do more schema qualification.
>
> The more I think about it, the more I think having a way to set a
> lexically-scoped search path is probably the answer. Many years ago,
> Perl had only "local" as a way of declaring local variables, which
> used dynamic scoping, and then they invented "my", which uses lexical
> scoping, and everybody abandoned "local" and started using "my,"
> because otherwise a reference to a variable inside a procedure may
> happen to refer to a local variable further up the call stack rather
> than a global variable if the names happen to collide. It turns out
> that not knowing to which object a reference will refer is awful, and
> you should avoid it like the plague. This is exactly the same problem
> we have with search_path. People define functions -- or index
> expressions -- assuming that the function and operator references will
> refer to the same thing at execution time that they do at definition
> time.
>
> That is, I think, an entirely *reasonable* expectation. Basically all
> programming languages work that way. If you could call a C function
> in such a way that the meaning of identifiers that appeared there was
> dependent on some piece of state inherited from the caller's context
> rather than from the declarations visible at the time that the C
> function was compiled, it would be utter and complete chaos. It would
> in fact greatly resemble the present state of affairs in PostgreSQL,
> where making your code reliably mean what you intended requires either
> forcing the search path everywhere or schema-qualifying the living
> daylights out of everything. I think we should try to follow Perl's
> example, acknowledge that we basically got this wrong on the first
> try, and invent something new that works better.

I think you are interpreting the problem too broadly. This is basically
just a privilege escalation attack vector. If there is a function that
gets run under different privileges than those of the caller, and if the
caller can coerce the function to do something with those elevated
privileges that the function author did not intend, then the caller can
do mischief. But if the caller is executing a function that operates
under the caller's own permissions, then there is no mischief possible,
since the caller can't trick the function to do anything that the caller
couldn't do herself directly.

For example, if there is a function that takes type anyarray and then
performs operations over the elements of that array using operator=,
there is no problem with me defining an operator= prior to calling that
function and having my operator be the one that gets used, assuming the
function doesn't escalate privileges.

To my mind, the search_path should get set whenever SetUserIdAndSecContext
or similar gets called and restored when SetUserIdAndSecContext gets called
to restore the saved uid. Currently coded stuff like:

GetUserIdAndSecContext(&saved_uid, &save_sec_context);
...
if (saved_uid != authid_uid)
SetUserIdAndSecContext(authid_uid,
save_sec_context | SECURITY_LOCAL_USERID_CHANGE);
...
SetUserIdAndSecContext(saved_uid, save_sec_context);

need to have a third variable, saved_search_path or similar.

Thoughts?

mark