Re: leaky views, yet again

The attached patch is a revised version of the fix-leaky-view patch.

List of updates:
- It was rebased to the latest git master.
- It revised pointer castings in rewriteHandler.c.
- It added a new regression test: 'security_views'.
- It added SECURITY VIEW support to pg_dump.
- It modified the definition of pg_views to show whether it is
a security view, or not.
- It revised psql to show SECURITY VIEW attribute of views in
verbose mode.
- I tried to add a new field to pg_class, but scale of code changes
are eventually similar to previous version. So, all I revised was
to define StdViewOptions, instead of abuse of StdRdOptions.

To avoid confusion, I'd like to make clear what is the matter this
patch tries to tackle.
As we have discussed for a long time, an interaction between functions
with side-effects and query optimization on views may cause information
leaks, even if owner of the views intend to use them for row-level
security.
In this context, we define the term of 'leak' as system can expose
content of tuples to be invisible unexpectedly, then people can see
the content directly without any inferences.

Thus, if and when people supplied a function that writes out given
arguments into other tables on the WHERE clause of views, it should
be invoked after all the conditions within view definition.

E.g) CREATE OR REPLACE FUNCTION f_malicious(text)
RETURNS bool COST 0.01 LANGUAGE 'sql'
AS 'INSERT INTO my_private VALUES($1); SELECT true;';
SELECT * FROM secret_view v WHERE f_malicious(v);

In addition, some of functions (including built-in functions) raise
an error with message that may contain given arguments. It also allows
to expose content of invisible tuples (although throughput of the
channel is relatively slow), if it would be pushed down into inside
of join-loop.

E.g) postgres=# select * from v2;
a | b | x | y
---+-----+---+-----
2 | bbb | 2 | www
4 | eee | 4 | yyy
(2 rows)

postgres=# select * from v2 where y::int = 1;
ERROR: invalid input syntax for integer: "vvv"

However, it is too expensive to prohibit pushing down all the function
calls into join-loops. So, we need allow to push down a part of harmless
functions. In this patch, I don't change the logic to decide it.
Thus, only operators implemented by functions with INTERNALlanguage
are allowed to push down into SECURITY VIEWs.

On the other hand, we can infer a certain value of hidden tuple with
iteration of probes. In this scenario, people never see the hidden
values directly. So, it is not a matter this patch tries to tackle.

E.g) postgres=# select * from v1;
a | b
---+-----
2 | bbb
4 | eee
(2 rows)

postgres=# insert into t1 values (3, 'xyz');
ERROR: duplicate key value violates unique constraint "t1_pkey"
DETAIL: Key (a)=(3) already exists.
postgres=# select * from v1 where 1/(3-a) > 0;
ERROR: division by zero

As long as I know, commercial RDBMSs with RLS also don't tackle such
kind of information leaks via side-channels. At least, I don't think
it is a matter that we should tackle with first priority.

Thanks,

(2010/10/05 18:01), Itagaki Takahiro wrote:
> 2010/9/6 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> The attached patch tackles both of the above two points, and allows to
>> control this deoptimization when CREATE SECURITY VIEW is used.
>
> I'll send a review for the patch.
>
> Contents& Purpose
> ==================
> The patch adds a new SECURITY option for VIEWs. Views defined with
> CREATE SECURITY
> VIEW command are not merged with external WHERE-clauses including
> security-leakable
> functions in queries. However, since internal functions and operators are not
> considered as leakable functions, the planner still works well for
> security views
> unless we use user-defined functions in the WHERE-clause.
>
> Initial Run
> ===========
> * Because of the delayed review, the patch has one hunk:
> 1 out of 5 hunks FAILED -- saving rejects to file
> src/backend/commands/tablecmds.c.rej
> but it is not serious at all, and can be easily fixed.
>
> * It can be compiled, but has two warnings:
> rewriteHandler.c: In function 'MarkFuncExprDepthWalker':
> rewriteHandler.c:1155: warning: cast from pointer to integer of different size
> rewriteHandler.c:1227: warning: cast to pointer from integer of different size
>
> They should be harmless, but casting intptr_t is often used to avoid warnings:
> - L1155: (int) (intptr_t) context;
> - L1227: (void *) (intptr_t) (depth + 1)
>
> * It passes all regression tests, but doesn't have own new tests.
>
> Remaining works
> ===============
> The patch might include only core code, but I'll let you know additional
> sub-works are still required to complete the feature. Also, I've not measured
> the performance yet, though performance might not be so critical for the patch.
>
> * Regression tests and documentation for the feature are required.
> * pg_dump must support to dump SECURITY VIEWs.
> They are dumped as normal views for now.
> * pg_views can have "security" column.
> * psql's \dv can show security attributes of views.
>
> Questions
> =========
>> postgres=# EXPLAIN select * from v1 where f_malicious(b) and a = 2;
>> ==> We assume operators implemented with built-in functions are safe.
>> So, we don't prevent this pushing-down, if the clause does not
>> contains something leakable.
>
> The term "built-in functions" means functions written in INTERNAL language
> here. But we also have SQL functions by default. Some of them are just a
> wrapper to internal functions. I'm not sure the checking of INTERNAL language
> is the best way for the purpose. Did you compare it with other methods?
> For example, "oid< FirstNormalObjectId" looks workable for me.
>
>> Instead of modifying the structure of pg_class, this patch stores a flag of
>> security view on the reloption. So, it needed to modify routines about
>> reloptions because it is the first case to store reloptions of views.
>
> Why did you need to extend StdRdOptions strucuture? Since other fields in
> the structure are not used by views at all, I think adding a new structure
> struct ViewOptions { vl_len, security_view }
> would be better than extending StdRdOptions.
>

--
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>