Re: row_security GUC, BYPASSRLS

* Noah Misch (noah(at)leadboat(dot)com) wrote:
> On Mon, Sep 21, 2015 at 09:30:15AM -0400, Stephen Frost wrote:
> > * Noah Misch (noah(at)leadboat(dot)com) wrote:
> > > Right now, if a BYPASSRLS user creates a SECURITY DEFINER function, any caller
> > > can change that function's behavior by toggling the GUC. Users won't test
> > > accordingly; better to have just one success-case behavior.
> >
> > I agree that's not good, though the function definer could set the
> > row_security GUC on the function, no? Similar to how we encourage
> > setting of search_path, we should be encouraging a similar approach to
> > anything which might be security relevant.
>
> Functions can do that. New features should not mimic search_path in their
> demands on SECURITY DEFINER authors.

I'm not convinced that having such a limitation on new features would be
a good thing. What was missing here was a safe default.

> > Are you planning to handle the ALTER TABLE .. FORCE ROW SECURITY (#3) as
> > well? I have no complaints if so; just want to make sure we aren't
> > doing double-work during this crunch time and didn't see your name
> > listed next to it, but the nearby thread seemed to imply you were
> > looking at it.
>
> I'm not.

I'll work on it then, if we can get agreement as to how it will work.

> > One item which wasn't discussed, that I recall, is just how it will work
> > without SECURITY_ROW_LEVEL_DISABLED, or something similar, to
> > differentiate when internal referencial integrity queries are being run,
> > which should still bypass RLS (even in the FORCE ROW SECURITY case), and
> > when regular or SECURITY DEFINER originated queries are being run.
>
> If the table owner enables FORCE ROW SECURITY, policies will affect
> referential integrity queries. Choose policies accordingly. For example,
> given only ON UPDATE NO ACTION constraints, it would be no problem to set
> owner-affecting policies for INSERT, UPDATE and/or DELETE.

Perhaps I'm not following correctly, but the above doesn't look correct
to me. An ON UPDATE NO ACTION constraint would run a query against the
referring table (which has FORCE ROW SECURITY set, perhaps by mistake
after a debugging session of the owner, with a policy that does not
allow any records to be seen by the owner), fail to find any rows, and
conclude that no error needs to be thrown, resulting in the referring
table having records which refer to keys in the referred-to table that
no longer exist (the UPDATE having changed them).

As a test, I hacked the pg_class_ownercheck() case in check_enable_rls()
to return RLS_ENABLED always. Then did this:

CREATE ROLE r1;
CREATE ROLE r2;
CREATE TABLE t1 (c1 INT PRIMARY KEY);
CREATE TABLE t2 (c1 INT REFERENCES t1 ON UPDATE NO ACTION);
ALTER TABLE t1 OWNER TO r1;
ALTER TABLE t2 OWNER TO r2;
GRANT SELECT ON t1 TO r2;
INSERT INTO t1 VALUES (1);
INSERT INTO t2 VALUES (1);
ALTER TABLE t2 ENABLE ROW LEVEL SECURITY;
UPDATE t1 SET c1 = 2;
ALTER TABLE t2 DISABLE ROW LEVEL SECURITY;
TABLE t1;
TABLE t2;

With results:

=*# TABLE t1;
c1
----
2
(1 row)

=*# TABLE t2;
c1
----
1
(1 row)

This would lead to trivial to cause (and likely hard to diagnose)
referential integrity data corruption issues. I find that a very hard
pill to swallow in favor of a theoretical concern that new code may open
avenues of exploit for a new security context mode to bypass RLS when
doing internal referential integrity checks. Further, it changes this
additional capability, which was agreed would be added to offset removal
of the 'row_security = force' option, from useful to downright
dangerous.

Hopefully, I'm simply misunderstanding your proposal for the
FORCE ROW LEVEL SECURITY option.

Thanks!

Stephen