Re: pgsql: Add new GUC createrole_self_grant.

On Fri, Jan 13, 2023 at 8:29 PM David G. Johnston
<david(dot)g(dot)johnston(at)gmail(dot)com> wrote:
>> The point of the security definer section is to explain how to safely write
>> security definer functions that you grant to less privileged users
>
> Yeah, we are really good at "how".
>
> + If the security definer function intends to create roles, and if it
> + is running as a non-superuser, <varname>createrole_self_grant</varname>
> + should also be set to a known value using the <literal>SET</literal>
> + clause.
>
> I'd like to know "why". Without knowing why we are adding this I can't give it a +1. I want the patch to include the why.

What don't you understand about the "why"? If your security-definer
function relies on some GUC having some particular value for some
security-critical purpose, and the caller can substitute some other
value, they might be able to create a security compromise. Since this
GUC has some connection to security, there is at least some distant
possibility of that happening. Reasonable people can, perhaps, differ
about how likely that is, but I don't really see what's confusing
about the theory. As a general statement about the human condition, if
you know that someone may be untrustworthy, you should be careful
about letting them influence your decisions. If you aren't, something
bad may happen to you.

The whole thing about SECURITY INVOKER functions is really a separate
issue. You can tell people how to write SECURITY DEFINER functions
more safely, and we do. You cannot tell them how to write SECURITY
INVOKER functions more safely, because the direction of the attack is
reversed. In the case of SECURITY DEFINER, the caller of the function
can attack the owner of the function. In the case of a SECURITY
INVOKER function, the owner of the function can attack the caller of
the function. We can't document how to write security invoker
functions safely because the author of the function is the one
potentially making an attack, and therefore would do the opposite of
whatever advice we gave. We *could* add whole new sections to the
documentation telling people to be careful about calling security
invoker functions, and that's a fine thing to discuss, but what I'm
doing here is following up an already-committed patch by adjusting
parts of the existing documentation to account for the changes.
Inventing whole new sections of the documentation would be a job for a
new patch on a new thread, not a follow-up patch on an existing
thread.

--
Robert Haas
EDB: http://www.enterprisedb.com