Thread: DROP OWNED BY fails to drop privileges granted by non-owners (was Re: [GENERAL] Bug, Feature, or what else?)
DROP OWNED BY fails to drop privileges granted by non-owners (was Re: [GENERAL] Bug, Feature, or what else?)
From
Tom Lane
Date:
Adrian Klaver <adrian.klaver@gmail.com> writes:
> On 02/08/2013 08:14 AM, Tom Lane wrote:
>> Of course, postgres has other options besides that, of which "DROP OWNED
>> BY ak02" is probably the most appropriate here. Or if you really want
>> to get rid of just that grant, SET ROLE TO akretschmer01 and revoke.
> The DROP OWNED was tried further up the thread and did not seem to work:
Huh. You're right, here is a complete test case:
regression=# create schema s1;
cCREATE SCHEMA
regression=# create user u1;
CREATE ROLE
regression=# create user u2;
CREATE ROLE
regression=# grant all on schema s1 to u1 with grant option;
GRANT
regression=# \c - u1
You are now connected to database "regression" as user "u1".
regression=> grant all on schema s1 to u2;
GRANT
regression=> \c - postgres
You are now connected to database "regression" as user "postgres".
regression=# \dn+ s1 List of schemasName | Owner | Access privileges | Description
------+----------+----------------------+-------------s1 | postgres | postgres=UC/postgres+| | |
u1=U*C*/postgres +| | | u2=UC/u1 |
(1 row)
regression=# drop user u2; -- expect failure here
ERROR: role "u2" cannot be dropped because some objects depend on it
DETAIL: privileges for schema s1
regression=# drop owned by u2;
DROP OWNED
regression=# drop user u2; -- failure here is wrong
ERROR: role "u2" cannot be dropped because some objects depend on it
DETAIL: privileges for schema s1
regression=# \dn+ s1 List of schemasName | Owner | Access privileges | Description
------+----------+----------------------+-------------s1 | postgres | postgres=UC/postgres+| | |
u1=U*C*/postgres +| | | u2=UC/u1 |
(1 row)
I believe the problem is that DROP OWNED for privileges is implemented
by calling REVOKE. As noted upthread, when a superuser does REVOKE,
it's executed as though the object owner did the REVOKE, so only
privileges granted directly by the object owner go away. In this
particular example, "DROP OWNED BY u1" makes the grant to u1 go away,
and then the grant to u2 goes away via cascade ... but "DROP OWNED BY
u2" fails to accomplish anything at all, because postgres never granted
anything directly to u2.
We haven't seen this reported before, probably because the use of
GRANT OPTIONS isn't very common, but AFAICS it's been wrong since
the invention of DROP OWNED.
It looks to me like DropOwnedObjects doesn't actually insist on
superuserness to do DROP OWNED, only ability to become the role,
which means that DROP OWNED BY is completely broken for privileges
if executed by a non-superuser; the only privileges it would remove
would be those granted by the current user to the target user.
I'm not really sure what the desirable behavior would be in such a
case though. Ordinary users can't revoke privileges granted *to*
them, only privileges granted *by* them. So it's not necessarily
the case that a non-superuser should be able to make all privileges
granted to a target role go away, even if he's allowed to become
the target role and thereby drop objects that it owns. I wonder
how sensible it is really to allow DROP OWNED to non-superusers.
regards, tom lane
Re: DROP OWNED BY fails to drop privileges granted by non-owners (was Re: [GENERAL] Bug, Feature, or what else?)
From
Adrian Klaver
Date:
On 02/08/2013 09:09 AM, Tom Lane wrote: > Adrian Klaver <adrian.klaver@gmail.com> writes: >> On 02/08/2013 08:14 AM, Tom Lane wrote: >>> Of course, postgres has other options besides that, of which "DROP OWNED >>> BY ak02" is probably the most appropriate here. Or if you really want >>> to get rid of just that grant, SET ROLE TO akretschmer01 and revoke. > >> The DROP OWNED was tried further up the thread and did not seem to work: > > Huh. You're right, here is a complete test case: > > regression=# create schema s1; > cCREATE SCHEMA > regression=# create user u1; > CREATE ROLE > regression=# create user u2; > CREATE ROLE > regression=# grant all on schema s1 to u1 with grant option; > GRANT > regression=# \c - u1 > You are now connected to database "regression" as user "u1". > regression=> grant all on schema s1 to u2; > GRANT > regression=> \c - postgres > You are now connected to database "regression" as user "postgres". > regression=# \dn+ s1 > List of schemas > Name | Owner | Access privileges | Description > ------+----------+----------------------+------------- > s1 | postgres | postgres=UC/postgres+| > | | u1=U*C*/postgres +| > | | u2=UC/u1 | > (1 row) > > regression=# drop user u2; -- expect failure here > ERROR: role "u2" cannot be dropped because some objects depend on it > DETAIL: privileges for schema s1 > regression=# drop owned by u2; > DROP OWNED > regression=# drop user u2; -- failure here is wrong > ERROR: role "u2" cannot be dropped because some objects depend on it > DETAIL: privileges for schema s1 > regression=# \dn+ s1 > List of schemas > Name | Owner | Access privileges | Description > ------+----------+----------------------+------------- > s1 | postgres | postgres=UC/postgres+| > | | u1=U*C*/postgres +| > | | u2=UC/u1 | > (1 row) > > I believe the problem is that DROP OWNED for privileges is implemented > by calling REVOKE. As noted upthread, when a superuser does REVOKE, > it's executed as though the object owner did the REVOKE, so only > privileges granted directly by the object owner go away. In this > particular example, "DROP OWNED BY u1" makes the grant to u1 go away, > and then the grant to u2 goes away via cascade ... but "DROP OWNED BY > u2" fails to accomplish anything at all, because postgres never granted > anything directly to u2. > > We haven't seen this reported before, probably because the use of > GRANT OPTIONS isn't very common, but AFAICS it's been wrong since > the invention of DROP OWNED. > > It looks to me like DropOwnedObjects doesn't actually insist on > superuserness to do DROP OWNED, only ability to become the role, > which means that DROP OWNED BY is completely broken for privileges > if executed by a non-superuser; the only privileges it would remove > would be those granted by the current user to the target user. > I'm not really sure what the desirable behavior would be in such a > case though. Ordinary users can't revoke privileges granted *to* > them, only privileges granted *by* them. So it's not necessarily > the case that a non-superuser should be able to make all privileges > granted to a target role go away, even if he's allowed to become > the target role and thereby drop objects that it owns. I wonder > how sensible it is really to allow DROP OWNED to non-superusers. I am not sure I am following. Are we talking two different cases here? 1) As mentioned in the first paragraph the case where running DROP OWNED as a supersuser does not work. 2) A non-superuser running DROP OWNED and not having the necessary privileges. > > regards, tom lane > -- Adrian Klaver adrian.klaver@gmail.com
Re: DROP OWNED BY fails to drop privileges granted by non-owners (was Re: [GENERAL] Bug, Feature, or what else?)
From
Tom Lane
Date:
Adrian Klaver <adrian.klaver@gmail.com> writes:
> I am not sure I am following. Are we talking two different cases here?
What I was pointing out was that the non-superuser case seems to be
broken almost completely, whereas the superuser case is only broken
if the object owner has given away some grant options and those have
been exercised.
regards, tom lane
Re: DROP OWNED BY fails to drop privileges granted by non-owners (was Re: [GENERAL] Bug, Feature, or what else?)
From
Adrian Klaver
Date:
On 02/08/2013 10:09 AM, Tom Lane wrote: > Adrian Klaver <adrian.klaver@gmail.com> writes: >> I am not sure I am following. Are we talking two different cases here? > > What I was pointing out was that the non-superuser case seems to be > broken almost completely, whereas the superuser case is only broken > if the object owner has given away some grant options and those have > been exercised. Got it, thanks. > > regards, tom lane > -- Adrian Klaver adrian.klaver@gmail.com
Re: DROP OWNED BY fails to drop privileges granted by non-owners (was Re: [GENERAL] Bug, Feature, or what else?)
From
Alvaro Herrera
Date:
Tom Lane escribió:
> I believe the problem is that DROP OWNED for privileges is implemented
> by calling REVOKE. As noted upthread, when a superuser does REVOKE,
> it's executed as though the object owner did the REVOKE, so only
> privileges granted directly by the object owner go away. In this
> particular example, "DROP OWNED BY u1" makes the grant to u1 go away,
> and then the grant to u2 goes away via cascade ... but "DROP OWNED BY
> u2" fails to accomplish anything at all, because postgres never granted
> anything directly to u2.
>
> We haven't seen this reported before, probably because the use of
> GRANT OPTIONS isn't very common, but AFAICS it's been wrong since
> the invention of DROP OWNED.
So it seems.
I have been mulling this over today, and it seems to me that one
way to fix it would be to have ExecGrant_Objecttype() loop over all
possible grantors when determining what to revoke when it's called by
DROP OWNED. A proof-of-concept is below (RemoveRoleFromObjectACL would
set istmt.all_grantors to true, whereas ExecuteGrantStmt leaves it as
false and behaves as today).
I am not sure about the restrict_and_check_grant() call I left out in
the middle of the operation; it seems to me that if we limit DROP OWNED
to be called only by a superuser, we can get away without that check.
Or maybe we need a new version of that routine that will only apply the
bitmask and not raise any error messages.
The patch below is just for ExecGrant_Relation(), but as far as I can
tell all the ExecGrant_Objecttype() routines do pretty much the same
here, so I think it'd be better to refactor the select_best_grantor/
restrict_and_check_grant/merge_acl_with_grant sequence into a common
function, and then have that function apply the loop for all grantors.
Thoughts?
***************
*** 1891,1932 **** ExecGrant_Relation(InternalGrant *istmt) AclObjectKind aclkind; /*
DetermineID to do the grant as, and available grant options */
! select_best_grantor(GetUserId(), this_privileges,
! old_acl, ownerId,
! &grantorId, &avail_goptions);
!
! switch (pg_class_tuple->relkind) {
! case RELKIND_SEQUENCE:
! aclkind = ACL_KIND_SEQUENCE;
! break;
! default:
! aclkind = ACL_KIND_CLASS;
! break; }
! /*
! * Restrict the privileges to what we can actually grant, and emit
! * the standards-mandated warning and error messages.
! */
! this_privileges =
! restrict_and_check_grant(istmt->is_grant, avail_goptions,
! istmt->all_privs, this_privileges,
! relOid, grantorId, aclkind,
! NameStr(pg_class_tuple->relname),
! 0, NULL);
! /*
! * Generate new ACL.
! */
! new_acl = merge_acl_with_grant(old_acl,
! istmt->is_grant,
! istmt->grant_option,
! istmt->behavior,
! istmt->grantees,
! this_privileges,
! grantorId,
! ownerId); /* * We need the members of both old
andnew ACLs so we can correct
--- 1895,1962 ---- AclObjectKind aclkind; /* Determine ID to do the grant as, and available
grantoptions */
! if (!istmt->all_grantors) {
! select_best_grantor(GetUserId(), this_privileges,
! old_acl, ownerId,
! &grantorId, &avail_goptions);
!
! switch (pg_class_tuple->relkind)
! {
! case RELKIND_SEQUENCE:
! aclkind = ACL_KIND_SEQUENCE;
! break;
! default:
! aclkind = ACL_KIND_CLASS;
! break;
! }
!
! /*
! * Restrict the privileges to what we can actually grant, and emit
! * the standards-mandated warning and error messages.
! */
! this_privileges =
! restrict_and_check_grant(istmt->is_grant, avail_goptions,
! istmt->all_privs, this_privileges,
! relOid, grantorId, aclkind,
! NameStr(pg_class_tuple->relname),
! 0, NULL);
!
! /*
! * Generate new ACL.
! */
! new_acl = merge_acl_with_grant(old_acl,
! istmt->is_grant,
! istmt->grant_option,
! istmt->behavior,
! istmt->grantees,
! this_privileges,
! grantorId,
! ownerId); }
+ else
+ {
+ int ngrantors;
+ Oid *grantors;
+ int i;
+
+ ngrantors = aclgrantors(old_acl, istmt->grantees, &grantors);
! new_acl = aclcopy(old_acl);
! for (i = 0; i < ngrantors; i++)
! {
! Oid grantorId = grantors[i];
! new_acl = merge_acl_with_grant(new_acl,
! istmt->is_grant,
! istmt->grant_option,
! istmt->behavior,
! istmt->grantees,
! this_privileges,
! grantorId,
! ownerId);
! }
! } /* * We need the members of both old and new ACLs so we can correct
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Re: DROP OWNED BY fails to drop privileges granted by non-owners (was Re: [GENERAL] Bug, Feature, or what else?)
From
Alvaro Herrera
Date:
Tom Lane escribió: > It looks to me like DropOwnedObjects doesn't actually insist on > superuserness to do DROP OWNED, only ability to become the role, > which means that DROP OWNED BY is completely broken for privileges > if executed by a non-superuser; the only privileges it would remove > would be those granted by the current user to the target user. Well, moreover it fails completely with "permission denied" when tried, so yeah it is broken today. Now, I don't necessarily think we should remove the capability completely, as it seems useful. Consider a database where the superuser has created various department roles; admin privileges are given to certain users (bosses?) for each of such department roles. Such admins can add and remove other users from their roles at will, via grant and revoke; it seems useful to be able to remove all privileges from a certain user without going through objects one by one. Example: create user alice; create user bob; create role arts; create role arts_boss; grant arts to arts_boss with admin option; grant arts_boss to bob; create schema s_arts authorization arts_boss; revoke create on schema s_arts from public; \c - bob set role arts_boss; grant arts to alice; create table s_arts.animations (id int, description text); grant all on s_arts.animations to arts; grant update on s_arts.animations to alice; Here, Bob can GRANT and REVOKE specific things to Alice; but it doesn't work for Bob to "DROP OWNED BY alice" (fails with "permission denied to drop objects"), when in fact it does make sense that it would revoke those privileges -- but not those that alice might have gotten via a different chain of command, say on role "building" on which Charlie is admin. Alternatively we might say this is just gilding the lily and we should disallow this whole thing and restrict DROP OWNED. Two further possibilities here: a) only a superuser can run it, period. b) any user can run it on itself, and this revokes all privileges granted to this role, regardless of where they come from. (In any case I think we should disallow alice from doing "DROP OWNED by arts".) As a final comment, I am inclined to go the simplest route possible, first because I can't spend infinite time on this, and also because I think messing too much with this might lead to strange security issues. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services