Thread: Re: [SQL] Foreign keys breaks tables permissions
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
I believe the reason that the trigger does a select for update was because otherwise there could exist a case that we select and see it and then have the row go away afterwards because nothing stops the delete. I could be really wrong, but I see the scenario as the below: If the delete happens first, the select for update waits and then knows that the row isn't there any more and it should fail. If the select for update happens first, the delete waits and the on delete semantics get operated. Without the lock, one transaction could delete the row and not have the on delete happen, because it doesn't see the inserted rows in the other transaction and the inserting transaction thinks the child is okay because we can see the parent, but when both commit we have a child without a parent. Not that that was probably terribly helpful as to how to go ahead, but... ----- Original Message ----- From: "Tom Lane" <tgl@sss.pgh.pa.us> To: "Jan Wieck" <wieck@debis.com> Cc: <pgsql-hackers@postgresql.org>; <pgsql-sql@postgresql.org> Sent: Thursday, May 18, 2000 4:17 PM Subject: Re: [SQL] Foreign keys breaks tables permissions > 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...
"Stephan Szabo" <sszabo@kick.com> writes:
> I believe the reason that the trigger does a select for update was
> because otherwise there could exist a case that we select and see it
> and then have the row go away afterwards because nothing stops the
> delete.
Hmm, good point. And I think I see the reason for the protection
logic as well: if you can do SELECT FOR UPDATE then you can acquire
a lock that will block a competing writer. Therefore, even though
you can't modify the table, you can create the same sort of denial-
of-service attack that someone with real UPDATE privileges could
create, just by leaving your transaction open.
So, either we live with update requiring update rights on the
table referenced as a foreign key, or we break something else.
Grumble.
Probably the denial-of-service argument is the weakest of the three
points. Is anyone in favor of reducing SELECT FOR UPDATE to only
requiring "SELECT" rights, and living with the possible lock-that-
you-shouldn't-really-have-been-able-to-get issue?
regards, tom lane
Tom Lane wrote: > "Stephan Szabo" <sszabo@kick.com> writes: > > I believe the reason that the trigger does a select for update was > > because otherwise there could exist a case that we select and see it > > and then have the row go away afterwards because nothing stops the > > delete. > > Probably the denial-of-service argument is the weakest of the three > points. Is anyone in favor of reducing SELECT FOR UPDATE to only > requiring "SELECT" rights, and living with the possible lock-that- > you-shouldn't-really-have-been-able-to-get issue? > But what about DELETE CASCADE cases for exmaple ? Maybe RI_trigger should be able to update/insert/delete the referenced table. However another kind of permission for foreign key seems to be needed. i.e only granted users could define foreign key of the referenced table in CREATE (ALTER) TABLE command. Otherwise not granted users could delete tuples of the referenced table by defining a bogus foreign key of the table with DELETE CASCADE option. Comments ? Regards. Hiroshi Inoue Inoue@tpf.co.jp
Hiroshi Inoue wrote: > > Tom Lane wrote: > > > "Stephan Szabo" <sszabo@kick.com> writes: > > > I believe the reason that the trigger does a select for update was > > > because otherwise there could exist a case that we select and see it > > > and then have the row go away afterwards because nothing stops the > > > delete. > > > > Probably the denial-of-service argument is the weakest of the three > > points. Is anyone in favor of reducing SELECT FOR UPDATE to only > > requiring "SELECT" rights, and living with the possible lock-that- > > you-shouldn't-really-have-been-able-to-get issue? > > > > But what about DELETE CASCADE cases for exmaple ? > Maybe RI_trigger should be able to update/insert/delete > the referenced table. > However another kind of permission for foreign key > seems to be needed. i.e only granted users could > define foreign key of the referenced table in CREATE > (ALTER) TABLE command. IIRC this is even in the SQL standard as a separate right (maybe REFERENCES ?) > Otherwise not granted > users could delete tuples of the referenced table > by defining a bogus foreign key of the table with > DELETE CASCADE option. > > Comments ? > > Regards. > > Hiroshi Inoue > Inoue@tpf.co.jp
Hannu Krosing wrote:
>
> Hiroshi Inoue wrote:
> >
> > Tom Lane wrote:
> >
> > > "Stephan Szabo" <sszabo@kick.com> writes:
> > > > I believe the reason that the trigger does a select for update was
> > > > because otherwise there could exist a case that we select and see it
> > > > and then have the row go away afterwards because nothing stops the
> > > > delete.
> > >
> > > Probably the denial-of-service argument is the weakest of the three
> > > points. Is anyone in favor of reducing SELECT FOR UPDATE to only
> > > requiring "SELECT" rights, and living with the possible lock-that-
> > > you-shouldn't-really-have-been-able-to-get issue?
> > >
> >
> > But what about DELETE CASCADE cases for exmaple ?
> > Maybe RI_trigger should be able to update/insert/delete
> > the referenced table.
> > However another kind of permission for foreign key
> > seems to be needed. i.e only granted users could
> > define foreign key of the referenced table in CREATE
> > (ALTER) TABLE command.
>
> IIRC this is even in the SQL standard as a separate right (maybe REFERENCES ?)
Here's from SQL92 draft:
We should at least consider it when designing our GRANT system
.........
4.26 Privileges
A privilege authorizes a given category of <action> to be per- formed on a specified base table, view,
column,domain,
character set, collation, or translation by a specified <authorization
iden- tifier>. The mapping of <authorization identifier>s to
operating system users is implementation-dependent. The <action>s that
can be specified are:
- INSERT
- INSERT (<column name list>)
- UPDATE
- UPDATE (<column name list>)
- DELETE
- SELECT
- REFERENCES
- REFERENCES (<column name list>)
- USAGE
.......
A privilege descriptor with an action of INSERT, UPDATE,
DELETE, SELECT, or REFERENCES is called a table privilege descriptor
and identifies the existence of a privilege on the table identified
by the privilege descriptor.
A privilege descriptor with an action of SELECT (<column name list>), INSERT (<column name list>),
UPDATE(<column name
list>), or REFERENCES (<column name list>) is called a column privilege
de- scriptor and identifies the existence of a privilege on the
column in the table identified by the privilege descriptor.
Note: In this International Standard, a SELECT column privilege cannot be explicitly granted or revoked.
However,for the sake of compatibility with planned future language extensions,
SELECT column privilege descriptors will appear in the Information
Schema.
A table privilege descriptor specifies that the privilege iden- tified by the action (unless the action
isDELETE) is to be au- tomatically granted by the grantor to the grantee on all
columns subsequently added to the table.
A privilege descriptor with an action of USAGE is called a
usage privilege descriptor and identifies the existence of a
privilege on the domain, character set, collation, or translation identified
by the privilege descriptor.
A grantable privilege is a privilege associated with a schema
that may be granted by a <grant statement>.
The phrase applicable privileges refers to the privileges
defined by the privilege descriptors that define privileges granted to
the current <authorization identifier>.
The set of applicable privileges for the current <authorization identifier> consists of the privileges
definedby the privilege descriptors associated with that <authorization identifier> and the privileges
definedby the privilege descriptors associated
with PUBLIC.
Privilege descriptors that represent privileges for the owner
of an object have a special grantor value, "_SYSTEM". This value
is reflected in the Information Schema for all privileges that
apply to the owner of the object.
........
11.36 <grant statement>
Function
Define privileges.
Format
<grant statement> ::= GRANT <privileges> ON <object name> TO <grantee> [ { <comma>
<grantee>}... ] [ WITH GRANT OPTION ]
<object name> ::= [ TABLE ] <table name> | DOMAIN <domain name> |
COLLATION<collation name> | CHARACTER SET <character set name> | TRANSLATION <translation
name>
-----------
Hannu
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