Re: [v9.2] LEAKPROOF attribute of FUNCTION (Re: [v9.2] Fix Leaky View Problem)

>> BTW, can you also resubmit the leakproof stuff as a separate patch for
>> the last CF?  Want to make sure we get that into 9.2, if at all
>> possible.
>>
> Yes, it shall be attached on the next message.
>
The attached patch adds LEAKPROOF attribute to pg_proc; that
enables DBA to set up obviously safe functions to be pushed down
into sub-query even if it has security-barrier attribute.
We assume this LEAKPROOF attribute shall be applied on operator
functions being used to upgrade execute plan from Seq-Scan to
Index-Scan.

The default is without-leakproof attribute on creation of functions,
and it requires superuser privilege to switch on.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Sun, Jan 8, 2012 at 10:52 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> BTW, can you also resubmit the leakproof stuff as a separate patch for
>>> the last CF?  Want to make sure we get that into 9.2, if at all
>>> possible.
>>>
>> Yes, it shall be attached on the next message.
>>
> The attached patch adds LEAKPROOF attribute to pg_proc; that
> enables DBA to set up obviously safe functions to be pushed down
> into sub-query even if it has security-barrier attribute.
> We assume this LEAKPROOF attribute shall be applied on operator
> functions being used to upgrade execute plan from Seq-Scan to
> Index-Scan.
>
> The default is without-leakproof attribute on creation of functions,
> and it requires superuser privilege to switch on.

The create_function_3 regression test fails for me with this applied:

*** /Users/rhaas/pgsql/src/test/regress/expected/create_function_3.out
2012-01-17 22:09:01.000000000 -0500
--- /Users/rhaas/pgsql/src/test/regress/results/create_function_3.out
2012-01-17 22:14:48.000000000 -0500
***************
*** 158,165 ****
'functext_E_2'::regproc);
proname | proleakproof
--------------+--------------
- functext_e_2 | t
functext_e_1 | t
(2 rows)

-- list of built-in leakproof functions
--- 158,165 ----
'functext_E_2'::regproc);
proname | proleakproof
--------------+--------------
functext_e_1 | t
+ functext_e_2 | t
(2 rows)

-- list of built-in leakproof functions
***************
*** 476,485 ****
'functext_F_4'::regproc);
proname | proisstrict
--------------+-------------
- functext_f_1 | f
functext_f_2 | t
functext_f_3 | f
functext_f_4 | t
(4 rows)

-- Cleanups
--- 476,485 ----
'functext_F_4'::regproc);
proname | proisstrict
--------------+-------------
functext_f_2 | t
functext_f_3 | f
functext_f_4 | t
+ functext_f_1 | f
(4 rows)

-- Cleanups

The new regression tests I just committed need updating as well.

Instead of contains_leakable_functions I suggest
contains_leaky_functions or contains_non_leakproof_functions, because
"leakable" isn't really a word (although I know what you mean).

The design of this function also doesn't seem very future-proof. If
someone adds a new node type that can contain a function call, and
forgets to add it here, then we've got a subtle security hole. Is
there some reasonable way to design this so that we assume
everything's dangerous except for those things we know are safe,
rather than the reverse?

I think you need to do a more careful check of which functions you're
marking leakproof - e.g. timestamp_ne_timestamptz isn't, at least
according to my understanding of the term.

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

Thanks for your reviewing.

I renamed the contain_leakable_functions() to contain_leaky_functions(),
and refreshed regression test.

> The design of this function also doesn't seem very future-proof.  If
> someone adds a new node type that can contain a function call, and
> forgets to add it here, then we've got a subtle security hole.  Is
> there some reasonable way to design this so that we assume
> everything's dangerous except for those things we know are safe,
> rather than the reverse?
>
And, modified the logic in contain_leaky_functions(); that add checks
whether the supplied node is know, or not. If the clause contains
newly defined node type, it handles as if ones contains leaky function.

> I think you need to do a more careful check of which functions you're
> marking leakproof - e.g. timestamp_ne_timestamptz isn't, at least
> according to my understanding of the term.
>
I marked the default leakproof function according to the criteria that
does not leak contents of the argument.
Indeed, timestamp_ne_timestamptz() has a code path that rises
an error of "timestamp out of range" message. Is it a good idea to
avoid mark "leakproof" on these functions also?

Thanks,

2012/1/18 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Sun, Jan 8, 2012 at 10:52 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>>> BTW, can you also resubmit the leakproof stuff as a separate patch for
>>>> the last CF?  Want to make sure we get that into 9.2, if at all
>>>> possible.
>>>>
>>> Yes, it shall be attached on the next message.
>>>
>> The attached patch adds LEAKPROOF attribute to pg_proc; that
>> enables DBA to set up obviously safe functions to be pushed down
>> into sub-query even if it has security-barrier attribute.
>> We assume this LEAKPROOF attribute shall be applied on operator
>> functions being used to upgrade execute plan from Seq-Scan to
>> Index-Scan.
>>
>> The default is without-leakproof attribute on creation of functions,
>> and it requires superuser privilege to switch on.
>
> The create_function_3 regression test fails for me with this applied:
>
> *** /Users/rhaas/pgsql/src/test/regress/expected/create_function_3.out
>  2012-01-17 22:09:01.000000000 -0500
> --- /Users/rhaas/pgsql/src/test/regress/results/create_function_3.out
>  2012-01-17 22:14:48.000000000 -0500
> ***************
> *** 158,165 ****
>                       'functext_E_2'::regproc);
>     proname    | proleakproof
>  --------------+--------------
> -  functext_e_2 | t
>   functext_e_1 | t
>  (2 rows)
>
>  -- list of built-in leakproof functions
> --- 158,165 ----
>                       'functext_E_2'::regproc);
>     proname    | proleakproof
>  --------------+--------------
>   functext_e_1 | t
> +  functext_e_2 | t
>  (2 rows)
>
>  -- list of built-in leakproof functions
> ***************
> *** 476,485 ****
>                       'functext_F_4'::regproc);
>     proname    | proisstrict
>  --------------+-------------
> -  functext_f_1 | f
>   functext_f_2 | t
>   functext_f_3 | f
>   functext_f_4 | t
>  (4 rows)
>
>  -- Cleanups
> --- 476,485 ----
>                       'functext_F_4'::regproc);
>     proname    | proisstrict
>  --------------+-------------
>   functext_f_2 | t
>   functext_f_3 | f
>   functext_f_4 | t
> +  functext_f_1 | f
>  (4 rows)
>
>  -- Cleanups
>
> The new regression tests I just committed need updating as well.
>
> Instead of contains_leakable_functions I suggest
> contains_leaky_functions or contains_non_leakproof_functions, because
> "leakable" isn't really a word (although I know what you mean).
>
> The design of this function also doesn't seem very future-proof.  If
> someone adds a new node type that can contain a function call, and
> forgets to add it here, then we've got a subtle security hole.  Is
> there some reasonable way to design this so that we assume
> everything's dangerous except for those things we know are safe,
> rather than the reverse?
>
> I think you need to do a more careful check of which functions you're
> marking leakproof - e.g. timestamp_ne_timestamptz isn't, at least
> according to my understanding of the term.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Sat, Jan 21, 2012 at 3:59 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> I marked the default leakproof function according to the criteria that
> does not leak contents of the argument.
> Indeed, timestamp_ne_timestamptz() has a code path that rises
> an error of "timestamp out of range" message. Is it a good idea to
> avoid mark "leakproof" on these functions also?

I think that anything which looks at the data and uses that as a basis
for whether or not to throw an error is non-leakproof. Even if
doesn't directly leak an arbitrary value, I think that leaking even
some information about what the value is no good. Otherwise, you
might imagine that we would allow /(int, int), because it only leaks
in the second_arg = 0 case. And you might imagine we'd allow -(int,
int) because it only leaks in the case where an overflow occurs. But
of course the combination of the two allows writing something of the
form 1/(a-constant) and getting it pushed down, and now you have the
ability to probe for an arbitrary value. So I think it's just no good
to allow any leaking at all: otherwise it'll be unclear how safe it
really is, especially when combinations of different functions or
operators are involved.

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

2012/1/21 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Sat, Jan 21, 2012 at 3:59 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> I marked the default leakproof function according to the criteria that
>> does not leak contents of the argument.
>> Indeed, timestamp_ne_timestamptz() has a code path that rises
>> an error of "timestamp out of range" message. Is it a good idea to
>> avoid mark "leakproof" on these functions also?
>
> I think that anything which looks at the data and uses that as a basis
> for whether or not to throw an error is non-leakproof.  Even if
> doesn't directly leak an arbitrary value, I think that leaking even
> some information about what the value is no good.  Otherwise, you
> might imagine that we would allow /(int, int), because it only leaks
> in the second_arg = 0 case.  And you might imagine we'd allow -(int,
> int) because it only leaks in the case where an overflow occurs.  But
> of course the combination of the two allows writing something of the
> form 1/(a-constant) and getting it pushed down, and now you have the
> ability to probe for an arbitrary value.  So I think it's just no good
> to allow any leaking at all: otherwise it'll be unclear how safe it
> really is, especially when combinations of different functions or
> operators are involved.
>
OK. I checked list of the default leakproof functions.

Functions that contains translation between date and timestamp(tz)
can raise an error depending on the supplied arguments.
Thus, I unmarked leakproof from them.

In addition, varstr_cmp() contains translation from UTF-8 to UTF-16 on
win32 platform; that may raise an error if string contains a character that
is unavailable to translate.
Although I'm not sure which case unavailable to translate between them,
it seems to me hit on the basis not to leak what kind of information is
no good. Thus, related operator functions of bpchar and text got unmarked.
(Note that bpchareq, bpcharne, texteq and textne don't use it.)

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Sun, Jan 22, 2012 at 5:12 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> 2012/1/21 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Sat, Jan 21, 2012 at 3:59 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> I marked the default leakproof function according to the criteria that
>>> does not leak contents of the argument.
>>> Indeed, timestamp_ne_timestamptz() has a code path that rises
>>> an error of "timestamp out of range" message. Is it a good idea to
>>> avoid mark "leakproof" on these functions also?
>>
>> I think that anything which looks at the data and uses that as a basis
>> for whether or not to throw an error is non-leakproof.  Even if
>> doesn't directly leak an arbitrary value, I think that leaking even
>> some information about what the value is no good.  Otherwise, you
>> might imagine that we would allow /(int, int), because it only leaks
>> in the second_arg = 0 case.  And you might imagine we'd allow -(int,
>> int) because it only leaks in the case where an overflow occurs.  But
>> of course the combination of the two allows writing something of the
>> form 1/(a-constant) and getting it pushed down, and now you have the
>> ability to probe for an arbitrary value.  So I think it's just no good
>> to allow any leaking at all: otherwise it'll be unclear how safe it
>> really is, especially when combinations of different functions or
>> operators are involved.
>>
> OK. I checked list of the default leakproof functions.
>
> Functions that contains translation between date and timestamp(tz)
> can raise an error depending on the supplied arguments.
> Thus, I unmarked leakproof from them.
>
> In addition, varstr_cmp() contains translation from UTF-8 to UTF-16 on
> win32 platform; that may raise an error if string contains a character that
> is unavailable to translate.
> Although I'm not sure which case unavailable to translate between them,
> it seems to me hit on the basis not to leak what kind of information is
> no good. Thus, related operator functions of bpchar and text got unmarked.
> (Note that bpchareq, bpcharne, texteq and textne don't use it.)

Can you rebase this? It seems that the pg_proc.h and
select_views{,_1}.out hunks no longer apply cleanly.

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

2012/1/25 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Sun, Jan 22, 2012 at 5:12 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> 2012/1/21 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>> On Sat, Jan 21, 2012 at 3:59 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>>> I marked the default leakproof function according to the criteria that
>>>> does not leak contents of the argument.
>>>> Indeed, timestamp_ne_timestamptz() has a code path that rises
>>>> an error of "timestamp out of range" message. Is it a good idea to
>>>> avoid mark "leakproof" on these functions also?
>>>
>>> I think that anything which looks at the data and uses that as a basis
>>> for whether or not to throw an error is non-leakproof.  Even if
>>> doesn't directly leak an arbitrary value, I think that leaking even
>>> some information about what the value is no good.  Otherwise, you
>>> might imagine that we would allow /(int, int), because it only leaks
>>> in the second_arg = 0 case.  And you might imagine we'd allow -(int,
>>> int) because it only leaks in the case where an overflow occurs.  But
>>> of course the combination of the two allows writing something of the
>>> form 1/(a-constant) and getting it pushed down, and now you have the
>>> ability to probe for an arbitrary value.  So I think it's just no good
>>> to allow any leaking at all: otherwise it'll be unclear how safe it
>>> really is, especially when combinations of different functions or
>>> operators are involved.
>>>
>> OK. I checked list of the default leakproof functions.
>>
>> Functions that contains translation between date and timestamp(tz)
>> can raise an error depending on the supplied arguments.
>> Thus, I unmarked leakproof from them.
>>
>> In addition, varstr_cmp() contains translation from UTF-8 to UTF-16 on
>> win32 platform; that may raise an error if string contains a character that
>> is unavailable to translate.
>> Although I'm not sure which case unavailable to translate between them,
>> it seems to me hit on the basis not to leak what kind of information is
>> no good. Thus, related operator functions of bpchar and text got unmarked.
>> (Note that bpchareq, bpcharne, texteq and textne don't use it.)
>
> Can you rebase this?  It seems that the pg_proc.h and
> select_views{,_1}.out hunks no longer apply cleanly.
>
OK, the attached one is the rebased one.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

2012/1/26 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2012/1/25 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Sun, Jan 22, 2012 at 5:12 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> 2012/1/21 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>>> On Sat, Jan 21, 2012 at 3:59 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>>>> I marked the default leakproof function according to the criteria that
>>>>> does not leak contents of the argument.
>>>>> Indeed, timestamp_ne_timestamptz() has a code path that rises
>>>>> an error of "timestamp out of range" message. Is it a good idea to
>>>>> avoid mark "leakproof" on these functions also?
>>>>
>>>> I think that anything which looks at the data and uses that as a basis
>>>> for whether or not to throw an error is non-leakproof.  Even if
>>>> doesn't directly leak an arbitrary value, I think that leaking even
>>>> some information about what the value is no good.  Otherwise, you
>>>> might imagine that we would allow /(int, int), because it only leaks
>>>> in the second_arg = 0 case.  And you might imagine we'd allow -(int,
>>>> int) because it only leaks in the case where an overflow occurs.  But
>>>> of course the combination of the two allows writing something of the
>>>> form 1/(a-constant) and getting it pushed down, and now you have the
>>>> ability to probe for an arbitrary value.  So I think it's just no good
>>>> to allow any leaking at all: otherwise it'll be unclear how safe it
>>>> really is, especially when combinations of different functions or
>>>> operators are involved.
>>>>
>>> OK. I checked list of the default leakproof functions.
>>>
>>> Functions that contains translation between date and timestamp(tz)
>>> can raise an error depending on the supplied arguments.
>>> Thus, I unmarked leakproof from them.
>>>
>>> In addition, varstr_cmp() contains translation from UTF-8 to UTF-16 on
>>> win32 platform; that may raise an error if string contains a character that
>>> is unavailable to translate.
>>> Although I'm not sure which case unavailable to translate between them,
>>> it seems to me hit on the basis not to leak what kind of information is
>>> no good. Thus, related operator functions of bpchar and text got unmarked.
>>> (Note that bpchareq, bpcharne, texteq and textne don't use it.)
>>
>> Can you rebase this?  It seems that the pg_proc.h and
>> select_views{,_1}.out hunks no longer apply cleanly.
>>
> OK, the attached one is the rebased one.
>
I rebased the patch due to the updates of pg_proc.h.

Please see the newer one. Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Mon, Feb 13, 2012 at 7:51 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> I rebased the patch due to the updates of pg_proc.h.
>
> Please see the newer one. Thanks,

Thanks, committed. I think, though, that some further adjustment is
needed here, because you currently can't do ALTER FUNCTION ... NO
LEAKPROOF, which seems unacceptable. It's fairly clear why not,
though: you get a grammar conflict, because the parser allows this:

create or replace function z() returns int as $$select 1$$ language
sql set transaction not deferrable;

However, since that syntax doesn't actually work, I'm thinking we
could just refactor things a bit to reject that at the parser stage.
The attached patch adopts that approach. Anyone have a better idea?

I also think we ought to stick create_function_3 into one of the
parallel groups in the regression tests, if possible. Can you
investigate that?

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

2012/2/14 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Mon, Feb 13, 2012 at 7:51 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> I rebased the patch due to the updates of pg_proc.h.
>>
>> Please see the newer one. Thanks,
>
> Thanks, committed.  I think, though, that some further adjustment is
> needed here, because you currently can't do ALTER FUNCTION ... NO
> LEAKPROOF, which seems unacceptable.  It's fairly clear why not,
> though: you get a grammar conflict, because the parser allows this:
>
> create or replace function z() returns int as $$select 1$$ language
> sql set transaction not deferrable;
>
> However, since that syntax doesn't actually work, I'm thinking we
> could just refactor things a bit to reject that at the parser stage.
> The attached patch adopts that approach.  Anyone have a better idea?
>
I could not find out where is the origin of grammer conflicts, although
it does not conflict with any options within ALTER FUNCTION.

Do you think the idea of ALTER ... NOT LEAKPROOF should be
integrated within v9.2 timeline also?

> I also think we ought to stick create_function_3 into one of the
> parallel groups in the regression tests, if possible.  Can you
> investigate that?
>
Not yet. This test does not have dependency with other tests,
so, I'm optimistic to run create_function_3 concurrently.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Tue, Feb 14, 2012 at 4:55 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> I could not find out where is the origin of grammer conflicts, although
> it does not conflict with any options within ALTER FUNCTION.
>
> Do you think the idea of ALTER ... NOT LEAKPROOF should be
> integrated within v9.2 timeline also?

Yes. Did you notice that I attached a patch to make that work? I'll
commit that today or tomorrow unless someone comes up with a better
solution.

>> I also think we ought to stick create_function_3 into one of the
>> parallel groups in the regression tests, if possible.  Can you
>> investigate that?
>>
> Not yet. This test does not have dependency with other tests,
> so, I'm optimistic to run create_function_3 concurrently.

Me, too.

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

2012/2/14 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Tue, Feb 14, 2012 at 4:55 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> I could not find out where is the origin of grammer conflicts, although
>> it does not conflict with any options within ALTER FUNCTION.
>>
>> Do you think the idea of ALTER ... NOT LEAKPROOF should be
>> integrated within v9.2 timeline also?
>
> Yes.  Did you notice that I attached a patch to make that work?  I'll
> commit that today or tomorrow unless someone comes up with a better
> solution.
>
Yes. I'll be available to work on the feature based on this patch.
It was a headache of mine to implement alter statement to add/remove
leakproof attribute.

>>> I also think we ought to stick create_function_3 into one of the
>>> parallel groups in the regression tests, if possible.  Can you
>>> investigate that?
>>>
>> Not yet. This test does not have dependency with other tests,
>> so, I'm optimistic to run create_function_3 concurrently.
>
> Me, too.
>
I tried to move create_function_3 into the group of create_view and
create_index, then it works correctly.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

The attached patch is additional regression tests of ALTER FUNCTION with
LEAKPROOF based on your patch.
It also moves create_function_3 into the group with create_aggregate and so on.

Thanks,

2012/2/14 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2012/2/14 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Tue, Feb 14, 2012 at 4:55 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> I could not find out where is the origin of grammer conflicts, although
>>> it does not conflict with any options within ALTER FUNCTION.
>>>
>>> Do you think the idea of ALTER ... NOT LEAKPROOF should be
>>> integrated within v9.2 timeline also?
>>
>> Yes.  Did you notice that I attached a patch to make that work?  I'll
>> commit that today or tomorrow unless someone comes up with a better
>> solution.
>>
> Yes. I'll be available to work on the feature based on this patch.
> It was a headache of mine to implement alter statement to add/remove
> leakproof attribute.
>
>>>> I also think we ought to stick create_function_3 into one of the
>>>> parallel groups in the regression tests, if possible.  Can you
>>>> investigate that?
>>>>
>>> Not yet. This test does not have dependency with other tests,
>>> so, I'm optimistic to run create_function_3 concurrently.
>>
>> Me, too.
>>
> I tried to move create_function_3 into the group of create_view and
> create_index, then it works correctly.
>
> Thanks,
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Wed, Feb 15, 2012 at 6:14 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> The attached patch is additional regression tests of ALTER FUNCTION with
> LEAKPROOF based on your patch.
> It also moves create_function_3 into the group with create_aggregate and so on.

Committed, thanks.

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