From: | Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Noah Misch <noah(at)leadboat(dot)com>, Kevin Grittner <Kevin(dot)Grittner(at)wicourts(dot)gov>, Kohei(dot)Kaigai(at)emea(dot)nec(dot)com, thom(at)linux(dot)com, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: [v9.2] LEAKPROOF attribute of FUNCTION (Re: [v9.2] Fix Leaky View Problem) |
Date: | 2012-01-21 08:59:37 |
Message-ID: | CADyhKSWtacu2wrBPhjpgwcGHLsf=EJLPMid8_V=Ax4sZu-vj2w@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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>
Attachment | Content-Type | Size |
---|---|---|
pgsql-v9.2-leakproof-function.v2.patch.gz | application/x-gzip | 77.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Geoghegan | 2012-01-21 09:24:07 | Re: Progress on fast path sorting, btree index creation time |
Previous Message | Kääriäinen Anssi | 2012-01-21 08:46:56 | Re: REVIEW: pg_stat_statements with query tree based normalization |