Re: leaky views, yet again

(2010/07/20 2:19), Heikki Linnakangas wrote:
> On 19/07/10 20:08, Robert Haas wrote:
>> 2010/7/8 KaiGai Kohei<kaigai@ak.jp.nec.com>:
>>> (2010/07/08 22:08), Robert Haas wrote:
>>>> I think we pretty much have conceptual agreement on the shape of the
>>>> solution to this problem: when a view is created with CREATE SECURITY
>>>> VIEW, restrict functions and operators that might disclose tuple data
>>>> from being pushed down into view (unless, perhaps, the user invoking
>>>> the view has sufficient privileges to execute the underlying query
>>>> anyhow, e.g. superuser or view owner). What we have not resolved is
>>>> exactly how we're going to decide which functions and operators might
>>>> do such a dastardly thing. I think the viable options are as follows:
>>>>
>>>> 1. Adopt Heikki's proposal of treating indexable operators as 
>>>> non-leaky.
>>>>
>>>> http://archives.postgresql.org/pgsql-hackers/2010-06/msg00291.php
>>>>
>>>> Or, perhaps, and even more restrictively, treat only
>>>> hashable/mergeable operators as non-leaky.
>>>>
>>>> 2. Add an explicit flag to pg_proc, which can only be set by
>>>> superusers (and which is cleared if a non-superuser modifies it in any
>>>> way), allowing a function to be tagged as non-leaky. I believe that
>>>> it would be reasonable to set this flag for all of our built-in
>>>> indexable operators (though I'd read the code before doing it), but it
>>>> would remove the need for the assumption that third-party operators
>>>> are equally sane.
>>>>
>>>> CREATE OR REPLACE FUNCTION doesnt_leak() RETURNS integer AS $$SELECT
>>>> 42$$ IMMUTABLE SEAWORTHY; -- doesn't leak
>>>>
>>>> This problem is not going away, so we should try to decide on 
>>>> something.
>>>>
>>> I'd like to vote the second option, because this approach will be also
>>> applied on another aspect of leaky views.
>>>
>>> When leaky and non-leaky functions are chained within a WHERE clause,
>>> it will be ordered by the cost of functions. So, we have possibility
>>> that leaky functions are executed earlier than non-leaky functions.
>>>
>>> It will not be an easy work to mark built-in functions as either leaky
>>> or non-leaky, but it seems to me the most straight forward solution.
>>
>> Does anyone else have an opinion on this?
> 
> I have a bad feeling that marking functions explicitly as seaworthy is 
> going to be hard to get right, and every time you get that wrong it's a 
> security issue.
>
Yes, it is indeed a uphill work to mark either secure or leaky on the
functions correctly. :(

> On the other hand, if it's enough from a performance 
> point of view to review and mark only a few built-in functions like 
> index operators, maybe it's ok.
> 
I also think it is a worthful idea to try as a proof-of-concept.

If we only allow to push down indexable operators, do you have any idea
to distinguish between a purely indexable operator qual and others?
At the distribute_qual_to_rels() stage, how can we find out a qual which
is consist of only indexable operators?

> It would be easier to assess this if we had a patch to play with that 
> contained all the planner changes to keep track of the seaworthiness of 
> functions and apply the quals in right order. You could then try out 
> different scenarios to see what the performance is like.
> 
> I guess what I'm saying is "write a patch, and I'll shoot it down when 
> you're done" :-/. But hopefully the planner changes don't depend much on 
> how we deduce which quals are leaky.
> 
I agree. The decision to mark either secure or leaky on functions should
be done independently from the planner code, like contain_volatile_functions().

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>