Re: CREATE POLICY and RETURNING

* Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
> On 6 June 2015 at 22:16, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> > On Fri, Oct 17, 2014 at 5:34 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >> On 2014-10-17 14:57:03 +0800, Craig Ringer wrote:
> >>> On 10/17/2014 02:49 AM, Robert Haas wrote:
> >>> > I think you could probably make the DELETE policy control what can get
> >>> > deleted, but then have the SELECT policy further filter what gets
> >>> > returned.
> >>>
> >>> That seems like the worst of both worlds to me.
> >>>
> >>> Suddenly DELETE ... RETURNING might delete more rows than it reports a
> >>> resultset for. As well as being potentially dangerous for people using
> >>> it in wCTEs, etc, to me that's the most astonishing possible outcome of all.
> >>>
> >>> I'd be much happier with even:
> >>>
> >>> ERROR: RETURNING not permitted with SELECT row-security policy
> >>
> >> FWIW, that doesn't sound acceptable to me.
> >
> > This is more or less what ended up happening with UPSERT and USING
> > security barrier quals on UPDATE/ALL policies. Realistically, the
> > large majority of use cases don't involve a user being able to
> > INSERT/DELETE tuples, but not SELECT them, and those that do should
> > not be surprised to have a RETURNING fail (it's an odd enough union of
> > different features that this seems acceptable to me).
> >
> > Like Fujii, I think that RETURNING with RLS should not get to avoid
> > SELECT policies. I agree with the concern about not seeing affected
> > rows with a DELETE (which, as I said, is very similar to UPSERT +
> > WCO_RLS_CONFLICT_CHECK policies), so an error seems like the only
> > alternative.
> >
> > The argument against not requiring SELECT *column* privilege on the
> > EXCLUDED.* pseudo relation for UPSERT might have been: "well, what can
> > be the harm of allowing the user to see what they themselves might
> > have inserted?". But that would have been a bad argument then had
> > anyone made it, because RETURNING with a (vanilla) INSERT requires
> > SELECT privilege, and that's also what the user then actually inserted
> > (as distinct from what the user *would have* inserted had the insert
> > path been taking, representing as the EXCLUDED.* pseudo relation --
> > for security purposes, ISTM that this is really no distinction at
> > all). Consider before row insert triggers that can modify EXCLUDED.*
> > tuples in a privileged way.
> >
> > So, the only logical reason that INSERT with RETURNING requires SELECT
> > column privilege that I can see is that a before row INSERT trigger
> > could modify the tuple inserted in a way that the inserter role should
> > not know the details of. This long standing convention was reason
> > enough to mandate that SELECT column privilege be required for the
> > EXCLUDED.* pseudo relation for UPSERT. And so, I think it isn't too
> > much of a jump to also say that we should do the same for RLS (for
> > INSERTs for the reason I state, but also for UPDATEs and DELETEs for a
> > far more obvious reason: the *existing* tuple can be projected, and
> > the updater/deleter might well have no business seeing its contents).
> >
> > In short: I think we should be tracking a new WCOKind (perhaps
> > WCO_RLS_RETURNING_CHECK?), that independently holds the security
> > barrier quals as WCO-style checks when that's recognized as being
> > necessary. For INSERT, these WCOs must be enforced against the target
> > tuple projected by RETURNING. For UPDATEs and DELETEs, FROM/USING
> > relations must also have SELECT privilege enforced against the
> > projected target tuple, as well as the non-target relation --
> > apparently the latter isn't currently happening, although Dean has
> > tried to address this with his recent patch [1]. That is, even
> > non-target relations (UPDATE ... FROM relations, or DELETE ... USING
> > relations) do not have SELECT policy enforcement, but rather have
> > UPDATE or DELETE policy enforcement only. I must admit that I was
> > rather surprised at that; it has to be a bug.
> >
>
> Yes, I think a query with a RETURNING clause ought to either succeed
> and return everything the user asked for, or error out if some/all of
> the data isn't visible to the user according to SELECT policies in
> effect. I think not applying the SELECT policies is wrong, and
> returning a portion of the data updated would just be confusing.

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.

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.

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, 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.

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.

> My previous patch causes the SELECT policies for all the non-target
> relations to be applied, but not the SELECT policies for the target
> relation. So I think it would suffice to just add another check to
> apply them to the target tuple, using something like
> WCO_RLS_RETURNING_CHECK, as you suggested. However, I think it must be
> applied to the target tuple *before* projecting it because the
> RETURNING expressions might contain malicious leaky functions.
> Actually I'm not sure the policy can be enforced against the projected
> target tuple, since it might not contain all the necessary columns to
> do the check.

Agreed- the SELECT policies for the non-target relations should be used,
that's clearly the right thing to do. Regarding the rest, I'd certainly
welcome your thoughts on my above response.

> In principle, it sounds easy to do, but I think it will be much
> simpler against my previous patch.

I'm planning to review your latest prior to PGCon. I really like that
you were able to eliminate the "extra" places we were adding the
default-deny policy and believe that will make the code a lot simpler to
follow.

Thanks!

Stephen