From 5d179a28fb4c819f3812c40fa7e626b1d3081982 Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Mon, 27 Jul 2015 16:48:26 -0400 Subject: [PATCH] Improve RLS handling in copy.c To avoid a race condition where the relation being COPY'd could be changed into a view or otherwise modified, keep the original lock on the relation. Further, fully qualify the relation when building the query up. Also remove the poorly thought-out Assert() and check the entire relationOids list as, post-RLS, there can certainly be multiple relations involved and the planner does not guarantee their ordering. Per discussion with Noah and Andres. Back-patch to 9.5 where RLS was introduced. --- src/backend/commands/copy.c | 45 ++++++++++++-------- src/test/regress/expected/rowsecurity.out | 50 +++++++++++++++++++++- src/test/regress/sql/rowsecurity.sql | 51 ++++++++++++++++++++++- 3 files changed, 126 insertions(+), 20 deletions(-) diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index 8904676609..47dd3accaf 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -896,8 +896,12 @@ DoCopy(const CopyStmt *stmt, const char *queryString, uint64 *processed) target->val = (Node *) cr; target->location = 1; - /* Build FROM clause */ - from = stmt->relation; + /* + * Build RangeVar for from clause, fully qualified based on the + * relation which we have opened and locked. + */ + from = makeRangeVar(get_namespace_name(RelationGetNamespace(rel)), + RelationGetRelationName(rel), -1); /* Build query */ select = makeNode(SelectStmt); @@ -906,8 +910,13 @@ DoCopy(const CopyStmt *stmt, const char *queryString, uint64 *processed) query = (Node *) select; - /* Close the handle to the relation as it is no longer needed. */ - heap_close(rel, (is_from ? RowExclusiveLock : AccessShareLock)); + /* + * Close the relation for now, but keep the lock on it to prevent + * changes between now and when we start the query-based COPY. + * + * We'll reopen it later as part of the query-based COPY. + */ + heap_close(rel, NoLock); rel = NULL; } } @@ -1407,25 +1416,25 @@ BeginCopy(bool is_from, plan = planner(query, 0, NULL); /* - * If we were passed in a relid, make sure we got the same one back - * after planning out the query. It's possible that it changed - * between when we checked the policies on the table and decided to - * use a query and now. + * With row level security and a user using "COPY relation TO", we + * have to convert the "COPY relation TO" to a query-based COPY (eg: + * "COPY (SELECT * FROM relation) TO"), to allow the rewriter to add + * in any RLS clauses. + * + * When this happens, we are passed in the relid of the originally + * found relation (which we have locked). As the planner will look + * up the relation again, we double-check here to make sure it found + * the same one that we have locked. */ if (queryRelId != InvalidOid) { - Oid relid = linitial_oid(plan->relationOids); - /* - * There should only be one relationOid in this case, since we - * will only get here when we have changed the command for the - * user from a "COPY relation TO" to "COPY (SELECT * FROM - * relation) TO", to allow row level security policies to be - * applied. + * Note that with RLS involved there may be multiple relations, + * and while the one we need is almost certainly first, we don't + * make any guarantees of that in the planner, so check the whole + * list and make sure we find the original relation. */ - Assert(list_length(plan->relationOids) == 1); - - if (relid != queryRelId) + if (!list_member_oid(plan->relationOids, queryRelId)) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("relation referenced by COPY statement has changed"))); diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out index e7c242cd22..72361e82a5 100644 --- a/src/test/regress/expected/rowsecurity.out +++ b/src/test/regress/expected/rowsecurity.out @@ -2672,7 +2672,7 @@ COPY (SELECT * FROM copy_t ORDER BY a ASC) TO STDOUT WITH DELIMITER ','; --ok 6,1679091c5a880faf6fb5e6087eb1b2dc 8,c9f0f895fb98ab9159f51fd0297e236d 10,d3d9446802a44259755d38e6d163e820 --- Check COPY TO as user without permissions.SET row_security TO OFF; +-- Check COPY TO as user without permissions. SET row_security TO OFF; SET SESSION AUTHORIZATION rls_regress_user2; SET row_security TO OFF; COPY (SELECT * FROM copy_t ORDER BY a ASC) TO STDOUT WITH DELIMITER ','; --fail - insufficient to bypass rls @@ -2683,6 +2683,53 @@ ERROR: permission denied for relation copy_t SET row_security TO FORCE; COPY (SELECT * FROM copy_t ORDER BY a ASC) TO STDOUT WITH DELIMITER ','; --fail - permission denied ERROR: permission denied for relation copy_t +-- Check COPY relation TO; keep it just one row to avoid reordering issues +RESET SESSION AUTHORIZATION; +SET row_security TO ON; +CREATE TABLE copy_rel_to (a integer, b text); +CREATE POLICY p1 ON copy_rel_to USING (a % 2 = 0); +ALTER TABLE copy_rel_to ENABLE ROW LEVEL SECURITY; +GRANT ALL ON copy_rel_to TO rls_regress_user1, rls_regress_exempt_user; +INSERT INTO copy_rel_to VALUES (1, md5('1')); +-- Check COPY TO as Superuser/owner. +RESET SESSION AUTHORIZATION; +SET row_security TO OFF; +COPY copy_rel_to TO STDOUT WITH DELIMITER ','; +1,c4ca4238a0b923820dcc509a6f75849b +SET row_security TO ON; +COPY copy_rel_to TO STDOUT WITH DELIMITER ','; +1,c4ca4238a0b923820dcc509a6f75849b +SET row_security TO FORCE; +COPY copy_rel_to TO STDOUT WITH DELIMITER ','; +-- Check COPY TO as user with permissions. +SET SESSION AUTHORIZATION rls_regress_user1; +SET row_security TO OFF; +COPY copy_rel_to TO STDOUT WITH DELIMITER ','; --fail - insufficient to bypass rls +ERROR: insufficient privilege to bypass row security. +SET row_security TO ON; +COPY copy_rel_to TO STDOUT WITH DELIMITER ','; --ok +SET row_security TO FORCE; +COPY copy_rel_to TO STDOUT WITH DELIMITER ','; --ok +-- Check COPY TO as user with permissions and BYPASSRLS +SET SESSION AUTHORIZATION rls_regress_exempt_user; +SET row_security TO OFF; +COPY copy_rel_to TO STDOUT WITH DELIMITER ','; --ok +1,c4ca4238a0b923820dcc509a6f75849b +SET row_security TO ON; +COPY copy_rel_to TO STDOUT WITH DELIMITER ','; --ok +SET row_security TO FORCE; +COPY copy_rel_to TO STDOUT WITH DELIMITER ','; --ok +-- Check COPY TO as user without permissions. SET row_security TO OFF; +SET SESSION AUTHORIZATION rls_regress_user2; +SET row_security TO OFF; +COPY copy_rel_to TO STDOUT WITH DELIMITER ','; --fail - permission denied +ERROR: permission denied for relation copy_rel_to +SET row_security TO ON; +COPY copy_rel_to TO STDOUT WITH DELIMITER ','; --fail - permission denied +ERROR: permission denied for relation copy_rel_to +SET row_security TO FORCE; +COPY copy_rel_to TO STDOUT WITH DELIMITER ','; --fail - permission denied +ERROR: permission denied for relation copy_rel_to -- Check COPY FROM as Superuser/owner. RESET SESSION AUTHORIZATION; SET row_security TO OFF; @@ -2731,6 +2778,7 @@ COPY copy_t FROM STDIN; --fail - permission denied. ERROR: permission denied for relation copy_t RESET SESSION AUTHORIZATION; DROP TABLE copy_t; +DROP TABLE copy_rel_to CASCADE; -- Check WHERE CURRENT OF SET SESSION AUTHORIZATION rls_regress_user0; CREATE TABLE current_check (currentid int, payload text, rlsuser text); diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql index e86f814314..f588fa2337 100644 --- a/src/test/regress/sql/rowsecurity.sql +++ b/src/test/regress/sql/rowsecurity.sql @@ -1028,7 +1028,7 @@ COPY (SELECT * FROM copy_t ORDER BY a ASC) TO STDOUT WITH DELIMITER ','; --ok SET row_security TO FORCE; COPY (SELECT * FROM copy_t ORDER BY a ASC) TO STDOUT WITH DELIMITER ','; --ok --- Check COPY TO as user without permissions.SET row_security TO OFF; +-- Check COPY TO as user without permissions. SET row_security TO OFF; SET SESSION AUTHORIZATION rls_regress_user2; SET row_security TO OFF; COPY (SELECT * FROM copy_t ORDER BY a ASC) TO STDOUT WITH DELIMITER ','; --fail - insufficient to bypass rls @@ -1037,6 +1037,54 @@ COPY (SELECT * FROM copy_t ORDER BY a ASC) TO STDOUT WITH DELIMITER ','; --fail SET row_security TO FORCE; COPY (SELECT * FROM copy_t ORDER BY a ASC) TO STDOUT WITH DELIMITER ','; --fail - permission denied +-- Check COPY relation TO; keep it just one row to avoid reordering issues +RESET SESSION AUTHORIZATION; +SET row_security TO ON; +CREATE TABLE copy_rel_to (a integer, b text); +CREATE POLICY p1 ON copy_rel_to USING (a % 2 = 0); + +ALTER TABLE copy_rel_to ENABLE ROW LEVEL SECURITY; + +GRANT ALL ON copy_rel_to TO rls_regress_user1, rls_regress_exempt_user; + +INSERT INTO copy_rel_to VALUES (1, md5('1')); + +-- Check COPY TO as Superuser/owner. +RESET SESSION AUTHORIZATION; +SET row_security TO OFF; +COPY copy_rel_to TO STDOUT WITH DELIMITER ','; +SET row_security TO ON; +COPY copy_rel_to TO STDOUT WITH DELIMITER ','; +SET row_security TO FORCE; +COPY copy_rel_to TO STDOUT WITH DELIMITER ','; + +-- Check COPY TO as user with permissions. +SET SESSION AUTHORIZATION rls_regress_user1; +SET row_security TO OFF; +COPY copy_rel_to TO STDOUT WITH DELIMITER ','; --fail - insufficient to bypass rls +SET row_security TO ON; +COPY copy_rel_to TO STDOUT WITH DELIMITER ','; --ok +SET row_security TO FORCE; +COPY copy_rel_to TO STDOUT WITH DELIMITER ','; --ok + +-- Check COPY TO as user with permissions and BYPASSRLS +SET SESSION AUTHORIZATION rls_regress_exempt_user; +SET row_security TO OFF; +COPY copy_rel_to TO STDOUT WITH DELIMITER ','; --ok +SET row_security TO ON; +COPY copy_rel_to TO STDOUT WITH DELIMITER ','; --ok +SET row_security TO FORCE; +COPY copy_rel_to TO STDOUT WITH DELIMITER ','; --ok + +-- Check COPY TO as user without permissions. SET row_security TO OFF; +SET SESSION AUTHORIZATION rls_regress_user2; +SET row_security TO OFF; +COPY copy_rel_to TO STDOUT WITH DELIMITER ','; --fail - permission denied +SET row_security TO ON; +COPY copy_rel_to TO STDOUT WITH DELIMITER ','; --fail - permission denied +SET row_security TO FORCE; +COPY copy_rel_to TO STDOUT WITH DELIMITER ','; --fail - permission denied + -- Check COPY FROM as Superuser/owner. RESET SESSION AUTHORIZATION; SET row_security TO OFF; @@ -1090,6 +1138,7 @@ COPY copy_t FROM STDIN; --fail - permission denied. RESET SESSION AUTHORIZATION; DROP TABLE copy_t; +DROP TABLE copy_rel_to CASCADE; -- Check WHERE CURRENT OF SET SESSION AUTHORIZATION rls_regress_user0; -- 2.40.0