Re: sepgsql and materialized views - Mailing list pgsql-hackers

Thanks for your introduction. It made me almost clear.

From selinux perspective, it does not switch user's privilege even when
query scans underlying tables because it has no ownership concept.
The current implementation does not make a problem because all the
MV refresh shall be done in a particular user session, thus, it is also
associated with a particular security label if sepgsql is enabled.
So, as you introduced, all we need to do is identical with SELECT ...
INTO and CREATE TABLE AS from selinux perspective also.

> I think these are issues require vigilance.  I hadn't really
> thought through all of this, but since the creation and refresh
> work off of the existing VIEW code, and the querying works off of
> the existing TABLE code, the existing security mechanisms tend to
> come into play by default.
>
If we will try to support refresh MV out of user session like what
autovacuum is doing towards vacuum by hand, probably,
a reasonable design is applying a pseudo session user-id come
from ownership of MV. I think, similar idea can be applied to
apply a temporary pseudo security label, as long as it allows
extensions to get control around these processed.
Anyway, I'm inclined to be optimistic about this possible issue
as long as extension can get necessary support such as new
hooks.

I think it is a reasonable alternative to apply db_table object
class for materialized-view, right now, because it performs as
if we are doing onto regular tables.
However, one exception is here; how to control privilege to
refresh the MV. Of course, db_table class does not have
"refresh" operation. Even though the default permission checks
its ownership (or superuser) at ExecRefreshMatView, but nothing
are checked in extension side.
(Of course, it will need a new hook around here, also.)

So, an ideal solution is to add db_materialized_view
(or db_matview in short) object class in selinux world, then
we checks "refresh" permission of MV.
However, it takes more time to have discussion in selinux
community then shipped to distributions.

So, I'd like to review two options.
1) we uses db_table object class for materialized-views for
a while, until selinux-side become ready. Probably, v9.3 will
use db_table class then switched at v9.4.
2) we uses db_materialized_view object class from the
begining, but its permission checks are ignored because
installed security policy does not support this class yet.

My preference is 2), even though we cannot apply label
based permission checks until selinux support it, because
1) makes troubles when selinux-side become ready to
support new db_materialized_view class. Even though
policy support MV class, working v9.3 will ignore the policy.

Let me ask selinux folks about this topic also.

Thanks,

2013/2/5 Kevin Grittner <kgrittn@ymail.com>:
> Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>
>> Let me confirm a significant point. Do you never plan a feature
>> that allows to update/refresh materialized-views out of user
>> session?
>
> Currently only the owner of the MV or a database superuser can
> refresh it, and the refresh is run with the permissions of the MV
> owner.  The main change to that I see is that the owner could
> establish a policy of automatic updates to the MV based on changes
> to the underlying tables, with a timing established by the MV owner
> or a database superuser.
>
>> I had an impression on asynchronous update of MV something like
>> a feature that moves data from regular tables to MV with
>> batch-jobs in mid-night, but under the privilege that bypass
>> regular permission checks.
>
> I would expect it to be more a matter of being based on the
> authority of the MV owner.  That raises interesting questions about
> what happens if a permission which allowed the MV to be defined is
> revoked from the owner, or if the MV is altered to have an owner
> without permission to access the underlying data.  With the current
> patch, if the owner is changed to a user who does not have rights
> to access the underlying table, a "permission denied" error is
> generated when that new owner tries to refresh the MV.
>
>> It it is never planned, my concern was pointless.
>
> I think these are issues require vigilance.  I hadn't really
> thought through all of this, but since the creation and refresh
> work off of the existing VIEW code, and the querying works off of
> the existing TABLE code, the existing security mechanisms tend to
> come into play by default.
>
>> My concern is future development that allows to update/refresh MV
>> asynchronously, out of privilege control.
>
> While it has not yet been defined, my first reaction is that it
> should happen under privileges of the MV owner.
>
>> As long as all the update/refresh operation is under privilege
>> control with user-id/security label of the current session, here
>> is no difference from regular writer operation of tables with
>> contents read from other tables.
>
> Again, it's just my first impression, but I think that the
> permissions of the current session would control whether the
> operation would be allowed on the underlying tables, but the
> permissions of the MV owner would control replication to the MV.
>
>> BTW, please clarify expected behavior in case when MV contains
>> WHERE clause that returns different result depending on privilege
>> of current session, such as:
>>   ... WHERE underlying_table.uname = CURRENT_USER
>
> Ah, good question.  Something else I hadn't thought about.  When I
> read that I was afraid that the current patch left a security hole
> where if the owner didn't have rights to populate the MV with
> something, it could still get there if a database superuser ran
> REFRESH, but it seems to do what I would think is the right thing.
> With kgrittn as a superuser and bob as a normal user:
>
> test=# set role bob;
> SET
> test=> select * from t;
> ERROR:  permission denied for relation t
> test=> reset role;
> RESET
> test=# alter materialized view tm owner to bob;
> ALTER MATERIALIZED VIEW
> test=# set role bob;
> SET
> test=> refresh MATERIALIZED VIEW tm;
> ERROR:  permission denied for relation t
> test=> reset role;
> RESET
> test=# refresh MATERIALIZED VIEW tm;
> ERROR:  permission denied for relation t
> test=# alter materialized view tm owner to kgrittn;
> ALTER MATERIALIZED VIEW
> test=# refresh MATERIALIZED VIEW tm;
> REFRESH MATERIALIZED VIEW
>
> So it seems to run the refresh as the owner, not under authority of
> the session.
>
>> It seems to me this MV saves just a snapshot from a standpoint of
>> a particular user who refreshed this MV; typically its owner.
>
> Exactly.  Typically a summary of a snapshot of underlying detail.
>
>> If bob has privilege to reference this MV, he will see rows to be
>> visible for alice. Of course, it does not contradictory, because
>> all alice doing is just writing data she can see into a table
>> being visible for public.
>
> Right.  From a security perspective it is rather like alice running
> CREATE TABLE AS.  And again, it is worth remembering that the usual
> reason for creating one of these is to summarize data, to support
> quick generation of statistical reports, for example.
>
>> Even if MV's contents were moved in out of privilege controls,
>> we can ensure the current user has rights to reference data of
>> MV, as long as he has privileges to reference underlying data
>> source.
>
> I don't think it should have anything to do with authority to the
> underlying tables from which the data is selected.
>
>> On the other hand, it can make problems if some internal stuff
>> moves data from regular tables with "confidential" label into MV
>> with "unconfidential" label; that works official information leak
>> channel.
>
> I don't see that it opens any new vulnerabilities compared to
> INSERT ... SELECT or CREATE TABLE AS.
>
>> Only point I'm concerned about is whether we will support a
>> feature that refresh materialized-view without appropriate
>> privilege control, or not.
>
> I guess the question is whether it makes sense to support these
> under sepgsql using table access labels, or whether we need new
> labels and the feature is not usable in environments without those
> labels.  That's what I"m not clear on either.  I first did the
> patch assuming a new label, but changed it based on feedback from
> Robert suggesting we should use the table labels.  I can change it
> back if you like.
>
> -Kevin



-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>