Re: Review of Row Level Security

This attached patch is revised version of row-security.

As I don't call it row-level-security, the feature is renamed to row-security,
thus, its syntax, catalog name, source files, ... are also renamed:

The new syntax is below:
ALTER TABLE xxx SET ROW SECURITY (<expression>);
ALTER TABLE xxx RESET ROW SECURITY;

Most significant change is the configured row-security policy is also
checked just before insertion or update of newer tuple, as follows.

postgres=> CREATE TABLE t1 (a int, b text);
CREATE TABLE
postgres=> CREATE TABLE t2 (x int, y text);
CREATE TABLE
postgres=> INSERT INTO t1 VALUES (1,'aaa'),(2,'bbb'),(3,'ccc');
INSERT 0 3
postgres=> INSERT INTO t2 VALUES (2,'xxx'),(3,'yyy'),(4,'zzz');
INSERT 0 3
postgres=> ALTER TABLE t1 SET ROW SECURITY (a in (SELECT x FROM t2));
ALTER TABLE
postgres=> SELECT * FROM t1;
a | b
---+-----
2 | bbb
3 | ccc
(2 rows)

postgres=> INSERT INTO t1 VALUES (4,'ddd');
INSERT 0 1
postgres=> INSERT INTO t1 VALUES (5,'eee');
ERROR: new row for relation "t1" violates row-secirity
DETAIL: Failing row contains (5, eee).

postgres=> UPDATE t1 SET a = a + 1;
ERROR: new row for relation "t1" violates row-secirity
DETAIL: Failing row contains (5, ddd).
postgres=> UPDATE t1 SET a = a + 1 WHERE b = 'ccc';
UPDATE 1

The configured row-security policy requires t1.a has to exist in t2.x,
thus, possible value is either 2, 3 or 4.
The first INSERT is OK, but second one violates.
Also, the first UPDATE gets violated when it tried to update the row
of (4,'ddd').

Also, \d+ command was extended to show the configured row-security policy.

postgres=> \d+
List of relations
Schema | Name | Type | Owner | Size | Description | Row-security
--------+------+-------+-------+-------+-------------+------------------------------
public | t1 | table | alice | 16 kB | | (a IN (SELECT
t2.x FROM t2))
public | t2 | table | alice | 16 kB | |
(2 rows)

According to the upthread discussion, I didn't touch the code around
TRUNCATE command due the nature of separated permission.

While I had code revising, I got some ideas to the issues Simon pointed out.

* row-security policy per command type.
If we can set arbitrary row-security policy towards each command type,
it may allow to leak contents of rows to be invisible, using DELETE ...
RETURNING * for example.
Origin of the problem was that row-security of UPDATE or DELETE
can be laxer than SELECT, thus, these commands can leak them.
So, if row-security policy of writer-side implies the one of reader-side,
it never become a problem.
For example, if reader's row-security policy is (uname = current_user)
and writer's one is (permission = 'w'), it is not difficult to combine them
using AND, as (uname = current_user AND permission = 'w').
A problem is, it may take redundant calculation if user gives very
complex expression on both of reader and writer commands but these
are different at a very tiny point.

* reference to statistics catalogs.
Once ANALYZE collect samples from the table with row-security policy,
the statistical data lost where this value come from. Thus, it is not
possible to apply checks individual elements of them.
I think, only possible way is to hide these values on view definition,
and a SQL function to indicate whether row-security policy is
configured, or not. If we have, it can be used on pg_stat definition
to hide raw statistical values.

Thanks,

2012/12/9 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2012/12/9 Simon Riggs <simon(at)2ndquadrant(dot)com>:
>> On 9 December 2012 06:08, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> 2012/12/7 Simon Riggs <simon(at)2ndquadrant(dot)com>:
>>>> On 5 December 2012 11:16, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>>>
>>>>>> * TRUNCATE works, and allows you to remove all rows of a table, even
>>>>>> ones you can't see to run a DELETE on. Er...
>>>>>>
>>>>> It was my oversight. My preference is to rewrite TRUNCATE command
>>>>> with DELETE statement in case when row-security policy is active on
>>>>> the target table.
>>>>> In this case, a NOTICE message may be helpful for users not to assume
>>>>> the table is always empty after the command.
>>>>
>>>> I think the default must be to throw an ERROR, since part of the
>>>> contract with TRUNCATE is that it is fast and removes storage.
>>>>
>>> OK. Does the default imply you are suggesting configurable
>>> behavior using GUC or something?
>>> I think both of the behaviors are reasonable from security point
>>> of view, as long as user cannot remove unprivileged rows.
>>
>> Hmm, its difficult one that. I guess this raises the question as to
>> whether users know they are accessing a table with RLS enabled. If
>> they don't and we want to keep it that way, then changing TRUNCATE
>> into DELETE makes sense.
>>
>> To issue TRUNCATE you need the correct privilege, which is separate from DELETE.
>>
>> If they have TRUNCATE privilege they should be allowed to remove all
>> rows, bypassing the row level security.
>>
>> If that behavious isn't wanted, then the table owner can create an
>> INSTEAD OF TRUNCATE trigger that turns the action into a DELETE, which
>> is then subject to RLS rules.
>>
> It seems to me make sense, also.
> Even though selinux does not define separated permissions for TRUNCATE,
> the later option will work well for me in case of row-level label based security
> is configured in the future version.
> So, I don't implement something special around TRUNCATE, except for
> paying mention at the documentation.
>
> Thanks,
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>