Re: has_privs_of_role vs. is_member_of_role, redux

On Wed, Sep 7, 2022 at 5:51 PM Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > To be more precise, I propose that in order for alice to create
> > objects owned by bob or to change one of her objects to be owned by
> > bob, she must not only be a member of role bob, but also inherit bob's
> > privileges. If she has the ability to SET ROLE bob but not does not
> > inherit his privileges, she can create new objects owned by bob only
> > if she first does SET ROLE bob, and she cannot reassign ownership of
> > her objects to bob at all.
>
> ... which means that to get a table owned by bob which is currently
> owned by alice, alice has to:
>
> set role bob;
> create table;
> grant insert on table to alice;
> reset role;
> insert into table select * from table;
>
> That's pretty sucky and is the case that had been contemplated at the
> time that was written to allow it (at least, if memory serves). iirc,
> that's also why we check the *bob* has CREATE rights in the place where
> this is happening (as otherwise the above process wouldn't work either).

Sure. I think it comes down to what you think that the system
administrator intended to block by not allowing alice to inherit bob's
permissions. In existing releases, there's no facility to prevent
alice from doing SET ROLE bob, so the system administrator can't have
intended this as a security measure. But the system administrator
might have intended that alice shouldn't do anything that relies on
bob's permissions by accident, else she should have SET ROLE. And in
that case the intention is defeated by allowing the operation. Now,
you may well have in mind some other intention that the system
administrator could have had where allowing alice to perform this
operation without needing to inherit bob's permissions is sensible;
I'm not trying to say there is no such case. I don't know what it is,
though.

My first reaction was in the same ballpark as yours: what's the big
deal? But as I think about it more, I struggle to reconcile that
instinct with any specific use case.

Fairly obviously, my thinking here is biased by having written the
patch to allow restricting SET ROLE. If alice can neither inherit
bob's privileges nor SET ROLE bob, she had better not be able to
create objects owned by bob, because otherwise she can make a table,
add an expression index that calls a user-defined function, do stuff
until it needs to be autovacuumed, and then give it to bob, and boom,
exploit. But that doesn't mean that the is_member_of_role() tests here
have to be changed to has_privs_of_role(). They could be changed to
has_privs_of_role() || member_can_set_role(). And if the consensus is
to do it that way, I'm OK with that.

I'm just a little unconvinced that it's actually the best route. I
think that logic of the form "well Alice could just SET ROLE and do it
anyway" is weak -- and not only because of the patch to allow
restricting SET ROLE, but because AFAICT there is no point to the
INHERIT option in the first place unless it is to force you to issue
SET ROLE. That is literally the only thing it does. If we're going to
have weird exceptions where you don't have to SET ROLE after all, why
even have INHERIT in the first place?

--
Robert Haas
EDB: http://www.enterprisedb.com