Re: [HACKERS] [COMMITTERS] pgsql: Fix permission tests for views/tables proven empty by constraint
Andres Freund writes: > On 2013-05-07 21:45:02 -0400, Tom Lane wrote: >> Well, it might fail to report a permissions violation when the >> not-allowed-to-be-accessed relation could be proven to yield no rows. > Couldn't it also cause tables not to be locked that ought to be? That > seems to be the nastier part to me. In ordinary immediate execution the parser or planner would have obtained the relevant table lock. I think what you say is possible if a prepared plan is re-executed, but TBH it doesn't sound like much of an issue to me. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Fix permission tests for views/tables proven empty by constraint
On 2013-05-07 21:45:02 -0400, Tom Lane wrote: > Greg Stark writes: > > If we just reverted your fix and didn't fix it in 9.2 that would also > > fix the crash right? The bug was only that it leaked the fact that the > > view was provably empty from the definition? > > Well, it might fail to report a permissions violation when the > not-allowed-to-be-accessed relation could be proven to yield no rows. > I agree that it's a bit hard to call that a security issue as long as > you assume that the attacker has access to the system catalogs; and > even if you don't assume that, being able to discern that there's a > check constraint on some table doesn't seem like a big leakage. Couldn't it also cause tables not to be locked that ought to be? That seems to be the nastier part to me. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Fix permission tests for views/tables proven empty by constraint
Greg Stark writes: > If we just reverted your fix and didn't fix it in 9.2 that would also > fix the crash right? The bug was only that it leaked the fact that the > view was provably empty from the definition? Well, it might fail to report a permissions violation when the not-allowed-to-be-accessed relation could be proven to yield no rows. I agree that it's a bit hard to call that a security issue as long as you assume that the attacker has access to the system catalogs; and even if you don't assume that, being able to discern that there's a check constraint on some table doesn't seem like a big leakage. I had originally thought that the issue only occurred in 9.2, but it turns out that the appendrel form of the problem occurs at least as far back as 8.4; for example the following admittedly-artificial query select * from ((select f1 as x from t1 offset 0) union all (select f2 as x from t2 offset 0)) ss where false; will not throw an error in any current release, even if the caller lacks select privilege on t1 and/or t2. With manipulation of the outer WHERE clause, you could find out about the nature of any check constraints on t1/t2. It's easier to see the bug in 9.2 because you no longer need a UNION ALL, but that doesn't much change the argument about whether it's a security issue. Given that forms of the bug have been around for a long time without anyone noticing, it might be okay to leave it unfixed in the back branches. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Fix permission tests for views/tables proven empty by constraint
Robert Haas writes: > On Wed, May 1, 2013 at 6:27 PM, Tom Lane wrote: >> Fix permission tests for views/tables proven empty by constraint exclusion. > I believe that this commit is responsible for the fact that the > following test case now crashes the server: > rhaas=# create or replace view foo (x) AS (select 1 union all select 2); > CREATE VIEW > rhaas=# select * from foo where false; > The connection to the server was lost. Attempting reset: Failed. OK, after looking a bit closer, this is actually a situation I had been wondering about last week, but I failed to construct a test case proving there was a problem. The issue is that the "append rel" code path also needs changes to handle the fact that subqueries that are appendrel members might get proven empty by constraint exclusion. (Even aside from the crash introduced here, the excluded subqueries fail to contribute to the final rangetable for permissions checks.) Unfortunately this blows a bit of a hole in the minimal fix I committed last week; there's no nice way to include these dummy subqueries into the plan tree that's passed on to setrefs.c. Ideally we'd add an out-of-band transmission path for them, and I think I will fix it that way in HEAD, but that requires adding a new List field to PlannerInfo. I'm afraid to do that in 9.2 for fear of breaking existing planner plugin modules. One way to fix it in 9.2 is to teach setrefs.c to thumb through the append_rel_list looking for excluded child subqueries to pull up their rangetables. Icky, but it shouldn't cost very many cycles in typical queries. The main downside of this solution is that it would add at least several dozen lines of new-and-poorly-tested code to a back branch, where it would get no additional testing to speak of before being loosed on users. The other way I can think of to handle it without PlannerInfo changes is to lobotomize set_append_rel_pathlist so that it doesn't omit dummy children from the AppendPath it constructs. The main downside of this is that fully dummy appendrels (those with no live children at all, such as in your example) wouldn't be recognized by IS_DUMMY_PATH, so the quality of planning in the outer query would be slightly degraded. But such cases are probably sufficiently unusual that this might be an okay price to pay for a back branch. Thoughts? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Fix permission tests for views/tables proven empty by constraint
Robert Haas writes: > rhaas=# create or replace view foo (x) AS (select 1 union all select 2); > CREATE VIEW > rhaas=# select * from foo where false; > The connection to the server was lost. Attempting reset: Failed. Ugh. I'm about to leave for the day, but I'll take a look later. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
