fixing CREATEROLE

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