Thread: Bug: RLS policy FOR SELECT is used to check new rows

Try this as a user with NOBYPASSRLS:


CREATE TABLE rlsbug (deleted boolean);

INSERT INTO rlsbug VALUES (FALSE);

CREATE POLICY p_sel ON rlsbug FOR SELECT TO laurenz USING (NOT deleted);

CREATE POLICY p_upd ON rlsbug FOR UPDATE TO laurenz USING (TRUE);

ALTER TABLE rlsbug ENABLE ROW LEVEL SECURITY;
ALTER TABLE rlsbug FORCE ROW LEVEL SECURITY;

UPDATE rlsbug SET deleted = TRUE WHERE NOT deleted;
ERROR:  new row violates row-level security policy for table "rlsbug"


I'd say that this error is wrong.  The FOR SELECT policy should be applied
to the WHERE condition, but certainly not to check new rows.

Yours,
Laurenz Albe

On Tue, 24 Oct 2023 at 09:36, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>
> I'd say that this error is wrong.  The FOR SELECT policy should be applied
> to the WHERE condition, but certainly not to check new rows.
>

Yes, I had the same thought recently. I would say that the SELECT
policies should only be used to check new rows if the UPDATE has a
RETURNING clause and SELECT permissions are required on the target
relation.

In other words, it should be OK to UPDATE a row to new values that are
not visible according to the table's SELECT policies, provided that
the UPDATE command does not attempt to return those new values. That
would be consistent with what we do for INSERT.

Note, that the current behaviour goes back a long way, though it's not
quite clear whether this was intentional [1].

[1] https://github.com/postgres/postgres/commit/7d8db3e8f37aec9d252353904e77381a18a2fa9f

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> On Tue, 24 Oct 2023 at 09:36, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>> I'd say that this error is wrong.  The FOR SELECT policy should be applied
>> to the WHERE condition, but certainly not to check new rows.

> Yes, I had the same thought recently. I would say that the SELECT
> policies should only be used to check new rows if the UPDATE has a
> RETURNING clause and SELECT permissions are required on the target
> relation.

> In other words, it should be OK to UPDATE a row to new values that are
> not visible according to the table's SELECT policies, provided that
> the UPDATE command does not attempt to return those new values. That
> would be consistent with what we do for INSERT.

> Note, that the current behaviour goes back a long way, though it's not
> quite clear whether this was intentional [1].

I'm fairly sure that it was intentional, but I don't recall the
reasoning; perhaps Stephen does.  In any case, I grasp your point
that maybe we should distinguish RETURNING from not-RETURNING cases.

            regards, tom lane

On Tue, 2023-10-24 at 11:59 -0400, Tom Lane wrote:
> I'm fairly sure that it was intentional, but I don't recall the
> reasoning; perhaps Stephen does.  In any case, I grasp your point
> that maybe we should distinguish RETURNING from not-RETURNING cases.

Perhaps the idea is that if there are constraints involved, the failure
or success of an INSERT/UPDATE/DELETE could leak information that you
don't have privileges to read.

Regards,
    Jeff Davis

On Tue, Oct 24, 2023 at 1:46 PM Jeff Davis <pgsql@j-davis.com> wrote:
> Perhaps the idea is that if there are constraints involved, the failure
> or success of an INSERT/UPDATE/DELETE could leak information that you
> don't have privileges to read.

My recollection of this topic is pretty hazy, but like Tom, I seem to
remember it being intentional, and I think the reason had something to
do with wanting the slice of a RLS-protect table that you can see to
feel like a complete table. When you update a row in a table all of
which is visible to you, the updated row can never vanish as a result
of that update, so it was thought, if I remember correctly, that this
should also be true here. It's also similar to what happens if an
updatable view has WITH CHECK OPTION, and I think that was part of the
precedent as well. I don't know whether or not the constraint issue
that you mention here was also part of the concern, but it may have
been. This was all quite a while ago...

--
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, 2023-10-24 at 15:05 -0400, Stephen Frost wrote:
> On Tue, Oct 24, 2023 at 14:42 Robert Haas <robertmhaas@gmail.com> wrote:
> > On Tue, Oct 24, 2023 at 1:46 PM Jeff Davis <pgsql@j-davis.com> wrote:
> > > Perhaps the idea is that if there are constraints involved, the failure
> > > or success of an INSERT/UPDATE/DELETE could leak information that you
> > > don't have privileges to read.
> >
> > My recollection of this topic is pretty hazy, but like Tom, I seem to
> > remember it being intentional, and I think the reason had something to
> > do with wanting the slice of a RLS-protect table that you can see to
> > feel like a complete table. When you update a row in a table all of
> > which is visible to you, the updated row can never vanish as a result
> > of that update, so it was thought, if I remember correctly, that this
> > should also be true here. It's also similar to what happens if an
> > updatable view has WITH CHECK OPTION, and I think that was part of the
> > precedent as well. I don't know whether or not the constraint issue
> > that you mention here was also part of the concern, but it may have
> > been. This was all quite a while ago...
>
> Yes, having it be similar to a view WITH CHECK OPTION was intentional,
> also on not wishing for things to be able to disappear or to not get saved.
> The risk of a constraint possibly causing the leak of information is better
> than either having data just thrown away or having the constraint not
> provide the guarantee it’s supposed to …

Thanks everybody for looking and remembering.

I can accept that the error is intentional, even though it violated the
POLA for me.  I can buy into the argument that an UPDATE should not make
a row seem to vanish.

I cannot buy into the constraint argument.  If the table owner wanted to
prevent you from causing a constraint violation error with a row you
cannot see, she wouldn't have given you a FOR UPDATE policy that allows
you to perform such an UPDATE.

Anyway, it is probably too late to change a behavior that has been like
that for a while and is not manifestly buggy.

Yours,
Laurenz Albe

On Wed, 2023-10-25 at 09:45 +0200, Laurenz Albe wrote:
> I can accept that the error is intentional, even though it violated the
> POLA for me.  I can buy into the argument that an UPDATE should not make
> a row seem to vanish.
>
> I cannot buy into the constraint argument.  If the table owner wanted to
> prevent you from causing a constraint violation error with a row you
> cannot see, she wouldn't have given you a FOR UPDATE policy that allows
> you to perform such an UPDATE.
>
> Anyway, it is probably too late to change a behavior that has been like
> that for a while and is not manifestly buggy.

I have thought some more about this, and I believe that if FOR SELECT
policies are used to check new rows, you should be allowed to specify
WITH CHECK on FOR SELECT policies.  Why not allow a user to specify
different conditions for fetching from a table and for new rows after
an UPDATE?

The attached patch does that.  What so you think?

Yours,
Laurenz Albe

On Thu, 9 Nov 2023 at 15:16, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>
> I have thought some more about this, and I believe that if FOR SELECT
> policies are used to check new rows, you should be allowed to specify
> WITH CHECK on FOR SELECT policies.  Why not allow a user to specify
> different conditions for fetching from a table and for new rows after
> an UPDATE?
>
> The attached patch does that.  What so you think?
>

So you'd be able to write policies that allowed you to do an
INSERT/UPDATE ... RETURNING, where the WITH CHECK part of the SELECT
policy allowed you see the new row, but then if you tried to SELECT it
later, the USING part of the policy might say no.

That seems pretty confusing. I would expect a row to either be visible
or not, consistently across all commands.

Regards,
Dean

On Thu, 2023-11-09 at 15:59 +0000, Dean Rasheed wrote:
> On Thu, 9 Nov 2023 at 15:16, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> > I have thought some more about this, and I believe that if FOR SELECT
> > policies are used to check new rows, you should be allowed to specify
> > WITH CHECK on FOR SELECT policies.  Why not allow a user to specify
> > different conditions for fetching from a table and for new rows after
> > an UPDATE?
> >
> > The attached patch does that.  What so you think?
>
> So you'd be able to write policies that allowed you to do an
> INSERT/UPDATE ... RETURNING, where the WITH CHECK part of the SELECT
> policy allowed you see the new row, but then if you tried to SELECT it
> later, the USING part of the policy might say no.
>
> That seems pretty confusing. I would expect a row to either be visible
> or not, consistently across all commands.

I think it can be useful to allow a user an UPDATE where the result
does not satisfy the USING clause of the FOR SELECT policy.

True, it could surprise that you cannot SELECT something you just saw
with UPDATE ... RETURNING, but I would argue that these are different
operations.

The idea that an UPDATE should only produce rows you can SELECT is not
true today: if you run an UPDATE without a WHERE clause, you can
create rows you cannot see.  The restriction is only on UPDATEs with
a WHERE clause.  Weird, isn't it?

Yours,
Laurenz Albe

On Thu, 9 Nov 2023 at 18:55, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>
> I think it can be useful to allow a user an UPDATE where the result
> does not satisfy the USING clause of the FOR SELECT policy.
>
> The idea that an UPDATE should only produce rows you can SELECT is not
> true today: if you run an UPDATE without a WHERE clause, you can
> create rows you cannot see.  The restriction is only on UPDATEs with
> a WHERE clause.  Weird, isn't it?
>

That's true, but only if the UPDATE also doesn't have a RETURNING
clause. What I find weird about your proposal is that it would allow
an UPDATE ... RETURNING command to return something that would be
visible just that once, but then subsequently disappear. That seems
like a cure that's worse than the original disease that kicked off
this discussion.

As mentioned by others, the intention was that RLS behave like WITH
CHECK OPTION on an updatable view, so that new rows can't just
disappear. There are, however, 2 differences between the way it
currently works for RLS, and an updatable view:

1). RLS only does this for UPDATE commands. INSERT commands *can*
insert new rows that aren't visible, and so disappear.

2). It can't be turned off. The WITH CHECK OPTION on an updatable view
is an option that the user can choose to turn on or off. That's not
possible with RLS.

In a green field, I would say that it would be better to fix (1), so
that INSERT and UPDATE are consistent. However, I fear that it may be
too late for that, because any such change would risk breaking
existing RLS policy setups in subtle ways.

It might be possible to change (2) though, by adding a new table-level
option (similar to a view's WITH CHECK OPTION) that enabled or
disabled the checking of new rows for that table, and whose default
matched the current behaviour.

Before going too far down that route though, it is perhaps worth
asking whether this is something users really want. Is there a real
use-case for being able to UPDATE rows and have them disappear?

Regards,
Dean

On Fri, 2023-11-10 at 09:39 +0000, Dean Rasheed wrote:
> On Thu, 9 Nov 2023 at 18:55, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> > I think it can be useful to allow a user an UPDATE where the result
> > does not satisfy the USING clause of the FOR SELECT policy.
> >
> > The idea that an UPDATE should only produce rows you can SELECT is not
> > true today: if you run an UPDATE without a WHERE clause, you can
> > create rows you cannot see.  The restriction is only on UPDATEs with
> > a WHERE clause.  Weird, isn't it?
>
> That's true, but only if the UPDATE also doesn't have a RETURNING
> clause. What I find weird about your proposal is that it would allow
> an UPDATE ... RETURNING command to return something that would be
> visible just that once, but then subsequently disappear. That seems
> like a cure that's worse than the original disease that kicked off
> this discussion.

What kicked off the discussion was my complaint that FOR SELECT
rules mess with UPDATE, so that's exactly what I would have liked:
an UPDATE that makes the rows vanish.

My naïve expectation was that FOR SELECT policies govern SELECT
and FOR UPDATE policies govern UPDATE.  After all, there is a
WITH CHECK clause for FOR UPDATE policies that checks the result rows.

So, from my perspective, we should never have let FOR SELECT policies
mess with an UPDATE.  But I am too late for that; such a change would
be way too invasive now.  So I'd like to introduce a "back door" by
creating a FOR SELECT policy with WITH CHECK (TRUE).

> As mentioned by others, the intention was that RLS behave like WITH
> CHECK OPTION on an updatable view, so that new rows can't just
> disappear. There are, however, 2 differences between the way it
> currently works for RLS, and an updatable view:
>
> 1). RLS only does this for UPDATE commands. INSERT commands *can*
> insert new rows that aren't visible, and so disappear.
>
> 2). It can't be turned off. The WITH CHECK OPTION on an updatable view
> is an option that the user can choose to turn on or off. That's not
> possible with RLS.

Right.  Plus the above-mentioned fact that you can make rows vanish
with an UPDATE that has no WHERE.

> It might be possible to change (2) though, by adding a new table-level
> option (similar to a view's WITH CHECK OPTION) that enabled or
> disabled the checking of new rows for that table, and whose default
> matched the current behaviour.

That would be a viable solution.

Pro: it doesn't make the already hideously complicated RLS system
even more complicated.

Con: yet another storage option...

> Before going too far down that route though, it is perhaps worth
> asking whether this is something users really want. Is there a real
> use-case for being able to UPDATE rows and have them disappear?

What triggered my investigation was this question:
https://stackoverflow.com/q/77346757/6464308

I personally don't have any stake in this.  I just wanted a way to
make RLS behave more like I think it should.

Yours,
Laurenz Albe

On Fri, Nov 10, 2023 at 7:43 AM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> So, from my perspective, we should never have let FOR SELECT policies
> mess with an UPDATE.  But I am too late for that; such a change would
> be way too invasive now.  So I'd like to introduce a "back door" by
> creating a FOR SELECT policy with WITH CHECK (TRUE).

In principle I see no problem with some kind of back door here, but
that seems like it might not be the right way to do it. I don't think
we want constant true to behave arbitrarily differently than any other
expression. Maybe that's not what you had in mind and I'm just not
seeing the full picture, though.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, 2023-11-13 at 12:57 -0500, Robert Haas wrote:
> On Fri, Nov 10, 2023 at 7:43 AM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> > So, from my perspective, we should never have let FOR SELECT policies
> > mess with an UPDATE.  But I am too late for that; such a change would
> > be way too invasive now.  So I'd like to introduce a "back door" by
> > creating a FOR SELECT policy with WITH CHECK (TRUE).
>
> In principle I see no problem with some kind of back door here, but
> that seems like it might not be the right way to do it. I don't think
> we want constant true to behave arbitrarily differently than any other
> expression. Maybe that's not what you had in mind and I'm just not
> seeing the full picture, though.

I experimented some more, and I think I see my mistake now.

Currently, the USING clause of FOR SELECT/ALL/UPDATE policies is
an *additional* restriction to the WITH CHECK clause.
So my suggestion of using the WITH CHECK clause *instead of*
the USING clause in FOR SELECT policies would be unprincipled.

Sorry for the noise.

Yours,
Laurenz Albe