[Chao Li <li.evan.chao@gmail.com>]

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/