Re: CREATE POLICY and RETURNING

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.

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.

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

Regards,
Dean