Horton

Thread

Commits

  1. Make ALTER DEFAULT PRIVILEGES require privileges, not membership.

  1. has_privs_of_role vs. is_member_of_role, redux

    Robert Haas <robertmhaas@gmail.com> — 2022-08-25T16:12:46Z

    Hi,

We've had some previous discussions about when to use
has_privs_of_role and when to use is_member_of_role, and
has_privs_of_role has mostly won the fight. That means that, if role
"robert" is set to NOINHERIT and you "GRANT stuff TO robert", for the
most part "robert" will not actually be able to do things that "stuff"
could do. Now, robert will be able TO "SET ROLE stuff" and then do all
and only those things that "stuff" can do, but he won't be able to do
those things as "robert". For example:

rhaas=# set role robert;
SET
rhaas=> select * from stuff_table;
ERROR:  permission denied for table stuff_table

So far, so good. But it's clearly not the case that "GRANT stuff TO
robert" has conferred no privileges at all on robert. At the very
least, it's enabled him to "SET ROLE stuff", but what else? I decided
to go through the code and make a list of the things that robert can
now do that he couldn't do before. Here it is:

1. robert can create new objects of various types owned by stuff:

rhaas=> create schema stuff_by_robert authorization stuff;
CREATE SCHEMA
rhaas=> create schema unrelated_by_robert authorization unrelated;
ERROR:  must be member of role "unrelated"

2. robert can change the owner of objects he owns to instead be owned by stuff:

rhaas=> alter table robert_table owner to unrelated;
ERROR:  must be member of role "unrelated"
rhaas=> alter table robert_table owner to stuff;
ALTER TABLE

3. robert can change the default privileges for stuff:

rhaas=> alter default privileges for role unrelated grant select on
tables to public;
ERROR:  must be member of role "unrelated"
rhaas=> alter default privileges for role stuff grant select on tables
to public;
ALTER DEFAULT PRIVILEGES

4. robert can execute "SET ROLE stuff".

That's it. There are two other behaviors that change -- the return
value of pg_has_role('robert', 'stuff', 'MEMBER') and pg_hba.conf
matching to groups -- but those aren't things that robert gains the
ability to do. The above is an exhaustive list of the things robert
gains the ability to do.

I argue that #3 is a clear bug. robert can't select from stuff's
tables or change privileges on stuff's objects, so why can he change
stuff's default privileges? is_member_of_role() has a note that it is
not to be used for privilege checking, and this seems like it's pretty
clearly a privilege check.

On the flip side, #4 is pretty clearly correct. Presumably, allowing
that to happen was the whole point of executing "GRANT stuff TO
robert" in the first place.

The other two are less clear, in my opinion. We don't want users to
end up owning objects that they didn't intend to own; in particular,
if any user could make a security-definer function and then gift it to
the superuser, it would be a disaster. So, arguably, the ability to
make some other role the owner of an object represents a privilege
that your role holds with respect to their role. Under that theory,
the is_member_of_role() checks that are performed in cases #1 and #2
are privilege checks, and we ought to be using has_privis_of_role()
instead, so that a non-inherited role grant doesn't confer those
privileges. But I don't find this very clear cut, because except when
the object you're gifting is a Trojan horse, giving stuff away helps
the recipient, not the donor.

Also, from a practical point of view, changing the owner of an object
is different from other things that robert might want to do. If robert
wants to create a table as user stuff or read some data from tables
user stuff can access or change privileges on objects that role stuff
owns, he can just execute "SET ROLE stuff" and then do any of that
stuff. But he can't give away his own objects by assuming stuff's
privileges. Either he can do it as himself, or he can't do it at all.
It wouldn't be crazy IMHO to decide that a non-inherited grant isn't
sufficient to donate objects to the granted role, and thus an
inherited grant is required in such cases. However, the current system
doesn't seem insane either, and in fact might be convenient in some
situations.

In short, my proposal is to change the ALTER DEFAULT PRIVILEGES code
so that you have to have the privileges of the target role, not jut
membership in the target role, and leave everything else unchanged.

Thoughts?

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

  2. Re: has_privs_of_role vs. is_member_of_role, redux

    Joe Conway <mail@joeconway.com> — 2022-08-25T19:03:27Z

    On 8/25/22 12:12, Robert Haas wrote:
> So far, so good. But it's clearly not the case that "GRANT stuff TO
> robert" has conferred no privileges at all on robert. At the very
> least, it's enabled him to "SET ROLE stuff", but what else? I decided
> to go through the code and make a list of the things that robert can
> now do that he couldn't do before. Here it is:
> 
> 1. robert can create new objects of various types owned by stuff:

> 2. robert can change the owner of objects he owns to instead be owned by stuff:

> 3. robert can change the default privileges for stuff:

> 4. robert can execute "SET ROLE stuff".

Nice analysis, and surprising (to me)

> I argue that #3 is a clear bug. robert can't select from stuff's
> tables or change privileges on stuff's objects, so why can he change
> stuff's default privileges? is_member_of_role() has a note that it is
> not to be used for privilege checking, and this seems like it's pretty
> clearly a privilege check.


+1 this feels very wrong to me


> On the flip side, #4 is pretty clearly correct. Presumably, allowing
> that to happen was the whole point of executing "GRANT stuff TO
> robert" in the first place.

Exactly

> The other two are less clear, in my opinion. We don't want users to
> end up owning objects that they didn't intend to own; in particular,
> if any user could make a security-definer function and then gift it to
> the superuser, it would be a disaster. So, arguably, the ability to
> make some other role the owner of an object represents a privilege
> that your role holds with respect to their role. Under that theory,
> the is_member_of_role() checks that are performed in cases #1 and #2
> are privilege checks, and we ought to be using has_privis_of_role()
> instead, so that a non-inherited role grant doesn't confer those
> privileges. But I don't find this very clear cut, because except when
> the object you're gifting is a Trojan horse, giving stuff away helps
> the recipient, not the donor.
> 
> Also, from a practical point of view, changing the owner of an object
> is different from other things that robert might want to do. If robert
> wants to create a table as user stuff or read some data from tables
> user stuff can access or change privileges on objects that role stuff
> owns, he can just execute "SET ROLE stuff" and then do any of that
> stuff. But he can't give away his own objects by assuming stuff's
> privileges. Either he can do it as himself, or he can't do it at all.
> It wouldn't be crazy IMHO to decide that a non-inherited grant isn't
> sufficient to donate objects to the granted role, and thus an
> inherited grant is required in such cases. However, the current system
> doesn't seem insane either, and in fact might be convenient in some
> situations.
> 
> In short, my proposal is to change the ALTER DEFAULT PRIVILEGES code
> so that you have to have the privileges of the target role, not jut
> membership in the target role, and leave everything else unchanged.
> 
> Thoughts?

I'm not sure about these last two. Does it matter that object creation 
is being logged, maybe for auditing purposes, under a different user 
than the owner of the object?

-- 
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  3. Re: has_privs_of_role vs. is_member_of_role, redux

    Robert Haas <robertmhaas@gmail.com> — 2022-08-25T20:19:21Z

    On Thu, Aug 25, 2022 at 3:03 PM Joe Conway <mail@joeconway.com> wrote:
> Nice analysis, and surprising (to me)

Thanks.

> > I argue that #3 is a clear bug. robert can't select from stuff's
> > tables or change privileges on stuff's objects, so why can he change
> > stuff's default privileges? is_member_of_role() has a note that it is
> > not to be used for privilege checking, and this seems like it's pretty
> > clearly a privilege check.
>
> +1 this feels very wrong to me

Cool. I'll prepare a patch for that, unless someone else beats me to it.

I really hate back-patching this kind of change but it's possible that
it's the right thing to do. There's no real security exposure because
the member could always SET ROLE and then do the exact same thing, so
back-patching feels to me like it has a significantly higher chance of
turning happy users into unhappy ones than the reverse. On the other
hand, it's pretty hard to defend the current behavior once you stop to
think about it, so perhaps it should be back-patched on those grounds.
On the third hand, the fact that this has gone undiscovered for a
decade makes you wonder whether we've really had clear enough ideas
about this to justify calling it a bug rather than, say, an elevation
of our thinking on this topic.

> I'm not sure about these last two. Does it matter that object creation
> is being logged, maybe for auditing purposes, under a different user
> than the owner of the object?

I'd be inclined to say that it doesn't matter, because the grant could
have just as well been inheritable, or the action could have been
performed by a superuser. Also, as a rule of thumb, I don't think we
should choose to prohibit things on the grounds that some auditing
regime might not be able to understand what happened. If that's an
issue, we should address it by making the logging better, or including
better logging hooks, or what have you. I think that the focus should
be on the permissions model: what is the "right thing" from a security
standpoint?

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

  4. Re: has_privs_of_role vs. is_member_of_role, redux

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-25T20:41:07Z

    Robert Haas <robertmhaas@gmail.com> writes:
> I really hate back-patching this kind of change but it's possible that
> it's the right thing to do. There's no real security exposure because
> the member could always SET ROLE and then do the exact same thing, so
> back-patching feels to me like it has a significantly higher chance of
> turning happy users into unhappy ones than the reverse. On the other
> hand, it's pretty hard to defend the current behavior once you stop to
> think about it, so perhaps it should be back-patched on those grounds.
> On the third hand, the fact that this has gone undiscovered for a
> decade makes you wonder whether we've really had clear enough ideas
> about this to justify calling it a bug rather than, say, an elevation
> of our thinking on this topic.

Yeah, I'd lean against back-patching.  This is the sort of behavioral
change that users tend not to like finding in minor releases.

			regards, tom lane

  5. Re: has_privs_of_role vs. is_member_of_role, redux

    Robert Haas <robertmhaas@gmail.com> — 2022-08-26T14:11:38Z

    On Thu, Aug 25, 2022 at 4:41 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Yeah, I'd lean against back-patching.  This is the sort of behavioral
> change that users tend not to like finding in minor releases.

Here's a small patch. Despite the small size of the patch, there are a
couple of debatable points here:

1. Should we have a code comment? I feel it isn't necessary, because
there's a comment just a few lines earlier saying "Look up the role
OIDs and do permissions checks" and that seems like sufficient
justification for what follows.

2. What about the error message? Personally, I'm not very excited
about "permission denied to whatever" as a way to phrase an error
message. It doesn't sound like particularly good grammar to me. But
it's the phrasing we use elsewhere, so I guess we should do the same
here.

3. Should we have a test case? We are extremely thin on test cases for
NOINHERIT behavior, it seems, and testing this one thing when we don't
test anything else seems relatively useless. Also, privileges.sql is a
giant mess. It's a 1700+ line test file that tests many fairly
unrelated things. I am inclined to think that this file badly needs to
be split up into a bunch of smaller files, because it's practically
unmaintainable as is. For instance, the stuff at the top of the file
is testing a bunch of things about role privileges, but then check
some stuff about leakproof functions before coming back to test stuff
about groups, which logically probably belongs with the role
privileges stuff. Perhaps a reasonable starting split would be
something like:

- Privileges on roles.
- Privileges on relations.
- Privileges on other kinds of objects.
- Default privileges.
- Security barriers and leakproof functions.
- Security-restricted operations.

Some of those files might be fairly small initially, but they might be
get bigger later, especially because it'd be a heck of a lot easier to
add new test cases if you didn't have to worry that some change you
make is going to break a test 1000 lines down in the file.

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

  6. Re: has_privs_of_role vs. is_member_of_role, redux

    Robert Haas <robertmhaas@gmail.com> — 2022-09-06T19:08:09Z

    Jeff Davis's comment in
http://postgr.es/m/4f8d536a9221bccc5a33bb784dace0ef2310ec4a.camel@j-davis.com
reminds me that I need to update this thread based on the patch posted
over there. That patch allows you to grant membership in one role to
another while withholding the ability to SET ROLE to the target role.
And it's already possible to grant membership in one role to another
while not allowing for inheritance of privileges. And I think that
sheds new light on the two debatable points from my original email:

On Thu, Aug 25, 2022 at 12:12 PM Robert Haas <robertmhaas@gmail.com> wrote:
> 1. robert can create new objects of various types owned by stuff:
>
> rhaas=> create schema stuff_by_robert authorization stuff;
> CREATE SCHEMA
> rhaas=> create schema unrelated_by_robert authorization unrelated;
> ERROR:  must be member of role "unrelated"
>
> 2. robert can change the owner of objects he owns to instead be owned by stuff:
>
> rhaas=> alter table robert_table owner to unrelated;
> ERROR:  must be member of role "unrelated"
> rhaas=> alter table robert_table owner to stuff;
> ALTER TABLE

It now seems to me that we should disallow these, because if we adopt
the patch from that other thread, and then you GRANT
pg_read_all_settings TO alice WITH INHERIT false, SET false, you might
reasonably expect that alice is not going to be able to clutter the
system with a bunch of objects owned by pg_read_all_settings, but
because of (1) and (2), alice can do exactly that.

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.

Meanwhile, the patch that I posted previously to fix point (3) from
the original email, that ALTER DEFAULT PRIVILEGES is allowed for no
good reason, still seems like a good idea. Any reviews appreciated.

Thanks,

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

  7. Re: has_privs_of_role vs. is_member_of_role, redux

    Stephen Frost <sfrost@snowman.net> — 2022-09-07T21:51:28Z

    Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> Jeff Davis's comment in
> http://postgr.es/m/4f8d536a9221bccc5a33bb784dace0ef2310ec4a.camel@j-davis.com
> reminds me that I need to update this thread based on the patch posted
> over there. That patch allows you to grant membership in one role to
> another while withholding the ability to SET ROLE to the target role.
> And it's already possible to grant membership in one role to another
> while not allowing for inheritance of privileges. And I think that
> sheds new light on the two debatable points from my original email:
> 
> On Thu, Aug 25, 2022 at 12:12 PM Robert Haas <robertmhaas@gmail.com> wrote:
> > 1. robert can create new objects of various types owned by stuff:
> >
> > rhaas=> create schema stuff_by_robert authorization stuff;
> > CREATE SCHEMA
> > rhaas=> create schema unrelated_by_robert authorization unrelated;
> > ERROR:  must be member of role "unrelated"
> >
> > 2. robert can change the owner of objects he owns to instead be owned by stuff:
> >
> > rhaas=> alter table robert_table owner to unrelated;
> > ERROR:  must be member of role "unrelated"
> > rhaas=> alter table robert_table owner to stuff;
> > ALTER TABLE
> 
> It now seems to me that we should disallow these, because if we adopt
> the patch from that other thread, and then you GRANT
> pg_read_all_settings TO alice WITH INHERIT false, SET false, you might
> reasonably expect that alice is not going to be able to clutter the
> system with a bunch of objects owned by pg_read_all_settings, but
> because of (1) and (2), alice can do exactly that.

Err, that shouldn't be allowed and if it is then that's my fault for not
implementing something to avoid having that happen.  imv, predefined
roles shouldn't be able to end up with objects they own except in cases
where we declare that a predefined role owns X.

I do think that the above two are correct and am fairly confident that
they were intentional as implemented as, otherwise, as noted in your
original message, you can't actually change the ownership of the
existing object/table and instead end up having to copy the whole thing,
which seems quite inefficient.  In other words, the same result could be
accomplished but in a much less efficient way and therefore it makes
sense to provide a way for it to be done that is efficient.

> 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).

> Meanwhile, the patch that I posted previously to fix point (3) from
> the original email, that ALTER DEFAULT PRIVILEGES is allowed for no
> good reason, still seems like a good idea. Any reviews appreciated.

Haven't looked at the patch, +1 on the general change though, that looks
like incorrect usage of is_member_of_role to me.

Thanks,

Stephen

  8. Re: has_privs_of_role vs. is_member_of_role, redux

    Robert Haas <robertmhaas@gmail.com> — 2022-09-08T13:21:31Z

    On Wed, Sep 7, 2022 at 5:51 PM Stephen Frost <sfrost@snowman.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

  9. Re: has_privs_of_role vs. is_member_of_role, redux

    Wolfgang Walther <walther@technowledgy.de> — 2022-09-08T15:45:07Z

    Robert Haas:
> 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?

I think to change the owner of an object from role A to role B, you just 
need a different "privilege" on that role B to "use" the role that way, 
which is distinct from INHERIT or SET ROLE "privileges".

When you are allowed to INHERIT a role, you are allowed to use the 
GRANTs that have been given to this role. When you are allowed to SET 
ROLE, then you are allowed to switch into this role. You could think of 
another "privilege", USAGE on a role, which would allow you to "use" 
this role as a target in a statement to change the owner of an object.

To change the owner for an object from role A to role B, you need:
- the privilege to ALTER the object, which is implied when you are A
- the privilege to "use" role B as a target

So basically the privilege to use role B as the new owner, is a 
privilege you have **on** the role object B, while the privilege to 
change the owner of an object is something you have **through** your 
membership in role A.

Up to v15, there were no separate privileges for this. You were either a 
member of a role or you were not. Now with INHERIT and maybe SET ROLE 
privileges/grant options, we can do two things:
- Keep the ability to use a role as a target in those statements as the 
most basic privilege on a role, that is implied by membership in that 
role and can't be taken away (currently the case), or
- invent a new privilege or grant option to allow changing that.

But mixing this with either INHERIT or SET ROLE doesn't make sense, imho.

Best

Wolfgang

  10. Re: has_privs_of_role vs. is_member_of_role, redux

    Robert Haas <robertmhaas@gmail.com> — 2022-09-08T16:02:06Z

    On Thu, Sep 8, 2022 at 11:45 AM Wolfgang Walther
<walther@technowledgy.de> wrote:
> I think to change the owner of an object from role A to role B, you just
> need a different "privilege" on that role B to "use" the role that way,
> which is distinct from INHERIT or SET ROLE "privileges".

It's not distinct, though, because if you can transfer ownership of a
table to another user, you can use that ability to gain the privileges
of that user.

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

  11. Re: has_privs_of_role vs. is_member_of_role, redux

    Wolfgang Walther <walther@technowledgy.de> — 2022-09-08T16:30:19Z

    Robert Haas:
>> I think to change the owner of an object from role A to role B, you just
>> need a different "privilege" on that role B to "use" the role that way,
>> which is distinct from INHERIT or SET ROLE "privileges".
> 
> It's not distinct, though, because if you can transfer ownership of a
> table to another user, you can use that ability to gain the privileges
> of that user.

Right, but the inverse is not neccessarily true, so you could have SET 
ROLE privileges, but not "USAGE" - and then couldn't change the owner of 
an object to this role.

USAGE is not a good term, because it implies "least amount of 
privileges", but in this case it's quite the opposite.

In any case, adding a grant option for SET ROLE, while keeping the 
required privileges for a transfer of ownership at the minimum 
(membership only), doesn't really make sense. I guess both threads 
should be discussed together?

Best

Wolfgang

  12. Re: has_privs_of_role vs. is_member_of_role, redux

    Wolfgang Walther <walther@technowledgy.de> — 2022-09-08T17:06:24Z

    Robert Haas:
> 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.

A different line of thought (compared to the "USAGE" privilege I 
discussed earlier), would be:
To transfer ownership of an object, you need two sets of privileges:
- You need to have the privilege to initiate a request to transfer 
ownership.
- You need to have the privilege to accept a request to transfer ownership.

Let's imagine there'd be such a request created temporarily, then when I 
start the process of changing ownership, I would have to change to the 
other role and then accept that request.

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.

So it should not be has_privs_of_role() nor has_privs_of_role() || 
member_can_set_role(), as you suggested above, but rather just 
member_can_set_role() only. Of course, only in the context of the SET 
ROLE patch.

Basically, with that patch is_member_of_role() has to become 
member_can_set_role().

> 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?

As stated above, I don't think this is about INHERIT. INHERIT works fine 
both without the SET ROLE patch (and keeping is_member_of_role) and with 
the SET ROLE patch (and changing to member_can_set_role).

The exception is made, because there is no formal two-step process for 
requesting and accepting a transfer of ownership. Or alternatively: 
There is no exception, it's just that during the command to transfer 
ownership, the current role has to be changed temporarily to the 
accepting role. And that's the same as checking is_member_of_role or 
member_can_set_role, respectively.

Best

Wolfgang

  13. Re: has_privs_of_role vs. is_member_of_role, redux

    Robert Haas <robertmhaas@gmail.com> — 2022-09-19T18:42:48Z

    On Fri, Aug 26, 2022 at 10:11 AM Robert Haas <robertmhaas@gmail.com> wrote:
> Here's a small patch. Despite the small size of the patch, there are a
> couple of debatable points here:

Nobody's commented on this patch specifically, but it seemed like we
had consensus that ALTER DEFAULT PRIVILEGES was doing The Wrong Thing,
so I've pushed the patch I posted previously for that issue.

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

  14. Re: has_privs_of_role vs. is_member_of_role, redux

    Robert Haas <robertmhaas@gmail.com> — 2022-09-19T19:32:02Z

    On Thu, Sep 8, 2022 at 1:06 PM <walther@technowledgy.de> wrote:
> A different line of thought (compared to the "USAGE" privilege I
> discussed earlier), would be:
> To transfer ownership of an object, you need two sets of privileges:
> - You need to have the privilege to initiate a request to transfer
> ownership.
> - You need to have the privilege to accept a request to transfer ownership.
>
> Let's imagine there'd be such a request created temporarily, then when I
> start the process of changing ownership, I would have to change to the
> other role and then accept that request.
>
> 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.

> So it should not be has_privs_of_role() nor has_privs_of_role() ||
> member_can_set_role(), as you suggested above, but rather just
> member_can_set_role() only. Of course, only in the context of the SET
> ROLE patch.

Now, having said that, this choice of behavior might have some
advantages. It would mean that you could GRANT pg_read_all_settings TO
someone WITH INHERIT TRUE, SET FALSE and that user would be able to
read all settings but would not be able to create objects owned by
pg_read_all_settings. It would also be upward-compatible with the
existing behavior, which is nice.

Well, maybe. Suppose that role A has been granted pg_read_all_settings
WITH INHERIT TRUE, SET TRUE and role B has been granted
pg_read_all_settings WITH INHERIT TRUE, SET FALSE. A can create a
table owned by pg_read_all_settings. If A does that, then B can now
create a trigger on that table and usurp the privileges of
pg_read_all_settings, after which B can now create any number of
objects owned by pg_read_all_settings. If A does not do that, though,
I think that with the proposed rule, B would have no way to create
objects owned by A. This is a bit unsatisfying. It seems like B should
either have the right to usurp pg_read_all_settings's privileges or
not, rather than maybe having that right depending on what some other
user chooses to do.

But maybe it's OK. It's hard to come up with perfect solutions here.
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).

But maybe there's a better answer. Ideas/opinions welcome.

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

  15. Re: has_privs_of_role vs. is_member_of_role, redux

    Wolfgang Walther <walther@technowledgy.de> — 2022-09-25T09:08:10Z

    Robert Haas:
> Well, maybe. Suppose that role A has been granted pg_read_all_settings
> WITH INHERIT TRUE, SET TRUE and role B has been granted
> pg_read_all_settings WITH INHERIT TRUE, SET FALSE. A can create a
> table owned by pg_read_all_settings. If A does that, then B can now
> create a trigger on that table and usurp the privileges of
> pg_read_all_settings, after which B can now create any number of
> objects owned by pg_read_all_settings.

I'm not seeing how this is possible. A trigger function would run with 
the invoking user's privileges by default, right? So B would have to 
create a trigger with a SECURITY DEFINER function, which is owned by 
pg_read_all_settings to actually usurp the privileges of that role. But 
creating objects with that owner is exactly the thing B can't do.

What am I missing?

Best

Wolfgang

  16. Re: has_privs_of_role vs. is_member_of_role, redux

    Robert Haas <robertmhaas@gmail.com> — 2022-09-26T15:27:52Z

    On Sun, Sep 25, 2022 at 5:08 AM Wolfgang Walther
<walther@technowledgy.de> wrote:
> Robert Haas:
> > Well, maybe. Suppose that role A has been granted pg_read_all_settings
> > WITH INHERIT TRUE, SET TRUE and role B has been granted
> > pg_read_all_settings WITH INHERIT TRUE, SET FALSE. A can create a
> > table owned by pg_read_all_settings. If A does that, then B can now
> > create a trigger on that table and usurp the privileges of
> > pg_read_all_settings, after which B can now create any number of
> > objects owned by pg_read_all_settings.
>
> I'm not seeing how this is possible. A trigger function would run with
> the invoking user's privileges by default, right? So B would have to
> create a trigger with a SECURITY DEFINER function, which is owned by
> pg_read_all_settings to actually usurp the privileges of that role. But
> creating objects with that owner is exactly the thing B can't do.

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:

rhaas=# create role alice;
CREATE ROLE
rhaas=# create table foo (a int, b text);
CREATE TABLE
rhaas=# alter table foo owner to pg_read_all_settings;
ALTER TABLE
rhaas=# grant pg_read_all_settings to alice;
GRANT ROLE
rhaas=# grant create on schema public to alice;
GRANT
rhaas=# set session authorization alice;
SET
rhaas=> create or replace function alice_function () returns trigger
as $$begin raise notice 'this trigger is running as %', current_user;
return null; end$$ language plpgsql;
CREATE FUNCTION
rhaas=> create trigger t1 before insert or update or delete on foo for
each row execute function alice_function();
CREATE TRIGGER
rhaas=> begin;
BEGIN
rhaas=*> insert into foo values (1, 'stuff');
NOTICE:  this trigger is running as alice
INSERT 0 0
rhaas=*> rollback;
ROLLBACK
rhaas=> reset session authorization;
RESET
rhaas=# begin;
BEGIN
rhaas=*# insert into foo values (1, 'stuff');
NOTICE:  this trigger is running as rhaas
INSERT 0 0
rhaas=*# rollback;
ROLLBACK

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. I'm not really sure how significant that is. If
you can create triggers as some other user and that user ever does
stuff as themselves, you can probably steal their privileges, because
they will probably eventually do DML on one of their own tables and
thereby execute your Trojan trigger. However, in the particular case
of pg_read_all_settings, the intent is probably that nobody would ever
run as that user, and there is probably also no reason to create
tables or other objects owned by that user. So maybe we really can say
that just blocking SET ROLE is enough.

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.

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

  17. Re: has_privs_of_role vs. is_member_of_role, redux

    Wolfgang Walther <walther@technowledgy.de> — 2022-09-26T16:16:46Z

    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.

> 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.

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

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)

Those implicit DDL privileges should be considered part of WITH SET 
TRUE. When you can't do SET ROLE x, then you can't act as the owner of 
any object owned by x.

Or to put it the other way around: Only allow implicit ownership 
privileges to be executed when the CURRENT_USER is the owner. But 
provide a shortcut, when you have the WITH SET TRUE option on a role, so 
that you don't need to do SET ROLE + CREATE TRIGGER, but can just do 
CREATE TRIGGER instead. This is similar to the mental model of 
"requesting and accepting a transfer of ownership" with an implicit SET 
ROLE built-in, that I used before.

Best

Wolfgang

  18. Re: has_privs_of_role vs. is_member_of_role, redux

    Robert Haas <robertmhaas@gmail.com> — 2022-09-26T17:15:31Z

    On Mon, Sep 26, 2022 at 12:16 PM Wolfgang Walther
<walther@technowledgy.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.

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.

Here, though, it doesn't really seem simple enough to explain in one
sentence, nor does it seem easy to reason about. I'm not sure that
there's any firm distinction between DML privileges and DDL
privileges. I'm not sure that the privileges that you mention are all
and only those that are security-relevant, nor that it would
necessarily remain true in the future even if it's true today.

There are many operations which are permitted or declined just using
an owner-check. One example is commenting on an object. That sure
sounds like it would fit within your proposed "DDL privileges
implicitly given through ownership" category, but it doesn't really
present any security hazard, so I do not think there is a good reason
to restrict that from a user who has INHERIT TRUE, SET FALSE. Another
is renaming an object, which is a little more murky. You can't
directly usurp someone's privileges by renaming an object that they
own, but you could potentially rename an object out of the way and
replace it with one that you own and thus induce a user to do
something dangerous. I don't really want to make even small exceptions
to the idea that inheriting a role's privileges means inheriting all
of them, and I especially don't want to make large exceptions, or
exceptions that involve judgement calls about the relative degree of
risk of each possible operation.

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

  19. Re: has_privs_of_role vs. is_member_of_role, redux

    Wolfgang Walther <walther@technowledgy.de> — 2022-09-26T19:16:50Z

    Robert Haas:
> 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.

Scratch my previous suggestion. A new, less fuzyy definition would be: 
Ownership is not a privilege itself and as such not inheritable.

When role A is granted to role B, two things happen:
1. Role B now has the right to use the GRANTed privileges of role A.
2. Role B now has the right to become role A via SET ROLE.

WITH SET controls whether point 2 is the case or not.

WITH INHERIT controls whether role B actually executes their right to 
use those privileges ("inheritance") **and** whether the set role is 
done implicitly for anything that requires ownership, but of course only 
WITH SET TRUE.

This is the same way that the role attributes INHERIT / NOINHERIT behave.

> 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.

This would be included in the above.

> Here, though, it doesn't really seem simple enough to explain in one
> sentence, nor does it seem easy to reason about.

I think the "ownership is not inheritable" idea is easy to explain.

> There are many operations which are permitted or declined just using
> an owner-check. One example is commenting on an object. That sure
> sounds like it would fit within your proposed "DDL privileges
> implicitly given through ownership" category, but it doesn't really
> present any security hazard, so I do not think there is a good reason
> to restrict that from a user who has INHERIT TRUE, SET FALSE. Another
> is renaming an object, which is a little more murky. You can't
> directly usurp someone's privileges by renaming an object that they
> own, but you could potentially rename an object out of the way and
> replace it with one that you own and thus induce a user to do
> something dangerous. I don't really want to make even small exceptions
> to the idea that inheriting a role's privileges means inheriting all
> of them, and I especially don't want to make large exceptions, or
> exceptions that involve judgement calls about the relative degree of
> risk of each possible operation.

I would not make this about security-risks only. We didn't distinguish 
between privileges and ownership that much before, because we didn't 
have WITH INHERIT or WITH SET. Now that we have both, we could do so.

The ideas of "inherited GRANTs" and "a shortcut to avoid SET ROLE to do 
owner-things" should be better to explain.

No judgement required.

All of this is to find a way to make WITH INHERIT TRUE, SET FALSE a 
"real", risk-free thing - and not just some syntactic sugar. And if that 
comes with the inability to COMMENT ON TABLE 
owned_by_pg_read_all_settings... fine. No need for that at all.

However, it would come with the inability to do SELECT * FROM 
owned_by_pg_read_all_settings, **unless** explicitly GRANTed to the 
owner, too. This might feel strange at first, but should not be a 
problem either. WITH INHERIT TRUE, SET FALSE is designed for built-in 
roles or other container roles that group a set of privileges. Those 
roles should not have objects they own anyway. And if they still do, 
denying access to those objects unless explicitly granted is the safe way.

Best

Wolfgang

  20. Re: has_privs_of_role vs. is_member_of_role, redux

    Stephen Frost <sfrost@snowman.net> — 2022-09-26T19:40:08Z

    Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Thu, Sep 8, 2022 at 1:06 PM <walther@technowledgy.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@gmail.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@technowledgy.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@gmail.com) wrote:
> On Mon, Sep 26, 2022 at 12:16 PM Wolfgang Walther
> <walther@technowledgy.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

  21. Re: has_privs_of_role vs. is_member_of_role, redux

    Stephen Frost <sfrost@snowman.net> — 2022-09-26T19:41:11Z

    Greetings,

* Wolfgang Walther (walther@technowledgy.de) wrote:
> Robert Haas:
> > 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.
> 
> Scratch my previous suggestion. A new, less fuzyy definition would be:
> Ownership is not a privilege itself and as such not inheritable.

One of the reasons the role system was brought into being was explicitly
to allow other roles to have ownership-level rights on objects that they
didn't directly own.

I don't see us changing that.

Thanks,

Stephen

  22. Re: has_privs_of_role vs. is_member_of_role, redux

    Robert Haas <robertmhaas@gmail.com> — 2022-09-26T20:24:06Z

    On Mon, Sep 26, 2022 at 3:16 PM Wolfgang Walther
<walther@technowledgy.de> wrote:
> Robert Haas:
> > 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.
>
> Scratch my previous suggestion. A new, less fuzyy definition would be:
> Ownership is not a privilege itself and as such not inheritable.
>
> When role A is granted to role B, two things happen:
> 1. Role B now has the right to use the GRANTed privileges of role A.
> 2. Role B now has the right to become role A via SET ROLE.
>
> WITH SET controls whether point 2 is the case or not.
>
> WITH INHERIT controls whether role B actually executes their right to
> use those privileges ("inheritance") **and** whether the set role is
> done implicitly for anything that requires ownership, but of course only
> WITH SET TRUE.

If I'm understanding correctly, this would amount to a major
redefinition of what it means to inherit privileges, and I think the
chances of such a change being accepted are approximately zero.
Inheriting privileges needs to keep meaning what it means now, namely,
you inherit all the rights of the granted role.

> > Here, though, it doesn't really seem simple enough to explain in one
> > sentence, nor does it seem easy to reason about.
>
> I think the "ownership is not inheritable" idea is easy to explain.

I don't. And even if I did think it were easy to explain, I don't
think it would be a good idea. One of my first patches to PostgreSQL
added a grantable TRUNCATE privilege to tables. I think that, under
your proposed definitions, the addition of this privilege would have
had the result that a role grant would cease to allow the recipient to
truncate tables owned by the granted role. There is currently a
proposal on the table to make VACUUM and ANALYZE grantable permissions
on tables, which would have the same issue. I think that if I made it
so that adding such privileges resulted in role inheritance not
working for those operations any more, people would come after me with
pitchforks. And I wouldn't blame them: that sounds terrible.

I think the only thing we should be discussing here is how to tighten
up the tests for operations in categories (1) and (2) in my original
email. The options so far proposed are: (a) do nothing, which makes
the proposed SET option on grants a lot less useful; (b) restrict
those operations by has_privs_of_role(), basically making them
dependent on the INHERIT option, (c) restrict them by
has_privs_of_role() || member_can_set_role(), requiring either the
INHERIT option or the SET option, or (d) restrict them by
member_can_set_role() only, i.e. making them depend on the SET  option
alone. A broader reworking of what the INHERIT option means is not on
the table: I don't want to write a patch for it, I don't think it's a
good idea, and I don't think the community would accept it even if I
did want to write a patch for it and even if I did think it was a good
idea.

I would like to hear more opinions on that topic. I understand your
vote from among those four options to be (d). I do not know what
anyone else thinks.

Thanks,

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

  23. Re: has_privs_of_role vs. is_member_of_role, redux

    Wolfgang Walther <walther@technowledgy.de> — 2022-09-27T06:05:23Z

    Robert Haas:
>> Scratch my previous suggestion. A new, less fuzyy definition would be:
>> Ownership is not a privilege itself and as such not inheritable.
>> [...]
> If I'm understanding correctly, this would amount to a major
> redefinition of what it means to inherit privileges, and I think the
> chances of such a change being accepted are approximately zero.
> Inheriting privileges needs to keep meaning what it means now, namely,
> you inherit all the rights of the granted role.

No. Inheriting stays the same, it's just WITH SET that's different from 
what it is "now".

> I don't. And even if I did think it were easy to explain, I don't
> think it would be a good idea. One of my first patches to PostgreSQL
> added a grantable TRUNCATE privilege to tables. I think that, under
> your proposed definitions, the addition of this privilege would have
> had the result that a role grant would cease to allow the recipient to
> truncate tables owned by the granted role. There is currently a
> proposal on the table to make VACUUM and ANALYZE grantable permissions
> on tables, which would have the same issue. I think that if I made it
> so that adding such privileges resulted in role inheritance not
> working for those operations any more, people would come after me with
> pitchforks. And I wouldn't blame them: that sounds terrible.

No, there is a misunderstanding. In my proposal, when you do WITH SET 
TRUE everything stays exactly the same as it is right now.

I'm just saying WITH SET FALSE should take away more of the things you 
can do (all the ownership things) to a point where it's safe to GRANT .. 
WITH INHERIT TRUE, SET FALSE and still be useful for pre-defined or 
privilege-container roles.

Could be discussed in the WITH SET thread, but it's a natural extension 
of the categories (1) and (2) in your original email. It's all about 
ownership.

Best

Wolfgang

  24. Re: has_privs_of_role vs. is_member_of_role, redux

    Robert Haas <robertmhaas@gmail.com> — 2022-09-27T11:55:14Z

    On Tue, Sep 27, 2022 at 2:05 AM Wolfgang Walther
<walther@technowledgy.de> wrote:
> I'm just saying WITH SET FALSE should take away more of the things you
> can do (all the ownership things) to a point where it's safe to GRANT ..
> WITH INHERIT TRUE, SET FALSE and still be useful for pre-defined or
> privilege-container roles.

I don't see that as viable, either. It's too murky what you'd have to
take away to make it safe, and it sounds like stuff that naturally
falls under INHERIT rather than SET.

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

  25. Re: has_privs_of_role vs. is_member_of_role, redux

    Jeff Davis <pgsql@j-davis.com> — 2022-10-20T19:57:43Z

    On Mon, 2022-09-19 at 15:32 -0400, Robert Haas wrote:
> 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).

I like this approach -- the idea that you can create a role that can't
own anything, can't create anything, and to which nobody else can "SET
ROLE".

Creating a "virtual" role like that feels much more declarative and
easy to document: "this isn't a real user, it's just a collection of
inheritable privileges". Even superusers couldn't "SET ROLE
pg_read_all_settings" or "OWNER TO pg_signal_backend".

I wouldn't call it "minimally privileged" (which feels wrong because it
wouldn't even have privileges on PUBLIC, as you say); I'd just say that
it's a type of role where those things just don't make sense.

-- 
Jeff Davis
PostgreSQL Contributor Team - AWS

  26. Re: has_privs_of_role vs. is_member_of_role, redux

    Jeff Davis <pgsql@j-davis.com> — 2022-10-20T20:05:04Z

    On Mon, 2022-09-26 at 15:40 -0400, Stephen Frost wrote:
> 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.

What about granting privileges to others? I don't think that makes
sense for a predefined role, either, because then they'd own a bunch of
grants, which is as awkward as owning objects.

> 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.

And that other role could be used for grants, if needed, too.

But I don't think we need to special-case predefined roles though. I
think a lot of administrators would like to declare some roles that are
just a collection of inheritable privileges.


-- 
Jeff Davis
PostgreSQL Contributor Team - AWS