Re: more RLS oversights - Mailing list pgsql-hackers

Noah,

First off, thanks again for your review and comments on RLS.  Hopefully
this addresses the last remaining open item from that review.

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

I believe the original intent was to only include the queries which were
against the primary table, but reviewing what ends up happening, that
clearly doesn't actually make sense.  As you note below, this is only
to address the 'row_security = force' mode, which I suspect is why it
wasn't picked up in earlier testing.

> 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.)

You're right, the reason for it to exist was to address the
'row_security = force' case.  I spent a bit of time considering that and
have come up with the attached for consideration (which needs additional
work before being committed, but should get the idea across clearly).

This removes the notion of SECURITY_ROW_LEVEL_DISABLED entirely and
instead ignores 'row_security = force' if InLocalUserIdChange() is true.
Further, this changes row_security to have GUC_NOT_WHILE_SEC_REST set.
I didn't set GUC_NO_RESET_ALL for it though as the default is for
row_security to be 'on'.

The biggest drawback that I can see to this is that users won't be able
to set the row_security GUC inside of a security definer function.  I
can imagine use-cases where someone might want to change it in a
security definer function but it doesn't seem like they're valuable
enough to counter your argument (which I agree with) that
SECURITY_ROW_LEVEL_DISABLED is a security landmine.

Another approach which I considered was to add a new 'RLS_IGNORE_FORCE'
flag, which would, again, ignore 'row_security=force' when set (meaning
owners would bypass RLS regardless of the actual row_security setting).
I didn't like adding that to sec_context though and it wasn't clear
where a good place would be.  Further, it seems like it'd be nice to
have a generic flag that says "we're running an internal referential
integrity operation, don't get in the way", though that could also be a
risky flag to have.  Such an approach would allow us to differentiate RI
queries from operations run under a security definer function though.

Thoughts?

Thanks!

Stephen