Re: leaky views, yet again

I noticed the previous patch has an obvious conflict to the latest
git mater, and it does not have any documentation updates.

So, I rebased the patch, and added descriptions about secure views.
Rest of parts are unchanged.

Thanks,

(2010/10/06 17:01), KaiGai Kohei wrote:
> 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@ak.jp.nec.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@ak.jp.nec.com>