Altering view ownership doesn't work ...

... because nowhere does it update the "checkAsUser" fields in the
view's query to be the OID of the new owner. This means that
permission checks about whether the view can access its underlying
tables will still be done as the old owner. An example:

regression=# create user u1;
CREATE ROLE
regression=# create user u2;
CREATE ROLE
regression=# \c - u1
You are now connected to database "regression" as user "u1".
regression=> create table t1(f1 int);
CREATE TABLE
regression=> create view v1 as select * from t1;
CREATE VIEW
regression=> grant select on v1 to u2;
GRANT

-- at this point u2 can select from v1 but not directly from t1

regression=> \c - postgres
You are now connected to database "regression" as user "postgres".
regression=# alter table v1 owner to u2;
ALTER TABLE
regression=# \c - u2
You are now connected to database "regression" as user "u2".
regression=> select * from v1;
f1
----
(0 rows)

-- this is WRONG, u2 should not have any ability to select from t1

The same problem applies to all rules, really, not only a view's
ON SELECT rule.

This is particularly bad because pg_dump is relying heavily on
ALTER OWNER these days. After a dump/restore, it is likely that
every view's "original owner" will be a superuser, and thus that
all permission checking is effectively disabled for accesses
from views. It wouldn't be too much of a stretch to call that
a security loophole.

I can think of two basic ways to fix this:

1. Add a bunch of code to ALTER OWNER to update every rule attached to
the target table.

2. Run setRuleCheckAsUser during rule load rather than rule store.

#2 is a lot simpler, and would fix the problem for existing broken rules
whereas #1 would not, so I'm kind of inclined to go with that. I doubt
there'd be any meaningful performance hit --- parsing the stored form
of a rule is relatively expensive anyway, so we cache the results.

Comments?

regards, tom lane

On Sun, Apr 30, 2006 at 12:34:42PM -0400, Tom Lane wrote:
> 2. Run setRuleCheckAsUser during rule load rather than rule store.
>
> #2 is a lot simpler, and would fix the problem for existing broken rules
> whereas #1 would not, so I'm kind of inclined to go with that. I doubt
> there'd be any meaningful performance hit --- parsing the stored form
> of a rule is relatively expensive anyway, so we cache the results.

FWIW, I think #2 is better also. It's the easiest way to ensure the
correct result and the performence isn't enough of a problem to worry
about doing it a different way.

Have a nice day,

--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
> On Sun, Apr 30, 2006 at 12:34:42PM -0400, Tom Lane wrote:
>> 2. Run setRuleCheckAsUser during rule load rather than rule store.

> FWIW, I think #2 is better also.

Actually, I'm sitting here realizing the problem is more complicated
than I thought :-(. The spanner in the works is the existence of the
RULE privilege --- a table owner can grant someone else the right to add
rules to his table. As things currently work, when the someone else
does so, it's *his* OID not the table owner's that gets put into the
rule's checkAsUser fields. Thus for example the someone else could add
a logging rule that makes entries into a table that the actual table
owner has no permissions for.

Whether or not you consider that sort of thing useful, it would
certainly be bad to use the table owner's OID for such permission
checks, because then granting RULE privilege on any table would be
tantamount to handing over every permission the table owner has ---
the grantee would be able to install arbitrary SQL to be executed as
the table owner. So really the RULE privilege only makes sense if a
rule is considered to be a separate object with separate ownership.

So it seems we either have to abandon the separate RULE privilege
(and just say that only table owners can install rules, and the
rules are always executed as though by the current owner), or we
have to promote rules to be fully separately owned objects. The
latter will be a horrid mess, in particular it will break existing
dump files that just ALTER the table's owner and don't go through
altering ownership of individual rules. (No, we can't have ALTER
TABLE OWNER automatically recurse to the individual rules, that'd just
create the same Trojan-horse situation where a malicious rule now has
the privileges it didn't have to start with.)

I'm inclined to think that the best choice is to drop the separate
RULE privilege. It's an interesting feature but I gauge its actual
usefulness by the fact that I didn't even realize it worked like that.

regards, tom lane

Agreed, RULE permissions seem to be of limited usefulness.

---------------------------------------------------------------------------

Tom Lane wrote:
> Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
> > On Sun, Apr 30, 2006 at 12:34:42PM -0400, Tom Lane wrote:
> >> 2. Run setRuleCheckAsUser during rule load rather than rule store.
>
> > FWIW, I think #2 is better also.
>
> Actually, I'm sitting here realizing the problem is more complicated
> than I thought :-(. The spanner in the works is the existence of the
> RULE privilege --- a table owner can grant someone else the right to add
> rules to his table. As things currently work, when the someone else
> does so, it's *his* OID not the table owner's that gets put into the
> rule's checkAsUser fields. Thus for example the someone else could add
> a logging rule that makes entries into a table that the actual table
> owner has no permissions for.
>
> Whether or not you consider that sort of thing useful, it would
> certainly be bad to use the table owner's OID for such permission
> checks, because then granting RULE privilege on any table would be
> tantamount to handing over every permission the table owner has ---
> the grantee would be able to install arbitrary SQL to be executed as
> the table owner. So really the RULE privilege only makes sense if a
> rule is considered to be a separate object with separate ownership.
>
> So it seems we either have to abandon the separate RULE privilege
> (and just say that only table owners can install rules, and the
> rules are always executed as though by the current owner), or we
> have to promote rules to be fully separately owned objects. The
> latter will be a horrid mess, in particular it will break existing
> dump files that just ALTER the table's owner and don't go through
> altering ownership of individual rules. (No, we can't have ALTER
> TABLE OWNER automatically recurse to the individual rules, that'd just
> create the same Trojan-horse situation where a malicious rule now has
> the privileges it didn't have to start with.)
>
> I'm inclined to think that the best choice is to drop the separate
> RULE privilege. It's an interesting feature but I gauge its actual
> usefulness by the fact that I didn't even realize it worked like that.
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly
>

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Martijn van Oosterhout wrote:
-- Start of PGP signed section.
> On Sun, Apr 30, 2006 at 12:34:42PM -0400, Tom Lane wrote:
> > 2. Run setRuleCheckAsUser during rule load rather than rule store.
> >
> > #2 is a lot simpler, and would fix the problem for existing broken rules
> > whereas #1 would not, so I'm kind of inclined to go with that. I doubt
> > there'd be any meaningful performance hit --- parsing the stored form
> > of a rule is relatively expensive anyway, so we cache the results.
>
> FWIW, I think #2 is better also. It's the easiest way to ensure the
> correct result and the performence isn't enough of a problem to worry
> about doing it a different way.

Has this been completed?

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Has this been completed?

No, still on the to-do list. IIRC that was about the time we got
diverted by fixing the encoding security issues...

regards, tom lane

Reminding folks this bug is still open.

---------------------------------------------------------------------------

Tom Lane wrote:
> ... because nowhere does it update the "checkAsUser" fields in the
> view's query to be the OID of the new owner. This means that
> permission checks about whether the view can access its underlying
> tables will still be done as the old owner. An example:
>
> regression=# create user u1;
> CREATE ROLE
> regression=# create user u2;
> CREATE ROLE
> regression=# \c - u1
> You are now connected to database "regression" as user "u1".
> regression=> create table t1(f1 int);
> CREATE TABLE
> regression=> create view v1 as select * from t1;
> CREATE VIEW
> regression=> grant select on v1 to u2;
> GRANT
>
> -- at this point u2 can select from v1 but not directly from t1
>
> regression=> \c - postgres
> You are now connected to database "regression" as user "postgres".
> regression=# alter table v1 owner to u2;
> ALTER TABLE
> regression=# \c - u2
> You are now connected to database "regression" as user "u2".
> regression=> select * from v1;
> f1
> ----
> (0 rows)
>
> -- this is WRONG, u2 should not have any ability to select from t1
>
> The same problem applies to all rules, really, not only a view's
> ON SELECT rule.
>
> This is particularly bad because pg_dump is relying heavily on
> ALTER OWNER these days. After a dump/restore, it is likely that
> every view's "original owner" will be a superuser, and thus that
> all permission checking is effectively disabled for accesses
> from views. It wouldn't be too much of a stretch to call that
> a security loophole.
>
> I can think of two basic ways to fix this:
>
> 1. Add a bunch of code to ALTER OWNER to update every rule attached to
> the target table.
>
> 2. Run setRuleCheckAsUser during rule load rather than rule store.
>
> #2 is a lot simpler, and would fix the problem for existing broken rules
> whereas #1 would not, so I'm kind of inclined to go with that. I doubt
> there'd be any meaningful performance hit --- parsing the stored form
> of a rule is relatively expensive anyway, so we cache the results.
>
> Comments?
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/docs/faq

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +