Re: more RLS oversights

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:

begin;
set row_security = force;
create table t (c) as values ('bar'::text);
create policy p on t using (c < ('foo'::text COLLATE "C"));
alter table t enable row level security;
table pg_policy; -- note ":inputcollid 0"
select * from t; -- ERROR: could not determine which collation ...
rollback;

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

begin;
create role alice;
create table t (c) as values ('bar'::text);
grant select on table t to alice;
create policy p on t to alice using (true);
select refclassid::regclass, refobjid, refobjsubid, deptype
from pg_depend where classid = 'pg_policy'::regclass;
select refclassid::regclass, refobjid, deptype
from pg_shdepend where classid = 'pg_policy'::regclass;
savepoint q; drop role alice; rollback to q;
revoke all on table t from alice;
\d t
drop role alice;
\d t
rollback;

> +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:

pg_restore: [archiver (db)] could not execute query: ERROR: syntax error at or near "CASE"
LINE 2: CASE
^
Command was: CREATE POLICY "p2" ON "category" FOR ALL TO PUBLIC USING
CASE
WHEN ("current_user"() = 'rls_regress_user1'::"name") THE...

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

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

(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:

begin;
set row_security = force;
create table t (c) as select * from generate_series(1,5);
create policy p on t using (c % 2 = 1);
alter table t enable row level security;
table t;
truncate t;
create rule "_RETURN" as on select to t do instead
select * from generate_series(1,5) t0(c);
table t;
select polrelid::regclass from pg_policy;
select relrowsecurity from pg_class where oid = 't'::regclass;
rollback;

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

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

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

(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:

begin;
set row_security = force;
create table t (c) as values ('bar'::text);
-- ERROR: aggregate functions are not allowed in WHERE
create policy p on t using (max(c));
rollback;