Re: has_privs_of_role vs. is_member_of_role, redux

Greetings,

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Thu, Sep 8, 2022 at 1:06 PM <walther(at)technowledgy(dot)de> wrote:
> > In theory, I could also inherit that privilege, but that's not how the
> > system works today. By using is_member_of_role, the decision was already
> > made that this should not depend on inheritance. What is left, is the
> > ability to do it via SET ROLE only.
>
> I do not accept the argument that we've already made the decision that
> this should not depend on inheritance. It's pretty clear that we
> haven't thought carefully enough about which checks should depend only
> on membership, and which ones should depend on inheritance. The patch
> I committed just now to fix ALTER DEFAULT PRIVILEGES is one clear
> example of where we've gotten that wrong. We also changed the way
> predefined roles worked with inheritance not too long ago, so that
> they started using has_privs_of_role() rather than
> is_member_of_role(). Our past thinking on this topic has been fuzzy
> enough that we can't really conclude that because something uses
> is_member_of_role() now that's what it should continue to do in the
> future. We are working to get from a messy situation where the rules
> aren't consistent or understandable to one where they are, and that
> may mean changing some things.

Agreed that we haven't been good about the distinction between these,
but that the recent work by Joshua and yourself has been moving us in
the right direction.

> One could take the view that the issue here is that
> pg_read_all_settings shouldn't have the right to create objects in the
> first place, and that this INHERIT vs. SET ROLE distinction is just a
> distraction. However, that would require accepting the idea that it's
> possible for a role to lack privileges granted to PUBLIC, which also
> sounds pretty unsatisfying. On the whole, I'm inclined to think it's
> reasonable to suppose that if you want to grant a role to someone
> without letting them create objects owned by that role, it should be a
> role that doesn't own any existing objects either. Essentially, that's
> legislating that predefined roles should be minimally privileged: they
> should hold the ability to do whatever it is that they are there to do
> (like read all settings) but not have any other privileges (like the
> ability to do stuff to objects they own).

Predefined roles are special in that they should GRANT just the
privileges that the role is described to GRANT and that users really
shouldn't be able to SET ROLE to them nor should they be allowed to own
objects, or at least that's my general feeling on them.

If an administrator doesn't wish for a user to have the privileges
provided by the predefined role by default, they should be able to set
that up by creating another role who has that privilege which the user
is able to SET ROLE to. That is:

CREATE ROLE admin WITH INHERIT FALSE;
GRANT pg_read_all_settings TO admin;
GRANT admin TO alice;

Would allow 'alice' to log in without the privileges associated with
pg_read_all_settings but 'alice' is able to SET ROLE admin; and gain
those privileges. It wasn't intended that 'alice' be able to SET ROLE
to pg_read_all_settings itself though.

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> Yeah, my statement before wasn't correct. It appears that alice can't
> just usurp the privileges of pg_read_all_settings trivially, but she
> can create a trigger on any preexisting table owned by
> pg_read_all_settings and then anyone who performs an operation that
> causes that trigger to fire is at risk:

Triggers aren't the only thing to be worried about in this area-
functions defined inside of views are also run with the privileges of
the user running the SELECT and not as the owner of the view. The same
is true of running SELECT against tables with RLS too, of course.
Generally speaking, it's always been very risky to access the objects of
users who you don't trust in any way and we don't currently provide any
particularly easy way to make that kind of access safe. RLS at least
provides an escape by allowing a user to turn it off, but the same isn't
available for setting a search_path and then running queries or
accessing views or running DML against tables with triggers.

> I'm slightly skeptical of that conclusion because the whole thing just
> feels a bit flimsy. Like, the whole idea that you can compromise your
> account by inserting a row into somebody else's table feels a little
> nuts to me. Triggers and row-level security policies make it easy to
> do things that look safe and are actually very dangerous. I think
> anyone would reasonably expect that calling a function owned by some
> other user might be risky, because who knows what that function might
> do, but it seems less obvious that accessing a table could execute
> arbitrary code, yet it can. And it is even less obvious that creating
> a table owned by one role might give some other role who inherits that
> user's privileges to booby-trap that table in a way that might fool a
> third user into doing something unsafe. But I have no idea what we
> could reasonably do to improve the situation.

Just to reiterate- this is not only about DML/triggers or RLS but also
includes SELECT statements against views and setting of the search_path
to that owned by someone trying to compromise your account (though the
latter does require a bit more than just the SET itself).

One approach to dealing with this would be to have a mechanism to define
exactly what code you feel comfortable running and set that to be only
the bootstrap superuser (or perhaps we'd have this as a superuser-set
GUC list) and the current role and then fail any queries that end up
calling code owned by any other role.

* Wolfgang Walther (walther(at)technowledgy(dot)de) wrote:
> Robert Haas:
> > This shows that if rhaas (or whoever) performs DML on a table owned by
> > pg_read_all_settings, he might trigger arbitrary code written by alice
> > to run under his own user ID. Now, that hazard would exist anyway for
> > tables owned by alice, but now it also exists for any tables owned by
> > pg_read_all_settings.
>
> This hazard exists for all tables that alice has been granted the TRIGGER
> privilege on. While we prevent alice from creating tables that are owned by
> pg_read_all_settings, we do not prevent inheriting the TRIGGER privilege.

The issue here is that we don't prevent alice from issuing a 'SET' to
pg_read_all_settings nor do we prevent predefined roles from creating
objects. I'd be inclined to change the system to explicitly prevent
both of those things from being allowed- for the special case of
predefined roles and not as some general role capability. Maybe there's
an argument that we should allow administrators to create roles that no
user is allowed to SET to or which aren't allowed to create/own objects
but I'm not sure that there's a strong use-case for that. I do think
it's useful to allow administrators to create roles that *some* users
are allowed to have the privileges are but aren't allowed to SET to, but
the whole point of predefined roles is to use the role GRANT system as a
more flexible way to give out certain distinct privileges to certain
roles and that's it.

> > I'm slightly skeptical of that conclusion because the whole thing just
> > feels a bit flimsy. Like, the whole idea that you can compromise your
> > account by inserting a row into somebody else's table feels a little
> > nuts to me. Triggers and row-level security policies make it easy to
> > do things that look safe and are actually very dangerous. I think
> > anyone would reasonably expect that calling a function owned by some
> > other user might be risky, because who knows what that function might
> > do, but it seems less obvious that accessing a table could execute
> > arbitrary code, yet it can. And it is even less obvious that creating
> > a table owned by one role might give some other role who inherits that
> > user's privileges to booby-trap that table in a way that might fool a
> > third user into doing something unsafe. But I have no idea what we
> > could reasonably do to improve the situation.
>
> Right. This will always be the case when giving out the TRIGGER privilege on
> one of your tables to somebody else.

Giving out of the TRIGGER privilege is really just a ill-conceived idea
that we should acknowledge only exists because it's part of the
standard.

> There is two kind of TRIGGER privileges: An explicitly GRANTed privilege and
> an implicit privilege, that is given to the table owner.

That's really only half-right: TRIGGER is just one of the privileges
that the owner has if there's no ACL on the table. If the ACL is
defined and the owner's entry has the TRIGGER privilege removed then
they'll lose the ability to create triggers on that table. Of course,
the owner can simply GRANT that ability back to themselves if they wish
but it's a useful distinction to be aware of.

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Mon, Sep 26, 2022 at 12:16 PM Wolfgang Walther
> <walther(at)technowledgy(dot)de> wrote:
> > I think, when WITH INHERIT TRUE, SET FALSE is set, we should:
> > - Inherit all explicitly granted privileges
> > - Not inherit any DDL privileges implicitly given through ownership:
> > CREATE, REFERENCES, TRIGGER.
> > - Inherit all other privileges implicitly given through ownership (DML +
> > others)
>
> I don't think we're going to be very happy if we redefine inheriting
> the privileges of another role to mean inheriting only some of them.
> That seems pretty counterintuitive to me. I also think that this
> particular definition is pretty fuzzy.

I agree with Robert on this part. Inheriting the privileges of another
role should generally mean exactly that and not some awkward subset of
the privileges.

> Your previous proposal was to make the SET attribute of a GRANT
> control not only the ability to SET ROLE to the target role but also
> the ability to create objects owned by that role and/or transfer
> objects to that role. I think some people might find that behavior a
> little bit surprising - certainly, it goes beyond what the name SET
> implies - but it is at least simple enough to explain in one sentence,
> and the consequences don't seem too difficult to reason about.

I still feel it's useful to allow users to transfer object to roles that
they can SET to even if they don't inherit the privileges of that role.
I don't feel that should be allowed for predefined roles, however.

One thing that I don't want to miss mentioning is that I'm not against
the idea of predefined roles having ownership of some objects- but
should that happen (tho I tend to doubt it will, because we usually use
the bootstrap superuser for objects that admins can use but shouldn't be
mucking around with and changing), those objects shouldn't be ones that
are able to be messed with by anyone except a superuser running around
with allow_system_table_mods or such.

Thanks,

Stephen