Re: A little RLS oversight?

* Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
> On 28 July 2015 at 03:19, Joe Conway <mail(at)joeconway(dot)com> wrote:
> > On 07/27/2015 03:05 PM, Stephen Frost wrote:
> >> AFK at the moment, but my thinking was that we should avoid having
> >> the error message change based on what a GUC is set to. I agree
> >> that there should be comments which explain that.
>
> Except it's already dependant on the GUC if it's set to FORCE.

Yeah, you can get it to change if you own the table and set the GUC to
FORCE.

> > I changed back to using GetUserId() for the call to check_enable_rls()
> > at those three call sites, and added to the comments to explain why.
>
> I'm not entirely convinced about this. The more I think about it, the
> more I think that if we know the user has BYPASSRLS, and they've set
> row_security to OFF, then they ought to get the more detailed error
> message, as they would if there was no RLS. That error detail is
> highly useful, and we know the user has been granted privilege by a
> superuser, and that they have direct access to the underlying table in
> this context, so we're not leaking any info that they cannot directly
> SELECT anyway.

I agree that the error detail is useful. Perhaps it's alright that
we'll end up with something different if you've set row security to OFF
and you have BYPASSRLS.

> > While looking at ri_ReportViolation() I spotted what I believe to be a
> > bug in the current logic -- namely, has_perm is initialized to true,
> > and when check_enable_rls() returns RLS_ENABLED we never reset
> > has_perm to false, and thus leak info even though the comments claim
> > we don't. I fixed that here, but someone please take a look and
> > confirm I am reading that correctly.
>
> Ah yes, well spotted. That looks correct to me.

Ugh, yes, agreed. The back-branches are fine, but master and 9.5 need
that fix.

Thanks!

Stephen