Re: pgsql: Add new GUC createrole_self_grant. - Mailing list pgsql-hackers
From | Robert Haas |
---|---|
Subject | Re: pgsql: Add new GUC createrole_self_grant. |
Date | |
Msg-id | CA+Tgmob=67ghS9SQ8iEV5kMUQn_E4Bdp_=yvkdt8W74eu-v5tQ@mail.gmail.com Whole thread Raw |
In response to | Re: pgsql: Add new GUC createrole_self_grant. (Tom Lane <tgl@sss.pgh.pa.us>) |
Responses |
Re: pgsql: Add new GUC createrole_self_grant.
|
List | pgsql-hackers |
On Tue, Jan 10, 2023 at 10:25 PM Tom Lane <tgl@sss.pgh.pa.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
pgsql-hackers by date: