Fix RLS checks for UPDATE/DELETE FOR PORTION OF leftover rows - Mailing list pgsql-hackers

Hi,

While revisiting “[8e72d914c] Add UPDATE/DELETE FOR PORTION OF”, I found a new issue where inserting leftover rows may
skiprow-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
policywere 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
itto 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
rightleftover 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
leftoverrows. But I think RLS should still be honored, because otherwise the policies are violated. For example,
directlyinserting [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-patchedif accepted. 

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/