[v9.3] Row-Level Security

On Thu, Jun 14, 2012 at 11:43 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

In the previous discussion, we planned to add a syntax option to
clarify the command type to fire the RLS policy, such as FOR UPDATE.
But current my opinion is, it is not necessary. For example, we can
reference the contents of rows being updated / deleted using
RETURNING clause. So, it does not make sense to have different
RLS policy at UPDATE / DELETE from SELECT.

I agree. That doesn't make sense, and we don't need to support it.

On the other hand, I'm not 100% sure about my design to restrict
rows to be updated and deleted. Similarly, it expands the target
relation of UPDATE or DELETE statement into a sub-query with
condition. ExecModifyTable() pulls a tuple from the sub-query,
instead of regular table, so it seems to me working at least, but
I didn't try all the possible cases of course.

I don't think there's any reason why that shouldn't work. DELETE ..
USING already allows ModifyTable to be scanning a join product, so
this is not much different.

Of course, here is some limitations, to keep the patch size reasonable
level to review.
- The permission to bypass RLS policy was under discussion.
 If and when we should provide a special permission to bypass RLS
 policy, the "OR has_superuser_privilege()" shall be replaced by
 "OR has_table_privilege(tableoid, 'RLSBYPASS')".

I think you're missing the point. Everyone who has commented on this
issue is in favor of having some check that causes the RLS predicate
*not to get added in the first place*. Adding a modified predicate is
not the same thing. First, the query planner might not be smart
enough to optimize away the clause even when the predicate holds,
causing an unnecessary performance drain. Second, there's too much
danger of being able to set a booby-trap for the superuser this way.
Suppose that the RLS subsystem replaces f_malicious() by f_malicious
OR has_superuser_privilege(). Now the superuser comes along with the
nightly pg_dump run and the query optimizer executes SELECT * FROM
nuts WHERE f_malicious() OR has_superuser_privilege(). The query
optimizer notes that the cost of f_malicious() is very low and decides
to execute that before has_superuser_privilege(). Oops. I think it's
just not acceptable to handle this by clause-munging: we need to not
add the clause in the first place.

Comments on the patch itself:

1. Please do not abbreviate rowlevel to rowlv or RowLevel to RowLv or
ROWLEVEL to ROWLV. That makes it harder to read and harder to grep.
Spell it out.

2. Since the entirety of ATExecSetRowLvSecurity is conditional on
whether clause != NULL, you might as well split it into two functions,
one for each case.

3. The fact that you've had to hack preprocess_targetlist and
adjust_appendrel_attrs_mutator suggests to me that the insertion of
the generate subquery is happening at the wrong phase of the process.
We don't need those special cases for views, so it seems like we
shouldn't need them here, either.

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

2012/6/26 Robert Haas <robertmhaas@gmail.com>:

Of course, here is some limitations, to keep the patch size reasonable
level to review.
- The permission to bypass RLS policy was under discussion.
 If and when we should provide a special permission to bypass RLS
 policy, the "OR has_superuser_privilege()" shall be replaced by
 "OR has_table_privilege(tableoid, 'RLSBYPASS')".

I think you're missing the point.  Everyone who has commented on this
issue is in favor of having some check that causes the RLS predicate
*not to get added in the first place*.  Adding a modified predicate is
not the same thing.  First, the query planner might not be smart
enough to optimize away the clause even when the predicate holds,
causing an unnecessary performance drain.  Second, there's too much
danger of being able to set a booby-trap for the superuser this way.
Suppose that the RLS subsystem replaces f_malicious() by f_malicious
OR has_superuser_privilege().  Now the superuser comes along with the
nightly pg_dump run and the query optimizer executes SELECT * FROM
nuts WHERE f_malicious() OR has_superuser_privilege().  The query
optimizer notes that the cost of f_malicious() is very low and decides
to execute that before has_superuser_privilege().  Oops.  I think it's
just not acceptable to handle this by clause-munging: we need to not
add the clause in the first place.

Here is a simple idea to avoid the second problematic scenario; that
assign 0 as cost of has_superuser_privilege(). I allows to keep this
function more lightweight than any possible malicious function, since
CreateFunction enforces positive value.

But the first point is still remaining.

As you pointed out before, it might be a solution to have case-handling
for superusers and others in case of simple query protocol; that uses
same snapshot for planner and executor stage.

How should we handle the issue?

During the previous discussion, Tom mentioned about an idea that
saves prepared statement hashed with user-id to switch the query
plan depending on user's privilege.
Even though I hesitated to buy this idea at that time, it might be
worth to investigate this idea to satisfy both security and performance;
that will generate multiple query plans to be chosen at executor
stage later.

How about your opinion?

Comments on the patch itself:

1. Please do not abbreviate rowlevel to rowlv or RowLevel to RowLv or
ROWLEVEL to ROWLV.  That makes it harder to read and harder to grep.
Spell it out.

OK,

2. Since the entirety of ATExecSetRowLvSecurity is conditional on
whether clause != NULL, you might as well split it into two functions,
one for each case.

OK,

3. The fact that you've had to hack preprocess_targetlist and
adjust_appendrel_attrs_mutator suggests to me that the insertion of
the generate subquery is happening at the wrong phase of the process.
We don't need those special cases for views, so it seems like we
shouldn't need them here, either.

Main reason why I had to patch them is special case handling for
references to system columns; that is unavailable to have for sub-
queries.
But, I'm not 100% sure around these implementation. So, it needs
more investigations.

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

Kohei KaiGai <kaigai@kaigai.gr.jp> writes:

2012/6/26 Robert Haas <robertmhaas@gmail.com>:

I think you're missing the point. �Everyone who has commented on this
issue is in favor of having some check that causes the RLS predicate
*not to get added in the first place*.

Here is a simple idea to avoid the second problematic scenario; that
assign 0 as cost of has_superuser_privilege().

I am not sure which part of "this isn't safe" isn't getting through to
you. Aside from the scenarios Robert mentioned, consider the
possibility that f_malicious() is marked immutable, so that the planner
is likely to call it (to replace the call with its value) before it will
ever think about whether has_superuser_privilege should be called first.

Please just do what everybody is asking for, and create a bypass that
does not require fragile, easily-broken-by-future-changes assumptions
about what the planner will do with a WHERE clause.

regards, tom lane

2012/6/26 Tom Lane <tgl@sss.pgh.pa.us>:

Kohei KaiGai <kaigai@kaigai.gr.jp> writes:

2012/6/26 Robert Haas <robertmhaas@gmail.com>:

I think you're missing the point.  Everyone who has commented on this
issue is in favor of having some check that causes the RLS predicate
*not to get added in the first place*.

Here is a simple idea to avoid the second problematic scenario; that
assign 0 as cost of has_superuser_privilege().

I am not sure which part of "this isn't safe" isn't getting through to
you.  Aside from the scenarios Robert mentioned, consider the
possibility that f_malicious() is marked immutable, so that the planner
is likely to call it (to replace the call with its value) before it will
ever think about whether has_superuser_privilege should be called first.

Please just do what everybody is asking for, and create a bypass that
does not require fragile, easily-broken-by-future-changes assumptions
about what the planner will do with a WHERE clause.

The problem is the way to implement it.
If we would have permission checks on planner stage, it cannot handle
a case when user-id would be switched prior to executor stage, thus
it needs something remedy to handle the scenario correctly.
Instead of a unique plan per query, it might be a solution to generate
multiple plans depending on user-id, and choose a proper one in
executor stage.

Which type of implementation is what everybody is asking for?

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

On Jun27, 2012, at 07:18 , Kohei KaiGai wrote:

The problem is the way to implement it.
If we would have permission checks on planner stage, it cannot handle
a case when user-id would be switched prior to executor stage, thus
it needs something remedy to handle the scenario correctly.
Instead of a unique plan per query, it might be a solution to generate
multiple plans depending on user-id, and choose a proper one in
executor stage.

Which type of implementation is what everybody is asking for?

I think you need to

a) Determine the user-id at planning time, and insert the matching
RLS clause

b1) Either re-plan the query if the user-id changes between planning
and execution time, which means making the user-id a part of the
plan-cache key.

b2) Or decree that for RLS purposes, it's the user-id at planning time,
not execution time, that counts.

best regards,
Florian Pflug

2012/6/27 Florian Pflug <fgp@phlo.org>:

On Jun27, 2012, at 07:18 , Kohei KaiGai wrote:

The problem is the way to implement it.
If we would have permission checks on planner stage, it cannot handle
a case when user-id would be switched prior to executor stage, thus
it needs something remedy to handle the scenario correctly.
Instead of a unique plan per query, it might be a solution to generate
multiple plans depending on user-id, and choose a proper one in
executor stage.

Which type of implementation is what everybody is asking for?

I think you need to

 a) Determine the user-id at planning time, and insert the matching
   RLS clause

b1) Either re-plan the query if the user-id changes between planning
   and execution time, which means making the user-id a part of the
   plan-cache key.

b2) Or decree that for RLS purposes, it's the user-id at planning time,
   not execution time, that counts.

My preference is b1, because b2 approach takes user visible changes
in concepts of permission checks.

Probably, plan-cache should be also invalidated when user's property
was modified or grant/revoke is issued, in addition to the table itself.

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

On Wed, Jun 27, 2012 at 7:21 AM, Florian Pflug <fgp@phlo.org> wrote:

On Jun27, 2012, at 07:18 , Kohei KaiGai wrote:

The problem is the way to implement it.
If we would have permission checks on planner stage, it cannot handle
a case when user-id would be switched prior to executor stage, thus
it needs something remedy to handle the scenario correctly.
Instead of a unique plan per query, it might be a solution to generate
multiple plans depending on user-id, and choose a proper one in
executor stage.

Which type of implementation is what everybody is asking for?

I think you need to

 a) Determine the user-id at planning time, and insert the matching
   RLS clause

b1) Either re-plan the query if the user-id changes between planning
   and execution time, which means making the user-id a part of the
   plan-cache key.

b2) Or decree that for RLS purposes, it's the user-id at planning time,
   not execution time, that counts.

Or b3, flag plans that depend on the user ID inside the plan-cache,
and just flush all of those (but only those) when the user ID changes.
In the common case where RLS is not used, that might ease the sting.

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

2012/6/27 Robert Haas <robertmhaas@gmail.com>:

On Wed, Jun 27, 2012 at 7:21 AM, Florian Pflug <fgp@phlo.org> wrote:

On Jun27, 2012, at 07:18 , Kohei KaiGai wrote:

The problem is the way to implement it.
If we would have permission checks on planner stage, it cannot handle
a case when user-id would be switched prior to executor stage, thus
it needs something remedy to handle the scenario correctly.
Instead of a unique plan per query, it might be a solution to generate
multiple plans depending on user-id, and choose a proper one in
executor stage.

Which type of implementation is what everybody is asking for?

I think you need to

 a) Determine the user-id at planning time, and insert the matching
   RLS clause

b1) Either re-plan the query if the user-id changes between planning
   and execution time, which means making the user-id a part of the
   plan-cache key.

b2) Or decree that for RLS purposes, it's the user-id at planning time,
   not execution time, that counts.

Or b3, flag plans that depend on the user ID inside the plan-cache,
and just flush all of those (but only those) when the user ID changes.
 In the common case where RLS is not used, that might ease the sting.

Probably, PlannedStmt->invalItems allows to handle invalidation of
plan-cache without big code changes. I'll try to put a flag of user-id
to track the query plan with RLS assumed, or InvalidOid if no RLS
was applied in this plan.
I'll investigate the implementation for more details.

Do we have any other scenario that run a query plan under different
user privilege rather than planner stage?

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

On Jun27, 2012, at 15:07 , Kohei KaiGai wrote:

Probably, PlannedStmt->invalItems allows to handle invalidation of
plan-cache without big code changes. I'll try to put a flag of user-id
to track the query plan with RLS assumed, or InvalidOid if no RLS
was applied in this plan.
I'll investigate the implementation for more details.

Do we have any other scenario that run a query plan under different
user privilege rather than planner stage?

Hm, what happens if a SECURITY DEFINER functions returns a refcursor?

Actually, I wonder how we handle that today. If the executor is
responsible for permission checks, that wouldn't we apply the calling
function's privilege level in that case, at least of the cursor isn't
fetched from in the SECURITY DEFINER function? If I find some time,
I'll check...

best regards,
Florian Pflug

2012/6/27 Florian Pflug <fgp@phlo.org>:

On Jun27, 2012, at 15:07 , Kohei KaiGai wrote:

Probably, PlannedStmt->invalItems allows to handle invalidation of
plan-cache without big code changes. I'll try to put a flag of user-id
to track the query plan with RLS assumed, or InvalidOid if no RLS
was applied in this plan.
I'll investigate the implementation for more details.

Do we have any other scenario that run a query plan under different
user privilege rather than planner stage?

Hm, what happens if a SECURITY DEFINER functions returns a refcursor?

Actually, I wonder how we handle that today. If the executor is
responsible for permission checks, that wouldn't we apply the calling
function's privilege level in that case, at least of the cursor isn't
fetched from in the SECURITY DEFINER function? If I find some time,
I'll check...

My impression is, here is no matter even if SECURITY DEFINER function
returns refcursor.

A SECURITY DEFINER function (or Trusted Procedure on sepgsql, or
Set-UID program on Linux) provides unprivileged users a particular
"limited way" to access protected data. It means owner of the security
definer function admits it is reasonable to show the protected data
as long as unprivileged users access them via the function.

It is same reason why we admit view's access for users who have
privileges on views but unprivileged to underlying tables.

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

Kohei KaiGai <kaigai@kaigai.gr.jp> writes:

2012/6/27 Florian Pflug <fgp@phlo.org>:

Hm, what happens if a SECURITY DEFINER functions returns a refcursor?

My impression is, here is no matter even if SECURITY DEFINER function
returns refcursor.

I think Florian has a point: it *should* work, but *will* it?

I believe it works today, because the executor only applies permissions
checks during query startup. So those checks are executed while still
within the SECURITY DEFINER context, and should behave as expected.
Subsequently, the cursor portal is returned to caller and caller can
execute it to completion, no problem.

However, with RLS security-related checks will happen throughout the
execution of the portal. They might do the wrong thing once the
SECURITY DEFINER function has been exited.

We might need to consider that a portal has a local value of
"current_user", which is kind of a pain, but probably doable.

regards, tom lane

On Jun28, 2012, at 17:29 , Tom Lane wrote:

Kohei KaiGai <kaigai@kaigai.gr.jp> writes:

2012/6/27 Florian Pflug <fgp@phlo.org>:

Hm, what happens if a SECURITY DEFINER functions returns a refcursor?

My impression is, here is no matter even if SECURITY DEFINER function
returns refcursor.

I think Florian has a point: it *should* work, but *will* it?

I believe it works today, because the executor only applies permissions
checks during query startup. So those checks are executed while still
within the SECURITY DEFINER context, and should behave as expected.
Subsequently, the cursor portal is returned to caller and caller can
execute it to completion, no problem.

Don't we (sometimes?) defer query startup to the first time FETCH is
called?

best regards,
Florian Pflug

Florian Pflug <fgp@phlo.org> writes:

On Jun28, 2012, at 17:29 , Tom Lane wrote:

I believe it works today, because the executor only applies permissions
checks during query startup. So those checks are executed while still
within the SECURITY DEFINER context, and should behave as expected.
Subsequently, the cursor portal is returned to caller and caller can
execute it to completion, no problem.

Don't we (sometimes?) defer query startup to the first time FETCH is
called?

There are things inside individual plan node functions that may only
happen when the first row is demanded, but permissions checks are done
in ExecutorStart().

regards, tom lane

2012/6/28 Tom Lane <tgl@sss.pgh.pa.us>:

Kohei KaiGai <kaigai@kaigai.gr.jp> writes:

2012/6/27 Florian Pflug <fgp@phlo.org>:

Hm, what happens if a SECURITY DEFINER functions returns a refcursor?

My impression is, here is no matter even if SECURITY DEFINER function
returns refcursor.

I think Florian has a point: it *should* work, but *will* it?

I believe it works today, because the executor only applies permissions
checks during query startup. So those checks are executed while still
within the SECURITY DEFINER context, and should behave as expected.
Subsequently, the cursor portal is returned to caller and caller can
execute it to completion, no problem.

However, with RLS security-related checks will happen throughout the
execution of the portal. They might do the wrong thing once the
SECURITY DEFINER function has been exited.

I tried the scenario that Florian pointed out.

postgres=# CREATE OR REPLACE FUNCTION f_test(refcursor) RETURNS refcursor
postgres-# SECURITY DEFINER LANGUAGE plpgsql
postgres-# AS 'BEGIN OPEN $1 FOR SELECT current_user, * FROM t1;
RETURN $1; END';
CREATE FUNCTION
postgres=# BEGIN;
BEGIN
postgres=# SELECT f_test('abc');
f_test
--------
abc
(1 row)

postgres=# FETCH abc;
current_user | a | b
--------------+---+-----
kaigai | 1 | aaa
(1 row)

postgres=# SET SESSION AUTHORIZATION alice;
SET
postgres=> FETCH abc;
current_user | a | b
--------------+---+-----
alice | 2 | bbb
(1 row)

postgres=> SET SESSION AUTHORIZATION bob;
SET
postgres=> FETCH abc;
current_user | a | b
--------------+---+-----
bob | 3 | ccc
(1 row)

Indeed, the output of "current_user" depends on the setting of session
authorization, even though it was declared within security definer
functions (thus, its security checks are applied on the privileges of
function owner).

We might need to consider that a portal has a local value of
"current_user", which is kind of a pain, but probably doable.

It seems to me, it is an individual matter to be fixed independent
from RLS feature. How about your opinion?

If we go ahead, an idea to tackle this matter is switch user-id
into saved one in the Portal at the beginning of PortanRun or
PortalRunFetch.

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

The attached patch is a revised version of row-level security feature.

I added a feature to invalidate plan cache if user-id was switched
between planner and optimizer. It enabled to generate more optimized
plan than the previous approach; that adds hardwired "OR superuser()".

Example)
postgres=# PREPARE p1(int) AS SELECT * FROM t1 WHERE x > $1 AND f_leak(y);
PREPARE
postgres=# EXPLAIN (costs off) EXECUTE p1(2);
QUERY PLAN
-----------------------------------
Seq Scan on t1
Filter: (f_leak(y) AND (x > 2))
(2 rows)

postgres=# SET SESSION AUTHORIZATION alice;
SET
postgres=> EXPLAIN (costs off) EXECUTE p1(2);
QUERY PLAN
---------------------------------------------
Subquery Scan on t1
Filter: f_leak(t1.y)
-> Seq Scan on t1
Filter: ((x > 2) AND ((x % 2) = 0))
(4 rows)

On the other hand, I removed support for UPDATE / DELETE commands
in this revision, because I'm still uncertain on the implementation that I
adopted in the previous patch. I believe it helps to keep patch size being
minimum reasonable.
Due to same reason, RLS is not supported on COPY TO command.

According to the Robert's comment, I revised the place to inject
applyRowLevelSecurity(). The reason why it needed to patch on
adjust_appendrel_attrs_mutator() was, we handled expansion from
regular relation to sub-query after expand_inherited_tables().
In this revision, it was moved to the head of sub-query planner.

Even though I added a syscache entry for pg_rowlevelsec catalog,
it was revised to read the catalog on construction of relcache, like
trigger descriptor, because it enables to reduce cost to parse an
expression tree in text format and memory consumption of hash
slot.

This revision adds support on pg_dump, and also adds support
to include SubLinks in the row-level security policy.

Thanks,

2012/7/1 Kohei KaiGai <kaigai@kaigai.gr.jp>:

2012/6/28 Tom Lane <tgl@sss.pgh.pa.us>:

Kohei KaiGai <kaigai@kaigai.gr.jp> writes:

2012/6/27 Florian Pflug <fgp@phlo.org>:

Hm, what happens if a SECURITY DEFINER functions returns a refcursor?

My impression is, here is no matter even if SECURITY DEFINER function
returns refcursor.

I think Florian has a point: it *should* work, but *will* it?

I believe it works today, because the executor only applies permissions
checks during query startup. So those checks are executed while still
within the SECURITY DEFINER context, and should behave as expected.
Subsequently, the cursor portal is returned to caller and caller can
execute it to completion, no problem.

However, with RLS security-related checks will happen throughout the
execution of the portal. They might do the wrong thing once the
SECURITY DEFINER function has been exited.

I tried the scenario that Florian pointed out.

postgres=# CREATE OR REPLACE FUNCTION f_test(refcursor) RETURNS refcursor
postgres-# SECURITY DEFINER LANGUAGE plpgsql
postgres-# AS 'BEGIN OPEN $1 FOR SELECT current_user, * FROM t1;
RETURN $1; END';
CREATE FUNCTION
postgres=# BEGIN;
BEGIN
postgres=# SELECT f_test('abc');
f_test
--------
abc
(1 row)

postgres=# FETCH abc;
current_user | a | b
--------------+---+-----
kaigai | 1 | aaa
(1 row)

postgres=# SET SESSION AUTHORIZATION alice;
SET
postgres=> FETCH abc;
current_user | a | b
--------------+---+-----
alice | 2 | bbb
(1 row)

postgres=> SET SESSION AUTHORIZATION bob;
SET
postgres=> FETCH abc;
current_user | a | b
--------------+---+-----
bob | 3 | ccc
(1 row)

Indeed, the output of "current_user" depends on the setting of session
authorization, even though it was declared within security definer
functions (thus, its security checks are applied on the privileges of
function owner).

We might need to consider that a portal has a local value of
"current_user", which is kind of a pain, but probably doable.

It seems to me, it is an individual matter to be fixed independent
from RLS feature. How about your opinion?

If we go ahead, an idea to tackle this matter is switch user-id
into saved one in the Portal at the beginning of PortanRun or
PortalRunFetch.

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

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

On Sun, Jul 15, 2012 at 5:52 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

The attached patch is a revised version of row-level security feature.

I added a feature to invalidate plan cache if user-id was switched
between planner and optimizer. It enabled to generate more optimized
plan than the previous approach; that adds hardwired "OR superuser()".

Example)
postgres=# PREPARE p1(int) AS SELECT * FROM t1 WHERE x > $1 AND f_leak(y);
PREPARE
postgres=# EXPLAIN (costs off) EXECUTE p1(2);
QUERY PLAN
-----------------------------------
Seq Scan on t1
Filter: (f_leak(y) AND (x > 2))
(2 rows)

postgres=# SET SESSION AUTHORIZATION alice;
SET
postgres=> EXPLAIN (costs off) EXECUTE p1(2);
QUERY PLAN
---------------------------------------------
Subquery Scan on t1
Filter: f_leak(t1.y)
-> Seq Scan on t1
Filter: ((x > 2) AND ((x % 2) = 0))
(4 rows)

On the other hand, I removed support for UPDATE / DELETE commands
in this revision, because I'm still uncertain on the implementation that I
adopted in the previous patch. I believe it helps to keep patch size being
minimum reasonable.
Due to same reason, RLS is not supported on COPY TO command.

According to the Robert's comment, I revised the place to inject
applyRowLevelSecurity(). The reason why it needed to patch on
adjust_appendrel_attrs_mutator() was, we handled expansion from
regular relation to sub-query after expand_inherited_tables().
In this revision, it was moved to the head of sub-query planner.

Even though I added a syscache entry for pg_rowlevelsec catalog,
it was revised to read the catalog on construction of relcache, like
trigger descriptor, because it enables to reduce cost to parse an
expression tree in text format and memory consumption of hash
slot.

This revision adds support on pg_dump, and also adds support
to include SubLinks in the row-level security policy.

This revision is too late for this CommitFest; I've moved it to the
next CommitFest and will look at it then, or hopefully sooner.

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

2012/7/17 Robert Haas <robertmhaas@gmail.com>:

On Sun, Jul 15, 2012 at 5:52 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

The attached patch is a revised version of row-level security feature.

I added a feature to invalidate plan cache if user-id was switched
between planner and optimizer. It enabled to generate more optimized
plan than the previous approach; that adds hardwired "OR superuser()".

Example)
postgres=# PREPARE p1(int) AS SELECT * FROM t1 WHERE x > $1 AND f_leak(y);
PREPARE
postgres=# EXPLAIN (costs off) EXECUTE p1(2);
QUERY PLAN
-----------------------------------
Seq Scan on t1
Filter: (f_leak(y) AND (x > 2))
(2 rows)

postgres=# SET SESSION AUTHORIZATION alice;
SET
postgres=> EXPLAIN (costs off) EXECUTE p1(2);
QUERY PLAN
---------------------------------------------
Subquery Scan on t1
Filter: f_leak(t1.y)
-> Seq Scan on t1
Filter: ((x > 2) AND ((x % 2) = 0))
(4 rows)

On the other hand, I removed support for UPDATE / DELETE commands
in this revision, because I'm still uncertain on the implementation that I
adopted in the previous patch. I believe it helps to keep patch size being
minimum reasonable.
Due to same reason, RLS is not supported on COPY TO command.

According to the Robert's comment, I revised the place to inject
applyRowLevelSecurity(). The reason why it needed to patch on
adjust_appendrel_attrs_mutator() was, we handled expansion from
regular relation to sub-query after expand_inherited_tables().
In this revision, it was moved to the head of sub-query planner.

Even though I added a syscache entry for pg_rowlevelsec catalog,
it was revised to read the catalog on construction of relcache, like
trigger descriptor, because it enables to reduce cost to parse an
expression tree in text format and memory consumption of hash
slot.

This revision adds support on pg_dump, and also adds support
to include SubLinks in the row-level security policy.

This revision is too late for this CommitFest; I've moved it to the
next CommitFest and will look at it then, or hopefully sooner.

It seems to me fair enough. I may be able to add UPDATE /
DELETE support until next commit-fest.

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

On 17 July 2012 05:02, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

2012/7/17 Robert Haas <robertmhaas@gmail.com>:

On Sun, Jul 15, 2012 at 5:52 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

The attached patch is a revised version of row-level security feature.
...
According to the Robert's comment, I revised the place to inject
applyRowLevelSecurity(). The reason why it needed to patch on
adjust_appendrel_attrs_mutator() was, we handled expansion from
regular relation to sub-query after expand_inherited_tables().
In this revision, it was moved to the head of sub-query planner.

Hi,

I had a quick look at this and spotted a problem - certain types of
query are able to bypass the RLS quals. For example:

SELECT * FROM (SELECT * FROM foo) foo;

since the RLS policy doesn't descend into subqueries, and is applied
before they are pulled up into the main query. Similarly for views on
top of tables with RLS, and SRF functions that query a table with RLS
that get inlined.

Also queries using UNION ALL are vulnerable if they end up being
flattened, for example:

SELECT * FROM foo UNION ALL SELECT * FROM foo;

FWIW I recently developed some similar code as part of a patch to
implement automatically updatable views
(http://archives.postgresql.org/pgsql-hackers/2012-08/msg00303.php).
Some parts of that code may be useful, possibly for adding
UPDATE/DELETE support.

Regards,
Dean

2012/9/2 Dean Rasheed <dean.a.rasheed@gmail.com>:

On 17 July 2012 05:02, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

2012/7/17 Robert Haas <robertmhaas@gmail.com>:

On Sun, Jul 15, 2012 at 5:52 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

The attached patch is a revised version of row-level security feature.
...
According to the Robert's comment, I revised the place to inject
applyRowLevelSecurity(). The reason why it needed to patch on
adjust_appendrel_attrs_mutator() was, we handled expansion from
regular relation to sub-query after expand_inherited_tables().
In this revision, it was moved to the head of sub-query planner.

Hi,

I had a quick look at this and spotted a problem - certain types of
query are able to bypass the RLS quals. For example:

SELECT * FROM (SELECT * FROM foo) foo;

since the RLS policy doesn't descend into subqueries, and is applied
before they are pulled up into the main query. Similarly for views on
top of tables with RLS, and SRF functions that query a table with RLS
that get inlined.

Also queries using UNION ALL are vulnerable if they end up being
flattened, for example:

SELECT * FROM foo UNION ALL SELECT * FROM foo;

Thanks for your comment.

Indeed, I missed the case of simple sub-queries and union-all being
pulled up into the main query. So, I adjusted the location to invoke
applyRowLevelSecurity() between all the pull-up stuff and expanding
inherited tables.

The attached patch is a fixed and rebased revision for CF:Sep.

FWIW I recently developed some similar code as part of a patch to
implement automatically updatable views
(http://archives.postgresql.org/pgsql-hackers/2012-08/msg00303.php).
Some parts of that code may be useful, possibly for adding
UPDATE/DELETE support.

Let me check it.

Best regards,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>