Hi,
While revisiting “[8e72d914c] Add UPDATE/DELETE FOR PORTION OF”, I found a new
issue where inserting leftover rows may skip row-level security checks.
This is a repro:
1. Prepare a role and a table with an int4range column, then add policies that
require the lower bound to be less than 50:
```
evantest=# create role u;
CREATE ROLE
evantest=# create table t (id int, valid_at int4range);
CREATE TABLE
evantest=# alter table t enable row level security;
ALTER TABLE
evantest=# alter table t force row level security;
ALTER TABLE
evantest=# create policy t_sel on t for select to u using (true);
CREATE POLICY
evantest=# create policy t_upd on t for update to u using (lower(valid_at)<50)
with check (lower(valid_at)<50);
CREATE POLICY
evantest=# create policy t_del on t for delete to u using (lower(valid_at)<50);
CREATE POLICY
evantest=# create policy t_ins on t for insert to u with check
(lower(valid_at)<50);
CREATE POLICY
evantest=# grant select, update, delete, insert on t to u;
GRANT
evantest=# insert into t values (1, '[10,100)');
INSERT 0 1
evantest=# set role u;
SET
```
2. Update the row:
```
evantest=> update t for portion of valid_at from 30 to 70 set id=2;
UPDATE 1
evantest=> select * from t;
id | valid_at
----+----------
2 | [30,70)
1 | [10,30)
1 | [70,100)
(3 rows)
```
Here, the right leftover [70,100) was inserted, even though the INSERT policy
requires lower(valid_at) < 50. If the policy were honored, the update should
fail because the leftover insert violates policy t_ins.
After tracing the update statement, I think the problem is as follows.
1.In the rewrite phase, get_policies_for_relation() gets only the UPDATE
policy, because commandType is CMD_UPDATE:
```
/*
* For SELECT, UPDATE and DELETE, add security quals to enforce the
USING
* policies. These security quals control access to existing table
rows.
* Restrictive policies are combined together using AND, and permissive
* policies are combined together using OR.
*/
get_policies_for_relation(rel, commandType, user_id,
&permissive_policies,
&restrictive_policies);
…..
/*
* For INSERT and UPDATE, add withCheckOptions to verify that any new
* records added are consistent with the security policies. This will
use
* each policy's WITH CHECK clause, or its USING clause if no explicit
* WITH CHECK clause is defined.
*/
if (commandType == CMD_INSERT || commandType == CMD_UPDATE)
{
/* This should be the target relation */
Assert(rt_index == root->resultRelation);
add_with_check_options(rel, rt_index,
commandType ==
CMD_INSERT ?
WCO_RLS_INSERT_CHECK
: WCO_RLS_UPDATE_CHECK,
permissive_policies,
restrictive_policies,
withCheckOptions,
hasSubLinks,
false);
```
2. In ExecForPortionOfLeftovers(), before inserting a leftover row, it
temporarily saves mtstate->operation and changes it to CMD_INSERT:
```
/*
* Save some mtstate things so we can restore them
below. XXX:
* Should we create our own ModifyTableState instead?
*/
oldOperation = mtstate->operation;
mtstate->operation = CMD_INSERT;
oldTcs = mtstate->mt_transition_capture;
```
3. Then it calls ExecInsert() with that state to insert the leftover row:
```
/*
* The standard says that each temporal leftover should execute
its
* own INSERT statement, firing all statement and row triggers,
but
* skipping insert permission checks. Therefore we give each
insert
* its own transition table. If we just push & pop a new
trigger level
* for each insert, we get exactly what we need.
*
* We have to make sure that the inserts don't add to the
ROW_COUNT
* diagnostic or the command tag, so we pass false for
canSetTag.
*/
AfterTriggerBeginQuery();
ExecSetupTransitionCaptureState(mtstate, estate);
fireBSTriggers(mtstate);
ExecInsert(context, resultRelInfo, leftoverSlot, false, NULL,
NULL);
fireASTriggers(mtstate);
AfterTriggerEndQuery(estate);
```
4. In ExecInsert(), wco_kind is chosen from mtstate->operation. Since it is now
CMD_INSERT, the value becomes WCO_RLS_INSERT_CHECK:
```
/*
* Check any RLS WITH CHECK policies.
*
* Normally we should check INSERT policies. But if the insert
is the
* result of a partition key update that moved the tuple to a
new
* partition, we should instead check UPDATE policies, because
we are
* executing policies defined on the target table, and not those
* defined on the child partitions.
*
* If we're running MERGE, we refer to the action that we're
executing
* to know if we're doing an INSERT or UPDATE to a partition
table.
*/
if (mtstate->operation == CMD_UPDATE)
wco_kind = WCO_RLS_UPDATE_CHECK;
else if (mtstate->operation == CMD_MERGE)
wco_kind =
(mtstate->mt_merge_action->mas_action->commandType == CMD_UPDATE) ?
WCO_RLS_UPDATE_CHECK : WCO_RLS_INSERT_CHECK;
else
wco_kind = WCO_RLS_INSERT_CHECK;
```
5. With this wco_kind, it calls ExecWithCheckOptions():
```
/*
* ExecWithCheckOptions() will skip any WCOs which are not of
the kind
* we are looking for at this point.
*/
if (resultRelInfo->ri_WithCheckOptions != NIL)
ExecWithCheckOptions(wco_kind, resultRelInfo, slot,
estate);
```
6. In ExecWithCheckOptions(), it loops over all WithCheckOptions built during
rewrite:
```
/* Check each of the constraints */
forboth(l1, resultRelInfo->ri_WithCheckOptions,
l2, resultRelInfo->ri_WithCheckOptionExprs)
{
WithCheckOption *wco = (WithCheckOption *) lfirst(l1);
ExprState *wcoExpr = (ExprState *) lfirst(l2);
/*
* Skip any WCOs which are not the kind we are looking for at
this
* time.
*/
if (wco->kind != kind)
continue;
```
In this loop, wco->kind is WCO_RLS_UPDATE_CHECK, but kind is
WCO_RLS_INSERT_CHECK, so the check is skipped and the right leftover row
[70,100) is inserted.
The same problem exists for DELETE as well:
```
evantest=> delete from t for portion of valid_at from 30 to 70;
DELETE 1
evantest=> select * from t;
id | valid_at
----+----------
1 | [10,30)
1 | [70,100)
(2 rows)
```
For DELETE, in code snippet 1 above, add_with_check_options() is not called at
all because commandType is CMD_DELETE. Then, in code snippet 5,
resultRelInfo->ri_WithCheckOptions is NULL, so ExecWithCheckOptions() is not
called.
I checked the docs, and they seem to mention only that INSERT privilege is not
required for the hidden insertion of leftover rows. But I think RLS should
still be honored, because otherwise the policies are violated. For example,
directly inserting [70,100) is blocked by policy t_ins, but a user can work
around that by inserting [1,100) and then updating [30,70), which seems like a
security hole.
To fix this, I think the rewriter should load INSERT policies for UPDATE and
DELETE operations when parse->forPortionOfexists, since UPDATE/DELETE FOR
PORTION OF may insert leftover rows.
See the attached patch for details. With the fix, both the update and delete
above now fail:
```
evantest=> update t for portion of valid_at from 30 to 70 set id=2;
ERROR: new row violates row-level security policy for table "t"
evantest=> delete from t for portion of valid_at from 30 to 70;
ERROR: new row violates row-level security policy for table "t"
```
BTW, I just saw that the v19 branch has been cut and HEAD is now v20, so it
looks like this patch will need to be back-patched if accepted.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
