Very minor concern about RLS
========================
This existing ExecUpdate() comment seems a little inaccurate:
/*
* Check any RLS UPDATE WITH CHECK policies
*
* If we generate a new candidate tuple after EvalPlanQual testing, we
* must loop back here and recheck any RLS policies and constraints.
* (We don't need to redo triggers, however. If there are any BEFORE
* triggers then trigger.c will have done heap_lock_tuple to lock the
* correct tuple, so there's no need to do them again.)
Because ExecBRUpdateTriggers() does an UPSERT-style heap_lock_tuple()
call, restarting within ExecUpdate() should be impossible once
ExecBRUpdateTriggers() returns and returns a value != NULL (this is
*essential* for UPSERT, but happens to be true here, too).
Aside from this comment understating what can be relied on in a way
that seems a bit inaccurate, I worry about RLS leaks made possible by
EPQ, which is a hazard that other MVCC systems naturally don't have,
because unlike Postgres they have statement-level rollback to deal
with READ COMMITTED mode concurrent UPDATEs. Excuse me if I missed it,
but was this something that was considered?
What I'm imagining is a maliciously constructed UPDATE. It's run in
such a way that it could be allowed to UPDATE a row based on that not
being limited by the security barrier quals added by RLS. However,
having followed the UPDATE chain (i.e. just before "goto lreplace"),
the quals now don't pass and ExecUpdate() returns NULL (because of the
security barrier quals not passing EvalPlanQual() recheck on the new
tuple version -- the only possible reason, we'll say). The UPDATE is
a no-op, in the sense that it generates an identical new tuple
(identical to the old/existing one). By enabling log_lock_waits, the
attacker can observe that the new row version does not pass.
Why does this matter, given what is already obviously possible --
repeatedly selecting (or maybe "no-op updating") from the target table
in a READ COMMITTED xact, and seeing what goes away? Well, for one
thing, log_lock_waits will leak the XID on the concurrent (more
privileged) updater. It might then be trivial to reconstruct the
identity of the privileged updater based on some other row in some
other table, where xmin matches that privileged XID. Maybe that's a
hazard not peculiar to Postgres, though -- one can imagine a system
with statement-level rollback also leaking this information, and
perhaps that's just the way things are for every system with RLS, sort
of like constraint-related leaks. What I'm more concerned about is the
way things may not be fully consistent with the command's MVCC
snapshot, and that really is a thing peculiar to Postgres.
Arguable RLS security bug and EvalPlanQual()
====================================
It is certainly conceivable that an UPDATE's USING security barrier
qual contains a subquery referencing a relation that is not the
UPDATE's target relation -- CREATE POLICY is very flexible in
accepting such USING security barrier quals. Does this not introduce
security hazards around other relations being queried using the
command's MVCC snapshot, while the target has an EPQ-installed (only
dirty snapshot visible) tuple? See attached patch, an example
illustrating this arguable security problem using isolationtester. It
could certainly be argued that EvalPlanQual() allows an unacceptable
information leak, and I'd say that at the very least we need to
document this.
If you're using another well known MVCC database system that has RLS,
I imagine when this happens the attacker similarly waits on the
conflicting (privileged) xact to finish (in my example in the patch,
Bob's xact). However, unlike with the Postgres READ COMMITTED mode,
Mallory would then have her malicious UPDATE statement entirely rolled
back, and her statement would acquire an entirely new MVCC snapshot,
to be used by the USING security barrier qual (and everything else)
from scratch. This other system would then re-run her UPDATE with the
new MVCC snapshot. This would repeat until Mallory's UPDATE statement
completes without encountering any concurrent UPDATEs/DELETEs to her
would-be affected rows.
In general, with this other database system, an UPDATE must run to
completion without violating MVCC, even in READ COMMITTED mode. For
that reason, I think we can take no comfort from the presumption that
this flexibility in USING security barrier quals (allowing subqueries,
etc) works securely in this other system. (I actually didn't check
this out, but I imagine it's true).
I'm sorry if I just missed the discussion around this and this sounds
a bit alarmist. I want to satisfy myself that we have this right.
--
Peter Geoghegan
From 7b4530ed10b799a3826d046f586659cebcea34d5 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <peter.geoghega...@gmail.com>
Date: Sun, 31 May 2015 20:41:27 -0700
Subject: [PATCH 1/2] RLS isolation test
Illustrates what is arguably a security issue in RLS as implemented.
Test should fail on master branch, although exact "expected" output does
not necessarily reflect my opinion of what users ought to expect, if
indeed we need to do anything differently here at all (beyond
documenting the issue).
---
src/test/isolation/expected/rls-problem.out | 18 ++++++
src/test/isolation/isolation_schedule | 1 +
src/test/isolation/specs/rls-problem.spec | 97 +++++++++++++++++++++++++++++
3 files changed, 116 insertions(+)
create mode 100644 src/test/isolation/expected/rls-problem.out
create mode 100644 src/test/isolation/specs/rls-problem.spec
diff --git a/src/test/isolation/expected/rls-problem.out b/src/test/isolation/expected/rls-problem.out
new file mode 100644
index 0000000..e4d40c7
--- /dev/null
+++ b/src/test/isolation/expected/rls-problem.out
@@ -0,0 +1,18 @@
+Parsed test spec with 2 sessions
+
+starting permutation: selectm no_trust_mallory update_hc updatem commit_hc selectm abort_malice
+step selectm: select 'mallory select: ' m, * from information where group_id = 2;
+m info group_id
+
+mallory select: slightly secret2
+step no_trust_mallory: update users set group_id = 1 where user_name = 'mallory';
+step update_hc: update information set info = 'secret' where group_id = 2;
+step updatem: update information set info = info where group_id = 2 returning 'mallory update: ' m, *; <waiting ...>
+step commit_hc: commit;
+step updatem: <... completed>
+m info group_id
+
+step selectm: select 'mallory select: ' m, * from information where group_id = 2;
+m info group_id
+
+step abort_malice: abort;
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index c0ed637..457dc24 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -20,6 +20,7 @@ test: insert-conflict-do-nothing
test: insert-conflict-do-update
test: insert-conflict-do-update-2
test: insert-conflict-do-update-3
+test: rls-problem
test: delete-abort-savept
test: delete-abort-savept-2
test: aborted-keyrevoke
diff --git a/src/test/isolation/specs/rls-problem.spec b/src/test/isolation/specs/rls-problem.spec
new file mode 100644
index 0000000..25890c4
--- /dev/null
+++ b/src/test/isolation/specs/rls-problem.spec
@@ -0,0 +1,97 @@
+# User's security level must be higher than or equal to information.
+setup
+{
+ create table groups (group_id int4 primary key, group_name text not null);
+ create table users (user_name text primary key, group_id int4 not null references groups);
+ create table information (info text, group_id int4 not null references groups);
+
+ insert into groups values (1, 'low'), (2, 'medium'), (5, 'high');
+ create user bob;
+ create user mallory;
+ insert into users values('bob', 5), ('mallory', 2), ('alice', 2);
+ insert into information values ('barely secret', 1), ('slightly secret', 2), ('very secret', 5);
+
+ alter table information enable row level security;
+ grant select on users to public;
+ grant select on groups to public;
+ grant all on information to public;
+ grant all on groups to bob;
+ grant all on users to bob;
+
+ create policy fp on information for update using (group_id <= (select group_id from users where user_name = current_user));
+ create policy fp2 on information for select using (group_id <= (select group_id from users where user_name = current_user));
+}
+
+teardown
+{
+ reset session authorization;
+ drop policy fp on information;
+ drop policy fp2 on information;
+ drop table users;
+ drop table information;
+ drop table groups;
+ drop user bob;
+ drop user mallory;
+}
+
+session "bobsession"
+setup
+{
+ begin isolation level read committed;
+ set session authorization bob;
+}
+# In principle, users in the medium clearance group should be able to see this
+# new secret information, but in light of the new information being somewhat
+# more confidential than what we'd trust mallory with, better demote her first.
+#
+# In other words, bob had informally considered mallory somewhat less
+# trustworthy than the rest of her group all along, a distinction that will now
+# start to matter, or perhaps has just learned of new information that calls
+# her character into question (maybe that's the secret). In light of this
+# secret information being made available to group 2, bob formalizes the fact
+# that mallory is not quite as trustworthy as other members of that group by
+# demoting her so she can't see this new information (the new information is
+# the string "secret" in this example scenario).
+#
+# bob does so in a single transaction, first updating mallory's group/security
+# clearance, and finally changing the information now presumed only visible to
+# the remaining members of group 2.
+step "no_trust_mallory" { update users set group_id = 1 where user_name = 'mallory'; }
+step "update_hc" { update information set info = 'secret' where group_id = 2; }
+step "commit_hc" { commit; }
+
+session "mallorysession"
+setup
+{
+ begin isolation level read committed;
+ set session authorization mallory;
+}
+# mallory anticipates this situation, despite not being aware of the exact
+# nature of the more secret information that only the remaining members of her
+# former group (group 2, now comprised only of alice) and bob himself ought to
+# be privy to.
+#
+# In practice, mallory probably writes a script that runs the update in an
+# infinite loop until something changes. With a little additional trickery,
+# mallory could make sure her MVCC snapshot was several minutes old before
+# reaching what is by then bob's updated tuple within the information table,
+# thereby virtually ensuring that the scenario plays out with the string
+# "secret" leaked to mallory.
+step "selectm" { select 'mallory select: ' m, * from information where group_id = 2; }
+step "updatem" { update information set info = info where group_id = 2 returning 'mallory update: ' m, *; }
+# possibly useful in covering mallory's tracks:
+step "abort_malice" { abort; }
+
+# First "selectm" run here will show mallory the string "slightly secret". She
+# was always trustworthy enough to see that, so the fact that she can see it
+# initially isn't a problem, or at least isn't a problem that Postgres can
+# reasonably be expected to do anything about. The problem is that she sees
+# the string "secret", contrary to bob's reasonable expectation that she can
+# never see that string.
+#
+# When her malicious update finishes, she then runs an update with a new MVCC
+# snapshot that verifies that she now cannot see "secret" (of the rows in the
+# table information, she is now only able to return "barely secret"). Too late
+# for that, though: the dirty snapshot used by her malicious update already
+# showed her what she was not supposed to be able to see.
+permutation "selectm" "no_trust_mallory" "update_hc" "updatem" "commit_hc" "selectm" "abort_malice"
--
1.9.1
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
