Thread: Bug introduced by recent ALTER OWNER permissions check change

postgres=# create user u1;
CREATE ROLE
postgres=# create schema s1;
CREATE SCHEMA
postgres=# create table s1.t1(f1 int);
CREATE TABLE
postgres=# alter table s1.t1 owner to u1;
ERROR:  permission denied for schema s1
postgres=#

Considering I am superuser, it should darn well allow this.

The problem of course is the test that u1 would have the rights to
create t1 in s1, which he doesn't.  I think we have to skip that
test if superuser.  As long as we need an explicit test on
superuserness, we may as well skip *all* the added code.

Comments?
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Considering I am superuser, it should darn well allow this.

Agreed.

> The problem of course is the test that u1 would have the rights to
> create t1 in s1, which he doesn't.  I think we have to skip that
> test if superuser.  As long as we need an explicit test on
> superuserness, we may as well skip *all* the added code.
>
> Comments?

I don't like this approach to solving the problem.  I would rather see
the check modified to allow the ownership change provided:

the user issueing the command has access to destination role
AND
(
the destination role can create the table OR the user issuing the command has owner rights on the schema/db
)
etc

This would solve the superuser() issue and would allow owners of schemas
to have an object in their schema owned by a role (which presumably
generally has less/limited access) which doesn't (and probably
shouldn't, ever, really) have create access on that schema.

A fairly common setup I have is to create roles with very limited access
(certainly not create access on a schema) and then grant only the access
they need.  In some cases, those roles do need ownership-level
permissions (unfortunately) on tables so that they can do truncates,
vacuums, etc, since our permissions system isn't granular enough to give
them just that access w/o ownership.

The cases where the limited access role gets ownership of a table isn't
very common (I try to avoid it) but it does happen from time to time.
Of course, currently I just use superuser to make such adjustments, but
being able to do it as my regular user would be nice, imv.
Thanks,
    Stephen

Stephen Frost <sfrost@snowman.net> writes:
> I don't like this approach to solving the problem.  I would rather see
> the check modified to allow the ownership change provided:

> the user issueing the command has access to destination role
> AND
> (
> the destination role can create the table
>   OR the user issuing the command has owner rights on the schema/db
> )
> etc

I don't think so --- this would allow unprivileged users to use ALTER
OWNER to arrive at states they could not arrive at otherwise; which
destroys the entire argument that non-superuser ALTER OWNER is not a
security risk.  Shall we reverse out the patch and require you to
justify it from scratch?

Superusers should be allowed to do whatever they want, but that doesn't
mean that we should weaken the rules applied to ordinary users.
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > I don't like this approach to solving the problem.  I would rather see
> > the check modified to allow the ownership change provided:
>
> > the user issueing the command has access to destination role
> > AND
> > (
> > the destination role can create the table
> >   OR the user issuing the command has owner rights on the schema/db
> > )
> > etc
>
> I don't think so --- this would allow unprivileged users to use ALTER
> OWNER to arrive at states they could not arrive at otherwise; which
> destroys the entire argument that non-superuser ALTER OWNER is not a
> security risk.  Shall we reverse out the patch and require you to
> justify it from scratch?

Does it really?  I don't think so.  If you have owner privileges on the
schema you can grant create rights to the role, then either ALTER OWNER
if the patch is kept or just change to the role, create table x as
select * from y;, etc, and then revoke the create privileges.  So, such
unprivileged users (which yet are owners of the schema in question, one
of the requirements above) could arrive at that state regardless.

> Superusers should be allowed to do whatever they want, but that doesn't
> mean that we should weaken the rules applied to ordinary users.

Having to special case superusers all over the place is an indication of
poor design, imho.
Thanks,
    Stephen

Stephen Frost <sfrost@snowman.net> writes:
> Does it really?  I don't think so.  If you have owner privileges on the
> schema you can grant create rights to the role, then either ALTER OWNER
> if the patch is kept or just change to the role, create table x as
> select * from y;, etc, and then revoke the create privileges.

Hmm, that would work, but it still leaves me itchy.  If we allow this,
why not even further-removed schemes requiring several SET ROLEs?
For instance, you could argue that ALTER OWNER should be allowed to
anyone who can become the old object owner, even if their current role
doesn't include that privilege.  (That is, the difference between
is_member and has_privs checks.)  Or say that either the old or new
object owner can be owner of the containing schema.  (Which would amount
to disregarding whether a schema owner has revoked his own CREATE
privilege, on the grounds that he could always choose to grant it to
himself again.)  I'm really leery of going down this path without
significant use-cases in its favor.

> Having to special case superusers all over the place is an indication of
> poor design, imho.

Contorting the privilege rules to avoid special-casing superusers is
worse, imho.  At least when you do "if (superuser())" you know you
aren't creating any holes that might be exploitable by non-superusers.
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > Does it really?  I don't think so.  If you have owner privileges on the
> > schema you can grant create rights to the role, then either ALTER OWNER
> > if the patch is kept or just change to the role, create table x as
> > select * from y;, etc, and then revoke the create privileges.
>
> Hmm, that would work, but it still leaves me itchy.  If we allow this,
> why not even further-removed schemes requiring several SET ROLEs?

I'm not entirely sure I follow...  In the 'inherit_role' case, which is
what I'm thinking about anyway, you have all the permissions which are
granted to any role you're in.  One of those permissions is to create
objects owned by that role.  Allowing users to 'alter owner', while
useful in its own right, also makes up for the inability to create an
object owned by a role you're in directly.

> For instance, you could argue that ALTER OWNER should be allowed to
> anyone who can become the old object owner, even if their current role
> doesn't include that privilege.  (That is, the difference between
> is_member and has_privs checks.)

I don't see how, having only the rights of the old object owner, you
could recreate the object as owned by another role which you're in,
unless you also had ownership permissions on the schema in question.

This isn't a case where several SET ROLEs would be necessary, this
appears to be a case which, regardless of how many SET ROLEs are done,
the user attempting to change the ownership wouldn't be able to.  As
such, I see it as something distinct and seperate from my argument.

Perhaps I've misunderstood though.

> Or say that either the old or new
> object owner can be owner of the containing schema.  (Which would amount
> to disregarding whether a schema owner has revoked his own CREATE
> privilege, on the grounds that he could always choose to grant it to
> himself again.)  I'm really leery of going down this path without
> significant use-cases in its favor.

Well, hang on, this case is already reasonably covered, imv:
You still have to be able to become the 'new owner' role, therefore you
have the permissions granted by that role, and those permissions may
provide the current user with 'owner' status on the schema.

I don't see this case as different from the original situation under
discussion.  The requirements being:

You must have ownership rights on the object before the change.
AND
You must have ownership rights on the object after the change.
AND
(
The new owner must have CREATE rights on the schema.
OR
You must have ownership rights on the schema.
)

The ownership rights on the schema being granted by the old role or the
new role, or some other role, or directly to the user, is unimportant.

> > Having to special case superusers all over the place is an indication of
> > poor design, imho.
>
> Contorting the privilege rules to avoid special-casing superusers is
> worse, imho.  At least when you do "if (superuser())" you know you
> aren't creating any holes that might be exploitable by non-superusers.

Instead you're requiring more and more users to be superusers in order
to get things done, increasing the chances of a superuser-level
compromise.  Having "if (superuser())" in alot of places encourages use
of superuser-level accounts which I think should generally be
discouraged in favor of more granular permissions in the system to help
mitigate the risk of a given account being compromised.
Thanks,
    Stephen