Обсуждение: [v9.2] Fix Leaky View Problem
Hi, The attached patches try to tackle our long-standing leaky view problem, and were revised according to the discussion wehad in the commit-fest 1st. We already knew two scenarios to leak contents of invisible tuples using functions with side-effects; such as error messagescontaining its arguments. The first one was the order of execution of qualifiers within a scan plan. Query optimizer shall pull-up simple sub-queriesinto inner-join due to the performance gain, however, it possibly cause a problem that functions supplied at outsideof the sub-query is launched earlier than functions come from inside of the sub-query; depending on the cost estimation.In the result, it allows users to reference contents of invisible tuples (to be filtered by view), if they providea function with side-effects as a part of WHERE clause. The second one was the distribution policy of qualifiers. In the case when a view (that intends row-level security) containsJOIN clause, we hope the qualifiers supplied from outside of the view to be launched after the table join, becausethe view may filter out some of tuples during checks of its join condition. However, the current query optimizer willdistribute a qualifier that reference only one-side of the join into inside of the join-loop to minimize number of tuplesto be joined. In the result, it also allows users to reference contents of invisible tuples. In the commit-fest 1st, we made a consensus that a part of views should perform as "security barrier" that enables to preventunexpected push-down and execution order of qualifiers; being marked by creator of the view. And, we also made a consensus obviously secure functions should be allowed to push-down across security barrier; to minimizeunnecessary performance damages. The part-1 patch implements SQL enhancement stuffs; (1) add reloption support on RELKIND_VIEW with "security_barrier" boolvariable (2) add pg_proc.proleaky flag to show whether the function is possibly leaky, or not. 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. Example) CREATE FUNCTION safe_func(text) RETURNS bool LANGUAGE 'C' NOLEAKY AS '$libdir/safe_lib', 'safe_func'; ^^^^^^^ A patch to add a new field into pg_proc always takes a large chunk, so the attached proctrans.php is the script I used toappend a new field to the existing functions. Right now, I marked it true (= possibly leaky), because we need to have adiscussion what shall be none-leaky functions in the default. The part-2 patch is same as we had discussed in the commit fest. Here is not updates except for rebasing to the latest tree.It enables to remember the nest level of the qualifier being originally used, and utilize it to sort order of the qualifiers. The part-3 patch was a bit revised, although its basic idea has not been changed. It enables to prevent qualifiers come from outside of security barrier being pushed down into inside of the security barrier,even if it references only a part of relations within the sub-query expanded from a view with "security_barrier"flag. Thanks, -- NEC Europe Ltd, SAP Global Competence Center KaiGai Kohei <kohei.kaigai@emea.nec.com>
Вложения
On 24 August 2011 13:38, Kohei Kaigai <Kohei.Kaigai@emea.nec.com> wrote:
The (2) is new stuff from the revision in commit-fest 1st. It enables to supply "NOLEAKY" option on CREATE FUNCTION statement, then the function is allowed to distribute across security barrier. Only superuser can set this option.
"NOLEAKY" doesn't really sound appropriate as it sounds like pidgin English. Also, it could be read as "Don't allow leaks in this function". Could we instead use something like TRUSTED or something akin to it being allowed to do more than safer functions? It then describes its level of behaviour rather than what it promises not to do.
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
2011/9/7 Thom Brown <thom@linux.com>: > On 24 August 2011 13:38, Kohei Kaigai <Kohei.Kaigai@emea.nec.com> wrote: >> >> The (2) is new stuff from the revision in commit-fest 1st. It enables to >> supply "NOLEAKY" option on CREATE FUNCTION statement, then the function is >> allowed to distribute across security barrier. Only superuser can set this >> option. > > "NOLEAKY" doesn't really sound appropriate as it sounds like pidgin English. > Also, it could be read as "Don't allow leaks in this function". Could we > instead use something like TRUSTED or something akin to it being allowed to > do more than safer functions? It then describes its level of behaviour > rather than what it promises not to do. > Thanks for your comment. I'm not a native English specker, so it is helpful. "TRUSTED" sounds meaningful for me, however, it is confusable with a concept of "trusted procedure" in label-based MAC. It is not only SELinux, Oracle's label based security also uses this term to mean a procedure that switches user's credential during its execution. http://download.oracle.com/docs/cd/B28359_01/network.111/b28529/storproc.htm So, how about "CREDIBLE", instead of "TRUSTED"? Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On 7 September 2011 14:34, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
2011/9/7 Thom Brown <thom@linux.com>:> On 24 August 2011 13:38, Kohei Kaigai <Kohei.Kaigai@emea.nec.com> wrote:Thanks for your comment. I'm not a native English specker, so it is helpful.
>>
>> 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.
>
"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
On Wed, Sep 7, 2011 at 9:39 AM, Thom Brown <thom@linux.com> wrote: > On 7 September 2011 14:34, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> 2011/9/7 Thom Brown <thom@linux.com>: >> > On 24 August 2011 13:38, Kohei Kaigai <Kohei.Kaigai@emea.nec.com> wrote: >> >> >> >> The (2) is new stuff from the revision in commit-fest 1st. It enables >> >> to >> >> supply "NOLEAKY" option on CREATE FUNCTION statement, then the function >> >> is >> >> allowed to distribute across security barrier. Only superuser can set >> >> this >> >> option. >> > >> > "NOLEAKY" doesn't really sound appropriate as it sounds like pidgin >> > English. >> > Also, it could be read as "Don't allow leaks in this function". Could >> > we >> > instead use something like TRUSTED or something akin to it being allowed >> > to >> > do more than safer functions? It then describes its level of behaviour >> > rather than what it promises not to do. >> > >> Thanks for your comment. I'm not a native English specker, so it is >> helpful. >> >> "TRUSTED" sounds meaningful for me, however, it is confusable with a >> concept >> of "trusted procedure" in label-based MAC. It is not only SELinux, >> Oracle's label >> based security also uses this term to mean a procedure that switches >> user's >> credential during its execution. >> >> http://download.oracle.com/docs/cd/B28359_01/network.111/b28529/storproc.htm >> >> So, how about "CREDIBLE", instead of "TRUSTED"? > > I can't say I'm keen on that alternative, but I'm probably not the one to > participate in bike-shedding here, so I'll leave comment to you hackers. :) I think TRUSTED actually does a reasonably good job capturing what we're after here, although I do share a bit of KaiGai's nervousness about terminological confusion. Still, I'd be inclined to go that way if we can't come up with anything better. CREDIBLE is definitely the wrong idea: that means "believable", which sounds more like a statement about the function's results than about its side-effects. I thought about TACITURN, since we need the error messages to not be excessively informative, but that doesn't do a good job characterizing the hazard created by side-effects, or the potential for abuse due to - for example - deliberate division by zero. I also thought about PURE, which is a term that's sometimes used to describe code that throws no errors and has no side effects, and comes pretty close to our actual requirement here, but doesn't necessarily convey that a security concern is involved. Yet another idea would be to use a variant of TRUSTED, such as TRUSTWORTHY, just to avoid confusion with the idea of a trusted procedure, but I'm not that excited about that idea despite have no real specific gripe with it other than length. So at the moment I am leaning toward TRUSTED. Anyone else want to bikeshed? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> wrote: > Anyone else want to bikeshed? I'm not sure they beat TRUSTED, but: SECURE OPAQUE -Kevin
2011/9/7 Robert Haas <robertmhaas@gmail.com>: > On Wed, Sep 7, 2011 at 9:39 AM, Thom Brown <thom@linux.com> wrote: >> On 7 September 2011 14:34, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >>> 2011/9/7 Thom Brown <thom@linux.com>: >>> > On 24 August 2011 13:38, Kohei Kaigai <Kohei.Kaigai@emea.nec.com> wrote: >>> >> >>> >> The (2) is new stuff from the revision in commit-fest 1st. It enables >>> >> to >>> >> supply "NOLEAKY" option on CREATE FUNCTION statement, then the function >>> >> is >>> >> allowed to distribute across security barrier. Only superuser can set >>> >> this >>> >> option. >>> > >>> > "NOLEAKY" doesn't really sound appropriate as it sounds like pidgin >>> > English. >>> > Also, it could be read as "Don't allow leaks in this function". Could >>> > we >>> > instead use something like TRUSTED or something akin to it being allowed >>> > to >>> > do more than safer functions? It then describes its level of behaviour >>> > rather than what it promises not to do. >>> > >>> Thanks for your comment. I'm not a native English specker, so it is >>> helpful. >>> >>> "TRUSTED" sounds meaningful for me, however, it is confusable with a >>> concept >>> of "trusted procedure" in label-based MAC. It is not only SELinux, >>> Oracle's label >>> based security also uses this term to mean a procedure that switches >>> user's >>> credential during its execution. >>> >>> http://download.oracle.com/docs/cd/B28359_01/network.111/b28529/storproc.htm >>> >>> So, how about "CREDIBLE", instead of "TRUSTED"? >> >> I can't say I'm keen on that alternative, but I'm probably not the one to >> participate in bike-shedding here, so I'll leave comment to you hackers. :) > > I think TRUSTED actually does a reasonably good job capturing what > we're after here, although I do share a bit of KaiGai's nervousness > about terminological confusion. Still, I'd be inclined to go that way > if we can't come up with anything better. CREDIBLE is definitely the > wrong idea: that means "believable", which sounds more like a > statement about the function's results than about its side-effects. I > thought about TACITURN, since we need the error messages to not be > excessively informative, but that doesn't do a good job characterizing > the hazard created by side-effects, or the potential for abuse due to > - for example - deliberate division by zero. I also thought about > PURE, which is a term that's sometimes used to describe code that > throws no errors and has no side effects, and comes pretty close to > our actual requirement here, but doesn't necessarily convey that a > security concern is involved. Yet another idea would be to use a > variant of TRUSTED, such as TRUSTWORTHY, just to avoid confusion with > the idea of a trusted procedure, but I'm not that excited about that > idea despite have no real specific gripe with it other than length. > So at the moment I am leaning toward TRUSTED. > > Anyone else want to bikeshed? > I also become leaning toward TRUSTED, although we still have a bit risk of terminology confusion, because I assume it is quite rare case to set this option by DBA and we will able to expect DBAs who try to this option have correct knowledge about background of the leaky-view problem. I'll submit the patch again. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On 2011-09-07 16:02, Kevin Grittner wrote: > Robert Haas<robertmhaas@gmail.com> wrote: > >> Anyone else want to bikeshed? > > I'm not sure they beat TRUSTED, but: > > SECURE > OPAQUE SAVE -- Yeb
On Wed, Sep 07, 2011 at 02:09:15PM +0100, Thom Brown wrote: > On 24 August 2011 13:38, Kohei Kaigai <Kohei.Kaigai@emea.nec.com> wrote: > > > The (2) is new stuff from the revision in commit-fest 1st. It enables to > > supply "NOLEAKY" option on CREATE FUNCTION statement, then the function is > > allowed to distribute across security barrier. Only superuser can set this > > option. > > > > "NOLEAKY" doesn't really sound appropriate as it sounds like pidgin English. > Also, it could be read as "Don't allow leaks in this function". Could we > instead use something like TRUSTED or something akin to it being allowed to > do more than safer functions? It then describes its level of behaviour > rather than what it promises not to do. I liked NOLEAKY for its semantics, though I probably would have spelled it "LEAKPROOF". PostgreSQL will trust the function to implement a specific, relatively-unintuitive security policy. We want the function implementers to read that policy closely and not rely on any intuition they have about the "trusted" term of art. Our use of TRUSTED in CREATE LANGUAGE is more conventional, I think, as is the trusted nature of SECURITY DEFINER. In that vein, folks who actually need SECURITY DEFINER might first look at TRUSTED; NOLEAKY would not attract the same unwarranted attention.
Noah Misch <noah@leadboat.com> writes: > I liked NOLEAKY for its semantics, though I probably would have spelled it > "LEAKPROOF". PostgreSQL will trust the function to implement a specific, > relatively-unintuitive security policy. We want the function implementers to > read that policy closely and not rely on any intuition they have about the > "trusted" term of art. Our use of TRUSTED in CREATE LANGUAGE is more > conventional, I think, as is the trusted nature of SECURITY DEFINER. In that > vein, folks who actually need SECURITY DEFINER might first look at TRUSTED; > NOLEAKY would not attract the same unwarranted attention. I agree that TRUSTED is a pretty bad choice here because of the high probability that people will think it means something else than what it really means. LEAKPROOF isn't too bad. regards, tom lane
2011/9/7 Tom Lane <tgl@sss.pgh.pa.us>: > Noah Misch <noah@leadboat.com> writes: >> I liked NOLEAKY for its semantics, though I probably would have spelled it >> "LEAKPROOF". PostgreSQL will trust the function to implement a specific, >> relatively-unintuitive security policy. We want the function implementers to >> read that policy closely and not rely on any intuition they have about the >> "trusted" term of art. Our use of TRUSTED in CREATE LANGUAGE is more >> conventional, I think, as is the trusted nature of SECURITY DEFINER. In that >> vein, folks who actually need SECURITY DEFINER might first look at TRUSTED; >> NOLEAKY would not attract the same unwarranted attention. > > I agree that TRUSTED is a pretty bad choice here because of the high > probability that people will think it means something else than what > it really means. LEAKPROOF isn't too bad. > It seems to me LEAKPROOF is never confusable for everyone, and no conflicts with other concept, although it was not in my vocaburary. If no better idea anymore, I'll submit the patch again; with LEAKPROOF keyword. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
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
2011/9/7 Andrew Dunstan <andrew@dunslane.net>: >> LEAKPROOF isn't too bad. >> >> > > It's fairly opaque unless you know the context, although that might be said > of some of our other terms too. Someone coming across it is going to think > "What would it leak?" > It is introduced in the documentation. I'll add a point to this chapter in the introduction of this keyword. http://developer.postgresql.org/pgdocs/postgres/rules-privileges.html Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Andrew Dunstan <andrew@dunslane.net> writes: > On 09/07/2011 12:05 PM, Tom Lane wrote: >> LEAKPROOF isn't too bad. > It's fairly opaque unless you know the context, although that might be > said of some of our other terms too. Someone coming across it is going > to think "What would it leak?" Well, the whole point is that we want people to RTFM instead of assuming they know what it means ... regards, tom lane
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. 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 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. Thanks, 2011/9/7 Kohei KaiGai <kaigai@kaigai.gr.jp>: > 2011/9/7 Tom Lane <tgl@sss.pgh.pa.us>: >> Noah Misch <noah@leadboat.com> writes: >>> I liked NOLEAKY for its semantics, though I probably would have spelled it >>> "LEAKPROOF". PostgreSQL will trust the function to implement a specific, >>> relatively-unintuitive security policy. We want the function implementers to >>> read that policy closely and not rely on any intuition they have about the >>> "trusted" term of art. Our use of TRUSTED in CREATE LANGUAGE is more >>> conventional, I think, as is the trusted nature of SECURITY DEFINER. In that >>> vein, folks who actually need SECURITY DEFINER might first look at TRUSTED; >>> NOLEAKY would not attract the same unwarranted attention. >> >> I agree that TRUSTED is a pretty bad choice here because of the high >> probability that people will think it means something else than what >> it really means. LEAKPROOF isn't too bad. >> > It seems to me LEAKPROOF is never confusable for everyone, and > no conflicts with other concept, although it was not in my vocaburary. > > If no better idea anymore, I'll submit the patch again; with LEAKPROOF keyword. > > Thanks, > -- > KaiGai Kohei <kaigai@kaigai.gr.jp> > -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Вложения
On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > I updated the patches of fix-leaky-view problem, according to the > previous discussion. > The "NOLEAKY" option was replaced by "LEAKPROOF" option, and several regression > test cases were added. Rest of stuffs are unchanged. You have a leftover reference to NOLEAKY. > For convenience of reviewer, below is summary of these patches: > > The Part-1 implements corresponding SQL syntax stuffs which are > "security_barrier" > reloption of views, and "LEAKPROOF" option on creation of functions to be stored > new pg_proc.proleakproof field. The way you have this implemented, we just blow away all view options whenever we do CREATE OR REPLACE VIEW. Is that the behavior we want? If a security_barrier view gets accidentally turned into a non-security_barrier view, doesn't that create a security_hole? I'm also wondering if the way you're using ResetViewOptions() is the right way to handle this anyhow. Isn't that going to update pg_class twice? I guess that's probably harmless from a performance standpoint, but wouldn't it be better not to? I guess we could define something like AT_ReplaceRelOptions to handle this case. The documentation in general is not nearly adequate, at least IMHO. I'm a bit nervous about storing security_barrier in the RTE. What happens to stored rules if the security_barrier option gets change later? More when I've had more time to look at this... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Sep 23, 2011 at 06:25:01PM -0400, Robert Haas wrote: > On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > > The Part-1 implements corresponding SQL syntax stuffs which are > > "security_barrier" > > reloption of views, and "LEAKPROOF" option on creation of functions to be stored > > new pg_proc.proleakproof field. > > The way you have this implemented, we just blow away all view options > whenever we do CREATE OR REPLACE VIEW. Is that the behavior we want? > If a security_barrier view gets accidentally turned into a > non-security_barrier view, doesn't that create a security_hole? I think CREATE OR REPLACE needs to keep meaning just that, never becoming "replace some characteristics, merge others". The consequence is less than delightful here, but I don't have an idea that avoids this problem without running afoul of some previously-raised design constraint.
On Sat, Sep 24, 2011 at 5:37 PM, Noah Misch <noah@leadboat.com> wrote: > On Fri, Sep 23, 2011 at 06:25:01PM -0400, Robert Haas wrote: >> On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> > The Part-1 implements corresponding SQL syntax stuffs which are >> > "security_barrier" >> > reloption of views, and "LEAKPROOF" option on creation of functions to be stored >> > new pg_proc.proleakproof field. >> >> The way you have this implemented, we just blow away all view options >> whenever we do CREATE OR REPLACE VIEW. Is that the behavior we want? >> If a security_barrier view gets accidentally turned into a >> non-security_barrier view, doesn't that create a security_hole? > > I think CREATE OR REPLACE needs to keep meaning just that, never becoming > "replace some characteristics, merge others". The consequence is less than > delightful here, but I don't have an idea that avoids this problem without > running afoul of some previously-raised design constraint. Hmm, you might be right, although I'm not sure we've been 100% consistent about that, since we previously made CREATE OR REPLACE LANGUAGE not replace the owner with the current user. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
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 functionsSchema | 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
2011/9/24 Robert Haas <robertmhaas@gmail.com>: > On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> I updated the patches of fix-leaky-view problem, according to the >> previous discussion. >> The "NOLEAKY" option was replaced by "LEAKPROOF" option, and several regression >> test cases were added. Rest of stuffs are unchanged. > > You have a leftover reference to NOLEAKY. > Oops, I'll fix it. >> For convenience of reviewer, below is summary of these patches: >> >> The Part-1 implements corresponding SQL syntax stuffs which are >> "security_barrier" >> reloption of views, and "LEAKPROOF" option on creation of functions to be stored >> new pg_proc.proleakproof field. > > The way you have this implemented, we just blow away all view options > whenever we do CREATE OR REPLACE VIEW. Is that the behavior we want? > If a security_barrier view gets accidentally turned into a > non-security_barrier view, doesn't that create a security_hole? > > I'm also wondering if the way you're using ResetViewOptions() is the > right way to handle this anyhow. Isn't that going to update pg_class > twice? I guess that's probably harmless from a performance > standpoint, but wouldn't it be better not to? I guess we could define > something like AT_ReplaceRelOptions to handle this case. > IIRC, we had a discussion that the behavior should follow the case when a view is newly defined, even if it would be replaced actually. However, it seems to me consistent way to keep existing setting as long as user does not provide new option explicitly. If so, I think AT_ReplaceRelOptions enables to simplify the code to implement such a behavior. > The documentation in general is not nearly adequate, at least IMHO. > Do you think the description is poor to introduce the behavior changes corresponding to security_barrier option? OK, I'll try to update the documentation. > I'm a bit nervous about storing security_barrier in the RTE. What > happens to stored rules if the security_barrier option gets change > later? > The rte->security_barrier is evaluated when a query referencing security views get expanded. So, rte->security_barrier is not stored to catalog. Even if security_barrier option was changed after PREPARE statement with references to security view, our existing mechanism re-evaluate the query on EXECUTE, thus, it shall be executed as we expected. (As an aside, I didn't know this mechanism and surprised at EXECUTE works correctly, even if VIEW definition was changed after PREPARE.) Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On Sun, Sep 25, 2011 at 3:25 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> I'm a bit nervous about storing security_barrier in the RTE. What >> happens to stored rules if the security_barrier option gets change >> later? >> > The rte->security_barrier is evaluated when a query referencing security > views get expanded. So, rte->security_barrier is not stored to catalog. I think it is. If you create a view that involves an RTE, the node tree is going to get stored in pg_rewrite.ev_action. And it's going to include the security_barrier attribute, because you added outfuncs support for it... No? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
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
On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch <noah@leadboat.com> wrote: > On Sun, Sep 25, 2011 at 11:22:03AM -0500, Kevin Grittner wrote: >> Robert Haas 09/25/11 10:58 AM >>> >> >> > I'm not sure we've been 100% consistent about that, since we >> > previously made CREATE OR REPLACE LANGUAGE not replace the owner >> > with the current user. >> >> I think we've been consistent in *not* changing security on an >> object when it is replaced. > >> [CREATE OR REPLACE FUNCTION does not change proowner or proacl] > > Good point. C-O-R VIEW also preserves column default values. I believe we are > consistent to the extent that everything possible to specify in each C-O-R > statement gets replaced outright. The preserved characteristics *require* > commands like GRANT, COMMENT and ALTER VIEW to set in the first place. > > The analogue I had in mind is SECURITY DEFINER, which C-O-R FUNCTION reverts to > SECURITY INVOKER if it's not specified each time. That default is safe, though, > while the proposed default of security_barrier=false is unsafe. Even though I normally take the opposite position, I still like the idea of dedicated syntax for this feature. Not knowing what view options we might end up with in the future, I hate having to decide on what the general behavior ought to be. But it would be easy to decide that CREATE SECURITY VIEW followed by CREATE OR REPLACE VIEW leaves the security flag set; it would be consistent with what we're doing with owner and acl information and wouldn't back us into any unpleasant decisions down the road. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
2011/9/26 Robert Haas <robertmhaas@gmail.com>: > On Sun, Sep 25, 2011 at 3:25 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >>> I'm a bit nervous about storing security_barrier in the RTE. What >>> happens to stored rules if the security_barrier option gets change >>> later? >>> >> The rte->security_barrier is evaluated when a query referencing security >> views get expanded. So, rte->security_barrier is not stored to catalog. > > I think it is. If you create a view that involves an RTE, the node > tree is going to get stored in pg_rewrite.ev_action. And it's going > to include the security_barrier attribute, because you added outfuncs > support for it... > > No? > IIUC, nested views are also expanded when user's query gets rewritten. Thus, rte->security_barrier shall be set based on the latest configuration of the view. I injected an elog(NOTICE, ...) to confirm the behavior, when security_barrier flag was set on rte->security_barrier at ApplyRetrieveRule(). postgres=# CREATE VIEW v1 WITH (security_barrier=true) AS SELECT * FROM t1 WHERE a % 2 = 0; CREATE VIEW postgres=# CREATE VIEW v2 WITH (security_barrier=true) AS SELECT a + a AS aa, b FROM v1; CREATE VIEW postgres=# SELECT * FROM v2; NOTICE: security barrier set on v1 NOTICE: security barrier set on v2aa | b ----+----- 4 | bbb (1 row) postgres=# ALTER TABLE v1 SET (security_barrier=false); ALTER TABLE postgres=# SELECT * FROM v2; NOTICE: security barrier set on v2aa | 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 v2aa | 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 <kaigai@kaigai.gr.jp>
2011/9/26 Robert Haas <robertmhaas@gmail.com>: > On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch <noah@leadboat.com> wrote: >> On Sun, Sep 25, 2011 at 11:22:03AM -0500, Kevin Grittner wrote: >>> Robert Haas 09/25/11 10:58 AM >>> >>> >>> > I'm not sure we've been 100% consistent about that, since we >>> > previously made CREATE OR REPLACE LANGUAGE not replace the owner >>> > with the current user. >>> >>> I think we've been consistent in *not* changing security on an >>> object when it is replaced. >> >>> [CREATE OR REPLACE FUNCTION does not change proowner or proacl] >> >> Good point. C-O-R VIEW also preserves column default values. I believe we are >> consistent to the extent that everything possible to specify in each C-O-R >> statement gets replaced outright. The preserved characteristics *require* >> commands like GRANT, COMMENT and ALTER VIEW to set in the first place. >> >> The analogue I had in mind is SECURITY DEFINER, which C-O-R FUNCTION reverts to >> SECURITY INVOKER if it's not specified each time. That default is safe, though, >> while the proposed default of security_barrier=false is unsafe. > > Even though I normally take the opposite position, I still like the > idea of dedicated syntax for this feature. Not knowing what view > options we might end up with in the future, I hate having to decide on > what the general behavior ought to be. But it would be easy to decide > that CREATE SECURITY VIEW followed by CREATE OR REPLACE VIEW leaves > the security flag set; it would be consistent with what we're doing > with owner and acl information and wouldn't back us into any > unpleasant decisions down the road. > Does the CREATE SECURITY VIEW statement mean a synonym of CREATE VIEW ... WITH (security_barrier=true) ? If so, it seems to me reasonable to keep the configuration when user provides no explicit option. 1) an explicit WITH(security_barrier=true) / CREATE SECURITY VIEW-> It always turns on a security_barrier option. 2) an explicit WITH(security_barrier=false)-> It always turns off security_barrier option. 3) no explicit option / CREATE VIEW-> Keep existing configuration, although inconsist with SECURITY DEFINER Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On Mon, Sep 26, 2011 at 6:28 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > 2011/9/26 Robert Haas <robertmhaas@gmail.com>: >> On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch <noah@leadboat.com> wrote: >>> On Sun, Sep 25, 2011 at 11:22:03AM -0500, Kevin Grittner wrote: >>>> Robert Haas 09/25/11 10:58 AM >>> >>>> >>>> > I'm not sure we've been 100% consistent about that, since we >>>> > previously made CREATE OR REPLACE LANGUAGE not replace the owner >>>> > with the current user. >>>> >>>> I think we've been consistent in *not* changing security on an >>>> object when it is replaced. >>> >>>> [CREATE OR REPLACE FUNCTION does not change proowner or proacl] >>> >>> Good point. C-O-R VIEW also preserves column default values. I believe we are >>> consistent to the extent that everything possible to specify in each C-O-R >>> statement gets replaced outright. The preserved characteristics *require* >>> commands like GRANT, COMMENT and ALTER VIEW to set in the first place. >>> >>> The analogue I had in mind is SECURITY DEFINER, which C-O-R FUNCTION reverts to >>> SECURITY INVOKER if it's not specified each time. That default is safe, though, >>> while the proposed default of security_barrier=false is unsafe. >> >> Even though I normally take the opposite position, I still like the >> idea of dedicated syntax for this feature. Not knowing what view >> options we might end up with in the future, I hate having to decide on >> what the general behavior ought to be. But it would be easy to decide >> that CREATE SECURITY VIEW followed by CREATE OR REPLACE VIEW leaves >> the security flag set; it would be consistent with what we're doing >> with owner and acl information and wouldn't back us into any >> unpleasant decisions down the road. >> > Does the CREATE SECURITY VIEW statement mean a synonym of > CREATE VIEW ... WITH (security_barrier=true) ? > > If so, it seems to me reasonable to keep the configuration when user > provides no explicit option. > > 1) an explicit WITH(security_barrier=true) / CREATE SECURITY VIEW > -> It always turns on a security_barrier option. > > 2) an explicit WITH(security_barrier=false) > -> It always turns off security_barrier option. > > 3) no explicit option / CREATE VIEW > -> Keep existing configuration, although inconsist with SECURITY DEFINER No, you're missing my point completely. If we use a flexible options syntax here, then we have to decide on what behavior CREATE OR REPLACE should have for all future options, without knowing what they are yet, or what behavior will be appropriate. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
2011/9/26 Robert Haas <robertmhaas@gmail.com>: > On Mon, Sep 26, 2011 at 6:28 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> 2011/9/26 Robert Haas <robertmhaas@gmail.com>: >>> On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch <noah@leadboat.com> wrote: >>>> On Sun, Sep 25, 2011 at 11:22:03AM -0500, Kevin Grittner wrote: >>>>> Robert Haas 09/25/11 10:58 AM >>> >>>>> >>>>> > I'm not sure we've been 100% consistent about that, since we >>>>> > previously made CREATE OR REPLACE LANGUAGE not replace the owner >>>>> > with the current user. >>>>> >>>>> I think we've been consistent in *not* changing security on an >>>>> object when it is replaced. >>>> >>>>> [CREATE OR REPLACE FUNCTION does not change proowner or proacl] >>>> >>>> Good point. C-O-R VIEW also preserves column default values. I believe we are >>>> consistent to the extent that everything possible to specify in each C-O-R >>>> statement gets replaced outright. The preserved characteristics *require* >>>> commands like GRANT, COMMENT and ALTER VIEW to set in the first place. >>>> >>>> The analogue I had in mind is SECURITY DEFINER, which C-O-R FUNCTION reverts to >>>> SECURITY INVOKER if it's not specified each time. That default is safe, though, >>>> while the proposed default of security_barrier=false is unsafe. >>> >>> Even though I normally take the opposite position, I still like the >>> idea of dedicated syntax for this feature. Not knowing what view >>> options we might end up with in the future, I hate having to decide on >>> what the general behavior ought to be. But it would be easy to decide >>> that CREATE SECURITY VIEW followed by CREATE OR REPLACE VIEW leaves >>> the security flag set; it would be consistent with what we're doing >>> with owner and acl information and wouldn't back us into any >>> unpleasant decisions down the road. >>> >> Does the CREATE SECURITY VIEW statement mean a synonym of >> CREATE VIEW ... WITH (security_barrier=true) ? >> >> If so, it seems to me reasonable to keep the configuration when user >> provides no explicit option. >> >> 1) an explicit WITH(security_barrier=true) / CREATE SECURITY VIEW >> -> It always turns on a security_barrier option. >> >> 2) an explicit WITH(security_barrier=false) >> -> It always turns off security_barrier option. >> >> 3) no explicit option / CREATE VIEW >> -> Keep existing configuration, although inconsist with SECURITY DEFINER > > No, you're missing my point completely. If we use a flexible options > syntax here, then we have to decide on what behavior CREATE OR REPLACE > should have for all future options, without knowing what they are yet, > or what behavior will be appropriate. > Hmm. Indeed, it seems to me fair enough reason. In this syntax case, the only way to clear the security_barrier flag is to drop view once, then create a view, isn't it? And, is the security_barrier flag still stored within reloptions field? Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On Mon, Sep 26, 2011 at 9:51 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> No, you're missing my point completely. If we use a flexible options >> syntax here, then we have to decide on what behavior CREATE OR REPLACE >> should have for all future options, without knowing what they are yet, >> or what behavior will be appropriate. >> > Hmm. Indeed, it seems to me fair enough reason. > > In this syntax case, the only way to clear the security_barrier flag > is to drop view > once, then create a view, isn't it? I was imagining we'd have ALTER VIEW .. [NO] SECURITY or something like that. > And, is the security_barrier flag still stored within reloptions field? No. That would be missing the point. But keep in mind no one else has endorsed my reasoning on this one as yet... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Sep 26, 2011 at 5:58 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> I think it is. If you create a view that involves an RTE, the node >> tree is going to get stored in pg_rewrite.ev_action. And it's going >> to include the security_barrier attribute, because you added outfuncs >> support for it... >> >> No? >> > IIUC, nested views are also expanded when user's query gets rewritten. > Thus, rte->security_barrier shall be set based on the latest configuration > of the view. > I injected an elog(NOTICE, ...) to confirm the behavior, when security_barrier > flag was set on rte->security_barrier at ApplyRetrieveRule(). Hmm, OK. I am still not convinced that this is the right approach. Normally, we don't cache anything in the RangeTblEntry that might change between plan time and execution time. Those things are normally stored in the RelOptInfo - why not do the same here? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
2011/9/26 Robert Haas <robertmhaas@gmail.com>: > On Mon, Sep 26, 2011 at 5:58 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >>> I think it is. If you create a view that involves an RTE, the node >>> tree is going to get stored in pg_rewrite.ev_action. And it's going >>> to include the security_barrier attribute, because you added outfuncs >>> support for it... >>> >>> No? >>> >> IIUC, nested views are also expanded when user's query gets rewritten. >> Thus, rte->security_barrier shall be set based on the latest configuration >> of the view. >> I injected an elog(NOTICE, ...) to confirm the behavior, when security_barrier >> flag was set on rte->security_barrier at ApplyRetrieveRule(). > > Hmm, OK. I am still not convinced that this is the right approach. > Normally, we don't cache anything in the RangeTblEntry that might > change between plan time and execution time. Those things are > normally stored in the RelOptInfo - why not do the same here? > The point is that a sub-query come from a particular view does not keep the information what view originally stored the sub-query when it was passed to the executor stage. PostgreSQL handles a view as just a sub-query after the rewriter stage. One possible idea not to store the flag in RangeTblEntry is to utilize rte->relid to show the relation-id of the source view, when rtekind is RTE_SUBQUERY; that enables to pull the security_barrier flag in executor stage. However, the interface to reference reloptions are designed to pull this information with Relation pointer, rather than lsyscache, so I implemented this revision with a new rte->security_barrier member. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Kohei KaiGai <kaigai@kaigai.gr.jp> writes: > However, the interface to reference reloptions are designed to pull > this information with Relation pointer, rather than lsyscache, so > I implemented this revision with a new rte->security_barrier member. This approach will guarantee that we can never implement an ALTER VIEW (or CREATE OR REPLACE VIEW) option that changes the state of the flag. I don't think that's a good idea. regards, tom lane
Kohei KaiGai <kaigai@kaigai.gr.jp> writes: > Sorry, are you saying the current (in other words, rte->security_barrier > stores the state of reloption) approach is not a good idea? Yes. I think the same as Robert: the way to handle this is to store it in RelOptInfo for the duration of planning, and pull it from the catalogs during planner startup (cf plancat.c). regards, tom lane
2011/9/26 Tom Lane <tgl@sss.pgh.pa.us>: > Kohei KaiGai <kaigai@kaigai.gr.jp> writes: >> However, the interface to reference reloptions are designed to pull >> this information with Relation pointer, rather than lsyscache, so >> I implemented this revision with a new rte->security_barrier member. > > This approach will guarantee that we can never implement an ALTER VIEW > (or CREATE OR REPLACE VIEW) option that changes the state of the flag. > I don't think that's a good idea. > Sorry, are you saying the current (in other words, rte->security_barrier stores the state of reloption) approach is not a good idea? Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
2011/9/26 Tom Lane <tgl@sss.pgh.pa.us>: > Kohei KaiGai <kaigai@kaigai.gr.jp> writes: >> Sorry, are you saying the current (in other words, rte->security_barrier >> stores the state of reloption) approach is not a good idea? > > Yes. I think the same as Robert: the way to handle this is to store it > in RelOptInfo for the duration of planning, and pull it from the > catalogs during planner startup (cf plancat.c). > Hmm. If so, it seems to me worthwhile to investigate an alternative approach that stores relation-id of the view on rte->relid if rtekind is RTE_SUBQUERY and pull the "security_barrier" flag from the catalog during planner stage. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > The Part-2 tries to tackles a leaky-view scenarios by functions with > very tiny cost > estimation value. It was same one we had discussed in the commitfest-1st. > It prevents to launch functions earlier than ones come from inside of views with > "security_barrier" option. > > The Part-3 tries to tackles a leaky-view scenarios by functions that references > one side of join loop. It prevents to distribute qualifiers including > functions without > "leakproof" attribute into relations across security-barrier. I took a little more of a look at this today. It has major problems. First, I get compiler warnings (which you might want to trap in the future by creating src/Makefile.custom with COPT=-Werror when compiling). Second, the regression tests fail on the select_views test. Third, it appears that the part2 patch works by adding an additional traversal of the entire query tree to standard_planner(). I don't think we want to add overhead to the common case where no security views are in use, or at least it had better be very small - so this doesn't seem acceptable to me. Here are some simple benchmarking with pgbench -S (scale factor 10, shared_buffers=400MB, MacBook Pro laptop) with and without this stack of patches. These aren't clear-cut enough to make me absolutely sure that this patch causes a noticeable performance regression, but I think it does, and I'm not at all sure that this is the worst case: results.kaigai.1:tps = 9359.908769 (including connections establishing) results.kaigai.1:tps = 9366.317857 (including connections establishing) results.kaigai.1:tps = 9413.593349 (including connections establishing) results.master.1:tps = 9444.494510 (including connections establishing) results.master.1:tps = 9400.486860 (including connections establishing) results.master.1:tps = 9472.220529 (including connections establishing) In the light of these problems, it doesn't seem worthwhile for me to spend any more time on this right now: it looks to me like this needs a lot more work before it can be considered for commit. I will mark it Waiting on Author for now, but I think Returned with Feedback might be more appropriate. This needs more than light cleanup; it needs much more rigorous testing, both as to correctness and performance, and at least a partial redesign. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Kohei KaiGai <kaigai@kaigai.gr.jp> writes: > One possible idea not to store the flag in RangeTblEntry is to utilize > rte->relid to show the relation-id of the source view, when rtekind is > RTE_SUBQUERY; that enables to pull the security_barrier flag in > executor stage. Maybe I'm confused here, but what does the executor need the information for? I thought this was a planner problem. regards, tom lane
2011/9/26 Tom Lane <tgl@sss.pgh.pa.us>: > Kohei KaiGai <kaigai@kaigai.gr.jp> writes: >> One possible idea not to store the flag in RangeTblEntry is to utilize >> rte->relid to show the relation-id of the source view, when rtekind is >> RTE_SUBQUERY; that enables to pull the security_barrier flag in >> executor stage. > > Maybe I'm confused here, but what does the executor need the information > for? I thought this was a planner problem. > Sorry, "planner" was what I wanted to say. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
2011/9/26 Robert Haas <robertmhaas@gmail.com>: > On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> The Part-2 tries to tackles a leaky-view scenarios by functions with >> very tiny cost >> estimation value. It was same one we had discussed in the commitfest-1st. >> It prevents to launch functions earlier than ones come from inside of views with >> "security_barrier" option. >> >> The Part-3 tries to tackles a leaky-view scenarios by functions that references >> one side of join loop. It prevents to distribute qualifiers including >> functions without >> "leakproof" attribute into relations across security-barrier. > > I took a little more of a look at this today. It has major problems. > > First, I get compiler warnings (which you might want to trap in the > future by creating src/Makefile.custom with COPT=-Werror when > compiling). > > Second, the regression tests fail on the select_views test. > > Third, it appears that the part2 patch works by adding an additional > traversal of the entire query tree to standard_planner(). I don't > think we want to add overhead to the common case where no security > views are in use, or at least it had better be very small - so this > doesn't seem acceptable to me. > The reason why I put a walker routine on the head of standard_planner() was that previous revision of this patch tracked strict depth of sub-queries, not a number of times to go through security barrier. The policy to count-up depth of qualifier was changed according to Noad's suggestion is commit-fest 1st, however, the suitable position to mark the depth value was kept. I'll try to revise the suitable position to track the depth value. It seems to me one candidate is pull_up_subqueries during its recursive call, because this patch originally set FromExpr->security_barrier here. In addition to the two points you mentioned above, I'll update this patch as follows: * Use CREATE [SECURITY] VIEW statement, instead of reloptions. the flag shall be stored within a new attribute of pg_class,and it shall be kept when an existing view getting replaced. * Utilize RangeTblEntry->relid, instead of rte->security_barrier, and the flag shall be pulled from the catalog on plannerstage. * Documentation and Regression test. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
I updated the patches according to the suggestion. And most of documentation and regression test changes are consolidated to the part-4 patch. * CREATE SECURITY VIEW statement, instead of reloptions. (Part-1) I added this new statement, instead of reloptions support on views. The "security_barrier" information gets being stored on pg_class.relissecbarrier field newly added. However, in my opinion, the previous implementation was simpler from the viewpoint of code, because it allows to utilize existing facilities to support reloptions. As long as we definitely inform users the reloptions are preserved even if a view is replaced, my preference is utilization of reloption... * RangeTblEntry->security_barrier has gone. The security_barrier field of RangeTblEntry was removed, and rte->relid also gets utilized to track what view was originally referenced instead of the sub-query. The rte->relid enables to pull "security_barrier" flag from the catalog, if and when a sub-query is originally referenced as a view. * Performance improvement I removed mark_qualifiers_depth that walks on whole of the Query tree from the head of standard_planner. Instead of this, I put this routine at pull_up_subqueries(). This design change enabled to avoid unneeded tree-walking when security view was not appeared in the query, because Expr nodes are initialized by 0 on makeNode(), so we don't need to mark the "depth" field by zero. The subquery_depth is incremented when we pull-up a sub-query that come from security view, and its initial value is 0. So, we don't need to mark depth unless we pull up a sub-query come from the security view. + /* + * Mark the sub-query depth of qualifiers to determine the original + * level of them, if necessary. Expr->depth is initialized to zero, + * so we don't need to walk on the expression tree, if security view + * was not used. + */ + if (subquery_depth > 0) + mark_qualifiers_depth(f->quals, subquery_depth); I have no idea why the patched version records better results, although it might be within the mergin of statistical errors. Could you reproduce it in your environment? - Xeon E5504 (2.00GHz), shared_buffers=512, scaling factor=10 - Using pgbench -S -c 4 -T 15 postgres * Git master + all the patches tps = 6414.525599 (excluding connections establishing) tps = 6422.960691 (excluding connections establishing) tps = 6239.301706 (excluding connections establishing) tps = 6406.008424 (excluding connections establishing) tps = 6361.722286 (excluding connections establishing) * Git master tps = 6141.622043 (excluding connections establishing) tps = 6243.385064 (excluding connections establishing) tps = 6266.548213 (excluding connections establishing) tps = 6020.004101 (excluding connections establishing) tps = 6210.104070 (excluding connections establishing) * Compiler warnning I fixed them using Makefile.custom. * Regression test I noticed there are two select_view*.out files in regtest/expected directory, and regress.diff compared results/select_view.out and expected/select_view_1.out. So, I copied my results/select_view.out to expected/select_view_1.out, then we have no unexpected regression test errors. However, it seems to me quite confusable. Thanks, 2011/9/27 Kohei KaiGai <kaigai@kaigai.gr.jp>: > 2011/9/26 Robert Haas <robertmhaas@gmail.com>: >> On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >>> The Part-2 tries to tackles a leaky-view scenarios by functions with >>> very tiny cost >>> estimation value. It was same one we had discussed in the commitfest-1st. >>> It prevents to launch functions earlier than ones come from inside of views with >>> "security_barrier" option. >>> >>> The Part-3 tries to tackles a leaky-view scenarios by functions that references >>> one side of join loop. It prevents to distribute qualifiers including >>> functions without >>> "leakproof" attribute into relations across security-barrier. >> >> I took a little more of a look at this today. It has major problems. >> >> First, I get compiler warnings (which you might want to trap in the >> future by creating src/Makefile.custom with COPT=-Werror when >> compiling). >> >> Second, the regression tests fail on the select_views test. >> >> Third, it appears that the part2 patch works by adding an additional >> traversal of the entire query tree to standard_planner(). I don't >> think we want to add overhead to the common case where no security >> views are in use, or at least it had better be very small - so this >> doesn't seem acceptable to me. >> > The reason why I put a walker routine on the head of standard_planner() > was that previous revision of this patch tracked strict depth of sub-queries, > not a number of times to go through security barrier. > The policy to count-up depth of qualifier was changed according to Noad's > suggestion is commit-fest 1st, however, the suitable position to mark the > depth value was kept. > I'll try to revise the suitable position to track the depth value. It seems to > me one candidate is pull_up_subqueries during its recursive call, because > this patch originally set FromExpr->security_barrier here. > > In addition to the two points you mentioned above, I'll update this patch > as follows: > * Use CREATE [SECURITY] VIEW statement, instead of reloptions. > the flag shall be stored within a new attribute of pg_class, and it shall > be kept when an existing view getting replaced. > > * Utilize RangeTblEntry->relid, instead of rte->security_barrier, and the > flag shall be pulled from the catalog on planner stage. > > * Documentation and Regression test. > > Thanks, > -- > KaiGai Kohei <kaigai@kaigai.gr.jp> > -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Вложения
On Sun, Sep 25, 2011 at 11:22:56PM -0400, Robert Haas wrote: > On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch <noah@leadboat.com> wrote: > > On Sun, Sep 25, 2011 at 11:22:03AM -0500, Kevin Grittner wrote: > >> Robert Haas ?09/25/11 10:58 AM >>> > >> > >> > I'm not sure we've been 100% consistent about that, since we > >> > previously made CREATE OR REPLACE LANGUAGE not replace the owner > >> > with the current user. > >> > >> I think we've been consistent in *not* changing security on an > >> object when it is replaced. > > > >> [CREATE OR REPLACE FUNCTION does not change proowner or proacl] > > > > Good point. ?C-O-R VIEW also preserves column default values. ?I believe we are > > consistent to the extent that everything possible to specify in each C-O-R > > statement gets replaced outright. ?The preserved characteristics *require* > > commands like GRANT, COMMENT and ALTER VIEW to set in the first place. > > > > The analogue I had in mind is SECURITY DEFINER, which C-O-R FUNCTION reverts to > > SECURITY INVOKER if it's not specified each time. ?That default is safe, though, > > while the proposed default of security_barrier=false is unsafe. > > Even though I normally take the opposite position, I still like the > idea of dedicated syntax for this feature. Not knowing what view > options we might end up with in the future, I hate having to decide on > what the general behavior ought to be. But it would be easy to decide > that CREATE SECURITY VIEW followed by CREATE OR REPLACE VIEW leaves > the security flag set; it would be consistent with what we're doing > with owner and acl information and wouldn't back us into any > unpleasant decisions down the road. I prefer the previous UI (WITH (security_barrier=...)) to this proposal, albeit for diffuse reasons. Both kinds of views can have the consequence of granting new access to data. One kind leaks tuples to untrustworthy code whenever it's convenient for performance, and the other does not. A "non-security view" would not mimic either of these objects; it would be a mere subquery macro. Using WITH (...) syntax attached to the CREATE VIEW command better evokes the similarity between the alternatives we're actually offering. I also find it mildly odd letting CREATE OR REPLACE VIEW update an object originating with CREATE SECURITY VIEW. Unqualified CREATE VIEW will retain no redeeming value apart from backward compatibility; new applications with any concern for database-level security should use only security_barrier=true and mark functions LEAKPROOF as needed. nm
2011/9/30 Noah Misch <noah@leadboat.com>: > On Sun, Sep 25, 2011 at 11:22:56PM -0400, Robert Haas wrote: >> On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch <noah@leadboat.com> wrote: >> > On Sun, Sep 25, 2011 at 11:22:03AM -0500, Kevin Grittner wrote: >> >> Robert Haas ?09/25/11 10:58 AM >>> >> >> >> >> > I'm not sure we've been 100% consistent about that, since we >> >> > previously made CREATE OR REPLACE LANGUAGE not replace the owner >> >> > with the current user. >> >> >> >> I think we've been consistent in *not* changing security on an >> >> object when it is replaced. >> > >> >> [CREATE OR REPLACE FUNCTION does not change proowner or proacl] >> > >> > Good point. ?C-O-R VIEW also preserves column default values. ?I believe we are >> > consistent to the extent that everything possible to specify in each C-O-R >> > statement gets replaced outright. ?The preserved characteristics *require* >> > commands like GRANT, COMMENT and ALTER VIEW to set in the first place. >> > >> > The analogue I had in mind is SECURITY DEFINER, which C-O-R FUNCTION reverts to >> > SECURITY INVOKER if it's not specified each time. ?That default is safe, though, >> > while the proposed default of security_barrier=false is unsafe. >> >> Even though I normally take the opposite position, I still like the >> idea of dedicated syntax for this feature. Not knowing what view >> options we might end up with in the future, I hate having to decide on >> what the general behavior ought to be. But it would be easy to decide >> that CREATE SECURITY VIEW followed by CREATE OR REPLACE VIEW leaves >> the security flag set; it would be consistent with what we're doing >> with owner and acl information and wouldn't back us into any >> unpleasant decisions down the road. > > I prefer the previous UI (WITH (security_barrier=...)) to this proposal, albeit > for diffuse reasons. Both kinds of views can have the consequence of granting > new access to data. One kind leaks tuples to untrustworthy code whenever it's > convenient for performance, and the other does not. A "non-security view" would > not mimic either of these objects; it would be a mere subquery macro. Using > WITH (...) syntax attached to the CREATE VIEW command better evokes the > similarity between the alternatives we're actually offering. I also find it > mildly odd letting CREATE OR REPLACE VIEW update an object originating with > CREATE SECURITY VIEW. > My preference is still also WITH(security_barrier=...) syntax. The arguable point was the behavior when a view is replaced without explicit WITH clause; whether we should consider it was specified a default value, or we should consider it means the option is preserved. If we stand on the viewpoint that object's attribute related to security (such as ownership, acl, label, ...) should be preserved, the security barrier also shall be preserved. On the other hand, we can never know what options will be added in the future, right now. Thus, we may need to sort out options related to security and not at DefineVirtualRelation(). However, do we need to limit type of the options to be preserved to security related? It is the first case that object with arbitrary options can be replaced. It seems to me we have no matter, even if we determine object's options are preserved unless an explicit new value is provided. Any other ideas? Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
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/AANLkTil1n2qWDD7IZlgBVt2IFL29rWfVkSseL9b9r2Fi@mail.gmail.com
2011/10/8 Noah Misch <noah@leadboat.com>: > On Sun, Oct 02, 2011 at 07:16:33PM +0200, Kohei KaiGai wrote: >> My preference is still also WITH(security_barrier=...) syntax. >> >> The arguable point was the behavior when a view is replaced without >> explicit WITH clause; >> whether we should consider it was specified a default value, or we >> should consider it means >> the option is preserved. >> If we stand on the viewpoint that object's attribute related to >> security (such as ownership, >> acl, label, ...) should be preserved, the security barrier also shall >> be preserved. >> On the other hand, we can never know what options will be added in the >> future, right now. >> Thus, we may need to sort out options related to security and not at >> DefineVirtualRelation(). >> >> However, do we need to limit type of the options to be preserved to >> security related? >> It is the first case that object with arbitrary options can be replaced. >> It seems to me we have no matter, even if we determine object's >> options are preserved >> unless an explicit new value is provided. > > Currently, you can predict how CREATE OR REPLACE affects a given object > characteristic with a simple rule: if the CREATE OR REPLACE statement can > specify a characteristic, we don't preserve its existing value. Otherwise, we > do preserve it. Let's not depart from that rule. > > Applying that rule to the proposed syntax, it shall not preserve the existing > security_barrier value. I think that is acceptable. If it's not acceptable, we > need a different syntax -- perhaps CREATE SECURITY VIEW. > No. It also preserves the security-barrier flag, when we replace a view without SECURITY option. The only difference is that we have no way to turn off security-barrier flag explicitly, right now. The major reason why I prefer reloptions rather than "SECURITY" option is that allows to reuse the existing capability to store a property of relation. It seems to me both of syntax enables to achieve the rule to preserve flags when a view is replaced. >> Any other ideas? > > Suppose we permitted pushdown of unsafe predicates when the user can read the > involved columns anyway, a generalization of the idea from the first paragraph > of [1]. Would that, along with LEAKPROOF, provide enough strategies for shoring > up performance to justify removing unsafe views entirely? > The problem was that we do all the access control decision at the executor stage, but planner has to make a plan prior to execution. So, it was also reason why we have tried to add LEAKPROOF flag to functions. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On Sat, Oct 08, 2011 at 09:11:08AM +0200, Kohei KaiGai wrote: > 2011/10/8 Noah Misch <noah@leadboat.com>: > > On Sun, Oct 02, 2011 at 07:16:33PM +0200, Kohei KaiGai wrote: > >> My preference is still also WITH(security_barrier=...) syntax. > >> > >> The arguable point was the behavior when a view is replaced without > >> explicit WITH clause; > >> whether we should consider it was specified a default value, or we > >> should consider it means > >> the option is preserved. > >> If we stand on the viewpoint that object's attribute related to > >> security (such as ownership, > >> acl, label, ...) should be preserved, the security barrier also shall > >> be preserved. > >> On the other hand, we can never know what options will be added in the > >> future, right now. > >> Thus, we may need to sort out options related to security and not at > >> DefineVirtualRelation(). > >> > >> However, do we need to limit type of the options to be preserved to > >> security related? > >> It is the first case that object with arbitrary options can be replaced. > >> It seems to me we have no matter, even if we determine object's > >> options are preserved > >> unless an explicit new value is provided. > > > > Currently, you can predict how CREATE OR REPLACE affects a given object > > characteristic with a simple rule: if the CREATE OR REPLACE statement can > > specify a characteristic, we don't preserve its existing value. ?Otherwise, we > > do preserve it. ?Let's not depart from that rule. > > > > Applying that rule to the proposed syntax, it shall not preserve the existing > > security_barrier value. ?I think that is acceptable. ?If it's not acceptable, we > > need a different syntax -- perhaps CREATE SECURITY VIEW. > > > No. It also preserves the security-barrier flag, when we replace a view without > SECURITY option. The only difference is that we have no way to turn off > security-barrier flag explicitly, right now. > > The major reason why I prefer reloptions rather than "SECURITY" option is > that allows to reuse the existing capability to store a property of relation. > It seems to me both of syntax enables to achieve the rule to preserve flags > when a view is replaced. Yes, there are no technical barriers to implementing either behavior with either syntax. However, CREATE OR REPLACE VIEW ... WITH (...) has a precedent guiding its behavior: if a CREATE OR REPLACE statement can specify a characteristic, we don't preserve its existing value. > >> Any other ideas? > > > > Suppose we permitted pushdown of unsafe predicates when the user can read the > > involved columns anyway, a generalization of the idea from the first paragraph > > of [1]. ?Would that, along with LEAKPROOF, provide enough strategies for shoring > > up performance to justify removing unsafe views entirely? > > > The problem was that we do all the access control decision at the > executor stage, > but planner has to make a plan prior to execution. So, it was also reason why we > have tried to add LEAKPROOF flag to functions. Yes; we'd need to invalidate relevant plans in response to anything that changes access control decisions. GRANT and ALTER ... OWNER TO already do that, but we'd need to cover pg_authid/pg_auth_members changes, SET ROLE, SET SESSION AUTHORIZATION, and probably a few other things. That might be a substantial project in its own right.
2011/10/8 Noah Misch <noah@leadboat.com>: > On Sat, Oct 08, 2011 at 09:11:08AM +0200, Kohei KaiGai wrote: >> 2011/10/8 Noah Misch <noah@leadboat.com>: >> > On Sun, Oct 02, 2011 at 07:16:33PM +0200, Kohei KaiGai wrote: >> >> My preference is still also WITH(security_barrier=...) syntax. >> >> >> >> The arguable point was the behavior when a view is replaced without >> >> explicit WITH clause; >> >> whether we should consider it was specified a default value, or we >> >> should consider it means >> >> the option is preserved. >> >> If we stand on the viewpoint that object's attribute related to >> >> security (such as ownership, >> >> acl, label, ...) should be preserved, the security barrier also shall >> >> be preserved. >> >> On the other hand, we can never know what options will be added in the >> >> future, right now. >> >> Thus, we may need to sort out options related to security and not at >> >> DefineVirtualRelation(). >> >> >> >> However, do we need to limit type of the options to be preserved to >> >> security related? >> >> It is the first case that object with arbitrary options can be replaced. >> >> It seems to me we have no matter, even if we determine object's >> >> options are preserved >> >> unless an explicit new value is provided. >> > >> > Currently, you can predict how CREATE OR REPLACE affects a given object >> > characteristic with a simple rule: if the CREATE OR REPLACE statement can >> > specify a characteristic, we don't preserve its existing value. ?Otherwise, we >> > do preserve it. ?Let's not depart from that rule. >> > >> > Applying that rule to the proposed syntax, it shall not preserve the existing >> > security_barrier value. ?I think that is acceptable. ?If it's not acceptable, we >> > need a different syntax -- perhaps CREATE SECURITY VIEW. >> > >> No. It also preserves the security-barrier flag, when we replace a view without >> SECURITY option. The only difference is that we have no way to turn off >> security-barrier flag explicitly, right now. >> >> The major reason why I prefer reloptions rather than "SECURITY" option is >> that allows to reuse the existing capability to store a property of relation. >> It seems to me both of syntax enables to achieve the rule to preserve flags >> when a view is replaced. > > Yes, there are no technical barriers to implementing either behavior with either > syntax. However, CREATE OR REPLACE VIEW ... WITH (...) has a precedent guiding > its behavior: if a CREATE OR REPLACE statement can specify a characteristic, we > don't preserve its existing value. > 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. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Вложения
On Sun, Oct 9, 2011 at 11:50 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > I tried to refactor the patches based on the interface of WITH (...) > and usage of > pg_class.reloptions, although here is no functionality changes; including the > behavior when a view is replaced. > > My preference is WITH (...) interface, however, it is not a strong one. > So, I hope either of versions being reviewed. I spent some more time looking at this, and I guess I'm pretty unsold on the whole approach. In the part 2 patch, for example, we're doing this: +static bool +mark_qualifiers_depth_walker(Node *node, void *context) +{ + int depth = *((int *)(context)); + + if (node == NULL) + return false; + + if (IsA(node, FuncExpr)) + { + ((FuncExpr *)node)->depth = depth; + } + else if (IsA(node, OpExpr)) + { + ((OpExpr *)node)->depth = depth; + } + else if (IsA(node, DistinctExpr)) + { + ((DistinctExpr *)node)->depth = depth; + } + else if (IsA(node, ScalarArrayOpExpr)) + { + ((ScalarArrayOpExpr *)node)->depth = depth; + } + else if (IsA(node, CoerceViaIO)) + { + ((CoerceViaIO *)node)->depth = depth; + } + else if (IsA(node, ArrayCoerceExpr)) + { + ((ArrayCoerceExpr *)node)->depth = depth; + } + else if (IsA(node, NullIfExpr)) + { + ((NullIfExpr *)node)->depth = depth; + } + else if (IsA(node, RowCompareExpr)) + { + ((RowCompareExpr *)node)->depth = depth; + } + return expression_tree_walker(node, mark_qualifiers_depth_walker, context); +} It seems really ugly to me to suppose that we need to add a depth field to every single one of these node types. If you've missed one, then we have a security hole. If someone else adds another node type later that requires this field and doesn't add it, we have a security hole. And since all of these depth fields are going to make their way into stored rules, those security holes will require an initdb to fix.Ouch! And what happens if the security view becomesa 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,10000) g; INSERT 0 10000 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.00000000000000000000000000000000000000001; CREATE FUNCTION rhaas=# explain analyze select * from window_on_secret; QUERY PLAN ---------------------------------------------------------------------------------------------------Seq Scan on secret (cost=0.00..209.00rows=1 width=39) (actual time=0.022..2.758 rows=1 loops=1) Filter: (a = 1) Rows Removed by Filter: 9999Total 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); QUERYPLAN -----------------------------------------------------------------------------------------------------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: 9999Total runtime: 126.565ms (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 1000000000; CREATE VIEW rhaas=# explain analyze select * from window_on_secret where snarf(a, b); QUERY PLAN ------------------------------------------------------------------------------------------------------------------Subquery Scanon window_on_secret (cost=0.00..209.01 rows=1 width=39) (actual time=0.320..2.348 rows=1 loops=1) Filter: snarf(window_on_secret.a, window_on_secret.b) -> Limit (cost=0.00..209.00rows=1 width=39) (actual time=0.016..2.043 rows=1 loops=1) -> Seq Scan on secret (cost=0.00..209.00 rows=1 width=39) (actual time=0.014..2.034 rows=1 loops=1) Filter: (a = 1) Rows Removed by Filter: 9999Total runtime:2.434 ms (7 rows) rhaas=# select * from leak;a | b ---+----------------------------------1 | 0.60352857504040.928101760800928 (1 row) 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: rhaas=# explain analyze select * from window_on_secret where b = 'not so hot'; QUERY PLAN ------------------------------------------------------------------------------------------------------------------Subquery Scanon window_on_secret (cost=0.00..209.01 rows=1 width=39) (actual time=2.080..2.080 rows=0 loops=1) Filter: (window_on_secret.b = 'not so hot'::text) Rows Removed by Filter:1 -> Limit (cost=0.00..209.00 rows=1 width=39) (actual time=0.014..2.077 rows=1 loops=1) -> Seq Scan on secret (cost=0.00..209.00 rows=1 width=39) (actual time=0.013..2.075 rows=1 loops=1) Filter: (a = 1) Rows Removed by Filter: 9999Total runtime:2.131 ms (8 rows) I don't have a clear idea what to do about that, and maybe it's an intractable problem, but I feel like once we've flattened the subquery, it's a whole lot harder to prevent bad stuff from happening, because now the bits that started inside the security view are all over the place. Somebody previously raised the issue of what happen when there are multiple security views involved in the same query. I don't see how the depth-based approach can possibly deal with that case correctly. Let's suppose that in the test case above, window_on_secret was created as a security view. Now, the bad guy comes along and creates a security view that uses with some maliciously crafted qual, and then joins that view against window_on_secret. IIUC, the quals from both views are going to have depth = 1, so from the planner's point of view it ought to be OK to interchange them - which it's not. Now, in the normal course of events it won't matter, because the quals in window_on_secret are going to apply to the "secret" table, and the quals in the other view are going to apply only to whatever view that table ranges over. But there's already at least one case in which that might not be true: if the equivalence class machinery throws a constant into the same bucket as a column in window_on_secret, it will feel free to add a qual comparing that value to the associated constant using the appropriate operator, and that qual could then (presumably) get reordered with the qual from the security view. I'm not 100% sure that it's possible to construct a security breach this way, but I'm definitely not 100% sure that it isn't. And future changes to the way we make deductions based on equivalence classes (like deducing implied equalities, something that's been requested more than once) could open up further possibilities for mischief. We could maybe hunt all of those down but it seems rather error-prone to me, and not very future-proof. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
2011/10/10 Robert Haas <robertmhaas@gmail.com>: > On Sun, Oct 9, 2011 at 11:50 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> I tried to refactor the patches based on the interface of WITH (...) >> and usage of >> pg_class.reloptions, although here is no functionality changes; including the >> behavior when a view is replaced. >> >> My preference is WITH (...) interface, however, it is not a strong one. >> So, I hope either of versions being reviewed. > > I spent some more time looking at this, and I guess I'm pretty unsold > on the whole approach. In the part 2 patch, for example, we're doing > this: > > +static bool > +mark_qualifiers_depth_walker(Node *node, void *context) > +{ > + int depth = *((int *)(context)); > + ... <snip> ... > + else if (IsA(node, RowCompareExpr)) > + { > + ((RowCompareExpr *)node)->depth = depth; > + } > + return expression_tree_walker(node, > mark_qualifiers_depth_walker, context); > +} > > It seems really ugly to me to suppose that we need to add a depth > field to every single one of these node types. If you've missed one, > then we have a security hole. If someone else adds another node type > later that requires this field and doesn't add it, we have a security > hole. And since all of these depth fields are going to make their way > into stored rules, those security holes will require an initdb to fix. > Indeed, I have to admit this disadvantage from the perspective of code maintenance, because it had also been a tough work for me to track the depth field in this patch. > If we make security views work like this, then we don't need to have > one mechanism to sort quals by depth and another to prevent them from > being pushed down through joins. It all just works. Now, there is > one problem: if snarf() were a non-leaky function rather than a > maliciously crafted one, it still wouldn't get pushed down: > Rather than my original design, I'm learning to the idea to keep sub-queries come from security views; without flatten, because of its straightforwardness. > If we make security views work like this, then we don't need to have > one mechanism to sort quals by depth and another to prevent them from > being pushed down through joins. It all just works. Now, there is > one problem: if snarf() were a non-leaky function rather than a > maliciously crafted one, it still wouldn't get pushed down: > I agreed. We have been on the standpoint that tries to prevent leakable functions to reference a portion of join-tree being already flatten, however, it has been a tough work. It seems to me it is much simple approach that enables to push down only non-leaky functions into inside of sub-queries. An idea is to add a hack on distribute_qual_to_rels() to relocate a qualifier into inside of the sub-query, when it references only a particular sub-query being come from a security view, and when the sub-query satisfies is_simple_subquery(), for example. Anyway, I'll try to tackle this long standing problem with this approach in the next commit-fest. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
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.GB21098@tornado.leadboat.com These patches were not context diffs. Thanks, nm
On 10 October 2011 21:28, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > 2011/10/10 Robert Haas <robertmhaas@gmail.com>: >> It seems really ugly to me to suppose that we need to add a depth >> field to every single one of these node types. If you've missed one, >> then we have a security hole. If someone else adds another node type >> later that requires this field and doesn't add it, we have a security >> hole. And since all of these depth fields are going to make their way >> into stored rules, those security holes will require an initdb to fix. >> > Indeed, I have to admit this disadvantage from the perspective of code > maintenance, because it had also been a tough work for me to track > the depth field in this patch. Would you consider putting the depth field directly into a generic superclass node, such as the "Expr" node? Perhaps that approach would be neater. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services
On Mon, Oct 10, 2011 at 4:28 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > I agreed. We have been on the standpoint that tries to prevent > leakable functions to reference a portion of join-tree being already > flatten, however, it has been a tough work. > It seems to me it is much simple approach that enables to push > down only non-leaky functions into inside of sub-queries. > > An idea is to add a hack on distribute_qual_to_rels() to relocate > a qualifier into inside of the sub-query, when it references only > a particular sub-query being come from a security view, and > when the sub-query satisfies is_simple_subquery(), for example. If you can make this work, I think it could be a pretty sweet plannner optimization even apart from the implications for security views. Consider a query of this form: A LEFT JOIN B LEFT JOIN C where B is a view defined as: B1 JOIN B2 JOIN B3 LEFT JOIN B4 LEFT JOIN B5 Now let's suppose that from_collapse_limit/join_collapse_limit are set low enough that we decline to fold these subproblems together. If there happens to be a qual B.x = 1, where B.x is really B1.x, then the generated plan sucks, because it will basically lose the ability to filter B1 early, very possibly on, say, a unique index. Or at least a highly selective index. If we could allow the B.x qual to trickle down inside of the subquery, we'd get a much better plan. Of course, it's still not as good as flattening, because it won't allow us to consider as many possible join orders - but the whole point of having from_collapse_limit/join_collapse_limit in the first place is that we can't consider all the join orders without having planning time and memory usage balloon wildly out of control. And in many real-world cases, I think that this would probably mitigate the effects of exceeding from_collapse_limit/join_collapse_limit quite a bit. In order to make it work, though, you'd need to arrange things so that we distribute quals to rels in the parent query, then let some of them filter down into the subquery, then distribute quals to rels in the subquery (possibly adjusting RTE indexes?), then finish planning the subquery, then finish planning the parent query. Not sure how possible/straightforward that is. It's probably a good idea to deal with this part first, because if you can't make it work then the whole approach is in trouble. I'm almost imagining that we could break this into three independent patches, like this: 1. Let quals percolate down into subqueries. 2. Add the notion of a security view, which prevents flattening and disables the optimization of patch #1 3. Add the notion of a leakproof function, which can benefit from the optimization of #1 even when the view involved is a security view as introduced in #2 Unlike the way you have it now, I think those patches could be independently committable. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
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); QUERYPLAN ------------------------------------------------------------------------------------Merge Join (cost=381.80..9597.97 rows=586624width=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 -> SeqScan 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.56rows=15510 width=44) -> Merge Join (cost=210.95..489.78 rows=15510 width=44) MergeCond: (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=1230width=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 <robertmhaas@gmail.com>: > On Mon, Oct 10, 2011 at 4:28 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> I agreed. We have been on the standpoint that tries to prevent >> leakable functions to reference a portion of join-tree being already >> flatten, however, it has been a tough work. >> It seems to me it is much simple approach that enables to push >> down only non-leaky functions into inside of sub-queries. >> >> An idea is to add a hack on distribute_qual_to_rels() to relocate >> a qualifier into inside of the sub-query, when it references only >> a particular sub-query being come from a security view, and >> when the sub-query satisfies is_simple_subquery(), for example. > > If you can make this work, I think it could be a pretty sweet plannner > optimization even apart from the implications for security views. > Consider a query of this form: > > A LEFT JOIN B LEFT JOIN C > > where B is a view defined as: > > B1 JOIN B2 JOIN B3 LEFT JOIN B4 LEFT JOIN B5 > > Now let's suppose that from_collapse_limit/join_collapse_limit are set > low enough that we decline to fold these subproblems together. If > there happens to be a qual B.x = 1, where B.x is really B1.x, then the > generated plan sucks, because it will basically lose the ability to > filter B1 early, very possibly on, say, a unique index. Or at least a > highly selective index. If we could allow the B.x qual to trickle > down inside of the subquery, we'd get a much better plan. Of course, > it's still not as good as flattening, because it won't allow us to > consider as many possible join orders - but the whole point of having > from_collapse_limit/join_collapse_limit in the first place is that we > can't consider all the join orders without having planning time and > memory usage balloon wildly out of control. And in many real-world > cases, I think that this would probably mitigate the effects of > exceeding from_collapse_limit/join_collapse_limit quite a bit. > > In order to make it work, though, you'd need to arrange things so that > we distribute quals to rels in the parent query, then let some of them > filter down into the subquery, then distribute quals to rels in the > subquery (possibly adjusting RTE indexes?), then finish planning the > subquery, then finish planning the parent query. Not sure how > possible/straightforward that is. > > It's probably a good idea to deal with this part first, because if you > can't make it work then the whole approach is in trouble. I'm almost > imagining that we could break this into three independent patches, > like this: > > 1. Let quals percolate down into subqueries. > 2. Add the notion of a security view, which prevents flattening and > disables the optimization of patch #1 > 3. Add the notion of a leakproof function, which can benefit from the > optimization of #1 even when the view involved is a security view as > introduced in #2 > > Unlike the way you have it now, I think those patches could be > independently committable. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On Sun, Oct 16, 2011 at 4:46 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > Hi Robert, > > I'm a bit confusing about this sentence. > >> If you can make this work, I think it could be a pretty sweet plannner >> optimization even apart from the implications for security views. >> Consider a query of this form: >> >> A LEFT JOIN B LEFT JOIN C >> >> where B is a view defined as: >> >> B1 JOIN B2 JOIN B3 LEFT JOIN B4 LEFT JOIN B5 >> >> Now let's suppose that from_collapse_limit/join_collapse_limit are set >> low enough that we decline to fold these subproblems together. If >> there happens to be a qual B.x = 1, where B.x is really B1.x, then the >> generated plan sucks, because it will basically lose the ability to >> filter B1 early, very possibly on, say, a unique index. Or at least a >> highly selective index. >> > > I tried to reproduce the scenario with enough small from/join_collapse_limit > (typically 1), but it allows to push down qualifiers into the least scan plan. Hmm, you're right. LIMIT 1000000000 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
Robert Haas <robertmhaas@gmail.com> writes: > On Sun, Oct 16, 2011 at 4:46 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> I tried to reproduce the scenario with enough small from/join_collapse_limit >> (typically 1), but it allows to push down qualifiers into the least scan plan. > Hmm, you're right. LIMIT 1000000000 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
2011/10/19 Tom Lane <tgl@sss.pgh.pa.us>: > Robert Haas <robertmhaas@gmail.com> writes: >> On Sun, Oct 16, 2011 at 4:46 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >>> I tried to reproduce the scenario with enough small from/join_collapse_limit >>> (typically 1), but it allows to push down qualifiers into the least scan plan. > >> Hmm, you're right. LIMIT 1000000000 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 <kaigai@kaigai.gr.jp>
On Wed, Oct 19, 2011 at 6:35 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > 2011/10/19 Tom Lane <tgl@sss.pgh.pa.us>: >> Robert Haas <robertmhaas@gmail.com> writes: >>> On Sun, Oct 16, 2011 at 4:46 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >>>> I tried to reproduce the scenario with enough small from/join_collapse_limit >>>> (typically 1), but it allows to push down qualifiers into the least scan plan. >> >>> Hmm, you're right. LIMIT 1000000000 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
Robert Haas <robertmhaas@gmail.com> writes: > Well, there's clearly some way to prevent pushdown from happening, > because sticking a LIMIT in there does the trick... I already pointed you at subquery_is_pushdown_safe ... regards, tom lane
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 <tgl@sss.pgh.pa.us>: > Robert Haas <robertmhaas@gmail.com> writes: >> Well, there's clearly some way to prevent pushdown from happening, >> because sticking a LIMIT in there does the trick... > > I already pointed you at subquery_is_pushdown_safe ... > > regards, tom lane > -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On Fri, Oct 21, 2011 at 10:36 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > So, I will split the patch into two parts as follows, in the next commit fest. > > Part-1) Views with security_barrier reloption > > The part-1 portion provides views "security_barrier" reloption; that enables > to keep sub-queries unflatten in the prepjoin.c stage. > In addition, these sub-queries (that originally come from views with > "security_barrier" option) don't allow to push down qualifiers from upper > level. It shall prevent both of the problematic scenarios. > > Part-2) Functions with leakproof attribute > > The part-2 portion provides functions "leakproof" attribute; that enables > to push down leakproof functions into sub-queries, even if it originally > come from security views. > It shall minimize performance damages when we use view for row-level > security purpose. Sounds reasonable. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
The attached patches are fixes to the leaky-view problem; a prerequisite to implement row-level security; that consists of two portion. Part-1) It adds WITH(options...) clause on view definition, and disallow to make sub-queries flatten, if this sub-query is originated a particular view with "security_barrier" reloption. In addition, it also disallow to push-down qualifiers across security-barrier, thus, we will have a way to guarantee order to launch qualifiers; that has been headache for us to achieve row-level security using view (or possibly similar feature). Part-2) It adds "leakproof" attribute to functions; that means functions are obviously leakproof to the supplied arguments, and only superuser can set. If a qualifier is consists of functions with "leakproof" only, the query planner handles it as an exception of the security-barrier. A typical case is WHERE x = 100; that shall promote the given scan plan from sequential to index in many cases. It requires the part-1 being applied prior to this patch, and compressed by gzip due to the size of patch (mostly pg_proc.h). The following examples shows how these features works: postgres=# CREATE VIEW v1 AS SELECT * FROM t1 WHERE a % 2 =0; CREATE VIEW postgres=# CREATE VIEW v1s WITH (security_barrier) AS SELECT * FROM t1 WHERE a % 2 =0; CREATE VIEW postgres=# CREATE VIEW v2 AS SELECT * FROM t1 JOIN t2 ON a = x WHERE a % 2 = 0; CREATE VIEW postgres=# CREATE VIEW v2s WITH (security_barrier) AS SELECT * FROM t1 JOIN t2 ON a = x W HERE a % 2 = 0; CREATE VIEW postgres=# CREATE OR REPLACE FUNCTION f_leak(text) RETURNS bool LANGUAGE plpgsql COST 0.0001 AS 'BEGIN RAISE notice ''f_leak => %'', $1; RETURN true; END';CREATE FUNCTION Without security_barrier ------------------------------------ postgres=# SELECT * FROM v1 WHERE f_leak(b);NOTICE: f_leak => aaaNOTICE: f_leak => bbbNOTICE: f_leak => cccNOTICE: f_leak => ddd a | b---+----- 2 | bbb 4 | ddd(2 rows) postgres=# EXPLAIN SELECT * FROM v1 WHERE f_leak(b); QUERY PLAN ---------------------------------------------------- Seq Scan on t1 (cost=0.00..28.45 rows=2 width=36) Filter: (f_leak(b) AND ((a % 2) = 0)) (2 rows) postgres=# SELECT * FROM v2 WHERE f_leak(y); NOTICE: f_leak => xxx NOTICE: f_leak => yyy NOTICE: f_leak => zzz NOTICE: f_leak => xyz a | b | x | y ---+-----+---+----- 2 | bbb | 2 | xxx 4 | ddd | 4 | zzz (2 rows) postgres=# EXPLAIN SELECT * FROM v2 WHERE f_leak(y); QUERY PLAN ---------------------------------------------------------------- Hash Join (cost=28.52..52.38 rows=2 width=72) Hash Cond: (t2.x = t1.a) -> Seq Scan on t2 (cost=0.00..22.30 rows=410 width=36) Filter: f_leak(y) -> Hash (cost=28.45..28.45 rows=6 width=36) -> Seq Scan on t1 (cost=0.00..28.45 rows=6 width=36) Filter: ((a % 2) = 0) (7 rows) With security_barrier ------------------------------- postgres=# SELECT * FROM v1s WHERE f_leak(b);NOTICE: f_leak => bbbNOTICE: f_leak => ddd a | b---+----- 2 | bbb 4 | ddd(2 rows) postgres=# EXPLAIN SELECT * FROM v1s WHERE f_leak(b); QUERY PLAN ---------------------------------------------------------- Subquery Scan on v1s (cost=0.00..28.51 rows=2 width=36) Filter: f_leak(v1s.b) -> Seq Scan on t1 (cost=0.00..28.45 rows=6 width=36) Filter: ((a % 2) = 0) (4 rows) postgres=# SELECT * FROM v2s WHERE f_leak(y); NOTICE: f_leak => xxx NOTICE: f_leak => zzz a | b | x | y ---+-----+---+----- 2 | bbb | 2 | xxx 4 | ddd | 4 | zzz (2 rows) postgres=# EXPLAIN SELECT * FROM v2s WHERE f_leak(y); QUERY PLAN ---------------------------------------------------------------------- Subquery Scan on v2s (cost=28.52..55.56 rows=2 width=72) Filter: f_leak(v2s.y) -> Hash Join (cost=28.52..55.50 rows=6 width=72) Hash Cond: (t2.x = t1.a) -> Seq Scan on t2 (cost=0.00..22.30 rows=1230 width=36) -> Hash (cost=28.45..28.45 rows=6 width=36) -> Seq Scan on t1 (cost=0.00..28.45 rows=6 width=36) Filter: ((a % 2) = 0) (8 rows) Leakproof function is exceptionally allowed to be pushed down ------------------------------------------------------------------------------------------ postgres=# SELECT * FROM v2s WHERE f_leak(y) AND a = 2; NOTICE: f_leak => xxx a | b | x | y ---+-----+---+----- 2 | bbb | 2 | xxx (1 row) (*) int4eq is set as a leakproof function in the default. postgres=# EXPLAIN SELECT * FROM v2s WHERE f_leak(y) AND a = 2; QUERY PLAN ------------------------------------------------------------------------------- Subquery Scan on v2s (cost=0.00..16.56 rows=1 width=72) Filter: f_leak(v2s.y) -> Nested Loop (cost=0.00..16.55 rows=1 width=72) -> Index Scan using t1_pkey on t1 (cost=0.00..8.27 rows=1 width=36) Index Cond: (a = 2) Filter: ((a % 2) = 0) -> Index Scan using t2_pkey on t2 (cost=0.00..8.27 rows=1 width=36) Index Cond: (x = 2) (8 rows) Thanks, 2011/10/21 Robert Haas <robertmhaas@gmail.com>: > On Fri, Oct 21, 2011 at 10:36 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> So, I will split the patch into two parts as follows, in the next commit fest. >> >> Part-1) Views with security_barrier reloption >> >> The part-1 portion provides views "security_barrier" reloption; that enables >> to keep sub-queries unflatten in the prepjoin.c stage. >> In addition, these sub-queries (that originally come from views with >> "security_barrier" option) don't allow to push down qualifiers from upper >> level. It shall prevent both of the problematic scenarios. >> >> Part-2) Functions with leakproof attribute >> >> The part-2 portion provides functions "leakproof" attribute; that enables >> to push down leakproof functions into sub-queries, even if it originally >> come from security views. >> It shall minimize performance damages when we use view for row-level >> security purpose. > > Sounds reasonable. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Вложения
On Wed, Nov 2, 2011 at 7:34 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > [ new patch, with example query plans ] I like the look of those query plans. Redefining the RangeTblEntry's relid field to be valid for either a table or a subquery that originated from a view seems problematic to me, though. For one thing, it's hard to say how much other code assumes that field to be valid only for a table. For example, you didn't update _readRangeTblEntry(), and I wouldn't bet on that being the only place that needs fixing. For another thing, instead of changing the meaning of the relid field, you could just leave that alone and instead add a "bool security_barrier field" that caches the answer; ApplyRetrieveRule() has the Relation object and could set that field appropriately, and then subquery_was_security_barrier() wouldn't need a syscache lookup. Now, the obvious objection is that the security-barrier attribute might change between the time the RTE is created and the time that it gets used. But if that's a danger, then presumably the whole view could also change, in which case the Query object would be pointing to the wrong data anyway. I'm not sure I fully understand the details here, but it seems like it ought to be safe to cache the security_barrier attribute any place it's safe to cache the Query itself. It certainly doesn't seem right to think that we might end up using a new value of the security_barrier attribute with an old query, or the other way around. So something seems funky here. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
2011/11/2 Robert Haas <robertmhaas@gmail.com>: > On Wed, Nov 2, 2011 at 7:34 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> [ new patch, with example query plans ] > > I like the look of those query plans. > > Redefining the RangeTblEntry's relid field to be valid for either a > table or a subquery that originated from a view seems problematic to > me, though. For one thing, it's hard to say how much other code > assumes that field to be valid only for a table. For example, you > didn't update _readRangeTblEntry(), and I wouldn't bet on that being > the only place that needs fixing. For another thing, instead of > changing the meaning of the relid field, you could just leave that > alone and instead add a "bool security_barrier field" that caches the > answer; ApplyRetrieveRule() has the Relation object and could set that > field appropriately, and then subquery_was_security_barrier() wouldn't > need a syscache lookup. > > Now, the obvious objection is that the security-barrier attribute > might change between the time the RTE is created and the time that it > gets used. But if that's a danger, then presumably the whole view > could also change, in which case the Query object would be pointing to > the wrong data anyway. I'm not sure I fully understand the details > here, but it seems like it ought to be safe to cache the > security_barrier attribute any place it's safe to cache the Query > itself. It certainly doesn't seem right to think that we might end up > using a new value of the security_barrier attribute with an old query, > or the other way around. So something seems funky here. > The reason why I redefined the relid of RangeTblEntry is to avoid the problem when security_barrier attribute get changed by concurrent transactions between rewriter and planenr stage. Of course, I'm not 100% sure whether we have a routine that assumes valid relid of RangeTblEntry is regular table, or not, although we could run the regression test correctly. As I examined before, updates of the issued pg_class shall invalidate prepared statements that assumed a particular security_barrier (maybe, PlanCacheRelCallback does this work?), so it is unavailable to use old plans based on old view definition. If we want to avoid syscache lookup on subquery_was_security_barrier(), I think it is a feasible idea to hold the value of security_barrier within RTE. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Kohei KaiGai <kaigai@kaigai.gr.jp> writes: > The reason why I redefined the relid of RangeTblEntry is to avoid > the problem when security_barrier attribute get changed by concurrent > transactions between rewriter and planenr stage. This is complete nonsense. If the information is being injected into the querytree by the rewriter, it's sufficient to assume that it's up to date. Were it not so, we'd have problems with CREATE OR REPLACE RULE, too. regards, tom lane
2011/11/2 Tom Lane <tgl@sss.pgh.pa.us>: > Kohei KaiGai <kaigai@kaigai.gr.jp> writes: >> The reason why I redefined the relid of RangeTblEntry is to avoid >> the problem when security_barrier attribute get changed by concurrent >> transactions between rewriter and planenr stage. > > This is complete nonsense. If the information is being injected into > the querytree by the rewriter, it's sufficient to assume that it's up to > date. Were it not so, we'd have problems with CREATE OR REPLACE RULE, > too. > I revised the patches to revert redefinition in relid of RangeTblEntry, and add a flag of "security_barrier". I seems to work fine, even if view's property was changed between rewriter and planner stage. postgres=# CREATE VIEW v1 WITH (security_barrier) AS SELECT * FROM t1 WHERE a % 2 = 0; CREATE VIEW postgres=# PREPARE p1 AS SELECT * FROM v1 WHERE f_leak(b); PREPARE postgres=# EXECUTE p1; NOTICE: f_leak => bbb NOTICE: f_leak => ddd a | b ---+----- 2 | bbb 4 | ddd (2 rows) postgres=# ALTER VIEW v1 SET (security_barrier=false); ALTER VIEW postgres=# EXECUTE p1; NOTICE: f_leak => aaa NOTICE: f_leak => bbb NOTICE: f_leak => ccc NOTICE: f_leak => ddd NOTICE: f_leak => eee a | b ---+----- 2 | bbb 4 | ddd (2 rows) postgres=# ALTER VIEW v1 SET (security_barrier=true); ALTER VIEW postgres=# EXECUTE p1; NOTICE: f_leak => bbb NOTICE: f_leak => ddd a | b ---+----- 2 | bbb 4 | ddd (2 rows) Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Вложения
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 <kaigai@kaigai.gr.jp>: > 2011/11/2 Tom Lane <tgl@sss.pgh.pa.us>: >> Kohei KaiGai <kaigai@kaigai.gr.jp> writes: >>> The reason why I redefined the relid of RangeTblEntry is to avoid >>> the problem when security_barrier attribute get changed by concurrent >>> transactions between rewriter and planenr stage. >> >> This is complete nonsense. If the information is being injected into >> the querytree by the rewriter, it's sufficient to assume that it's up to >> date. Were it not so, we'd have problems with CREATE OR REPLACE RULE, >> too. >> > I revised the patches to revert redefinition in relid of RangeTblEntry, and > add a flag of "security_barrier". > I seems to work fine, even if view's property was changed between > rewriter and planner stage. > > postgres=# CREATE VIEW v1 WITH (security_barrier) AS SELECT * FROM t1 > WHERE a % 2 = 0; > CREATE VIEW > postgres=# PREPARE p1 AS SELECT * FROM v1 WHERE f_leak(b); > PREPARE > postgres=# EXECUTE p1; > NOTICE: f_leak => bbb > NOTICE: f_leak => ddd > a | b > ---+----- > 2 | bbb > 4 | ddd > (2 rows) > > postgres=# ALTER VIEW v1 SET (security_barrier=false); > ALTER VIEW > postgres=# EXECUTE p1; > NOTICE: f_leak => aaa > NOTICE: f_leak => bbb > NOTICE: f_leak => ccc > NOTICE: f_leak => ddd > NOTICE: f_leak => eee > a | b > ---+----- > 2 | bbb > 4 | ddd > (2 rows) > > postgres=# ALTER VIEW v1 SET (security_barrier=true); > ALTER VIEW > postgres=# EXECUTE p1; > NOTICE: f_leak => bbb > NOTICE: f_leak => ddd > a | b > ---+----- > 2 | bbb > 4 | ddd > (2 rows) > > Thanks, > -- > KaiGai Kohei <kaigai@kaigai.gr.jp> -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Sorry, patches were not attached. Again. I rebased my patch set. New functions in pg_proc.h prevented to apply previous revision cleanly. Here is no functional changes. Thanks, 2011/12/3 Kohei KaiGai <kaigai@kaigai.gr.jp>: > 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 <kaigai@kaigai.gr.jp>: >> 2011/11/2 Tom Lane <tgl@sss.pgh.pa.us>: >>> Kohei KaiGai <kaigai@kaigai.gr.jp> writes: >>>> The reason why I redefined the relid of RangeTblEntry is to avoid >>>> the problem when security_barrier attribute get changed by concurrent >>>> transactions between rewriter and planenr stage. >>> >>> This is complete nonsense. If the information is being injected into >>> the querytree by the rewriter, it's sufficient to assume that it's up to >>> date. Were it not so, we'd have problems with CREATE OR REPLACE RULE, >>> too. >>> >> I revised the patches to revert redefinition in relid of RangeTblEntry, and >> add a flag of "security_barrier". >> I seems to work fine, even if view's property was changed between >> rewriter and planner stage. >> >> postgres=# CREATE VIEW v1 WITH (security_barrier) AS SELECT * FROM t1 >> WHERE a % 2 = 0; >> CREATE VIEW >> postgres=# PREPARE p1 AS SELECT * FROM v1 WHERE f_leak(b); >> PREPARE >> postgres=# EXECUTE p1; >> NOTICE: f_leak => bbb >> NOTICE: f_leak => ddd >> a | b >> ---+----- >> 2 | bbb >> 4 | ddd >> (2 rows) >> >> postgres=# ALTER VIEW v1 SET (security_barrier=false); >> ALTER VIEW >> postgres=# EXECUTE p1; >> NOTICE: f_leak => aaa >> NOTICE: f_leak => bbb >> NOTICE: f_leak => ccc >> NOTICE: f_leak => ddd >> NOTICE: f_leak => eee >> a | b >> ---+----- >> 2 | bbb >> 4 | ddd >> (2 rows) >> >> postgres=# ALTER VIEW v1 SET (security_barrier=true); >> ALTER VIEW >> postgres=# EXECUTE p1; >> NOTICE: f_leak => bbb >> NOTICE: f_leak => ddd >> a | b >> ---+----- >> 2 | bbb >> 4 | ddd >> (2 rows) >> >> Thanks, >> -- >> KaiGai Kohei <kaigai@kaigai.gr.jp> > > > > -- > KaiGai Kohei <kaigai@kaigai.gr.jp> -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Вложения
On Mon, Sep 26, 2011 at 3:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Kohei KaiGai <kaigai@kaigai.gr.jp> writes: >> Sorry, are you saying the current (in other words, rte->security_barrier >> stores the state of reloption) approach is not a good idea? > > Yes. I think the same as Robert: the way to handle this is to store it > in RelOptInfo for the duration of planning, and pull it from the > catalogs during planner startup (cf plancat.c). Having looked at this more, I'm starting to believe KaiGai has this part right after all. The trouble is that the rewriter does this: /* * Now, plug the view query in as a subselect, replacing the relation's * original RTE. */ rte = rt_fetch(rt_index, parsetree->rtable); rte->rtekind = RTE_SUBQUERY; rte->relid = InvalidOid; rte->subquery = rule_action; rte->inh = false; /* must not be set for a subquery */ In other words, by the time the planner comes along and tries to decide whether or not it should flatten this subquery, the view has already been rewritten into a subquery - and that subquery is in most respects indistinguishable from a subquery that the user wrote directly. There is one difference: the permission check that would have been done against the view gets attached to the OLD entry in the subquery's range table. It would probably be possible to make this work by having the code paths that need to know whether or not a given subquery originated from a security-barrier-enabled view do that same trick: peek down into the OLD entry in the subquery rangetable, extract the view OID from there, and go check its reloptions. But that seems awfully complicated and error-prone, hence my feeling that just flagging the subquery explicitly is probably a better approach. One other possibility that comes to mind is that, instead of adding "bool security_view" to the RTE, we could instead add a new RTEKind, something like RTE_SECURITY_VIEW. That would mean going through and finding all the places that refer to RTE_SUBQUERY and adjusting them to handle RTE_SECURITY_VIEW in either the same way or differently as may be appropriate. The possible advantage of this approach is that it doesn't bloat the RTE structure (and stored rules that use it) with an additional attribute that (I think) will always be false - because security_barrier can only be set on a subquery RTE after rewriting has happened, and stored rules are haven't been rewritten yet. It might also force people to think a bit more carefully about how security views should be handled during future code changes, which could also be viewed as a plus. I'm attaching my current version of KaiGai's patch (with substantial cleanup of the comments and documentation, and some other changes) for reference. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Вложения
Robert Haas <robertmhaas@gmail.com> writes: > Having looked at this more, I'm starting to believe KaiGai has this > part right after all. Yeah, you have a point. The rewriter is intentionally trying to make an expanded view look just the same as an in-line SELECT-in-FROM, and we need it to be easier to distinguish them. > One other possibility that comes to mind is that, instead of adding > "bool security_view" to the RTE, we could instead add a new RTEKind, > something like RTE_SECURITY_VIEW. That would mean going through and > finding all the places that refer to RTE_SUBQUERY and adjusting them > to handle RTE_SECURITY_VIEW in either the same way or differently as > may be appropriate. The possible advantage of this approach is that > it doesn't bloat the RTE structure (and stored rules that use it) with > an additional attribute that (I think) will always be false - because > security_barrier can only be set on a subquery RTE after rewriting has > happened, and stored rules are haven't been rewritten yet. It might > also force people to think a bit more carefully about how security > views should be handled during future code changes, which could also > be viewed as a plus. Hmm. The question is whether the places where we need to care about this would naturally be looking at RTEKind anyway. If they are, or many are, then I think this might be a good idea. However if a lot of the action is elsewhere then I don't know if we get much leverage from the new RTEKind. I haven't read the patch lately so can't opine on that. regards, tom lane
On Wed, Dec 7, 2011 at 1:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> One other possibility that comes to mind is that, instead of adding >> "bool security_view" to the RTE, we could instead add a new RTEKind, >> something like RTE_SECURITY_VIEW. That would mean going through and >> finding all the places that refer to RTE_SUBQUERY and adjusting them >> to handle RTE_SECURITY_VIEW in either the same way or differently as >> may be appropriate. The possible advantage of this approach is that >> it doesn't bloat the RTE structure (and stored rules that use it) with >> an additional attribute that (I think) will always be false - because >> security_barrier can only be set on a subquery RTE after rewriting has >> happened, and stored rules are haven't been rewritten yet. It might >> also force people to think a bit more carefully about how security >> views should be handled during future code changes, which could also >> be viewed as a plus. > > Hmm. The question is whether the places where we need to care about > this would naturally be looking at RTEKind anyway. If they are, or many > are, then I think this might be a good idea. However if a lot of the > action is elsewhere then I don't know if we get much leverage from the > new RTEKind. I haven't read the patch lately so can't opine on that. *reads through the code* It looks to me like most places that look at RTE_SUBQUERY really have no reason to care about this. So probably it's just as well to have a separate flag for it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Sat, Dec 3, 2011 at 3:19 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > I rebased my patch set. New functions in pg_proc.h prevented to apply > previous revision cleanly. Here is no functional changes. I was thinking that my version of this (attached to an email from earlier today) might be about ready to commit. But while I was trolling through the archives on this problem trying to figure out who to credit, I found an old complaint of Tom's that we never fixed, and which represents a security exposure for this patch: rhaas=# create table foo (a integer); CREATE TABLE rhaas=# insert into foo select generate_series(1,10); INSERT 0 10 rhaas=# insert into foo values (1); INSERT 0 1 rhaas=# analyze foo; ANALYZE rhaas=# create view safe_foo with (security_barrier) as select * from foo where a > 5; CREATE VIEW rhaas=# grant select on safe_foo to bob; GRANT Secure in the knowledge that Bob will only be able to see rows where a is 6 or higher, we go to bed. But Bob finds a way to outsmart us: rhaas=> create or replace function leak(integer,integer) returns boolean as $$begin raise notice 'leak % %', $1, $2; return false; end$$ language plpgsql; CREATE FUNCTION rhaas=> create operator !! (procedure = leak, leftarg = integer, rightarg = integer, restrict = eqsel); CREATE OPERATOR rhaas=> explain select * from safe_foo where a !! 0; NOTICE: leak 1 0 QUERY PLAN -------------------------------------------------------------Subquery Scan on safe_foo (cost=0.00..2.70 rows=1 width=4) Filter: (safe_foo.a !! 0) -> Seq Scan on foo (cost=0.00..1.14 rows=6 width=4) Filter: (a > 5) (4 rows) OOPS. The *executor* has been persuaded not to apply the possibly-nefarious operator !! to the data until after applying the security-critical qual "a > 5". But the *planner* has no such compunctions, and has cheerfully leaked the most common value in the table, which the user wasn't supposed to see. I guess it's hopeless to suppose that we're going to completely conceal the list of MCVs from the user, since it might change the plan - and even if ProcessUtility_hook or somesuch is used to disable EXPLAIN, the user can still try to ferret out the MCVs via a timing attack. That having been said, the above behavior doesn't sit well with me: letting the user probe for MCVs via a timing attack or a plan change is one thing; printing them out on request is a little bit too convenient for my taste. :-( -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
2011/12/8 Robert Haas <robertmhaas@gmail.com>: > On Sat, Dec 3, 2011 at 3:19 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> I rebased my patch set. New functions in pg_proc.h prevented to apply >> previous revision cleanly. Here is no functional changes. > > I was thinking that my version of this (attached to an email from > earlier today) might be about ready to commit. But while I was > trolling through the archives on this problem trying to figure out who > to credit, I found an old complaint of Tom's that we never fixed, and > which represents a security exposure for this patch: > > rhaas=# create table foo (a integer); > CREATE TABLE > rhaas=# insert into foo select generate_series(1,10); > INSERT 0 10 > rhaas=# insert into foo values (1); > INSERT 0 1 > rhaas=# analyze foo; > ANALYZE > rhaas=# create view safe_foo with (security_barrier) as select * from > foo where a > 5; > CREATE VIEW > rhaas=# grant select on safe_foo to bob; > GRANT > > Secure in the knowledge that Bob will only be able to see rows where a > is 6 or higher, we go to bed. But Bob finds a way to outsmart us: > > rhaas=> create or replace function leak(integer,integer) returns > boolean as $$begin raise notice 'leak % %', $1, $2; return false; > end$$ language plpgsql; > CREATE FUNCTION > rhaas=> create operator !! (procedure = leak, leftarg = integer, > rightarg = integer, restrict = eqsel); > CREATE OPERATOR > rhaas=> explain select * from safe_foo where a !! 0; > NOTICE: leak 1 0 > QUERY PLAN > ------------------------------------------------------------- > Subquery Scan on safe_foo (cost=0.00..2.70 rows=1 width=4) > Filter: (safe_foo.a !! 0) > -> Seq Scan on foo (cost=0.00..1.14 rows=6 width=4) > Filter: (a > 5) > (4 rows) > > OOPS. The *executor* has been persuaded not to apply the > possibly-nefarious operator !! to the data until after applying the > security-critical qual "a > 5". But the *planner* has no such > compunctions, and has cheerfully leaked the most common value in the > table, which the user wasn't supposed to see. I guess it's hopeless > to suppose that we're going to completely conceal the list of MCVs > from the user, since it might change the plan - and even if > ProcessUtility_hook or somesuch is used to disable EXPLAIN, the user > can still try to ferret out the MCVs via a timing attack. That having > been said, the above behavior doesn't sit well with me: letting the > user probe for MCVs via a timing attack or a plan change is one thing; > printing them out on request is a little bit too convenient for my > taste. :-( > Sorry, I missed this scenario, and have not investigated this code path in detail yet. My first impression remind me an idea that I proposed before, even though it got negative response due to user visible changes. It requires superuser privilege to create new operators, since we assume superuser does not set up harmful configuration. I still think it is an idea. Or, maybe, we can adopt a bit weaker restriction; functions being used to operators must have "leakproof" property. Is it worthful to have a discussion again? Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On Thu, Dec 8, 2011 at 5:17 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > My first impression remind me an idea that I proposed before, even > though it got negative response due to user visible changes. > It requires superuser privilege to create new operators, since we > assume superuser does not set up harmful configuration. I don't think that's acceptable from a usability point of view; this feature is important, but not important enough to go start ripping out other features that people are already using, like non-superuser operators. I'm also pretty skeptical that it would fix the problem, because the superuser might fail to realize that creating an operator was going to create this type of security exposure. After all, you and I also failed to realize that, so it's obviously a fairly subtle problem. I feel like there must be some logic in the planner somewhere that is "looking through" the subquery RTE and figuring out that safe_foo.a is really the same variable as foo.a, and which therefore feels entitled to apply !!'s selectivity estimator to foo.a's statistics. If that's the case, it might be possible to handicap that logic so that when the security_barrier flag is set, it doesn't do that, and instead treats safe_foo.a as a black box. That would, obviously, degrade the quality of complex plans involving security views, but I think we should focus on getting something that meets our security goals first and then try to improve performance later. (For example, I am fairly certain that only a superuser can install a new selectivity estimator; so perhaps we could allow selectivity estimators to be signaled with the information that a security view interposes or not, and then they can make an estimator-specific decision on how to punt; but on the other hand that might be a stupid idea; so for step #1 let's just figure out how to batten down the hatches.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
2011/12/9 Robert Haas <robertmhaas@gmail.com>: > On Thu, Dec 8, 2011 at 5:17 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> My first impression remind me an idea that I proposed before, even >> though it got negative response due to user visible changes. >> It requires superuser privilege to create new operators, since we >> assume superuser does not set up harmful configuration. > > I don't think that's acceptable from a usability point of view; this > feature is important, but not important enough to go start ripping out > other features that people are already using, like non-superuser > operators. I'm also pretty skeptical that it would fix the problem, > because the superuser might fail to realize that creating an operator > was going to create this type of security exposure. After all, you > and I also failed to realize that, so it's obviously a fairly subtle > problem. > OK, I agree with your opinion. It may stand on a fiction story that superuser understand all effects and risk of his operations. If this assumption get broken, system's security is also broken. > I feel like there must be some logic in the planner somewhere that is > "looking through" the subquery RTE and figuring out that safe_foo.a is > really the same variable as foo.a, and which therefore feels entitled > to apply !!'s selectivity estimator to foo.a's statistics. If that's > the case, it might be possible to handicap that logic so that when the > security_barrier flag is set, it doesn't do that, and instead treats > safe_foo.a as a black box. That would, obviously, degrade the quality > of complex plans involving security views, but I think we should focus > on getting something that meets our security goals first and then try > to improve performance later. > I tried to investigate the code around size-estimation, and it seems to me here is two candidates to put this type of checks. The one is examine_simple_variable() that is used to pull a datum from statistic information, but it locates on the code path restriction estimator of operators; so user controlable, although it requires least code changes just after if (rte->rtekind == RTE_SUBQUERY). The other is clause_selectivity(). Its code path is not user controlable, so we can apply necessary checks to prevent qualifier that reference variable come from sub-query with security-barrier. In my sense, clause_selectivity() is better place to apply this type of checks. But, on the other hand, it provides get_relation_stats_hook to allow extensions to control references to statistic data. So, I wonder whether the PG core assumes this routine covers all the code path here? In addition, I also consider the case when we add a functionality that forcibly adds restriction on WHERE clause of regular tables in the future version, like: SELECT * FROM t WHERE a > 0; ==> SELECT * FROM t WHERE sepgsql_policy(selinux_label) AND a > 0; Probably, same solution will be available to avoid unintentional references to pg_statistic; as long as security_barrier is set on rte of regular tables. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
2011/12/10 Kohei KaiGai <kaigai@kaigai.gr.jp>: > 2011/12/9 Robert Haas <robertmhaas@gmail.com>: >> I feel like there must be some logic in the planner somewhere that is >> "looking through" the subquery RTE and figuring out that safe_foo.a is >> really the same variable as foo.a, and which therefore feels entitled >> to apply !!'s selectivity estimator to foo.a's statistics. If that's >> the case, it might be possible to handicap that logic so that when the >> security_barrier flag is set, it doesn't do that, and instead treats >> safe_foo.a as a black box. That would, obviously, degrade the quality >> of complex plans involving security views, but I think we should focus >> on getting something that meets our security goals first and then try >> to improve performance later. >> > I tried to investigate the code around size-estimation, and it seems to > me here is two candidates to put this type of checks. > > The one is examine_simple_variable() that is used to pull a datum > from statistic information, but it locates on the code path restriction > estimator of operators; so user controlable, although it requires least > code changes just after if (rte->rtekind == RTE_SUBQUERY). > The other is clause_selectivity(). Its code path is not user controlable, > so we can apply necessary checks to prevent qualifier that reference > variable come from sub-query with security-barrier. > > In my sense, clause_selectivity() is better place to apply this type of > checks. But, on the other hand, it provides get_relation_stats_hook > to allow extensions to control references to statistic data. > So, I wonder whether the PG core assumes this routine covers > all the code path here? > The attached patch adds checks around invocation of selectivity estimator functions, and it changes behavior of the estimator, if the supplied operator tries to touch variables come from security-barrier relations. Then, it fixes the problem you mentioned. postgres=# explain select * from safe_foo where a !! 0; QUERY PLAN ------------------------------------------------------------- Subquery Scan on safe_foo (cost=0.00..2.70 rows=3 width=4) Filter: (safe_foo.a !! 0) -> Seq Scan on foo (cost=0.00..1.14 rows=6 width=4) Filter: (a > 5) (4 rows) However, I'm still a bit skeptical on my patch, because it still allows to invoke estimator function when operator's argument does not touch values originated from security-barrier relation. In the case when oprrest or oprjoin are implemented without our regular convention (E.g, it anyway reference whole of statistical data), it will break this solution. Of course, it is an assumption that we cannot prevent any attack using binary modules, so we need to say "use it your own risk" if people tries to use extensional modules. And, we also need to keep the built-in code secure. Some of built-in estimator functions (such as eqsel) provides a feature that invokes operator function with arguments originated from pg_statistics table. It didn't harmless, however, we got understand that this logic can be used to break row-level security. So, I begin to consider the routines to be revised are some of built-in functions to be used for estimator functions; such as eqsel and ... These function eventually reference statistical data at examine_variable. It might be a better approach to add checks whether invocation of the supplied operator possibly leaks contents to be invisible. It seems to me the Idea of the attached patch depends on something internal stuff of existing built-in estimator functions... Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Вложения
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 <kaigai@kaigai.gr.jp>: > 2011/12/10 Kohei KaiGai <kaigai@kaigai.gr.jp>: >> 2011/12/9 Robert Haas <robertmhaas@gmail.com>: >>> I feel like there must be some logic in the planner somewhere that is >>> "looking through" the subquery RTE and figuring out that safe_foo.a is >>> really the same variable as foo.a, and which therefore feels entitled >>> to apply !!'s selectivity estimator to foo.a's statistics. If that's >>> the case, it might be possible to handicap that logic so that when the >>> security_barrier flag is set, it doesn't do that, and instead treats >>> safe_foo.a as a black box. That would, obviously, degrade the quality >>> of complex plans involving security views, but I think we should focus >>> on getting something that meets our security goals first and then try >>> to improve performance later. >>> >> I tried to investigate the code around size-estimation, and it seems to >> me here is two candidates to put this type of checks. >> >> The one is examine_simple_variable() that is used to pull a datum >> from statistic information, but it locates on the code path restriction >> estimator of operators; so user controlable, although it requires least >> code changes just after if (rte->rtekind == RTE_SUBQUERY). >> The other is clause_selectivity(). Its code path is not user controlable, >> so we can apply necessary checks to prevent qualifier that reference >> variable come from sub-query with security-barrier. >> >> In my sense, clause_selectivity() is better place to apply this type of >> checks. But, on the other hand, it provides get_relation_stats_hook >> to allow extensions to control references to statistic data. >> So, I wonder whether the PG core assumes this routine covers >> all the code path here? >> > The attached patch adds checks around invocation of selectivity > estimator functions, and it changes behavior of the estimator, if the > supplied operator tries to touch variables come from security-barrier > relations. > Then, it fixes the problem you mentioned. > > postgres=# explain select * from safe_foo where a !! 0; > QUERY PLAN > ------------------------------------------------------------- > Subquery Scan on safe_foo (cost=0.00..2.70 rows=3 width=4) > Filter: (safe_foo.a !! 0) > -> Seq Scan on foo (cost=0.00..1.14 rows=6 width=4) > Filter: (a > 5) > (4 rows) > > However, I'm still a bit skeptical on my patch, because it still allows > to invoke estimator function when operator's argument does not > touch values originated from security-barrier relation. > In the case when oprrest or oprjoin are implemented without our > regular convention (E.g, it anyway reference whole of statistical data), > it will break this solution. > > Of course, it is an assumption that we cannot prevent any attack > using binary modules, so we need to say "use it your own risk" if > people tries to use extensional modules. And, we also need to > keep the built-in code secure. > > Some of built-in estimator functions (such as eqsel) provides > a feature that invokes operator function with arguments originated > from pg_statistics table. It didn't harmless, however, we got understand > that this logic can be used to break row-level security. > So, I begin to consider the routines to be revised are some of built-in > functions to be used for estimator functions; such as eqsel and ... > These function eventually reference statistical data at examine_variable. > > It might be a better approach to add checks whether invocation of > the supplied operator possibly leaks contents to be invisible. > It seems to me the Idea of the attached patch depends on something > internal stuff of existing built-in estimator functions... > > Thanks, > -- > KaiGai Kohei <kaigai@kaigai.gr.jp> -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Вложения
On Mon, Dec 12, 2011 at 12:00 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > The "v8.option-2" add checks around examine_simple_variable, and > prevent to reference statistical data, if Var node tries to reference > relation with security-barrier attribute. I adopted this approach, and committed this. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
2011/12/22 Robert Haas <robertmhaas@gmail.com>: > On Mon, Dec 12, 2011 at 12:00 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> The "v8.option-2" add checks around examine_simple_variable, and >> prevent to reference statistical data, if Var node tries to reference >> relation with security-barrier attribute. > > I adopted this approach, and committed this. > Thanks for your help and efforts. I'd like the regression test on select_view test being committed also to detect unexpected changed in the future. How about it? Best regards, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On Fri, Dec 23, 2011 at 5:56 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > I'd like the regression test on select_view test being committed also > to detect unexpected changed in the future. How about it? Can you resend that as a separate patch? I remember there were some things I didn't like about it, but I don't remember what they were at the moment... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
2011/12/23 Robert Haas <robertmhaas@gmail.com>: > On Fri, Dec 23, 2011 at 5:56 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> I'd like the regression test on select_view test being committed also >> to detect unexpected changed in the future. How about it? > > Can you resend that as a separate patch? I remember there were some > things I didn't like about it, but I don't remember what they were at > the moment... > Sorry for this late response. The attached one is patch of the regression test that checks scenario of malicious function with/without security_barrier option. I guess you concerned about that expected/select_views_1.out is patched, not expected/select_views.out. I'm not sure the reason why regression test script tries to make diff between results/select_views and expected/select_views_1.out. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Вложения
On Tue, Jan 3, 2012 at 12:11 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > 2011/12/23 Robert Haas <robertmhaas@gmail.com>: >> On Fri, Dec 23, 2011 at 5:56 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >>> I'd like the regression test on select_view test being committed also >>> to detect unexpected changed in the future. How about it? >> >> Can you resend that as a separate patch? I remember there were some >> things I didn't like about it, but I don't remember what they were at >> the moment... >> > Sorry for this late response. > > The attached one is patch of the regression test that checks scenario > of malicious function with/without security_barrier option. > > I guess you concerned about that expected/select_views_1.out is > patched, not expected/select_views.out. > I'm not sure the reason why regression test script tries to make diff > between results/select_views and expected/select_views_1.out. select_views.out and select_views_1.out are alternate expected output files. The regression tests pass if the actual output matches either one. Thus, you have to patch both. BTW, can you also resubmit the leakproof stuff as a separate patch for the last CF? Want to make sure we get that into 9.2, if at all possible. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
>> 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 <kaigai@kaigai.gr.jp>
Вложения
On Sun, Jan 8, 2012 at 10:32 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >>> I guess you concerned about that expected/select_views_1.out is >>> patched, not expected/select_views.out. >>> I'm not sure the reason why regression test script tries to make diff >>> between results/select_views and expected/select_views_1.out. >> >> select_views.out and select_views_1.out are alternate expected output >> files. The regression tests pass if the actual output matches either >> one. Thus, you have to patch both. >> > It was new for me. The attached patch updates both of the expected > files, however, I'm not certain whether select_view.out is suitable, or > not, because my results/select_view.out matched with expected/select_view_1.out. Committed. We'll see what the buildfarm thinks. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Jan 17, 2012 at 7:08 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Sun, Jan 8, 2012 at 10:32 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >>>> I guess you concerned about that expected/select_views_1.out is >>>> patched, not expected/select_views.out. >>>> I'm not sure the reason why regression test script tries to make diff >>>> between results/select_views and expected/select_views_1.out. >>> >>> select_views.out and select_views_1.out are alternate expected output >>> files. The regression tests pass if the actual output matches either >>> one. Thus, you have to patch both. >>> >> It was new for me. The attached patch updates both of the expected >> files, however, I'm not certain whether select_view.out is suitable, or >> not, because my results/select_view.out matched with expected/select_view_1.out. > > Committed. We'll see what the buildfarm thinks. This passes installcheck initially. Then upon second invocation of installcheck, it fails. It creates the role "alice", and doesn't clean it up. On next invocation alice already exists and cases a failure in test select_views. Cheers, Jeff
2012/1/21 Jeff Janes <jeff.janes@gmail.com>: > On Tue, Jan 17, 2012 at 7:08 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Sun, Jan 8, 2012 at 10:32 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >>>>> I guess you concerned about that expected/select_views_1.out is >>>>> patched, not expected/select_views.out. >>>>> I'm not sure the reason why regression test script tries to make diff >>>>> between results/select_views and expected/select_views_1.out. >>>> >>>> select_views.out and select_views_1.out are alternate expected output >>>> files. The regression tests pass if the actual output matches either >>>> one. Thus, you have to patch both. >>>> >>> It was new for me. The attached patch updates both of the expected >>> files, however, I'm not certain whether select_view.out is suitable, or >>> not, because my results/select_view.out matched with expected/select_view_1.out. >> >> Committed. We'll see what the buildfarm thinks. > > This passes installcheck initially. Then upon second invocation of > installcheck, it fails. > > It creates the role "alice", and doesn't clean it up. On next > invocation alice already exists and cases a failure in test > select_views. > Thanks for your pointing out. The attached patch adds cleaning-up part of object being defined within this test; includes user "alice". Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Вложения
On Sun, Jan 22, 2012 at 5:57 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> This passes installcheck initially. Then upon second invocation of >> installcheck, it fails. >> >> It creates the role "alice", and doesn't clean it up. On next >> invocation alice already exists and cases a failure in test >> select_views. >> > Thanks for your pointing out. > > The attached patch adds cleaning-up part of object being defined > within this test; > includes user "alice". Urp. I failed to notice this patch and committed a different fix for the problem pointed out by Jeff. I'm inclined to think it's OK to leave the non-shared objects behind - among other things, if people are testing pg_upgrade using the regression database, this will help to ensure that pg_dump is handling security_barrier views correctly. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company