Thread: [PATCH] Simplify permission checking logic in user.c
Hey, hackers, As part of building Postgres for a managed environment in Google Cloud SQL, we've made various small changes to permission checks to carefully limit what customers have access to. We were making some changes in src/backend/commands/user.c and noticed some clunky logic related to verifying that the current user has sufficient permissions to perform an action: https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/commands/user.c;h=0e6800bf3e4f3f584bbb0b2f6f8ae999f5d94bf1;hb=HEAD#l288 The checks for whether the current user can create a user with the SUPERUSER, REPLICATION, or BYPASSRLS attributes are chained together using if/else-if, before finally checking whether the user has CREATEROLE privileges in a final else. This construction is error-prone, since once one branch is visited, later ones are skipped, and it implicitly assumes that the permissions needed for each subsequent action are subsets of the permissions needed for the previous action. Since each branch requires SUPERUSER this is fine, but changing one of the checks could inadvertently allow users without the CREATEROLE permission to create users. A similar construction is used later in the file in the AlterRole function: https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/commands/user.c;h=0e6800bf3e4f3f584bbb0b2f6f8ae999f5d94bf1;hb=HEAD#l717 This small patch simply modifies the code to replace the if/else-if chain with separate if statements, and always checks whether the user has the CREATEROLE privilege. (The have_createrole_privilege function includes another superuser check.) Given the current checks in each branch, this code is equivalent, but easier to modify in the future. - Paul
Attachment
On Tue, Dec 29, 2020 at 02:26:19PM -0600, Paul Martinez wrote:
> The checks for whether the current user can create a user with the SUPERUSER,
> REPLICATION, or BYPASSRLS attributes are chained together using if/else-if,
> before finally checking whether the user has CREATEROLE privileges in a
> final else. This construction is error-prone, since once one branch is visited,
> later ones are skipped, and it implicitly assumes that the permissions needed
> for each subsequent action are subsets of the permissions needed for the
> previous action. Since each branch requires SUPERUSER this is fine, but
> changing one of the checks could inadvertently allow users without the
> CREATEROLE permission to create users.
Hmm. I agree with your position here. A careless change could easily
miss that all those if branches have to use a superuser check.
+ if (isreplication)
{
if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to create replication users")));
}
}
It would be more readable to combine both conditions together, no?
Your change also means that we may call superuser() more times than
necessary, but last_roleid_is_super leverages that, and this code path
is not performance-critical either.
I am adding Stephen in CC, just in case he has some comments to
share.
--
Michael
Attachment
Greetings, * Paul Martinez (paulmtz@google.com) wrote: > This small patch simply modifies the code to replace the if/else-if chain with > separate if statements, and always checks whether the user has the CREATEROLE > privilege. (The have_createrole_privilege function includes another superuser > check.) Given the current checks in each branch, this code is equivalent, but > easier to modify in the future. While I do appreciate that it'd be nice if we made all of the code in the backend easier for folks to maintain their own patch sets against core, it'd also mean that we're now being asked to provide some level of commitment that we aren't going to change these things later because then we'd break $whomever's patches. That's certainly not something that is really reasonable for us to agree to. I'd strongly suggest that, instead, you consider proposing changes which would address the actual use cases you have and work with the community to have those included in core, which would further have the added property that everyone would then benefit from those improvements. Thanks, Stephen
Attachment
Greetings, * Michael Paquier (michael@paquier.xyz) wrote: > On Tue, Dec 29, 2020 at 02:26:19PM -0600, Paul Martinez wrote: > > The checks for whether the current user can create a user with the SUPERUSER, > > REPLICATION, or BYPASSRLS attributes are chained together using if/else-if, > > before finally checking whether the user has CREATEROLE privileges in a > > final else. This construction is error-prone, since once one branch is visited, > > later ones are skipped, and it implicitly assumes that the permissions needed > > for each subsequent action are subsets of the permissions needed for the > > previous action. Since each branch requires SUPERUSER this is fine, but > > changing one of the checks could inadvertently allow users without the > > CREATEROLE permission to create users. > > Hmm. I agree with your position here. A careless change could easily > miss that all those if branches have to use a superuser check. That really strikes me as pretty darn unlikely to happen, it's not like this code is really spread out across very many lines or that it's hard to see the pretty clear pattern. In thinking about these role attributes and the restrictions we have about what attributes don't require superuser to set and what do, I do feel like we're probably missing a bet regarding replication and bypassrls.. In other words, I suspect people would be happier if we provided a way for non-superusers a way to create replication roles and bypassrls roles. Exactly how we do that is a bit tricky to figure out but that's certainly a much more productive direction to be going in. Thanks, Stephen
Attachment
> 30 дек. 2020 г., в 20:26, Stephen Frost <sfrost@snowman.net> написал(а): > > I'd strongly suggest that, instead, you consider proposing changes which > would address the actual use cases you have and work with the community > to have those included in core, which would further have the added > property that everyone would then benefit from those improvements. +1. Last time we asked to change something in privileges[0], we got a feedback pointing to possible vulnerability. We fixed it in our services and reported to, AFAIR, RDS and Aiven (with PoC exploits). I think that sharing "various small changes to permission checks" is a really good idea. > 30 дек. 2020 г., в 20:39, Stephen Frost <sfrost@snowman.net> написал(а): > In other words, I suspect people would be happier if we > provided a way for non-superusers a way to create replication roles and > bypassrls roles. +1 again. I hope we will return to the topic soon. Best regards, Andrey Borodin. [0] https://www.postgresql.org/message-id/flat/1269681541151271%40myt5-68ad52a76c91.qloud-c.yandex.net
On Wed, Dec 30, 2020 at 12:28 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> I think that sharing "various small changes to permission checks" is a really good idea.
>
> > 30 дек. 2020 г., в 20:39, Stephen Frost <sfrost@snowman.net> написал(а):
> > In other words, I suspect people would be happier if we
> > provided a way for non-superusers a way to create replication roles and
> > bypassrls roles.
> +1 again. I hope we will return to the topic soon.
On Wed, Dec 30, 2020 at 9:26 AM Stephen Frost <sfrost@snowman.net> wrote:
> While I do appreciate that it'd be nice if we made all of the code in
> the backend easier for folks to maintain their own patch sets against
> core, it'd also mean that we're now being asked to provide some level of
> commitment that we aren't going to change these things later because
> then we'd break $whomever's patches. That's certainly not something
> that is really reasonable for us to agree to.
>
> I'd strongly suggest that, instead, you consider proposing changes which
> would address the actual use cases you have and work with the community
> to have those included in core, which would further have the added
> property that everyone would then benefit from those improvements.
On Wed, Dec 30, 2020 at 9:39 AM Stephen Frost <sfrost@snowman.net> wrote:
>
> That really strikes me as pretty darn unlikely to happen, it's not like
> this code is really spread out across very many lines or that it's hard
> to see the pretty clear pattern.
>
> In thinking about these role attributes and the restrictions we have
> about what attributes don't require superuser to set and what do, I do
> feel like we're probably missing a bet regarding replication and
> bypassrls.. In other words, I suspect people would be happier if we
> provided a way for non-superusers a way to create replication roles and
> bypassrls roles. Exactly how we do that is a bit tricky to figure out
> but that's certainly a much more productive direction to be going in.
Thanks everyone for taking a look!
You've identified exactly the problem we're running into -- we want to
allow customers, who aren't superusers, to create replication roles.
In Google Cloud SQL we create a role for customers, cloudsqlsuperuser,
which, confusingly, is not a SUPERUSER, but does have some extra
permissions. We've modified a lot of "if (!superuser())" checks to
"if (!superuser() && !cloudsqlsuperuser())".
That was actually what I initially tried to do when modifying this bit of
code:
if (issuperuser)
if (!superuser()) ereport(...);
elsif (isreplication)
- if (!superuser()) ereport(...);
+ if (!superuser() && !cloudsqlsuperuser()) ereport(...);
elsif (bypassrls)
if (!superuser()) ereport(...);
else
if (!have_createrole_privilege()) ereport()
But this inadvertently allowed cloudsqlsuperuser to _also_ create users
with the BYPASSRLS attribute by creating a user with both REPLICATION
and BYPASSRLS, which I only realized when a couple of tests failed.
Properly modifying the required permissions required separating the
if/else branches, which led to this patch.
From my understanding, AWS RDS Postgres takes a slightly different approach
and allows the REPLICATION attribute to be inherited through membership in
a special rds_replication role.
We haven't seriously considered what some sort of general functionality
would look like to support our managed Postgres use-cases, but here's a
rough sketch of what we're trying to accomplish with roles and permissions:
- We, ideally, want to give our customers access to as much of Postgres'
functionality as possible, including SUPERUSER capabilities
- But we do not want customers to have access to the underlying VM or
filesystem
- There are certain parts of the system (i.e., certain databases, tables,
individual rows in catalog tables, etc.) that we want to manage and we
can't allow customers to modify these at all. Examples include:
- Our own SUPERUSER role that we use to connect to the cluster; customers
shouldn't be able to modify this role at all
- Replication slots used for managed replication shouldn't be able to be
modified by customers (even if they have the REPLICATION attribute)
- We want to prevent customers from depending on any data we choose to store
in other database, as that limits our flexibility to make future changes
- Notably this means we only can support logical replication, and not
physical replication.
I suppose this could be roughly summarized as "90% of a superuser, but also
still obeys SQL privileges for objects created by someone else".
We would definitely be happy to explore what sort of functionality like this
would be useful for others!
Greetings, * Paul Martinez (paulmtz@google.com) wrote: > You've identified exactly the problem we're running into -- we want to > allow customers, who aren't superusers, to create replication roles. This is also where it's probably useful to think about what the impact of that is- after all, since it seems you're considering this for the purposes of logical replication, it'd make sense to draw your attention to this: https://www.postgresql.org/docs/10/logical-replication-security.html which talks about the rather serious consequences, from a security perspective, of allowing non-superusers access to logical replication and modifying publication and subscription sets and tables. Perhaps these are things you've already addressed, but we'd need to address them in core to have such a capability make sense. > We haven't seriously considered what some sort of general functionality > would look like to support our managed Postgres use-cases, but here's a > rough sketch of what we're trying to accomplish with roles and permissions: > > - We, ideally, want to give our customers access to as much of Postgres' > functionality as possible, including SUPERUSER capabilities > > - But we do not want customers to have access to the underlying VM or > filesystem Certainly, the above two things are in pretty direct conflict, as you've discovered. :) That said, we've started working in this direction with the initial (or default) role system, as a way to express capabilities which can then be inherited or GRANT'd by roles with appropriate rights to other roles. In general, it seems like that's a better approach and that it'd make sense to move away from the role attributes. > - There are certain parts of the system (i.e., certain databases, tables, > individual rows in catalog tables, etc.) that we want to manage and we > can't allow customers to modify these at all. Examples include: > - Our own SUPERUSER role that we use to connect to the cluster; customers > shouldn't be able to modify this role at all Roles, in general, can't modify other roles (except with CREATEROLE, which is more an argument to get rid of it and not use it than anything else, imv). > - Replication slots used for managed replication shouldn't be able to be > modified by customers (even if they have the REPLICATION attribute) That's an interesting example and one which probably means we need to have some kind of ownership/privilege model added to replication slots. > - We want to prevent customers from depending on any data we choose to store > in other database, as that limits our flexibility to make future changes > - Notably this means we only can support logical replication, and not > physical replication. > Being able to give users the ability to use logical replication for a subset of the system is certainly a capability that I think we'd like to have. > I suppose this could be roughly summarized as "90% of a superuser, but also > still obeys SQL privileges for objects created by someone else". I don't really think that it makes sense to think of it that way- superuser is superuser, everything is "user with certain privileges granted to it" and we should be thinking of it that way. Thanks, Stephen
Attachment
> 31 дек. 2020 г., в 00:37, Paul Martinez <paulmtz@google.com> написал(а): > > In Google Cloud SQL we create a role for customers, cloudsqlsuperuser, > which, confusingly, is not a SUPERUSER, but does have some extra > permissions. We've modified a lot of "if (!superuser())" checks to > "if (!superuser() && !cloudsqlsuperuser())". You use cloudsqlsuperuser. RDS has rds_superuser. Aiven has their aiven_extras extension. Yandex Cloud has mdb_admin and mdb_replication. Some of us, probably, could do something useful for the project instead of rebasing those patches and extensions. Let's start to work together with community. Let's address issues that thread[0] faced and restart it. Happy New Year :) Best regards, Andrey Borodin. [0] https://www.postgresql.org/message-id/flat/f41d93b6-69ba-fa05-c91a-045bafa5f832%402ndquadrant.com#636c800abc6f464c388db7f532a389ba