Thread: RLS restrictive hook policies

In get_row_security_policies():
   /*    * If the only built-in policy is the default-deny one, and hook policies    * exist, then use the hook
policiesonly and do not apply the    * default-deny policy.  Otherwise, we will apply both sets below.    */   if
(defaultDeny&&       (hook_policies_restrictive != NIL || hook_policies_permissive != NIL))   {       rowsec_expr =
NULL;      rowsec_with_check_expr = NULL;   }
 

So if the only policies that exist are restrictive policies from an
extension, the default-deny policy is not applied and the restrictive
policies are applied instead, which is effectively a "default-allow"
situation with each restrictive policy further limiting access to the
table.

I think this is potentially very dangerous when both kinds of policy
exist. Consider for example a situation where initially multiple
permissive policies are set up for different users allowing them
access to different subsets of the table, and users not covered by any
of those permissive policies have no access. Then suppose that later a
restrictive policy is added, applying to all users -- the intention
being to prevent any user from accessing some particularly sensitive
data. The result of adding that restrictive policy would be that all
the users who previously had no access at all would suddenly have
access to everything not prohibited by the restrictive policy.

That goes against everything I would expect from a restrictive policy
-- adding more restrictive policies should only ever reduce the number
of rows visible, not ever open up more. So I think the test above
should only disable the default-deny policy if there are any
permissive policies from the extension.

Of course that will make life a little harder for people who only want
to use restrictive policies, since they will be forced to first add a
permissive policy, possibly just one that's always true, but I think
it's the safest approach.

Possibly if/when we add proper SQL support for defining restrictive
policies, we might also add an option to ALTER TABLE ... ENABLE ROW
LEVEL SECURITY to allow a table to have RLS enabled in default-allow
mode, for users who know they only want to add restrictive policies.
However, I think that should be something the user has to explicitly
ask for, not an automatic decision based on the existence of
restrictive policies.

Thoughts?

Regards,
Dean

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> In get_row_security_policies():
>
>     /*
>      * If the only built-in policy is the default-deny one, and hook policies
>      * exist, then use the hook policies only and do not apply the
>      * default-deny policy.  Otherwise, we will apply both sets below.
>      */
>     if (defaultDeny &&
>         (hook_policies_restrictive != NIL || hook_policies_permissive != NIL))
>     {
>         rowsec_expr = NULL;
>         rowsec_with_check_expr = NULL;
>     }
>
> So if the only policies that exist are restrictive policies from an
> extension, the default-deny policy is not applied and the restrictive
> policies are applied instead, which is effectively a "default-allow"
> situation with each restrictive policy further limiting access to the
> table.

Right, the idea there being that applying the default-deny would render
the restrictive policies pointless, since nothing would ever be visible,
however..

> I think this is potentially very dangerous when both kinds of policy
> exist. Consider for example a situation where initially multiple
> permissive policies are set up for different users allowing them
> access to different subsets of the table, and users not covered by any
> of those permissive policies have no access. Then suppose that later a
> restrictive policy is added, applying to all users -- the intention
> being to prevent any user from accessing some particularly sensitive
> data. The result of adding that restrictive policy would be that all
> the users who previously had no access at all would suddenly have
> access to everything not prohibited by the restrictive policy.
>
> That goes against everything I would expect from a restrictive policy
> -- adding more restrictive policies should only ever reduce the number
> of rows visible, not ever open up more. So I think the test above
> should only disable the default-deny policy if there are any
> permissive policies from the extension.
>
> Of course that will make life a little harder for people who only want
> to use restrictive policies, since they will be forced to first add a
> permissive policy, possibly just one that's always true, but I think
> it's the safest approach.

Requiring a permissive policy which allows the records first, to avoid
the default-deny, makes a ton of sense.

> Possibly if/when we add proper SQL support for defining restrictive
> policies, we might also add an option to ALTER TABLE ... ENABLE ROW
> LEVEL SECURITY to allow a table to have RLS enabled in default-allow
> mode, for users who know they only want to add restrictive policies.

Perhaps...  I'm not sure that's really better than simply requiring a
'create policy p1 on t1 using (true);' to be done.

> However, I think that should be something the user has to explicitly
> ask for, not an automatic decision based on the existence of
> restrictive policies.

Agreed.  I'm happy to commit that change and back-patch it to 9.5,
barring objections.  Given that the only way to have restrictive
policies currently is using hooks, I don't believe there's any
documentation update required either; of course, I'll update the comment
to reflect this discussion/decision and make any necessary changes to
test_rls_hooks.
Thanks!
    Stephen

Dean, all,

* Stephen Frost (sfrost@snowman.net) wrote:
> Agreed.  I'm happy to commit that change and back-patch it to 9.5,
> barring objections.  Given that the only way to have restrictive
> policies currently is using hooks, I don't believe there's any
> documentation update required either; of course, I'll update the comment
> to reflect this discussion/decision and make any necessary changes to
> test_rls_hooks.

Patch attached to make this change, with appropriate comment updates and
fixes to the test_rls_hooks modules.

Comments?

    Thanks!

        Stephen

On 3 August 2015 at 16:09, Stephen Frost <sfrost@snowman.net> wrote:
> Dean, all,
>
> * Stephen Frost (sfrost@snowman.net) wrote:
>> Agreed.  I'm happy to commit that change and back-patch it to 9.5,
>> barring objections.  Given that the only way to have restrictive
>> policies currently is using hooks, I don't believe there's any
>> documentation update required either; of course, I'll update the comment
>> to reflect this discussion/decision and make any necessary changes to
>> test_rls_hooks.
>
> Patch attached to make this change, with appropriate comment updates and
> fixes to the test_rls_hooks modules.
>
> Comments?
>

Looks good to me.

Regards,
Dean

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 3 August 2015 at 16:09, Stephen Frost <sfrost@snowman.net> wrote:
> > * Stephen Frost (sfrost@snowman.net) wrote:
> >> Agreed.  I'm happy to commit that change and back-patch it to 9.5,
> >> barring objections.  Given that the only way to have restrictive
> >> policies currently is using hooks, I don't believe there's any
> >> documentation update required either; of course, I'll update the comment
> >> to reflect this discussion/decision and make any necessary changes to
> >> test_rls_hooks.
> >
> > Patch attached to make this change, with appropriate comment updates and
> > fixes to the test_rls_hooks modules.
> >
> > Comments?
> >
>
> Looks good to me.

Thanks!  Pushed to master and 9.5.
Stephen