[RFC] Interface of Row Level Security

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

Let me have a discussion to get preferable interface for row-level security.
My planned feature will perform to append additional conditions to WHERE
clause implicitly, to restrict tuples being visible for the current user.
For example, when row-level policy "uname = getpgusername()" is configured
on the table T1, the following query:
select * from T1 where X > 20;
should be rewritten to:
select * from T1 where (X > 20) AND (uname = getpgusername());

Hm. Simple and fairly noninvasive, but ... would this not be subject to
the same sorts of information-leak hazards that were addressed in the
"security views" feature? That is, I see no guarantee that the RLS
condition will be evaluated before any conditions supplied by the user.
So it seems easy to get information out of rows the RLS policy is
supposed to prevent access to. It would be far more secure to just
use a security view to apply the RLS condition.

Also, if the point here is to provide security for tables not views,
it seems like you really need to have (at least a design for) RLS
security on insert/update/delete operations. Just adding the same
filter condition might be adequate for deletes, but I don't think it
works at all for inserts. And for updates, what prevents the user from
updating columns he shouldn't, or updating them to key values he
shouldn't be able to use?

regards, tom lane

On Wed, May 23, 2012 at 5:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

Let me have a discussion to get preferable interface for row-level security.
My planned feature will perform to append additional conditions to WHERE
clause implicitly, to restrict tuples being visible for the current user.
For example, when row-level policy "uname = getpgusername()" is configured
on the table T1, the following query:
    select * from T1 where X > 20;
should be rewritten to:
    select * from T1 where (X > 20) AND (uname = getpgusername());

Hm.  Simple and fairly noninvasive, but ... would this not be subject to
the same sorts of information-leak hazards that were addressed in the
"security views" feature?  That is, I see no guarantee that the RLS
condition will be evaluated before any conditions supplied by the user.
So it seems easy to get information out of rows the RLS policy is
supposed to prevent access to.  It would be far more secure to just
use a security view to apply the RLS condition.

Since adding a condition to the where clause is a relatively simple
operation (compared to the full potential scope of a view) could the
RLS rewrite of the query create a CTE with the additional condition[s]
rather than adding condition[s] to the user-supplied query? This would
provide the forced ordering of the evaluating the conditions, thereby
avoiding many of the potential points of leakage.

Bell.

2012/5/23 Tom Lane <tgl@sss.pgh.pa.us>:

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

Let me have a discussion to get preferable interface for row-level security.
My planned feature will perform to append additional conditions to WHERE
clause implicitly, to restrict tuples being visible for the current user.
For example, when row-level policy "uname = getpgusername()" is configured
on the table T1, the following query:
    select * from T1 where X > 20;
should be rewritten to:
    select * from T1 where (X > 20) AND (uname = getpgusername());

Hm.  Simple and fairly noninvasive, but ... would this not be subject to
the same sorts of information-leak hazards that were addressed in the
"security views" feature?  That is, I see no guarantee that the RLS
condition will be evaluated before any conditions supplied by the user.
So it seems easy to get information out of rows the RLS policy is
supposed to prevent access to.  It would be far more secure to just
use a security view to apply the RLS condition.

I wanted to have discussion to handle this problem.

Unlike leaky-view problem, we don't need to worry about unexpected
qualifier distribution on either side of join, because a scan on table
never contains any join. Thus, all we need to care about is order of
qualifiers chained on a particular scan node; being reordered by
the cost to invoke functions.

How about an idea to track FuncExpr come from the security policy
and enforce "0" on its cost? Regular functions never reach zero
cost, so the security policy must be re-ordered to the head.

Also, if the point here is to provide security for tables not views,
it seems like you really need to have (at least a design for) RLS
security on insert/update/delete operations.  Just adding the same
filter condition might be adequate for deletes, but I don't think it
works at all for inserts.  And for updates, what prevents the user from
updating columns he shouldn't, or updating them to key values he
shouldn't be able to use?

If we also apply the security policy to newer version of tuples on
update and insert, one idea is to inject a before-row-(update|insert)
trigger to check whether it satisfies the security policy.
For same reason, the trigger should be executed at the end of
trigger chain.

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

2012/5/23 Alastair Turner <bell@ctrlf5.co.za>:

On Wed, May 23, 2012 at 5:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

Let me have a discussion to get preferable interface for row-level security.
My planned feature will perform to append additional conditions to WHERE
clause implicitly, to restrict tuples being visible for the current user.
For example, when row-level policy "uname = getpgusername()" is configured
on the table T1, the following query:
    select * from T1 where X > 20;
should be rewritten to:
    select * from T1 where (X > 20) AND (uname = getpgusername());

Hm.  Simple and fairly noninvasive, but ... would this not be subject to
the same sorts of information-leak hazards that were addressed in the
"security views" feature?  That is, I see no guarantee that the RLS
condition will be evaluated before any conditions supplied by the user.
So it seems easy to get information out of rows the RLS policy is
supposed to prevent access to.  It would be far more secure to just
use a security view to apply the RLS condition.

Since adding a condition to the where clause is a relatively simple
operation (compared to the full potential scope of a view) could the
RLS rewrite of the query create a CTE with the additional condition[s]
rather than adding condition[s] to the user-supplied query? This would
provide the forced ordering of the evaluating the conditions, thereby
avoiding many of the potential points of leakage.

An interesting idea. However, I cannot imagine how does it works on
update or delete statement.

For select statement, it will get better performance to rewrite reference
to a particular table by a subquery with security_barrier flag than CTE,
because it allows to push down leakproof functions.

Could you tell me your idea for more details?
An example will help me understand well.

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

On Wed, May 23, 2012 at 3:45 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

I wanted to have discussion to handle this problem.

Unlike leaky-view problem, we don't need to worry about unexpected
qualifier distribution on either side of join, because a scan on table
never contains any join. Thus, all we need to care about is order of
qualifiers chained on a particular scan node; being reordered by
the cost to invoke functions.

How about an idea to track FuncExpr come from the security policy
and enforce "0" on its cost? Regular functions never reach zero
cost, so the security policy must be re-ordered to the head.

Hmm. That would disregard the relative costs of multiple qualifiers
all of which were injected by the security policy, which I suspect is
not a good idea. Furthermore, I think that we should not assume that
there is no join involved. I would expect a fairly popular RLS qual
to be something of the form "WHERE NOT EXISTS (SELECT 1 FROM hide_me
WHERE hide_me.pk = thistab.pk)". Perhaps when we see that RLS
applies, we should replace the reference to the original table with a
subquery RTE that has the security_barrier flag set - essentially
treating a table with RLS as if it were a security view.

Also, suppose that Bob applies an RLS policy to a table, and, later,
Alice selects from the table. How do we keep Bob from usurping
Alice's privileges? If we insist that Bob's RLS policy function runs
as Bob, then it defeats inlining; but if it runs as Alice, then Bob
can steal Alice's credentials. One idea is to apply the security
policy only if Alice's access to the table is granted by Bob. That
way, if Alice is (for example) the superuser, she's immune to RLS.
But that doesn't seem to completely solve the problem, because Alice
might merely be some other relatively unprivileged user and we still
don't want Bob to be able to walk off with her access.

Another idea is to set things up so that the RLS policy function isn't
applied to each row directly; instead, it's invoked once per query and
*returns* a WHERE clause. This would be a lot more powerful than the
proposed design, because now the table owner can write a function that
imposes quals on some people but not others, which seems very useful.

Also, if the point here is to provide security for tables not views,
it seems like you really need to have (at least a design for) RLS
security on insert/update/delete operations.  Just adding the same
filter condition might be adequate for deletes, but I don't think it
works at all for inserts.  And for updates, what prevents the user from
updating columns he shouldn't, or updating them to key values he
shouldn't be able to use?

If we also apply the security policy to newer version of tuples on
update and insert, one idea is to inject a before-row-(update|insert)
trigger to check whether it satisfies the security policy.
For same reason, the trigger should be executed at the end of
trigger chain.

It's not clear to me that there is any need for built-in server
functionality here. If the table owner wants to enforce some sort of
policy regarding INSERT or UPDATE or DELETE, they can already do that
today just by attaching a trigger to the table. And they can enforce
whatever policy they like that way. Before designing any new
mechanism, what's wrong with the existing one?

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

2012/5/23 Robert Haas <robertmhaas@gmail.com>:

On Wed, May 23, 2012 at 3:45 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

I wanted to have discussion to handle this problem.

Unlike leaky-view problem, we don't need to worry about unexpected
qualifier distribution on either side of join, because a scan on table
never contains any join. Thus, all we need to care about is order of
qualifiers chained on a particular scan node; being reordered by
the cost to invoke functions.

How about an idea to track FuncExpr come from the security policy
and enforce "0" on its cost? Regular functions never reach zero
cost, so the security policy must be re-ordered to the head.

Hmm.  That would disregard the relative costs of multiple qualifiers
all of which were injected by the security policy, which I suspect is
not a good idea.
 Furthermore, I think that we should not assume that
there is no join involved.  I would expect a fairly popular RLS qual
to be something of the form "WHERE NOT EXISTS (SELECT 1 FROM hide_me
WHERE hide_me.pk = thistab.pk)".

Please ignore the approach to track cost value of qualifiers.
I believe it does not work well without something fundamental updates.

Perhaps when we see that RLS
applies, we should replace the reference to the original table with a
subquery RTE that has the security_barrier flag set - essentially
treating a table with RLS as if it were a security view.

I become to think it is a better approach than tracking origin of each
qualifiers. One problem is case handling on update or delete statement.

It may be possible to rewrite the update / delete query as follows:

From:
UPDATE tbl SET X = X + 1 WHERE f_leak(Y)
To:
UPDATE tbl SET X = X + 1 WHERE ctid = (
SELECT * FROM (
SELECT ctid FROM tbl WHERE uname = getpgusername() <== (*)
should have security-barrier
) AS tbl_subqry WHERE f_leak(Y)
);

Expanded sub-queries will have security-barrier flag, so it enforces
the "uname = getpgusername()" being checked earlier than f_leak(Y).
We may need to measure the performance impact due to the reform.

Also, suppose that Bob applies an RLS policy to a table, and, later,
Alice selects from the table.  How do we keep Bob from usurping
Alice's privileges?  If we insist that Bob's RLS policy function runs
as Bob, then it defeats inlining; but if it runs as Alice, then Bob
can steal Alice's credentials.  One idea is to apply the security
policy only if Alice's access to the table is granted by Bob.  That
way, if Alice is (for example) the superuser, she's immune to RLS.
But that doesn't seem to completely solve the problem, because Alice
might merely be some other relatively unprivileged user and we still
don't want Bob to be able to walk off with her access.

I think, this situation is similar to a case when we reference a view
without privileges to underlying tables. If Bob set up a view with
something "tricky" function, it allows Bob to reference credentials
of users who reference the view.
More or less, it might be a problem when a user try to invoke
a user defined function declared by others.
(Thus, sepgsql policy does not allow users to invoke a function
declared by another one in different domain; without DBA's checks.)

I think it is a good idea not to apply RLS when current user has
superuser privilege from perspective of security model consistency,
but it is inconsistent to check privileges underlying tables.

Another idea is to set things up so that the RLS policy function isn't
applied to each row directly; instead, it's invoked once per query and
*returns* a WHERE clause.  This would be a lot more powerful than the
proposed design, because now the table owner can write a function that
imposes quals on some people but not others, which seems very useful.

Sorry, I don't favor this idea. Even if table owner set up a function to
generate additional qualifiers, it also has no guarantee the qualifiers
are invoked prior to user-given one.
It seems to me this approach will have same problem...

Also, if the point here is to provide security for tables not views,
it seems like you really need to have (at least a design for) RLS
security on insert/update/delete operations.  Just adding the same
filter condition might be adequate for deletes, but I don't think it
works at all for inserts.  And for updates, what prevents the user from
updating columns he shouldn't, or updating them to key values he
shouldn't be able to use?

If we also apply the security policy to newer version of tuples on
update and insert, one idea is to inject a before-row-(update|insert)
trigger to check whether it satisfies the security policy.
For same reason, the trigger should be executed at the end of
trigger chain.

It's not clear to me that there is any need for built-in server
functionality here.  If the table owner wants to enforce some sort of
policy regarding INSERT or UPDATE or DELETE, they can already do that
today just by attaching a trigger to the table.  And they can enforce
whatever policy they like that way.  Before designing any new
mechanism, what's wrong with the existing one?

Yes, we don't need any new invent to check the value of new tuples.
But it should be done after all the user-defined triggers. Existing
trigger does not have a mechanism to enforce order to be invoked.
So, what I really implement is a mechanism to inject some pseudo
triggers "at tail of the Trigger array".

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

On May23, 2012, at 22:12 , Robert Haas wrote:

Also, suppose that Bob applies an RLS policy to a table, and, later,
Alice selects from the table. How do we keep Bob from usurping
Alice's privileges?

That problem isn't restricted to RLW, though. Bob could just as well
have booby-trapped any other SQL object, e.g. could have added
bobs_malicious_function() to a view Alice selects from, or attached
it as a trigger to a table Alice inserts to.

It would be nice if there was (optional) protection against these
kinds of attacks, but it's not really something that RLS should be
burdened with.

BTW, I've wondered in the past how tight our protection against some
trying to take over the postgres role during pg_dump is. On the surface,
it seems that we're safe because pg_dump doesn't invoke user-defined
functions except output functions (which require superuser privileges
to modify). But that's not exactly a solid line of defense...

If we also apply the security policy to newer version of tuples on
update and insert, one idea is to inject a before-row-(update|insert)
trigger to check whether it satisfies the security policy.
For same reason, the trigger should be executed at the end of
trigger chain.

It's not clear to me that there is any need for built-in server
functionality here. If the table owner wants to enforce some sort of
policy regarding INSERT or UPDATE or DELETE, they can already do that
today just by attaching a trigger to the table. And they can enforce
whatever policy they like that way. Before designing any new
mechanism, what's wrong with the existing one?

Yeah, applying the security policy to the new row (for UPDATES
and INSERTS) seems weird - the policy determines what you can see,
not what you can store, which might be two different things.

But the security policy should still apply to the old rows, i.e.
you shouldn't be after to UPDATE or DELETE rows you cannot see, no?

best regards,
Florian Pflug

2012/5/24 Florian Pflug <fgp@phlo.org>:

If we also apply the security policy to newer version of tuples on
update and insert, one idea is to inject a before-row-(update|insert)
trigger to check whether it satisfies the security policy.
For same reason, the trigger should be executed at the end of
trigger chain.

It's not clear to me that there is any need for built-in server
functionality here.  If the table owner wants to enforce some sort of
policy regarding INSERT or UPDATE or DELETE, they can already do that
today just by attaching a trigger to the table.  And they can enforce
whatever policy they like that way.  Before designing any new
mechanism, what's wrong with the existing one?

Yeah, applying the security policy to the new row (for UPDATES
and INSERTS) seems weird - the policy determines what you can see,
not what you can store, which might be two different things.

But the security policy should still apply to the old rows, i.e.
you shouldn't be after to UPDATE or DELETE rows you cannot see, no?

The case of INSERT / DELETE are simple; All we need to apply is
checks on either new or old tuples.

In case of UPDATE, we need to check on the old tuple whether use can
see, and on the new tuple whether use can store them.
Indeed, these are different checks, however, it seems like a black hole
if the new tuple is allowed to write but no reader privileges.
I expect most use cases choose same policy on reader timing and
writer times at UPDATE statement.

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

On May24, 2012, at 12:43 , Kohei KaiGai wrote:

The case of INSERT / DELETE are simple; All we need to apply is
checks on either new or old tuples.

In case of UPDATE, we need to check on the old tuple whether use can
see, and on the new tuple whether use can store them.
Indeed, these are different checks, however, it seems like a black hole
if the new tuple is allowed to write but no reader privileges.
I expect most use cases choose same policy on reader timing and
writer times at UPDATE statement.

I don't think preventing block holes is sensible here - it might,
in fact, be *just* what the user wants.

Imagine a messaging system. A reasonable RLS policy would be to allow
a user to see messages addressed to him. Yet you wouldn't want to prevent
her from creating messages to other people - cause what good is a messaging
system that only allows you to send messages to yourself. What you
probably *would* want to do, though, is to check that she did put herself in
as the sender when she creates a message. And you'd probably wanna forbit
updates entirely. So you'd have

- A RLS policy that checks current_user = ANY(recipients)
- An ON INSERT trigger which checks current_user = sender
- An ON UPDATE trigger which errors out

If RLS policy applies to INSERTEed rows also, how would you do that?

Another example, although in the realm of filesystem permissions, is Mac OS X.
Per default, every user has a "Drop Box" folder, which anybody can write to, yet
only the owner can read. This allows you to easily transfer files from one
user to another without allowing a third party to read it.

best regards,
Florian Pflug

2012/5/24 Florian Pflug <fgp@phlo.org>:

On May24, 2012, at 12:43 , Kohei KaiGai wrote:

The case of INSERT / DELETE are simple; All we need to apply is
checks on either new or old tuples.

In case of UPDATE, we need to check on the old tuple whether use can
see, and on the new tuple whether use can store them.
Indeed, these are different checks, however, it seems like a black hole
if the new tuple is allowed to write but no reader privileges.
I expect most use cases choose same policy on reader timing and
writer times at UPDATE statement.

I don't think preventing block holes is sensible here - it might,
in fact, be *just* what the user wants.

Imagine a messaging system. A reasonable RLS policy would be to allow
a user to see messages addressed to him. Yet you wouldn't want to prevent
her from creating messages to other people - cause what good is a messaging
system that only allows you to send messages to yourself. What you
probably *would* want to do, though, is to check that she did put herself in
as the sender when she creates a message. And you'd probably wanna forbit
updates entirely. So you'd have

 - A RLS policy that checks current_user = ANY(recipients)
 - An ON INSERT trigger which checks current_user = sender
 - An ON UPDATE trigger which errors out

If RLS policy applies to INSERTEed rows also, how would you do that?

Another example, although in the realm of filesystem permissions, is Mac OS X.
Per default, every user has a "Drop Box" folder, which anybody can write to, yet
only the owner can read. This allows you to easily transfer files from one
user to another without allowing a third party to read it.

Indeed, you are right. We have no special reason why to enforce same rules
on both of reader and writer stage on UPDATE statement.

So, the proposed interface might be revised as follows:
ALTER TABLE <tblname> ADD SECURITY POLICY
<func_name>(<args>, ...) [FOR SELECT |
INSERT |
[BEFORE|AFTER] UPDATE |
DELETE];

In case of INSERT or AFTER UPDATE, I assume the check shall be applied
on the tail of before-row triggers.

(*) I don't check whether it conflicts syntax or not yet.

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

On May24, 2012, at 16:19 , Kohei KaiGai wrote:

So, the proposed interface might be revised as follows:
ALTER TABLE <tblname> ADD SECURITY POLICY
<func_name>(<args>, ...) [FOR SELECT |
INSERT |
[BEFORE|AFTER] UPDATE |
DELETE];

In case of INSERT or AFTER UPDATE, I assume the check shall be applied
on the tail of before-row triggers.

I'd go with just "SELECT, UPDATE, DELETE", and leave the INSERT and BEFORE
UPDATE case to regular triggers, for two reasons

First, it's conceptually much simpler, since the policy always just adds
an implicit WHERE clause, period. This of course assumes that DELETE and
(BEFORE) UPDATE simply skips rows for which the policy function returns false,
instead of reporting 'permission denied' or something. But that's the most
reasonable behaviour anyway, I think, because otherwise you'd make batch
UPDATEs and DELETEs pretty much unusable, 'cause there'd always be the risk
of tripping over some invisible row and getting and error.

And second, it avoids mimicking functionality that is already provided
by an existing feature, namely triggers.

People will have to deal with the trigger ordering issue, but that's nothing
new, and I bet most people have a system in place for that. I usually prefix
my trigger names with 'a_' to 'z_', for example, to make the ordering explicit.

Another issue, BTW, are FOREIGN KEY constraints. Should you be allowed to
created references to rows which are invisible to you, or should FOREIGN KEY
constraints be exempt from security policies? I'd say they shouldn't be, i.e.
the policy WHERE clause should be added to constraint checking queries like
usual. But maybe I'm missing some reason why that'd be undesirable…

best regards,
Florian Pflug

2012/5/24 Florian Pflug <fgp@phlo.org>:

On May24, 2012, at 16:19 , Kohei KaiGai wrote:

So, the proposed interface might be revised as follows:
 ALTER TABLE <tblname> ADD SECURITY POLICY
     <func_name>(<args>, ...) [FOR SELECT |
                                                        INSERT |
                                                        [BEFORE|AFTER] UPDATE |
                                                        DELETE];

In case of INSERT or AFTER UPDATE, I assume the check shall be applied
on the tail of before-row triggers.

I'd go with just "SELECT, UPDATE, DELETE", and leave the INSERT and BEFORE
UPDATE case to regular triggers, for two reasons

First, it's conceptually much simpler, since the policy always just adds
an implicit WHERE clause, period. This of course assumes that DELETE and
(BEFORE) UPDATE simply skips rows for which the policy function returns false,
instead of reporting 'permission denied' or something. But that's the most
reasonable behaviour anyway, I think, because otherwise you'd make batch
UPDATEs and DELETEs pretty much unusable, 'cause there'd always be the risk
of tripping over some invisible row and getting and error.

I definitely agree with starting a new feature from simple implementation.

Although I'm inclined to the approach to replace references to tables with
security policy by sub-queries with security barrier flag, instead of adding
qualifiers of where clause to avoid the leaky-view scenario, it will make its
implementation mush simpler.

Another issue, BTW, are FOREIGN KEY constraints. Should you be allowed to
created references to rows which are invisible to you, or should FOREIGN KEY
constraints be exempt from security policies? I'd say they shouldn't be, i.e.
the policy WHERE clause should be added to constraint checking queries like
usual. But maybe I'm missing some reason why that'd be undesirable…

I agree. The row level security policy should not be applied during FK checks
(or other internal stuff; to be harmless). At the previous discussion, it was
issued that iteration of FK/PK proving enables malicious one to estimate
existence of invisible tuple and its key value, although they cannot see the
actual values. It is well documented limitation, thus, user should not use row-
level security (or should not use natural key) if they cannot accept
this limitation.

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

I'd like to summarize the current design being discussed.

syntax:
ALTER TABLE <tblname> WITH ROW LEVEL SECURITY
( <condition clause> ) [FOR (SELECT | UPDATE | DELETE)];
ALTER TABLE <tblname> WITHOUT ROW LEVEL SECURITY;

I tried to patch the parser/gram.y, but here was syntax conflicts
on ADD / DROP sub-command.
And, I noticed "ROW LEVEL SECURITY" allows to implement
without adding new keyword, unlike "SECURITY POLICY".

As we discussed, it causes a problem with approach to append
additional qualifiers to where clause implicitly, because it does
not solve the matter corresponding to the order to execute
qualifiers. So, I'm inclined to the approach to replace reference
to tables with security policy by sub-queries with security barrier
flag.
For example, if "tbl" has security policy, this query shall be rewritten
internally, as follows:

original)
SELECT * FROM tbl WHERE X > 20 AND f_leak(Y);

rewritten)
SELECT * FROM (
SELECT * FROM tbl WHERE uname = getpgusername()
) AS tbl_subqry WHERE X > 20 AND f_leak(Y);

The sub-query shall have security-barrier flag, so f_leak() is never
pushed down but X > 20 will be pushed down because of leakproof
attribute of the function.

It is a bit complex at UPDATE or DELETE statement, but
what I try to do is same.

original)
UPDATE tbl SET X = X + 1 WHERE X > 20 AND f_leak(Y);

rewritten)
UPDATE tbl SET X = X + 1 WHERE ctid = (
SELECT ctid FROM (
SELECT ctid, * FROM uname = getpgusername()
) AS tbl_subqry WHERE X > 20 AND f_leak(Y)
);

That guarantees the security policy ("uname = getpgusername()")
is evaluated prior to user given conditions.

One thing still I'm thinking is whether the security policy should
be provided as a function or a clause. Enough simple sql function
is inlined at simplify_function(), so here is no meaningful difference.
I was afraid of code complexity, but all we should do is to append
configured clause on the where clause of sub-query inside.

| ALTER TABLE <tblname> WITH ROW LEVEL SECURITY
| ( <condition clause> ) [FOR (SELECT | UPDATE | DELETE)];

So, I tried to put "condition clause" instead of a function, right now.

Regarding to FK constraints, I don't think it is a situation to
apply row-level security policy towards internal queries.
So, I plan to disable during FK checks.

One other issue we didn't have discussed is table inheritance.
In case when a table TBLP has a child table TBLC and only
TBLC has its security policy, what security policy should be
applied when we run "SELECT * FROM TBLP".
My preference is, the security policy is only applied to scan on
TBLC, not TBLP. It is not desirable behavior that visible tuples
are different from way to reference a certain table.
In addition, if and when TBLP and TBLC have their own policy
individually, what is a desirable behavior?
I think, the security policy of both TBLP and TBLC should be
applied on TBLC; in other words, it applies the security policy
of all the parent tables to scan on child table.

Any comments please. Thanks,

2012/5/24 Kohei KaiGai <kaigai@kaigai.gr.jp>:

2012/5/24 Florian Pflug <fgp@phlo.org>:

On May24, 2012, at 16:19 , Kohei KaiGai wrote:

So, the proposed interface might be revised as follows:
 ALTER TABLE <tblname> ADD SECURITY POLICY
     <func_name>(<args>, ...) [FOR SELECT |
                                                        INSERT |
                                                        [BEFORE|AFTER] UPDATE |
                                                        DELETE];

In case of INSERT or AFTER UPDATE, I assume the check shall be applied
on the tail of before-row triggers.

I'd go with just "SELECT, UPDATE, DELETE", and leave the INSERT and BEFORE
UPDATE case to regular triggers, for two reasons

First, it's conceptually much simpler, since the policy always just adds
an implicit WHERE clause, period. This of course assumes that DELETE and
(BEFORE) UPDATE simply skips rows for which the policy function returns false,
instead of reporting 'permission denied' or something. But that's the most
reasonable behaviour anyway, I think, because otherwise you'd make batch
UPDATEs and DELETEs pretty much unusable, 'cause there'd always be the risk
of tripping over some invisible row and getting and error.

I definitely agree with starting a new feature from simple implementation.

Although I'm inclined to the approach to replace references to tables with
security policy by sub-queries with security barrier flag, instead of adding
qualifiers of where clause to avoid the leaky-view scenario, it will make its
implementation mush simpler.

Another issue, BTW, are FOREIGN KEY constraints. Should you be allowed to
created references to rows which are invisible to you, or should FOREIGN KEY
constraints be exempt from security policies? I'd say they shouldn't be, i.e.
the policy WHERE clause should be added to constraint checking queries like
usual. But maybe I'm missing some reason why that'd be undesirable…

I agree. The row level security policy should not be applied during FK checks
(or other internal stuff; to be harmless). At the previous discussion, it was
issued that iteration of FK/PK proving enables malicious one to estimate
existence of invisible tuple and its key value, although they cannot see the
actual values. It is well documented limitation, thus, user should not use row-
level security (or should not use natural key) if they cannot accept
this limitation.

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

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

On Thu, May 24, 2012 at 6:11 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

Perhaps when we see that RLS
applies, we should replace the reference to the original table with a
subquery RTE that has the security_barrier flag set - essentially
treating a table with RLS as if it were a security view.

I become to think it is a better approach than tracking origin of each
qualifiers. One problem is case handling on update or delete statement.

It may be possible to rewrite the update / delete query as follows:

From:
 UPDATE tbl SET X = X + 1 WHERE f_leak(Y)
To:
 UPDATE tbl SET X = X + 1 WHERE ctid = (
     SELECT * FROM (
         SELECT ctid FROM tbl WHERE uname = getpgusername()  <== (*)
should have security-barrier
     ) AS tbl_subqry WHERE f_leak(Y)
 );

Expanded sub-queries will have security-barrier flag, so it enforces
the "uname = getpgusername()" being checked earlier than f_leak(Y).
We may need to measure the performance impact due to the reform.

The problem with this is that it introduces an extra instance of tbl
into the query - there are now two rather than one. UPDATE .. FROM is
supposed to be a way to avoid this, but it's insufficiently general to
handle all the cases (e.g. UPDATE a LEFT JOIN b can't be written using
the existing syntax). Anyway we want to avoid inserting self-joins
for performance reasons if at all possible. It should be easy to do
that in the case of SELECT; UPDATE and DELETE may need a bit more
work.

I think, this situation is similar to a case when we reference a view
without privileges to underlying tables. If Bob set up a view with
something "tricky" function, it allows Bob to reference credentials
of users who reference the view.
More or less, it might be a problem when a user try to invoke
a user defined function declared by others.
(Thus, sepgsql policy does not allow users to invoke a function
declared by another one in different domain; without DBA's checks.)

This is true, but there are still some new threat models. For
example, currently, pg_dump isn't going to run any user-defined code
just because you do SELECT * FROM table, but that will change with
this patch. Note that pg_dump need not actually select from views,
only tables.

I think it is a good idea not to apply RLS when current user has
superuser privilege from perspective of security model consistency,
but it is inconsistent to check privileges underlying tables.

Seems like a somewhat random wart, if it's just an exception for
superusers. I think we need to do better than that. For example, at
my last company, sales reps A and B were permitted to see all
customers of the company, but sales reps C, D, E, F, G, H, I, and J
were permitted to see only their own accounts. Those sorts of
policies need to be easy to implement.

Another idea is to set things up so that the RLS policy function isn't
applied to each row directly; instead, it's invoked once per query and
*returns* a WHERE clause.  This would be a lot more powerful than the
proposed design, because now the table owner can write a function that
imposes quals on some people but not others, which seems very useful.

Sorry, I don't favor this idea. Even if table owner set up a function to
generate additional qualifiers, it also has no guarantee the qualifiers
are invoked prior to user-given one.
It seems to me this approach will have same problem...

It's not intended to solve the qual-ordering problem, just to allow
additional policy flexibility.

It's not clear to me that there is any need for built-in server
functionality here.  If the table owner wants to enforce some sort of
policy regarding INSERT or UPDATE or DELETE, they can already do that
today just by attaching a trigger to the table.  And they can enforce
whatever policy they like that way.  Before designing any new
mechanism, what's wrong with the existing one?

Yes, we don't need any new invent to check the value of new tuples.
But it should be done after all the user-defined triggers. Existing
trigger does not have a mechanism to enforce order to be invoked.
So, what I really implement is a mechanism to inject some pseudo
triggers "at tail of the Trigger array".

Start the trigger names with the letter "z".

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

On Thu, May 24, 2012 at 6:20 AM, Florian Pflug <fgp@phlo.org> wrote:

But the security policy should still apply to the old rows, i.e.
you shouldn't be after to UPDATE or DELETE rows you cannot see, no?

Agreed.

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

On Thu, May 24, 2012 at 12:00 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

Another issue, BTW, are FOREIGN KEY constraints. Should you be allowed to
created references to rows which are invisible to you, or should FOREIGN KEY
constraints be exempt from security policies? I'd say they shouldn't be, i.e.
the policy WHERE clause should be added to constraint checking queries like
usual. But maybe I'm missing some reason why that'd be undesirable…

I agree. The row level security policy should not be applied during FK checks
(or other internal stuff; to be harmless). At the previous discussion, it was
issued that iteration of FK/PK proving enables malicious one to estimate
existence of invisible tuple and its key value, although they cannot see the
actual values. It is well documented limitation, thus, user should not use row-
level security (or should not use natural key) if they cannot accept
this limitation.

You say "I agree", but it seems to me that you and Florian are in fact
taking opposite positions.

FWIW, I'm inclined to think that you should NOT be able to create a
row that references an invisible row. You might end up with that
situation anyway, because we don't know what the semantics of the
security policy are: rows might become visible or invisible after the
fact, and we can't police that. But I think that if you take the
opposite position that the select queries inside fkey triggers ought
to be exempt from security policy, then you need to build some new
mechanism to make that happen, which seems like extra work for no
benefit.

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

On May24, 2012, at 18:42 , Kohei KaiGai wrote:

I'd like to summarize the current design being discussed.

syntax:
ALTER TABLE <tblname> WITH ROW LEVEL SECURITY
( <condition clause> ) [FOR (SELECT | UPDATE | DELETE)];
ALTER TABLE <tblname> WITHOUT ROW LEVEL SECURITY;

I tried to patch the parser/gram.y, but here was syntax conflicts
on ADD / DROP sub-command.
And, I noticed "ROW LEVEL SECURITY" allows to implement
without adding new keyword, unlike "SECURITY POLICY".

Let the bike-shedding begin ;-)

ALTER TABLE … WITH sounds a bit weird. What about

ALTER TABLE <tblname> SET ROW POLICY <condition> FOR { SELECT | UPDATE | DELETE }
ALTER TABLE <tblname> RESET ROW POLICY

As we discussed, it causes a problem with approach to append
additional qualifiers to where clause implicitly, because it does
not solve the matter corresponding to the order to execute
qualifiers. So, I'm inclined to the approach to replace reference
to tables with security policy by sub-queries with security barrier
flag.

Since the security barrier flag carries a potentially hefty performance
penalty, I think it should be optional. Application which don't allow
SQL-level access to the database might still benefit from row-level security,
because it saves them from having to manually add the WHERE clause to every
statement, or having to wrap all their tables with views. Yet without direct
SQL-level access, the security barrier thing isn't really necessary, so
it'd be nice if they wouldn't have to pay for it. How about

ALTER TABLE … SET ROW POLICY … WITH (security_barrier)

One thing still I'm thinking is whether the security policy should
be provided as a function or a clause. Enough simple sql function
is inlined at simplify_function(), so here is no meaningful difference.
I was afraid of code complexity, but all we should do is to append
configured clause on the where clause of sub-query inside.

| ALTER TABLE <tblname> WITH ROW LEVEL SECURITY
| ( <condition clause> ) [FOR (SELECT | UPDATE | DELETE)];

So, I tried to put "condition clause" instead of a function, right now.

A single function seems much easier implementation-wise, since you wouldn't
need to store an arbitrary expression in the catalog, just an oid. It also
delegates the dependency tracking problem to the function. It also simplies
the grammar, because the "FOR … " clause cannot be mistaken to belong to
the condition clause.

One other issue we didn't have discussed is table inheritance.
In case when a table TBLP has a child table TBLC and only
TBLC has its security policy, what security policy should be
applied when we run "SELECT * FROM TBLP".
My preference is, the security policy is only applied to scan on
TBLC, not TBLP.

Agreed.

It is not desirable behavior that visible tuples
are different from way to reference a certain table.
In addition, if and when TBLP and TBLC have their own policy
individually, what is a desirable behavior?
I think, the security policy of both TBLP and TBLC should be
applied on TBLC; in other words, it applies the security policy
of all the parent tables to scan on child table.

I think security policies should only apply to the table they're
declared for, not their child tables. Mostly because that is how
triggers operate, and security policies and triggers will often
be used together, so having their semantics regarding inheritance
be the same seems to be the least surprising option.

Also, if policies are inherited, how would you define a policy
which applies only to the parent, not to the child?

best regards,
Florian Pflug

On May24, 2012, at 19:25 , Robert Haas wrote:

FWIW, I'm inclined to think that you should NOT be able to create a
row that references an invisible row. You might end up with that
situation anyway, because we don't know what the semantics of the
security policy are: rows might become visible or invisible after the
fact, and we can't police that.

Right. I just realized, however, that there's another case which wasn't
considered yet, which is how to handle the initial check during
ALTER TABEL ADD CONSTRAINT. I'm thinking that it's fine to only consider
visible rows in the parent table there too, but we should be checking
all rows in the child table. The easiest way would be to restrict
ALTER TABLE ADD CONSTRAINT to the table owner for tables with RLS
(it seems that currently the REFERENCES privilege is sufficient), and
make the table owner exempt from RLS on that table. The latter means you'd
need at least two roles to use RLS, but anyone security-conscious enough
to use RLS will probably not user the same role for DDL and DML operations
anyway...

But I think that if you take the
opposite position that the select queries inside fkey triggers ought
to be exempt from security policy, then you need to build some new
mechanism to make that happen, which seems like extra work for no
benefit.

Hm, interesting angle. Continuing this thought, without any extra work,
UNIQUE and EXCLUSION constraints *will* be enforced regardless of row
visibility, because their implementation isn't SPI-based but instead
detects conflicts while inserting tuples into the index.

For being so obviously inconsistent in its treatment of UNIQUE and
EXCLUSION constraints vs. FK constraints, this feels surprisingly
right. So, to prevent design by accident, here's an attempt to explain
that divergence.

For UNIQUE and EXCLUSION constraints, the most conservative assumption
possible is that all rows are visible, since that leads to the most
rejections. With that assumption, no matter what the actual policy is,
the data returned by a query will always satisfy the constraint. Plus,
the constraint is still sensible because it neither rejects nor allows
all rows. So that conservative assumption is the one we make, i.e. we
ignore RLS visibility when checking those kinds of constraints.

For FK constraints, OTOH, the most conservative assumption is that
no rows are visible. But that is meaningless, since it will simply reject
all possible rows. Having thus no chance of enforcing the constraint
ourselves under all possible policies, the best we can do is to at least
make it possible for the constraint to work correctly for as many policies
as possible. Now, if we go with KaiGai's suggestion of skipping RLS
while checking FK constraints, the only policy that the constraint will
work correctly for is one which doesn't actually hide any parent rows.
Whereas if we apply RLS checks while checking FK constraints, all policies
which behave consistently for parent and child rows (i.e. don't hide the
former but show the latter) will work correctly. We thus go with the
second option, since the class of working policies is larger.

best regards,
Florian Pflug

2012/5/24 Robert Haas <robertmhaas@gmail.com>:

On Thu, May 24, 2012 at 6:11 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

Perhaps when we see that RLS
applies, we should replace the reference to the original table with a
subquery RTE that has the security_barrier flag set - essentially
treating a table with RLS as if it were a security view.

I become to think it is a better approach than tracking origin of each
qualifiers. One problem is case handling on update or delete statement.

It may be possible to rewrite the update / delete query as follows:

From:
 UPDATE tbl SET X = X + 1 WHERE f_leak(Y)
To:
 UPDATE tbl SET X = X + 1 WHERE ctid = (
     SELECT * FROM (
         SELECT ctid FROM tbl WHERE uname = getpgusername()  <== (*)
should have security-barrier
     ) AS tbl_subqry WHERE f_leak(Y)
 );

Expanded sub-queries will have security-barrier flag, so it enforces
the "uname = getpgusername()" being checked earlier than f_leak(Y).
We may need to measure the performance impact due to the reform.

The problem with this is that it introduces an extra instance of tbl
into the query - there are now two rather than one.  UPDATE .. FROM is
supposed to be a way to avoid this, but it's insufficiently general to
handle all the cases (e.g. UPDATE a LEFT JOIN b can't be written using
the existing syntax).  Anyway we want to avoid inserting self-joins
for performance reasons if at all possible.  It should be easy to do
that in the case of SELECT; UPDATE and DELETE may need a bit more
work.

I'll try to investigate a way to solve the matter without twice scan.
Right now, I have no reasonable ideas. Please give us suggestion
if you have something...

I think, this situation is similar to a case when we reference a view
without privileges to underlying tables. If Bob set up a view with
something "tricky" function, it allows Bob to reference credentials
of users who reference the view.
More or less, it might be a problem when a user try to invoke
a user defined function declared by others.
(Thus, sepgsql policy does not allow users to invoke a function
declared by another one in different domain; without DBA's checks.)

This is true, but there are still some new threat models.  For
example, currently, pg_dump isn't going to run any user-defined code
just because you do SELECT * FROM table, but that will change with
this patch.  Note that pg_dump need not actually select from views,
only tables.

I think it is a good idea not to apply RLS when current user has
superuser privilege from perspective of security model consistency,
but it is inconsistent to check privileges underlying tables.

Seems like a somewhat random wart, if it's just an exception for
superusers.  I think we need to do better than that.  For example, at
my last company, sales reps A and B were permitted to see all
customers of the company, but sales reps C, D, E, F, G, H, I, and J
were permitted to see only their own accounts.  Those sorts of
policies need to be easy to implement.

Probably, if "sales_rep" column records its responsible repo, its
security policy is able to be described as:
(my_sales_rep() in ('A', 'B') OR sales_rep = my_sales_rep())

Indeed, the design to check underlying table seems to me like
column-level privileges towards table-level privileges, since it
is checked only when user does not have requires privileges
on whole of the table.
However, I have no idea to modify ExecCheckRTEPerms()
regarding to RLS. If we assume RLS is applied when user has
no privileges on tables, the current ExecCheckRTEPerms()
always raises an error towards unprivileged users, prior to
execution of queries.
Isn't it preferable behavior to allow unprivileged users to
reference a table (or columns) when it has RLS policy?

I think, table and column level privilege should be checked
individually, in addition to row-level security policy.

Another idea is to set things up so that the RLS policy function isn't
applied to each row directly; instead, it's invoked once per query and
*returns* a WHERE clause.  This would be a lot more powerful than the
proposed design, because now the table owner can write a function that
imposes quals on some people but not others, which seems very useful.

Sorry, I don't favor this idea. Even if table owner set up a function to
generate additional qualifiers, it also has no guarantee the qualifiers
are invoked prior to user-given one.
It seems to me this approach will have same problem...

It's not intended to solve the qual-ordering problem, just to allow
additional policy flexibility.

At the beginning, I thought it takes complex code to parse
where-clause being provided as security policy, so it is the
reason why I was inclined to give a function, instead of a clause.
But I noticed we already have similar code at CreateTrigger()
to handle it.
Does it give policy flexibility?

It's not clear to me that there is any need for built-in server
functionality here.  If the table owner wants to enforce some sort of
policy regarding INSERT or UPDATE or DELETE, they can already do that
today just by attaching a trigger to the table.  And they can enforce
whatever policy they like that way.  Before designing any new
mechanism, what's wrong with the existing one?

Yes, we don't need any new invent to check the value of new tuples.
But it should be done after all the user-defined triggers. Existing
trigger does not have a mechanism to enforce order to be invoked.
So, what I really implement is a mechanism to inject some pseudo
triggers "at tail of the Trigger array".

Start the trigger names with the letter "z".

OK, I'd like to postpone this future from the 1st stage.

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