Hi, While testing ON CONFLICT DO SELECT on master, I found that the user's WHERE clause is evaluated against the existing row before the row is checked against the RLS policies. A user who cannot see a row through SELECT/UPDATE policies can therefore read its columns, as long as a proposed row conflicts with it on a unique key.
Both ExecOnConflictUpdate() and ExecOnConflictSelect() in
nodeModifyTable.c do ExecQual(onConflict*Where) first and only then
ExecWithCheckOptions(WCO_RLS_CONFLICT_CHECK). The CREATE POLICY docs
say the existing row is checked against the USING expressions first.
Repro (attached repro.sql, as superuser; then as a role that can't see
row 2):
insert into oc values (2,'u_sel','x')
on conflict (id) do select where leak(oc.secret) returning id;
NOTICE: LEAKED: HIDDEN-VALUE
where leak() is a plpgsql function that does RAISE NOTICE. The same
works with DO UPDATE. Without any function, a plain predicate is an
oracle: "where oc.secret like 'HID%'" raises the RLS error when it
matches and is a silent no-op when it doesn't. The attacker needs to
hit the hidden row's key, which is easy to enumerate for serial ids.
DO UPDATE has had this ordering since 168d5805e4 (2015); DO SELECT
(88327092ff) copied it. I have only tested master and 16beta2, I haven't
tested the other stable branches.
The attached patch swaps the order in both functions, so RLS is checked
before the WHERE clause. With it the repro no longer leaks, and the
regress suite passes. No existing test covers this, so it also needs
a regression test. One behavior change: a row failing the RLS check
now raises the error even when WHERE would have been false. That
matches what the docs describe.
Thanks,
João Marcelo
0001-Check-RLS-policies-before-evaluating-the-ON-CONFLICT.patch
Description: Binary data
repro.sql
Description: Binary data
