Re: more RLS oversights

Noah,

* Noah Misch (noah(at)leadboat(dot)com) wrote:
> On Wed, Feb 25, 2015 at 11:37:24PM -0500, Stephen Frost wrote:
> > * Stephen Frost (sfrost(at)snowman(dot)net) wrote:
> > > I agree that it's great that we're catching issues prior to when the
> > > feature is released and look forward to anything else you (or anyone
> > > else!) finds.
> >
> > I've pushed a fix for this. Please let me know if you see any other
> > issues or run into any problems.
>
> (1) CreatePolicy() and AlterPolicy() omit to call assign_expr_collations() on
> the node trees. Test case:

Will fix.

> (2) CreatePolicy() and AlterPolicy() omit to create a pg_shdepend entry for
> each role in the TO clause. Test case:

Will fix.

> > +static void
> > +dumpPolicy(Archive *fout, PolicyInfo *polinfo)
> ...
> > + if (polinfo->polqual != NULL)
> > + appendPQExpBuffer(query, " USING %s", polinfo->polqual);
>
> (3) The USING clause needs parentheses; a dump+reload failed like so:

Will fix.

> Add the same parentheses to psql \d output also, keeping that output similar
> to the SQL syntax.

Yup.

> (3a) I found this by hacking the rowsecurity.sql regression test to not drop
> its objects, then running the pg_upgrade test suite. New features that affect
> pg_dump should leave objects in the regression database to test the pg_dump
> support via that suite.

Will fix.

> (4) When DefineQueryRewrite() is about to convert a table to a view, it checks
> the table for features unavailable to views. For example, it rejects tables
> having triggers. It omits to reject tables having relrowsecurity or a
> pg_policy record. Test case:

Will fix.

> > + <para>
> > + Referential integrity checks, such as unique or primary key constraints
> > + and foreign key references, will bypass row security to ensure that
> > + data integrity is maintained. Care must be taken when developing
> > + schemas and row level policies to avoid a "covert channel" leak of
> > + information through these referential integrity checks.
> ...
> > + /*
> > + * Row-level security should be disabled in the case where a foreign-key
> > + * relation is queried to check existence of tuples that references the
> > + * primary-key being modified.
> > + */
> > + temp_sec_context = save_sec_context | SECURITY_LOCAL_USERID_CHANGE;
> > + if (qkey->constr_queryno == RI_PLAN_CHECK_LOOKUPPK
> > + || qkey->constr_queryno == RI_PLAN_CHECK_LOOKUPPK_FROM_PK
> > + || qkey->constr_queryno == RI_PLAN_RESTRICT_DEL_CHECKREF
> > + || qkey->constr_queryno == RI_PLAN_RESTRICT_UPD_CHECKREF)
> > + temp_sec_context |= SECURITY_ROW_LEVEL_DISABLED;
>
> (5) This code does not use SECURITY_ROW_LEVEL_DISABLED for foreign keys with
> CASCADE, SET NULL or SET DEFAULT actions. The associated documentation says
> nothing about this presumably-important distinction.

Agreed, will fix.

> Is SECURITY_ROW_LEVEL_DISABLED mode even needed? We switch users to the owner
> of the FROM-clause table before running an RI query. That means use of this
> mode can only matter under row_security=force, right? I would rest easier if
> this mode went away, because it is a security landmine. If user code managed
> to run in this mode, it would bypass every policy in the database. (I find no
> such vulnerability today, because we use the mode only for parse analysis of
> ri_triggers.c queries.)

That's a very interesting point.. At first blush, I agree, it shouldn't
be necessary. I'll play with it and see if I can get everything to work
properly without it.

> (6) AlterPolicy() calls InvokeObjectPostAlterHook(PolicyRelationId, ...), but
> CreatePolicy() and DropPolicy() lack their respective hook invocations.

Will fix.

> (7) Using an aggregate function in a policy predicate elicits an inapposite
> error message due to use of EXPR_KIND_WHERE for parse analysis. Need a new
> ParseExprKind. Test case:

Will fix.

Thanks a lot for your review! Much appreciated.

Stephen