Обсуждение: Row security violation error is misleading

Craig,

* Craig Ringer (craig@2ndquadrant.com) wrote:
> When attempting to insert a row that violates a row security policy that
> applies to writes, the error message emitted references WITH CHECK OPTION,
> even though (as far as the user knows) there's no such thing involved.
> If you understand the internals you'd know that row security re-uses the
> same logic as WITH CHECK OPTION, but it's going to be confusing for users.

Agreed and we actually have a patch from Dean already to address this,
it's just been waiting on me (with a couple of other ones).  It'd
certainly be great if you have time to take a look at those, though,
generally speaking, I feel prety happy about where those are and believe
they really just need to be reviewed/tested and maybe a bit of word-
smithing around the docs.
Thanks!
    Stephen

On Tue, Apr 7, 2015 at 5:11 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
> postgres=> INSERT INTO clients (account_name, account_manager) VALUES
> ('peters', 'peter'), ('johannas', 'johanna');
> ERROR:  44000: new row violates WITH CHECK OPTION for "clients"
> DETAIL:  Failing row contains (7, johannas, johanna).
> LOCATION:  ExecWithCheckOptions, execMain.c:1683
>
>
> ... yet "clients" is a table, not a view, and cannot have a WITH CHECK
> OPTION clause.
>
> There is no reference to the policy being violated or to the fact that it's
> row security involved.


FWIW, I also think that this is very confusing.

-- 
Peter Geoghegan

On 7 April 2015 at 13:11, Craig Ringer <craig@2ndquadrant.com> wrote:
> When attempting to insert a row that violates a row security policy that
> applies to writes, the error message emitted references WITH CHECK OPTION,
> even though (as far as the user knows) there's no such thing involved.
> If you understand the internals you'd know that row security re-uses the
> same logic as WITH CHECK OPTION, but it's going to be confusing for users.
>
> postgres=> INSERT INTO clients (account_name, account_manager) VALUES
> ('peters', 'peter'), ('johannas', 'johanna');
> ERROR:  44000: new row violates WITH CHECK OPTION for "clients"
> DETAIL:  Failing row contains (7, johannas, johanna).
> LOCATION:  ExecWithCheckOptions, execMain.c:1683
>
>
> ... yet "clients" is a table, not a view, and cannot have a WITH CHECK
> OPTION clause.
>
> There is no reference to the policy being violated or to the fact that it's
> row security involved.
>
> I think this is going to be very confusing for users. I was expecting to see
> something more like:
>
> ERROR:  44000: new row in table 'clients' violates row level security policy
> 'just_own_clients'
>

Yes, I agree - that's a bit confusing.

Note that it doesn't make sense to ask which RLS policy was violated.
There is a default deny policy in place, and each defined policy's
quals are combined using OR, so when there are multiple policies this
error indicates that none of the policies passed (in a sense, they
were all violated).


> Re-using the SQLSTATE 44000 is a bit iffy too. We should probably define
> something to differentiate this, like:
>
>    44P01 ROW SECURITY WRITE POLICY VIOLATION
>

Yes, that sounds sensible.

Regards,
Dean

On 7 April 2015 at 16:21, Stephen Frost <sfrost@snowman.net> wrote:
> Agreed and we actually have a patch from Dean already to address this,
> it's just been waiting on me (with a couple of other ones).  It'd
> certainly be great if you have time to take a look at those, though,
> generally speaking, I feel prety happy about where those are and believe
> they really just need to be reviewed/tested and maybe a bit of word-
> smithing around the docs.
>

The first of those patches [1] has bit-rotted somewhat, due to the
recent changes to the way rowmarks are handled, so here's an updated
version. It wasn't a trivial merge, because commit
cb1ca4d800621dcae67ca6c799006de99fa4f0a5 made a change to
ExecBuildAuxRowMark() without a matching change to
preprocess_targetlist(), and one of the new RLS-with-inheritance tests
fell over that.

This is not a complete review of RLS, but it does fix a number of issues:

1). In prepend_row_security_policies(), defaultDeny was always true,
so if there were any hook policies, the RLS policies on the table
would just get discarded.

2). In prepend_row_security_policies(), I think it is better to have
any table RLS policies applied before any hook policies, so that a
hook cannot be used to bypass built-in RLS.

3). The infinite recursion detection in fireRIRrules() didn't properly
manage the activeRIRs list in the case of WCOs, so it would
incorrectly report infinite recusion if the same relation with RLS
appeared more than once in the rtable, for example "UPDATE t ... FROM
t ...".

4). The RLS expansion code in fireRIRrules() was handling RLS in the
main loop through the rtable. This lead to RTEs being visited twice if
they contained sublink subqueries, which
prepend_row_security_policies() attempted to handle by exiting early
if the RTE already had securityQuals. That didn't work, however, since
if the query involved a security barrier view on top of a table with
RLS, the RTE would already have securityQuals (from the view) by the
time fireRIRrules() was invoked, and so the table's RLS policies would
be ignored. This is most easily fixed in fireRIRrules() by handling
RLS in a separate loop at the end, after dealing with any other
sublink subqueries, thus ensuring that each RTE is only visited once
for RLS expansion.

5). The inheritance planner code didn't correctly handle non-target
relations with RLS, which would get turned into subqueries during
planning. Thus an update of the form "UPDATE t1 ... FROM t2 ..." where
t1 has inheritance and t2 has RLS quals would fail.

6). process_policies() was adding WCOs to non-target relations, which
is unnecessary, and could lead to a lot of wasted time in the rewriter
and the planner. WCOs are only needed on the result relation.


The second patch [2] is the one that is actually relevant to this
thread. This patch is primarily to apply the RLS checks earlier,
before an update is attempted, more like a regular permissions check.
This adds a new enum to classify the kinds of WCO, a side benefit of
which is that it allows different error messages when RLS checks are
violated, as opposed to WITH CHECK OPTIONs on views.

I actually re-used the sql status code 42501 -
ERRCODE_INSUFFICIENT_PRIVILEGE for a RLS check failure because of the
parallel with permissions checks, but I quite like Craig's idea of
inventing a new status code for this, so that it can be more easily
distinguished from a lack of GRANTed privileges.

There's another side benefit to this patch, which is that the new enum
could be extended to include a new kind of WCO for a check of the
USING quals of a RLS UPDATE policy on the update path of an INSERT..ON
CONFLICT UPDATE. As I pointed out on that thread, I think these quals
need to be treated differently from the WITH CHECK quals of a RLS
UPDATE policy, since they ought to apply to different tuples.
Therefore classifying the WCOs by command type is insufficient to
distinguish the 2 cases.

Regards,
Dean


[1] http://www.postgresql.org/message-id/CAEZATCVoaNiy5qLGda4K-nmP7GbJJgpVGucBAtA1ckVm-uWvgg@mail.gmail.com

[2] http://www.postgresql.org/message-id/CAEZATCVz1u3dyjdzXN3F26rxks2BYXDz--_2rTZfRhuU0zfWSw@mail.gmail.com

Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

>> Re-using the SQLSTATE 44000 is a bit iffy too. We should
>> probably define something to differentiate this, like:
>>
>>    44P01 ROW SECURITY WRITE POLICY VIOLATION
>
> Yes, that sounds sensible.

I would be more inclined to use:

42501  ERRCODE_INSUFFICIENT_PRIVILEGE

I know this is used 173 other places where a user attempts to do
something they are not authorized to do, so you would not be able
to differentiate the specific cause based on SQLSTATE if this is
used -- but why don't we feel that way about the other 173 causes?
Why does this security violation require a separate SQLSTATE?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Kevin Grittner (kgrittn@ymail.com) wrote:
> Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> >> Re-using the SQLSTATE 44000 is a bit iffy too. We should
> >> probably define something to differentiate this, like:
> >>
> >>    44P01 ROW SECURITY WRITE POLICY VIOLATION
> >
> > Yes, that sounds sensible.
>
> I would be more inclined to use:
>
> 42501  ERRCODE_INSUFFICIENT_PRIVILEGE
>
> I know this is used 173 other places where a user attempts to do
> something they are not authorized to do, so you would not be able
> to differentiate the specific cause based on SQLSTATE if this is
> used -- but why don't we feel that way about the other 173 causes?
> Why does this security violation require a separate SQLSTATE?

I tend to agree with this and it feels more consistent.  SQLSTATE is
already a very generic response system and knowing that it's a policy
violation instead of a GRANT violations strikes me as unlikely to be
terribly interesting at the level where you're just looking at the
SQLSTATE code.
Thanks!
    Stephen

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 7 April 2015 at 16:21, Stephen Frost <sfrost@snowman.net> wrote:
> > Agreed and we actually have a patch from Dean already to address this,
> > it's just been waiting on me (with a couple of other ones).  It'd
> > certainly be great if you have time to take a look at those, though,
> > generally speaking, I feel prety happy about where those are and believe
> > they really just need to be reviewed/tested and maybe a bit of word-
> > smithing around the docs.
>
> The first of those patches [1] has bit-rotted somewhat, due to the
> recent changes to the way rowmarks are handled, so here's an updated
> version. It wasn't a trivial merge, because commit
> cb1ca4d800621dcae67ca6c799006de99fa4f0a5 made a change to
> ExecBuildAuxRowMark() without a matching change to
> preprocess_targetlist(), and one of the new RLS-with-inheritance tests
> fell over that.

Thanks!

> 1). In prepend_row_security_policies(), defaultDeny was always true,
> so if there were any hook policies, the RLS policies on the table
> would just get discarded.

That was certainly unintentional (and wasn't the case at one point..  I
recall specifically checking that), thanks for addressing it.

> 2). In prepend_row_security_policies(), I think it is better to have
> any table RLS policies applied before any hook policies, so that a
> hook cannot be used to bypass built-in RLS.

While I agree that we want to include the RLS policies defined against
the table prior to any policies which are added by the hook, I don't
quite follow what you mean by "cannot be used to bypass built-in RLS".
If we allow, as intended, extensions to define their own policies then
it's entirely possible for the extension to return a "allow all" policy,
and I believe that's perfectly fine.

The idea has come up a couple of times to also have "restrictive"
policies and I agree that's an interesting idea and, once implemented,
we would want to allow extensions to define both kinds and make sure
that we apply "restrictive" policies defined in table-level policies
in addition to policies from extensions, but we're not quite there yet.

> 3). The infinite recursion detection in fireRIRrules() didn't properly
> manage the activeRIRs list in the case of WCOs, so it would
> incorrectly report infinite recusion if the same relation with RLS
> appeared more than once in the rtable, for example "UPDATE t ... FROM
> t ...".

Right, thanks for working through that and providing a fix.

> 4). The RLS expansion code in fireRIRrules() was handling RLS in the
> main loop through the rtable. This lead to RTEs being visited twice if
> they contained sublink subqueries, which
> prepend_row_security_policies() attempted to handle by exiting early
> if the RTE already had securityQuals. That didn't work, however, since
> if the query involved a security barrier view on top of a table with
> RLS, the RTE would already have securityQuals (from the view) by the
> time fireRIRrules() was invoked, and so the table's RLS policies would
> be ignored. This is most easily fixed in fireRIRrules() by handling
> RLS in a separate loop at the end, after dealing with any other
> sublink subqueries, thus ensuring that each RTE is only visited once
> for RLS expansion.

Agreed.

> 5). The inheritance planner code didn't correctly handle non-target
> relations with RLS, which would get turned into subqueries during
> planning. Thus an update of the form "UPDATE t1 ... FROM t2 ..." where
> t1 has inheritance and t2 has RLS quals would fail.

Urgh.  Thanks for the testing and fix for that.

> 6). process_policies() was adding WCOs to non-target relations, which
> is unnecessary, and could lead to a lot of wasted time in the rewriter
> and the planner. WCOs are only needed on the result relation.

Ah, yes, that makes sense.

> The second patch [2] is the one that is actually relevant to this
> thread. This patch is primarily to apply the RLS checks earlier,
> before an update is attempted, more like a regular permissions check.
> This adds a new enum to classify the kinds of WCO, a side benefit of
> which is that it allows different error messages when RLS checks are
> violated, as opposed to WITH CHECK OPTIONs on views.

Right.

> I actually re-used the sql status code 42501 -
> ERRCODE_INSUFFICIENT_PRIVILEGE for a RLS check failure because of the
> parallel with permissions checks, but I quite like Craig's idea of
> inventing a new status code for this, so that it can be more easily
> distinguished from a lack of GRANTed privileges.

As I mentioned to Kevin, I'm not sure that this is really a useful
distinction.  I'm quite curious if other systems provide that
distinction between grant violations and policy violations.  If they do
then that would certainly bolster the argument to provide the
distinction in PG.

> There's another side benefit to this patch, which is that the new enum
> could be extended to include a new kind of WCO for a check of the
> USING quals of a RLS UPDATE policy on the update path of an INSERT..ON
> CONFLICT UPDATE. As I pointed out on that thread, I think these quals
> need to be treated differently from the WITH CHECK quals of a RLS
> UPDATE policy, since they ought to apply to different tuples.
> Therefore classifying the WCOs by command type is insufficient to
> distinguish the 2 cases.

Good point.
Thanks!
    Stephen

On 8 April 2015 at 16:27, Stephen Frost <sfrost@snowman.net> wrote:
>> 2). In prepend_row_security_policies(), I think it is better to have
>> any table RLS policies applied before any hook policies, so that a
>> hook cannot be used to bypass built-in RLS.
>
> While I agree that we want to include the RLS policies defined against
> the table prior to any policies which are added by the hook, I don't
> quite follow what you mean by "cannot be used to bypass built-in RLS".
> If we allow, as intended, extensions to define their own policies then
> it's entirely possible for the extension to return a "allow all" policy,
> and I believe that's perfectly fine.
>

That doesn't match what the code currently does:
    * Also, allow extensions to add their own policies.    *    * Note that, as with the internal policies, if multiple
policiesare    * returned then they will be combined into a single expression with    * all of them OR'd together.
However,to avoid the situation of an    * extension granting more access to a table than the internal policies    *
wouldallow, the extension's policies are AND'd with the internal    * policies.  In other words - extensions can only
providefurther    * filtering of the result set (or further reduce the set of records    * allowed to be added).
 

which seems reasonable, and means that if there are both internal and
external policies, an "allow all" external policy would be a no-op.

All the patch does is to ensure that the quals from the external
policies are on the outer security barrier, so that if they contain
leaky functions they cannot leak data that doesn't pass the internal
policies (like a SB view on top of another SB view).

Regards,
Dean

On 8 April 2015 at 16:27, Stephen Frost <sfrost@snowman.net> wrote:
> * Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
>> I actually re-used the sql status code 42501 -
>> ERRCODE_INSUFFICIENT_PRIVILEGE for a RLS check failure because of the
>> parallel with permissions checks, but I quite like Craig's idea of
>> inventing a new status code for this, so that it can be more easily
>> distinguished from a lack of GRANTed privileges.
>
> As I mentioned to Kevin, I'm not sure that this is really a useful
> distinction.  I'm quite curious if other systems provide that
> distinction between grant violations and policy violations.  If they do
> then that would certainly bolster the argument to provide the
> distinction in PG.
>

OK, on further reflection I think that's probably right.

ERRCODE_INSUFFICIENT_PRIVILEGE is certainly more appropriate than
anything based on a WCO violation, because it reflects the fact that
the current user isn't allowed to perform the insert/update, but
another user might be allowed, so this is a privilege problem, not a
data error.

Regards,
Dean

* Craig Ringer (craig@2ndquadrant.com) wrote:
> On 9 April 2015 at 01:30, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> > That doesn't match what the code currently does:

Ah, right.

> >      * Also, allow extensions to add their own policies.
> >      *
> >      * Note that, as with the internal policies, if multiple policies are
> >      * returned then they will be combined into a single expression with
> >      * all of them OR'd together.  However, to avoid the situation of an
> >      * extension granting more access to a table than the internal policies
> >      * would allow, the extension's policies are AND'd with the internal
> >      * policies.  In other words - extensions can only provide further
> >      * filtering of the result set (or further reduce the set of records
> >      * allowed to be added).
> >
> > which seems reasonable, and means that if there are both internal and
> > external policies, an "allow all" external policy would be a no-op.
>
> Great, I'm glad to see that they're ANDed now.
>
> I wasn't caught up with the current state of this. At some earlier point
> policies from hooks were being ORed, which made mandatory access control
> extensions impossible.

That's what I had been recalling also.  I'm certainly on-board with
wanting to support MAC, but I'm wondering what we're going to do when we
add support for "restrictive" policies.  We'd certainly want extensions
to be able to provide both kinds and we will need to make sure they are
added correctly, with all of the restrictive policies being combined
and applied together, and then the permissive policies similairly
combined (but with OR's).

Thoughts?
Thanks!
    Stephen

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 7 April 2015 at 16:21, Stephen Frost <sfrost@snowman.net> wrote:
> > Agreed and we actually have a patch from Dean already to address this,
> > it's just been waiting on me (with a couple of other ones).  It'd
> > certainly be great if you have time to take a look at those, though,
> > generally speaking, I feel prety happy about where those are and believe
> > they really just need to be reviewed/tested and maybe a bit of word-
> > smithing around the docs.
>
> The first of those patches [1] has bit-rotted somewhat, due to the
> recent changes to the way rowmarks are handled, so here's an updated
> version. It wasn't a trivial merge, because commit
> cb1ca4d800621dcae67ca6c799006de99fa4f0a5 made a change to
> ExecBuildAuxRowMark() without a matching change to
> preprocess_targetlist(), and one of the new RLS-with-inheritance tests
> fell over that.

Thanks a lot for this.  Please take a look at the attached.  It still
includes the preprocess_targetlist() changes, so the regression tests
don't fail, but that'll be committed independently (hopefully soon).

I've taken an initial look at the second patch (actually, a few times)
and plan to do a thorough review soon.  I'd definitely like to get these
both committed and done with very shortly.  Again, apologies about the
delays; this past weekend was quite a bit busier than I originally
anticipated.

    Thanks!

        Stephen

On 21 April 2015 at 20:50, Stephen Frost <sfrost@snowman.net> wrote:
> Thanks a lot for this.  Please take a look at the attached.

I've given this a quick read-through, and it looks good to me. The
interaction of permissive and restrictive policies from hooks matches
my expections, and it's a definite improvement having tests for RLS
hooks.

The only thing I spotted was that the file comment for
test_rls_hooks.c needs updating.

Is there any documentation for hooks? If not, perhaps that's something
we should be considering too.

Regards,
Dean

On 21 April 2015 at 22:21, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On 21 April 2015 at 20:50, Stephen Frost <sfrost@snowman.net> wrote:
>> Thanks a lot for this.  Please take a look at the attached.
>
> I've given this a quick read-through, and it looks good to me. The
> interaction of permissive and restrictive policies from hooks matches
> my expections, and it's a definite improvement having tests for RLS
> hooks.
>
> The only thing I spotted was that the file comment for
> test_rls_hooks.c needs updating.
>

So re-reading this, I spotted what looks like another (pre-existing)
bug. In process_policies() there's a loop over all the policies,
collecting quals and with_check_quals, then a test at the end to use
the USING quals for the WITH CHECK quals if there were no
with_check_quals. I think we want to instead do that test inside the
loop -- i.e., for each policy, if there is no with_check_qual *for
that policy*, use it's USING qual instead.

Regards,
Dean

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 21 April 2015 at 22:21, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> > On 21 April 2015 at 20:50, Stephen Frost <sfrost@snowman.net> wrote:
> >> Thanks a lot for this.  Please take a look at the attached.
> >
> > I've given this a quick read-through, and it looks good to me. The
> > interaction of permissive and restrictive policies from hooks matches
> > my expections, and it's a definite improvement having tests for RLS
> > hooks.
> >
> > The only thing I spotted was that the file comment for
> > test_rls_hooks.c needs updating.
>
> So re-reading this, I spotted what looks like another (pre-existing)
> bug. In process_policies() there's a loop over all the policies,
> collecting quals and with_check_quals, then a test at the end to use
> the USING quals for the WITH CHECK quals if there were no
> with_check_quals. I think we want to instead do that test inside the
> loop -- i.e., for each policy, if there is no with_check_qual *for
> that policy*, use it's USING qual instead.

Agreed, the USING -> WITH CHECK copy should be happening for all
policies independently, not wholesale at the end.

I've updated my tree and am testing.
Thanks!
    Stephen

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 21 April 2015 at 22:21, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> > On 21 April 2015 at 20:50, Stephen Frost <sfrost@snowman.net> wrote:
> >> Thanks a lot for this.  Please take a look at the attached.
> >
> > I've given this a quick read-through, and it looks good to me. The
> > interaction of permissive and restrictive policies from hooks matches
> > my expections, and it's a definite improvement having tests for RLS
> > hooks.
> >
> > The only thing I spotted was that the file comment for
> > test_rls_hooks.c needs updating.
>
> So re-reading this, I spotted what looks like another (pre-existing)
> bug. In process_policies() there's a loop over all the policies,
> collecting quals and with_check_quals, then a test at the end to use
> the USING quals for the WITH CHECK quals if there were no
> with_check_quals. I think we want to instead do that test inside the
> loop -- i.e., for each policy, if there is no with_check_qual *for
> that policy*, use it's USING qual instead.

Pushed with those changes, please take a look and test!

Thanks again for all of your help with this.  I'm going to be looking
over that second patch with an eye towards getting it in very soon, it's
been kicking around for far longer than it should have been and that's
my fault, apologies about that.
Stephen

On 22 April 2015 at 17:02, Stephen Frost <sfrost@snowman.net> wrote:
> Pushed with those changes, please take a look and test!
>

Excellent, thanks! Will test.

Regards,
Dean

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 21 April 2015 at 20:50, Stephen Frost <sfrost@snowman.net> wrote:
> > Thanks a lot for this.  Please take a look at the attached.
>
> I've given this a quick read-through, and it looks good to me. The
> interaction of permissive and restrictive policies from hooks matches
> my expections, and it's a definite improvement having tests for RLS
> hooks.
>
> The only thing I spotted was that the file comment for
> test_rls_hooks.c needs updating.

Fixed!

> Is there any documentation for hooks? If not, perhaps that's something
> we should be considering too.

We don't generally document hooks beyond in the source code..  I'm happy
to expand on what's there, if anyone feels it'd be helpful to do so.
Having the test module is a form of documentation also, of course.
Thanks!
    Stephen

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> The second patch [2] is the one that is actually relevant to this
> thread. This patch is primarily to apply the RLS checks earlier,
> before an update is attempted, more like a regular permissions check.
> This adds a new enum to classify the kinds of WCO, a side benefit of
> which is that it allows different error messages when RLS checks are
> violated, as opposed to WITH CHECK OPTIONs on views.

I've gone ahead and pushed this, please take a look and test it and let
me know if you see any issues or have any questions or concerns.
Thanks!
    Stephen

On 25 April 2015 at 01:52, Stephen Frost <sfrost@snowman.net> wrote:
> * Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
>> The second patch [2] is the one that is actually relevant to this
>> thread. This patch is primarily to apply the RLS checks earlier,
>> before an update is attempted, more like a regular permissions check.
>> This adds a new enum to classify the kinds of WCO, a side benefit of
>> which is that it allows different error messages when RLS checks are
>> violated, as opposed to WITH CHECK OPTIONs on views.
>
> I've gone ahead and pushed this, please take a look and test it and let
> me know if you see any issues or have any questions or concerns.
>

Brilliant, thanks! I gave it a quick read-through and all the changes
look good, so thanks for all your work on this.

Regards,
Dean