CREATE POLICY and RETURNING
Hi,
While I was checking the behavior of RLS, I found that the policy for SELECT
doesn't seem to be applied to RETURNING. Is this intentional? Please see
the following example.
CREATE ROLE foo LOGIN NOSUPERUSER;
CREATE TABLE hoge AS SELECT col FROM generate_series(1,10) col;
ALTER TABLE hoge ENABLE ROW LEVEL SECURITY;
GRANT SELECT, DELETE ON hoge TO foo;
CREATE POLICY hoge_select_policy ON hoge FOR SELECT TO foo USING (col < 4);
CREATE POLICY hoge_delete_policy ON hoge FOR DELETE TO foo USING (col < 8);
\c - foo
DELETE FROM hoge WHERE col = 6 RETURNING *;
The policy "hoge_select_policy" should disallow the user "foo" to see the row
with "col = 6". But the last DELETE RETURNING returns that row.
One minor suggestion is: what about changing the message as follows?
There are two more similar messages in policy.c, and they use the word
"row-policy" instead of "policy". For the consistency, I think that
the following also should use the word "row-policy".
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 3627539..df45385 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -551,7 +551,7 @@ CreatePolicy(CreatePolicyStmt *stmt)
if (HeapTupleIsValid(rsec_tuple))
ereport(ERROR,
(errcode(ERRCODE_DUPLICATE_OBJECT),
- errmsg("policy \"%s\" for relation
\"%s\" already exists",
+ errmsg("row-policy \"%s\" for
relation \"%s\" already exists",Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 10/16/2014 12:25 PM, Fujii Masao wrote:
Hi,
While I was checking the behavior of RLS, I found that the policy for SELECT
doesn't seem to be applied to RETURNING. Is this intentional?
This is why I was opposed to having a "SELECT" policy at all. It should
be "VISIBLE", "INSERT", "UPDATE", "DELETE".
I say "VISIBLE" instead of "READ" because I don't think the rows
affected by an UPDATE or DELETE should be affected by whether or not
they have a RETURNING clause. That's IMO nonsensical.and violates the
usual expectations about which clauses can have filtering effects.
So the read-filtering policy should apply to all statements. Not just
SELECT.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 10/16/2014 01:44 PM, Craig Ringer wrote:
So the read-filtering policy should apply to all statements. Not just
SELECT.
Oh, IIRC one wrinkle in the prior discussion about this was that doing
this will prevent the implementation of policies that permit users to
update/delete rows they cannot otherwise see.
That's an argument in favour of only applying a read-filtering policy
where a RETURNING clause is present, but that introduces the "surprise!
the effects of your DELETE changed based on an unrelated clause!" issue.
Keep in mind, when considering RETURNING, that users don't always add
this clause directly. PgJDBC will tack a RETURNING clause on the end of
a statement if the user requests generated keys, for example. They will
be very surprised if the behaviour of their DML changes based on whether
or not they asked to get generated keys.
To my mind having behaviour change based on RETURNING is actively wrong,
wheras policies that permit rows to be updated/deleted but not selected
are a nice-to-have at most.
I'd really like to see some more coverage of the details of how these
policies apply to inheritance, both the read- and write- sides of DML
with RETURNING clauses, etc.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Fujii,
* Fujii Masao (masao.fujii@gmail.com) wrote:
While I was checking the behavior of RLS, I found that the policy for SELECT
doesn't seem to be applied to RETURNING. Is this intentional? Please see
the following example.
Yes, it was intentional. That said, I'm not against changing it.
CREATE ROLE foo LOGIN NOSUPERUSER;
CREATE TABLE hoge AS SELECT col FROM generate_series(1,10) col;
ALTER TABLE hoge ENABLE ROW LEVEL SECURITY;
GRANT SELECT, DELETE ON hoge TO foo;
CREATE POLICY hoge_select_policy ON hoge FOR SELECT TO foo USING (col < 4);
CREATE POLICY hoge_delete_policy ON hoge FOR DELETE TO foo USING (col < 8);
\c - foo
DELETE FROM hoge WHERE col = 6 RETURNING *;The policy "hoge_select_policy" should disallow the user "foo" to see the row
with "col = 6". But the last DELETE RETURNING returns that row.
The DELETE USING policy allows DELETE to see the record and therefore
it's available for RETURNING.
One minor suggestion is: what about changing the message as follows?
There are two more similar messages in policy.c, and they use the word
"row-policy" instead of "policy". For the consistency, I think that
the following also should use the word "row-policy".
I was looking at these while going over the larger "try to be more
consistent" concerns, but that was leading me towards 'policy' instead
of 'row-policy', as the commands are 'CREATE POLICY', etc.
Not against going the other way, but it seems more consistent to do
'policy' everywhere..
Thanks!
Stephen
* Craig Ringer (craig@2ndquadrant.com) wrote:
On 10/16/2014 01:44 PM, Craig Ringer wrote:
So the read-filtering policy should apply to all statements. Not just
SELECT.Oh, IIRC one wrinkle in the prior discussion about this was that doing
this will prevent the implementation of policies that permit users to
update/delete rows they cannot otherwise see.
Yeah.
That's an argument in favour of only applying a read-filtering policy
where a RETURNING clause is present, but that introduces the "surprise!
the effects of your DELETE changed based on an unrelated clause!" issue.
No- if we were going to do this, I wouldn't want to change the existing
structure but rather provide either:
a) a way to simply disable RETURNING if the policy is in effect and the
policy creator doesn't wish to allow it
b) allow the user to define another clause which would be applied to the
rows in the RETURNING set
Keep in mind, when considering RETURNING, that users don't always add
this clause directly. PgJDBC will tack a RETURNING clause on the end of
a statement if the user requests generated keys, for example. They will
be very surprised if the behaviour of their DML changes based on whether
or not they asked to get generated keys.
Right- that consideration was one of the reasons to not mess with
RETURNING, in my view.
To my mind having behaviour change based on RETURNING is actively wrong,
wheras policies that permit rows to be updated/deleted but not selected
are a nice-to-have at most.I'd really like to see some more coverage of the details of how these
policies apply to inheritance, both the read- and write- sides of DML
with RETURNING clauses, etc.
I assume you mean with regard to documentation..? I'll work on
improving that.
Thanks!
Stephen
That's an argument in favour of only applying a read-filtering policy
where a RETURNING clause is present, but that introduces the "surprise!
the effects of your DELETE changed based on an unrelated clause!" issue.No- if we were going to do this, I wouldn't want to change the existing
structure but rather provide either:a) a way to simply disable RETURNING if the policy is in effect and the
policy creator doesn't wish to allow it
b) allow the user to define another clause which would be applied to the
rows in the RETURNING set
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.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
than this.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas wrote
That's an argument in favour of only applying a read-filtering policy
where a RETURNING clause is present, but that introduces the "surprise!
the effects of your DELETE changed based on an unrelated clause!" issue.No- if we were going to do this, I wouldn't want to change the existing
structure but rather provide either:a) a way to simply disable RETURNING if the policy is in effect and the
policy creator doesn't wish to allow it
b) allow the user to define another clause which would be applied to the
rows in the RETURNING setI think you could probably make the DELETE policy control what can get
deleted, but then have the SELECT policy further filter what gets
returned.
Without commenting on the usefulness of blind deletes...
How about returning a placeholder row but with all the values replaced with
NULL?
In the absence of returning does the delete count show the total number of
rows deleted or only the number of rows deleted that the user would be aware
of if they issued a select with the same criteria? Whatever the answer the
number of rows returned with returning should match the row count normally
noted.
David J.
--
View this message in context: http://postgresql.1045698.n5.nabble.com/CREATE-POLICY-and-RETURNING-tp5823192p5823374.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 17 October 2014 07:57, Craig Ringer <craig@2ndquadrant.com> 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
than this.
+1
This suggestion is most in line with what I would expect to occur.
Thom
On Fri, Oct 17, 2014 at 3:49 AM, Robert Haas <robertmhaas@gmail.com> wrote:
That's an argument in favour of only applying a read-filtering policy
where a RETURNING clause is present, but that introduces the "surprise!
the effects of your DELETE changed based on an unrelated clause!" issue.No- if we were going to do this, I wouldn't want to change the existing
structure but rather provide either:a) a way to simply disable RETURNING if the policy is in effect and the
policy creator doesn't wish to allow it
b) allow the user to define another clause which would be applied to the
rows in the RETURNING setI think you could probably make the DELETE policy control what can get
deleted, but then have the SELECT policy further filter what gets
returned.
+1
That's more intuitive to me because another security feature "privilege"
works in that way, i.e., SELECT privilege not DELETE controls RETURNING.
Another minor problem that I found is that pg_dump always fails when
there is a row-level policy for update. I think that the attached patch
should be applied.
Regards,
--
Fujii Masao
Attachments:
fix_update_policy_and_pg_dump_bug.patchtext/x-patch; charset=US-ASCII; name=fix_update_policy_and_pg_dump_bug.patchDownload+1-1
* Fujii Masao (masao.fujii@gmail.com) wrote:
Another minor problem that I found is that pg_dump always fails when
there is a row-level policy for update. I think that the attached patch
should be applied.
Urgh. That was tested before but we switched the characters used and
evidently missed that. :(
Will fix, thanks!
Stephen
* Thom Brown (thom@linux.com) wrote:
On 17 October 2014 07:57, Craig Ringer <craig@2ndquadrant.com> 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
than this.
+1
This suggestion is most in line with what I would expect to occur.
This was along the lines that I've been thinking for how to address this
also and I think it's the least surprising- but I want it controllable..
Thoughts on 'WITH RETURNING' / 'WITHOUT RETURNING' and what the default
should be?
Thanks!
Stephen
* David G Johnston (david.g.johnston@gmail.com) wrote:
How about returning a placeholder row but with all the values replaced with
NULL?
I don't think that would be a good approach.. A user actually looking
at those rows would be highly confused.
In the absence of returning does the delete count show the total number of
rows deleted or only the number of rows deleted that the user would be aware
of if they issued a select with the same criteria? Whatever the answer the
number of rows returned with returning should match the row count normally
noted.
Today, everything matches up, yes. Having rows which are deleted but
which don't show up in RETURNING could certainly surprise people and
applications, which is why I tend to favor the 'all-or-error' approach
that others have also suggested. Adding that wouldn't be difficult,
though we'd need to decide which should be the default.
Thanks!
Stephen
On Fri, Oct 17, 2014 at 7:46 AM, Stephen Frost <sfrost@snowman.net> wrote:
Thoughts on 'WITH RETURNING' / 'WITHOUT RETURNING' and what the default
should be?
That sounds like a horrendous pile of nasty complication for no good purpose.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Oct 17, 2014 at 5:34 AM, Andres Freund <andres@2ndquadrant.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]/messages/by-id/CAEZATCVE7hdtfZGCJN-oevVaWBtBGG8-fBCh9VhDBHuZrsWY5w@mail.gmail.com -- Peter Geoghegan. 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.
[1]: /messages/by-id/CAEZATCVE7hdtfZGCJN-oevVaWBtBGG8-fBCh9VhDBHuZrsWY5w@mail.gmail.com -- Peter Geoghegan
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 6 June 2015 at 22:16, Peter Geoghegan <pg@heroku.com> wrote:
On Fri, Oct 17, 2014 at 5:34 AM, Andres Freund <andres@2ndquadrant.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
On 6 June 2015 at 22:16, Peter Geoghegan <pg@heroku.com> wrote:
On Fri, Oct 17, 2014 at 5:34 AM, Andres Freund <andres@2ndquadrant.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.
I definitely don't like the idea of returning a portion of the data in a
RETURNING clause. Returning an error if the RETURNING clause ends up
not passing the SELECT policy is an interesting idea, but I do have
doubts about how often that will be a useful exercise. Another issue to
note is that, currently, SELECT policies never cause errors to be
returned, and this would change that.
There was discussion about a VISIBILITY policy instead of a SELECT
policy at one point. What I think we're seeing here is confusion about
the various policies which exist, because the USING policy of an UPDATE
is precisely its VISIBILITY policy, in my view, which is why I wasn't
bothered by the RETURNING clause being able to be used in that case. I
recall working on the documentation to make this clear, but it clearly
needs work.
The primary use-case for having a different VISIBILITY for UPDATE (or
DELETE) than for SELECT is that you wish to allow users to only modify a
subset of the rows in the table which they can see. I agree that the
current arrangement is that this can be used to allow users to UPDATE
records which they can't see (except through a RETURNING). Perhaps
that's a mistake and we should, instead, force the SELECT policy to be
included for the UPDATE and DELETE policies and have the USING clauses
from those be AND'd with the SELECT policy. That strikes me as a much
simpler approach than applying the SELECT policy against the RETURNING
clause and then throwing an error if it fails, though this would remove
a bit of flexibility in the system since we would no longer allow blind
UPDATEs or DELETEs. I'm not sure if that's really an issue though- to
compare it to our GRANT-based system, the only blind UPDATE allowed
there is one which goes across the entire table- any WHERE clause used
results in a check to ensure you have SELECT rights.
Ultimately, we're trying to provide as much flexibility as possible
while having the system be easily understandable by the policy authors
and allowing them to write simple policies to enforce the necessary
constraints. Perhaps allowing the open-ended USING clauses for UPDATE
and DELETE is simply making for a more complicated system as policy
authors then have to make sure they embed whatever SELECT policy they
have into their UPDATE and DELETE policies explicitly- and the result if
they don't do that correctly is that users may be able to update/delete
things they really shouldn't be able to. Now, as we've discussed
previously, users should be expected to test their systems and make sure
that they are behaving as they expect, but we don't want to put
landmines in either or have ways which they can be easily tripped up.
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.
Agreed- the SELECT policies for the non-target relations should be used,
that's clearly the right thing to do. Regarding the rest, I'd certainly
welcome your thoughts on my above response.
In principle, it sounds easy to do, but I think it will be much
simpler against my previous patch.
I'm planning to review your latest prior to PGCon. I really like that
you were able to eliminate the "extra" places we were adding the
default-deny policy and believe that will make the code a lot simpler to
follow.
Thanks!
Stephen
On 8 June 2015 at 16:53, Stephen Frost <sfrost@snowman.net> wrote:
I definitely don't like the idea of returning a portion of the data in a
RETURNING clause. Returning an error if the RETURNING clause ends up
not passing the SELECT policy is an interesting idea, but I do have
doubts about how often that will be a useful exercise. Another issue to
note is that, currently, SELECT policies never cause errors to be
returned, and this would change that.
True. Before UPSERT was added, it was the case that USING clauses from
all kinds of policies didn't cause errors, and CHECK clauses did, but
that has now changed for UPDATE, and I don't think it's necessarily a
problem to change it for SELECT, if we decide that it makes sense to
apply SELECT policies to rows returned by RETURNING clauses.
There was discussion about a VISIBILITY policy instead of a SELECT
policy at one point. What I think we're seeing here is confusion about
the various policies which exist, because the USING policy of an UPDATE
is precisely its VISIBILITY policy, in my view, which is why I wasn't
bothered by the RETURNING clause being able to be used in that case. I
recall working on the documentation to make this clear, but it clearly
needs work.
Yeah, perhaps there's scope for improving the documentation,
regardless of whether or not we change the current behaviour. One
place that we could add additional documentation is the pages
describing the INSERT, UPDATE and DELETE commands. These currently
each have a paragraph in their main description sections describing
what privileges they require, so perhaps we should add similar
paragraphs describing what RLS policies are checked, if RLS is enabled
on the table.
The primary use-case for having a different VISIBILITY for UPDATE (or
DELETE) than for SELECT is that you wish to allow users to only modify a
subset of the rows in the table which they can see. I agree that the
current arrangement is that this can be used to allow users to UPDATE
records which they can't see (except through a RETURNING). Perhaps
that's a mistake and we should, instead, force the SELECT policy to be
included for the UPDATE and DELETE policies and have the USING clauses
from those be AND'd with the SELECT policy. That strikes me as a much
simpler approach than applying the SELECT policy against the RETURNING
clause and then throwing an error if it fails,
I don't think that quite addresses the concern with RETURNING though
because the resulting clauses would only be applied to the old
(pre-update) data, whereas a RETURNING clause might still return new
data that you couldn't otherwise see.
though this would remove
a bit of flexibility in the system since we would no longer allow blind
UPDATEs or DELETEs. I'm not sure if that's really an issue though- to
compare it to our GRANT-based system, the only blind UPDATE allowed
there is one which goes across the entire table- any WHERE clause used
results in a check to ensure you have SELECT rights.
That's not quite the extent of it. Column-level privileges might allow
you to SELECT the PK column of a table, and then you might be able to
UPDATE or DELETE specific rows despite not being able to see their
entire contents. I can see cases where that might be useful, but I
have to admit that I'm struggling to think of any case where the
row-level equivalent would make sense (although that, by itself is not
a good argument for not allowing it).
Ultimately, we're trying to provide as much flexibility as possible
while having the system be easily understandable by the policy authors
and allowing them to write simple policies to enforce the necessary
constraints. Perhaps allowing the open-ended USING clauses for UPDATE
and DELETE is simply making for a more complicated system as policy
authors then have to make sure they embed whatever SELECT policy they
have into their UPDATE and DELETE policies explicitly- and the result if
they don't do that correctly is that users may be able to update/delete
things they really shouldn't be able to. Now, as we've discussed
previously, users should be expected to test their systems and make sure
that they are behaving as they expect, but we don't want to put
landmines in either or have ways which they can be easily tripped up.
Well I'm all for keeping this as simple and easily understandable as
possible. Right now the USING clauses of UPDATE policies control which
existing rows can be updated and the CHECK clauses control what new
data can go in. That's about as simple as it could be IMO, and trying
to merge SELECT policies with either of those would only make it more
complicated, and hence make it more likely for users to get it wrong.
OTOH, when you tag a RETURNING clause onto the UPDATE, it's kind of
like tying an UPDATE and a SELECT together, and it does make sense to
me that the new rows returned by the SELECT part of the command be
constrained by any SELECT policies on the table. That's consistent
with what happens with GRANT-based privileges, so I don't think that
it should be that surprising or difficult for users to understand.
I also think that if it ever were the case that a row resulting from
an UPDATE were not visible to the user who made that update, then that
would most likely indicate that the policies were poorly-defined.
Ideally the UPDATE policy's CHECK clause would prevent the user from
making such an update. Having a RETURNING clause that was checked
against the table's SELECT policy would then actually provide the user
with a way to validate the consistency of their policy clauses.
Hitting this new RETURNING-SELECT-POLICY error would be a sign that
perhaps the WITH CHECK clauses on your policies aren't properly
constraining new data.
So I actually think this additional check would increase the chances
of policies being defined correctly, and I think that consistency with
GRANT-based privileges will make it easier to understand.
Regards,
Dean
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Dean,
* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
On 8 June 2015 at 16:53, Stephen Frost <sfrost@snowman.net> wrote:
I definitely don't like the idea of returning a portion of the data in a
RETURNING clause. Returning an error if the RETURNING clause ends up
not passing the SELECT policy is an interesting idea, but I do have
doubts about how often that will be a useful exercise. Another issue to
note is that, currently, SELECT policies never cause errors to be
returned, and this would change that.True. Before UPSERT was added, it was the case that USING clauses from
all kinds of policies didn't cause errors, and CHECK clauses did, but
that has now changed for UPDATE, and I don't think it's necessarily a
problem to change it for SELECT, if we decide that it makes sense to
apply SELECT policies to rows returned by RETURNING clauses.
Fair enough.
There was discussion about a VISIBILITY policy instead of a SELECT
policy at one point. What I think we're seeing here is confusion about
the various policies which exist, because the USING policy of an UPDATE
is precisely its VISIBILITY policy, in my view, which is why I wasn't
bothered by the RETURNING clause being able to be used in that case. I
recall working on the documentation to make this clear, but it clearly
needs work.Yeah, perhaps there's scope for improving the documentation,
regardless of whether or not we change the current behaviour. One
place that we could add additional documentation is the pages
describing the INSERT, UPDATE and DELETE commands. These currently
each have a paragraph in their main description sections describing
what privileges they require, so perhaps we should add similar
paragraphs describing what RLS policies are checked, if RLS is enabled
on the table.
Agreed.
The primary use-case for having a different VISIBILITY for UPDATE (or
DELETE) than for SELECT is that you wish to allow users to only modify a
subset of the rows in the table which they can see. I agree that the
current arrangement is that this can be used to allow users to UPDATE
records which they can't see (except through a RETURNING). Perhaps
that's a mistake and we should, instead, force the SELECT policy to be
included for the UPDATE and DELETE policies and have the USING clauses
from those be AND'd with the SELECT policy. That strikes me as a much
simpler approach than applying the SELECT policy against the RETURNING
clause and then throwing an error if it fails,I don't think that quite addresses the concern with RETURNING though
because the resulting clauses would only be applied to the old
(pre-update) data, whereas a RETURNING clause might still return new
data that you couldn't otherwise see.
This part doesn't make sense to me. You can issue a SELECT statement
which will return records that are not visible to you also, by simply
running some transformation of the appropriate column as it comes out.
I don't believe it makes any sense to try and protect the results of a
transformation from the user who is defining what the transformations
are.
Worse, with this approach, it might *look* like RETURNING results are
being filtered in a way that prevents users from being able to see data
they shouldn't be allowed to see in the table via the SELECT policy when
they can- they just have to transform the appropriate column during the
UPDATE to get it to pass the UPDATE CHECK and SELECT USING policies.
Here is an example of what I mean:
CREATE TABLE my_data (
color text,
secret text
);
CREATE POLICY only_green ON my_data
FOR SELECT USING (color = 'green');
CREATE POLICY allow_update ON my_data FOR UPDATE
USING (true) -- a mistake
WITH CHECK (color = 'green');
If the attacker was interested in rows which had 'red' for the color,
they could simply run:
UPDATE my_data SET color = 'green' WHERE color = 'red' RETURNING *;
The above command returns all the 'secret' data for rows which had
a color of 'red' in the table, even though the SELECT policy clearly
doesn't allow the user to see those rows and the UPDATE WITH CHECK
policy only allows the user to create records with the color 'green',
and this would still be the case even with the proposed changes.
Note that the above WHERE clause isn't actually necessary at all, but
the 'color' column would need to be set to 'green' to pass the SELECT
and UPDATE policies, and therefore the attacker would lose the data in
that column- but we're not trying to hide just the data in that
particular column, nor do we want to have to define policies for every
column.
Basically, I don't think it makes much sense to try and enforce a SELECT
policy on a value which an attacker can set. This is very different
from enforcing a policy about adding rows to a table or enforcing a
policy about what rows are visible to a user based on the data already
in the table.
Ultimately, we're not actually protecting any data by filtering or
erroring based on the results of a transformation that the attacker gets
to define.
Worse, with this check, users might feel that they have their policies
correctly defined based on testing a command such as:
UPDATE my_data SET color = 'red' WHERE color = 'red' RETURNING *;
(or perhaps setting some other, unrelated, column but trying to pull out
the 'red' secret data)
As that would then result in an error from the new
RETURNING-SELECT-POLICY check, but the data wouldn't actually be
protected, as illustrated above.
though this would remove
a bit of flexibility in the system since we would no longer allow blind
UPDATEs or DELETEs. I'm not sure if that's really an issue though- to
compare it to our GRANT-based system, the only blind UPDATE allowed
there is one which goes across the entire table- any WHERE clause used
results in a check to ensure you have SELECT rights.That's not quite the extent of it. Column-level privileges might allow
you to SELECT the PK column of a table, and then you might be able to
UPDATE or DELETE specific rows despite not being able to see their
entire contents. I can see cases where that might be useful, but I
have to admit that I'm struggling to think of any case where the
row-level equivalent would make sense (although that, by itself is not
a good argument for not allowing it).
That's a good point that you could use the PK in the WHERE of an UPDATE
or DELETE and not have rights to SELECT the other columns. I can
imagine cases where users would want to be able to 'give away' rows
they can currently see, but I have a hard time coming up with a reason
to allow them to acquire rows they are not currently allowed to see. I
see that as an argument for including the SELECT policy implicitly
though, as I suggested above, but I take it that you're not advocating
for that, per your comments below.
Ultimately, we're trying to provide as much flexibility as possible
while having the system be easily understandable by the policy authors
and allowing them to write simple policies to enforce the necessary
constraints. Perhaps allowing the open-ended USING clauses for UPDATE
and DELETE is simply making for a more complicated system as policy
authors then have to make sure they embed whatever SELECT policy they
have into their UPDATE and DELETE policies explicitly- and the result if
they don't do that correctly is that users may be able to update/delete
things they really shouldn't be able to. Now, as we've discussed
previously, users should be expected to test their systems and make sure
that they are behaving as they expect, but we don't want to put
landmines in either or have ways which they can be easily tripped up.Well I'm all for keeping this as simple and easily understandable as
possible. Right now the USING clauses of UPDATE policies control which
existing rows can be updated and the CHECK clauses control what new
data can go in. That's about as simple as it could be IMO, and trying
to merge SELECT policies with either of those would only make it more
complicated, and hence make it more likely for users to get it wrong.
I like the flexibility and simplicity of having SELECT be independent of
the UPDATE and other policies and the ALL option for defining policies
certainly helps with that, when the policies for all operations have
the same clauses. That said, I can see an argument for having SELECT
policies enforced on the UPDATE USING clause as actually being simpler
as it means you don't have to think about the UPDATE USING clause
independently, unless you want to further reduce the set of rows
available through the policy.
Still, I'm not going to argue if we end up agreeing that what we have
already makes sense.
OTOH, when you tag a RETURNING clause onto the UPDATE, it's kind of
like tying an UPDATE and a SELECT together, and it does make sense to
me that the new rows returned by the SELECT part of the command be
constrained by any SELECT policies on the table. That's consistent
with what happens with GRANT-based privileges, so I don't think that
it should be that surprising or difficult for users to understand.
There is a big difference here though- there's no such thing as a 'new'
or 'old' row when talking about the GRANT system. In the GRANT system,
everything is checked at the outset of the command and the RETURNING
clause's columns are checked for SELECT rights, even if the entire
column has been replaced during the UPDATE statement with a constant or
other value defined by the user. Therefore, I don't believe you can
make the argument that the GRANT system supports this notion of checking
the SELECT policy against the 'new' rows being returned.
I also think that if it ever were the case that a row resulting from
an UPDATE were not visible to the user who made that update, then that
would most likely indicate that the policies were poorly-defined.
This is a case that I disagree with. As mentioned above, I can imagine
environments where it's acceptable to be able to "give away" rows.
Moving away from the security arena for a moment, consider a 'queue'
table where the rows move through different states. RLS might be a very
handy way to implement that queue by having different processes only
able to see the rows which have a particular state, but they can update
the rows to give them a different state which then moves them to another
processes' queue. Bringing it back to the security side, consider a
hospital where there can only be one 'responsible' physician assigned,
but they are allowed to trade cases around based on some external
criteria. Or the case files for a judge. Perhaps these change
operations should require an 'administrator' user to implement, but
perhaps not. Allowing users to acquire rows which they can't currently
see is a much more questionable action, in my view, as it implies
independent knowledge that such data exists to be acquired.
Ideally the UPDATE policy's CHECK clause would prevent the user from
making such an update. Having a RETURNING clause that was checked
against the table's SELECT policy would then actually provide the user
with a way to validate the consistency of their policy clauses.
Hitting this new RETURNING-SELECT-POLICY error would be a sign that
perhaps the WITH CHECK clauses on your policies aren't properly
constraining new data.
Agreed, the CHECK clause should prevent users from giving away records
in environments where that isn't appropriate. I'm not convinced that
adding a RETURNING-SELECT-POLICY error would actually be terribly useful
to users for checking that their policies are constructed correctly, see
above regarding my concerns about enforcing policies against the results
of the transformations requested by the user.
So I actually think this additional check would increase the chances
of policies being defined correctly, and I think that consistency with
GRANT-based privileges will make it easier to understand.
I'm afraid it wouldn't actually prevent attackers from getting access to
data they shouldn't have access to and I worry that it'd introduce
complications that would mask incorrect policy definitions, as discussed
above.
Thanks!
Stephen
