Re: pgsql: Add new GUC createrole_self_grant.

On Sat, Jan 14, 2023 at 8:12 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
> OK, given all of that, I suggest reworking the first paragraph of security definer functions safely to something like
thefollowing: 
>
> (Replace: Because a SECURITY DEFINER function is executed with the privileges of the user that owns it, care is
neededto ensure that the function cannot be misused. For security, search_path should be set to exclude any schemas
writableby untrusted users.) with: 
>
> The execution of a SECURITY DEFINER function has two interacting behaviors that make writing and administering such
functionsrequire extra care.  While the privileges that come into play during execution are those of the function
owner,the execution environment is inherited from the calling context.  Therefore, any settings that the function
reliesupon must be specified in the SET clause of the CREATE command (or within the body of the function). 
>
> Of particular importance is the search_path setting.  The search_path should be set to the bare minimum required for
thefunction to operate and, more importantly, not include any schemas writable by untrusted users. 
>
> <existing wording>
> This prevents malicious users [...]
> (existing example)
> [...] the function could be subverted by creating a temporary table named pwds.
> </existing wording>

I find this wording less clear than what we have now. And I reiterate
that the purpose of the patch under discussion is to add a mention of
the new GUC to an existing section, not to rewrite that section -- or
any other section -- of the documentation.

> <added note="specifically by this patch">
> Another setting of note (at least in the case that the function owner is not a superuser) is createrole_self_grant.
Whilethe function owner has their own pg_db_role_setting preference for this setting, when wrapping execution of CREATE
ROLEwithin a function, particularly to be executed by others, it is the executor's setting that would be in effect, not
theowner's. 
> </added>

I think these sentences are really contorted, and they are also
factually incorrect. For this setting to matter, you need (1) the
function to be running CREATEROLE and (2) the owner of that function
to not be a superuser. You've put those facts at opposite end of the
sentence. You've also brought pg_db_role_setting into it, which
doesn't matter here because it's applied at login, and a security
definer function doesn't log in as the user to which it switches.
There really is no such thing as "the owner's" setting. There may be a
setting which is applied to the owner's session if the owner logs in,
but there's no default value for all code run as the owner -- perhaps
there should be, but that's not how it works. I don't think we have
much precedent for using the word "executor" to mean "the user who
called a function" as opposed to "the code that executed a planned
query".

I don't really think there's too much wrong with what I wrote in the
patch as proposed, and I would like to get it committed and move on
without getting drawn into a wide-ranging discussion of every way in
which we might be able to improve the surrounding structure.

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