Noah, Andres, * Andres Freund (and...@anarazel.de) wrote: > On 2015-07-09 01:28:28 -0400, Noah Misch wrote: > > > - Keep the OID check, shouldn't hurt to have it > > > > What benefit is left? > > A bit of defense in depth. We execute user defined code in COPY > (e.g. BEFORE triggers). That user defined code could very well replace > the relation. Now I think right now that'd happen late enough, so the > second lookup already happened. But a bit more robust defense against > that sounds good to me.
Attached patch keeps the relation locked, fully qualifies it when
building up the query, and uses list_member_oid() to check that the
relation's OID ends up in the resulting relationOids list (to address
Noah's point that the planner doesn't guarantee the ordering; I doubt
that list will ever be more than a few entries long).
Also removes the misguided Assert().
Barring objections, I'll commit this (and backpatch to 9.5) tomorrow.
Thanks!
Stephen
From 66ce76c9816d68c58809fa5962f05a2c989ac104 Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfr...@snowman.net>
Date: Thu, 9 Jul 2015 17:01:33 -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 8904676..47dd3ac 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 4073c1b..351742c 100644
--- a/src/test/regress/expected/rowsecurity.out
+++ b/src/test/regress/expected/rowsecurity.out
@@ -2670,7 +2670,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
@@ -2681,6 +2681,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;
@@ -2729,6 +2776,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;
--
-- Clean up objects
--
diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
index fdd9b89..7eb1b9e 100644
--- a/src/test/regress/sql/rowsecurity.sql
+++ b/src/test/regress/sql/rowsecurity.sql
@@ -1024,7 +1024,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
@@ -1033,6 +1033,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;
@@ -1086,6 +1134,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
--
1.9.1
signature.asc
Description: Digital signature
