Re: [v9.3] Row-Level Security

2012/6/26 Robert Haas <robertmhaas(at)gmail(dot)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(at)kaigai(dot)gr(dot)jp>