Thread: Foreign keys breaks tables permissions
Hi, If one does: create table master ( id integer not null, primary key (id)); create table detail ( id integer not null, master_id integer not null, primary key (id), foreign key (master_id) referencesmaster (id)); insert into master (id) values (1); grant select on master to a_user;grant select, insert, update, delete on detail to a_user; then if login as "a_user" and does: insert into detail (id, master_id) values (1, 10); this will result in: "ERROR: master: Permission denied". This seems a bug to me ? Isn't it ? Regards, Raul Chirea.
> > Hi, > > If one does: > > [...] > grant select on master to a_user; > grant select, insert, update, delete on detail to a_user; > > then if login as "a_user" and does: > > insert into detail (id, master_id) values (1, 10); > > this will result in: "ERROR: master: Permission denied". > > This seems a bug to me ? Isn't it ? Outch, yes, we missed something here. Peter, you said you'll probably work on the ACL stuff after 7.0. We need to coordinate that work with the function manager redesign to go for SETUID triggers and functions. Jan -- #======================================================================# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #========================================= wieck@debis.com (Jan Wieck) #
Jan Wieck writes: > Peter, you said you'll probably work on the ACL stuff after 7.0. We > need to coordinate that work with the function manager redesign to go > for SETUID triggers and functions. Yes, very nice feature. Far down the road in my dreams though. However, SQL has a REFERENCES privilege, which would probably be the more appropriate one here. -- Peter Eisentraut Sernanders väg 10:115 peter_e@gmx.net 75262 Uppsala http://yi.org/peter-e/ Sweden
Resurrecting a bug report from mid-April:
wieck@debis.com (Jan Wieck) writes:
>> If one does:
>>
>> [...]
>> grant select on master to a_user;
>> grant select, insert, update, delete on detail to a_user;
>>
>> then if login as "a_user" and does:
>>
>> insert into detail (id, master_id) values (1, 10);
>>
>> this will result in: "ERROR: master: Permission denied".
>>
>> This seems a bug to me ? Isn't it ?
> Outch,
> yes, we missed something here. Peter, you said you'll
> probably work on the ACL stuff after 7.0. We need to
> coordinate that work with the function manager redesign to go
> for SETUID triggers and functions.
I looked at this some more because people were complaining that it
was still broken in 7.0. AFAICT, it's got nothing to do with SETUID
triggers or anything so hairy, it's just a question of what permissions
we think ought to be required for which actions. The issue is very
simple: the RI insert trigger doesn't do a SELECT on the master table,
it does a SELECT FOR UPDATE --- and execMain.c thinks that that should
require UPDATE access rights to the master.
So, two questions:
1. Why is RI_FKey_check() using SELECT FOR UPDATE and not plain SELECT?
2. What permissions should SELECT FOR UPDATE require?
If the existing code is correct on both these points, then I think the
answer is that there is no bug: updating a table that has a foreign
key reference will require update rights on the master as well. I would
rather conclude that one of these two points is wrong...
regards, tom lane
Tom Lane writes: > I looked at this some more because people were complaining that it > was still broken in 7.0. AFAICT, it's got nothing to do with SETUID > triggers or anything so hairy, it's just a question of what permissions > we think ought to be required for which actions. Since the foreign keys are implemented in semi-userspace the triggers will either have to abide by the userspace privilege rules (not really good, see below), circumvent the privilege system (e.g., not use SPI, but scan the table yourself; probably no good), or be given special privileges, i.e., setuid or similar. In SQL land the privilege required for a foreign key *definition* is REFERENCES, once you have it set up, no further privileges are required to do the referencing. That makes some sense because changes to the FK table never change the PK table, only the other way around. > 1. Why is RI_FKey_check() using SELECT FOR UPDATE and not plain SELECT? AFAIU this function checks upon changes to the FK table whether a PK exists. In don't think you need to lock the PK table for that because once you know the PK existed at some point during the insert/update you have satisfied the requirement. If someone mangles the PK while you're still running then any ignited delete or update on the FK table will block with the normal lock mechanisms. > 2. What permissions should SELECT FOR UPDATE require? UPDATE seems reasonable. SELECT is no good because it would give read-only users the locking power of users with write access. > If the existing code is correct on both these points, then I think the > answer is that there is no bug: updating a table that has a foreign > key reference will require update rights on the master as well. I don't think that's acceptable. -- Peter Eisentraut Sernanders väg 10:115 peter_e@gmx.net 75262 Uppsala http://yi.org/peter-e/ Sweden
Peter Eisentraut <peter_e@gmx.net> writes:
>> 1. Why is RI_FKey_check() using SELECT FOR UPDATE and not plain SELECT?
>> 2. What permissions should SELECT FOR UPDATE require?
> UPDATE seems reasonable. SELECT is no good because it would give read-only
> users the locking power of users with write access.
>> If the existing code is correct on both these points, then I think the
>> answer is that there is no bug: updating a table that has a foreign
>> key reference will require update rights on the master as well.
> I don't think that's acceptable.
I don't like it either, but if an FK check must use SELECT FOR UPDATE
then anyone who can trigger an FK check has the ability to create a
write-class lock on the referenced table. Wrapping the FK check
in a SETUID trigger doesn't change that fundamental fact; it'll just
mean that the user triggering the check is now able to create a lock
that he doesn't have the privileges to create directly.
This is perhaps the least undesirable of the choices we have, but it's
still a security hole.
regards, tom lane
Tom Lane writes: > This is perhaps the least undesirable of the choices we have, but it's > still a security hole. The reason this concerns me is that requiring update rights on the referenced table eliminates much the benefit of foreign keys from an administration point of view: If the primary keys can be updated freely, they no longer constrain the data in the referencing table effectively. I suppose we'll have to live with that for now but I'd suggest that it be put on the TODO list somewhere. -- Peter Eisentraut Sernanders väg 10:115 peter_e@gmx.net 75262 Uppsala http://yi.org/peter-e/ Sweden
Peter Eisentraut <peter_e@gmx.net> writes:
> Tom Lane writes:
>> This is perhaps the least undesirable of the choices we have, but it's
>> still a security hole.
> The reason this concerns me is that requiring update rights on the
> referenced table eliminates much the benefit of foreign keys from an
> administration point of view: If the primary keys can be updated freely,
> they no longer constrain the data in the referencing table effectively.
> I suppose we'll have to live with that for now but I'd suggest that it be
> put on the TODO list somewhere.
What we need to do about it is implement the separate REFERENCES right
as specified by SQL92, and then fix FK support to require that right
rather than UPDATE...
regards, tom lane