Re: [v9.2] LEAKPROOF attribute of FUNCTION (Re: [HACKERS] [v9.2] Fix Leaky View Problem)
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 kai...@kaigai.gr.jp: 2012/2/14 Robert Haas robertmh...@gmail.com: On Tue, Feb 14, 2012 at 4:55 AM, Kohei KaiGai kai...@kaigai.gr.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 kai...@kaigai.gr.jp -- KaiGai Kohei kai...@kaigai.gr.jp pgsql-v9.2-alter-function-leakproof-regtest.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [v9.2] LEAKPROOF attribute of FUNCTION (Re: [HACKERS] [v9.2] Fix Leaky View Problem)
On Wed, Feb 15, 2012 at 6:14 AM, Kohei KaiGai kai...@kaigai.gr.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [v9.2] LEAKPROOF attribute of FUNCTION (Re: [HACKERS] [v9.2] Fix Leaky View Problem)
2012/2/14 Robert Haas robertmh...@gmail.com: On Mon, Feb 13, 2012 at 7:51 AM, Kohei KaiGai kai...@kaigai.gr.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 kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [v9.2] LEAKPROOF attribute of FUNCTION (Re: [HACKERS] [v9.2] Fix Leaky View Problem)
On Tue, Feb 14, 2012 at 4:55 AM, Kohei KaiGai kai...@kaigai.gr.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [v9.2] LEAKPROOF attribute of FUNCTION (Re: [HACKERS] [v9.2] Fix Leaky View Problem)
2012/2/14 Robert Haas robertmh...@gmail.com: On Tue, Feb 14, 2012 at 4:55 AM, Kohei KaiGai kai...@kaigai.gr.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 kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [v9.2] LEAKPROOF attribute of FUNCTION (Re: [HACKERS] [v9.2] Fix Leaky View Problem)
On Mon, Feb 13, 2012 at 7:51 AM, Kohei KaiGai kai...@kaigai.gr.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 not-leakproof.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [v9.2] LEAKPROOF attribute of FUNCTION (Re: [HACKERS] [v9.2] Fix Leaky View Problem)
On Sun, Jan 22, 2012 at 5:12 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
2012/1/21 Robert Haas robertmh...@gmail.com:
On Sat, Jan 21, 2012 at 3:59 AM, Kohei KaiGai kai...@kaigai.gr.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Sun, Jan 22, 2012 at 5:57 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: This passes installcheck initially. Then upon second invocation of installcheck, it fails. It creates the role alice, and doesn't clean it up. On next invocation alice already exists and cases a failure in test select_views. Thanks for your pointing out. The attached patch adds cleaning-up part of object being defined within this test; includes user alice. Urp. I failed to notice this patch and committed a different fix for the problem pointed out by Jeff. I'm inclined to think it's OK to leave the non-shared objects behind - among other things, if people are testing pg_upgrade using the regression database, this will help to ensure that pg_dump is handling security_barrier views correctly. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Tue, Jan 17, 2012 at 7:08 PM, Robert Haas robertmh...@gmail.com wrote: On Sun, Jan 8, 2012 at 10:32 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: I guess you concerned about that expected/select_views_1.out is patched, not expected/select_views.out. I'm not sure the reason why regression test script tries to make diff between results/select_views and expected/select_views_1.out. select_views.out and select_views_1.out are alternate expected output files. The regression tests pass if the actual output matches either one. Thus, you have to patch both. It was new for me. The attached patch updates both of the expected files, however, I'm not certain whether select_view.out is suitable, or not, because my results/select_view.out matched with expected/select_view_1.out. Committed. We'll see what the buildfarm thinks. This passes installcheck initially. Then upon second invocation of installcheck, it fails. It creates the role alice, and doesn't clean it up. On next invocation alice already exists and cases a failure in test select_views. Cheers, Jeff -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
2012/1/21 Jeff Janes jeff.ja...@gmail.com: On Tue, Jan 17, 2012 at 7:08 PM, Robert Haas robertmh...@gmail.com wrote: On Sun, Jan 8, 2012 at 10:32 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: I guess you concerned about that expected/select_views_1.out is patched, not expected/select_views.out. I'm not sure the reason why regression test script tries to make diff between results/select_views and expected/select_views_1.out. select_views.out and select_views_1.out are alternate expected output files. The regression tests pass if the actual output matches either one. Thus, you have to patch both. It was new for me. The attached patch updates both of the expected files, however, I'm not certain whether select_view.out is suitable, or not, because my results/select_view.out matched with expected/select_view_1.out. Committed. We'll see what the buildfarm thinks. This passes installcheck initially. Then upon second invocation of installcheck, it fails. It creates the role alice, and doesn't clean it up. On next invocation alice already exists and cases a failure in test select_views. Thanks for your pointing out. The attached patch adds cleaning-up part of object being defined within this test; includes user alice. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp pgsql-v9.2-fix-regtest-select-views-cleanup.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [v9.2] LEAKPROOF attribute of FUNCTION (Re: [HACKERS] [v9.2] Fix Leaky View Problem)
On Sat, Jan 21, 2012 at 3:59 AM, Kohei KaiGai kai...@kaigai.gr.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [v9.2] LEAKPROOF attribute of FUNCTION (Re: [HACKERS] [v9.2] Fix Leaky View Problem)
On Sun, Jan 8, 2012 at 10:52 AM, Kohei KaiGai kai...@kaigai.gr.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.0 -0500 --- /Users/rhaas/pgsql/src/test/regress/results/create_function_3.out 2012-01-17 22:14:48.0 -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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Sun, Jan 8, 2012 at 10:32 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: I guess you concerned about that expected/select_views_1.out is patched, not expected/select_views.out. I'm not sure the reason why regression test script tries to make diff between results/select_views and expected/select_views_1.out. select_views.out and select_views_1.out are alternate expected output files. The regression tests pass if the actual output matches either one. Thus, you have to patch both. It was new for me. The attached patch updates both of the expected files, however, I'm not certain whether select_view.out is suitable, or not, because my results/select_view.out matched with expected/select_view_1.out. Committed. We'll see what the buildfarm thinks. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
I guess you concerned about that expected/select_views_1.out is patched, not expected/select_views.out. I'm not sure the reason why regression test script tries to make diff between results/select_views and expected/select_views_1.out. select_views.out and select_views_1.out are alternate expected output files. The regression tests pass if the actual output matches either one. Thus, you have to patch both. It was new for me. The attached patch updates both of the expected files, however, I'm not certain whether select_view.out is suitable, or not, because my results/select_view.out matched with expected/select_view_1.out. 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. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp pgsql-regtest-leaky-views.2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Tue, Jan 3, 2012 at 12:11 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote: 2011/12/23 Robert Haas robertmh...@gmail.com: On Fri, Dec 23, 2011 at 5:56 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: I'd like the regression test on select_view test being committed also to detect unexpected changed in the future. How about it? Can you resend that as a separate patch? I remember there were some things I didn't like about it, but I don't remember what they were at the moment... Sorry for this late response. The attached one is patch of the regression test that checks scenario of malicious function with/without security_barrier option. I guess you concerned about that expected/select_views_1.out is patched, not expected/select_views.out. I'm not sure the reason why regression test script tries to make diff between results/select_views and expected/select_views_1.out. select_views.out and select_views_1.out are alternate expected output files. The regression tests pass if the actual output matches either one. Thus, you have to patch both. 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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
2011/12/23 Robert Haas robertmh...@gmail.com: On Fri, Dec 23, 2011 at 5:56 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: I'd like the regression test on select_view test being committed also to detect unexpected changed in the future. How about it? Can you resend that as a separate patch? I remember there were some things I didn't like about it, but I don't remember what they were at the moment... Sorry for this late response. The attached one is patch of the regression test that checks scenario of malicious function with/without security_barrier option. I guess you concerned about that expected/select_views_1.out is patched, not expected/select_views.out. I'm not sure the reason why regression test script tries to make diff between results/select_views and expected/select_views_1.out. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp pgsql-regtest-leaky-views.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
2011/12/22 Robert Haas robertmh...@gmail.com: On Mon, Dec 12, 2011 at 12:00 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote: The v8.option-2 add checks around examine_simple_variable, and prevent to reference statistical data, if Var node tries to reference relation with security-barrier attribute. I adopted this approach, and committed this. Thanks for your help and efforts. I'd like the regression test on select_view test being committed also to detect unexpected changed in the future. How about it? Best regards, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Fri, Dec 23, 2011 at 5:56 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: I'd like the regression test on select_view test being committed also to detect unexpected changed in the future. How about it? Can you resend that as a separate patch? I remember there were some things I didn't like about it, but I don't remember what they were at the moment... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Mon, Dec 12, 2011 at 12:00 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote: The v8.option-2 add checks around examine_simple_variable, and prevent to reference statistical data, if Var node tries to reference relation with security-barrier attribute. I adopted this approach, and committed this. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
The attached patches are cut-off version based on the latest Robert's updates. The v8.regtest adds regression test cases on variable leaky-view scenarios with/without security-barrier property. The v8.option-1 add checks around restriction_selectivity, and prevent to invoke estimator function if Var node touches relation with security- barrier attribute. The v8.option-2 add checks around examine_simple_variable, and prevent to reference statistical data, if Var node tries to reference relation with security-barrier attribute. (And, it shall be marked as leakproof) I initially thought restriction_selectivity called by clause_selectivity is the best point to add checks, however, I reconsidered it might not be the origin of this problem. As long as user-defined functions acquires control on selectivity estimation of operators, same problems can be re-produced; if someone tries to reference unrelated data within estimator. This scenario is normally prevented, because only superuser can define a function that can bypass permission checks to reference internal data structures; using untrusted procedural-language. If my conclusion is right, what we should fix up is built-in estimators side, and we should enforce estimator function being leakproof, even though we still allow unprivileged users to define operators. Thanks, 2011/12/11 Kohei KaiGai kai...@kaigai.gr.jp: 2011/12/10 Kohei KaiGai kai...@kaigai.gr.jp: 2011/12/9 Robert Haas robertmh...@gmail.com: I feel like there must be some logic in the planner somewhere that is looking through the subquery RTE and figuring out that safe_foo.a is really the same variable as foo.a, and which therefore feels entitled to apply !!'s selectivity estimator to foo.a's statistics. If that's the case, it might be possible to handicap that logic so that when the security_barrier flag is set, it doesn't do that, and instead treats safe_foo.a as a black box. That would, obviously, degrade the quality of complex plans involving security views, but I think we should focus on getting something that meets our security goals first and then try to improve performance later. I tried to investigate the code around size-estimation, and it seems to me here is two candidates to put this type of checks. The one is examine_simple_variable() that is used to pull a datum from statistic information, but it locates on the code path restriction estimator of operators; so user controlable, although it requires least code changes just after if (rte-rtekind == RTE_SUBQUERY). The other is clause_selectivity(). Its code path is not user controlable, so we can apply necessary checks to prevent qualifier that reference variable come from sub-query with security-barrier. In my sense, clause_selectivity() is better place to apply this type of checks. But, on the other hand, it provides get_relation_stats_hook to allow extensions to control references to statistic data. So, I wonder whether the PG core assumes this routine covers all the code path here? The attached patch adds checks around invocation of selectivity estimator functions, and it changes behavior of the estimator, if the supplied operator tries to touch variables come from security-barrier relations. Then, it fixes the problem you mentioned. postgres=# explain select * from safe_foo where a !! 0; QUERY PLAN - Subquery Scan on safe_foo (cost=0.00..2.70 rows=3 width=4) Filter: (safe_foo.a !! 0) - Seq Scan on foo (cost=0.00..1.14 rows=6 width=4) Filter: (a 5) (4 rows) However, I'm still a bit skeptical on my patch, because it still allows to invoke estimator function when operator's argument does not touch values originated from security-barrier relation. In the case when oprrest or oprjoin are implemented without our regular convention (E.g, it anyway reference whole of statistical data), it will break this solution. Of course, it is an assumption that we cannot prevent any attack using binary modules, so we need to say use it your own risk if people tries to use extensional modules. And, we also need to keep the built-in code secure. Some of built-in estimator functions (such as eqsel) provides a feature that invokes operator function with arguments originated from pg_statistics table. It didn't harmless, however, we got understand that this logic can be used to break row-level security. So, I begin to consider the routines to be revised are some of built-in functions to be used for estimator functions; such as eqsel and ... These function eventually reference statistical data at examine_variable. It might be a better approach to add checks whether invocation of the supplied operator possibly leaks contents to be invisible. It seems to me the Idea of the attached patch depends on something internal stuff of existing built-in estimator functions... Thanks,
Re: [HACKERS] [v9.2] Fix Leaky View Problem
2011/12/9 Robert Haas robertmh...@gmail.com: On Thu, Dec 8, 2011 at 5:17 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote: My first impression remind me an idea that I proposed before, even though it got negative response due to user visible changes. It requires superuser privilege to create new operators, since we assume superuser does not set up harmful configuration. I don't think that's acceptable from a usability point of view; this feature is important, but not important enough to go start ripping out other features that people are already using, like non-superuser operators. I'm also pretty skeptical that it would fix the problem, because the superuser might fail to realize that creating an operator was going to create this type of security exposure. After all, you and I also failed to realize that, so it's obviously a fairly subtle problem. OK, I agree with your opinion. It may stand on a fiction story that superuser understand all effects and risk of his operations. If this assumption get broken, system's security is also broken. I feel like there must be some logic in the planner somewhere that is looking through the subquery RTE and figuring out that safe_foo.a is really the same variable as foo.a, and which therefore feels entitled to apply !!'s selectivity estimator to foo.a's statistics. If that's the case, it might be possible to handicap that logic so that when the security_barrier flag is set, it doesn't do that, and instead treats safe_foo.a as a black box. That would, obviously, degrade the quality of complex plans involving security views, but I think we should focus on getting something that meets our security goals first and then try to improve performance later. I tried to investigate the code around size-estimation, and it seems to me here is two candidates to put this type of checks. The one is examine_simple_variable() that is used to pull a datum from statistic information, but it locates on the code path restriction estimator of operators; so user controlable, although it requires least code changes just after if (rte-rtekind == RTE_SUBQUERY). The other is clause_selectivity(). Its code path is not user controlable, so we can apply necessary checks to prevent qualifier that reference variable come from sub-query with security-barrier. In my sense, clause_selectivity() is better place to apply this type of checks. But, on the other hand, it provides get_relation_stats_hook to allow extensions to control references to statistic data. So, I wonder whether the PG core assumes this routine covers all the code path here? In addition, I also consider the case when we add a functionality that forcibly adds restriction on WHERE clause of regular tables in the future version, like: SELECT * FROM t WHERE a 0; == SELECT * FROM t WHERE sepgsql_policy(selinux_label) AND a 0; Probably, same solution will be available to avoid unintentional references to pg_statistic; as long as security_barrier is set on rte of regular tables. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Thu, Dec 8, 2011 at 5:17 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote: My first impression remind me an idea that I proposed before, even though it got negative response due to user visible changes. It requires superuser privilege to create new operators, since we assume superuser does not set up harmful configuration. I don't think that's acceptable from a usability point of view; this feature is important, but not important enough to go start ripping out other features that people are already using, like non-superuser operators. I'm also pretty skeptical that it would fix the problem, because the superuser might fail to realize that creating an operator was going to create this type of security exposure. After all, you and I also failed to realize that, so it's obviously a fairly subtle problem. I feel like there must be some logic in the planner somewhere that is looking through the subquery RTE and figuring out that safe_foo.a is really the same variable as foo.a, and which therefore feels entitled to apply !!'s selectivity estimator to foo.a's statistics. If that's the case, it might be possible to handicap that logic so that when the security_barrier flag is set, it doesn't do that, and instead treats safe_foo.a as a black box. That would, obviously, degrade the quality of complex plans involving security views, but I think we should focus on getting something that meets our security goals first and then try to improve performance later. (For example, I am fairly certain that only a superuser can install a new selectivity estimator; so perhaps we could allow selectivity estimators to be signaled with the information that a security view interposes or not, and then they can make an estimator-specific decision on how to punt; but on the other hand that might be a stupid idea; so for step #1 let's just figure out how to batten down the hatches.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
2011/12/8 Robert Haas robertmh...@gmail.com: On Sat, Dec 3, 2011 at 3:19 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: I rebased my patch set. New functions in pg_proc.h prevented to apply previous revision cleanly. Here is no functional changes. I was thinking that my version of this (attached to an email from earlier today) might be about ready to commit. But while I was trolling through the archives on this problem trying to figure out who to credit, I found an old complaint of Tom's that we never fixed, and which represents a security exposure for this patch: rhaas=# create table foo (a integer); CREATE TABLE rhaas=# insert into foo select generate_series(1,10); INSERT 0 10 rhaas=# insert into foo values (1); INSERT 0 1 rhaas=# analyze foo; ANALYZE rhaas=# create view safe_foo with (security_barrier) as select * from foo where a 5; CREATE VIEW rhaas=# grant select on safe_foo to bob; GRANT Secure in the knowledge that Bob will only be able to see rows where a is 6 or higher, we go to bed. But Bob finds a way to outsmart us: rhaas= create or replace function leak(integer,integer) returns boolean as $$begin raise notice 'leak % %', $1, $2; return false; end$$ language plpgsql; CREATE FUNCTION rhaas= create operator !! (procedure = leak, leftarg = integer, rightarg = integer, restrict = eqsel); CREATE OPERATOR rhaas= explain select * from safe_foo where a !! 0; NOTICE: leak 1 0 QUERY PLAN - Subquery Scan on safe_foo (cost=0.00..2.70 rows=1 width=4) Filter: (safe_foo.a !! 0) - Seq Scan on foo (cost=0.00..1.14 rows=6 width=4) Filter: (a 5) (4 rows) OOPS. The *executor* has been persuaded not to apply the possibly-nefarious operator !! to the data until after applying the security-critical qual a 5. But the *planner* has no such compunctions, and has cheerfully leaked the most common value in the table, which the user wasn't supposed to see. I guess it's hopeless to suppose that we're going to completely conceal the list of MCVs from the user, since it might change the plan - and even if ProcessUtility_hook or somesuch is used to disable EXPLAIN, the user can still try to ferret out the MCVs via a timing attack. That having been said, the above behavior doesn't sit well with me: letting the user probe for MCVs via a timing attack or a plan change is one thing; printing them out on request is a little bit too convenient for my taste. :-( Sorry, I missed this scenario, and have not investigated this code path in detail yet. My first impression remind me an idea that I proposed before, even though it got negative response due to user visible changes. It requires superuser privilege to create new operators, since we assume superuser does not set up harmful configuration. I still think it is an idea. Or, maybe, we can adopt a bit weaker restriction; functions being used to operators must have leakproof property. Is it worthful to have a discussion again? Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Mon, Sep 26, 2011 at 3:59 PM, Tom Lane t...@sss.pgh.pa.us wrote: Kohei KaiGai kai...@kaigai.gr.jp writes: Sorry, are you saying the current (in other words, rte-security_barrier stores the state of reloption) approach is not a good idea? Yes. I think the same as Robert: the way to handle this is to store it in RelOptInfo for the duration of planning, and pull it from the catalogs during planner startup (cf plancat.c). Having looked at this more, I'm starting to believe KaiGai has this part right after all. The trouble is that the rewriter does this: /* * Now, plug the view query in as a subselect, replacing the relation's * original RTE. */ rte = rt_fetch(rt_index, parsetree-rtable); rte-rtekind = RTE_SUBQUERY; rte-relid = InvalidOid; rte-subquery = rule_action; rte-inh = false; /* must not be set for a subquery */ In other words, by the time the planner comes along and tries to decide whether or not it should flatten this subquery, the view has already been rewritten into a subquery - and that subquery is in most respects indistinguishable from a subquery that the user wrote directly. There is one difference: the permission check that would have been done against the view gets attached to the OLD entry in the subquery's range table. It would probably be possible to make this work by having the code paths that need to know whether or not a given subquery originated from a security-barrier-enabled view do that same trick: peek down into the OLD entry in the subquery rangetable, extract the view OID from there, and go check its reloptions. But that seems awfully complicated and error-prone, hence my feeling that just flagging the subquery explicitly is probably a better approach. One other possibility that comes to mind is that, instead of adding bool security_view to the RTE, we could instead add a new RTEKind, something like RTE_SECURITY_VIEW. That would mean going through and finding all the places that refer to RTE_SUBQUERY and adjusting them to handle RTE_SECURITY_VIEW in either the same way or differently as may be appropriate. The possible advantage of this approach is that it doesn't bloat the RTE structure (and stored rules that use it) with an additional attribute that (I think) will always be false - because security_barrier can only be set on a subquery RTE after rewriting has happened, and stored rules are haven't been rewritten yet. It might also force people to think a bit more carefully about how security views should be handled during future code changes, which could also be viewed as a plus. I'm attaching my current version of KaiGai's patch (with substantial cleanup of the comments and documentation, and some other changes) for reference. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company leaky-views-20111207.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
Robert Haas robertmh...@gmail.com writes: Having looked at this more, I'm starting to believe KaiGai has this part right after all. Yeah, you have a point. The rewriter is intentionally trying to make an expanded view look just the same as an in-line SELECT-in-FROM, and we need it to be easier to distinguish them. One other possibility that comes to mind is that, instead of adding bool security_view to the RTE, we could instead add a new RTEKind, something like RTE_SECURITY_VIEW. That would mean going through and finding all the places that refer to RTE_SUBQUERY and adjusting them to handle RTE_SECURITY_VIEW in either the same way or differently as may be appropriate. The possible advantage of this approach is that it doesn't bloat the RTE structure (and stored rules that use it) with an additional attribute that (I think) will always be false - because security_barrier can only be set on a subquery RTE after rewriting has happened, and stored rules are haven't been rewritten yet. It might also force people to think a bit more carefully about how security views should be handled during future code changes, which could also be viewed as a plus. Hmm. The question is whether the places where we need to care about this would naturally be looking at RTEKind anyway. If they are, or many are, then I think this might be a good idea. However if a lot of the action is elsewhere then I don't know if we get much leverage from the new RTEKind. I haven't read the patch lately so can't opine on that. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Wed, Dec 7, 2011 at 1:45 PM, Tom Lane t...@sss.pgh.pa.us wrote: One other possibility that comes to mind is that, instead of adding bool security_view to the RTE, we could instead add a new RTEKind, something like RTE_SECURITY_VIEW. That would mean going through and finding all the places that refer to RTE_SUBQUERY and adjusting them to handle RTE_SECURITY_VIEW in either the same way or differently as may be appropriate. The possible advantage of this approach is that it doesn't bloat the RTE structure (and stored rules that use it) with an additional attribute that (I think) will always be false - because security_barrier can only be set on a subquery RTE after rewriting has happened, and stored rules are haven't been rewritten yet. It might also force people to think a bit more carefully about how security views should be handled during future code changes, which could also be viewed as a plus. Hmm. The question is whether the places where we need to care about this would naturally be looking at RTEKind anyway. If they are, or many are, then I think this might be a good idea. However if a lot of the action is elsewhere then I don't know if we get much leverage from the new RTEKind. I haven't read the patch lately so can't opine on that. *reads through the code* It looks to me like most places that look at RTE_SUBQUERY really have no reason to care about this. So probably it's just as well to have a separate flag for it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Sat, Dec 3, 2011 at 3:19 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: I rebased my patch set. New functions in pg_proc.h prevented to apply previous revision cleanly. Here is no functional changes. I was thinking that my version of this (attached to an email from earlier today) might be about ready to commit. But while I was trolling through the archives on this problem trying to figure out who to credit, I found an old complaint of Tom's that we never fixed, and which represents a security exposure for this patch: rhaas=# create table foo (a integer); CREATE TABLE rhaas=# insert into foo select generate_series(1,10); INSERT 0 10 rhaas=# insert into foo values (1); INSERT 0 1 rhaas=# analyze foo; ANALYZE rhaas=# create view safe_foo with (security_barrier) as select * from foo where a 5; CREATE VIEW rhaas=# grant select on safe_foo to bob; GRANT Secure in the knowledge that Bob will only be able to see rows where a is 6 or higher, we go to bed. But Bob finds a way to outsmart us: rhaas= create or replace function leak(integer,integer) returns boolean as $$begin raise notice 'leak % %', $1, $2; return false; end$$ language plpgsql; CREATE FUNCTION rhaas= create operator !! (procedure = leak, leftarg = integer, rightarg = integer, restrict = eqsel); CREATE OPERATOR rhaas= explain select * from safe_foo where a !! 0; NOTICE: leak 1 0 QUERY PLAN - Subquery Scan on safe_foo (cost=0.00..2.70 rows=1 width=4) Filter: (safe_foo.a !! 0) - Seq Scan on foo (cost=0.00..1.14 rows=6 width=4) Filter: (a 5) (4 rows) OOPS. The *executor* has been persuaded not to apply the possibly-nefarious operator !! to the data until after applying the security-critical qual a 5. But the *planner* has no such compunctions, and has cheerfully leaked the most common value in the table, which the user wasn't supposed to see. I guess it's hopeless to suppose that we're going to completely conceal the list of MCVs from the user, since it might change the plan - and even if ProcessUtility_hook or somesuch is used to disable EXPLAIN, the user can still try to ferret out the MCVs via a timing attack. That having been said, the above behavior doesn't sit well with me: letting the user probe for MCVs via a timing attack or a plan change is one thing; printing them out on request is a little bit too convenient for my taste. :-( -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
I rebased my patch set. New functions in pg_proc.h prevented to apply previous revision cleanly. Here is no functional changes. 2011/11/3 Kohei KaiGai kai...@kaigai.gr.jp: 2011/11/2 Tom Lane t...@sss.pgh.pa.us: Kohei KaiGai kai...@kaigai.gr.jp writes: The reason why I redefined the relid of RangeTblEntry is to avoid the problem when security_barrier attribute get changed by concurrent transactions between rewriter and planenr stage. This is complete nonsense. If the information is being injected into the querytree by the rewriter, it's sufficient to assume that it's up to date. Were it not so, we'd have problems with CREATE OR REPLACE RULE, too. I revised the patches to revert redefinition in relid of RangeTblEntry, and add a flag of security_barrier. I seems to work fine, even if view's property was changed between rewriter and planner stage. postgres=# CREATE VIEW v1 WITH (security_barrier) AS SELECT * FROM t1 WHERE a % 2 = 0; CREATE VIEW postgres=# PREPARE p1 AS SELECT * FROM v1 WHERE f_leak(b); PREPARE postgres=# EXECUTE p1; NOTICE: f_leak = bbb NOTICE: f_leak = ddd a | b ---+- 2 | bbb 4 | ddd (2 rows) postgres=# ALTER VIEW v1 SET (security_barrier=false); ALTER VIEW postgres=# EXECUTE p1; NOTICE: f_leak = aaa NOTICE: f_leak = bbb NOTICE: f_leak = ccc NOTICE: f_leak = ddd NOTICE: f_leak = eee a | b ---+- 2 | bbb 4 | ddd (2 rows) postgres=# ALTER VIEW v1 SET (security_barrier=true); ALTER VIEW postgres=# EXECUTE p1; NOTICE: f_leak = bbb NOTICE: f_leak = ddd a | b ---+- 2 | bbb 4 | ddd (2 rows) Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Wed, Nov 2, 2011 at 7:34 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: [ new patch, with example query plans ] I like the look of those query plans. Redefining the RangeTblEntry's relid field to be valid for either a table or a subquery that originated from a view seems problematic to me, though. For one thing, it's hard to say how much other code assumes that field to be valid only for a table. For example, you didn't update _readRangeTblEntry(), and I wouldn't bet on that being the only place that needs fixing. For another thing, instead of changing the meaning of the relid field, you could just leave that alone and instead add a bool security_barrier field that caches the answer; ApplyRetrieveRule() has the Relation object and could set that field appropriately, and then subquery_was_security_barrier() wouldn't need a syscache lookup. Now, the obvious objection is that the security-barrier attribute might change between the time the RTE is created and the time that it gets used. But if that's a danger, then presumably the whole view could also change, in which case the Query object would be pointing to the wrong data anyway. I'm not sure I fully understand the details here, but it seems like it ought to be safe to cache the security_barrier attribute any place it's safe to cache the Query itself. It certainly doesn't seem right to think that we might end up using a new value of the security_barrier attribute with an old query, or the other way around. So something seems funky here. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
2011/11/2 Robert Haas robertmh...@gmail.com: On Wed, Nov 2, 2011 at 7:34 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: [ new patch, with example query plans ] I like the look of those query plans. Redefining the RangeTblEntry's relid field to be valid for either a table or a subquery that originated from a view seems problematic to me, though. For one thing, it's hard to say how much other code assumes that field to be valid only for a table. For example, you didn't update _readRangeTblEntry(), and I wouldn't bet on that being the only place that needs fixing. For another thing, instead of changing the meaning of the relid field, you could just leave that alone and instead add a bool security_barrier field that caches the answer; ApplyRetrieveRule() has the Relation object and could set that field appropriately, and then subquery_was_security_barrier() wouldn't need a syscache lookup. Now, the obvious objection is that the security-barrier attribute might change between the time the RTE is created and the time that it gets used. But if that's a danger, then presumably the whole view could also change, in which case the Query object would be pointing to the wrong data anyway. I'm not sure I fully understand the details here, but it seems like it ought to be safe to cache the security_barrier attribute any place it's safe to cache the Query itself. It certainly doesn't seem right to think that we might end up using a new value of the security_barrier attribute with an old query, or the other way around. So something seems funky here. The reason why I redefined the relid of RangeTblEntry is to avoid the problem when security_barrier attribute get changed by concurrent transactions between rewriter and planenr stage. Of course, I'm not 100% sure whether we have a routine that assumes valid relid of RangeTblEntry is regular table, or not, although we could run the regression test correctly. As I examined before, updates of the issued pg_class shall invalidate prepared statements that assumed a particular security_barrier (maybe, PlanCacheRelCallback does this work?), so it is unavailable to use old plans based on old view definition. If we want to avoid syscache lookup on subquery_was_security_barrier(), I think it is a feasible idea to hold the value of security_barrier within RTE. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
Kohei KaiGai kai...@kaigai.gr.jp writes: The reason why I redefined the relid of RangeTblEntry is to avoid the problem when security_barrier attribute get changed by concurrent transactions between rewriter and planenr stage. This is complete nonsense. If the information is being injected into the querytree by the rewriter, it's sufficient to assume that it's up to date. Were it not so, we'd have problems with CREATE OR REPLACE RULE, too. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
So, I will split the patch into two parts as follows, in the next commit fest. Part-1) Views with security_barrier reloption The part-1 portion provides views security_barrier reloption; that enables to keep sub-queries unflatten in the prepjoin.c stage. In addition, these sub-queries (that originally come from views with security_barrier option) don't allow to push down qualifiers from upper level. It shall prevent both of the problematic scenarios. Part-2) Functions with leakproof attribute The part-2 portion provides functions leakproof attribute; that enables to push down leakproof functions into sub-queries, even if it originally come from security views. It shall minimize performance damages when we use view for row-level security purpose. 2011/10/19 Tom Lane t...@sss.pgh.pa.us: Robert Haas robertmh...@gmail.com writes: Well, there's clearly some way to prevent pushdown from happening, because sticking a LIMIT in there does the trick... I already pointed you at subquery_is_pushdown_safe ... regards, tom lane -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Fri, Oct 21, 2011 at 10:36 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: So, I will split the patch into two parts as follows, in the next commit fest. Part-1) Views with security_barrier reloption The part-1 portion provides views security_barrier reloption; that enables to keep sub-queries unflatten in the prepjoin.c stage. In addition, these sub-queries (that originally come from views with security_barrier option) don't allow to push down qualifiers from upper level. It shall prevent both of the problematic scenarios. Part-2) Functions with leakproof attribute The part-2 portion provides functions leakproof attribute; that enables to push down leakproof functions into sub-queries, even if it originally come from security views. It shall minimize performance damages when we use view for row-level security purpose. Sounds reasonable. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
2011/10/19 Tom Lane t...@sss.pgh.pa.us: Robert Haas robertmh...@gmail.com writes: On Sun, Oct 16, 2011 at 4:46 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: I tried to reproduce the scenario with enough small from/join_collapse_limit (typically 1), but it allows to push down qualifiers into the least scan plan. Hmm, you're right. LIMIT 10 prevents qual pushdown, but hitting from_collapse_limit/join_collapse_limit apparently doesn't. I could have sworn I've seen this work the other way, but I guess not. No, the collapse_limit variables are entirely unrelated to subquery flattening, or to qual pushdown for that matter. They only restrict the number of join paths we consider. And we will attempt to push down quals into an unflattened subquery, too, if it looks safe. See subquery_is_pushdown_safe, qual_is_pushdown_safe, etc in allpaths.c. I tried to observe the behavior with a bit modification of is_simple_subquery that become to return 'false' always. (It is a simulation if and when a view with security_barrier would be given.) The expected behavior is to keep sub-query without flatten. However, the externally provided qualifiers are correctly pushed down. Do we need to focus on the code around above functions rather than distribute_qual_to_rels, to prevent undesirable pushing-down across security barrier? postgres=# CREATE VIEW v1 AS SELECT * FROM t1 WHERE a 100; CREATE VIEW postgres=# CREATE VIEW v2 AS SELECT * FROM t2 JOIN t3 ON x = s; CREATE VIEW postgres=# EXPLAIN SELECT * FROM v1 WHERE b = 'bbb'; QUERY PLAN Seq Scan on t1 (cost=0.00..28.45 rows=2 width=36) Filter: ((a 100) AND (b = 'bbb'::text)) (2 rows) postgres=# EXPLAIN SELECT * FROM v2 WHERE t = 'ttt'; QUERY PLAN Hash Join (cost=25.45..52.73 rows=37 width=72) Hash Cond: (t2.x = t3.s) - Seq Scan on t2 (cost=0.00..22.30 rows=1230 width=36) - Hash (cost=25.38..25.38 rows=6 width=36) - Seq Scan on t3 (cost=0.00..25.38 rows=6 width=36) Filter: (t = 'ttt'::text) (6 rows) Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Wed, Oct 19, 2011 at 6:35 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: 2011/10/19 Tom Lane t...@sss.pgh.pa.us: Robert Haas robertmh...@gmail.com writes: On Sun, Oct 16, 2011 at 4:46 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: I tried to reproduce the scenario with enough small from/join_collapse_limit (typically 1), but it allows to push down qualifiers into the least scan plan. Hmm, you're right. LIMIT 10 prevents qual pushdown, but hitting from_collapse_limit/join_collapse_limit apparently doesn't. I could have sworn I've seen this work the other way, but I guess not. No, the collapse_limit variables are entirely unrelated to subquery flattening, or to qual pushdown for that matter. They only restrict the number of join paths we consider. And we will attempt to push down quals into an unflattened subquery, too, if it looks safe. See subquery_is_pushdown_safe, qual_is_pushdown_safe, etc in allpaths.c. I tried to observe the behavior with a bit modification of is_simple_subquery that become to return 'false' always. (It is a simulation if and when a view with security_barrier would be given.) The expected behavior is to keep sub-query without flatten. However, the externally provided qualifiers are correctly pushed down. Do we need to focus on the code around above functions rather than distribute_qual_to_rels, to prevent undesirable pushing-down across security barrier? postgres=# CREATE VIEW v1 AS SELECT * FROM t1 WHERE a 100; CREATE VIEW postgres=# CREATE VIEW v2 AS SELECT * FROM t2 JOIN t3 ON x = s; CREATE VIEW postgres=# EXPLAIN SELECT * FROM v1 WHERE b = 'bbb'; QUERY PLAN Seq Scan on t1 (cost=0.00..28.45 rows=2 width=36) Filter: ((a 100) AND (b = 'bbb'::text)) (2 rows) postgres=# EXPLAIN SELECT * FROM v2 WHERE t = 'ttt'; QUERY PLAN Hash Join (cost=25.45..52.73 rows=37 width=72) Hash Cond: (t2.x = t3.s) - Seq Scan on t2 (cost=0.00..22.30 rows=1230 width=36) - Hash (cost=25.38..25.38 rows=6 width=36) - Seq Scan on t3 (cost=0.00..25.38 rows=6 width=36) Filter: (t = 'ttt'::text) (6 rows) Well, there's clearly some way to prevent pushdown from happening, because sticking a LIMIT in there does the trick... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
Robert Haas robertmh...@gmail.com writes: Well, there's clearly some way to prevent pushdown from happening, because sticking a LIMIT in there does the trick... I already pointed you at subquery_is_pushdown_safe ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Sun, Oct 16, 2011 at 4:46 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: Hi Robert, I'm a bit confusing about this sentence. If you can make this work, I think it could be a pretty sweet plannner optimization even apart from the implications for security views. Consider a query of this form: A LEFT JOIN B LEFT JOIN C where B is a view defined as: B1 JOIN B2 JOIN B3 LEFT JOIN B4 LEFT JOIN B5 Now let's suppose that from_collapse_limit/join_collapse_limit are set low enough that we decline to fold these subproblems together. If there happens to be a qual B.x = 1, where B.x is really B1.x, then the generated plan sucks, because it will basically lose the ability to filter B1 early, very possibly on, say, a unique index. Or at least a highly selective index. I tried to reproduce the scenario with enough small from/join_collapse_limit (typically 1), but it allows to push down qualifiers into the least scan plan. Hmm, you're right. LIMIT 10 prevents qual pushdown, but hitting from_collapse_limit/join_collapse_limit apparently doesn't. I could have sworn I've seen this work the other way, but I guess not. E.g) mytest=# SET from_collapse_limit = 1; mytest=# SET join_collapse_limit = 1; mytest=# CREATE VIEW B AS SELECT B1.* FROM B1,B2,B3 WHERE B1.x = B2.x AND B2.x = B3.x; mytest=# EXPLAIN SELECT * FROM A,B,C WHERE A.x=B.x AND B.x=C.x AND f_leak(B.y); This I wouldn't expect to have any effect anyway, because you're using the ad-hoc join syntax rather than explicit join syntax. But I tried it with explicit join syntax and it seems to only constrain the join order, not prevent qual pushdown. I agree with the following approach to tackle this problem in 100%. However, I'm unclear how from/join_collapse_limit affects to keep sub-queries unflatten. It seems to me it is determined based on the result of is_simple_subquery(). I think you are right, but I'm not sure it's right to hack is_simple_subquery() directly. Perhaps what we want to do is modify pull_up_subquery()? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
Robert Haas robertmh...@gmail.com writes: On Sun, Oct 16, 2011 at 4:46 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: I tried to reproduce the scenario with enough small from/join_collapse_limit (typically 1), but it allows to push down qualifiers into the least scan plan. Hmm, you're right. LIMIT 10 prevents qual pushdown, but hitting from_collapse_limit/join_collapse_limit apparently doesn't. I could have sworn I've seen this work the other way, but I guess not. No, the collapse_limit variables are entirely unrelated to subquery flattening, or to qual pushdown for that matter. They only restrict the number of join paths we consider. And we will attempt to push down quals into an unflattened subquery, too, if it looks safe. See subquery_is_pushdown_safe, qual_is_pushdown_safe, etc in allpaths.c. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
Hi Robert, I'm a bit confusing about this sentence. If you can make this work, I think it could be a pretty sweet plannner optimization even apart from the implications for security views. Consider a query of this form: A LEFT JOIN B LEFT JOIN C where B is a view defined as: B1 JOIN B2 JOIN B3 LEFT JOIN B4 LEFT JOIN B5 Now let's suppose that from_collapse_limit/join_collapse_limit are set low enough that we decline to fold these subproblems together. If there happens to be a qual B.x = 1, where B.x is really B1.x, then the generated plan sucks, because it will basically lose the ability to filter B1 early, very possibly on, say, a unique index. Or at least a highly selective index. I tried to reproduce the scenario with enough small from/join_collapse_limit (typically 1), but it allows to push down qualifiers into the least scan plan. E.g) mytest=# SET from_collapse_limit = 1; mytest=# SET join_collapse_limit = 1; mytest=# CREATE VIEW B AS SELECT B1.* FROM B1,B2,B3 WHERE B1.x = B2.x AND B2.x = B3.x; mytest=# EXPLAIN SELECT * FROM A,B,C WHERE A.x=B.x AND B.x=C.x AND f_leak(B.y); QUERY PLAN Merge Join (cost=381.80..9597.97 rows=586624 width=108) Merge Cond: (a.x = b1.x) - Merge Join (cost=170.85..290.46 rows=7564 width=72) Merge Cond: (a.x = c.x) - Sort (cost=85.43..88.50 rows=1230 width=36) Sort Key: a.x - Seq Scan on a (cost=0.00..22.30 rows=1230 width=36) - Sort (cost=85.43..88.50 rows=1230 width=36) Sort Key: c.x - Seq Scan on c (cost=0.00..22.30 rows=1230 width=36) - Materialize (cost=210.95..528.56 rows=15510 width=44) - Merge Join (cost=210.95..489.78 rows=15510 width=44) Merge Cond: (b1.x = b3.x) - Merge Join (cost=125.52..165.40 rows=2522 width=40) Merge Cond: (b1.x = b2.x) - Sort (cost=40.09..41.12 rows=410 width=36) Sort Key: b1.x - Seq Scan on b1 (cost=0.00..22.30 rows=410 width=36) Filter: f_leak(y) - Sort (cost=85.43..88.50 rows=1230 width=4) Sort Key: b2.x - Seq Scan on b2 (cost=0.00..22.30 rows=1230 width=4) - Sort (cost=85.43..88.50 rows=1230 width=4) Sort Key: b3.x - Seq Scan on b3 (cost=0.00..22.30 rows=1230 width=4) (25 rows) In this example, f_leak() takes an argument come from B1 table within B view, and it was correctly distributed to SeqScan on B1. From perspective of the code, the *_collapse_limit affects the contents of joinlist being returned from deconstruct_jointree() whether its sub-portion is flatten, or not. However, the qualifiers are distributed on distribute_restrictinfo_to_rels() to RelOptInfo based on its dependency of relations being referenced by arguments. Thus, the above f_leak() was distributed to B1, not B, because its arguments come from only B1. I agree with the following approach to tackle this problem in 100%. However, I'm unclear how from/join_collapse_limit affects to keep sub-queries unflatten. It seems to me it is determined based on the result of is_simple_subquery(). 1. Let quals percolate down into subqueries. 2. Add the notion of a security view, which prevents flattening and disables the optimization of patch #1 3. Add the notion of a leakproof function, which can benefit from the optimization of #1 even when the view involved is a security view as introduced in #2 Thanks, 2011/10/11 Robert Haas robertmh...@gmail.com: On Mon, Oct 10, 2011 at 4:28 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote: I agreed. We have been on the standpoint that tries to prevent leakable functions to reference a portion of join-tree being already flatten, however, it has been a tough work. It seems to me it is much simple approach that enables to push down only non-leaky functions into inside of sub-queries. An idea is to add a hack on distribute_qual_to_rels() to relocate a qualifier into inside of the sub-query, when it references only a particular sub-query being come from a security view, and when the sub-query satisfies is_simple_subquery(), for example. If you can make this work, I think it could be a pretty sweet plannner optimization even apart from the implications for security views. Consider a query of this form: A LEFT JOIN B LEFT JOIN C where B is a view defined as: B1 JOIN B2 JOIN B3 LEFT JOIN B4 LEFT JOIN B5 Now let's suppose that from_collapse_limit/join_collapse_limit are set low enough that we decline to fold these subproblems together. If there happens to be a qual B.x = 1, where B.x is really B1.x, then the generated plan sucks, because it will basically lose
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Sun, Oct 9, 2011 at 11:50 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
I tried to refactor the patches based on the interface of WITH (...)
and usage of
pg_class.reloptions, although here is no functionality changes; including the
behavior when a view is replaced.
My preference is WITH (...) interface, however, it is not a strong one.
So, I hope either of versions being reviewed.
I spent some more time looking at this, and I guess I'm pretty unsold
on the whole approach. In the part 2 patch, for example, we're doing
this:
+static bool
+mark_qualifiers_depth_walker(Node *node, void *context)
+{
+ int depth = *((int *)(context));
+
+ if (node == NULL)
+ return false;
+
+ if (IsA(node, FuncExpr))
+ {
+ ((FuncExpr *)node)-depth = depth;
+ }
+ else if (IsA(node, OpExpr))
+ {
+ ((OpExpr *)node)-depth = depth;
+ }
+ else if (IsA(node, DistinctExpr))
+ {
+ ((DistinctExpr *)node)-depth = depth;
+ }
+ else if (IsA(node, ScalarArrayOpExpr))
+ {
+ ((ScalarArrayOpExpr *)node)-depth = depth;
+ }
+ else if (IsA(node, CoerceViaIO))
+ {
+ ((CoerceViaIO *)node)-depth = depth;
+ }
+ else if (IsA(node, ArrayCoerceExpr))
+ {
+ ((ArrayCoerceExpr *)node)-depth = depth;
+ }
+ else if (IsA(node, NullIfExpr))
+ {
+ ((NullIfExpr *)node)-depth = depth;
+ }
+ else if (IsA(node, RowCompareExpr))
+ {
+ ((RowCompareExpr *)node)-depth = depth;
+ }
+ return expression_tree_walker(node,
mark_qualifiers_depth_walker, context);
+}
It seems really ugly to me to suppose that we need to add a depth
field to every single one of these node types. If you've missed one,
then we have a security hole. If someone else adds another node type
later that requires this field and doesn't add it, we have a security
hole. And since all of these depth fields are going to make their way
into stored rules, those security holes will require an initdb to fix.
Ouch! And what happens if the security view becomes a non-security
view or visca versa? Now all of those stored depth fields are out of
date. Maybe you can argue that we can just patch that up when we
reload them, but that seems to me to miss the point. If the data in a
stored rule can get out of date, then it shouldn't be stored there in
the first place.
Tom may have a better feeling on this than I do, but my gut feeling
here is that this whole approach is letting the cat out of the bag and
then trying to stuff it back in. I don't think that's going to be
very reliable, and more than that, I don't like our chances of having
confidence in its reliability. I feel like the heart of what we're
doing here ought to be preventing the subquery from getting flattened.
For example:
rhaas=# create table secret (a int, b text);
CREATE TABLE
rhaas=# insert into secret select g, random()::text||random()::text
from generate_series(1,1) g;
INSERT 0 1
rhaas=# create view window_on_secret as select * from secret where a = 1;
CREATE VIEW
rhaas=# create table leak (a int, b text);
CREATE TABLE
rhaas=# create or replace function snarf(a int, b text) returns
boolean as $$begin insert into leak values ($1, $2); return true;
end$$ language plpgsql cost
0.1;
CREATE FUNCTION
rhaas=# explain analyze select * from window_on_secret;
QUERY PLAN
---
Seq Scan on secret (cost=0.00..209.00 rows=1 width=39) (actual
time=0.022..2.758 rows=1 loops=1)
Filter: (a = 1)
Rows Removed by Filter:
Total runtime: 2.847 ms
(4 rows)
rhaas=# select * from leak;
a | b
---+---
(0 rows)
rhaas=# explain analyze select * from window_on_secret where snarf(a, b);
QUERY PLAN
-
Seq Scan on secret (cost=0.00..209.00 rows=1 width=39) (actual
time=0.671..126.521 rows=1 loops=1)
Filter: (snarf(a, b) AND (a = 1))
Rows Removed by Filter:
Total runtime: 126.565 ms
(4 rows)
Woops! I've stolen the whole table. But look what happens when I
change the definition of window_on_secret so that it can't be
flattened:
rhaas=# truncate leak;
TRUNCATE TABLE
rhaas=# create or replace view window_on_secret as select * from
secret where a = 1 limit 10;
CREATE VIEW
rhaas=# explain analyze select * from window_on_secret where snarf(a, b);
QUERY PLAN
--
Subquery Scan on window_on_secret (cost=0.00..209.01 rows=1
width=39) (actual
Re: [HACKERS] [v9.2] Fix Leaky View Problem
2011/10/10 Robert Haas robertmh...@gmail.com:
On Sun, Oct 9, 2011 at 11:50 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
I tried to refactor the patches based on the interface of WITH (...)
and usage of
pg_class.reloptions, although here is no functionality changes; including the
behavior when a view is replaced.
My preference is WITH (...) interface, however, it is not a strong one.
So, I hope either of versions being reviewed.
I spent some more time looking at this, and I guess I'm pretty unsold
on the whole approach. In the part 2 patch, for example, we're doing
this:
+static bool
+mark_qualifiers_depth_walker(Node *node, void *context)
+{
+ int depth = *((int *)(context));
+
... snip ...
+ else if (IsA(node, RowCompareExpr))
+ {
+ ((RowCompareExpr *)node)-depth = depth;
+ }
+ return expression_tree_walker(node,
mark_qualifiers_depth_walker, context);
+}
It seems really ugly to me to suppose that we need to add a depth
field to every single one of these node types. If you've missed one,
then we have a security hole. If someone else adds another node type
later that requires this field and doesn't add it, we have a security
hole. And since all of these depth fields are going to make their way
into stored rules, those security holes will require an initdb to fix.
Indeed, I have to admit this disadvantage from the perspective of code
maintenance, because it had also been a tough work for me to track
the depth field in this patch.
If we make security views work like this, then we don't need to have
one mechanism to sort quals by depth and another to prevent them from
being pushed down through joins. It all just works. Now, there is
one problem: if snarf() were a non-leaky function rather than a
maliciously crafted one, it still wouldn't get pushed down:
Rather than my original design, I'm learning to the idea to keep
sub-queries come from security views; without flatten, because
of its straightforwardness.
If we make security views work like this, then we don't need to have
one mechanism to sort quals by depth and another to prevent them from
being pushed down through joins. It all just works. Now, there is
one problem: if snarf() were a non-leaky function rather than a
maliciously crafted one, it still wouldn't get pushed down:
I agreed. We have been on the standpoint that tries to prevent
leakable functions to reference a portion of join-tree being already
flatten, however, it has been a tough work.
It seems to me it is much simple approach that enables to push
down only non-leaky functions into inside of sub-queries.
An idea is to add a hack on distribute_qual_to_rels() to relocate
a qualifier into inside of the sub-query, when it references only
a particular sub-query being come from a security view, and
when the sub-query satisfies is_simple_subquery(), for example.
Anyway, I'll try to tackle this long standing problem with this
approach in the next commit-fest.
Thanks,
--
KaiGai Kohei kai...@kaigai.gr.jp
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Sun, Oct 09, 2011 at 05:50:52PM +0200, Kohei KaiGai wrote: [patch v4] Each revision of this patch yielded a 1.2 MiB email. Please gzip attachments this large. The two revisions you sent in September constituted 18% of the pgsql-hackers bits for the month, and the next-largest message was only 315 KiB. Your mailer also picks base64 for textual attachments, needlessly inflating them by 37%. At the same time, the patch is large because it rewrites every line in pg_proc.h. Especially since it leaves proleakproof = 'f' for _all_ rows, consider instead using an approach like this: http://archives.postgresql.org/message-id/20110611211304.gb21...@tornado.leadboat.com These patches were not context diffs. Thanks, nm -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On 10 October 2011 21:28, Kohei KaiGai kai...@kaigai.gr.jp wrote: 2011/10/10 Robert Haas robertmh...@gmail.com: It seems really ugly to me to suppose that we need to add a depth field to every single one of these node types. If you've missed one, then we have a security hole. If someone else adds another node type later that requires this field and doesn't add it, we have a security hole. And since all of these depth fields are going to make their way into stored rules, those security holes will require an initdb to fix. Indeed, I have to admit this disadvantage from the perspective of code maintenance, because it had also been a tough work for me to track the depth field in this patch. Would you consider putting the depth field directly into a generic superclass node, such as the Expr node? Perhaps that approach would be neater. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Mon, Oct 10, 2011 at 4:28 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote: I agreed. We have been on the standpoint that tries to prevent leakable functions to reference a portion of join-tree being already flatten, however, it has been a tough work. It seems to me it is much simple approach that enables to push down only non-leaky functions into inside of sub-queries. An idea is to add a hack on distribute_qual_to_rels() to relocate a qualifier into inside of the sub-query, when it references only a particular sub-query being come from a security view, and when the sub-query satisfies is_simple_subquery(), for example. If you can make this work, I think it could be a pretty sweet plannner optimization even apart from the implications for security views. Consider a query of this form: A LEFT JOIN B LEFT JOIN C where B is a view defined as: B1 JOIN B2 JOIN B3 LEFT JOIN B4 LEFT JOIN B5 Now let's suppose that from_collapse_limit/join_collapse_limit are set low enough that we decline to fold these subproblems together. If there happens to be a qual B.x = 1, where B.x is really B1.x, then the generated plan sucks, because it will basically lose the ability to filter B1 early, very possibly on, say, a unique index. Or at least a highly selective index. If we could allow the B.x qual to trickle down inside of the subquery, we'd get a much better plan. Of course, it's still not as good as flattening, because it won't allow us to consider as many possible join orders - but the whole point of having from_collapse_limit/join_collapse_limit in the first place is that we can't consider all the join orders without having planning time and memory usage balloon wildly out of control. And in many real-world cases, I think that this would probably mitigate the effects of exceeding from_collapse_limit/join_collapse_limit quite a bit. In order to make it work, though, you'd need to arrange things so that we distribute quals to rels in the parent query, then let some of them filter down into the subquery, then distribute quals to rels in the subquery (possibly adjusting RTE indexes?), then finish planning the subquery, then finish planning the parent query. Not sure how possible/straightforward that is. It's probably a good idea to deal with this part first, because if you can't make it work then the whole approach is in trouble. I'm almost imagining that we could break this into three independent patches, like this: 1. Let quals percolate down into subqueries. 2. Add the notion of a security view, which prevents flattening and disables the optimization of patch #1 3. Add the notion of a leakproof function, which can benefit from the optimization of #1 even when the view involved is a security view as introduced in #2 Unlike the way you have it now, I think those patches could be independently committable. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
2011/10/8 Noah Misch n...@leadboat.com: On Sun, Oct 02, 2011 at 07:16:33PM +0200, Kohei KaiGai wrote: My preference is still also WITH(security_barrier=...) syntax. The arguable point was the behavior when a view is replaced without explicit WITH clause; whether we should consider it was specified a default value, or we should consider it means the option is preserved. If we stand on the viewpoint that object's attribute related to security (such as ownership, acl, label, ...) should be preserved, the security barrier also shall be preserved. On the other hand, we can never know what options will be added in the future, right now. Thus, we may need to sort out options related to security and not at DefineVirtualRelation(). However, do we need to limit type of the options to be preserved to security related? It is the first case that object with arbitrary options can be replaced. It seems to me we have no matter, even if we determine object's options are preserved unless an explicit new value is provided. Currently, you can predict how CREATE OR REPLACE affects a given object characteristic with a simple rule: if the CREATE OR REPLACE statement can specify a characteristic, we don't preserve its existing value. Otherwise, we do preserve it. Let's not depart from that rule. Applying that rule to the proposed syntax, it shall not preserve the existing security_barrier value. I think that is acceptable. If it's not acceptable, we need a different syntax -- perhaps CREATE SECURITY VIEW. No. It also preserves the security-barrier flag, when we replace a view without SECURITY option. The only difference is that we have no way to turn off security-barrier flag explicitly, right now. The major reason why I prefer reloptions rather than SECURITY option is that allows to reuse the existing capability to store a property of relation. It seems to me both of syntax enables to achieve the rule to preserve flags when a view is replaced. Any other ideas? Suppose we permitted pushdown of unsafe predicates when the user can read the involved columns anyway, a generalization of the idea from the first paragraph of [1]. Would that, along with LEAKPROOF, provide enough strategies for shoring up performance to justify removing unsafe views entirely? The problem was that we do all the access control decision at the executor stage, but planner has to make a plan prior to execution. So, it was also reason why we have tried to add LEAKPROOF flag to functions. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Sat, Oct 08, 2011 at 09:11:08AM +0200, Kohei KaiGai wrote: 2011/10/8 Noah Misch n...@leadboat.com: On Sun, Oct 02, 2011 at 07:16:33PM +0200, Kohei KaiGai wrote: My preference is still also WITH(security_barrier=...) syntax. The arguable point was the behavior when a view is replaced without explicit WITH clause; whether we should consider it was specified a default value, or we should consider it means the option is preserved. If we stand on the viewpoint that object's attribute related to security (such as ownership, acl, label, ...) should be preserved, the security barrier also shall be preserved. On the other hand, we can never know what options will be added in the future, right now. Thus, we may need to sort out options related to security and not at DefineVirtualRelation(). However, do we need to limit type of the options to be preserved to security related? It is the first case that object with arbitrary options can be replaced. It seems to me we have no matter, even if we determine object's options are preserved unless an explicit new value is provided. Currently, you can predict how CREATE OR REPLACE affects a given object characteristic with a simple rule: if the CREATE OR REPLACE statement can specify a characteristic, we don't preserve its existing value. ?Otherwise, we do preserve it. ?Let's not depart from that rule. Applying that rule to the proposed syntax, it shall not preserve the existing security_barrier value. ?I think that is acceptable. ?If it's not acceptable, we need a different syntax -- perhaps CREATE SECURITY VIEW. No. It also preserves the security-barrier flag, when we replace a view without SECURITY option. The only difference is that we have no way to turn off security-barrier flag explicitly, right now. The major reason why I prefer reloptions rather than SECURITY option is that allows to reuse the existing capability to store a property of relation. It seems to me both of syntax enables to achieve the rule to preserve flags when a view is replaced. Yes, there are no technical barriers to implementing either behavior with either syntax. However, CREATE OR REPLACE VIEW ... WITH (...) has a precedent guiding its behavior: if a CREATE OR REPLACE statement can specify a characteristic, we don't preserve its existing value. Any other ideas? Suppose we permitted pushdown of unsafe predicates when the user can read the involved columns anyway, a generalization of the idea from the first paragraph of [1]. ?Would that, along with LEAKPROOF, provide enough strategies for shoring up performance to justify removing unsafe views entirely? The problem was that we do all the access control decision at the executor stage, but planner has to make a plan prior to execution. So, it was also reason why we have tried to add LEAKPROOF flag to functions. Yes; we'd need to invalidate relevant plans in response to anything that changes access control decisions. GRANT and ALTER ... OWNER TO already do that, but we'd need to cover pg_authid/pg_auth_members changes, SET ROLE, SET SESSION AUTHORIZATION, and probably a few other things. That might be a substantial project in its own right. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Sun, Oct 02, 2011 at 07:16:33PM +0200, Kohei KaiGai wrote: My preference is still also WITH(security_barrier=...) syntax. The arguable point was the behavior when a view is replaced without explicit WITH clause; whether we should consider it was specified a default value, or we should consider it means the option is preserved. If we stand on the viewpoint that object's attribute related to security (such as ownership, acl, label, ...) should be preserved, the security barrier also shall be preserved. On the other hand, we can never know what options will be added in the future, right now. Thus, we may need to sort out options related to security and not at DefineVirtualRelation(). However, do we need to limit type of the options to be preserved to security related? It is the first case that object with arbitrary options can be replaced. It seems to me we have no matter, even if we determine object's options are preserved unless an explicit new value is provided. Currently, you can predict how CREATE OR REPLACE affects a given object characteristic with a simple rule: if the CREATE OR REPLACE statement can specify a characteristic, we don't preserve its existing value. Otherwise, we do preserve it. Let's not depart from that rule. Applying that rule to the proposed syntax, it shall not preserve the existing security_barrier value. I think that is acceptable. If it's not acceptable, we need a different syntax -- perhaps CREATE SECURITY VIEW. Any other ideas? Suppose we permitted pushdown of unsafe predicates when the user can read the involved columns anyway, a generalization of the idea from the first paragraph of [1]. Would that, along with LEAKPROOF, provide enough strategies for shoring up performance to justify removing unsafe views entirely? nm [1] http://archives.postgresql.org/message-id/aanlktil1n2qwdd7izlgbvt2ifl29rwfvkssel9b9r...@mail.gmail.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
2011/9/30 Noah Misch n...@leadboat.com: On Sun, Sep 25, 2011 at 11:22:56PM -0400, Robert Haas wrote: On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch n...@leadboat.com wrote: On Sun, Sep 25, 2011 at 11:22:03AM -0500, Kevin Grittner wrote: Robert Haas ?09/25/11 10:58 AM I'm not sure we've been 100% consistent about that, since we previously made CREATE OR REPLACE LANGUAGE not replace the owner with the current user. I think we've been consistent in *not* changing security on an object when it is replaced. [CREATE OR REPLACE FUNCTION does not change proowner or proacl] Good point. ?C-O-R VIEW also preserves column default values. ?I believe we are consistent to the extent that everything possible to specify in each C-O-R statement gets replaced outright. ?The preserved characteristics *require* commands like GRANT, COMMENT and ALTER VIEW to set in the first place. The analogue I had in mind is SECURITY DEFINER, which C-O-R FUNCTION reverts to SECURITY INVOKER if it's not specified each time. ?That default is safe, though, while the proposed default of security_barrier=false is unsafe. Even though I normally take the opposite position, I still like the idea of dedicated syntax for this feature. Not knowing what view options we might end up with in the future, I hate having to decide on what the general behavior ought to be. But it would be easy to decide that CREATE SECURITY VIEW followed by CREATE OR REPLACE VIEW leaves the security flag set; it would be consistent with what we're doing with owner and acl information and wouldn't back us into any unpleasant decisions down the road. I prefer the previous UI (WITH (security_barrier=...)) to this proposal, albeit for diffuse reasons. Both kinds of views can have the consequence of granting new access to data. One kind leaks tuples to untrustworthy code whenever it's convenient for performance, and the other does not. A non-security view would not mimic either of these objects; it would be a mere subquery macro. Using WITH (...) syntax attached to the CREATE VIEW command better evokes the similarity between the alternatives we're actually offering. I also find it mildly odd letting CREATE OR REPLACE VIEW update an object originating with CREATE SECURITY VIEW. My preference is still also WITH(security_barrier=...) syntax. The arguable point was the behavior when a view is replaced without explicit WITH clause; whether we should consider it was specified a default value, or we should consider it means the option is preserved. If we stand on the viewpoint that object's attribute related to security (such as ownership, acl, label, ...) should be preserved, the security barrier also shall be preserved. On the other hand, we can never know what options will be added in the future, right now. Thus, we may need to sort out options related to security and not at DefineVirtualRelation(). However, do we need to limit type of the options to be preserved to security related? It is the first case that object with arbitrary options can be replaced. It seems to me we have no matter, even if we determine object's options are preserved unless an explicit new value is provided. Any other ideas? Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Sun, Sep 25, 2011 at 11:22:56PM -0400, Robert Haas wrote: On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch n...@leadboat.com wrote: On Sun, Sep 25, 2011 at 11:22:03AM -0500, Kevin Grittner wrote: Robert Haas ?09/25/11 10:58 AM I'm not sure we've been 100% consistent about that, since we previously made CREATE OR REPLACE LANGUAGE not replace the owner with the current user. I think we've been consistent in *not* changing security on an object when it is replaced. [CREATE OR REPLACE FUNCTION does not change proowner or proacl] Good point. ?C-O-R VIEW also preserves column default values. ?I believe we are consistent to the extent that everything possible to specify in each C-O-R statement gets replaced outright. ?The preserved characteristics *require* commands like GRANT, COMMENT and ALTER VIEW to set in the first place. The analogue I had in mind is SECURITY DEFINER, which C-O-R FUNCTION reverts to SECURITY INVOKER if it's not specified each time. ?That default is safe, though, while the proposed default of security_barrier=false is unsafe. Even though I normally take the opposite position, I still like the idea of dedicated syntax for this feature. Not knowing what view options we might end up with in the future, I hate having to decide on what the general behavior ought to be. But it would be easy to decide that CREATE SECURITY VIEW followed by CREATE OR REPLACE VIEW leaves the security flag set; it would be consistent with what we're doing with owner and acl information and wouldn't back us into any unpleasant decisions down the road. I prefer the previous UI (WITH (security_barrier=...)) to this proposal, albeit for diffuse reasons. Both kinds of views can have the consequence of granting new access to data. One kind leaks tuples to untrustworthy code whenever it's convenient for performance, and the other does not. A non-security view would not mimic either of these objects; it would be a mere subquery macro. Using WITH (...) syntax attached to the CREATE VIEW command better evokes the similarity between the alternatives we're actually offering. I also find it mildly odd letting CREATE OR REPLACE VIEW update an object originating with CREATE SECURITY VIEW. Unqualified CREATE VIEW will retain no redeeming value apart from backward compatibility; new applications with any concern for database-level security should use only security_barrier=true and mark functions LEAKPROOF as needed. nm -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
2011/9/26 Robert Haas robertmh...@gmail.com: On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote: The Part-2 tries to tackles a leaky-view scenarios by functions with very tiny cost estimation value. It was same one we had discussed in the commitfest-1st. It prevents to launch functions earlier than ones come from inside of views with security_barrier option. The Part-3 tries to tackles a leaky-view scenarios by functions that references one side of join loop. It prevents to distribute qualifiers including functions without leakproof attribute into relations across security-barrier. I took a little more of a look at this today. It has major problems. First, I get compiler warnings (which you might want to trap in the future by creating src/Makefile.custom with COPT=-Werror when compiling). Second, the regression tests fail on the select_views test. Third, it appears that the part2 patch works by adding an additional traversal of the entire query tree to standard_planner(). I don't think we want to add overhead to the common case where no security views are in use, or at least it had better be very small - so this doesn't seem acceptable to me. The reason why I put a walker routine on the head of standard_planner() was that previous revision of this patch tracked strict depth of sub-queries, not a number of times to go through security barrier. The policy to count-up depth of qualifier was changed according to Noad's suggestion is commit-fest 1st, however, the suitable position to mark the depth value was kept. I'll try to revise the suitable position to track the depth value. It seems to me one candidate is pull_up_subqueries during its recursive call, because this patch originally set FromExpr-security_barrier here. In addition to the two points you mentioned above, I'll update this patch as follows: * Use CREATE [SECURITY] VIEW statement, instead of reloptions. the flag shall be stored within a new attribute of pg_class, and it shall be kept when an existing view getting replaced. * Utilize RangeTblEntry-relid, instead of rte-security_barrier, and the flag shall be pulled from the catalog on planner stage. * Documentation and Regression test. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
2011/9/26 Robert Haas robertmh...@gmail.com: On Sun, Sep 25, 2011 at 3:25 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote: I'm a bit nervous about storing security_barrier in the RTE. What happens to stored rules if the security_barrier option gets change later? The rte-security_barrier is evaluated when a query referencing security views get expanded. So, rte-security_barrier is not stored to catalog. I think it is. If you create a view that involves an RTE, the node tree is going to get stored in pg_rewrite.ev_action. And it's going to include the security_barrier attribute, because you added outfuncs support for it... No? IIUC, nested views are also expanded when user's query gets rewritten. Thus, rte-security_barrier shall be set based on the latest configuration of the view. I injected an elog(NOTICE, ...) to confirm the behavior, when security_barrier flag was set on rte-security_barrier at ApplyRetrieveRule(). postgres=# CREATE VIEW v1 WITH (security_barrier=true) AS SELECT * FROM t1 WHERE a % 2 = 0; CREATE VIEW postgres=# CREATE VIEW v2 WITH (security_barrier=true) AS SELECT a + a AS aa, b FROM v1; CREATE VIEW postgres=# SELECT * FROM v2; NOTICE: security barrier set on v1 NOTICE: security barrier set on v2 aa | b +- 4 | bbb (1 row) postgres=# ALTER TABLE v1 SET (security_barrier=false); ALTER TABLE postgres=# SELECT * FROM v2; NOTICE: security barrier set on v2 aa | b +- 4 | bbb (1 row) postgres=# ALTER TABLE v1 SET (security_barrier=true); ALTER TABLE postgres=# SELECT * FROM v2; NOTICE: security barrier set on v1 NOTICE: security barrier set on v2 aa | b +- 4 | bbb (1 row) It seems to me the rte-security_barrier flag is correctly set, even if underlying view's option was changed later. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
2011/9/26 Robert Haas robertmh...@gmail.com: On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch n...@leadboat.com wrote: On Sun, Sep 25, 2011 at 11:22:03AM -0500, Kevin Grittner wrote: Robert Haas 09/25/11 10:58 AM I'm not sure we've been 100% consistent about that, since we previously made CREATE OR REPLACE LANGUAGE not replace the owner with the current user. I think we've been consistent in *not* changing security on an object when it is replaced. [CREATE OR REPLACE FUNCTION does not change proowner or proacl] Good point. C-O-R VIEW also preserves column default values. I believe we are consistent to the extent that everything possible to specify in each C-O-R statement gets replaced outright. The preserved characteristics *require* commands like GRANT, COMMENT and ALTER VIEW to set in the first place. The analogue I had in mind is SECURITY DEFINER, which C-O-R FUNCTION reverts to SECURITY INVOKER if it's not specified each time. That default is safe, though, while the proposed default of security_barrier=false is unsafe. Even though I normally take the opposite position, I still like the idea of dedicated syntax for this feature. Not knowing what view options we might end up with in the future, I hate having to decide on what the general behavior ought to be. But it would be easy to decide that CREATE SECURITY VIEW followed by CREATE OR REPLACE VIEW leaves the security flag set; it would be consistent with what we're doing with owner and acl information and wouldn't back us into any unpleasant decisions down the road. Does the CREATE SECURITY VIEW statement mean a synonym of CREATE VIEW ... WITH (security_barrier=true) ? If so, it seems to me reasonable to keep the configuration when user provides no explicit option. 1) an explicit WITH(security_barrier=true) / CREATE SECURITY VIEW - It always turns on a security_barrier option. 2) an explicit WITH(security_barrier=false) - It always turns off security_barrier option. 3) no explicit option / CREATE VIEW - Keep existing configuration, although inconsist with SECURITY DEFINER Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Mon, Sep 26, 2011 at 6:28 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: 2011/9/26 Robert Haas robertmh...@gmail.com: On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch n...@leadboat.com wrote: On Sun, Sep 25, 2011 at 11:22:03AM -0500, Kevin Grittner wrote: Robert Haas 09/25/11 10:58 AM I'm not sure we've been 100% consistent about that, since we previously made CREATE OR REPLACE LANGUAGE not replace the owner with the current user. I think we've been consistent in *not* changing security on an object when it is replaced. [CREATE OR REPLACE FUNCTION does not change proowner or proacl] Good point. C-O-R VIEW also preserves column default values. I believe we are consistent to the extent that everything possible to specify in each C-O-R statement gets replaced outright. The preserved characteristics *require* commands like GRANT, COMMENT and ALTER VIEW to set in the first place. The analogue I had in mind is SECURITY DEFINER, which C-O-R FUNCTION reverts to SECURITY INVOKER if it's not specified each time. That default is safe, though, while the proposed default of security_barrier=false is unsafe. Even though I normally take the opposite position, I still like the idea of dedicated syntax for this feature. Not knowing what view options we might end up with in the future, I hate having to decide on what the general behavior ought to be. But it would be easy to decide that CREATE SECURITY VIEW followed by CREATE OR REPLACE VIEW leaves the security flag set; it would be consistent with what we're doing with owner and acl information and wouldn't back us into any unpleasant decisions down the road. Does the CREATE SECURITY VIEW statement mean a synonym of CREATE VIEW ... WITH (security_barrier=true) ? If so, it seems to me reasonable to keep the configuration when user provides no explicit option. 1) an explicit WITH(security_barrier=true) / CREATE SECURITY VIEW - It always turns on a security_barrier option. 2) an explicit WITH(security_barrier=false) - It always turns off security_barrier option. 3) no explicit option / CREATE VIEW - Keep existing configuration, although inconsist with SECURITY DEFINER No, you're missing my point completely. If we use a flexible options syntax here, then we have to decide on what behavior CREATE OR REPLACE should have for all future options, without knowing what they are yet, or what behavior will be appropriate. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
2011/9/26 Robert Haas robertmh...@gmail.com: On Mon, Sep 26, 2011 at 6:28 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: 2011/9/26 Robert Haas robertmh...@gmail.com: On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch n...@leadboat.com wrote: On Sun, Sep 25, 2011 at 11:22:03AM -0500, Kevin Grittner wrote: Robert Haas 09/25/11 10:58 AM I'm not sure we've been 100% consistent about that, since we previously made CREATE OR REPLACE LANGUAGE not replace the owner with the current user. I think we've been consistent in *not* changing security on an object when it is replaced. [CREATE OR REPLACE FUNCTION does not change proowner or proacl] Good point. C-O-R VIEW also preserves column default values. I believe we are consistent to the extent that everything possible to specify in each C-O-R statement gets replaced outright. The preserved characteristics *require* commands like GRANT, COMMENT and ALTER VIEW to set in the first place. The analogue I had in mind is SECURITY DEFINER, which C-O-R FUNCTION reverts to SECURITY INVOKER if it's not specified each time. That default is safe, though, while the proposed default of security_barrier=false is unsafe. Even though I normally take the opposite position, I still like the idea of dedicated syntax for this feature. Not knowing what view options we might end up with in the future, I hate having to decide on what the general behavior ought to be. But it would be easy to decide that CREATE SECURITY VIEW followed by CREATE OR REPLACE VIEW leaves the security flag set; it would be consistent with what we're doing with owner and acl information and wouldn't back us into any unpleasant decisions down the road. Does the CREATE SECURITY VIEW statement mean a synonym of CREATE VIEW ... WITH (security_barrier=true) ? If so, it seems to me reasonable to keep the configuration when user provides no explicit option. 1) an explicit WITH(security_barrier=true) / CREATE SECURITY VIEW - It always turns on a security_barrier option. 2) an explicit WITH(security_barrier=false) - It always turns off security_barrier option. 3) no explicit option / CREATE VIEW - Keep existing configuration, although inconsist with SECURITY DEFINER No, you're missing my point completely. If we use a flexible options syntax here, then we have to decide on what behavior CREATE OR REPLACE should have for all future options, without knowing what they are yet, or what behavior will be appropriate. Hmm. Indeed, it seems to me fair enough reason. In this syntax case, the only way to clear the security_barrier flag is to drop view once, then create a view, isn't it? And, is the security_barrier flag still stored within reloptions field? Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Mon, Sep 26, 2011 at 9:51 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: No, you're missing my point completely. If we use a flexible options syntax here, then we have to decide on what behavior CREATE OR REPLACE should have for all future options, without knowing what they are yet, or what behavior will be appropriate. Hmm. Indeed, it seems to me fair enough reason. In this syntax case, the only way to clear the security_barrier flag is to drop view once, then create a view, isn't it? I was imagining we'd have ALTER VIEW .. [NO] SECURITY or something like that. And, is the security_barrier flag still stored within reloptions field? No. That would be missing the point. But keep in mind no one else has endorsed my reasoning on this one as yet... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Mon, Sep 26, 2011 at 5:58 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: I think it is. If you create a view that involves an RTE, the node tree is going to get stored in pg_rewrite.ev_action. And it's going to include the security_barrier attribute, because you added outfuncs support for it... No? IIUC, nested views are also expanded when user's query gets rewritten. Thus, rte-security_barrier shall be set based on the latest configuration of the view. I injected an elog(NOTICE, ...) to confirm the behavior, when security_barrier flag was set on rte-security_barrier at ApplyRetrieveRule(). Hmm, OK. I am still not convinced that this is the right approach. Normally, we don't cache anything in the RangeTblEntry that might change between plan time and execution time. Those things are normally stored in the RelOptInfo - why not do the same here? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
2011/9/26 Robert Haas robertmh...@gmail.com: On Mon, Sep 26, 2011 at 5:58 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: I think it is. If you create a view that involves an RTE, the node tree is going to get stored in pg_rewrite.ev_action. And it's going to include the security_barrier attribute, because you added outfuncs support for it... No? IIUC, nested views are also expanded when user's query gets rewritten. Thus, rte-security_barrier shall be set based on the latest configuration of the view. I injected an elog(NOTICE, ...) to confirm the behavior, when security_barrier flag was set on rte-security_barrier at ApplyRetrieveRule(). Hmm, OK. I am still not convinced that this is the right approach. Normally, we don't cache anything in the RangeTblEntry that might change between plan time and execution time. Those things are normally stored in the RelOptInfo - why not do the same here? The point is that a sub-query come from a particular view does not keep the information what view originally stored the sub-query when it was passed to the executor stage. PostgreSQL handles a view as just a sub-query after the rewriter stage. One possible idea not to store the flag in RangeTblEntry is to utilize rte-relid to show the relation-id of the source view, when rtekind is RTE_SUBQUERY; that enables to pull the security_barrier flag in executor stage. However, the interface to reference reloptions are designed to pull this information with Relation pointer, rather than lsyscache, so I implemented this revision with a new rte-security_barrier member. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
Kohei KaiGai kai...@kaigai.gr.jp writes: However, the interface to reference reloptions are designed to pull this information with Relation pointer, rather than lsyscache, so I implemented this revision with a new rte-security_barrier member. This approach will guarantee that we can never implement an ALTER VIEW (or CREATE OR REPLACE VIEW) option that changes the state of the flag. I don't think that's a good idea. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
Kohei KaiGai kai...@kaigai.gr.jp writes: Sorry, are you saying the current (in other words, rte-security_barrier stores the state of reloption) approach is not a good idea? Yes. I think the same as Robert: the way to handle this is to store it in RelOptInfo for the duration of planning, and pull it from the catalogs during planner startup (cf plancat.c). regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
2011/9/26 Tom Lane t...@sss.pgh.pa.us: Kohei KaiGai kai...@kaigai.gr.jp writes: However, the interface to reference reloptions are designed to pull this information with Relation pointer, rather than lsyscache, so I implemented this revision with a new rte-security_barrier member. This approach will guarantee that we can never implement an ALTER VIEW (or CREATE OR REPLACE VIEW) option that changes the state of the flag. I don't think that's a good idea. Sorry, are you saying the current (in other words, rte-security_barrier stores the state of reloption) approach is not a good idea? Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
2011/9/26 Tom Lane t...@sss.pgh.pa.us: Kohei KaiGai kai...@kaigai.gr.jp writes: Sorry, are you saying the current (in other words, rte-security_barrier stores the state of reloption) approach is not a good idea? Yes. I think the same as Robert: the way to handle this is to store it in RelOptInfo for the duration of planning, and pull it from the catalogs during planner startup (cf plancat.c). Hmm. If so, it seems to me worthwhile to investigate an alternative approach that stores relation-id of the view on rte-relid if rtekind is RTE_SUBQUERY and pull the security_barrier flag from the catalog during planner stage. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote: The Part-2 tries to tackles a leaky-view scenarios by functions with very tiny cost estimation value. It was same one we had discussed in the commitfest-1st. It prevents to launch functions earlier than ones come from inside of views with security_barrier option. The Part-3 tries to tackles a leaky-view scenarios by functions that references one side of join loop. It prevents to distribute qualifiers including functions without leakproof attribute into relations across security-barrier. I took a little more of a look at this today. It has major problems. First, I get compiler warnings (which you might want to trap in the future by creating src/Makefile.custom with COPT=-Werror when compiling). Second, the regression tests fail on the select_views test. Third, it appears that the part2 patch works by adding an additional traversal of the entire query tree to standard_planner(). I don't think we want to add overhead to the common case where no security views are in use, or at least it had better be very small - so this doesn't seem acceptable to me. Here are some simple benchmarking with pgbench -S (scale factor 10, shared_buffers=400MB, MacBook Pro laptop) with and without this stack of patches. These aren't clear-cut enough to make me absolutely sure that this patch causes a noticeable performance regression, but I think it does, and I'm not at all sure that this is the worst case: results.kaigai.1:tps = 9359.908769 (including connections establishing) results.kaigai.1:tps = 9366.317857 (including connections establishing) results.kaigai.1:tps = 9413.593349 (including connections establishing) results.master.1:tps = 9444.494510 (including connections establishing) results.master.1:tps = 9400.486860 (including connections establishing) results.master.1:tps = 9472.220529 (including connections establishing) In the light of these problems, it doesn't seem worthwhile for me to spend any more time on this right now: it looks to me like this needs a lot more work before it can be considered for commit. I will mark it Waiting on Author for now, but I think Returned with Feedback might be more appropriate. This needs more than light cleanup; it needs much more rigorous testing, both as to correctness and performance, and at least a partial redesign. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
Kohei KaiGai kai...@kaigai.gr.jp writes: One possible idea not to store the flag in RangeTblEntry is to utilize rte-relid to show the relation-id of the source view, when rtekind is RTE_SUBQUERY; that enables to pull the security_barrier flag in executor stage. Maybe I'm confused here, but what does the executor need the information for? I thought this was a planner problem. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
2011/9/26 Tom Lane t...@sss.pgh.pa.us: Kohei KaiGai kai...@kaigai.gr.jp writes: One possible idea not to store the flag in RangeTblEntry is to utilize rte-relid to show the relation-id of the source view, when rtekind is RTE_SUBQUERY; that enables to pull the security_barrier flag in executor stage. Maybe I'm confused here, but what does the executor need the information for? I thought this was a planner problem. Sorry, planner was what I wanted to say. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Sat, Sep 24, 2011 at 5:37 PM, Noah Misch n...@leadboat.com wrote: On Fri, Sep 23, 2011 at 06:25:01PM -0400, Robert Haas wrote: On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote: The Part-1 implements corresponding SQL syntax stuffs which are security_barrier reloption of views, and LEAKPROOF option on creation of functions to be stored new pg_proc.proleakproof field. The way you have this implemented, we just blow away all view options whenever we do CREATE OR REPLACE VIEW. Is that the behavior we want? If a security_barrier view gets accidentally turned into a non-security_barrier view, doesn't that create a security_hole? I think CREATE OR REPLACE needs to keep meaning just that, never becoming replace some characteristics, merge others. The consequence is less than delightful here, but I don't have an idea that avoids this problem without running afoul of some previously-raised design constraint. Hmm, you might be right, although I'm not sure we've been 100% consistent about that, since we previously made CREATE OR REPLACE LANGUAGE not replace the owner with the current user. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
Robert Haas 09/25/11 10:58 AM I'm not sure we've been 100% consistent about that, since we previously made CREATE OR REPLACE LANGUAGE not replace the owner with the current user. I think we've been consistent in *not* changing security on an object when it is replaced. test=# create user someoneelse; CREATE ROLE test=# create user yetanother; CREATE ROLE test=# create function one() returns int language sql as 'select 1;'; CREATE FUNCTION test=# alter function one() owner to someoneelse; ALTER FUNCTION test=# revoke execute on function one() from public; REVOKE test=# create or replace function one() returns int language plpgsql as $$begin return 1; end;$$; CREATE FUNCTION test=# \df+ one() List of functions Schema | Name | Result data type | Argument data types | Type | Volatility |Owner| Language | Source code | Description +--+--+-+++-+--+--+- public | one | integer | | normal | volatile | someoneelse | plpgsql | begin return 1; end; | (1 row) test=# set role yetanother; SET test= select one(); ERROR: permission denied for function one -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
2011/9/24 Robert Haas robertmh...@gmail.com: On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote: I updated the patches of fix-leaky-view problem, according to the previous discussion. The NOLEAKY option was replaced by LEAKPROOF option, and several regression test cases were added. Rest of stuffs are unchanged. You have a leftover reference to NOLEAKY. Oops, I'll fix it. For convenience of reviewer, below is summary of these patches: The Part-1 implements corresponding SQL syntax stuffs which are security_barrier reloption of views, and LEAKPROOF option on creation of functions to be stored new pg_proc.proleakproof field. The way you have this implemented, we just blow away all view options whenever we do CREATE OR REPLACE VIEW. Is that the behavior we want? If a security_barrier view gets accidentally turned into a non-security_barrier view, doesn't that create a security_hole? I'm also wondering if the way you're using ResetViewOptions() is the right way to handle this anyhow. Isn't that going to update pg_class twice? I guess that's probably harmless from a performance standpoint, but wouldn't it be better not to? I guess we could define something like AT_ReplaceRelOptions to handle this case. IIRC, we had a discussion that the behavior should follow the case when a view is newly defined, even if it would be replaced actually. However, it seems to me consistent way to keep existing setting as long as user does not provide new option explicitly. If so, I think AT_ReplaceRelOptions enables to simplify the code to implement such a behavior. The documentation in general is not nearly adequate, at least IMHO. Do you think the description is poor to introduce the behavior changes corresponding to security_barrier option? OK, I'll try to update the documentation. I'm a bit nervous about storing security_barrier in the RTE. What happens to stored rules if the security_barrier option gets change later? The rte-security_barrier is evaluated when a query referencing security views get expanded. So, rte-security_barrier is not stored to catalog. Even if security_barrier option was changed after PREPARE statement with references to security view, our existing mechanism re-evaluate the query on EXECUTE, thus, it shall be executed as we expected. (As an aside, I didn't know this mechanism and surprised at EXECUTE works correctly, even if VIEW definition was changed after PREPARE.) Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Sun, Sep 25, 2011 at 3:25 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote: I'm a bit nervous about storing security_barrier in the RTE. What happens to stored rules if the security_barrier option gets change later? The rte-security_barrier is evaluated when a query referencing security views get expanded. So, rte-security_barrier is not stored to catalog. I think it is. If you create a view that involves an RTE, the node tree is going to get stored in pg_rewrite.ev_action. And it's going to include the security_barrier attribute, because you added outfuncs support for it... No? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Sun, Sep 25, 2011 at 11:22:03AM -0500, Kevin Grittner wrote: Robert Haas 09/25/11 10:58 AM I'm not sure we've been 100% consistent about that, since we previously made CREATE OR REPLACE LANGUAGE not replace the owner with the current user. I think we've been consistent in *not* changing security on an object when it is replaced. [CREATE OR REPLACE FUNCTION does not change proowner or proacl] Good point. C-O-R VIEW also preserves column default values. I believe we are consistent to the extent that everything possible to specify in each C-O-R statement gets replaced outright. The preserved characteristics *require* commands like GRANT, COMMENT and ALTER VIEW to set in the first place. The analogue I had in mind is SECURITY DEFINER, which C-O-R FUNCTION reverts to SECURITY INVOKER if it's not specified each time. That default is safe, though, while the proposed default of security_barrier=false is unsafe. Thanks, nm -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch n...@leadboat.com wrote: On Sun, Sep 25, 2011 at 11:22:03AM -0500, Kevin Grittner wrote: Robert Haas 09/25/11 10:58 AM I'm not sure we've been 100% consistent about that, since we previously made CREATE OR REPLACE LANGUAGE not replace the owner with the current user. I think we've been consistent in *not* changing security on an object when it is replaced. [CREATE OR REPLACE FUNCTION does not change proowner or proacl] Good point. C-O-R VIEW also preserves column default values. I believe we are consistent to the extent that everything possible to specify in each C-O-R statement gets replaced outright. The preserved characteristics *require* commands like GRANT, COMMENT and ALTER VIEW to set in the first place. The analogue I had in mind is SECURITY DEFINER, which C-O-R FUNCTION reverts to SECURITY INVOKER if it's not specified each time. That default is safe, though, while the proposed default of security_barrier=false is unsafe. Even though I normally take the opposite position, I still like the idea of dedicated syntax for this feature. Not knowing what view options we might end up with in the future, I hate having to decide on what the general behavior ought to be. But it would be easy to decide that CREATE SECURITY VIEW followed by CREATE OR REPLACE VIEW leaves the security flag set; it would be consistent with what we're doing with owner and acl information and wouldn't back us into any unpleasant decisions down the road. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Fri, Sep 23, 2011 at 06:25:01PM -0400, Robert Haas wrote: On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote: The Part-1 implements corresponding SQL syntax stuffs which are security_barrier reloption of views, and LEAKPROOF option on creation of functions to be stored new pg_proc.proleakproof field. The way you have this implemented, we just blow away all view options whenever we do CREATE OR REPLACE VIEW. Is that the behavior we want? If a security_barrier view gets accidentally turned into a non-security_barrier view, doesn't that create a security_hole? I think CREATE OR REPLACE needs to keep meaning just that, never becoming replace some characteristics, merge others. The consequence is less than delightful here, but I don't have an idea that avoids this problem without running afoul of some previously-raised design constraint. pgpFge1bfLlD6.pgp Description: PGP signature
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote: I updated the patches of fix-leaky-view problem, according to the previous discussion. The NOLEAKY option was replaced by LEAKPROOF option, and several regression test cases were added. Rest of stuffs are unchanged. You have a leftover reference to NOLEAKY. For convenience of reviewer, below is summary of these patches: The Part-1 implements corresponding SQL syntax stuffs which are security_barrier reloption of views, and LEAKPROOF option on creation of functions to be stored new pg_proc.proleakproof field. The way you have this implemented, we just blow away all view options whenever we do CREATE OR REPLACE VIEW. Is that the behavior we want? If a security_barrier view gets accidentally turned into a non-security_barrier view, doesn't that create a security_hole? I'm also wondering if the way you're using ResetViewOptions() is the right way to handle this anyhow. Isn't that going to update pg_class twice? I guess that's probably harmless from a performance standpoint, but wouldn't it be better not to? I guess we could define something like AT_ReplaceRelOptions to handle this case. The documentation in general is not nearly adequate, at least IMHO. I'm a bit nervous about storing security_barrier in the RTE. What happens to stored rules if the security_barrier option gets change later? More when I've had more time to look at this... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On 24 August 2011 13:38, Kohei Kaigai kohei.kai...@emea.nec.com wrote: The (2) is new stuff from the revision in commit-fest 1st. It enables to supply NOLEAKY option on CREATE FUNCTION statement, then the function is allowed to distribute across security barrier. Only superuser can set this option. NOLEAKY doesn't really sound appropriate as it sounds like pidgin English. Also, it could be read as Don't allow leaks in this function. Could we instead use something like TRUSTED or something akin to it being allowed to do more than safer functions? It then describes its level of behaviour rather than what it promises not to do. -- Thom Brown Twitter: @darkixion IRC (freenode): dark_ixion Registered Linux user: #516935 EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] [v9.2] Fix Leaky View Problem
2011/9/7 Thom Brown t...@linux.com: On 24 August 2011 13:38, Kohei Kaigai kohei.kai...@emea.nec.com wrote: The (2) is new stuff from the revision in commit-fest 1st. It enables to supply NOLEAKY option on CREATE FUNCTION statement, then the function is allowed to distribute across security barrier. Only superuser can set this option. NOLEAKY doesn't really sound appropriate as it sounds like pidgin English. Also, it could be read as Don't allow leaks in this function. Could we instead use something like TRUSTED or something akin to it being allowed to do more than safer functions? It then describes its level of behaviour rather than what it promises not to do. Thanks for your comment. I'm not a native English specker, so it is helpful. TRUSTED sounds meaningful for me, however, it is confusable with a concept of trusted procedure in label-based MAC. It is not only SELinux, Oracle's label based security also uses this term to mean a procedure that switches user's credential during its execution. http://download.oracle.com/docs/cd/B28359_01/network.111/b28529/storproc.htm So, how about CREDIBLE, instead of TRUSTED? Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On 7 September 2011 14:34, Kohei KaiGai kai...@kaigai.gr.jp wrote: 2011/9/7 Thom Brown t...@linux.com: On 24 August 2011 13:38, Kohei Kaigai kohei.kai...@emea.nec.com wrote: The (2) is new stuff from the revision in commit-fest 1st. It enables to supply NOLEAKY option on CREATE FUNCTION statement, then the function is allowed to distribute across security barrier. Only superuser can set this option. NOLEAKY doesn't really sound appropriate as it sounds like pidgin English. Also, it could be read as Don't allow leaks in this function. Could we instead use something like TRUSTED or something akin to it being allowed to do more than safer functions? It then describes its level of behaviour rather than what it promises not to do. Thanks for your comment. I'm not a native English specker, so it is helpful. TRUSTED sounds meaningful for me, however, it is confusable with a concept of trusted procedure in label-based MAC. It is not only SELinux, Oracle's label based security also uses this term to mean a procedure that switches user's credential during its execution. http://download.oracle.com/docs/cd/B28359_01/network.111/b28529/storproc.htm So, how about CREDIBLE, instead of TRUSTED? I can't say I'm keen on that alternative, but I'm probably not the one to participate in bike-shedding here, so I'll leave comment to you hackers. :) -- Thom Brown Twitter: @darkixion IRC (freenode): dark_ixion Registered Linux user: #516935 EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Wed, Sep 7, 2011 at 9:39 AM, Thom Brown t...@linux.com wrote: On 7 September 2011 14:34, Kohei KaiGai kai...@kaigai.gr.jp wrote: 2011/9/7 Thom Brown t...@linux.com: On 24 August 2011 13:38, Kohei Kaigai kohei.kai...@emea.nec.com wrote: The (2) is new stuff from the revision in commit-fest 1st. It enables to supply NOLEAKY option on CREATE FUNCTION statement, then the function is allowed to distribute across security barrier. Only superuser can set this option. NOLEAKY doesn't really sound appropriate as it sounds like pidgin English. Also, it could be read as Don't allow leaks in this function. Could we instead use something like TRUSTED or something akin to it being allowed to do more than safer functions? It then describes its level of behaviour rather than what it promises not to do. Thanks for your comment. I'm not a native English specker, so it is helpful. TRUSTED sounds meaningful for me, however, it is confusable with a concept of trusted procedure in label-based MAC. It is not only SELinux, Oracle's label based security also uses this term to mean a procedure that switches user's credential during its execution. http://download.oracle.com/docs/cd/B28359_01/network.111/b28529/storproc.htm So, how about CREDIBLE, instead of TRUSTED? I can't say I'm keen on that alternative, but I'm probably not the one to participate in bike-shedding here, so I'll leave comment to you hackers. :) I think TRUSTED actually does a reasonably good job capturing what we're after here, although I do share a bit of KaiGai's nervousness about terminological confusion. Still, I'd be inclined to go that way if we can't come up with anything better. CREDIBLE is definitely the wrong idea: that means believable, which sounds more like a statement about the function's results than about its side-effects. I thought about TACITURN, since we need the error messages to not be excessively informative, but that doesn't do a good job characterizing the hazard created by side-effects, or the potential for abuse due to - for example - deliberate division by zero. I also thought about PURE, which is a term that's sometimes used to describe code that throws no errors and has no side effects, and comes pretty close to our actual requirement here, but doesn't necessarily convey that a security concern is involved. Yet another idea would be to use a variant of TRUSTED, such as TRUSTWORTHY, just to avoid confusion with the idea of a trusted procedure, but I'm not that excited about that idea despite have no real specific gripe with it other than length. So at the moment I am leaning toward TRUSTED. Anyone else want to bikeshed? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
Robert Haas robertmh...@gmail.com wrote: Anyone else want to bikeshed? I'm not sure they beat TRUSTED, but: SECURE OPAQUE -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
2011/9/7 Robert Haas robertmh...@gmail.com: On Wed, Sep 7, 2011 at 9:39 AM, Thom Brown t...@linux.com wrote: On 7 September 2011 14:34, Kohei KaiGai kai...@kaigai.gr.jp wrote: 2011/9/7 Thom Brown t...@linux.com: On 24 August 2011 13:38, Kohei Kaigai kohei.kai...@emea.nec.com wrote: The (2) is new stuff from the revision in commit-fest 1st. It enables to supply NOLEAKY option on CREATE FUNCTION statement, then the function is allowed to distribute across security barrier. Only superuser can set this option. NOLEAKY doesn't really sound appropriate as it sounds like pidgin English. Also, it could be read as Don't allow leaks in this function. Could we instead use something like TRUSTED or something akin to it being allowed to do more than safer functions? It then describes its level of behaviour rather than what it promises not to do. Thanks for your comment. I'm not a native English specker, so it is helpful. TRUSTED sounds meaningful for me, however, it is confusable with a concept of trusted procedure in label-based MAC. It is not only SELinux, Oracle's label based security also uses this term to mean a procedure that switches user's credential during its execution. http://download.oracle.com/docs/cd/B28359_01/network.111/b28529/storproc.htm So, how about CREDIBLE, instead of TRUSTED? I can't say I'm keen on that alternative, but I'm probably not the one to participate in bike-shedding here, so I'll leave comment to you hackers. :) I think TRUSTED actually does a reasonably good job capturing what we're after here, although I do share a bit of KaiGai's nervousness about terminological confusion. Still, I'd be inclined to go that way if we can't come up with anything better. CREDIBLE is definitely the wrong idea: that means believable, which sounds more like a statement about the function's results than about its side-effects. I thought about TACITURN, since we need the error messages to not be excessively informative, but that doesn't do a good job characterizing the hazard created by side-effects, or the potential for abuse due to - for example - deliberate division by zero. I also thought about PURE, which is a term that's sometimes used to describe code that throws no errors and has no side effects, and comes pretty close to our actual requirement here, but doesn't necessarily convey that a security concern is involved. Yet another idea would be to use a variant of TRUSTED, such as TRUSTWORTHY, just to avoid confusion with the idea of a trusted procedure, but I'm not that excited about that idea despite have no real specific gripe with it other than length. So at the moment I am leaning toward TRUSTED. Anyone else want to bikeshed? I also become leaning toward TRUSTED, although we still have a bit risk of terminology confusion, because I assume it is quite rare case to set this option by DBA and we will able to expect DBAs who try to this option have correct knowledge about background of the leaky-view problem. I'll submit the patch again. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On 2011-09-07 16:02, Kevin Grittner wrote: Robert Haasrobertmh...@gmail.com wrote: Anyone else want to bikeshed? I'm not sure they beat TRUSTED, but: SECURE OPAQUE SAVE -- Yeb -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Wed, Sep 07, 2011 at 02:09:15PM +0100, Thom Brown wrote: On 24 August 2011 13:38, Kohei Kaigai kohei.kai...@emea.nec.com wrote: The (2) is new stuff from the revision in commit-fest 1st. It enables to supply NOLEAKY option on CREATE FUNCTION statement, then the function is allowed to distribute across security barrier. Only superuser can set this option. NOLEAKY doesn't really sound appropriate as it sounds like pidgin English. Also, it could be read as Don't allow leaks in this function. Could we instead use something like TRUSTED or something akin to it being allowed to do more than safer functions? It then describes its level of behaviour rather than what it promises not to do. I liked NOLEAKY for its semantics, though I probably would have spelled it LEAKPROOF. PostgreSQL will trust the function to implement a specific, relatively-unintuitive security policy. We want the function implementers to read that policy closely and not rely on any intuition they have about the trusted term of art. Our use of TRUSTED in CREATE LANGUAGE is more conventional, I think, as is the trusted nature of SECURITY DEFINER. In that vein, folks who actually need SECURITY DEFINER might first look at TRUSTED; NOLEAKY would not attract the same unwarranted attention. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
Noah Misch n...@leadboat.com writes: I liked NOLEAKY for its semantics, though I probably would have spelled it LEAKPROOF. PostgreSQL will trust the function to implement a specific, relatively-unintuitive security policy. We want the function implementers to read that policy closely and not rely on any intuition they have about the trusted term of art. Our use of TRUSTED in CREATE LANGUAGE is more conventional, I think, as is the trusted nature of SECURITY DEFINER. In that vein, folks who actually need SECURITY DEFINER might first look at TRUSTED; NOLEAKY would not attract the same unwarranted attention. I agree that TRUSTED is a pretty bad choice here because of the high probability that people will think it means something else than what it really means. LEAKPROOF isn't too bad. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
2011/9/7 Tom Lane t...@sss.pgh.pa.us: Noah Misch n...@leadboat.com writes: I liked NOLEAKY for its semantics, though I probably would have spelled it LEAKPROOF. PostgreSQL will trust the function to implement a specific, relatively-unintuitive security policy. We want the function implementers to read that policy closely and not rely on any intuition they have about the trusted term of art. Our use of TRUSTED in CREATE LANGUAGE is more conventional, I think, as is the trusted nature of SECURITY DEFINER. In that vein, folks who actually need SECURITY DEFINER might first look at TRUSTED; NOLEAKY would not attract the same unwarranted attention. I agree that TRUSTED is a pretty bad choice here because of the high probability that people will think it means something else than what it really means. LEAKPROOF isn't too bad. It seems to me LEAKPROOF is never confusable for everyone, and no conflicts with other concept, although it was not in my vocaburary. If no better idea anymore, I'll submit the patch again; with LEAKPROOF keyword. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On 09/07/2011 12:05 PM, Tom Lane wrote: I agree that TRUSTED is a pretty bad choice here because of the high probability that people will think it means something else than what it really means. Agreed. LEAKPROOF isn't too bad. It's fairly opaque unless you know the context, although that might be said of some of our other terms too. Someone coming across it is going to think What would it leak? cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
2011/9/7 Andrew Dunstan and...@dunslane.net: LEAKPROOF isn't too bad. It's fairly opaque unless you know the context, although that might be said of some of our other terms too. Someone coming across it is going to think What would it leak? It is introduced in the documentation. I'll add a point to this chapter in the introduction of this keyword. http://developer.postgresql.org/pgdocs/postgres/rules-privileges.html Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix Leaky View Problem
Andrew Dunstan and...@dunslane.net writes: On 09/07/2011 12:05 PM, Tom Lane wrote: LEAKPROOF isn't too bad. It's fairly opaque unless you know the context, although that might be said of some of our other terms too. Someone coming across it is going to think What would it leak? Well, the whole point is that we want people to RTFM instead of assuming they know what it means ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 2
On 2011-07-09 09:14, Kohei KaiGai wrote: OK, I'll try to modify the patch according to the flag of pg_proc design. As long as the default of user-defined function is off, and we provide built-in functions with appropriate configurations, it seems to me the burden of DBA is quite limited. A different solution to the leaky view problem could be to check access to a tuple at or near the heaptuple visibility level, in addition to adding tuple access filter conditions to the query. This would have both the possible performance benefits of the query rewriting solution, as the everything is filtered before further processing at the heaptuple visibility level. Fixing leaky views is not needed because they don't exist in this case, the code is straightforward, and there's less change of future security bugs by either misconfiguration of leakproof functions or code that might introduce another leak path. regards, -- Yeb Havinga http://www.mgrid.net/ Mastering Medical Data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 2
2011/7/20 Yeb Havinga yebhavi...@gmail.com: On 2011-07-09 09:14, Kohei KaiGai wrote: OK, I'll try to modify the patch according to the flag of pg_proc design. As long as the default of user-defined function is off, and we provide built-in functions with appropriate configurations, it seems to me the burden of DBA is quite limited. A different solution to the leaky view problem could be to check access to a tuple at or near the heaptuple visibility level, in addition to adding tuple access filter conditions to the query. This would have both the possible performance benefits of the query rewriting solution, as the everything is filtered before further processing at the heaptuple visibility level. Fixing leaky views is not needed because they don't exist in this case, the code is straightforward, and there's less change of future security bugs by either misconfiguration of leakproof functions or code that might introduce another leak path. I'm not fun with this approach. The harderst one to find out a solution is a way to distinguish qualifiers of security policy and others. Leaky functions looks like a harmless function, them the optimizer will distribute them onto particular scan plans. If it was executed on the visibility check of tuples, same problem will be reproduced. So, I'm still fun with a flag of pg_proc catalog and idea of security barrier. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 2
On Wed, Jul 20, 2011 at 09:02:59AM +0200, Yeb Havinga wrote: On 2011-07-09 09:14, Kohei KaiGai wrote: OK, I'll try to modify the patch according to the flag of pg_proc design. As long as the default of user-defined function is off, and we provide built-in functions with appropriate configurations, it seems to me the burden of DBA is quite limited. A different solution to the leaky view problem could be to check access to a tuple at or near the heaptuple visibility level, in addition to adding tuple access filter conditions to the query. This would have both the possible performance benefits of the query rewriting solution, as the everything is filtered before further processing at the heaptuple visibility level. Fixing leaky views is not needed because they don't exist in this case, the code is straightforward, and there's less change of future security bugs by either misconfiguration of leakproof functions or code that might introduce another leak path. The SQL-level semantics of the view define the access rules in question. How would you translate that into tests to apply at a lower level? -- Noah Mischhttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 2
On 2011-07-20 16:06, Noah Misch wrote: The SQL-level semantics of the view define the access rules in question. How would you translate that into tests to apply at a lower level? I assumed the leaky view thread was about row level security, not about access rules to views, since it was mentioned at the RLS wiki page for se-pgsql. Sorry for the confusion. regards, Yeb -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 2
On 2011-07-20 16:15, Yeb Havinga wrote: On 2011-07-20 16:06, Noah Misch wrote: The SQL-level semantics of the view define the access rules in question. How would you translate that into tests to apply at a lower level? I assumed the leaky view thread was about row level security, not about access rules to views, since it was mentioned at the RLS wiki page for se-pgsql. Sorry for the confusion. Had to digg a bit for the wiki, it was this one : http://wiki.postgresql.org/wiki/RLS#Issue:_A_leaky_VIEWs_for_RLS regards, Yeb -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 2
On Wed, Jul 20, 2011 at 04:23:10PM +0200, Yeb Havinga wrote: On 2011-07-20 16:15, Yeb Havinga wrote: On 2011-07-20 16:06, Noah Misch wrote: The SQL-level semantics of the view define the access rules in question. How would you translate that into tests to apply at a lower level? I assumed the leaky view thread was about row level security, not about access rules to views, since it was mentioned at the RLS wiki page for se-pgsql. Sorry for the confusion. Had to digg a bit for the wiki, it was this one : http://wiki.postgresql.org/wiki/RLS#Issue:_A_leaky_VIEWs_for_RLS It is about row-level security, broadly. These patches close the hazard described in the latter half of this page: http://www.postgresql.org/docs/9.0/static/rules-privileges.html In the example given there, phone NOT LIKE '412%' is the (row-level) access rule that needs to apply before any possibly-leaky function sees the tuple. -- Noah Mischhttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
2011/7/8 Noah Misch n...@2ndquadrant.com: On Fri, Jul 08, 2011 at 09:20:46AM +0100, Kohei KaiGai wrote: 2011/7/7 Noah Misch n...@2ndquadrant.com: On Thu, Jul 07, 2011 at 03:56:26PM +0100, Kohei KaiGai wrote: 2011/7/7 Noah Misch n...@2ndquadrant.com: On Wed, Jul 06, 2011 at 10:25:12PM +0200, Kohei KaiGai wrote: That gets the job done for today, but DefineVirtualRelation() should not need to know all view options by name to simply replace the existing list with a new one. ?I don't think you can cleanly use the ALTER TABLE SET/RESET code for this. ?Instead, compute an option list similar to how DefineRelation() does so at tablecmds.c:491, then update pg_class. My opinion is ALTER TABLE SET/RESET code should be enhanced to accept an operation to reset all the existing options, rather than tricky updates of pg_class. The pg_class update has ~20 lines of idiomatic code; see tablecmds.c:7931-7951. Even if idiomatic, another part of DefineVirtualRelation() uses AlterTableInternal(). I think a common way is more straightforward. The fact that we use ALTER TABLE ADD COLUMN in DefineVirtualRelation() is not itself cause to use ALTER TABLE SET (...) nearby. We should do so only if it brings some advantage, like simpler or more-robust code. I'm not seeing either advantage. Those can be points of style, so perhaps I have the poor taste here. The attached patch is a revised version according to the approach that updates pg_class system catalog before AlterTableInternal(). It invokes the new ResetViewOptions when rel-rd_options is not null, and it set null on the pg_class.reloptions of the view and increments command counter. Rest of stuffs are not changed from the v5. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp pgsql-v9.2-fix-leaky-view-part-0.v6.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 2
2011/7/9 Robert Haas robertmh...@gmail.com: On Fri, Jul 8, 2011 at 4:57 PM, Noah Misch n...@2ndquadrant.com wrote: Note that it does not matter whether we're actually doing an index scan -- a seq scan with a filter using only leakproof operators is equally acceptable. What I had in mind was to enumerate all operators in operator classes of indexes below each security view. Those become the leak-free operators for that security view. If the operator for an OpExpr is considered leak-free by all sources of its operands, then we may push it down. That's purely a high-level sketch: I haven't considered implementation concerns in any detail. The resulting behavior could be surprising: adding an index may change a plan without the new plan actually using the index. I lean toward favoring the pg_proc flag. Functions like texteq will be taken as leakproof even if no involved table has an index on a text column. It works for functions that will never take a place in an operator class, like length(text). When a user reports a qualifier not getting pushed down, the answer is much more satisfying: Run 'CREATE OR REPLACE FUNCTION ... I_DONT_LEAK' as a superuser. Compare to Define an operator class that includes the function, if needed, and create an otherwise-useless index. The main disadvantage I see is the loss of policy locality. Only a superuser (or maybe database owner?) can create or modify declared-leakproof functions, and that decision applies throughout the database. However, I think the other advantages clearly outweigh that loss. This strikes me as a fairly compelling refutation of Heikki's proposed approach. OK, I'll try to modify the patch according to the flag of pg_proc design. As long as the default of user-defined function is off, and we provide built-in functions with appropriate configurations, it seems to me the burden of DBA is quite limited. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
On Sat, Jul 09, 2011 at 09:00:30AM +0200, Kohei KaiGai wrote:
The attached patch is a revised version according to the approach that updates
pg_class system catalog before AlterTableInternal().
It invokes the new ResetViewOptions when rel-rd_options is not null, and it
set
null on the pg_class.reloptions of the view and increments command counter.
+ /*
+ * ResetViewOptions
+ *
+ * It clears all the reloptions prior to replacing
+ */
+ static void
+ ResetViewOptions(Oid viewOid)
+ {
+ Relationpg_class;
+ HeapTuple oldtup;
+ HeapTuple newtup;
+ Datum values[Natts_pg_class];
+ boolnulls[Natts_pg_class];
+ boolreplaces[Natts_pg_class];
+
+ pg_class = heap_open(RelationRelationId, RowExclusiveLock);
+
+ oldtup = SearchSysCache1(RELOID, DatumGetObjectId(viewOid));
Use SearchSysCacheCopy1, since you're modifying the tuple.
+ if (!HeapTupleIsValid(oldtup))
+ elog(ERROR, cache lookup failed for relation %u, viewOid);
+
+ memset(values, 0, sizeof(values));
+ memset(nulls, false, sizeof(nulls));
+ memset(replaces, false, sizeof(replaces));
+
+ replaces[Anum_pg_class_reloptions - 1] = true;
+ nulls[Anum_pg_class_reloptions - 1] = true;
+
+ newtup = heap_modify_tuple(oldtup, RelationGetDescr(pg_class),
+values, nulls,
replaces);
+ simple_heap_update(pg_class, newtup-t_self, newtup);
+
+ CatalogUpdateIndexes(pg_class, newtup);
+
+ ReleaseSysCache(oldtup);
+
+ heap_close(pg_class, RowExclusiveLock);
+
+ CommandCounterIncrement();
Why is a CCI necessary?
+ }
In any event, we seem to be converging on a version of parts 0 and 1 that are
ready for committer. However, Robert contends that this will not be committed
separately from part 2. Unless someone wishes to contest that, I suggest we
mark this Returned with Feedback and let the CF entry for part 2 subsume its
future development. Does that sound reasonable?
Thanks,
nm
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
2011/7/9 Noah Misch n...@2ndquadrant.com:
On Sat, Jul 09, 2011 at 09:00:30AM +0200, Kohei KaiGai wrote:
The attached patch is a revised version according to the approach that
updates
pg_class system catalog before AlterTableInternal().
It invokes the new ResetViewOptions when rel-rd_options is not null, and it
set
null on the pg_class.reloptions of the view and increments command counter.
+ /*
+ * ResetViewOptions
+ *
+ * It clears all the reloptions prior to replacing
+ */
+ static void
+ ResetViewOptions(Oid viewOid)
+ {
+ Relation pg_class;
+ HeapTuple oldtup;
+ HeapTuple newtup;
+ Datum values[Natts_pg_class];
+ bool nulls[Natts_pg_class];
+ bool replaces[Natts_pg_class];
+
+ pg_class = heap_open(RelationRelationId, RowExclusiveLock);
+
+ oldtup = SearchSysCache1(RELOID, DatumGetObjectId(viewOid));
Use SearchSysCacheCopy1, since you're modifying the tuple.
The heap_modify_tuple() allocates a new tuple as a copy of old tuple.
No need to worry about.
+ if (!HeapTupleIsValid(oldtup))
+ elog(ERROR, cache lookup failed for relation %u, viewOid);
+
+ memset(values, 0, sizeof(values));
+ memset(nulls, false, sizeof(nulls));
+ memset(replaces, false, sizeof(replaces));
+
+ replaces[Anum_pg_class_reloptions - 1] = true;
+ nulls[Anum_pg_class_reloptions - 1] = true;
+
+ newtup = heap_modify_tuple(oldtup, RelationGetDescr(pg_class),
+ values, nulls,
replaces);
+ simple_heap_update(pg_class, newtup-t_self, newtup);
+
+ CatalogUpdateIndexes(pg_class, newtup);
+
+ ReleaseSysCache(oldtup);
+
+ heap_close(pg_class, RowExclusiveLock);
+
+ CommandCounterIncrement();
Why is a CCI necessary?
ATExecSetRelOptions() reference the view to be updated using syscache,
however, this update will not become visible without CCI.
In the result, it will reference old tuple, then get an error because
it tries to
update already updated tuple.
+ }
In any event, we seem to be converging on a version of parts 0 and 1 that are
ready for committer. However, Robert contends that this will not be committed
separately from part 2. Unless someone wishes to contest that, I suggest we
mark this Returned with Feedback and let the CF entry for part 2 subsume its
future development. Does that sound reasonable?
At least, it seems to me we don't need to tackle to this matter from
the beginning
on the next commit fest again.
Thanks,
--
KaiGai Kohei kai...@kaigai.gr.jp
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
On Sat, Jul 09, 2011 at 10:52:33AM +0200, Kohei KaiGai wrote:
2011/7/9 Noah Misch n...@2ndquadrant.com:
On Sat, Jul 09, 2011 at 09:00:30AM +0200, Kohei KaiGai wrote:
The attached patch is a revised version according to the approach that
updates
pg_class system catalog before AlterTableInternal().
It invokes the new ResetViewOptions when rel-rd_options is not null, and
it set
null on the pg_class.reloptions of the view and increments command counter.
+ /*
+ * ResetViewOptions
+ *
+ * It clears all the reloptions prior to replacing
+ */
+ static void
+ ResetViewOptions(Oid viewOid)
+ {
+ Relation pg_class;
+ HeapTuple oldtup;
+ HeapTuple newtup;
+ Datum values[Natts_pg_class];
+ bool nulls[Natts_pg_class];
+ bool replaces[Natts_pg_class];
+
+ pg_class = heap_open(RelationRelationId, RowExclusiveLock);
+
+ oldtup = SearchSysCache1(RELOID, DatumGetObjectId(viewOid));
Use SearchSysCacheCopy1, since you're modifying the tuple.
The heap_modify_tuple() allocates a new tuple as a copy of old tuple.
No need to worry about.
Ah, yes. Sorry for the noise.
+ if (!HeapTupleIsValid(oldtup))
+ elog(ERROR, cache lookup failed for relation %u, viewOid);
+
+ memset(values, 0, sizeof(values));
+ memset(nulls, false, sizeof(nulls));
+ memset(replaces, false, sizeof(replaces));
+
+ replaces[Anum_pg_class_reloptions - 1] = true;
+ nulls[Anum_pg_class_reloptions - 1] = true;
+
+ newtup = heap_modify_tuple(oldtup, RelationGetDescr(pg_class),
+ values, nulls,
replaces);
+ simple_heap_update(pg_class, newtup-t_self, newtup);
+
+ CatalogUpdateIndexes(pg_class, newtup);
+
+ ReleaseSysCache(oldtup);
+
+ heap_close(pg_class, RowExclusiveLock);
+
+ CommandCounterIncrement();
Why is a CCI necessary?
ATExecSetRelOptions() reference the view to be updated using syscache,
however, this update will not become visible without CCI.
In the result, it will reference old tuple, then get an error because
it tries to
update already updated tuple.
Okay, thanks.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 2
2011/7/7 Noah Misch n...@2ndquadrant.com:
On Sun, Jul 03, 2011 at 11:41:47AM +0200, Kohei KaiGai wrote:
The simplified version of fix-leaky-view patch. The part of reloptions
for views got splitted out
into the part-0 patch, so it needs to be applied prior to this patch.
Rest of logic to prevent unexpected pushing down across security
barrier is not changed.
Thanks,
2011/6/6 Kohei Kaigai kohei.kai...@emea.nec.com:
This patch enables to fix up leaky-view problem using qualifiers that
reference only one-side of join-loop inside of view definition.
The point of this scenario is criteria to distribute qualifiers of
scanning-plan distributed in distribute_qual_to_rels(). If and when a
qualifiers that reference only one-side of join-loop, the optimizer may
distribute this qualifier into inside of the join-loop, even if it goes
over the boundary of a subquery expanded from a view for row-level
security.
This behavior allows us to reference whole of one-side of join-loop using
functions with side-effects.
The solution is quite simple; it prohibits to distribute qualifiers over
the boundary of subquery, however, performance cost is unignorable,
because it also disables to utilize obviously indexable qualifiers such as
(id=123), so this patch requires users a hint whether a particular view is
for row-level security, or not.
This patch newly adds CREATE SECURITY VIEW statement that marks a flag
to show this view was defined for row-level security purpose. This flag
shall be stored as reloptions.
If this flag was set, the optimizer does not distribute qualifiers over
the boundary of subqueries expanded from security views, except for
obviously safe qualifiers.
(Right now, we consider built-in indexable operators are safe, but it
might be arguable.)
I took a moderately-detailed look at this patch. This jumped out:
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
+static bool
+contain_leakable_functions_walker(Node *node, void *context)
+{
+ if (node == NULL)
+ return false;
+
+ if (IsA(node, FuncExpr))
+ {
+ /*
+ * Right now, we have no way to distinguish safe functions with
+ * leakable ones, so, we treat all the function call possibly
+ * leakable.
+ */
+ return true;
+ }
+ else if (IsA(node, OpExpr))
+ {
+ OpExpr *expr = (OpExpr *) node;
+
+ /*
+ * Right now, we assume operators implemented by built-in
functions
+ * are not leakable, so it does not need to prevent
optimization.
+ */
+ set_opfuncid(expr);
+ if (get_func_lang(expr-opfuncid) != INTERNALlanguageId)
+ return true;
+ /* else fall through to check args */
+ }
Any user can do this:
CREATE OPERATOR !-! (PROCEDURE = int4in, RIGHTARG = cstring);
SELECT !-! 'foo';
As I mentioned at the source code comments, this ad-hoc assumption was
come from we have no way to distinguish a non-leaky function from others.
So, I definitely love the approach (2), because only trusted function creator
can determine whether it is possible leaky or not.
Making a distinction based simply on the call being an operator vs. a function
is a dead end. I see these options:
1. The user defining a security view can be assumed to trust the operator
class
members of indexes defined on the tables he references. Keep track of which
those are and treat only them as non-leakable. This covers many interesting
cases, but it's probably tricky to implement and/or costly at runtime.
It requires DBA massive amount of detailed knowledge about functions underlying
operators used in a view. I don't think it is a realistic assumption.
2. Add a pg_proc flag indicating whether the function is known leak-free.
Simple, but tedious and perhaps error-prone.
+1
3. Trust operators owned by PGUID. This is simple and probably covers the
essential cases, but it's an ugly hack.
Some of built-in functions are also leaky. For example, int4div raise an error
when we try to divid a particular value by zero.
4. Trust nothing as leak-free. Simple; performance will be unattractive.
-1, Because of performance perspective.
Thanks,
--
KaiGai Kohei kai...@kaigai.gr.jp
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 2
On 08.07.2011 11:03, Kohei KaiGai wrote: 2011/7/7 Noah Mischn...@2ndquadrant.com: Making a distinction based simply on the call being an operator vs. a function is a dead end. I see these options: 1. The user defining a security view can be assumed to trust the operator class members of indexes defined on the tables he references. Keep track of which those are and treat only them as non-leakable. This covers many interesting cases, but it's probably tricky to implement and/or costly at runtime. It requires DBA massive amount of detailed knowledge about functions underlying operators used in a view. I don't think it is a realistic assumption. 2. Add a pg_proc flag indicating whether the function is known leak-free. Simple, but tedious and perhaps error-prone. +1 IMHO the situation from DBA's point of view is exactly opposite. Option two requires deep knowledge of this leaky views issue. The DBA needs to inspect any function he wants to mark as leak-free closely, and understand that innocent-looking things like casts can cause leaks. That is not feasible in practice. Option 1, however, requires no such knowledge. Operators used in indexes are already expected to not throw errors, or you would get errors when inserting certain values to the table, for example. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
2011/7/7 Noah Misch n...@2ndquadrant.com:
On Thu, Jul 07, 2011 at 03:56:26PM +0100, Kohei KaiGai wrote:
2011/7/7 Noah Misch n...@2ndquadrant.com:
On Wed, Jul 06, 2011 at 10:25:12PM +0200, Kohei KaiGai wrote:
*** a/src/backend/commands/view.c
--- b/src/backend/commands/view.c
--- 227,257
atcmd-def = (Node *) lfirst(c);
atcmds = lappend(atcmds, atcmd);
}
}
/*
+ * If optional parameters are specified, we must set options
+ * using ALTER TABLE SET OPTION internally.
+ */
+ if (list_length(options) 0)
+ {
+ atcmd = makeNode(AlterTableCmd);
+ atcmd-subtype = AT_SetRelOptions;
+ atcmd-def = (List *)options;
+
+ atcmds = lappend(atcmds, atcmd);
+ }
+ else
+ {
+ atcmd = makeNode(AlterTableCmd);
+ atcmd-subtype = AT_ResetRelOptions;
+ atcmd-def = (Node *)
list_make1(makeDefElem(security_barrier,
+
NULL));
+ }
+ if (atcmds != NIL)
+ AlterTableInternal(viewOid, atcmds, true);
+
+ /*
* Seems okay, so return the OID of the pre-existing view.
*/
relation_close(rel, NoLock); /* keep the lock! */
That gets the job done for today, but DefineVirtualRelation() should not
need
to know all view options by name to simply replace the existing list with a
new one. I don't think you can cleanly use the ALTER TABLE SET/RESET code
for
this. Instead, compute an option list similar to how DefineRelation()
does so
at tablecmds.c:491, then update pg_class.
My opinion is ALTER TABLE SET/RESET code should be enhanced to accept
an operation to reset all the existing options, rather than tricky
updates of pg_class.
The pg_class update has ~20 lines of idiomatic code; see
tablecmds.c:7931-7951.
Even if idiomatic, another part of DefineVirtualRelation() uses
AlterTableInternal().
I think a common way is more straightforward.
So, how about an idea to add a function that pull-out existing options
from syscache,
and merge with the supplied options list prior to AlterTableInternal()?
Thanks,
--
KaiGai Kohei kai...@kaigai.gr.jp
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 2
2011/7/8 Heikki Linnakangas heikki.linnakan...@enterprisedb.com: On 08.07.2011 11:03, Kohei KaiGai wrote: 2011/7/7 Noah Mischn...@2ndquadrant.com: Making a distinction based simply on the call being an operator vs. a function is a dead end. I see these options: 1. The user defining a security view can be assumed to trust the operator class members of indexes defined on the tables he references. Keep track of which those are and treat only them as non-leakable. This covers many interesting cases, but it's probably tricky to implement and/or costly at runtime. It requires DBA massive amount of detailed knowledge about functions underlying operators used in a view. I don't think it is a realistic assumption. 2. Add a pg_proc flag indicating whether the function is known leak-free. Simple, but tedious and perhaps error-prone. +1 IMHO the situation from DBA's point of view is exactly opposite. Option two requires deep knowledge of this leaky views issue. The DBA needs to inspect any function he wants to mark as leak-free closely, and understand that innocent-looking things like casts can cause leaks. That is not feasible in practice. Option 1, however, requires no such knowledge. Operators used in indexes are already expected to not throw errors, or you would get errors when inserting certain values to the table, for example. I might misread his description at first. Hmm. If we introduce DBA the scenario and the condition to push down qualifiers, it may be possible to explain more simply. A challenge of this approach is to determine what qualifier shall be used to index accesses in the stage of distribute_qual_to_rels(); prior to the optimizer's selection of access methods. Do you have any good idea, or suggestion? Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
