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) references master (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(at)debis(dot)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(at)gmx(dot)net 75262 Uppsala
http://yi.org/peter-e/ Sweden

Resurrecting a bug report from mid-April:

wieck(at)debis(dot)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(at)sss(dot)pgh(dot)pa(dot)us>
To: "Jan Wieck" <wieck(at)debis(dot)com>
Cc: <pgsql-hackers(at)postgresql(dot)org>; <pgsql-sql(at)postgresql(dot)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(at)kick(dot)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(at)kick(dot)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(at)tpf(dot)co(dot)jp

Hiroshi Inoue wrote:
>
> Tom Lane wrote:
>
> > "Stephan Szabo" <sszabo(at)kick(dot)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(at)tpf(dot)co(dot)jp

Hannu Krosing wrote:
>
> Hiroshi Inoue wrote:
> >
> > Tom Lane wrote:
> >
> > > "Stephan Szabo" <sszabo(at)kick(dot)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 is DELETE) 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 defined by the privilege
descriptors associated with that <authorization identifier> and
the privileges defined by 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(at)gmx(dot)net 75262 Uppsala
http://yi.org/peter-e/ Sweden

Peter Eisentraut <peter_e(at)gmx(dot)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(at)gmx(dot)net 75262 Uppsala
http://yi.org/peter-e/ Sweden

Peter Eisentraut <peter_e(at)gmx(dot)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