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
Attachments:
v1-0003-Restrict-the-privileges-of-CREATEROLE-users.patchapplication/octet-stream; name=v1-0003-Restrict-the-privileges-of-CREATEROLE-users.patchDownload+106-58
v1-0004-Add-role-attributes-INHERITCREATEDROLES-and-SETCR.patchapplication/octet-stream; name=v1-0004-Add-role-attributes-INHERITCREATEDROLES-and-SETCR.patchDownload+99-17
v1-0001-Refactor-permissions-checking-for-role-grants.patchapplication/octet-stream; name=v1-0001-Refactor-permissions-checking-for-role-grants.patchDownload+54-63
v1-0002-Pass-down-current-user-ID-to-AddRoleMems-and-DelR.patchapplication/octet-stream; name=v1-0002-Pass-down-current-user-ID-to-AddRoleMems-and-DelR.patchDownload+22-20
Robert Haas:
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.
Agreed.
Instead, I propose
to change things so that you can only administer roles for which you
have ADMIN OPTION. [...] > I'm curious to hear what other people think of these proposals, [...]
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.
Once you can restrict CREATEROLE to only manage "your own" (no matter
how that is defined, e.g. via ADMIN or through some "ownership" concept)
roles, the possibility to "namespace" those roles somehow will become a
lot more important. For example in a multi-tenant setup in the same
cluster, where each tenant has their own database and admin user with a
restricted CREATEROLE privilege, it will very quickly be at least quite
annoying to have conflicts with other tenants' role names. I'm not sure
whether it could even be a serious problem, because I should still be
able to GRANT my own roles to other roles from other tenants - and that
could affect matching of +group records in pg_hba.conf?
My suggestion to $subject and the namespace problem would be to
introduce database-specific roles, i.e. add a database column to
pg_authid. Having this column set to 0 will make the role a cluster-wide
role ("cluster role") just as currently the case. But having a database
oid set will make the role exist in the context of that database only
("database role"). Then, the following principles should be enforced:
- database roles can not share the same name with a cluster role.
- database roles can have the same name as database roles in other
databases.
- database roles can not be members of database roles in **other**
databases.
- database roles with CREATEROLE can only create or alter database roles
in their own database, but not roles in other databases and also not
cluster roles.
- database roles with CREATEROLE can GRANT all database roles in the
same database, but only those cluster roles they have ADMIN privilege on.
- database roles with CREATEROLE can not set SUPERUSER.
To be able to create database roles with a cluster role, there needs to
be some syntax, e.g. something like
CREATE ROLE name IN DATABASE dbname ...
A database role with CREATEROLE should not need to use that syntax,
though - every CREATE ROLE should be IN DATABASE anyway.
With database roles, it would be possible to hand out CREATEROLE without
the ability to grant SUPERUSER or any of the built-in roles. It would be
much more useful on top of that, too. Not only is the namespace problem
mentioned above solved, but it would also be possible to let pg_dump
dump a whole database, including the database roles and their
memberships. This would allow dumping (and restoring) a single
tenant/application including the relevant roles and privileges - without
dumping all roles in the cluster. Plus, it's backwards compatible
because without creating database roles, everything stays exactly the same.
Best,
Wolfgang
On Tue, Nov 22, 2022 at 3:02 AM <walther@technowledgy.de> wrote:
My suggestion to $subject and the namespace problem would be to
introduce database-specific roles, i.e. add a database column to
pg_authid. Having this column set to 0 will make the role a cluster-wide
role ("cluster role") just as currently the case. But having a database
oid set will make the role exist in the context of that database only
("database role"). Then, the following principles should be enforced:- database roles can not share the same name with a cluster role.
- database roles can have the same name as database roles in other
databases.
- database roles can not be members of database roles in **other**
databases.
- database roles with CREATEROLE can only create or alter database roles
in their own database, but not roles in other databases and also not
cluster roles.
- database roles with CREATEROLE can GRANT all database roles in the
same database, but only those cluster roles they have ADMIN privilege on.
- database roles with CREATEROLE can not set SUPERUSER.To be able to create database roles with a cluster role, there needs to
be some syntax, e.g. something likeCREATE ROLE name IN DATABASE dbname ...
I have three comments on this:
1. It's a good idea and might make for some interesting followup work.
2. There are some serious implementation challenges because the
constraints on duplicate object names must be something which can be
enforced by unique constraints on the relevant catalogs. Off-hand, I
don't see how to do that. It would be easy to make the cluster roles
all have unique names, and it would be easy to make the database roles
have unique names within each database, but I have no idea how you
would keep a database role from having the same name as a cluster
role. For anyone to try to implement this, we'd need to have a
solution to that problem.
3. I don't want to sidetrack this thread into talking about possible
future features or followup work. There is enough to do just getting
consensus on the design ideas that I proposed without addressing the
question of what else we might do later. I do not think there is any
reasonable argument that we can't clean up the CREATEROLE mess without
also implementing database-specific roles, and I have no intention of
including that in this patch series. Whether I or someone else might
work on it in the future is a question we can leave for another day.
--
Robert Haas
EDB: http://www.enterprisedb.com
Robert Haas:
2. There are some serious implementation challenges because the
constraints on duplicate object names must be something which can be
enforced by unique constraints on the relevant catalogs. Off-hand, I
don't see how to do that. It would be easy to make the cluster roles
all have unique names, and it would be easy to make the database roles
have unique names within each database, but I have no idea how you
would keep a database role from having the same name as a cluster
role. For anyone to try to implement this, we'd need to have a
solution to that problem.
For each database created, create a partial unique index:
CREATE UNIQUE INDEX ... ON pg_authid (rolname) WHERE roldatabase IN (0,
<database_oid>);
Is that possible on catalogs?
Best,
Wolfgang
walther@technowledgy.de writes:
Robert Haas:
2. There are some serious implementation challenges because the
constraints on duplicate object names must be something which can be
enforced by unique constraints on the relevant catalogs. Off-hand, I
don't see how to do that.
For each database created, create a partial unique index:
CREATE UNIQUE INDEX ... ON pg_authid (rolname) WHERE roldatabase IN (0,
<database_oid>);
Is that possible on catalogs?
No, we don't support partial indexes on catalogs, and I don't think
we want to change that. Partial indexes would require expression
evaluations occurring at very inopportune times.
Also, we don't support creating shared indexes post-initdb.
The code has hard-wired lists of which relations are shared,
besides which there's no way to update other databases' pg_class.
Even without that, the idea of a shared catalog ending up with 10000
indexes after you create 10000 databases (requiring 10^8 pg_class
entries across the whole cluster) seems ... unattractive.
regards, tom lane
Tom Lane:
No, we don't support partial indexes on catalogs, and I don't think
we want to change that. Partial indexes would require expression
evaluations occurring at very inopportune times.
I see. Is that the same for indexes *on* an expression? Or would those
be ok?
With a custom operator, an EXCLUDE constraint on the ROW(reldatabase,
relname) expression could work. The operator would compare:
- (0, name1) and (0, name2) as name1 == name2
- (db1, name1) and (0, name2) as name1 == name2
- (0, name1) and (db2, name2) as name1 == name2
- (db1, name1) and (db2, name2) as db1 == db2 && name1 == name2
or just (db1 == 0 || db2 == 0 || db1 == db2) && name1 == name2.
Now, you are going to tell me that EXCLUDE constraints are not supported
on catalogs either, right? ;)
Best,
Wolfgang
Wolfgang Walther:
Tom Lane:
No, we don't support partial indexes on catalogs, and I don't think
we want to change that. Partial indexes would require expression
evaluations occurring at very inopportune times.I see. Is that the same for indexes *on* an expression? Or would those
be ok?With a custom operator, an EXCLUDE constraint on the ROW(reldatabase,
relname) expression could work. The operator would compare:
- (0, name1) and (0, name2) as name1 == name2
- (db1, name1) and (0, name2) as name1 == name2
- (0, name1) and (db2, name2) as name1 == name2
- (db1, name1) and (db2, name2) as db1 == db2 && name1 == name2or just (db1 == 0 || db2 == 0 || db1 == db2) && name1 == name2.
Does it even need to be on the expression? I don't think so. It would be
enough to just make it compare relname WITH = and reldatabase WITH the
custom operator (db1 == 0 || db2 == 0 || db1 == db2), right?
Best,
Wolfgang
walther@technowledgy.de writes:
No, we don't support partial indexes on catalogs, and I don't think
we want to change that. Partial indexes would require expression
evaluations occurring at very inopportune times.
I see. Is that the same for indexes *on* an expression? Or would those
be ok?
Right, we don't support those on catalogs either.
Now, you are going to tell me that EXCLUDE constraints are not supported
on catalogs either, right? ;)
Nor those.
regards, tom lane
On 11/21/22 15:39, Robert Haas wrote:
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.
+1
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.
Yep, seems highly likely
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.
On quick inspection I like what you have proposed and no significantly
"better" ideas jump to mind. I will try to think on it though.
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.
I have looked at, and even done some doc improvements in this area in
the past, and concluded that it is simply hard to describe it in a
clear, straightforward way.
There are multiple competing concepts (privs on objects, attributes of
roles, membership, when things are inherited versus not, settings bound
to roles, etc). I don't know what to do about it, but yeah, fixing the
documentation would be a noble goal.
--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Nov 21, 2022, at 12:39 PM, Robert Haas <robertmhaas@gmail.com> wrote:
I have drafted a few patches to try to improve the situation.
The 0001 and 0002 patches appear to be uncontroversial refactorings. Patch 0003 looks on-point and a move in the right direction. The commit message in that patch is well written. Patch 0004 feels like something that won't get committed. The INHERITCREATEDROLES and SETCREATEDROLES in 0004 seems clunky.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Tue, Nov 22, 2022 at 3:01 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
On Nov 21, 2022, at 12:39 PM, Robert Haas <robertmhaas@gmail.com> wrote:
I have drafted a few patches to try to improve the situation.The 0001 and 0002 patches appear to be uncontroversial refactorings. Patch 0003 looks on-point and a move in the right direction. The commit message in that patch is well written.
Thanks.
Patch 0004 feels like something that won't get committed. The INHERITCREATEDROLES and SETCREATEDROLES in 0004 seems clunky.
I think role properties are kind of clunky in general, the way we've
implemented them in PostgreSQL, but I don't really see why these are
worse than anything else. I think we need some way to control the
behavior, and I don't really see a reasonable place to put it other
than a per-role property. And if we're going to do that then they
might as well look like the other properties that we've already got.
Do you have a better idea?
--
Robert Haas
EDB: http://www.enterprisedb.com
On Nov 22, 2022, at 2:02 PM, Robert Haas <robertmhaas@gmail.com> wrote:
Patch 0004 feels like something that won't get committed. The INHERITCREATEDROLES and SETCREATEDROLES in 0004 seems clunky.
I think role properties are kind of clunky in general, the way we've
implemented them in PostgreSQL, but I don't really see why these are
worse than anything else. I think we need some way to control the
behavior, and I don't really see a reasonable place to put it other
than a per-role property. And if we're going to do that then they
might as well look like the other properties that we've already got.Do you have a better idea?
Whatever behavior is to happen in the CREATE ROLE statement should be spelled out in that statement. "CREATE ROLE bob WITH INHERIT false WITH SET false" doesn't seem too unwieldy, and has the merit that it can be read and understood without reference to hidden parameters. Forcing this to be explicit should be safer if these statements ultimately make their way into dump/restore scripts, or into logical replication.
That's not to say that I wouldn't rather that it always work one way or always the other. It's just to say that I don't want it to work differently based on some poorly advertised property of the role executing the command.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Tue, Nov 22, 2022 at 5:48 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
Whatever behavior is to happen in the CREATE ROLE statement should be spelled out in that statement. "CREATE ROLE bob WITH INHERIT false WITH SET false" doesn't seem too unwieldy, and has the merit that it can be read and understood without reference to hidden parameters. Forcing this to be explicit should be safer if these statements ultimately make their way into dump/restore scripts, or into logical replication.
That's not to say that I wouldn't rather that it always work one way or always the other. It's just to say that I don't want it to work differently based on some poorly advertised property of the role executing the command.
That seems rather pejorative. If we stipulate from the outset that the
property is poorly advertised, then obviously anything at all
depending on it is not going to seem like a very good idea. But why
should we assume that it will be poorly-advertised? I clearly said
that the documentation needs a bunch of work, and that I planned to
work on it. As an initial matter, the documentation is where we
advertise new features, so I think you ought to take it on faith that
this will be well-advertised, unless you think that I'm completely
hopeless at writing documentation or something.
On the actual issue, I think that one key question is who should
control what happens when a role is created. Is that the superuser's
decision, or the CREATEROLE user's decision? I think it's better for
it to be the superuser's decision. Consider first the use case where
you want to set up a user who "feels like a superuser" i.e. inherits
the privileges of users they create. You don't want them to have to
specify anything when they create a role for that to happen. You just
want it to happen. So you want to set up their account so that it will
happen automatically, not throw the complexity back on them. In the
reverse scenario where you don't want the privileges inherited, I
think it's a little less clear, possibly just because I haven't
thought about that scenario as much, but I think it's very reasonable
here too to want the superuser to set up a configuration for the
CREATEROLE user that does what the superuser wants, rather than what
the CREATEROLE user wants.
Even aside from the question of who controls what, I think it is far
better from a usability perspective to have ways of setting up good
defaults. That is why we have the default_tablespace GUC, for example.
We could have made the CREATE TABLE command always use the database's
default tablespace, or we could have made it always use the main
tablespace. Then it would not be dependent on (poorly advertised?)
settings elsewhere. But it would also be really inconvenient, because
if someone is creating a lot of tables and wants them all to end up in
the same place, they don't want to have to specify the name of that
tablespace each time. They want to set a default and have that get
used by each command.
There's another, subtler consideration here, too. Since
ce6b672e4455820a0348214be0da1a024c3f619f, there are constraints on who
can validly be recorded as the grantor of a particular role grant,
just as we have always done for other types of grants. The grants have
to form a tree, with each grant having a grantor that was themselves
granted ADMIN OPTION by someone else, until eventually you get back to
the bootstrap superuser who is the source of all privileges. Thus,
today, when a CREATEROLE user grants membership in a role, the grantor
is recorded as the bootstrap superuser, because they might not
actually possess ADMIN OPTION on the role at all, and so we can't
necessarily record them as the grantor. But this patch changes that,
which I think is a significant improvement. The implicit grant that is
created when CREATE ROLE is issued has the bootstrap superuser as
grantor, because there is no other legal option, but then any further
grants performed by the CREATE ROLE user rely on that user having that
grant, and thus record the OID of the CREATEROLE user as the grantor,
not the bootstrap superuser.
That, in turn, has a number of rather nice consequences. It means in
particular that the CREATEROLE user can't remove the implicit grant,
nor can they alter it. They are, after all, not the grantor, who is
the bootstrap superuser, nor do they any longer have the authority to
act as the bootstrap superuser. Thus, if you have two or more
CREATEROLE users running around doing stuff, you can tell who did
what. Every role that those users created is linked back to the
creating role in a way that the creator can't alter. A CREATEROLE user
is unable to contrive a situation where they no longer control a role
that they created. That also means that the superuser, if desired, can
revoke all membership grants performed by any particular CREATEROLE
user by revoking the implicit grants with CASCADE.
But since this implicit grant has, and must have, the bootstrap
superuser as grantor, it is also only reasonable that superusers get
to determine what options are used when creating that grant, rather
than leaving that up to the CREATEROLE user.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Nov 23, 2022, at 9:01 AM, Robert Haas <robertmhaas@gmail.com> wrote:
That's not to say that I wouldn't rather that it always work one way or always the other. It's just to say that I don't want it to work differently based on some poorly advertised property of the role executing the command.
That seems rather pejorative. If we stipulate from the outset that the
property is poorly advertised, then obviously anything at all
depending on it is not going to seem like a very good idea. But why
should we assume that it will be poorly-advertised? I clearly said
that the documentation needs a bunch of work, and that I planned to
work on it. As an initial matter, the documentation is where we
advertise new features, so I think you ought to take it on faith that
this will be well-advertised, unless you think that I'm completely
hopeless at writing documentation or something.
Oh, I don't mean that it will be poorly documented. I mean that the way the command is written won't advertise what it is going to do. That's concerning if you fat-finger a CREATE ROLE command, then realize you need to drop and recreate the role, only to discover that a property you weren't thinking about, and which you are accustomed to being set the opposite way, is set such that you can't drop the role you just created. I think if you're going to create-and-disown something, you should have to say so, to make sure you mean it.
This consideration differs from the default schema or default tablespace settings. If I fat-finger the creation of a table, regardless of where it gets placed, I'm still the owner of the table, and I can still drop and recreate the table to fix my mistake.
Why not make this be a permissions issue, rather than a default behavior issue? Instead of a single CREATEROLE privilege, grant roles privileges to CREATE-WITH-INHERIT, CREATE-WITH-ADMIN, CREATE-SANS-INHERIT, CREATE-SANS-ADMIN, and thereby limit which forms of the command they may execute. That way, the semantics of the command do not depend on some property external to the command. Users (and older scripts) will expect the traditional syntax to behave consistent with how CREATE ROLE has worked in the past. The behaviors can be specified explicitly.
On the actual issue, I think that one key question is who should
control what happens when a role is created. Is that the superuser's
decision, or the CREATEROLE user's decision? I think it's better for
it to be the superuser's decision. Consider first the use case where
you want to set up a user who "feels like a superuser" i.e. inherits
the privileges of users they create. You don't want them to have to
specify anything when they create a role for that to happen. You just
want it to happen. So you want to set up their account so that it will
happen automatically, not throw the complexity back on them. In the
reverse scenario where you don't want the privileges inherited, I
think it's a little less clear, possibly just because I haven't
thought about that scenario as much, but I think it's very reasonable
here too to want the superuser to set up a configuration for the
CREATEROLE user that does what the superuser wants, rather than what
the CREATEROLE user wants.Even aside from the question of who controls what, I think it is far
better from a usability perspective to have ways of setting up good
defaults. That is why we have the default_tablespace GUC, for example.
We could have made the CREATE TABLE command always use the database's
default tablespace, or we could have made it always use the main
tablespace. Then it would not be dependent on (poorly advertised?)
settings elsewhere. But it would also be really inconvenient, because
if someone is creating a lot of tables and wants them all to end up in
the same place, they don't want to have to specify the name of that
tablespace each time. They want to set a default and have that get
used by each command.There's another, subtler consideration here, too. Since
ce6b672e4455820a0348214be0da1a024c3f619f, there are constraints on who
can validly be recorded as the grantor of a particular role grant,
just as we have always done for other types of grants. The grants have
to form a tree, with each grant having a grantor that was themselves
granted ADMIN OPTION by someone else, until eventually you get back to
the bootstrap superuser who is the source of all privileges. Thus,
today, when a CREATEROLE user grants membership in a role, the grantor
is recorded as the bootstrap superuser, because they might not
actually possess ADMIN OPTION on the role at all, and so we can't
necessarily record them as the grantor. But this patch changes that,
which I think is a significant improvement. The implicit grant that is
created when CREATE ROLE is issued has the bootstrap superuser as
grantor, because there is no other legal option, but then any further
grants performed by the CREATE ROLE user rely on that user having that
grant, and thus record the OID of the CREATEROLE user as the grantor,
not the bootstrap superuser.That, in turn, has a number of rather nice consequences. It means in
particular that the CREATEROLE user can't remove the implicit grant,
nor can they alter it. They are, after all, not the grantor, who is
the bootstrap superuser, nor do they any longer have the authority to
act as the bootstrap superuser. Thus, if you have two or more
CREATEROLE users running around doing stuff, you can tell who did
what. Every role that those users created is linked back to the
creating role in a way that the creator can't alter. A CREATEROLE user
is unable to contrive a situation where they no longer control a role
that they created. That also means that the superuser, if desired, can
revoke all membership grants performed by any particular CREATEROLE
user by revoking the implicit grants with CASCADE.But since this implicit grant has, and must have, the bootstrap
superuser as grantor, it is also only reasonable that superusers get
to determine what options are used when creating that grant, rather
than leaving that up to the CREATEROLE user.
Yes, this all makes sense, but does it entail that the CREATE ROLE command must behave differently on the basis of a setting?
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Wed, Nov 23, 2022 at 12:36 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
Oh, I don't mean that it will be poorly documented. I mean that the way the command is written won't advertise what it is going to do. That's concerning if you fat-finger a CREATE ROLE command, then realize you need to drop and recreate the role, only to discover that a property you weren't thinking about, and which you are accustomed to being set the opposite way, is set such that you can't drop the role you just created.
That doesn't ever happen. No matter how the properties are set, you
end up with ADMIN OPTION on the newly-created role and can drop it.
The flags control things like whether you can select from the newly
created role's tables even if you otherwise lack permissions on them
(INHERIT), and whether you can SET ROLE to it (SET). You can always
administer it, i.e. grant rights on it to others, change its password,
drop it.
I think if you're going to create-and-disown something, you should have to say so, to make sure you mean it.
Reasonable, but not relevant, since that isn't what's happening.
Why not make this be a permissions issue, rather than a default behavior issue? Instead of a single CREATEROLE privilege, grant roles privileges to CREATE-WITH-INHERIT, CREATE-WITH-ADMIN, CREATE-SANS-INHERIT, CREATE-SANS-ADMIN, and thereby limit which forms of the command they may execute. That way, the semantics of the command do not depend on some property external to the command. Users (and older scripts) will expect the traditional syntax to behave consistent with how CREATE ROLE has worked in the past. The behaviors can be specified explicitly.
Perhaps if we get the confusion above cleared up you won't be as
concerned about this, but let me just say that this patch is
absolutely breaking backward compatibility. I don't feel bad about
that, either. I think it's a good thing in this case, because the
current behavior is abjectly broken and horrible. What we've been
doing for the last several years is shipping a product that has a
built-in exploit that a clever 10-year-old could use to escalate
privileges from CREATEROLE to SUPERUSER. We should not be OK with
that, and we should be OK with changing the behavior however much is
required to fix it. I'm personally of the opinion that this patch set
does a rather clever job minimizing that blast radius, but that might
be my own bias as the patch author. Regardless, I don't think there's
any reasonable argument for maintaining the current behavior. I don't
entirely follow exactly what you have in mind in the sentence above,
but if it involves keeping the current CREATEROLE behavior around in
any form, -1 from me.
But since this implicit grant has, and must have, the bootstrap
superuser as grantor, it is also only reasonable that superusers get
to determine what options are used when creating that grant, rather
than leaving that up to the CREATEROLE user.Yes, this all makes sense, but does it entail that the CREATE ROLE command must behave differently on the basis of a setting?
Well, we certainly don't HAVE to add those new role-level properties;
that's why they are in a separate patch. But I think they add a lot of
useful functionality for a pretty minimal amount of extra code
complexity.
--
Robert Haas
EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Nov 23, 2022 at 12:36 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:Yes, this all makes sense, but does it entail that the CREATE ROLE command must behave differently on the basis of a setting?
Well, we certainly don't HAVE to add those new role-level properties;
that's why they are in a separate patch. But I think they add a lot of
useful functionality for a pretty minimal amount of extra code
complexity.
I haven't thought about these issues hard enough to form an overall
opinion (though I agree that making CREATEROLE less tantamount
to superuser would be an improvement). However, I share Mark's
discomfort about making these commands act differently depending on
context. We learned long ago that allowing GUCs to affect query
semantics was a bad idea. Basing query semantics on properties
of the issuing role (beyond success-or-permissions-failure) doesn't
seem a whole lot different from that. It still means that
applications can't just issue command X and expect Y to happen;
they have to inquire about context in order to find out that Z might
happen instead. That's bad in any case, but it seems especially bad
for security-critical behaviors.
regards, tom lane
On Wed, Nov 23, 2022 at 1:11 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
I haven't thought about these issues hard enough to form an overall
opinion (though I agree that making CREATEROLE less tantamount
to superuser would be an improvement). However, I share Mark's
discomfort about making these commands act differently depending on
context. We learned long ago that allowing GUCs to affect query
semantics was a bad idea. Basing query semantics on properties
of the issuing role (beyond success-or-permissions-failure) doesn't
seem a whole lot different from that. It still means that
applications can't just issue command X and expect Y to happen;
they have to inquire about context in order to find out that Z might
happen instead. That's bad in any case, but it seems especially bad
for security-critical behaviors.
I'm not sure that this behavior qualifies as security-critical. If
INHERITCREATEDROLES and SETCREATEDROLES are both true, then the grant
has INHERIT TRUE and SET TRUE and there are no more rights to be
gained. If not, the createrole user can do something like GRANT
new_role TO my_own_account WITH INHERIT TRUE, SET TRUE. Even if we
somehow disallowed that, they could gain access to the privilege of
the created role in a bunch of other ways, such as granting the rights
to someone else, or just changing the password and using the new
password to log into the account.
When I started working in this area, I thought non-inherited grants
were pretty useless, because you can so easily work around it.
However, other people did not agree. From what I can gather, I think
the reason why people like non-inherited grants is that they prevent
mistakes. A user who has ADMIN OPTION on another role but does not
inherit its privileges can break into that account and do whatever
they want, but they cannot ACCIDENTALLY perform an operation that
makes use of that user's privileges. They will have to SET ROLE, or
GRANT themselves something, and those actions can be logged and
audited if desired. Because of the potential for that sort of logging
and auditing, you can certainly make an argument that this is a
security-critical behavior, but it's not that clear cut, because it's
making assumptions about the behavior of other software, and of human
beings. Strictly speaking, looking just at PostgreSQL, these options
don't affect security.
On the more general question of configurability, I tend to agree that
it's not great to have the behavior of commands depend too much on
context, especially for security-critical things. A particularly toxic
example IMHO is search_path, which I think is an absolute disaster in
terms of security that I believe we will never be able to fully fix.
Yet there are plenty of examples of configurability that no one finds
problematic. No one agitates against the idea that a database can have
a default tablespace, or that you can ALTER USER or ALTER DATABASE to
configure an setting on a user-specific or database-specific setting,
even a security-critical one like search_path, or one that affects
query behavior like work_mem. No one is outraged that a data type has
a default btree operator class that is used for indexes unless you
specify another one explicitly. What people mostly complain about IME
is stuff like standard_conforming_strings, or bytea_output, or
datestyle. Often, when proposal come up on pgsql-hackers and get shot
down on these grounds, the issue is that they would essentially make
it impossible to write SQL that will run portably on PostgreSQL.
Instead, you'd need to write your application to issue different SQL
depending on the value of settings on the local system. That's un-fun
at best, and impossible at worst, as in the case of extension scripts,
whose content has to be static when you ship the thing.
But it's not exactly clear to me what the broader organizing principle
is here, or ought to be. I think it would be ridiculous to propose --
and I assume that you are not proposing -- that no command should have
behavior that in any way depends on what SQL commands have been
executed previously. Taken to a silly extreme, that would imply that
CREATE TABLE ought to be removed, because the behavior of SELECT *
FROM something will otherwise depend on whether someone has previously
issued CREATE TABLE something. Obviously that is a stupid argument.
But on the other hand, it would also be ridiculous to propose the
reverse, that it's fine to add arbitrary settings that affect the
behavior of any command whatsoever in arbitrary ways. Simon's proposal
to add a GUC that would make vacuum request a background vacuum rather
than performing one in the foreground is a good example of a proposal
that did not sit well with either of us.
But I don't know on what basis exactly we put a proposal like this in
one category rather than the other. I'm not sure I can really
articulate the general principle in a sensible way. For me, this
clearly falls into the "good" category: it's configuration that you
put into the database that makes things happen the way you want, not a
behavior-changing setting that comes along and ruins somebody's day.
But if someone else feels otherwise, I'm not sure I can defend that
view in a really rigorous way, because I'm not really sure what the
litmus test is, or should be. I think the best that I can do is to say
that if we *don't* add those options but *do* adopt the rest of the
patch set, we will have to make a decision about what behavior
everyone is going to get, and no matter what we decide, some people
are not going to be really unhappy with the result. I would like to
find a way to avoid that.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Nov 23, 2022, at 11:02 AM, Robert Haas <robertmhaas@gmail.com> wrote:
For me, this
clearly falls into the "good" category: it's configuration that you
put into the database that makes things happen the way you want, not a
behavior-changing setting that comes along and ruins somebody's day.
I had incorrectly imagined that if the bootstrap superuser granted CREATEROLE to Alice with particular settings, those settings would limit the things that Alice could do when creating role Bob, specifically limiting how much she could administer/inherit/set role Bob thereafter. Apparently, your proposal only configures what happens by default, and Alice can work around that if she wants to. But if that's the case, did I misunderstand upthread that these are properties the superuser specifies about Alice? Can Alice just set these properties about herself, so she gets the behavior she wants? I'm confused now about who controls these settings.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Wed, Nov 23, 2022 at 2:28 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
On Nov 23, 2022, at 11:02 AM, Robert Haas <robertmhaas@gmail.com> wrote:
For me, this
clearly falls into the "good" category: it's configuration that you
put into the database that makes things happen the way you want, not a
behavior-changing setting that comes along and ruins somebody's day.I had incorrectly imagined that if the bootstrap superuser granted CREATEROLE to Alice with particular settings, those settings would limit the things that Alice could do when creating role Bob, specifically limiting how much she could administer/inherit/set role Bob thereafter. Apparently, your proposal only configures what happens by default, and Alice can work around that if she wants to.
Right.
But if that's the case, did I misunderstand upthread that these are properties the superuser specifies about Alice? Can Alice just set these properties about herself, so she gets the behavior she wants? I'm confused now about who controls these settings.
Because they are role-level properties, they can be set by whoever has
ADMIN OPTION on the role. That always includes every superuser, and it
never includes Alice herself (except if she's a superuser). It could
include other users depending on the system configuration. For
example, under this proposal, the superuser might create alice and set
her account to CREATEROLE, configuring the INHERITCREATEDROLES and
SETCREATEDROLES properties on Alice's account according to preference.
Then, alice might create another user, say bob, and make him
CREATEROLE as well. In such a case, either the superuser or alice
could set these properties for role bob, because alice enjoys ADMIN
OPTION on role bob.
Somewhat to one side of the question you were asking, but related to
the above, I believe there is an opportunity, and perhaps a need, to
modify the scope of CREATEROLE in terms of what role-level options a
CREATEROLE user can set. For instance, if a CREATEROLE user doesn't
have CREATEDB, they can still create users and give them that
privilege, even with these patches, and likewise these two new
properties. This patch is only concerned about which roles you can
manipulate, not what role-level properties you can set. Somebody might
feel that's a serious problem, and they might even feel that this
patch set ought to something about it. In my view, the issues are
somewhat severable. I don't think you can do anything as evil by
setting role-level properties (except for SUPERUSER, of course) as
what you can do by granting predefined roles, so I don't find
restricting those capabilities to be as urgent as doing something to
restrict role grants.
Also, and this definitely plays into it too, I think there's some
debate about what the model ought to look like there. For instance,
you could simply stipulate that you can't give what you don't have,
but that would mean that every CREATEROLE user can create additional
CREATEROLE users, and I suspect some people might like to restrict
that. We could add a new CREATECREATEROLE property to decide whether a
user can make CREATEROLE users, but by that argument we'd also need a
new CREATECREATECREATEROLE property to decide whether a role can make
CREATECREATEROLE users, and then it just recurses indefinitely from
there. Similarly for CREATEDB. Also, what if anything should you
restrict about how the new INHERITCREATEDROLES and SETCREATEDROLES
properties should be set? You could argue that they ought to be
superuser-only (because the implicit grant is performed by the
bootstrap superuser) or that it's fine for them to be set by a
CREATEROLE user with ADMIN OPTION (because it's not all that
security-critical how they get set) or maybe even that a user ought to
be able to set those properties on his or her own role.
I'm not very certain about any of that stuff; I don't have a clear
mental model of how it should work, or even what exact problem we're
trying to solve. To me, the patches that I posted make sense as far as
they go, but I'm not under the illusion that they solve all the
problems in this area, or even that I understand what all of the
problems are.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Nov 23, 2022, at 12:04 PM, Robert Haas <robertmhaas@gmail.com> wrote:
But if that's the case, did I misunderstand upthread that these are properties the superuser specifies about Alice? Can Alice just set these properties about herself, so she gets the behavior she wants? I'm confused now about who controls these settings.
Because they are role-level properties, they can be set by whoever has
ADMIN OPTION on the role. That always includes every superuser, and it
never includes Alice herself (except if she's a superuser). It could
include other users depending on the system configuration. For
example, under this proposal, the superuser might create alice and set
her account to CREATEROLE, configuring the INHERITCREATEDROLES and
SETCREATEDROLES properties on Alice's account according to preference.
Then, alice might create another user, say bob, and make him
CREATEROLE as well. In such a case, either the superuser or alice
could set these properties for role bob, because alice enjoys ADMIN
OPTION on role bob.
Ok, so the critical part of this proposal is that auditing tools can tell when Alice circumvents these settings. Without that bit, the whole thing is inane. Why make Alice jump through hoops that you are explicitly allowing her to jump through? Apparently the answer is that you can point a high speed camera at the hoops.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
