Re: BUG #4350: 'select' acess given to views containing "union all" even though user has no grants

The following bug has been logged online:

Bug reference: 4350
Logged by: Brendan O'Shea
Email address: boshea(at)akamai(dot)com
PostgreSQL version: 8.2.9
Operating system: linux-2.4 and windows XP
Description: 'select' acess given to views containing "union all"
even though user has no grants
Details:

There appears to be a bug in the way that permissions are determined for
views that contain "UNION ALL" in their definition.

There is a simple test case to reproduce the bug.

1) As a superuser create the following objects:

CREATE ROLE test_perm LOGIN PASSWORD 'test_perm';

CREATE OR REPLACE VIEW public.simple_select AS SELECT 1;
CREATE OR REPLACE VIEW public.union_all AS SELECT 1 UNION ALL SELECT 2;

2) Now log in as the test_perm user and run the following SQL:

select * from public.simple_select;
select * from public.union_all;

The first SQL statement correctly produces an error, but the second
statement will return results with no error, it should instead generate a
permission error.

Brendan O'Shea wrote:
> There appears to be a bug in the way that permissions are determined for
> views that contain "UNION ALL" in their definition.
>
> There is a simple test case to reproduce the bug.
>
> 1) As a superuser create the following objects:
>
> CREATE ROLE test_perm LOGIN PASSWORD 'test_perm';
>
> CREATE OR REPLACE VIEW public.simple_select AS SELECT 1;
> CREATE OR REPLACE VIEW public.union_all AS SELECT 1 UNION ALL SELECT 2;
>
>
> 2) Now log in as the test_perm user and run the following SQL:
>
> select * from public.simple_select;
> select * from public.union_all;
>
> The first SQL statement correctly produces an error, but the second
> statement will return results with no error, it should instead generate a
> permission error.

Hmm, looks like pull_up_subqueries somehow loses the range table entry
referring the original view. It's reproducible on PG version 8.2 and
higher, 8.1 seems to work. I'll dig deeper tomorrow, unless someone else
beats me to it.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas wrote:
> Brendan O'Shea wrote:
>> There appears to be a bug in the way that permissions are determined for
>> views that contain "UNION ALL" in their definition.
>> There is a simple test case to reproduce the bug.
>>
>> 1) As a superuser create the following objects:
>>
>> CREATE ROLE test_perm LOGIN PASSWORD 'test_perm';
>>
>> CREATE OR REPLACE VIEW public.simple_select AS SELECT 1;
>> CREATE OR REPLACE VIEW public.union_all AS SELECT 1 UNION ALL SELECT 2;
>>
>>
>> 2) Now log in as the test_perm user and run the following SQL:
>>
>> select * from public.simple_select;
>> select * from public.union_all;
>>
>> The first SQL statement correctly produces an error, but the second
>> statement will return results with no error, it should instead generate a
>> permission error.
>
> Hmm, looks like pull_up_subqueries somehow loses the range table entry
> referring the original view. It's reproducible on PG version 8.2 and
> higher, 8.1 seems to work. I'll dig deeper tomorrow, unless someone else
> beats me to it.

Yep, pull_up_simple_union_all neglects to copy those range table
references that are not directly referenced in any of the UNION ALL
subqueries.

Attached is a patch for this. I have to go to sleep now, but will commit
this tomorrow unless someone comes up with a better idea. One thing that
I'm not totally happy about is that I'm using list_union, which uses
O(n^2) algorithm.

Thanks for the report!

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

"Heikki Linnakangas" <heikki(at)enterprisedb(dot)com> writes:
> + root->parse->rtable = list_union(root->parse->rtable, subquery->rtable);

That's one heck of a scary patch: nowhere in list_union's API is there
any guarantee that it preserves list ordering, but we *must not* change
the positions of the existing rtable entries.

I think it might be better to fix the problem in
pull_up_union_leaf_queries instead; given that it wasn't broken till
recently, I think it's arguably that function's fault. Can we redesign
it to pull up everything in the subquery rtable, not just what was
referenced?

regards, tom lane

I wrote:
> That's one heck of a scary patch: nowhere in list_union's API is there
> any guarantee that it preserves list ordering, but we *must not* change
> the positions of the existing rtable entries.

Actually there's a more fundamental problem, namely that pulled-up
subqueries aren't necessarily equal() to the originals. They will
definitely be different if there were any uplevel Var references.

While you could argue that it doesn't matter because we'll only
end up redundantly checking permissions on multiple copies of the
RTEs, that's a bit beyond my threshold of ugliness...

regards, tom lane

Tom Lane wrote:
> I wrote:
>> That's one heck of a scary patch: nowhere in list_union's API is there
>> any guarantee that it preserves list ordering, but we *must not* change
>> the positions of the existing rtable entries.

Good point.

> Actually there's a more fundamental problem, namely that pulled-up
> subqueries aren't necessarily equal() to the originals. They will
> definitely be different if there were any uplevel Var references.

Another good point. I just saw that they're copied with copyObject and
are thus equal, but missed the IncrementVarSublevelsUp call.

> While you could argue that it doesn't matter because we'll only
> end up redundantly checking permissions on multiple copies of the
> RTEs, that's a bit beyond my threshold of ugliness...

Yeah, that wasn't the intention.

Attached is a patch with slightly different approach. Instead of
list_union, I'm keeping track of which rtes are copied by
pull_up_union_leaf_queries in a bitmapset.

BTW, I wonder if it's possible to end up with multiple copies of the
same RTE anyway, if there's multiple references to the same RTE. I'm
guessing that it's either not possible or we don't care, because that
can happen without the patch just as well.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Tom Lane wrote:
> I think it might be better to fix the problem in
> pull_up_union_leaf_queries instead; given that it wasn't broken till
> recently, I think it's arguably that function's fault.

Not sure what you mean. pull_up_union_leaf_queries was introduced at the
same time as the rest of the logic to pull up UNION ALL subqueries, in
8.2, and has been broken ever since.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

"Heikki Linnakangas" <heikki(at)enterprisedb(dot)com> writes:
> Tom Lane wrote:
>> I think it might be better to fix the problem in
>> pull_up_union_leaf_queries instead; given that it wasn't broken till
>> recently, I think it's arguably that function's fault.

> Not sure what you mean. pull_up_union_leaf_queries was introduced at the
> same time as the rest of the logic to pull up UNION ALL subqueries, in
> 8.2, and has been broken ever since.

I was defining 8.2 as "recent" ;-)

Seriously, I think what this shows is that piecemeal pullup is wrong in
principle, and that the right approach is always to concat the
subquery's rtable in toto to the upper level, and then go from there on
adjusting varnos. Do you want to look into that approach?

regards, tom lane

Tom Lane wrote:
> Seriously, I think what this shows is that piecemeal pullup is wrong in
> principle, and that the right approach is always to concat the
> subquery's rtable in toto to the upper level, and then go from there on
> adjusting varnos. Do you want to look into that approach?

You mean like pull_up_simple_subquery() does? Yeah, I can try that. This
isn't really something I'm familiar with, but it's great exercise :-).

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas wrote:
> Tom Lane wrote:
>> Seriously, I think what this shows is that piecemeal pullup is wrong in
>> principle, and that the right approach is always to concat the
>> subquery's rtable in toto to the upper level, and then go from there on
>> adjusting varnos. Do you want to look into that approach?
>
> You mean like pull_up_simple_subquery() does? Yeah, I can try that. This
> isn't really something I'm familiar with, but it's great exercise :-).

Here we go. Now that I see it, I do like this approach better.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

"Heikki Linnakangas" <heikki(at)enterprisedb(dot)com> writes:
> Here we go. Now that I see it, I do like this approach better.

Hm, the "Assert(rte->subquery != NULL)" doesn't seem right ...
couldn't there be non-RTE_SUBQUERY rtes in the child? I think the
original coding was guaranteed to visit only subquery-type RTEs
but I'm much less convinced about this one. It might
be better to say
if (rte->rtekind == RTE_SUBQUERY)
IncrementVarSublevelsUp(...);

Or maybe it's okay; I'm too lazy to recheck the representation of
UNION ALL right now.

regards, tom lane

Tom Lane wrote:
> Hm, the "Assert(rte->subquery != NULL)" doesn't seem right ...
> couldn't there be non-RTE_SUBQUERY rtes in the child? I think the
> original coding was guaranteed to visit only subquery-type RTEs
> but I'm much less convinced about this one. It might
> be better to say
> if (rte->rtekind == RTE_SUBQUERY)
> IncrementVarSublevelsUp(...);
>
> Or maybe it's okay; I'm too lazy to recheck the representation of
> UNION ALL right now.

Oh, indeed it's not okay. The original UNION ALL view is a prime example
of that. I didn't notice because I was testing without assertions.

Hmm, do we need the copyObject() call for non-subquery RTEs? I'm
guessing no, because they're not modified.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

"Heikki Linnakangas" <heikki(at)enterprisedb(dot)com> writes:
> Tom Lane wrote:
>> Hm, the "Assert(rte->subquery != NULL)" doesn't seem right ...
>> couldn't there be non-RTE_SUBQUERY rtes in the child?

> Oh, indeed it's not okay. The original UNION ALL view is a prime example
> of that. I didn't notice because I was testing without assertions.

Boo ...

> Hmm, do we need the copyObject() call for non-subquery RTEs? I'm
> guessing no, because they're not modified.

Probably not. But it strikes me that there's another sin of omission
here: function and values RTEs need to be tweaked too, because they
contain expressions thst could have uplevel Vars in them. I'm not
certain such RTEs could appear at top level in a UNION query, but I'm
not sure they couldn't either.

I think I'd recommend continuing to copy the RTE unconditionally,
because in the cases where it's not going to be modified, there's
not enough substructure to make this expensive.

regards, tom lane

Tom Lane wrote:
> Probably not. But it strikes me that there's another sin of omission
> here: function and values RTEs need to be tweaked too, because they
> contain expressions thst could have uplevel Vars in them. I'm not
> certain such RTEs could appear at top level in a UNION query, but I'm
> not sure they couldn't either.

Hmm. Maybe through a rewrite or something?

We should use range_table_walker, which knows how to descend into all
kinds of RTEs...

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

"Heikki Linnakangas" <heikki(at)enterprisedb(dot)com> writes:
> We should use range_table_walker, which knows how to descend into all
> kinds of RTEs...

Yeah, I had been thinking of that but didn't see an easy way to apply
it; of course the answer is to use IncrementVarSublevelsUp_walker
directly as you have here.

Looks good except IncrementVarSublevelsUp_rtable really ought to have
a comment. Please fix that, commit, and backpatch as needed.

regards, tom lane

Tom Lane wrote:
> "Heikki Linnakangas" <heikki(at)enterprisedb(dot)com> writes:
>> We should use range_table_walker, which knows how to descend into all
>> kinds of RTEs...
>
> Yeah, I had been thinking of that but didn't see an easy way to apply
> it; of course the answer is to use IncrementVarSublevelsUp_walker
> directly as you have here.
>
> Looks good except IncrementVarSublevelsUp_rtable really ought to have
> a comment. Please fix that, commit, and backpatch as needed.

Done.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com