Обсуждение: 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@svana.org> http://svana.org/kleptog/ > From each according to his ability. To each according to his ability to litigate.
Martijn van Oosterhout <kleptog@svana.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@svana.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@postgresql.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@candle.pha.pa.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@momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +