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.

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 88b6edc0cd9b60b5a10ee3bbcc31c89264a65a92..783c9f4c42e32465cb94310740af1ff17cdb8816 100644 (file)
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -943,8 +943,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);
@@ -953,8 +957,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;
                }
        }
@@ -1498,25 +1507,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 f7db76bac212324e563404e382fce510eccdd60f..576bcafa49693caaa95c12383d48c6dcab93eb65 100644 (file)
--- a/src/test/regress/expected/rowsecurity.out
+++ b/src/test/regress/expected/rowsecurity.out
@@ -2607,7 +2607,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
@@ -2618,6 +2618,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;

diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
index 310a1050ccd6f8b869563fe40fa7f427aba28161..86909f88e5ab2a15927a7f5a379128071fa1ffdd 100644 (file)
--- a/src/test/regress/sql/rowsecurity.sql
+++ b/src/test/regress/sql/rowsecurity.sql
@@ -1045,7 +1045,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
@@ -1054,6 +1054,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;
@@ -1107,6 +1155,7 @@ COPY copy_t FROM STDIN; --fail - permission denied.
 
 RESET SESSION AUTHORIZATION;
 DROP TABLE copy_t;
+DROP TABLE copy_rel_to CASCADE;
 
 --
 -- Clean up objects