row_security GUC, BYPASSRLS

On Thu, Sep 10, 2015 at 03:23:13PM -0400, Stephen Frost wrote:
> First off, thanks again for your review and comments on RLS. Hopefully
> this addresses the last remaining open item from that review.

Item (3a), too, remains open.

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

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

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

SECURITY DEFINER execution blocks SET ROLE, SET SESSION AUTHORIZATION, and
sometimes "GRANT role1 TO role2". Otherwise, it works like regular execution.
Adding exceptions, particularly silent behavior changes as opposed to hard
errors, is a big deal.

When I wrote the 2015-07-03 review, I had in mind to just let policies run
normally during referential integrity checks. Table owners, which can already
break referential integrity with triggers and rules, would be responsible for
crafting policies accordingly.

Pondering it afresh this week, I see now that row_security=force itself is the
problem. It's a new instance of the maligned "behavior-changing GUC".
Function authors will not consistently test the row_security=force case, and
others can run the functions under row_security=force and get novel results.
This poses a reliability and security threat. While row_security=force is
handy for testing, visiting a second role for testing purposes is a fine
replacement. Let's remove "force", making row_security a config_bool. If
someone later desires to revive the capability, a DDL-specified property of
each policy would be free from these problems.

On a related topic, check_enable_rls() has two kinds of RLS bypass semantics.
Under row_security=off|on, superusers bypass every policy, and table owners
bypass policies of their own tables. Under row_security=off only, roles with
BYPASSRLS privilege additionally bypass every policy; BYPASSRLS doesn't affect
semantics under row_security=on. Is that distinction a good thing? Making
BYPASSRLS GUC-independent, like superuser bypass, would simplify things for
the user and would make row_security no longer a behavior-changing GUC.
row_security would come to mean simply "if a policy would otherwise attach to
one of my queries, raise an error instead." Again, if you have cause to
toggle BYPASSRLS, use multiple roles. Implementing that with GUCs creates
harder problems.

In summary, I propose to remove row_security=force and make BYPASSRLS
effective under any setting of the row_security GUC.

Some past, brief discussion leading to the current semantics:
http://www.postgresql.org/message-id/CA+TgmoYA=uixXmN390SFgfQgVmLL-As5bJaL0oM7yrpPVwNPxQ@mail.gmail.com
http://www.postgresql.org/message-id/CAKRt6CQnghzWUGwb5Pkwg5gfXwd+-joy8MmMEnqh+O6vpLYzfA@mail.gmail.com
http://www.postgresql.org/message-id/20150729130927.GS3587@tamriel.snowman.net