Re: Delegating superuser tasks to new security roles (Was: Granting control of SUSET gucs to non-superusers)

Greetings,

* Mark Dilger (mark(dot)dilger(at)enterprisedb(dot)com) wrote:
> > On Aug 23, 2021, at 11:13 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > This I have to object to pretty strongly- we have got to get away from
> > the idea that just because X isn't a superuser or isn't owned by a
> > superuser that it's fine to allow some non-superuser to mess with it.
>
> I thought we were trying to create a set of roles which could cumulatively do everything *inside the sandbox* that superuser can do, but which cannot escape the sandbox. I would think this pg_manage_database_objects role would be reasonable in the context of that effort.

I wasn't objecting to the general concept of trying to have a role that
can do lots of things inside the sandbox but aren't allowed to escape
it. I was specifically objecting to the idea that just checking if an
object is directly owned by a superuser is not sufficient to prevent a
role from being able to escape the sandbox.

> > In particlar, just because a role isn't explicitly marked as a superuser
> > doesn't mean that the role can't *become* a superuser, or that it hasn't
> > got privileged access to the system in other ways, such as by being a
> > member of other predefined roles that perhaps the role who is a member
> > of pg_manage_database_objects doesn't have.
>
> The implementation does not allow pg_manage_database_objects to mess with objects that are owned by a role which satisfies superuser_arg(). If you are renting out a database to a tenant and change the ownership of stuff to a non-superuser, then you get what you get. But why would you do that?

Simply using superuser_arg() isn't sufficient is exactly the point that
I'm trying to make. As a 'landlord', I might very well want to have
some kind of 'landlord' role that isn't directly a superuser but which
could *become* a superuser by having been GRANT'd a superuser role- but
I certainly don't want that role's objects to be able to be messed with
by the tenant.

> > Such a check against
> > modifying of "superuser owned" objects implies that it's providing some
> > kind of protection against the role being able to become a superuser
> > when it doesn't actually provide that protection in any kind of reliable
> > fashion and instead ends up fooling the user.
>
> Please provide steps to reproduce this issue. Assume that a database is initialized and that everything is owned by the system. A "tenant" role is created and granted pg_manage_database_objects, and other non-superuser roles are created. Now, what exactly can "tenant" do that you find objectionable?

If one of those other non-superuser roles is, itself, a role that can
become a superuser and it decides to create some functions for its own
purposes, then the tenant role would be able to modify those functions,
allowing the tenant to gain access to the non-superuser role, and from
there being able to gain access to superuser.

Something along these lines, basically:

CREATE USER tenant;
GRANT pg_manage_database_objects TO tenant;
CREATE USER landlord;
GRANT postgres TO landlord;
SET ROLE landlord;
CREATE FUNCTION do_stuff();
put call to do_stuff() into a cronjob
SET ROLE tenant;
CREATE OR REPLACE do_stuff(); -- with code to take over landlord

poof- tenant has ability to be landlord and then further to become
postgres.

All of the above applies beyond just superuser too- consider a
non-superuser role which has been grant'd pg_execute_server_program.
That won't trip up superuser_arg() but it sure would allow a role to
break out of the sandbox.

> > This is the issue with CREATEROLE and we definitely shouldn't be
> > doubling-down on that mistake, and also brings up the point that I, at
> > least, had certainly hoped that part of this effort would include a way
> > for roles to be created by a user with an appropriate predefined role,
> > and w/o CREATEROLE (which would then be deprecated or, ideally, just
> > outright removed).
>
> Well, pg_manage_database_objects has no special ability to create or drop roles. I thought separating those powers made more sense than grouping them together. We can have a new role for doing what you say, but that seems redundant with CREATEROLE. I didn't want this patch set to be bogged down waiting for a consensus on how to change the CREATEROLE privilege.

CREATEROLE doesn't work to give to folks generally because of the issues
above- its check is, similarly, too simple and always has been. This
isn't news either, it's been discussed in various places from time to
time and is part of why people who run cloud providers end up either not
giving out that role attribute and providing another way, or they hack
up the PG core code to handle the way that attribute works differently.

> > I get that this doesn't have to be in the first
> > patch or even patch set going down this road but the lack of discussion
> > or of any coordination between this effort and the other one that is
> > trying to address the CREATEROLE issue seems likely to land us in a bad
> > place with two distinct approaches being used.
>
> I'm confused. This patch set doesn't come within a country mile of CREATEROLE. Why should this patch set have to coordinate with that one? I'm not arguing with you -- merely asking what I'm misunderstanding?

Perhaps it's just because I'm looking at the exact same issues cropping
up here that do with the CREATEROLE situation and I'd really like to
find a solution that gets us away from putting out a half-solution that
won't actually be directly usable by the folks who care about making
sure people don't break out of the sandbox because of the issues
outlined above.

Thanks,

Stephen