fixing CREATEROLE - Mailing list pgsql-hackers
From | Robert Haas |
---|---|
Subject | fixing CREATEROLE |
Date | |
Msg-id | CA+TgmobGds7oefDjZUY+k_J7p1sS=pTq3sZ060qdb=oKei1Dkw@mail.gmail.com Whole thread Raw |
Responses |
Re: fixing CREATEROLE
Re: fixing CREATEROLE Re: fixing CREATEROLE |
List | pgsql-hackers |
The CREATEROLE permission is in a very bad spot right now. The biggest problem that I know about is that it allows you to trivially access the OS user account under which PostgreSQL is running, which is expected behavior for a superuser but simply wrong behavior for any other user. This is because CREATEROLE conveys powerful capabilities not only to create roles but also to manipulate them in various ways, including granting any non-superuser role in the system to any new or existing user, including themselves. Since v11, the roles that can be granted include pg_execute_server_program and pg_write_server_files which are trivially exploitable. Perhaps this should have been treated as an urgent security issue and a fix back-patched, although it is not clear to me exactly what such a fix would look like. Since we haven't done that, I went looking for a way to improve things in a principled way going forward, taking advantage also of recent master-only work to improve various aspects of the role grant system. Here, I feel it important to point out that I think the current system would be broken even if we didn't have predefined roles that are trivially exploitable to obtain OS user access. We would still lack any way to restrict the scope of the CREATEROLE privilege. Sure, the privilege doesn't extend to superusers, but that's not really good enough. Consider: rhaas=# create role alice createrole; CREATE ROLE rhaas=# create role bob password 'known_only_to_bob'; CREATE ROLE rhaas=# set session authorization alice; SET rhaas=> alter role bob password 'known_to_alice'; ALTER ROLE Assuming that some form of password authentication is supported, alice is basically empowered to break into any non-superuser account on the system and assume all of its privileges. That's really not cool: it's OK, I think, to give a non-superuser the right to change somebody else's passwords, but it should be possible to limit it in some way, e.g. to the users that alice creates. Also, while the ability to make this sort of change seems to be the clear intention of the code, it's not documented on the CREATE ROLE page. The problems with pg_execute_server_program et. al. are not documented either; all it says is that you should "regard roles that have the CREATEROLE privilege as almost-superuser-roles," which seems to me to be understating the extent of the problem. I have drafted a few patches to try to improve the situation. It seems to me that the root of any fix in this area must be to change the rule that CREATEROLE can administer any role whatsoever. Instead, I propose to change things so that you can only administer roles for which you have ADMIN OPTION. Administering a role here includes changing the password for a role, renaming a role, dropping a role, setting the comment or security label on a role, or granting membership in that role to another role, whether at role creation time or later. All of these options are treated in essentially two places in the code, so it makes sense to handle them all in a symmetric way. One problem with this proposal is that, if we did exactly this much, then a CREATEROLE user wouldn't be able to administer the roles which they themselves had just created. That seems like it would be restricting the privileges of CREATEROLE users too much. To fix that, I propose when a non-superuser creates a role, the role be implicitly granted back to the creator WITH ADMIN OPTION. This arguably doesn't add any fundamentally new capability because the CREATEROLE user could do something like "CREATE ROLE some_new_role ADMIN myself" anyway, but that's awkward to remember and doing it automatically seems a lot more convenient. However, there's a little bit of trickiness here, too. Granting the new role back to the creator might, depending on whether the INHERIT or SET flags are true or false for the new grant, allow the CREATEROLE user to inherit the privileges of, or set role to, the target role, which under current rules would not be allowed. We can minimize behavior changes from the status quo by setting up the new, implicit grant with SET FALSE, INHERIT FALSE. However, that might not be what everyone wants. It's definitely not what *I* want. I want a way for non-superusers to create new roles and automatically inherit the privileges of those roles just as a superuser automatically inherits everyone's privileges. I just don't want the users who can do this to also be able to break out to the OS as if they were superusers when they're not actually supposed to be. However, it's clear from previous discussion that other people do NOT want that, so I propose to make it configurable. Thus, this patch series also proposes to add INHERITCREATEDROLES and SETCREATEDROLES properties to roles. These have no meaning if the role is not marked CREATEROLE. If it is, then they control the properties of the implicit grant that happens when a CREATEROLE user who is not a superuser creates a role. If INHERITCREATEDROLES is set, then the implicit grant back to the creator is WITH INHERIT TRUE, else it's WITH INHERIT FALSE; likewise, SETCREATEDROLES affects whether the implicit grant is WITH SET TRUE or WITH SET FALSE. I'm curious to hear what other people think of these proposals, but let me first say what I think about them. First, I think it's clear that we need to do something, because things right now are pretty badly broken and in a way that affects security. Although these patches are not back-patchable, they at least promise to improve things as older versions go out of use. Second, it's possible that we should look for back-patchable fixes here, but I can't really see that we're going to come up with anything much better than just telling people not to use this feature against older releases, because back-patching catalog changes or dramatic behavior changes seems like a non-starter. In other words, I think this is going to be a master-only fix. Third, someone could well have a better or just different idea how to fix the problems in this area than what I'm proposing here. This is the best that I've been able to come up with so far, but that's not to say it's free of problems or that no improvements are possible. Finally, I think that whatever we do about the code, the documentation needs quite a bit of work, because the code is doing a lot of stuff that is security-critical and entirely non-obvious from the documentation. I have not in this version of these patches included any documentation changes and the regression test changes that I have included are quite minimal. That all needs to be fixed up before there could be any thought of moving forward with these patches. However, I thought it best to get rough patches and an outline of the proposed direction on the table first, before doing a lot of work refining things. -- Robert Haas EDB: http://www.enterprisedb.com
Attachment
pgsql-hackers by date: