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 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)
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 : > 2012/2/14 Robert Haas : >> On Tue, Feb 14, 2012 at 4:55 AM, Kohei KaiGai wrote: >>> I could not find out where is the origin of grammer conflicts, although >>> it does not conflict with any options within ALTER FUNCTION. >>> >>> Do you think the idea of ALTER ... NOT LEAKPROOF should be >>> integrated within v9.2 timeline also? >> >> Yes. Did you notice that I attached a patch to make that work? I'll >> commit that today or tomorrow unless someone comes up with a better >> solution. >> > Yes. I'll be available to work on the feature based on this patch. > It was a headache of mine to implement alter statement to add/remove > leakproof attribute. > I also think we ought to stick create_function_3 into one of the parallel groups in the regression tests, if possible. Can you investigate that? >>> Not yet. This test does not have dependency with other tests, >>> so, I'm optimistic to run create_function_3 concurrently. >> >> Me, too. >> > I tried to move create_function_3 into the group of create_view and > create_index, then it works correctly. > > Thanks, > -- > KaiGai Kohei -- KaiGai Kohei 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)
2012/2/14 Robert Haas : > On Tue, Feb 14, 2012 at 4:55 AM, Kohei KaiGai 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 -- 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 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 : > On Mon, Feb 13, 2012 at 7:51 AM, Kohei KaiGai 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 -- 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 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: [HACKERS] [v9.2] Fix Leaky View Problem
On Sun, Jan 22, 2012 at 5:57 AM, Kohei KaiGai 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: [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 wrote:
> 2012/1/21 Robert Haas :
>> On Sat, Jan 21, 2012 at 3:59 AM, Kohei KaiGai 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
2012/1/21 Jeff Janes : > On Tue, Jan 17, 2012 at 7:08 PM, Robert Haas wrote: >> On Sun, Jan 8, 2012 at 10:32 AM, Kohei KaiGai 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 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: [HACKERS] [v9.2] Fix Leaky View Problem
On Tue, Jan 17, 2012 at 7:08 PM, Robert Haas wrote: > On Sun, Jan 8, 2012 at 10:32 AM, Kohei KaiGai 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: [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 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: [HACKERS] [v9.2] Fix Leaky View Problem
On Sun, Jan 8, 2012 at 10:32 AM, Kohei KaiGai 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: [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 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
>> 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 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 wrote: > 2011/12/23 Robert Haas : >> On Fri, Dec 23, 2011 at 5:56 AM, Kohei KaiGai 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 : > On Fri, Dec 23, 2011 at 5:56 AM, Kohei KaiGai 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 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
On Fri, Dec 23, 2011 at 5:56 AM, Kohei KaiGai 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
2011/12/22 Robert Haas : > On Mon, Dec 12, 2011 at 12:00 PM, Kohei KaiGai 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 -- 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 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 : > 2011/12/10 Kohei KaiGai : >> 2011/12/9 Robert Haas : >>> 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 > in
Re: [HACKERS] [v9.2] Fix Leaky View Problem
2011/12/9 Robert Haas : > On Thu, Dec 8, 2011 at 5:17 PM, Kohei KaiGai 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 -- 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 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 : > On Sat, Dec 3, 2011 at 3:19 AM, Kohei KaiGai 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 -- 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 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
On Wed, Dec 7, 2011 at 1:45 PM, Tom Lane 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
Robert Haas 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 Mon, Sep 26, 2011 at 3:59 PM, Tom Lane wrote: > Kohei KaiGai 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
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 : > 2011/11/2 Tom Lane : >> Kohei KaiGai 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 -- KaiGai Kohei -- 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 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
2011/11/2 Robert Haas : > On Wed, Nov 2, 2011 at 7:34 AM, Kohei KaiGai 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 -- 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 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
On Fri, Oct 21, 2011 at 10:36 AM, Kohei KaiGai 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
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 : > Robert Haas 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 -- 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 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 Wed, Oct 19, 2011 at 6:35 AM, Kohei KaiGai wrote: > 2011/10/19 Tom Lane : >> Robert Haas writes: >>> On Sun, Oct 16, 2011 at 4:46 AM, Kohei KaiGai 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
2011/10/19 Tom Lane : > Robert Haas writes: >> On Sun, Oct 16, 2011 at 4:46 AM, Kohei KaiGai 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 -- 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 writes: > On Sun, Oct 16, 2011 at 4:46 AM, Kohei KaiGai 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
On Sun, Oct 16, 2011 at 4:46 AM, Kohei KaiGai 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
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 : > On Mon, Oct 10, 2011 at 4:28 PM, Kohei KaiGai 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 suck
Re: [HACKERS] [v9.2] Fix Leaky View Problem
On Mon, Oct 10, 2011 at 4:28 PM, Kohei KaiGai 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
On 10 October 2011 21:28, Kohei KaiGai wrote: > 2011/10/10 Robert Haas : >> 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 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
2011/10/10 Robert Haas :
> On Sun, Oct 9, 2011 at 11:50 AM, Kohei KaiGai 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));
> +
... ...
> + 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
--
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 9, 2011 at 11:50 AM, Kohei KaiGai 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 time=0.320..
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 : > > 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
2011/10/8 Noah Misch : > 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 -- 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 : > On Sun, Sep 25, 2011 at 11:22:56PM -0400, Robert Haas wrote: >> On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch 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 -- 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 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 : > On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai 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 -- 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 : > Kohei KaiGai 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 -- 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 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
On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai 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
2011/9/26 Tom Lane : > Kohei KaiGai 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 -- 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 : > Kohei KaiGai 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 -- 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 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
Kohei KaiGai 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
2011/9/26 Robert Haas : > On Mon, Sep 26, 2011 at 5:58 AM, Kohei KaiGai 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 -- 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 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
On Mon, Sep 26, 2011 at 9:51 AM, Kohei KaiGai 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
2011/9/26 Robert Haas : > On Mon, Sep 26, 2011 at 6:28 AM, Kohei KaiGai wrote: >> 2011/9/26 Robert Haas : >>> On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch 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 -- 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 wrote: > 2011/9/26 Robert Haas : >> On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch 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 : > On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch 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 -- 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 : > On Sun, Sep 25, 2011 at 3:25 PM, Kohei KaiGai 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 -- 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 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 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 3:25 PM, Kohei KaiGai 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
2011/9/24 Robert Haas : > On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai 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 -- 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
On Sat, Sep 24, 2011 at 5:37 PM, Noah Misch 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 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
On Fri, Sep 23, 2011 at 06:25:01PM -0400, Robert Haas wrote: > On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai 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 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
Andrew Dunstan 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
2011/9/7 Andrew Dunstan : >> 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 -- 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 Tom Lane : > Noah Misch 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 -- 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 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
On Wed, Sep 07, 2011 at 02:09:15PM +0100, Thom Brown wrote: > On 24 August 2011 13:38, Kohei Kaigai 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
On 2011-09-07 16:02, Kevin Grittner wrote: Robert Haas 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
2011/9/7 Robert Haas : > On Wed, Sep 7, 2011 at 9:39 AM, Thom Brown wrote: >> On 7 September 2011 14:34, Kohei KaiGai wrote: >>> 2011/9/7 Thom Brown : >>> > On 24 August 2011 13:38, Kohei Kaigai 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 -- 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 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
On Wed, Sep 7, 2011 at 9:39 AM, Thom Brown wrote: > On 7 September 2011 14:34, Kohei KaiGai wrote: >> 2011/9/7 Thom Brown : >> > On 24 August 2011 13:38, Kohei Kaigai 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
On 7 September 2011 14:34, Kohei KaiGai wrote: > 2011/9/7 Thom Brown : > > On 24 August 2011 13:38, Kohei Kaigai 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
2011/9/7 Thom Brown : > On 24 August 2011 13:38, Kohei Kaigai 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 -- 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 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, 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 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 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 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
2011/7/20 Yeb Havinga : > 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 -- 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 1
On Sat, Jul 09, 2011 at 10:52:33AM +0200, Kohei KaiGai wrote:
> 2011/7/9 Noah Misch :
> > 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 1
2011/7/9 Noah Misch :
> 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
--
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 2
2011/7/9 Robert Haas : > On Fri, Jul 8, 2011 at 4:57 PM, Noah Misch 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 -- 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 : > On Fri, Jul 08, 2011 at 09:20:46AM +0100, Kohei KaiGai wrote: >> 2011/7/7 Noah Misch : >> > On Thu, Jul 07, 2011 at 03:56:26PM +0100, Kohei KaiGai wrote: >> >> 2011/7/7 Noah Misch : >> >> > 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 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
On Fri, Jul 8, 2011 at 4:57 PM, Noah Misch 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. -- 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, part 2
On Fri, Jul 08, 2011 at 10:09:54AM +0100, Kohei KaiGai wrote: > 2011/7/8 Heikki Linnakangas : > > On 08.07.2011 11:03, Kohei KaiGai wrote: > >> > >> 2011/7/7 Noah Misch: > >>> > >>> 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? 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. Incidentally, whichever policy we choose here can also loosen the constraints on qualifier order (part 1 of your original submission). -- 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 Fri, Jul 08, 2011 at 09:20:46AM +0100, Kohei KaiGai wrote: > 2011/7/7 Noah Misch : > > On Thu, Jul 07, 2011 at 03:56:26PM +0100, Kohei KaiGai wrote: > >> 2011/7/7 Noah Misch : > >> > 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. > 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()? It seems wrong to me to trawl through the view's existing option list just to replace it completely with a new option list. Again, it's subjective. If you'd like to proceed with this and let the committer decide, that's fine with me. 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 2
On Fri, Jul 8, 2011 at 4:18 AM, Heikki Linnakangas wrote: > 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. But, IMHO, the chance of the DBA wanting to set this flag is miniscule. I think that 99.9% of DBAs will be perfectly happy to just use whatever set we mark as built-ins. And an explicit pg_proc flag gives us a lot more flexibility. -- 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
