Review of Row Level Security

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

My comments are below.

2012/12/4 Simon Riggs <simon@2ndquadrant.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@kaigai.gr.jp>

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 5 December 2012 11:16, Kohei KaiGai <kaigai@kaigai.gr.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 5 December 2012 11:16, Kohei KaiGai <kaigai@kaigai.gr.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2012/12/7 Simon Riggs <simon@2ndquadrant.com>:

On 5 December 2012 11:16, Kohei KaiGai <kaigai@kaigai.gr.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@kaigai.gr.jp>

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2012/12/7 Simon Riggs <simon@2ndquadrant.com>:

On 5 December 2012 11:16, Kohei KaiGai <kaigai@kaigai.gr.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@kaigai.gr.jp>

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 9 December 2012 06:21, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

2012/12/7 Simon Riggs <simon@2ndquadrant.com>:

On 5 December 2012 11:16, Kohei KaiGai <kaigai@kaigai.gr.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 9 December 2012 06:08, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

2012/12/7 Simon Riggs <simon@2ndquadrant.com>:

On 5 December 2012 11:16, Kohei KaiGai <kaigai@kaigai.gr.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2012/12/9 Simon Riggs <simon@2ndquadrant.com>:

On 9 December 2012 06:08, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

2012/12/7 Simon Riggs <simon@2ndquadrant.com>:

On 5 December 2012 11:16, Kohei KaiGai <kaigai@kaigai.gr.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@kaigai.gr.jp>

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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@kaigai.gr.jp>:

2012/12/9 Simon Riggs <simon@2ndquadrant.com>:

On 9 December 2012 06:08, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

2012/12/7 Simon Riggs <simon@2ndquadrant.com>:

On 5 December 2012 11:16, Kohei KaiGai <kaigai@kaigai.gr.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@kaigai.gr.jp>

--
KaiGai Kohei <kaigai@kaigai.gr.jp>

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 19 December 2012 17:25, Robert Haas <robertmhaas@gmail.com> wrote:

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 19 December 2012 18:40, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Simon Riggs wrote:

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

But you are arguing that users should not be able to make something
secure if they, and everyone with the same permissions, could not
later access it. I thought about situations where I've seen a need
for something like this, and probably the best fit that I've seen
is the ability of a judge to order that something is sealed. There
are various levels where that can happen, but I'll focus on just
one which Wisconsin Courts have implemented at the application
level, but which would be nice to be able to support at the
database level.

Let's say we're talking about Milwaukee County, where hundreds of
people work for the courts and related organizations with some
rights to view the court data. Let's say a battered wife has moved
to a new address she wants to keep secret for safety. She files a
case with the court for a temporary restraining order, prohibiting
the husband from coming near her. The court needs her address for
the official record, but the judge will order the address "sealed"
so that only people with a certain authority can see it. The
authority is very limited, for obvious reasons.

It is quite likely that the person initially entering the address
and flagging it as sealed will not have authority to see the
address if they go back and look up the case. Neither will the
dozens of other people making the same kind of entries in the
county. Obviously, if the person doing the initial entry is a
friend of the husband, the data is compromised; but not allowing
entry of the data in a state sealed by people without authority to
look it up exposes the data to every other person with entry
authority, with fairly obvious risks.

The more secure behavior is to allow entry of data which will not
be visible by the person doing the entry.

-Kevin

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 19 December 2012 19:46, Kevin Grittner <kgrittn@mail.com> wrote:

But you are arguing that users should not be able to make something
secure if they, and everyone with the same permissions, could not
later access it.

Not exactly, no.

I've argued that row security should apply to ALL commands by default.
Which is exactly the same default as Oracle, as well as being the
obvious common sense understanding of "row security", which does not
cause a POLA violation.

I have no objection to an option to allow row security to not apply to
inserts, if people want that.

I do object to the idea that row security for inserts/updates should
only happen via triggers, which is an ugly and non-performant route,
as well as complicating security.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2012-12-19 14:46:18 -0500, Kevin Grittner wrote:

Simon Riggs wrote:

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

But you are arguing that users should not be able to make something
secure if they, and everyone with the same permissions, could not
later access it. I thought about situations where I've seen a need
for something like this, and probably the best fit that I've seen
is the ability of a judge to order that something is sealed. There
are various levels where that can happen, but I'll focus on just
one which Wisconsin Courts have implemented at the application
level, but which would be nice to be able to support at the
database level.

Let's say we're talking about Milwaukee County, where hundreds of
people work for the courts and related organizations with some
rights to view the court data. Let's say a battered wife has moved
to a new address she wants to keep secret for safety. She files a
case with the court for a temporary restraining order, prohibiting
the husband from coming near her. The court needs her address for
the official record, but the judge will order the address "sealed"
so that only people with a certain authority can see it. The
authority is very limited, for obvious reasons.

It is quite likely that the person initially entering the address
and flagging it as sealed will not have authority to see the
address if they go back and look up the case. Neither will the
dozens of other people making the same kind of entries in the
county. Obviously, if the person doing the initial entry is a
friend of the husband, the data is compromised; but not allowing
entry of the data in a state sealed by people without authority to
look it up exposes the data to every other person with entry
authority, with fairly obvious risks.

The more secure behavior is to allow entry of data which will not
be visible by the person doing the entry.

I don't think it is that simple. Allowing inserts without regard for row
level restrictions makes it far easier to probe for data. E.g. by
inserting rows and checking for unique violations.

Greetings,

Andres Freund

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Simon Riggs wrote:

I've argued that row security should apply to ALL commands by default.
Which is exactly the same default as Oracle, as well as being the
obvious common sense understanding of "row security", which does not
cause a POLA violation.

I have no objection to an option to allow row security to not apply to
inserts, if people want that.

I do object to the idea that row security for inserts/updates should
only happen via triggers, which is an ugly and non-performant route,
as well as complicating security.

In the software where I've seen this managed at an application
level, an address (for example) can be sealed by an update to the
"sealed" column or inserted with the sealed column set to true. I'm
not sure why you would want to allow one and not the other.

Now, I can envision a situation where it could make sense to use
the same predicate for limiting what a role could read and what a
role could write, but I'm not buying that that is more secure. In
fact, I see cases where you cannot achieve decent security without
the ability for both INSERT and UPDATE to write rows which security
excludes on reads. Functionally, I don't see anything which can't
be accomplished with just the read security and triggers.

I do agree that it would be nice if there were an easy way to
specify that the same predicate applies to both reads and writes.
I hope we can leave the syntax for this feature open to such
specification, even if the initial implementation only supports
limiting reads.

-Kevin

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andres Freund wrote:

I don't think it is that simple. Allowing inserts without regard for row
level restrictions makes it far easier to probe for data. E.g. by
inserting rows and checking for unique violations.

Unless you want to go to a military-style security level system
where people at each security level have a separate version of the
same data, primary keys (and I think other unique constraints) can
leak. It seems clear enough that sensitive data should not be used
for such constraints.

That doesn't even require completely meaningless numeric keys.
Court cases in Wisconsin have been numbered within county by year,
case type, a sequential portion, and an optional apha suffix since
before things were computerized -- you may know that there is a
2010 case in Dane County for mental commitment number 45, but that
doesn't leak any sensitive data.

In the sealed address example, if that were moved to the database
layer, someone might be able to test whether addess number 5
existed on a case by seeing whether an insert succeeded; but if it
did, there would be a record of that insert with their user ID that
they could not retract in any way -- they would know very little,
and be exposed as having done something inappropriate.

-Kevin

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

The more secure behavior is to allow entry of data which will not be
visible by the person doing the entry.

I don't think it is that simple. Allowing inserts without regard for row

level

restrictions makes it far easier to probe for data. E.g. by inserting rows

and

checking for unique violations.

So the PK column(s) are not as secure as, say, the address-related column.
Vice-versa I may know that someone lives at a given address (because my
attempt to place someone else there failed) but I would have no way of
knowing who that other person is. My recourse would be to escalate the
data-entry request to someone with higher security permissions who could
read and write to the appropriate tables and resolve the conflict. In both
cases the direct write-only situation necessitates that some level of
exposure occurs. The work-around if that is unacceptable would be to accept
all data but any entries that cannot be directly inserted into the table
would remain in a staging area that someone with higher security would have
to monitor and clear as needed. The same intervention is required but in
the first situation you can at least avoid coding the special logic and
instead trade security for ease-of-use.

As a default level of security we could throw a generic "secure DLL rejected
for ROW(...)" and not tell the user anything about the cause. If that
person knows all unique indexes and constraints defined on the table they
could use trial-and-error to discover information about stored records but
even then if they get an error on two different columns they still have no
way of knowing if those "errors" belong to the same record.

Beyond that level you provide the user with some information as to the cause
so that they have a reasonable chance to catch typos and other mistakes
instead of escalating an benign issue.

Lastly is the custom solution whereby the developers accept ALL data entered
as being correct but saved to a staging table. A review process by someone
with higher security clearances would then process and clear out that table
as necessary. If the user is write-only then regardless of whether the
entry succeeded or failed they are considered to be "done" with their task
at that point and no meaningful results from the system can be supplied to
them.

None of these options disallows the presence of non-security related check
constraints to be checked, enforced, and communicated to the user.

I've probably lost sight of the bigger picture as my response to mostly
informed by these last couple of messages.

David J.

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers