Re: Broken lock management in policy.c.

On Sun, Jan 3, 2016 at 5:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Hmm.  I agree that this test case's behavior does not depend on CREATE
> POLICY's lock mismanagement.  I think what is going on here is that the
> RLS quals are being checked with an older snapshot than what controls
> the output of the UPDATE RETURNING.  Even the EPQ recheck that's done
> after getting update lock on the "information" row doesn't fix it,
> because we *don't* insist on taking an update lock on the "users" table,
> so we don't see the new value of that row.

That's clearly what's going on in the example scenario: the EPQ
recheck walks the update chain, and uses a combination of a dirty
snapshot (for the target's scan tuple), and the MVCC snapshot (for
everything else). The scenario involves an attacker exploiting the
inconsistency, where the admin did not anticipate such an
inconsistency.

This seemed reasonably plausible to me, not least because READ
COMMITTED conflict handling is a mystery to the vast majority of
users.

> If that diagnosis is correct, you could fix it by changing the RLS
> policies' sub-selects to use SELECT FOR UPDATE, though the loss of
> concurrency might well be unacceptable.

That was one of the things that we talked about doing. Of course, that
would work because we walk the UPDATE chain to do EPQ recheck for
SELECT FOR UPDATE, just as with an UPDATE or a DELETE.

> In any case, the text in create_policy.sgml seems to be a misleading
> description of the problem, as it's talking about DDL modifications
> which are *not* what's happening here.

I'm not sure what you mean. The CREATE POLICY changes in commit
43cd468cf01007f3 specifically call out the issue illustrated in my
example test case. There are some other changes made in that commit,
but they don't seem to be attempting to address this specific problem.
They also seem fine.

-- 
Peter Geoghegan