Re: ON CONFLICT issues around whole row vars, - Mailing list pgsql-hackers

On Thu, Oct 1, 2015 at 3:42 AM, Andres Freund <andres@anarazel.de> wrote:
> Yes, that what I think as well.  At this point we'll already have
> executed insert rls stuff on the EXCLUDED tuple:
>                 /*
>                  * Check any RLS INSERT WITH CHECK policies
>                  *
>                  * 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_RLS_INSERT_CHECK,
>                                                                  resultRelInfo, slot, estate);
> and before executing the actual projection we also checked the existing
> tuple:
>                 ExecWithCheckOptions(WCO_RLS_CONFLICT_CHECK, resultRelInfo,
>                                                          mtstate->mt_existing,
>                                                          mtstate->ps.state);
>
> after the update triggers have, if applicable run, we run the the normal
> checks there as well because it's just ExecUpdate()
>                 if (resultRelInfo->ri_WithCheckOptions != NIL)
>                         ExecWithCheckOptions(WCO_RLS_UPDATE_CHECK,
>                                                                  resultRelInfo, slot, estate);
>
> so I do indeed think that there's no point in layering RLS above
> EXCLUDED.

I see your point, I think. It might be a problem if we weren't already
making the statement error out, but we are.

However, we're checking the excluded tuple (the might-be-inserted,
might-be-excluded tuple that reflects before row insert trigger
effects) with WCO_RLS_INSERT_CHECK, not WCO_RLS_UPDATE_CHECK. The
WCO_RLS_UPDATE_CHECK applies to the tuple to be appended to the
relation (the tuple that an UPDATE makes supersede some existing
tuple, a new row version).

We all seem to be in agreement that excluded.* ought to be subject to
column-level privilege enforcement, mostly due to possible leaks with
before row insert triggers (these could be SoD; a malicious UPSERT
could be written a certain way). None of the checks in the code above
are the exact RLS equivalent of the principle we have for column
privileges, AFAICT, because update-applicable policies (everything but
insert-applicable policies, actually) are not checked against the
excluded tuple. Shouldn't select-applicable policies also be applied
to the excluded tuples, just as with UPDATE ... FROM "join from"
tables, which excluded is kinda similar to?

I'm not trying to be pedantic; I just don't grok the underlying
principles here. Couldn't a malicious WHERE clause leak the excluded.*
tuple contents (and cause the UPDATE to not proceed) before the
WCO_RLS_CONFLICT_CHECK call site was reached, while also preventing it
from ever actually being reached (with a malicious function that
returns false after stashing excluded.* elsewhere)? You can put
volatile functions in UPDATE WHERE clauses, even if it is generally a
bad idea.

Perhaps I'm simply not following you here, though. I think that this
is one challenge with having per-command policies with a system that
checks permissions dynamically (not during parse analysis). Note that
I'm not defending the status quo of the master branch -- I'm just a
little uneasy about what the ideal, least surprising behavior is here.

-- 
Peter Geoghegan