Re: CREATE POLICY and RETURNING

Dean,

* Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
> On 8 June 2015 at 16:53, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > I definitely don't like the idea of returning a portion of the data in a
> > RETURNING clause. Returning an error if the RETURNING clause ends up
> > not passing the SELECT policy is an interesting idea, but I do have
> > doubts about how often that will be a useful exercise. Another issue to
> > note is that, currently, SELECT policies never cause errors to be
> > returned, and this would change that.
>
> True. Before UPSERT was added, it was the case that USING clauses from
> all kinds of policies didn't cause errors, and CHECK clauses did, but
> that has now changed for UPDATE, and I don't think it's necessarily a
> problem to change it for SELECT, if we decide that it makes sense to
> apply SELECT policies to rows returned by RETURNING clauses.

Fair enough.

> > There was discussion about a VISIBILITY policy instead of a SELECT
> > policy at one point. What I think we're seeing here is confusion about
> > the various policies which exist, because the USING policy of an UPDATE
> > is precisely its VISIBILITY policy, in my view, which is why I wasn't
> > bothered by the RETURNING clause being able to be used in that case. I
> > recall working on the documentation to make this clear, but it clearly
> > needs work.
>
> Yeah, perhaps there's scope for improving the documentation,
> regardless of whether or not we change the current behaviour. One
> place that we could add additional documentation is the pages
> describing the INSERT, UPDATE and DELETE commands. These currently
> each have a paragraph in their main description sections describing
> what privileges they require, so perhaps we should add similar
> paragraphs describing what RLS policies are checked, if RLS is enabled
> on the table.

Agreed.

> > The primary use-case for having a different VISIBILITY for UPDATE (or
> > DELETE) than for SELECT is that you wish to allow users to only modify a
> > subset of the rows in the table which they can see. I agree that the
> > current arrangement is that this can be used to allow users to UPDATE
> > records which they can't see (except through a RETURNING). Perhaps
> > that's a mistake and we should, instead, force the SELECT policy to be
> > included for the UPDATE and DELETE policies and have the USING clauses
> > from those be AND'd with the SELECT policy. That strikes me as a much
> > simpler approach than applying the SELECT policy against the RETURNING
> > clause and then throwing an error if it fails,
>
> I don't think that quite addresses the concern with RETURNING though
> because the resulting clauses would only be applied to the old
> (pre-update) data, whereas a RETURNING clause might still return new
> data that you couldn't otherwise see.

This part doesn't make sense to me. You can issue a SELECT statement
which will return records that are not visible to you also, by simply
running some transformation of the appropriate column as it comes out.
I don't believe it makes any sense to try and protect the results of a
transformation from the user who is defining what the transformations
are.

Worse, with this approach, it might *look* like RETURNING results are
being filtered in a way that prevents users from being able to see data
they shouldn't be allowed to see in the table via the SELECT policy when
they can- they just have to transform the appropriate column during the
UPDATE to get it to pass the UPDATE CHECK and SELECT USING policies.

Here is an example of what I mean:

CREATE TABLE my_data (
color text,
secret text
);

CREATE POLICY only_green ON my_data
FOR SELECT USING (color = 'green');

CREATE POLICY allow_update ON my_data FOR UPDATE
USING (true) -- a mistake
WITH CHECK (color = 'green');

If the attacker was interested in rows which had 'red' for the color,
they could simply run:

UPDATE my_data SET color = 'green' WHERE color = 'red' RETURNING *;

The above command returns all the 'secret' data for rows which had
a color of 'red' in the table, even though the SELECT policy clearly
doesn't allow the user to see those rows and the UPDATE WITH CHECK
policy only allows the user to create records with the color 'green',
and this would still be the case even with the proposed changes.

Note that the above WHERE clause isn't actually necessary at all, but
the 'color' column would need to be set to 'green' to pass the SELECT
and UPDATE policies, and therefore the attacker would lose the data in
that column- but we're not trying to hide just the data in that
particular column, nor do we want to have to define policies for every
column.

Basically, I don't think it makes much sense to try and enforce a SELECT
policy on a value which an attacker can set. This is very different
from enforcing a policy about adding rows to a table or enforcing a
policy about what rows are visible to a user based on the data already
in the table.

Ultimately, we're not actually protecting any data by filtering or
erroring based on the results of a transformation that the attacker gets
to define.

Worse, with this check, users might feel that they have their policies
correctly defined based on testing a command such as:

UPDATE my_data SET color = 'red' WHERE color = 'red' RETURNING *;
(or perhaps setting some other, unrelated, column but trying to pull out
the 'red' secret data)

As that would then result in an error from the new
RETURNING-SELECT-POLICY check, but the data wouldn't actually be
protected, as illustrated above.

> > though this would remove
> > a bit of flexibility in the system since we would no longer allow blind
> > UPDATEs or DELETEs. I'm not sure if that's really an issue though- to
> > compare it to our GRANT-based system, the only blind UPDATE allowed
> > there is one which goes across the entire table- any WHERE clause used
> > results in a check to ensure you have SELECT rights.
>
> That's not quite the extent of it. Column-level privileges might allow
> you to SELECT the PK column of a table, and then you might be able to
> UPDATE or DELETE specific rows despite not being able to see their
> entire contents. I can see cases where that might be useful, but I
> have to admit that I'm struggling to think of any case where the
> row-level equivalent would make sense (although that, by itself is not
> a good argument for not allowing it).

That's a good point that you could use the PK in the WHERE of an UPDATE
or DELETE and not have rights to SELECT the other columns. I can
imagine cases where users would want to be able to 'give away' rows
they can currently see, but I have a hard time coming up with a reason
to allow them to acquire rows they are not currently allowed to see. I
see that as an argument for including the SELECT policy implicitly
though, as I suggested above, but I take it that you're not advocating
for that, per your comments below.

> > Ultimately, we're trying to provide as much flexibility as possible
> > while having the system be easily understandable by the policy authors
> > and allowing them to write simple policies to enforce the necessary
> > constraints. Perhaps allowing the open-ended USING clauses for UPDATE
> > and DELETE is simply making for a more complicated system as policy
> > authors then have to make sure they embed whatever SELECT policy they
> > have into their UPDATE and DELETE policies explicitly- and the result if
> > they don't do that correctly is that users may be able to update/delete
> > things they really shouldn't be able to. Now, as we've discussed
> > previously, users should be expected to test their systems and make sure
> > that they are behaving as they expect, but we don't want to put
> > landmines in either or have ways which they can be easily tripped up.
>
> Well I'm all for keeping this as simple and easily understandable as
> possible. Right now the USING clauses of UPDATE policies control which
> existing rows can be updated and the CHECK clauses control what new
> data can go in. That's about as simple as it could be IMO, and trying
> to merge SELECT policies with either of those would only make it more
> complicated, and hence make it more likely for users to get it wrong.

I like the flexibility and simplicity of having SELECT be independent of
the UPDATE and other policies and the ALL option for defining policies
certainly helps with that, when the policies for all operations have
the same clauses. That said, I can see an argument for having SELECT
policies enforced on the UPDATE USING clause as actually being simpler
as it means you don't have to think about the UPDATE USING clause
independently, unless you want to further reduce the set of rows
available through the policy.

Still, I'm not going to argue if we end up agreeing that what we have
already makes sense.

> OTOH, when you tag a RETURNING clause onto the UPDATE, it's kind of
> like tying an UPDATE and a SELECT together, and it does make sense to
> me that the new rows returned by the SELECT part of the command be
> constrained by any SELECT policies on the table. That's consistent
> with what happens with GRANT-based privileges, so I don't think that
> it should be that surprising or difficult for users to understand.

There is a big difference here though- there's no such thing as a 'new'
or 'old' row when talking about the GRANT system. In the GRANT system,
everything is checked at the outset of the command and the RETURNING
clause's columns are checked for SELECT rights, even if the entire
column has been replaced during the UPDATE statement with a constant or
other value defined by the user. Therefore, I don't believe you can
make the argument that the GRANT system supports this notion of checking
the SELECT policy against the 'new' rows being returned.

> I also think that if it ever were the case that a row resulting from
> an UPDATE were not visible to the user who made that update, then that
> would most likely indicate that the policies were poorly-defined.

This is a case that I disagree with. As mentioned above, I can imagine
environments where it's acceptable to be able to "give away" rows.
Moving away from the security arena for a moment, consider a 'queue'
table where the rows move through different states. RLS might be a very
handy way to implement that queue by having different processes only
able to see the rows which have a particular state, but they can update
the rows to give them a different state which then moves them to another
processes' queue. Bringing it back to the security side, consider a
hospital where there can only be one 'responsible' physician assigned,
but they are allowed to trade cases around based on some external
criteria. Or the case files for a judge. Perhaps these change
operations should require an 'administrator' user to implement, but
perhaps not. Allowing users to acquire rows which they can't currently
see is a much more questionable action, in my view, as it implies
independent knowledge that such data exists to be acquired.

> Ideally the UPDATE policy's CHECK clause would prevent the user from
> making such an update. Having a RETURNING clause that was checked
> against the table's SELECT policy would then actually provide the user
> with a way to validate the consistency of their policy clauses.
> Hitting this new RETURNING-SELECT-POLICY error would be a sign that
> perhaps the WITH CHECK clauses on your policies aren't properly
> constraining new data.

Agreed, the CHECK clause should prevent users from giving away records
in environments where that isn't appropriate. I'm not convinced that
adding a RETURNING-SELECT-POLICY error would actually be terribly useful
to users for checking that their policies are constructed correctly, see
above regarding my concerns about enforcing policies against the results
of the transformations requested by the user.

> So I actually think this additional check would increase the chances
> of policies being defined correctly, and I think that consistency with
> GRANT-based privileges will make it easier to understand.

I'm afraid it wouldn't actually prevent attackers from getting access to
data they shouldn't have access to and I worry that it'd introduce
complications that would mask incorrect policy definitions, as discussed
above.

Thanks!

Stephen