Re: more RLS oversights

Noah,

* Noah Misch (noah@leadboat.com) wrote:
> On Wed, Feb 25, 2015 at 11:37:24PM -0500, Stephen Frost wrote:
> > * Stephen Frost (sfrost@snowman.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