Re: pgsql: Add new GUC createrole_self_grant.

On Tue, Jan 10, 2023 at 10:25 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Of course, if it's possible for a non-CREATEROLE user to set the value
> > that a CREATEROLE user experiences, that'd be more of a problem --
>
> That's exactly the case I'm worried about, and it's completely reachable
> if a CREATEROLE user makes a SECURITY DEFINER function that executes
> an affected GRANT and is callable by an unprivileged user. Now, that
> probably isn't terribly likely, and it's unclear that there'd be any
> serious security consequence even if the GRANT did do something
> different than the author of the SECURITY DEFINER function was expecting.
> Nonetheless, I'm feeling itchy about this chain of assumptions.

If you want to make safe a SECURITY DEFINER function written using sql
or plpgsql, you either have to schema-qualify every single reference
or, more realistically, attach a SET clause to the function to set the
search_path to a sane value during the time that the function is
executing. The problem here can be handled the same way, except that
it's needed in a vastly more limited set of circumstances: you have to
be calling a SECURITY DEFINER function that will execute CREATE ROLE
as a non-superuser (and that user then needs to be sensitive to the
value of this GUC in some security-relevant way). It might be good to
document this -- I just noticed that the CREATE FUNCTION page has a
section on "Writing SECURITY DEFINER Functions Safely" which talks
about dealing with the search_path issues, and it seems like it would
be worth adding a sentence or two there to talk about this.

It might also be a good idea to make the section of the page that
explains the meaning of the SECURITY INVOKER and SECURITY DEFINER
clauses cross-link to the section on writing such functions safely,
because that section is way down at the bottom of the page and seems
easy to miss.

But I'm not convinced that we need more than documentation changes here.

> Bottom line is that a GUC doesn't feel like the right mechanism to use.
> What do you think about inventing a role property, or a grantable role
> that controls this behavior?

Well, I originally had a pair of role properties called
INHERITCREATEDROLES and SETCREATEDROLES, but nobody liked that,
including you. One reason was probably that the names are long and
ugly, but this is also unlike existing role properties, which
typically can only be changed by a superuser, or by a CREATEROLE user
with ADMIN OPTION on the target role. Since you don't have admin
option on yourself, that would mean you couldn't change this property
for yourself, which I wasn't too exercised about, but everyone else
who commented disliked it. We could go back to having those properties
and have the rules for changing them be different than the roles for
changing other role properties, I suppose.

But I don't think that's better. Some of the existing role properties
(SUPERUSER, REPLICATION, BYPASSRLS, CREATEROLE, CREATEDB) are
capabilities, and others (LOGIN, CONNECTION LIMIT, VALID UNTIL) are
limits established by the system administrator. This is neither: it's
a default. So if we put it into the role property system, then we're
saying this one particular default is a role property, whereas pretty
much all of the others are GUCs. Now, admittedly, I did have it as a
role property originally, but that was before we decided that it made
sense to give the CREATEROLE user, rather than the superuser, control
over the value of the property. And I think we decided that for good
reason, because the CREATEROLE user can always turn "no" into "yes" by
granting the created role back to themselves with additional options.
They can't necessarily turn "yes" into "no," because we could create
the implicit grant with one or both of those options turned on and the
CREATEROLE user can't undo that. But nobody thought that was a useful
thing to enforce, and neither do I. Given that shift in thinking, I
have a hard time believing that it's a good idea to shove this option
into a system that is meant for capabilities or limits and has no
existing precedent for anything that behaves like a setting or user
preference (except perhaps ALTER USER ... PASSWORD '...' but calling
that a preference might be a stretch).

If we used grantable roles, I suppose the design there would to invent
two new predefined role and grant them to the CREATEROLE user, or not,
to affect whether and how subsequent implicit self-grants by that user
would be performed. But that again takes the decision out of the hands
of the CREATEROLE user, and it again puts a user preference into a
system that we currently use only for capabilities.

Stepping back a second, I think that your underlying concern here is
that the entire GUC system is shaky from a security perspective.
Trying to write SECURITY DEFINER functions or procedures in sql or
plpgsql is not unlike trying to write a safe setuid shell script.
Among the problems with trying to do that is that the caller might
establish surprising values for PATH, IFS, or other environment
variables before calling your script, and if you're not careful,
you'll end up doing whatever the caller wants instead of what you
intended to be doing. I think it's really justifiable to be worried
about the same kinds of problems within PostgreSQL, but I don't think
that the right solution is to have new settings opt out of the GUC
mechanism on a retail basis. We need a solution that's going to work
for every GUC we have, in a consistent and understandable way, and if
we can't have that then we at least need a solution for search_path.
If we come up with such a solution, it seems likely that also adopting
that solution for createrole_self_grant would be a good idea. But if
we don't, I have trouble believing that doing something only for
createrole_self_grant is really going to improve security.

In fact, it might make it harder to fix the real problems in this
area. If we have all of our settings in one system, then any solution
we devise can apply to all of them equally. If we start storing some
of them in other places, it's potentially more separate things that
have to be fixed. I don't want to make overly strong statements here
because I don't think we really know what any of the fixes to these
problems are. If we can find some place to put this where it fits
nicely and that place isn't the GUC system, well and good: I don't
mind writing a patch to do it. But what I don't want to do is do
contortions to avoid relying on GUCs because we don't trust the GUC
mechanism in general. I don't think that kind of thing will end well.

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