Re: [v9.2] Object access hooks with arguments support (v1)

On Tue, Oct 18, 2011 at 1:23 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> If you are suggesting DAC and MAC permissions should be checked
> on the same place like as we already doing at ExecCheckRTPerms(),
> I'd like to agree with the suggestion, rather than all the checks within
> object_access_hook, although it will take code reworking around
> access controls.

Agreed.

> In the example table creation, heap_create_with_catalog() is invoked
> by 5 routines, however, 3 of them are just internal usages, so it is not
> preferable to apply permission checks on table creation....

Some wit once made the remark that if a function has 10 arguments, it
has 11 arguments, meaning that functions with very large numbers of
arguments typically indicate a poor choice of abstraction boundary.
That's pretty much what I think is going on with
heap_create_with_catalog(). I think maybe some refactoring is in
order there, though I am not sure quite what.

> If we may have a hook on table creation next to the place currently
> we checks permission on the namespace being created, it is quite
> helpful to implement sepgsql.

Makes sense.

> BTW, it seems to me SELECT INTO should also check insert permission
> on DAC level, because it become an incorrect assumption that owner of
> table has insert permission on its creation time.
>
> postgres=# ALTER DEFAULT PRIVILEGES FOR USER tak GRANT select ON TABLES TO tak;
> ALTER DEFAULT PRIVILEGES
> postgres=# \ddp
>         Default access privileges
>  Owner | Schema | Type  | Access privileges
> -------+--------+-------+-------------------
>  tak   |        | table | tak=r/tak
> (1 row)
>
> postgres=# SET SESSION AUTHORIZATION tak;
> SET
> postgres=> SELECT * INTO t1 FROM (VALUES(1,'aaa'), (2,'bbb'), (3,'ccc')) AS v;
> SELECT 3
> postgres=> SELECT * FROM t1;
>  column1 | column2
> ---------+---------
>       1 | aaa
>       2 | bbb
>       3 | ccc
> (3 rows)
>
> postgres=> INSERT INTO t1 VALUES (4,'ddd');
> ERROR:  permission denied for relation t1
>
> Oops!

You could make the argument that there's no real security hole there,
because the user could give himself those same privileges right back.
You could also make the argument that a SELECT .. INTO is not the same
as an INSERT and therefore INSERT privileges shouldn't be checked. I
think there are valid arguments on both sides, but my main point is
that we shouldn't have core do it one way and sepgsql do it the other
way. That's going to be impossible to maintain and doesn't really
make any logical sense anyway.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company