Re: [v9.2] Fix Leaky View Problem

2011/12/9 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Thu, Dec 8, 2011 at 5:17 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>