Review of Row Level Security

Patch looks good and also like it will/can be ready for 9.3. I'm happy
to put time into this as committer and/or reviewer and take further
responsibility for it, unless others wish to.

LIKES

* It's pretty simple to understand and use

* Check qual is stored in pre-parsed form. That is fast, and also
secure, since changing search_path of the user doesn't change the
security check at all. Nice.

* Performance overhead is low, integration with indexing is clear and
effective and it works with partitioning

* It works, apart from design holes listed below, easily plugged IMHO

DISLIKEs

* Who gets to see stats on the underlying table? Are the stats
protected by security? How does ANALYZE work?

* INSERT ignores the SECURITY clause, on the ground that this has no
meaning. So its possible to INSERT data you can't see. For example,
you can insert medical records for *another* patient, or insert new
top secret information. This also causes a security hole... since
inserted rows can violate defined constraints, letting you know that
other keys you can't see exist. Why don't we treat the SECURITY clause
as a CHECK constraint? That makes intuitive sense to me.

* UPDATE allows you to bypass the SECURITY clause, to produce new rows
that you can't see. (Not good). But you can't get them back again, cos
you can't see them.

* TRUNCATE works, and allows you to remove all rows of a table, even
ones you can't see to run a DELETE on. Er...

None of those things are cool, at all.

Oracle defaults to putting VPD on all event types: INSERT, UPDATE,
DELETE, SELECT. ISTM we should be doing the same, not just say "we can
add an INSERT trigger if you want".

Adding a trigger just begs the question as to why we are bothering in
the first place, since this functionality could already be added by
INSERT, UPDATE or DELETE triggers, if they are a full replacement for
this feature. The only answer is "ease of use"

We can easily add syntax like this

[ROW SECURITY CHECK ( .... ) [ON [ ALL | INSERT, UPDATE, DELETE, SELECT [..,]]]]

with the default being "ALL"

* The design has nothing at all to do with SECURITY LABELs. Why did we
create them, I wonder? I understood we would have row-level label
security. Doesn't that require us to have a data type, such as
reglabel or similar enum? Seems strange. Oracle has two features:
Oracle Label Security and Row Level Security -

OTHER

* The docs should explain a little better how to optimize using RLS.
Specifically, the fact that indexable operators are marked leakproof
and thus can be optimized ahead of the rlsquals. The docs say "rls
quals are guaranteed to be applied first" which isn't true in all
cases.

* Why is pg_rowlevelsec in a separate catalog table?

* Internally, I think we should call this "rowsecurity" rather than
"rowlevelsec" - the "level" word is just noise, whereas the "security"
word benefits from being spelt out in full.

* psql \d support needed

* Docs need work, but thats OK.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Thanks for your reviewing in spite of large number of lines.

My comments are below.

2012/12/4 Simon Riggs <simon(at)2ndquadrant(dot)com>:
> Patch looks good and also like it will/can be ready for 9.3. I'm happy
> to put time into this as committer and/or reviewer and take further
> responsibility for it, unless others wish to.
>
> LIKES
>
> * It's pretty simple to understand and use
>
> * Check qual is stored in pre-parsed form. That is fast, and also
> secure, since changing search_path of the user doesn't change the
> security check at all. Nice.
>
> * Performance overhead is low, integration with indexing is clear and
> effective and it works with partitioning
>
> * It works, apart from design holes listed below, easily plugged IMHO
>
>
> DISLIKEs
>
> * Who gets to see stats on the underlying table? Are the stats
> protected by security? How does ANALYZE work?
>
I think, ANALYZE should perform on the raw tables without row-security
policy. Even though statistics are "gray-zone", it is not a complete set
of the raw table contents, so all we can do is just implying the original
from processed statistical values. The situation is similar to iteration of
probe using PK/FK violation. In general, it is called covert channel, and
out of the scope in regular access control mechanism (including MAC).
So, I don't think we have special protection on pg_stat even if row-
security is configured.

> * INSERT ignores the SECURITY clause, on the ground that this has no
> meaning. So its possible to INSERT data you can't see. For example,
> you can insert medical records for *another* patient, or insert new
> top secret information. This also causes a security hole... since
> inserted rows can violate defined constraints, letting you know that
> other keys you can't see exist. Why don't we treat the SECURITY clause
> as a CHECK constraint? That makes intuitive sense to me.
>
> * UPDATE allows you to bypass the SECURITY clause, to produce new rows
> that you can't see. (Not good). But you can't get them back again, cos
> you can't see them.
>
The above two comments seems me that you are suggesting to apply
checks on both of scanning rows stage (UPDATE case) and modifying
rows stage (INSERT and UPDATE), to prevent touchable rows getting
gone anywhere.
In the previous discussion, it was suggested we can implement this
feature using before-row trigger. However, I love the idea to support
same row-security policy integrated with CHECK constraint; that kills
individual user's operation to define triggers.
One problem is a case when row-security policy contains SubLink node.
I expect it takes a special case handling, however, also guess not hard
to implement so much.
Let me investigate the code around here.

> * 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.

> None of those things are cool, at all.
>
> Oracle defaults to putting VPD on all event types: INSERT, UPDATE,
> DELETE, SELECT. ISTM we should be doing the same, not just say "we can
> add an INSERT trigger if you want".
>
> Adding a trigger just begs the question as to why we are bothering in
> the first place, since this functionality could already be added by
> INSERT, UPDATE or DELETE triggers, if they are a full replacement for
> this feature. The only answer is "ease of use"
>
> We can easily add syntax like this
>
> [ROW SECURITY CHECK ( .... ) [ON [ ALL | INSERT, UPDATE, DELETE, SELECT [..,]]]]
>
> with the default being "ALL"
>
I think it is flaw of Oracle. :-)
In case when user can define leakable function, it enables to leak contents
of invisible rows at the timing when executor fetch the rows, prior to
modification
stage, even if we allows to configure individual row-security policies
for SELECT
and DELETE or UPDATE commands.
My preference is one policy on a particular table for all the commands.

> * The design has nothing at all to do with SECURITY LABELs. Why did we
> create them, I wonder? I understood we would have row-level label
> security. Doesn't that require us to have a data type, such as
> reglabel or similar enum? Seems strange. Oracle has two features:
> Oracle Label Security and Row Level Security -
>
I think it should be implemented on the next step. It additionally takes
two independent features (1) functionality to inject a column to store
security label at table definition. (2) functionality to assign a default
security label when a new row is inserted.
As Oracle constructs OLS on the top of VPD feature, the base row-
security feature shall be upstreamed first.

> OTHER
>
> * The docs should explain a little better how to optimize using RLS.
> Specifically, the fact that indexable operators are marked leakproof
> and thus can be optimized ahead of the rlsquals. The docs say "rls
> quals are guaranteed to be applied first" which isn't true in all
> cases.
>
Indeed. It should be updated as:
although mechanism guarantees to evaluate this condition earlier
than any other user given condition without LEAKPROOF flag
(that means qualifier can have side-effects, thus it possibly leaks
rows should be invisible.)

> * Why is pg_rowlevelsec in a separate catalog table?
>
To define dependency towards functions, operators or relations being
referenced with SubLinks. If we store row-security policy within pg_class
catalog, here is no way to distinguish a dependency records due to row-
security policy or others, thus it makes problem when user wants to
replace the row-security policy.
Do you have a good idea? If this problem can be solved, I can prefer
an approach to store the policy within pg_class.

> * Internally, I think we should call this "rowsecurity" rather than
> "rowlevelsec" - the "level" word is just noise, whereas the "security"
> word benefits from being spelt out in full.
>
OK, I'll update them.

> * psql \d support needed
>
Are you suggesting to print out full qualifiers of row-security?
Or, a mark to indicate whether row-security is configured, or not?

> * Docs need work, but thats OK.
>
I'd like to want some help with native English speakers.

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

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.

>> * Docs need work, but thats OK.
>>
> I'd like to want some help with native English speakers.

I'll help with that.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 5 December 2012 11:16, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:

>> Oracle defaults to putting VPD on all event types: INSERT, UPDATE,
>> DELETE, SELECT. ISTM we should be doing the same, not just say "we can
>> add an INSERT trigger if you want".
>>
>> Adding a trigger just begs the question as to why we are bothering in
>> the first place, since this functionality could already be added by
>> INSERT, UPDATE or DELETE triggers, if they are a full replacement for
>> this feature. The only answer is "ease of use"
>>
>> We can easily add syntax like this
>>
>> [ROW SECURITY CHECK ( .... ) [ON [ ALL | INSERT, UPDATE, DELETE, SELECT [..,]]]]
>>
>> with the default being "ALL"
>>
> I think it is flaw of Oracle. :-)

Agreed

> In case when user can define leakable function, it enables to leak contents
> of invisible rows at the timing when executor fetch the rows, prior to
> modification
> stage, even if we allows to configure individual row-security policies
> for SELECT
> and DELETE or UPDATE commands.
> My preference is one policy on a particular table for all the commands.

Yes, only one security policy allowed.

Question is, should we offer the option to enforce it on a subset of
command types.

That isn't anything I can see a need for myself.

>> * psql \d support needed
>>
> Are you suggesting to print out full qualifiers of row-security?
> Or, a mark to indicate whether row-security is configured, or not?

One of those options, yes

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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.

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

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:
>
>>> Oracle defaults to putting VPD on all event types: INSERT, UPDATE,
>>> DELETE, SELECT. ISTM we should be doing the same, not just say "we can
>>> add an INSERT trigger if you want".
>>>
>>> Adding a trigger just begs the question as to why we are bothering in
>>> the first place, since this functionality could already be added by
>>> INSERT, UPDATE or DELETE triggers, if they are a full replacement for
>>> this feature. The only answer is "ease of use"
>>>
>>> We can easily add syntax like this
>>>
>>> [ROW SECURITY CHECK ( .... ) [ON [ ALL | INSERT, UPDATE, DELETE, SELECT [..,]]]]
>>>
>>> with the default being "ALL"
>>>
>> I think it is flaw of Oracle. :-)
>
> Agreed
>
>> In case when user can define leakable function, it enables to leak contents
>> of invisible rows at the timing when executor fetch the rows, prior to
>> modification
>> stage, even if we allows to configure individual row-security policies
>> for SELECT
>> and DELETE or UPDATE commands.
>> My preference is one policy on a particular table for all the commands.
>
> Yes, only one security policy allowed.
>
> Question is, should we offer the option to enforce it on a subset of
> command types.
>
> That isn't anything I can see a need for myself.
>
It is not hard to support a feature not to apply security policy on
particular command types, from implementation perspective.
So, my preference is to support only the behavior corresponding
to above "ALL" option, then support per commands basis when
we got strong demands.
How about your thought?

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

On 9 December 2012 06:21, 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:
>>
>>>> Oracle defaults to putting VPD on all event types: INSERT, UPDATE,
>>>> DELETE, SELECT. ISTM we should be doing the same, not just say "we can
>>>> add an INSERT trigger if you want".
>>>>
>>>> Adding a trigger just begs the question as to why we are bothering in
>>>> the first place, since this functionality could already be added by
>>>> INSERT, UPDATE or DELETE triggers, if they are a full replacement for
>>>> this feature. The only answer is "ease of use"
>>>>
>>>> We can easily add syntax like this
>>>>
>>>> [ROW SECURITY CHECK ( .... ) [ON [ ALL | INSERT, UPDATE, DELETE, SELECT [..,]]]]
>>>>
>>>> with the default being "ALL"
>>>>
>>> I think it is flaw of Oracle. :-)
>>
>> Agreed
>>
>>> In case when user can define leakable function, it enables to leak contents
>>> of invisible rows at the timing when executor fetch the rows, prior to
>>> modification
>>> stage, even if we allows to configure individual row-security policies
>>> for SELECT
>>> and DELETE or UPDATE commands.
>>> My preference is one policy on a particular table for all the commands.
>>
>> Yes, only one security policy allowed.
>>
>> Question is, should we offer the option to enforce it on a subset of
>> command types.
>>
>> That isn't anything I can see a need for myself.
>>
> It is not hard to support a feature not to apply security policy on
> particular command types, from implementation perspective.
> So, my preference is to support only the behavior corresponding
> to above "ALL" option, then support per commands basis when
> we got strong demands.
> How about your thought?

Very much agree that ALL should be the default, and only option for
first commit of this feature.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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>

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>

On Tue, Dec 18, 2012 at 3:39 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> 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).

I've argued against this before - and maybe I should drop my
objection, because a number of people seem to be on the other side.
But I still think there will be some people who don't want this
behavior. Right now, for example, you can give someone INSERT but not
SELECT permission on a table, and they will then be able to put rows
into the table that they cannot read back. Similarly, in the RLS
case, it is not necessarily undesirable for a user to be able to
insert a row that they can't read back; or for them to be able to
update a row from a value that they can see to one that they cannot.
Some people will want to prohibit that, while others will not.

Previously, I suggested that we handle this by enforcing row-level
security only on data read from the table - the OLD row, so to speak -
and not on data written to the table - the NEW row, so to speak -
because the latter case can be handled well enough by triggers. (The
OLD case cannot, because not seeing the row is different from erroring
out when you do see it.) There are other alternatives, like allowing
the user to specify which behavior they want. But I think that simply
decreeing that the policy will apply not only to rows read but also
rows written in all cases will be less flexible than we will
ultimately want to be.

YMMV, of course.

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

On 19 December 2012 17:25, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Dec 18, 2012 at 3:39 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> 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).
>
> I've argued against this before - and maybe I should drop my
> objection, because a number of people seem to be on the other side.
> But I still think there will be some people who don't want this
> behavior. Right now, for example, you can give someone INSERT but not
> SELECT permission on a table, and they will then be able to put rows
> into the table that they cannot read back. Similarly, in the RLS
> case, it is not necessarily undesirable for a user to be able to
> insert a row that they can't read back; or for them to be able to
> update a row from a value that they can see to one that they cannot.
> Some people will want to prohibit that, while others will not.

I can see a use case for not having security apply for users who have
*only* INSERT privilege. This would allow people to run bulk loads of
data into a table with row security. We should add that. That is not
the common case, so with proper documentation that should be a useful
feature without relaxing default security.

Never applying security for INSERT and then forcing them to add BEFORE
triggers if they want full security is neither secure nor performant.

> Previously, I suggested that we handle this by enforcing row-level
> security only on data read from the table - the OLD row, so to speak -
> and not on data written to the table - the NEW row, so to speak -
> because the latter case can be handled well enough by triggers. (The
> OLD case cannot, because not seeing the row is different from erroring
> out when you do see it.) There are other alternatives, like allowing
> the user to specify which behavior they want. But I think that simply
> decreeing that the policy will apply not only to rows read but also
> rows written in all cases will be less flexible than we will
> ultimately want to be.

As discussed, we should add a security feature that is secure by
default. Adding options to make it less secure can follow initial
commit. We might even make it in this release if the review of the
main feature goes well.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Dec 18, 2012 at 3:39 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> 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).

> I've argued against this before - and maybe I should drop my
> objection, because a number of people seem to be on the other side.
> But I still think there will be some people who don't want this
> behavior. Right now, for example, you can give someone INSERT but not
> SELECT permission on a table, and they will then be able to put rows
> into the table that they cannot read back.

There is also precedent for your opinion in the spec-mandated behavior
of updatable views: it is perfectly possible to INSERT a row that you
can't read back via the view, or UPDATE it to a state you can't see
via the view. The RLS patch's current behavior corresponds to a view
created WITH CHECK OPTION --- which we don't support yet. Whether
we add that feature soon or not, what seems important for the current
debate is that the SQL spec authors chose not to make it the default
behavior. This seems to weigh heavily against making it the default,
much less only, behavior for RLS cases.

I'd also suggest that "throw an error" is not the only response that
people are likely to want for attempts to insert/update non-compliant
rows, so hard-wiring that choice is insufficiently flexible even if you
grant that local policy is to not allow such updates. (As an example,
they might prefer to log the attempt and substitute some other value.)

> Previously, I suggested that we handle this by enforcing row-level
> security only on data read from the table - the OLD row, so to speak -
> and not on data written to the table - the NEW row, so to speak -
> because the latter case can be handled well enough by triggers.

+1. I'm less than excited about RLS in the first place, so the less
complexity we have to put into the core system for it the better IMO.

regards, tom lane

On 19 December 2012 18:40, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Tue, Dec 18, 2012 at 3:39 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> 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).
>
>> I've argued against this before - and maybe I should drop my
>> objection, because a number of people seem to be on the other side.
>> But I still think there will be some people who don't want this
>> behavior. Right now, for example, you can give someone INSERT but not
>> SELECT permission on a table, and they will then be able to put rows
>> into the table that they cannot read back.
>
> There is also precedent for your opinion in the spec-mandated behavior
> of updatable views: it is perfectly possible to INSERT a row that you
> can't read back via the view, or UPDATE it to a state you can't see
> via the view. The RLS patch's current behavior corresponds to a view
> created WITH CHECK OPTION --- which we don't support yet. Whether
> we add that feature soon or not, what seems important for the current
> debate is that the SQL spec authors chose not to make it the default
> behavior. This seems to weigh heavily against making it the default,
> much less only, behavior for RLS cases.

This is security, not spec compliance. By default, we need full security.

Nobody has argued that it should be the only behaviour, only that it
is the most typically requested behaviour and the most secure,
therefore the one we should do first.

> I'd also suggest that "throw an error" is not the only response that
> people are likely to want for attempts to insert/update non-compliant
> rows, so hard-wiring that choice is insufficiently flexible even if you
> grant that local policy is to not allow such updates. (As an example,
> they might prefer to log the attempt and substitute some other value.)
>
>> Previously, I suggested that we handle this by enforcing row-level
>> security only on data read from the table - the OLD row, so to speak -
>> and not on data written to the table - the NEW row, so to speak -
>> because the latter case can be handled well enough by triggers.
>
> +1. I'm less than excited about RLS in the first place, so the less
> complexity we have to put into the core system for it the better IMO.

Agree with the need for less complexity, but that decision increases
complexity for the typical user and does very little to the complexity
of the patch. Treating a security rule as a check constraint is
natural and obvious, so there are no core system problems here.

If we don't enforce rules on INSERT the user has to specifically add a
trigger, which makes things noticeably slower. There is more
maintenance work for the average user, less performance and more
mistakes to make.

The way to do this is by adding an option to allow users to specify
INSERT should be exempt from the security rule, which Kaigai and I
agreed on list some weeks back should come after the initial patch, to
no other comment.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2012-12-19 18:25, Robert Haas wrote:
> On Tue, Dec 18, 2012 at 3:39 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> 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).
> I've argued against this before - and maybe I should drop my
> objection, because a number of people seem to be on the other side.
> But I still think there will be some people who don't want this
> behavior. Right now, for example, you can give someone INSERT but not
> SELECT permission on a table, and they will then be able to put rows
> into the table that they cannot read back. Similarly, in the RLS
> case, it is not necessarily undesirable for a user to be able to
> insert a row that they can't read back; or for them to be able to
> update a row from a value that they can see to one that they cannot.
> Some people will want to prohibit that, while others will not.

Maybe it is an idea to provide different RLS expressions for read and
write. I remember reading a scenario (it might be well known in security
land) where it is possible to write to authorization levels >= users
level, and read levels <= the users level. In this setup Kevin's address
example is possible, a user could write to e.g. the highest level, but
then not read it anymore if his own level was lower than the highest.
This setup also shows that to implement it, one would need a different
expression for read and write (or the rls expression should know the
query's commandtype).

regards,
Yeb

On Wed, Dec 19, 2012 at 12:54 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> I can see a use case for not having security apply for users who have
> *only* INSERT privilege. This would allow people to run bulk loads of
> data into a table with row security. We should add that. That is not
> the common case, so with proper documentation that should be a useful
> feature without relaxing default security.
>
> Never applying security for INSERT and then forcing them to add BEFORE
> triggers if they want full security is neither secure nor performant.

I think INSERT vs. not-INSERT is not the relevant distinction, because
the question also arises for UPDATE. In the UPDATE case, the question
is whether the RLS qual should be checked only against the OLD tuple
(to make sure that we can see the tuple to modify it) or also against
the NEW tuple (to make sure that we're not modifying it to a form that
we can no longer see). In other words, the question is not "do we
support all of the commands?" but rather "do we check not only the
tuple read but also the tuple written?". For INSERT, we only write a
tuple, without reading. For SELECT and DELETE, we only read a tuple,
without writing a new one. UPDATE does both a read and a write.

>> Previously, I suggested that we handle this by enforcing row-level
>> security only on data read from the table - the OLD row, so to speak -
>> and not on data written to the table - the NEW row, so to speak -
>> because the latter case can be handled well enough by triggers. (The
>> OLD case cannot, because not seeing the row is different from erroring
>> out when you do see it.) There are other alternatives, like allowing
>> the user to specify which behavior they want. But I think that simply
>> decreeing that the policy will apply not only to rows read but also
>> rows written in all cases will be less flexible than we will
>> ultimately want to be.
>
> As discussed, we should add a security feature that is secure by
> default. Adding options to make it less secure can follow initial
> commit. We might even make it in this release if the review of the
> main feature goes well.

Saying that something is or is not secure is not meaningful without
defining what you want to be secure against. There's nothing
"insecure" about checking only the tuples read; it's just a different
(and useful) threat model.

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

On Wed, Dec 19, 2012 at 1:58 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> If we don't enforce rules on INSERT the user has to specifically add a
> trigger, which makes things noticeably slower. There is more
> maintenance work for the average user, less performance and more
> mistakes to make.

Well, again, only if that's the behavior they want.

Also, it's also worth noting that, even if we assume that it is in
fact the behavior that users will want, the contention that it is
faster than a trigger is thus far unsubstantiated by any actual
benchmarks. It may indeed be faster ... but I don't know without
testing whether it's slightly faster or a whole lot faster. That
might be a good thing to find out, because if it is a whole lot
faster, that would certainly strengthen the case for including a mode
that works that way, whether or not we also provide other options.

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

On 20 December 2012 00:24, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Dec 19, 2012 at 12:54 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> I can see a use case for not having security apply for users who have
>> *only* INSERT privilege. This would allow people to run bulk loads of
>> data into a table with row security. We should add that. That is not
>> the common case, so with proper documentation that should be a useful
>> feature without relaxing default security.
>>
>> Never applying security for INSERT and then forcing them to add BEFORE
>> triggers if they want full security is neither secure nor performant.
>
> I think INSERT vs. not-INSERT is not the relevant distinction, because
> the question also arises for UPDATE.

Not sure I understand you. You suggested it was a valid use case for a
user to have only INSERT privilege and wish to bypass security checks.
I agreed and suggested it could be special-cased.

> In the UPDATE case, the question
> is whether the RLS qual should be checked only against the OLD tuple
> (to make sure that we can see the tuple to modify it) or also against
> the NEW tuple (to make sure that we're not modifying it to a form that
> we can no longer see). In other words, the question is not "do we
> support all of the commands?" but rather "do we check not only the
> tuple read but also the tuple written?". For INSERT, we only write a
> tuple, without reading. For SELECT and DELETE, we only read a tuple,
> without writing a new one. UPDATE does both a read and a write.

I'm not sure what this comment adds to the discussion. What you say is
understood.

>>> Previously, I suggested that we handle this by enforcing row-level
>>> security only on data read from the table - the OLD row, so to speak -
>>> and not on data written to the table - the NEW row, so to speak -
>>> because the latter case can be handled well enough by triggers. (The
>>> OLD case cannot, because not seeing the row is different from erroring
>>> out when you do see it.) There are other alternatives, like allowing
>>> the user to specify which behavior they want. But I think that simply
>>> decreeing that the policy will apply not only to rows read but also
>>> rows written in all cases will be less flexible than we will
>>> ultimately want to be.
>>
>> As discussed, we should add a security feature that is secure by
>> default. Adding options to make it less secure can follow initial
>> commit. We might even make it in this release if the review of the
>> main feature goes well.
>
> Saying that something is or is not secure is not meaningful without
> defining what you want to be secure against. There's nothing
> "insecure" about checking only the tuples read; it's just a different
> (and useful) threat model.

There are three main points

* "Applies to all commands" should not be implemented via triggers.
Complex, slow, unacceptable thing to force upon users. Doing that begs
the question of why we would have the feature at all, since we already
have triggers and barrier views.

* the default for row security should be "applies to all commands".
Anything else may be useful in some cases, but is surprising to users
and requires careful thought to determine if it is appropriate.

* How to handle asymmetric row security policies? KaiGai has already
begun discussing problems caused by a security policy that differs
between reads/writes, on his latest patch post. That needs further
analysis to check that it actually makes sense to allow it, since it
is more complex. It would be better to fully analyse that situation
and post solutions, rather than simply argue its OK. Kevin has made
good arguments to show there could be value in such a setup; nobody
has talked about banning it, but we do need analysis, suggested
syntax/mechanisms and extensive documentation to explain it etc.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Dec 20, 2012 at 4:35 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> Not sure I understand you. You suggested it was a valid use case for a
> user to have only INSERT privilege and wish to bypass security checks.
> I agreed and suggested it could be special-cased.

That's not really what I intended to suggest. I view checking an
inserted tuple and checking the new version of an updated tuple as of
a piece. I would think we would check against the RLS quals in either
both of those situations or neither, not one without the other.

> * "Applies to all commands" should not be implemented via triggers.
> Complex, slow, unacceptable thing to force upon users. Doing that begs
> the question of why we would have the feature at all, since we already
> have triggers and barrier views.

I agree that it is questionable whether we need this feature given
that we already have security barrier views. I don't agree that
performing security checks via triggers is unacceptably slow or
complex. Rather, I would say it is flexible and can meet a variety of
needs, unlike this feature, which imposes much tighter constraints on
what you can and cannot check and in which situations.

> * the default for row security should be "applies to all commands".
> Anything else may be useful in some cases, but is surprising to users
> and requires careful thought to determine if it is appropriate.

I (and several other people, it seems) do not agree.

> * How to handle asymmetric row security policies? KaiGai has already
> begun discussing problems caused by a security policy that differs
> between reads/writes, on his latest patch post. That needs further
> analysis to check that it actually makes sense to allow it, since it
> is more complex. It would be better to fully analyse that situation
> and post solutions, rather than simply argue its OK. Kevin has made
> good arguments to show there could be value in such a setup; nobody
> has talked about banning it, but we do need analysis, suggested
> syntax/mechanisms and extensive documentation to explain it etc.

Frankly, in view of your comments above, I am starting to rethink
whether we want this at all. I mean, if you've got security barrier
views, you can check the data being read. If you've got triggers, you
can check the data being written. So what's left? There's something
notationally appealing about being able to apply a security policy to
a table rather than creating a separate view and telling people to use
the view in lieu of the table, but how much is that notational
convenience worth? It has some value from the standpoint of
compatibility with other database products ... but probably not a
whole lot, since all the syntax we're inventing here is
PostgreSQL-specific anyway. Your proposal to check both tuples read
and tuples written might add some value ... but unless there's an
as-yet-undemonstrated performance benefit, it is again mostly a
notational benefit.

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

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> > * "Applies to all commands" should not be implemented via triggers.
> > Complex, slow, unacceptable thing to force upon users. Doing that begs
> > the question of why we would have the feature at all, since we already
> > have triggers and barrier views.

I would rather neither requires writing custom triggers but rather both
are supported through this feature.

> I agree that it is questionable whether we need this feature given
> that we already have security barrier views.

This I don't agree with- the plan has long been to have PG-specific RLS
first and then to support SELinux capabilities on top of it. We didn't
want to have SELinux-specific functionality that couldn't be achieved
without SELinux being involved, and I continue to agree with that.

There are many situations, environments, and individuals that would
view having to implement RLS through views and triggers as being
far-and-away too painful and error-prone to rely on.

Thanks,

Stephen

2012/12/20 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Thu, Dec 20, 2012 at 4:35 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> Not sure I understand you. You suggested it was a valid use case for a
>> user to have only INSERT privilege and wish to bypass security checks.
>> I agreed and suggested it could be special-cased.
>
> That's not really what I intended to suggest. I view checking an
> inserted tuple and checking the new version of an updated tuple as of
> a piece. I would think we would check against the RLS quals in either
> both of those situations or neither, not one without the other.
>
>> * "Applies to all commands" should not be implemented via triggers.
>> Complex, slow, unacceptable thing to force upon users. Doing that begs
>> the question of why we would have the feature at all, since we already
>> have triggers and barrier views.
>
> I agree that it is questionable whether we need this feature given
> that we already have security barrier views. I don't agree that
> performing security checks via triggers is unacceptably slow or
> complex. Rather, I would say it is flexible and can meet a variety of
> needs, unlike this feature, which imposes much tighter constraints on
> what you can and cannot check and in which situations.
>
I'd like to ask Simon which point is more significant; performance
penalty or complex operations by users.
If later, FK constraint is a good example that automatically defines
triggers that applies its checks on inserted tuple and newer version
of updated tuple.
Even though we need to consider how to handle dynamically added
row-security policy by extension (e.g sepgsql), I don't think we need
to enforce users to define triggers for each tables with row-security
as long as system support it.

>> * the default for row security should be "applies to all commands".
>> Anything else may be useful in some cases, but is surprising to users
>> and requires careful thought to determine if it is appropriate.
>
> I (and several other people, it seems) do not agree.
>
>> * How to handle asymmetric row security policies? KaiGai has already
>> begun discussing problems caused by a security policy that differs
>> between reads/writes, on his latest patch post. That needs further
>> analysis to check that it actually makes sense to allow it, since it
>> is more complex. It would be better to fully analyse that situation
>> and post solutions, rather than simply argue its OK. Kevin has made
>> good arguments to show there could be value in such a setup; nobody
>> has talked about banning it, but we do need analysis, suggested
>> syntax/mechanisms and extensive documentation to explain it etc.
>
> Frankly, in view of your comments above, I am starting to rethink
> whether we want this at all. I mean, if you've got security barrier
> views, you can check the data being read. If you've got triggers, you
> can check the data being written. So what's left?
>
In some cases, it is not a reasonable choice to re-define kind of
database objects or its name from what existing application assumes.
It is a reason why we need adaptive security features on regular
tables without or minimum application changes....

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