Обсуждение: Re: [COMMITTERS] pgsql: Fix permission tests for views/tables proven empty by constraint

On Wed, May 1, 2013 at 6:27 PM, Tom Lane <tgl@sss.pgh.pa.us> 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.

(gdb) bt
#0  is_dummy_plan (plan=0x0) at planner.c:1850
#1  0x000000010bd44c3e in create_append_plan [inlined] () at
/Users/rhaas/pgsql/src/backend/optimizer/plan/createplan.c:706
#2  0x000000010bd44c3e in create_plan_recurse (root=0x7fff54080e60,
best_path=0x7f9d4b109270) at createplan.c:247
#3  0x000000010bd3f4bd in create_plan (root=0x7f9d4b0389d0,
best_path=0x7f9d4b109270) at createplan.c:201
#4  0x000000010bd4aa64 in grouping_planner (root=0x7f9d4b0389d0,
tuple_fraction=6.9532132623547611e-310) at planner.c:1294
#5  0x000000010bd4c74e in subquery_planner (glob=0x7f9d4b0389d0,
parse=0x7fff54081290, parent_root=0x7f9d4b107838,
tuple_fraction=6.9532132623808478e-310, subroot=0x7fff54081290,
hasRecursion=0 '\0') at planner.c:558
#6  0x000000010bd4ca0c in standard_planner (parse=0x7f9d4b038020,
cursorOptions=0, boundParams=0x7f9d4b038020) at planner.c:209
#7  0x000000010bdc4ba3 in pg_plan_query (querytree=0x7f9d4b037ce8,
cursorOptions=1258519784, boundParams=0x7f9d4b109470) at
postgres.c:753
#8  0x000000010bdc746c in pg_plan_queries [inlined] () at
/Users/rhaas/pgsql/src/backend/tcop/postgres.c:812
#9  0x000000010bdc746c in exec_simple_query [inlined] () at
/Users/rhaas/pgsql/src/backend/tcop/postgres.c:977
#10 0x000000010bdc746c in PostgresMain (dbname=0x7f9d4b01f028 "rhaas",
argc=1, argv=0x10bf11824, username=0x7f9d4b037a58 "?\002") at
postgres.c:3985
#11 0x000000010bd754b3 in PostmasterMain (argc=1409820832,
argv=0x7fff540828a0) at postmaster.c:3985
#12 0x000000010bd09f18 in main (argc=1, argv=0x7f9d4ac04050) at main.c:196

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> 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

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, May 1, 2013 at 6:27 PM, Tom Lane <tgl@sss.pgh.pa.us> 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

On Tue, May 7, 2013 at 6:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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

That's kind of dismaying. ORMs have a tendency to create queries like
this and people may have even written such queries by hand and tested
them to determine that postgres was able to exclude the useless
relation. To have them install a security update and discover that
something they had previously tested no longer worked would be
annoying.

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? But it's been that way
for a long time and after all:

xxx=> select * from pg_views where viewname = 'v';schemaname | viewname | viewowner |   definition
------------+----------+-----------+----------------public     | v        | stark     |  SELECT 1     +           |
    |           |   WHERE false;
 
(1 row)

-- 
greg

Greg Stark <stark@mit.edu> 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

On 2013-05-07 21:45:02 -0400, Tom Lane wrote:
> Greg Stark <stark@mit.edu> 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

Andres Freund <andres@2ndquadrant.com> 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

Greg Stark <stark@mit.edu> writes:
> That's kind of dismaying. ORMs have a tendency to create queries like
> this and people may have even written such queries by hand and tested
> them to determine that postgres was able to exclude the useless
> relation. To have them install a security update and discover that
> something they had previously tested no longer worked would be
> annoying.

Turns out to be more to this than I realized before.  In an example
such as the one I showed

select * from ((select f1 as x from t1 offset 0)  union all  (select f2 as x from t2 offset 0)) ss
where false;

where an appendrel subselect member can be proven empty on the basis
of outer-query clauses alone, *we don't even plan that subquery*.
The fix I had in mind for this fails to capture table references from
such a subquery.

We could extend setrefs.c to dig into unplanned subqueries and grab RTEs
out of them, but that would not be a complete fix.  In particular, RTEs
would not get made for inheritance children of parent tables mentioned
in the query, since inheritance expansion is done by the planner.  Now,
that wouldn't affect permissions checks because no extra permissions
checks are done on inheritance children, but it would affect the locking
behavior that Andres was worried about.

I think the only reliable way to make this optimization fully
transparent would be to go ahead and plan every subquery, even when we
know we'll discard the planning results immediately.  That seems like
quite a lot of overkill.  I'm not really sure I buy Greg's argument
that people might be depending on the performance benefits of not
planning such subqueries, but I guess it's not impossible either.

My inclination is to go ahead and write the extra code to dig RTEs out
of unplanned subqueries, and not worry about failing to lock inheritance
children in them.

I'm also now pretty firmly in the camp of "let's not try at all to fix
this in the back branches".

Thoughts?
        regards, tom lane

On 2013-05-08 12:30:31 -0400, Tom Lane wrote:
> Greg Stark <stark@mit.edu> writes:
> > That's kind of dismaying. ORMs have a tendency to create queries like
> > this and people may have even written such queries by hand and tested
> > them to determine that postgres was able to exclude the useless
> > relation. To have them install a security update and discover that
> > something they had previously tested no longer worked would be
> > annoying.
> 
> Turns out to be more to this than I realized before.  In an example
> such as the one I showed
> 
> select * from
>   ((select f1 as x from t1 offset 0)
>    union all
>    (select f2 as x from t2 offset 0)) ss
> where false;
> 
> where an appendrel subselect member can be proven empty on the basis
> of outer-query clauses alone, *we don't even plan that subquery*.
> The fix I had in mind for this fails to capture table references from
> such a subquery.

> We could extend setrefs.c to dig into unplanned subqueries and grab RTEs
> out of them, but that would not be a complete fix.  In particular, RTEs
> would not get made for inheritance children of parent tables mentioned
> in the query, since inheritance expansion is done by the planner.  Now,
> that wouldn't affect permissions checks because no extra permissions
> checks are done on inheritance children, but it would affect the locking
> behavior that Andres was worried about.

I first thought that is fair enough since I thought that in most if not
all cases where locking plays a user visible role the parent relation
would get locked anyway when a child relations gets locked. Turns out,
we do it only the other way round, i.e. lock child relations when we
lock a parent relation, even for most ddl in child relations.

I am not sure if its really problematic, but it seems to allow scenarios
like:

S1: BEGIN;
S1: SELECT * FROM ((SELECT * FROM parent OFFSET 0) UNION ALL (SELECT * FROM parent OFFSET 0)) f WHERE false;
-- parent is locked now, children are not
S2: BEGIN;
S2: ALTER TABLE child_1 DROP CONSTRAINT foo;
S1: SELECT * FROM parent WHERE ...
-- blocks, waiting for S1 since child_1 is locked.

This seems like somewhat confusing behaviour, although it has gone
unnoticed so far, since one normally expect that a previous lock allows
you to continue workin with a relation.

But I guess this is better fixed by making all DDL on child relations
also lock their parent relation? That seems like a good idea anyway.

I am not at all convinced that this must be fixed, but also not the
other way round. I just wanted to point this out since I am not sure
there aren't any more problematic cases.

> I think the only reliable way to make this optimization fully
> transparent would be to go ahead and plan every subquery, even when we
> know we'll discard the planning results immediately.  That seems like
> quite a lot of overkill.  I'm not really sure I buy Greg's argument
> that people might be depending on the performance benefits of not
> planning such subqueries, but I guess it's not impossible either.

I didn't understand Greg's argument as being based on performance but as
being worried about the changed locking and such from a functional
perspective. Greg?

I don't really buy the performance argument either, but I agree that we
shouldn't do all this in the back branches as the "bug" isn't really bad
and it has some potential for introducing problems.

> I'm also now pretty firmly in the camp of "let's not try at all to fix
> this in the back branches".

+1 independent of where this goes.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services