From: Chao Li <li.evan.chao@gmail.com>
Content-Type: text/plain;
	charset=utf-8
Content-Transfer-Encoding: quoted-printable
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.600.51.1.1\))
Subject: Fix RLS checks for UPDATE/DELETE FOR PORTION OF leftover rows
Message-Id: <6C34A987-AC50-4477-BD71-2D4AFEE1A589@gmail.com>
Date: Tue, 30 Jun 2026 15:45:43 +0800
Cc: Peter Eisentraut <peter@eisentraut.org>,
 Paul A Jungwirth <pj@illuminatedcomputing.com>
To: PostgreSQL-development <pgsql-hackers@postgresql.org>
Archived-At: 
 <https://www.postgresql.org/message-id/6C34A987-AC50-4477-BD71-2D4AFEE1A589%40gmail.com>
Precedence: bulk

Hi,

While revisiting =E2=80=9C[8e72d914c] Add UPDATE/DELETE FOR PORTION =
OF=E2=80=9D, 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=3D# create role u;
CREATE ROLE
evantest=3D# create table t (id int, valid_at int4range);
CREATE TABLE
evantest=3D# alter table t enable row level security;
ALTER TABLE
evantest=3D# alter table t force row level security;
ALTER TABLE
evantest=3D# create policy t_sel on t for select to u using (true);
CREATE POLICY
evantest=3D# create policy t_upd on t for update to u using =
(lower(valid_at)<50) with check (lower(valid_at)<50);
CREATE POLICY
evantest=3D# create policy t_del on t for delete to u using =
(lower(valid_at)<50);
CREATE POLICY
evantest=3D# create policy t_ins on t for insert to u with check =
(lower(valid_at)<50);
CREATE POLICY
evantest=3D# grant select, update, delete, insert on t to u;
GRANT
evantest=3D# insert into t values (1, '[10,100)');
INSERT 0 1
evantest=3D# set role u;
SET
```

2. Update the row:
```
evantest=3D> update t for portion of valid_at from 30 to 70 set id=3D2;
UPDATE 1
evantest=3D> 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);

        =E2=80=A6..

	/*
	 * 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 =3D=3D CMD_INSERT || commandType =3D=3D =
CMD_UPDATE)
	{
		/* This should be the target relation */
		Assert(rt_index =3D=3D root->resultRelation);

		add_with_check_options(rel, rt_index,
							   commandType =
=3D=3D 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 =3D mtstate->operation;
			mtstate->operation =3D CMD_INSERT;
			oldTcs =3D 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 =3D=3D CMD_UPDATE)
			wco_kind =3D WCO_RLS_UPDATE_CHECK;
		else if (mtstate->operation =3D=3D CMD_MERGE)
			wco_kind =3D =
(mtstate->mt_merge_action->mas_action->commandType =3D=3D CMD_UPDATE) ?
				WCO_RLS_UPDATE_CHECK : =
WCO_RLS_INSERT_CHECK;
		else
			wco_kind =3D 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 !=3D 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 =3D (WithCheckOption *) lfirst(l1);
		ExprState  *wcoExpr =3D (ExprState *) lfirst(l2);

		/*
		 * Skip any WCOs which are not the kind we are looking =
for at this
		 * time.
		 */
		if (wco->kind !=3D 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=3D> delete from t for portion of valid_at from 30 to 70;
DELETE 1
evantest=3D> 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=3D> update t for portion of valid_at from 30 to 70 set id=3D2;
ERROR:  new row violates row-level security policy for table "t"
evantest=3D>  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/