summary | shortlog | log | commit | commitdiff | tree
raw | patch | inline | side by side (parent: eeb4eee)
raw | patch | inline | side by side (parent: eeb4eee)
| author | Dean Rasheed <dean.a.rasheed@gmail.com> | |
| Mon, 7 Aug 2023 08:28:47 +0000 (09:28 +0100) | ||
| committer | Dean Rasheed <dean.a.rasheed@gmail.com> | |
| Mon, 7 Aug 2023 08:28:47 +0000 (09:28 +0100) |
If MERGE executes an UPDATE action on a table with row-level security,
the code incorrectly applied the WITH CHECK clauses from the target
table's INSERT policies to new rows, instead of the clauses from the
table's UPDATE policies. In addition, it failed to check new rows
against the target table's SELECT policies, if SELECT permissions were
required (likely to always be the case).
In addition, if MERGE executes a DO NOTHING action for matched rows,
the code incorrectly applied the USING clauses from the target table's
DELETE policies to existing target tuples. These policies were applied
as checks that would throw an error, if they did not pass.
Fix this, so that a MERGE UPDATE action applies the same RLS policies
as a plain UPDATE query with a WHERE clause, and a DO NOTHING action
does not apply any RLS checks (other than adding clauses from SELECT
policies to the join).
Back-patch to v15, where MERGE was introduced.
Dean Rasheed, reviewed by Stephen Frost.
Security: CVE-2023-39418
the code incorrectly applied the WITH CHECK clauses from the target
table's INSERT policies to new rows, instead of the clauses from the
table's UPDATE policies. In addition, it failed to check new rows
against the target table's SELECT policies, if SELECT permissions were
required (likely to always be the case).
In addition, if MERGE executes a DO NOTHING action for matched rows,
the code incorrectly applied the USING clauses from the target table's
DELETE policies to existing target tuples. These policies were applied
as checks that would throw an error, if they did not pass.
Fix this, so that a MERGE UPDATE action applies the same RLS policies
as a plain UPDATE query with a WHERE clause, and a DO NOTHING action
does not apply any RLS checks (other than adding clauses from SELECT
policies to the join).
Back-patch to v15, where MERGE was introduced.
Dean Rasheed, reviewed by Stephen Frost.
Security: CVE-2023-39418
index 2a5fec8d017e5fe38dc4651c3cceb6743add5345..5005d8c0d12ca28c5f068f549f5ebd2c890cdaa8 100644 (file)
* UPDATE/DELETE RLS policies. If those checks fail, we throw an
* error.
*
- * The WITH CHECK quals are applied in ExecUpdate() and hence we need
- * not do anything special to handle them.
+ * The WITH CHECK quals for UPDATE RLS policies are applied in
+ * ExecUpdateAct() and hence we need not do anything special to handle
+ * them.
*
* NOTE: We must do this after WHEN quals are evaluated, so that we
* check policies only when they matter.
*/
- if (resultRelInfo->ri_WithCheckOptions)
+ if (resultRelInfo->ri_WithCheckOptions && commandType != CMD_NOTHING)
{
ExecWithCheckOptions(commandType == CMD_UPDATE ?
WCO_RLS_MERGE_UPDATE_CHECK : WCO_RLS_MERGE_DELETE_CHECK,
index 5c3fe4eda285f7c746d9188a1c8d646df6b6a1cd..b1620e4625d2d1f19dd5cf4bd380eaabe9bdd93a 100644 (file)
* and set them up so that we can enforce the appropriate policy depending
* on the final action we take.
*
- * We already fetched the SELECT policies above.
+ * We already fetched the SELECT policies above, to check existing rows,
+ * but we must also check that new rows created by UPDATE actions are
+ * visible, if SELECT rights are required for this relation. We don't do
+ * this for INSERT actions, since an INSERT command would only do this
+ * check if it had a RETURNING list, and MERGE does not support RETURNING.
*
* We don't push the UPDATE/DELETE USING quals to the RTE because we don't
* really want to apply them while scanning the relation since we don't
*/
if (commandType == CMD_MERGE)
{
- List *merge_permissive_policies;
- List *merge_restrictive_policies;
+ List *merge_update_permissive_policies;
+ List *merge_update_restrictive_policies;
+ List *merge_delete_permissive_policies;
+ List *merge_delete_restrictive_policies;
+ List *merge_insert_permissive_policies;
+ List *merge_insert_restrictive_policies;
/*
* Fetch the UPDATE policies and set them up to execute on the
* existing target row before doing UPDATE.
*/
get_policies_for_relation(rel, CMD_UPDATE, user_id,
- &merge_permissive_policies,
- &merge_restrictive_policies);
+ &merge_update_permissive_policies,
+ &merge_update_restrictive_policies);
/*
* WCO_RLS_MERGE_UPDATE_CHECK is used to check UPDATE USING quals on
*/
add_with_check_options(rel, rt_index,
WCO_RLS_MERGE_UPDATE_CHECK,
- merge_permissive_policies,
- merge_restrictive_policies,
+ merge_update_permissive_policies,
+ merge_update_restrictive_policies,
withCheckOptions,
hasSubLinks,
true);
+ /* Enforce the WITH CHECK clauses of the UPDATE policies */
+ add_with_check_options(rel, rt_index,
+ WCO_RLS_UPDATE_CHECK,
+ merge_update_permissive_policies,
+ merge_update_restrictive_policies,
+ withCheckOptions,
+ hasSubLinks,
+ false);
+
+ /*
+ * Add ALL/SELECT policies as WCO_RLS_UPDATE_CHECK WCOs, to ensure
+ * that the updated row is visible when executing an UPDATE action, if
+ * SELECT rights are required for this relation.
+ */
+ if (perminfo->requiredPerms & ACL_SELECT)
+ {
+ List *merge_select_permissive_policies;
+ List *merge_select_restrictive_policies;
+
+ get_policies_for_relation(rel, CMD_SELECT, user_id,
+ &merge_select_permissive_policies,
+ &merge_select_restrictive_policies);
+ add_with_check_options(rel, rt_index,
+ WCO_RLS_UPDATE_CHECK,
+ merge_select_permissive_policies,
+ merge_select_restrictive_policies,
+ withCheckOptions,
+ hasSubLinks,
+ true);
+ }
+
/*
- * Same with DELETE policies.
+ * Fetch the DELETE policies and set them up to execute on the
+ * existing target row before doing DELETE.
*/
get_policies_for_relation(rel, CMD_DELETE, user_id,
- &merge_permissive_policies,
- &merge_restrictive_policies);
+ &merge_delete_permissive_policies,
+ &merge_delete_restrictive_policies);
+ /*
+ * WCO_RLS_MERGE_DELETE_CHECK is used to check DELETE USING quals on
+ * the existing target row.
+ */
add_with_check_options(rel, rt_index,
WCO_RLS_MERGE_DELETE_CHECK,
- merge_permissive_policies,
- merge_restrictive_policies,
+ merge_delete_permissive_policies,
+ merge_delete_restrictive_policies,
withCheckOptions,
hasSubLinks,
true);
* withCheckOptions.
*/
get_policies_for_relation(rel, CMD_INSERT, user_id,
- &merge_permissive_policies,
- &merge_restrictive_policies);
+ &merge_insert_permissive_policies,
+ &merge_insert_restrictive_policies);
add_with_check_options(rel, rt_index,
WCO_RLS_INSERT_CHECK,
- merge_permissive_policies,
- merge_restrictive_policies,
- withCheckOptions,
- hasSubLinks,
- false);
-
- /* Enforce the WITH CHECK clauses of the UPDATE policies */
- add_with_check_options(rel, rt_index,
- WCO_RLS_UPDATE_CHECK,
- merge_permissive_policies,
- merge_restrictive_policies,
+ merge_insert_permissive_policies,
+ merge_insert_restrictive_policies,
withCheckOptions,
hasSubLinks,
false);
index 4e54976618466313d7f7acf8789bffd4bdf27229..97ca9bf72c5a2a41e688e67e587ca79986731bc2 100644 (file)
CREATE POLICY p1 ON document FOR SELECT USING (true);
-- one may insert documents only authored by them
CREATE POLICY p2 ON document FOR INSERT WITH CHECK (dauthor = current_user);
--- one may only update documents in 'novel' category
+-- one may only update documents in 'novel' category and new dlevel must be > 0
CREATE POLICY p3 ON document FOR UPDATE
USING (cid = (SELECT cid from category WHERE cname = 'novel'))
- WITH CHECK (dauthor = current_user);
+ WITH CHECK (dlevel > 0);
-- one may only delete documents in 'manga' category
CREATE POLICY p4 ON document FOR DELETE
USING (cid = (SELECT cid from category WHERE cname = 'manga'));
(14 rows)
SET SESSION AUTHORIZATION regress_rls_bob;
--- Fails, since update violates WITH CHECK qual on dauthor
+-- Fails, since update violates WITH CHECK qual on dlevel
MERGE INTO document d
USING (SELECT 1 as sdid) s
ON did = s.sdid
WHEN MATCHED THEN
- UPDATE SET dnotes = dnotes || ' notes added by merge1 ', dauthor = 'regress_rls_alice';
+ UPDATE SET dnotes = dnotes || ' notes added by merge1 ', dlevel = 0;
ERROR: new row violates row-level security policy for table "document"
-- Should be OK since USING and WITH CHECK quals pass
MERGE INTO document d
ON did = s.sdid
WHEN MATCHED THEN
UPDATE SET dnotes = dnotes || ' notes added by merge2 ';
--- Even when dauthor is updated explicitly, but to the existing value
+-- Even when dlevel is updated explicitly, but to the existing value
MERGE INTO document d
USING (SELECT 1 as sdid) s
ON did = s.sdid
WHEN MATCHED THEN
- UPDATE SET dnotes = dnotes || ' notes added by merge3 ', dauthor = 'regress_rls_bob';
+ UPDATE SET dnotes = dnotes || ' notes added by merge3 ', dlevel = 1;
-- There is a MATCH for did = 3, but UPDATE's USING qual does not allow
-- updating an item in category 'science fiction'
MERGE INTO document d
WHEN MATCHED THEN
DELETE;
ERROR: target row violates row-level security policy (USING expression) for table "document"
+-- OK if DELETE is replaced with DO NOTHING
+MERGE INTO document d
+USING (SELECT 4 as sdid) s
+ON did = s.sdid
+WHEN MATCHED AND dnotes <> '' THEN
+ UPDATE SET dnotes = dnotes || ' notes added by merge '
+WHEN MATCHED THEN
+ DO NOTHING;
SELECT * FROM document WHERE did = 4;
did | cid | dlevel | dauthor | dtitle | dnotes
-----+-----+--------+-----------------+----------------+--------
WHEN NOT MATCHED THEN
INSERT VALUES (12, 11, 1, 'regress_rls_bob', 'another novel');
-- drop and create a new SELECT policy which prevents us from reading
--- any document except with category 'magna'
+-- any document except with category 'novel'
RESET SESSION AUTHORIZATION;
DROP POLICY p1 ON document;
CREATE POLICY p1 ON document FOR SELECT
- USING (cid = (SELECT cid from category WHERE cname = 'manga'));
+ USING (cid = (SELECT cid from category WHERE cname = 'novel'));
SET SESSION AUTHORIZATION regress_rls_bob;
-- MERGE can no longer see the matching row and hence attempts the
-- NOT MATCHED action, which results in unique key violation
MERGE INTO document d
-USING (SELECT 1 as sdid) s
+USING (SELECT 7 as sdid) s
ON did = s.sdid
WHEN MATCHED THEN
UPDATE SET dnotes = dnotes || ' notes added by merge5 '
WHEN NOT MATCHED THEN
INSERT VALUES (12, 11, 1, 'regress_rls_bob', 'another novel');
ERROR: duplicate key value violates unique constraint "document_pkey"
+-- UPDATE action fails if new row is not visible
+MERGE INTO document d
+USING (SELECT 1 as sdid) s
+ON did = s.sdid
+WHEN MATCHED THEN
+ UPDATE SET dnotes = dnotes || ' notes added by merge6 ',
+ cid = (SELECT cid from category WHERE cname = 'technology');
+ERROR: new row violates row-level security policy for table "document"
+-- but OK if new row is visible
+MERGE INTO document d
+USING (SELECT 1 as sdid) s
+ON did = s.sdid
+WHEN MATCHED THEN
+ UPDATE SET dnotes = dnotes || ' notes added by merge7 ',
+ cid = (SELECT cid from category WHERE cname = 'novel');
+-- OK to insert a new row that is not visible
+MERGE INTO document d
+USING (SELECT 13 as sdid) s
+ON did = s.sdid
+WHEN MATCHED THEN
+ UPDATE SET dnotes = dnotes || ' notes added by merge8 '
+WHEN NOT MATCHED THEN
+ INSERT VALUES (13, 44, 1, 'regress_rls_bob', 'new manga');
RESET SESSION AUTHORIZATION;
-- drop the restrictive SELECT policy so that we can look at the
-- final state of the table
DROP POLICY p1 ON document;
-- Just check everything went per plan
SELECT * FROM document;
- did | cid | dlevel | dauthor | dtitle | dnotes
------+-----+--------+-------------------+----------------------------------+-----------------------------------------------------------------------
+ did | cid | dlevel | dauthor | dtitle | dnotes
+-----+-----+--------+-------------------+----------------------------------+----------------------------------------------------------------------------------------------
3 | 22 | 2 | regress_rls_bob | my science fiction |
5 | 44 | 2 | regress_rls_bob | my second manga |
6 | 22 | 1 | regress_rls_carol | great science fiction |
78 | 33 | 1 | regress_rls_bob | some technology novel |
79 | 33 | 1 | regress_rls_bob | technology book, can only insert |
12 | 11 | 1 | regress_rls_bob | another novel |
- 1 | 11 | 1 | regress_rls_bob | my first novel | notes added by merge2 notes added by merge3 notes added by merge4
-(14 rows)
+ 1 | 11 | 1 | regress_rls_bob | my first novel | notes added by merge2 notes added by merge3 notes added by merge4 notes added by merge7
+ 13 | 44 | 1 | regress_rls_bob | new manga |
+(15 rows)
--
-- ROLE/GROUP
index 3d664538a69653e2a8a0fa3611c001ef25753eb5..dec7340538cd4f28dc7de2fe506215835ed3f9f3 100644 (file)
CREATE POLICY p1 ON document FOR SELECT USING (true);
-- one may insert documents only authored by them
CREATE POLICY p2 ON document FOR INSERT WITH CHECK (dauthor = current_user);
--- one may only update documents in 'novel' category
+-- one may only update documents in 'novel' category and new dlevel must be > 0
CREATE POLICY p3 ON document FOR UPDATE
USING (cid = (SELECT cid from category WHERE cname = 'novel'))
- WITH CHECK (dauthor = current_user);
+ WITH CHECK (dlevel > 0);
-- one may only delete documents in 'manga' category
CREATE POLICY p4 ON document FOR DELETE
USING (cid = (SELECT cid from category WHERE cname = 'manga'));
SET SESSION AUTHORIZATION regress_rls_bob;
--- Fails, since update violates WITH CHECK qual on dauthor
+-- Fails, since update violates WITH CHECK qual on dlevel
MERGE INTO document d
USING (SELECT 1 as sdid) s
ON did = s.sdid
WHEN MATCHED THEN
- UPDATE SET dnotes = dnotes || ' notes added by merge1 ', dauthor = 'regress_rls_alice';
+ UPDATE SET dnotes = dnotes || ' notes added by merge1 ', dlevel = 0;
-- Should be OK since USING and WITH CHECK quals pass
MERGE INTO document d
WHEN MATCHED THEN
UPDATE SET dnotes = dnotes || ' notes added by merge2 ';
--- Even when dauthor is updated explicitly, but to the existing value
+-- Even when dlevel is updated explicitly, but to the existing value
MERGE INTO document d
USING (SELECT 1 as sdid) s
ON did = s.sdid
WHEN MATCHED THEN
- UPDATE SET dnotes = dnotes || ' notes added by merge3 ', dauthor = 'regress_rls_bob';
+ UPDATE SET dnotes = dnotes || ' notes added by merge3 ', dlevel = 1;
-- There is a MATCH for did = 3, but UPDATE's USING qual does not allow
-- updating an item in category 'science fiction'
WHEN MATCHED THEN
DELETE;
+-- OK if DELETE is replaced with DO NOTHING
+MERGE INTO document d
+USING (SELECT 4 as sdid) s
+ON did = s.sdid
+WHEN MATCHED AND dnotes <> '' THEN
+ UPDATE SET dnotes = dnotes || ' notes added by merge '
+WHEN MATCHED THEN
+ DO NOTHING;
+
SELECT * FROM document WHERE did = 4;
-- Switch to regress_rls_carol role and try the DELETE again. It should succeed
INSERT VALUES (12, 11, 1, 'regress_rls_bob', 'another novel');
-- drop and create a new SELECT policy which prevents us from reading
--- any document except with category 'magna'
+-- any document except with category 'novel'
RESET SESSION AUTHORIZATION;
DROP POLICY p1 ON document;
CREATE POLICY p1 ON document FOR SELECT
- USING (cid = (SELECT cid from category WHERE cname = 'manga'));
+ USING (cid = (SELECT cid from category WHERE cname = 'novel'));
SET SESSION AUTHORIZATION regress_rls_bob;
-- MERGE can no longer see the matching row and hence attempts the
-- NOT MATCHED action, which results in unique key violation
MERGE INTO document d
-USING (SELECT 1 as sdid) s
+USING (SELECT 7 as sdid) s
ON did = s.sdid
WHEN MATCHED THEN
UPDATE SET dnotes = dnotes || ' notes added by merge5 '
WHEN NOT MATCHED THEN
INSERT VALUES (12, 11, 1, 'regress_rls_bob', 'another novel');
+-- UPDATE action fails if new row is not visible
+MERGE INTO document d
+USING (SELECT 1 as sdid) s
+ON did = s.sdid
+WHEN MATCHED THEN
+ UPDATE SET dnotes = dnotes || ' notes added by merge6 ',
+ cid = (SELECT cid from category WHERE cname = 'technology');
+
+-- but OK if new row is visible
+MERGE INTO document d
+USING (SELECT 1 as sdid) s
+ON did = s.sdid
+WHEN MATCHED THEN
+ UPDATE SET dnotes = dnotes || ' notes added by merge7 ',
+ cid = (SELECT cid from category WHERE cname = 'novel');
+
+-- OK to insert a new row that is not visible
+MERGE INTO document d
+USING (SELECT 13 as sdid) s
+ON did = s.sdid
+WHEN MATCHED THEN
+ UPDATE SET dnotes = dnotes || ' notes added by merge8 '
+WHEN NOT MATCHED THEN
+ INSERT VALUES (13, 44, 1, 'regress_rls_bob', 'new manga');
+
RESET SESSION AUTHORIZATION;
-- drop the restrictive SELECT policy so that we can look at the
-- final state of the table