Обсуждение: A little RLS oversight?

Hi.

I've tried RLS for a little (in PostgreSQL 9.5alpha1), and want to ask why
this wasn't taken in account
in its implementation:

"Rather than look at pg_statistic directly, it's better to look at its view
pg_stats when examining the statistics manually. pg_stats is designed to be
more easily readable. Furthermore, pg_stats is readable by all, whereas
pg_statistic is only readable by a superuser. (This prevents unprivileged
users from learning something about the contents of other people's tables
from
the statistics. The pg_stats view is restricted to show only rows about
tables
that the current user can read.)"

i.e. after:
ALTER TABLE test ENABLE ROW LEVEL SECURITY;

I can still see all statistics for 'test' in pg_stats under unprivileged
user.
I'd prefer statistics on RLS-enabled tables to be simply hidden completely
for unprivileged users.







-----
WBR, Yaroslav Schekin.
--
View this message in context: http://postgresql.nabble.com/A-little-RLS-oversight-tp5857659.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

On Sun, Jul 12, 2015 at 5:59 PM, Yaroslav wrote:
> I can still see all statistics for 'test' in pg_stats under unprivileged
> user.

Indeed, this looks like an oversight of RLS. Even if a policy is
defined to prevent a user from seeing the rows of other users, it is
still possible to get some information though this view.
I am adding an open item regarding that for 9.5.

> I'd prefer statistics on RLS-enabled tables to be simply hidden completely
> for unprivileged users.

This looks like something simple enough to do.
@Stephen: perhaps you have some thoughts on the matter? Currently
pg_stats breaks its promise to only show information about the rows
current user can read.
-- 
Michael

Michael,

* Michael Paquier (michael.paquier@gmail.com) wrote:
> On Sun, Jul 12, 2015 at 5:59 PM, Yaroslav wrote:
> > I can still see all statistics for 'test' in pg_stats under unprivileged
> > user.
>
> Indeed, this looks like an oversight of RLS. Even if a policy is
> defined to prevent a user from seeing the rows of other users, it is
> still possible to get some information though this view.
> I am adding an open item regarding that for 9.5.

We need to be careful to avoid the slippery slope of trying to prevent
all covert channels, which has been extensively discussed previously.  I
tend to agree with this specific case of, if you have RLS configured on
the table then we probably shouldn't allow normal users to see the stats
on the table, but I don't have a problem with the usage of those stats
for generating plans, which users could see the results of via EXPLAIN.

> > I'd prefer statistics on RLS-enabled tables to be simply hidden completely
> > for unprivileged users.
>
> This looks like something simple enough to do.
> @Stephen: perhaps you have some thoughts on the matter? Currently
> pg_stats breaks its promise to only show information about the rows
> current user can read.

I agree that it should be reasonably simple to do and, provided that's
the case, I'm fine with doing it once I get back (currently out until
the 27th).
Thanks!
    Stephen

On Tue, Jul 14, 2015 at 4:01 AM, Stephen Frost <sfrost@snowman.net> wrote:
> Michael,
>
> * Michael Paquier (michael.paquier@gmail.com) wrote:
>> On Sun, Jul 12, 2015 at 5:59 PM, Yaroslav wrote:
>> > I can still see all statistics for 'test' in pg_stats under unprivileged
>> > user.
>>
>> Indeed, this looks like an oversight of RLS. Even if a policy is
>> defined to prevent a user from seeing the rows of other users, it is
>> still possible to get some information though this view.
>> I am adding an open item regarding that for 9.5.
>
> We need to be careful to avoid the slippery slope of trying to prevent
> all covert channels, which has been extensively discussed previously.  I
> tend to agree with this specific case of, if you have RLS configured on
> the table then we probably shouldn't allow normal users to see the stats
> on the table, but I don't have a problem with the usage of those stats
> for generating plans, which users could see the results of via EXPLAIN.

You mean for example the case where EXPLAIN adds in its output the
number of rows filtered out for all users? This gives an hint about
the number of rows of a relation even if a user that a limited access
to its rows with a policy.

>> > I'd prefer statistics on RLS-enabled tables to be simply hidden completely
>> > for unprivileged users.
>>
>> This looks like something simple enough to do.
>> @Stephen: perhaps you have some thoughts on the matter? Currently
>> pg_stats breaks its promise to only show information about the rows
>> current user can read.
>
> I agree that it should be reasonably simple to do and, provided that's
> the case, I'm fine with doing it once I get back (currently out until
> the 27th).

Looking at that I am not seeing any straight-forward way to resolve
this issue except by hardening pg_stats by having an additional filter
of this type so as a non-owner of a relation cannot see the stats of
this table directly when RLS is enabled:
c.relrowsecurity = false OR c.relowner = current_user::regrole::oid
Attached is a patch doing that (/me now hides, expecting to receive
laser shots because of the use of current_user on a system view).
Thoughts?
--
Michael

On 21 July 2015 at 04:53, Michael Paquier <michael.paquier@gmail.com> wrote:
> On Tue, Jul 14, 2015 at 4:01 AM, Stephen Frost <sfrost@snowman.net> wrote:
>> We need to be careful to avoid the slippery slope of trying to prevent
>> all covert channels, which has been extensively discussed previously.

I think this is more serious than the covert channel leaks discussed
before, since most_common_vals explicitly reveals values from the
table, making it an overt leak, albeit of a small portion of the
table's values.

> Looking at that I am not seeing any straight-forward way to resolve
> this issue except by hardening pg_stats by having an additional filter
> of this type so as a non-owner of a relation cannot see the stats of
> this table directly when RLS is enabled:
> c.relrowsecurity = false OR c.relowner = current_user::regrole::oid
> Attached is a patch doing that (/me now hides, expecting to receive
> laser shots because of the use of current_user on a system view).
> Thoughts?

Hmm, I think it probably ought to do more, based on whether or not RLS
is being bypassed or in force-mode -- see the first few checks in
get_row_security_policies(). Perhaps a new SQL-callable function
exposing those checks and calling check_enable_rls(). It's probably
still worth including the "c.relrowsecurity = false" check in SQL to
save calling the function for the majority of tables that don't have
RLS.

There's another issue here though -- just adding filters to the
pg_stats view won't prevent a determined user from seeing the contents
of the underlying table. For that, the view needs to have the
security_barrier property. Arguably the fact that pg_stats isn't a
security barrier view is a long-standing information leak allowing
users to see values from tables for which they don't have any
permissions. Is anyone concerned about that?

Regards,
Dean

On Wed, Jul 22, 2015 at 5:17 PM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> There's another issue here though -- just adding filters to the
> pg_stats view won't prevent a determined user from seeing the contents
> of the underlying table. For that, the view needs to have the
> security_barrier property. Arguably the fact that pg_stats isn't a
> security barrier view is a long-standing information leak allowing
> users to see values from tables for which they don't have any
> permissions. Is anyone concerned about that?

Hrm.  There's no help for that in the back-branches, but we should
probably change it in 9.5+.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:
> On Wed, Jul 22, 2015 at 5:17 PM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> > There's another issue here though -- just adding filters to the
> > pg_stats view won't prevent a determined user from seeing the contents
> > of the underlying table. For that, the view needs to have the
> > security_barrier property. Arguably the fact that pg_stats isn't a
> > security barrier view is a long-standing information leak allowing
> > users to see values from tables for which they don't have any
> > permissions. Is anyone concerned about that?
> 
> Hrm.  There's no help for that in the back-branches, but we should
> probably change it in 9.5+.

Perhaps not code-wise, but we could have a release note item suggesting
to run such-and-such command to plug the leak.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 07/22/2015 02:17 PM, Dean Rasheed wrote:
> On 21 July 2015 at 04:53, Michael Paquier <michael.paquier@gmail.com> wrote:
>> On Tue, Jul 14, 2015 at 4:01 AM, Stephen Frost <sfrost@snowman.net> wrote:
>>> We need to be careful to avoid the slippery slope of trying to prevent
>>> all covert channels, which has been extensively discussed previously.
>
> I think this is more serious than the covert channel leaks discussed
> before, since most_common_vals explicitly reveals values from the
> table, making it an overt leak, albeit of a small portion of the
> table's values.
>
>> Looking at that I am not seeing any straight-forward way to resolve
>> this issue except by hardening pg_stats by having an additional filter
>> of this type so as a non-owner of a relation cannot see the stats of
>> this table directly when RLS is enabled:
>> c.relrowsecurity = false OR c.relowner = current_user::regrole::oid
>> Attached is a patch doing that (/me now hides, expecting to receive
>> laser shots because of the use of current_user on a system view).
>> Thoughts?
>
> Hmm, I think it probably ought to do more, based on whether or not RLS
> is being bypassed or in force-mode -- see the first few checks in
> get_row_security_policies(). Perhaps a new SQL-callable function
> exposing those checks and calling check_enable_rls(). It's probably
> still worth including the "c.relrowsecurity = false" check in SQL to
> save calling the function for the majority of tables that don't have
> RLS.

Please see the attached patch and let me know what you think. I believe
the only thing lacking is documentation for the two new user visible
functions. Comments?

Joe

On 25 July 2015 at 19:12, Joe Conway <joe.conway@crunchydata.com> wrote:
> On 07/22/2015 02:17 PM, Dean Rasheed wrote:
>> Hmm, I think it probably ought to do more, based on whether or not RLS
>> is being bypassed or in force-mode -- see the first few checks in
>> get_row_security_policies(). Perhaps a new SQL-callable function
>> exposing those checks and calling check_enable_rls(). It's probably
>> still worth including the "c.relrowsecurity = false" check in SQL to
>> save calling the function for the majority of tables that don't have
>> RLS.
>
> Please see the attached patch and let me know what you think. I believe
> the only thing lacking is documentation for the two new user visible
> functions. Comments?
>

I'm not convinced about exporting convert_table_name() from acl.c,
particularly with such a non-descriptive name. It's only a couple of
lines of code, so I think they may as well just be included directly
in the new function, as seems to be common practice elsewhere.

As it stands, passing checkAsUser = GetUserId() to check_enable_rls()
will cause it to skip the check for row_security = OFF, and so it may
falsely report that RLS is active when the user has bypassed it. To
avoid that row_security_active() needs to pass checkAsUser =
InvalidOid to check_enable_rls().

[ Actually there seem to be a few other callers of check_enable_rls()
that suffer the same problem. I don't really understand the reasoning
behind that check in check_enable_rls() - if the current user has the
BYPASSRLS attribute and row_security = OFF, shouldn't RLS be disabled
on every table no matter how it is accessed? ]

I think it would be better if the security context check were part of
this new function too. Right now that can't make any difference, since
only the RI triggers set SECURITY_ROW_LEVEL_DISABLED and this function
cannot be called in that code path, but it's possible that in the
future other code might set SECURITY_ROW_LEVEL_DISABLED, so moving the
check down guarantees that check_enable_rls()/row_security_active()
always accurately return the RLS status for the table.

While I was looking at it, I spotted a couple of other things to tidy
up in existing related code:

* The comment for GetUserIdAndSecContext() needed updating for the new RLS bit.

* Calling GetUserIdAndSecContext() and then throwing away the user_id
returned seems ugly. There is already a code style precedent for
checking the other bits of SecurityRestrictionContext, so I've added a
similar bit-testing function for SECURITY_ROW_LEVEL_DISABLED which
makes this code a bit neater.

Attached is an updated patch (still needs some docs for the functions).

Regards,
Dean

On 07/26/2015 07:19 AM, Dean Rasheed wrote:
> I'm not convinced about exporting convert_table_name() from acl.c,
> particularly with such a non-descriptive name. It's only a couple of
> lines of code, so I think they may as well just be included directly
> in the new function, as seems to be common practice elsewhere.

fair enough

> As it stands, passing checkAsUser = GetUserId() to check_enable_rls()
> will cause it to skip the check for row_security = OFF, and so it may
> falsely report that RLS is active when the user has bypassed it. To
> avoid that row_security_active() needs to pass checkAsUser =
> InvalidOid to check_enable_rls().

> [ Actually there seem to be a few other callers of check_enable_rls()
> that suffer the same problem. I don't really understand the reasoning
> behind that check in check_enable_rls() - if the current user has the
> BYPASSRLS attribute and row_security = OFF, shouldn't RLS be disabled
> on every table no matter how it is accessed? ]


Hmmm, I can see that now but find it surprising. Seems like an odd
interface, and probably deserves some explanation in the function
comments. Maybe Stephen or someone else can weigh in on this...


> I think it would be better if the security context check were part of
> this new function too. Right now that can't make any difference, since
> only the RI triggers set SECURITY_ROW_LEVEL_DISABLED and this function
> cannot be called in that code path, but it's possible that in the
> future other code might set SECURITY_ROW_LEVEL_DISABLED, so moving the
> check down guarantees that check_enable_rls()/row_security_active()
> always accurately return the RLS status for the table.
> 
> While I was looking at it, I spotted a couple of other things to tidy
> up in existing related code:
> 
> * The comment for GetUserIdAndSecContext() needed updating for the new RLS bit.
> 
> * Calling GetUserIdAndSecContext() and then throwing away the user_id
> returned seems ugly. There is already a code style precedent for
> checking the other bits of SecurityRestrictionContext, so I've added a
> similar bit-testing function for SECURITY_ROW_LEVEL_DISABLED which
> makes this code a bit neater.
> 
> Attached is an updated patch (still needs some docs for the functions).

Thanks for that. I'll add the docs.

Joe

On 07/26/2015 07:59 AM, Joe Conway wrote:
> On 07/26/2015 07:19 AM, Dean Rasheed wrote:
>> Attached is an updated patch (still needs some docs for the functions).
>
> Thanks for that. I'll add the docs.

Documentation added. Also added comment to check_enable_rls about
passing InvalidOid versus GetUserId().

I believe this is ready to go -- any other comments?

Joe

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/27/2015 10:03 AM, Joe Conway wrote:
> On 07/26/2015 07:59 AM, Joe Conway wrote:
>> On 07/26/2015 07:19 AM, Dean Rasheed wrote:
>>> Attached is an updated patch (still needs some docs for the
>>> functions).
>>
>> Thanks for that. I'll add the docs.
>
> Documentation added. Also added comment to check_enable_rls about
> passing InvalidOid versus GetUserId().
>
> I believe this is ready to go -- any other comments?

Strike that - now I really think it is ready to go :-)

In this patch I additionally changed instances of:
  check_enable_rls(indrelid, GetUserId(), true)
to
  check_enable_rls(indrelid, InvalidOid, true)
per Dean's earlier remark and my new comment.

- --
Joe Conway
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJVtma+AAoJEDfy90M199hl01wP+wYTV6VfBbpVEVf2+ZmbQlbJ
pgquLXkXsZ9vdsw/jY09+7HKwVQFjqq+E3zjqj/Pn9Q0h17cgflPuYSvde30Mb+l
86zVD5oKLttFlCb9Ablbauc8FoYTud3D+fJkGwDPBYh5VeIlFRwQMRSKQRxKHFfr
PvXmv3z7TmYGBe7dLEl24WyGncOtsJxPiHZDYA5Cna7lG+jlHqVIDz5itu6xGHgy
OOLfr07aZX3Bt9zmzg1NdxcBZNc6NkSVtKFzkqrJ+rCIcoMFxyIWsVp2IAEOItFI
o7hNEqrRk8yMcyX+Ej7K/6arOqCjQ6+RT+tJarCNDPv7WRXwt4PInircCjswt+uX
/vMM7zhzhrW+BMc2rbkU4TKfcEfI78SxUh3jKRTMbUWM6UJPZ+ca1mo6EQGNhUaS
mOMnpPD+huKXZpKlAC1ImH1boFPYqf9de6ToQRIdm7GKLUhKK8llWg3wC2GwMrtq
JDojJhPUohhofMaU7YjokJWx0vAa3NckgCO4nmYvL5Sc36+QUDlW4Amm43el7PvB
SkD2B0AvLZFmMJlrh3eAnuDleXzjRmVc1WoJtGGT2qwmL9oSDtT6y4Uh+0VnDJkh
T7XJ1NgvwFGNzG/heVTv346Mah2wRl/4A43jpojzQLjbNZ7t2gi8h9DkanA7/iGK
JOmMBbIfVlKnT+SKEOVJ
=WZhM
-----END PGP SIGNATURE-----

On 27 July 2015 at 18:13, Joe Conway <mail@joeconway.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 07/27/2015 10:03 AM, Joe Conway wrote:
>> On 07/26/2015 07:59 AM, Joe Conway wrote:
>>> On 07/26/2015 07:19 AM, Dean Rasheed wrote:
>>>> Attached is an updated patch (still needs some docs for the
>>>> functions).
>>>
>>> Thanks for that. I'll add the docs.
>>
>> Documentation added. Also added comment to check_enable_rls about
>> passing InvalidOid versus GetUserId().
>>
>> I believe this is ready to go -- any other comments?
>
> Strike that - now I really think it is ready to go :-)
>
> In this patch I additionally changed instances of:
>   check_enable_rls(indrelid, GetUserId(), true)
> to
>   check_enable_rls(indrelid, InvalidOid, true)
> per Dean's earlier remark and my new comment.
>

Looks good to me, except I'm not sure about those latest changes
because I don't understand the reasoning behind the logic in
check_enable_rls() when row_security is set to OFF.

I would expect that if the current user has permission to bypass RLS,
and they have set row_security to OFF, then it should be off for all
tables that they have access to, regardless of how they access those
tables (directly or through a view). If it really is intentional that
RLS remains active when querying through a view not owned by the
table's owner, then the other calls to check_enable_rls() should
probably be left as they were, since the table might have been updated
through such a view and that code can't really tell at that point.

Regards,
Dean

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 27 July 2015 at 18:13, Joe Conway <mail@joeconway.com> wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> >
> > On 07/27/2015 10:03 AM, Joe Conway wrote:
> >> On 07/26/2015 07:59 AM, Joe Conway wrote:
> >>> On 07/26/2015 07:19 AM, Dean Rasheed wrote:
> >>>> Attached is an updated patch (still needs some docs for the
> >>>> functions).
> >>>
> >>> Thanks for that. I'll add the docs.
> >>
> >> Documentation added. Also added comment to check_enable_rls about
> >> passing InvalidOid versus GetUserId().
> >>
> >> I believe this is ready to go -- any other comments?
> >
> > Strike that - now I really think it is ready to go :-)
> >
> > In this patch I additionally changed instances of:
> >   check_enable_rls(indrelid, GetUserId(), true)
> > to
> >   check_enable_rls(indrelid, InvalidOid, true)
> > per Dean's earlier remark and my new comment.
>
> Looks good to me, except I'm not sure about those latest changes
> because I don't understand the reasoning behind the logic in
> check_enable_rls() when row_security is set to OFF.
>
> I would expect that if the current user has permission to bypass RLS,
> and they have set row_security to OFF, then it should be off for all
> tables that they have access to, regardless of how they access those
> tables (directly or through a view). If it really is intentional that
> RLS remains active when querying through a view not owned by the
> table's owner, then the other calls to check_enable_rls() should
> probably be left as they were, since the table might have been updated
> through such a view and that code can't really tell at that point.

Joe and I were discussing this earlier and it was certainly intentional
that RLS still be enabled if you're querying through a view as the RLS
rights of the view owner are used, not your own.  Note that we don't
allow a user to assume the BYPASSRLS right of the view owner though,
also intentionally.

As a comparison to what we do today, even if you have access to a table,
if you query it through a view, it's the view owner's permissions which
are used to determine access to the table through the view, not your
own.  I agree that can be a bit odd at times, as you can get a
permission denied error when using the view even though you have access
to the table which is complained about, but that's how views have worked
for quite a long time.
Thanks!
    Stephen

On 27 July 2015 at 21:58, Stephen Frost <sfrost@snowman.net> wrote:
> Dean,
>
> * Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
>> On 27 July 2015 at 18:13, Joe Conway <mail@joeconway.com> wrote:
>> > -----BEGIN PGP SIGNED MESSAGE-----
>> > Hash: SHA1
>> >
>> > On 07/27/2015 10:03 AM, Joe Conway wrote:
>> >> On 07/26/2015 07:59 AM, Joe Conway wrote:
>> >>> On 07/26/2015 07:19 AM, Dean Rasheed wrote:
>> >>>> Attached is an updated patch (still needs some docs for the
>> >>>> functions).
>> >>>
>> >>> Thanks for that. I'll add the docs.
>> >>
>> >> Documentation added. Also added comment to check_enable_rls about
>> >> passing InvalidOid versus GetUserId().
>> >>
>> >> I believe this is ready to go -- any other comments?
>> >
>> > Strike that - now I really think it is ready to go :-)
>> >
>> > In this patch I additionally changed instances of:
>> >   check_enable_rls(indrelid, GetUserId(), true)
>> > to
>> >   check_enable_rls(indrelid, InvalidOid, true)
>> > per Dean's earlier remark and my new comment.
>>
>> Looks good to me, except I'm not sure about those latest changes
>> because I don't understand the reasoning behind the logic in
>> check_enable_rls() when row_security is set to OFF.
>>
>> I would expect that if the current user has permission to bypass RLS,
>> and they have set row_security to OFF, then it should be off for all
>> tables that they have access to, regardless of how they access those
>> tables (directly or through a view). If it really is intentional that
>> RLS remains active when querying through a view not owned by the
>> table's owner, then the other calls to check_enable_rls() should
>> probably be left as they were, since the table might have been updated
>> through such a view and that code can't really tell at that point.
>
> Joe and I were discussing this earlier and it was certainly intentional
> that RLS still be enabled if you're querying through a view as the RLS
> rights of the view owner are used, not your own.  Note that we don't
> allow a user to assume the BYPASSRLS right of the view owner though,
> also intentionally.
>
> As a comparison to what we do today, even if you have access to a table,
> if you query it through a view, it's the view owner's permissions which
> are used to determine access to the table through the view, not your
> own.  I agree that can be a bit odd at times, as you can get a
> permission denied error when using the view even though you have access
> to the table which is complained about, but that's how views have worked
> for quite a long time.
>

OK, fair enough.

Regards,
Dean

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/27/2015 01:25 PM, Dean Rasheed wrote:
> Looks good to me, except I'm not sure about those latest changes 
> because I don't understand the reasoning behind the logic in 
> check_enable_rls() when row_security is set to OFF.
> 
> I would expect that if the current user has permission to bypass
> RLS, and they have set row_security to OFF, then it should be off
> for all tables that they have access to, regardless of how they
> access those tables (directly or through a view). If it really is
> intentional that RLS remains active when querying through a view
> not owned by the table's owner, then the other calls to
> check_enable_rls() should probably be left as they were, since the
> table might have been updated through such a view and that code
> can't really tell at that point.

After looking again at those three call sites, I'm now on the fence.
All three are in functions which are trying to avoid leaking info in
error messages. If we leave the original coding, then the messages
will remain the same for a given user regardless of the row_security
setting, whereas if we change them as in my latest patch, the messages
will be different depending on row_security for users with BYPASSRLS.

I think if we do the former (original coding) there should probably be
a comment added to indicate we are doing that intentionally.

Thoughts?

- -- 
Joe Conway
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJVtqRuAAoJEDfy90M199hlIQEQAIli3fJHPbpBBPocIjzo04EH
78YTiRYlb58ZK9l+VKj+sA9JbOdUVEes168hJjHSdnw6HcjJnKY+J3+aKjcRgXu2
s197hMOiP2+nqj0r0+KX/W8cuHT/4x5NtQ46BR9UoAPGdW9139CSbf3nQ9gTIllR
PQs7TRJIsJRWhuZhX5eCvQqix+RUYY2PKPMNdo8OLQpZxlsA7ezWr5QuDBx0PYFd
WTkaOsRxpAtfPBQrt+0xnX8oKi1pF4QLGt0Nb0J5XQmxUbKUdQsYY7+iu7Utrmf2
i5TORkX5HIpyH1N6R5Zu9wyRiOpLQv8SyH0kWovDA2neMlrApkM8kYfTYZAPIQUE
H6fOs6bafMMP4vWH7CwDhOwasccoLwdkEg80wiGnn5Nu+K4nq4k6Dq05oq+G7ZL1
6vZCXR0zts1TuX4abwtAcURaNbw+y7D/1XN9Dn0kDwuV3cXRh2tc33r/0SJ9XQFj
+gEdqptm3gyIgBExGlZDwNtpHwHEs2xqFjIBChyDV3cMjvFeTlYgiFiJlm40Ac/3
zelJ6hpsttAHWBg+42MoogrV7wKdCLOH/npwRx0zw5hH3meMs3ydQibtUwb/z+yl
6zl7xDMrTDOg/gV+gXbruVzSQIgNmfDEXmcHDKRr2IQwcNXXkTzEiIxJBRB7FhJg
GgXBUGlGDKRGXF8Koauy
=jCXT
-----END PGP SIGNATURE-----

On 27 July 2015 at 22:36, Joe Conway <mail@joeconway.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 07/27/2015 01:25 PM, Dean Rasheed wrote:
>> Looks good to me, except I'm not sure about those latest changes
>> because I don't understand the reasoning behind the logic in
>> check_enable_rls() when row_security is set to OFF.
>>
>> I would expect that if the current user has permission to bypass
>> RLS, and they have set row_security to OFF, then it should be off
>> for all tables that they have access to, regardless of how they
>> access those tables (directly or through a view). If it really is
>> intentional that RLS remains active when querying through a view
>> not owned by the table's owner, then the other calls to
>> check_enable_rls() should probably be left as they were, since the
>> table might have been updated through such a view and that code
>> can't really tell at that point.
>
> After looking again at those three call sites, I'm now on the fence.
> All three are in functions which are trying to avoid leaking info in
> error messages. If we leave the original coding, then the messages
> will remain the same for a given user regardless of the row_security
> setting, whereas if we change them as in my latest patch, the messages
> will be different depending on row_security for users with BYPASSRLS.
>
> I think if we do the former (original coding) there should probably be
> a comment added to indicate we are doing that intentionally.
>
> Thoughts?
>

I could go either way on that, but I don't think it's too serious to
worry about leaking information to users with BYPASSRLS.

Regards,
Dean

On 27 July 2015 at 22:36, Joe Conway <mail@joeconway.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 07/27/2015 01:25 PM, Dean Rasheed wrote:
>> Looks good to me, except I'm not sure about those latest changes
>> because I don't understand the reasoning behind the logic in
>> check_enable_rls() when row_security is set to OFF.
>>
>> I would expect that if the current user has permission to bypass
>> RLS, and they have set row_security to OFF, then it should be off
>> for all tables that they have access to, regardless of how they
>> access those tables (directly or through a view). If it really is
>> intentional that RLS remains active when querying through a view
>> not owned by the table's owner, then the other calls to
>> check_enable_rls() should probably be left as they were, since the
>> table might have been updated through such a view and that code
>> can't really tell at that point.
>
> After looking again at those three call sites, I'm now on the fence.
> All three are in functions which are trying to avoid leaking info in
> error messages. If we leave the original coding, then the messages
> will remain the same for a given user regardless of the row_security
> setting, whereas if we change them as in my latest patch, the messages
> will be different depending on row_security for users with BYPASSRLS.
>
> I think if we do the former (original coding) there should probably be
> a comment added to indicate we are doing that intentionally.
>
> Thoughts?
>

I could go either way on that, but I don't think it's too serious to
worry about leaking information to users with BYPASSRLS.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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.

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.

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.

Beyond that, any additional comments?

Thanks,

Joe

- --
Joe Conway
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJVtuadAAoJEDfy90M199hl67kQAJw9iekYVAm52+kOxmBi8YLK
NMoRUWLv5AZ7coaltQBBTiYYbjWHVKoWaMrOg2OjtxjyeshYaZ+xsBQl4umznI9j
b2unSfUmRPcCgy7O6R63+IXePh6krKowlMAIkSelYjvV05nSgQfy87/xjcBXS12r
MajLambTyJycS8fQXdt9sG8uGZh7ncXUtip3mUOYgl9Omn5GiDcgbdV1xQR+GBRJ
48S9lTHIJenpvi83Y9/7CXfDwxdcvkziUkR67UL4jxqmjWBDrrGZWEWOE1KOn78W
dIvItOnuw/tKoxmhcwkgys+u92uhfQUUwhDQsJRVKsqzvPcVt6Oh15rtlqipbYEn
YfcM35Sa4sUtxL9JIoyof+8audagPy9nzD26c4jA2A7EWXHt8Dim/z7D5RgrOpdn
xBqlwViuR5Zt+kLynf3aZyDtmaMIRA+tvzJPam1vrl7g86LCJbZslFNktveiGeYl
17+PKLTDcVO5f6CdT1NTnlaks0J7D4VThxGemDs09KX6P8iCe6VFaUqfEONpjb41
WsumlDJkT9bu5i8W68xtEskXBYgBmDCo6yho4bKn/f6tpHc10yyh8RpK48P5xPt9
ZLSTLmYkfLL7wsINw6eNrQ4OZCtWwiydD9CmjQZOzscyBBusOvlIcI9Kfrle0I1V
r2gQN651WyY4YJIoEggu
=hlUr
-----END PGP SIGNATURE-----

On 28 July 2015 at 03:19, Joe Conway <mail@joeconway.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> 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.

> 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.

> 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.

Regards,
Dean

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/28/2015 12:32 AM, Dean Rasheed 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 dependent on the GUC if it's set to FORCE.

Dean is correct. See test case below...

>> 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.

Agreed -- this patch version goes back to using InvalidOid at those
three call sites and the behavior is what I now believe to be correct.
Here is a test case to illustrate:

8<--------------------
BEGIN;
CREATE ROLE alice;
CREATE ROLE bob WITH BYPASSRLS;

SET SESSION AUTHORIZATION alice;
CREATE TABLE t1 (id int primary key, f1 text);
INSERT INTO t1 VALUES(1,'a');
CREATE TABLE t2 (id int primary key, f1 text, t1_id int REFERENCES
t1(id));
GRANT ALL ON t2 TO bob;
ALTER TABLE t2 ENABLE ROW LEVEL SECURITY;
CREATE POLICY P ON t2 TO alice, bob USING (true);

SET SESSION AUTHORIZATION bob;
INSERT INTO t2 VALUES(1,'a',1); -- should succeed

SAVEPOINT q;
INSERT INTO t2 VALUES(2,'b',2); -- should fail, no details
ROLLBACK TO q;

SET row_security = OFF;
SAVEPOINT q;
INSERT INTO t2 VALUES(2,'b',2); -- should fail, full details
ROLLBACK TO q;

SET SESSION AUTHORIZATION alice;
SAVEPOINT q;
INSERT INTO t2 VALUES(2,'b',2); -- should fail, full details
ROLLBACK TO q;

SET row_security = FORCE;
SAVEPOINT q;
INSERT INTO t2 VALUES(2,'b',2); -- should fail, no details
ROLLBACK TO q;

ROLLBACK;
8<--------------------

I'm going to commit the attached in the next few hours unless someone
has serious objections. We can always revisit the specific behavior of
those messages separately if we change our minds...

Joe

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJVt8dQAAoJEDfy90M199hl1ncQAI/XUZ3VSoW0Vegf09Y2DqlJ
f8lGnwSf+djSXgKVrUsKQsuLn7c+Ac9fqoRUQJMuCcOvnu7auljzaMjuMjrXhOIC
hhiP8QyYUoEMF+5Sggh/A532rFXRbI1R/g9eu8TTT9vJGkITVMGAucizSY+fPiBg
gH3JyuCVaIZbvlVv0OkqPXPiP1VR/7bDTBcIbv56XHOk1AavNlUMW5BWWR0/9Mt8
VMh9ri3eQ5beKxrDhAZ+39ddlQzk9yJsN5pd/Pu0zPNxwBcvTNra0ZZGv7PoPwUF
7F98A1bL/NWFDdFuOI2E61/a5lA70t5HV4UTPPugQr82NhBS9JdpxgqA8W7B9+P9
4TKqmmYIvOcxM+TtglIlyr+JBwfJERw8j3+IcnM3mjLnkyflNbX2kbOF0B5Ghpt/
EzrVIJi/Pl3ctm+9r/oQYiwo/6Qsy8hco9QLCY4GVhBEE93Wr8P6NVlcjzyocMRs
FBjgvxeL/1wL8g3Q8ZDsAVOu9Ld0OCGEkA27XRS3sXbZfHroeTNW5aUqvKIzFkKB
gsr09pIVtdd7ysEdxxHZpELaU8H2rcA5O8b380HauIi41GaDc5E0XLXJSu6dIWCP
x/Em3qTpt74YgZiqsbs3a21Ak5n8fBdTMyXhmPQbXctllALI3Kj7bbyqoeGywpxi
PKhGDzgw+M7OQzfWS7UF
=e5Qo
-----END PGP SIGNATURE-----

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 28 July 2015 at 03:19, Joe Conway <mail@joeconway.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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/28/2015 11:17 AM, Joe Conway wrote:
> I'm going to commit the attached in the next few hours unless
> someone has serious objections. We can always revisit the specific
> behavior of those messages separately if we change our minds...

Pushed to master and 9.5

Joe
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJVt+U0AAoJEDfy90M199hlPb8P/0cgoaY4YWIcGc2HsezMAvfn
Fx2NqTpN7i1HCkWZcVv85TqfY4lura9wkakHOf6WFPSfBPMKGwBjDOPGwDc/iR42
RchChOXFK8Up12iqEZJMQ8PJQoaN3ZWg2QRrIpxPlQkhxsOKDDR9ZehczYSRrK5P
Nx/PE+cCHvI48zRQ0b31uPTeYMtUoCu/7qPQeSk3es/K5x3GBGls8aV/drw3CBRK
nrq7wu84jCrENoRhJSYKnntpjtBF43/SSojnJd0uOL6dytdEztxfyugEDnGni46k
vdEJNewKPIqZR5V8Qa4UGF4YKzxH8IgyWpYOuHTeNTEnNY+3/D+jmJoIwNw/P7gn
t+MMj6m4oRvWFY/J6sRZJ7bsFwj9KexQuu2rGDesnpWbJE5B7IricyUdtDrgia7p
qCS5wEnoG9uTrisPNi+oMZ1+IdsZ0FOHjfq6pNUUGDlmdR/iN4A8iNrAxIto5mHy
mB1Fx37LsMxeknDNQvsepOVa573goGlLwEDb5slxDySEGdXx6vm2pQo+Yy+L5s1g
dsFM7aVhCzsPjHZLKM9hEb84cjor7YzjuKlkQKfLkr7e6v6N2mtk/PTGYR51+li5
/8eZUxNF9K9tVe8dtP+RGtQ7ZgwmzOXcR34v6JYXOJz+X0DBT9c50A3986KMMS2X
1FrY7SUZlzMxRExmPXGH
=0YFQ
-----END PGP SIGNATURE-----

On Mon, Jul 27, 2015 at 4:58 PM, Stephen Frost <sfrost@snowman.net> wrote:
>> I would expect that if the current user has permission to bypass RLS,
>> and they have set row_security to OFF, then it should be off for all
>> tables that they have access to, regardless of how they access those
>> tables (directly or through a view). If it really is intentional that
>> RLS remains active when querying through a view not owned by the
>> table's owner, then the other calls to check_enable_rls() should
>> probably be left as they were, since the table might have been updated
>> through such a view and that code can't really tell at that point.
>
> Joe and I were discussing this earlier and it was certainly intentional
> that RLS still be enabled if you're querying through a view as the RLS
> rights of the view owner are used, not your own.  Note that we don't
> allow a user to assume the BYPASSRLS right of the view owner though,
> also intentionally.

That seems inconsistent.  If querying through a view doesn't adopt the
BYPASSRLS right (or lack thereof) of the view owner, and I agree that
it shouldn't, then it also shouldn't disregard the fact that the
person issuing the query has that right.

In other words, we've made a decision, when going through views, to
test ACLs based on who owns the view.  We do that in all cases, not
only sometimes.  Now, when querying a view, whose BYPASSRLS privilege
do we consult?  It should either be the person issuing the query in
all cases, or the view owner in all cases, not some hybrid of the two.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Mon, Jul 27, 2015 at 4:58 PM, Stephen Frost <sfrost@snowman.net> wrote:
> >> I would expect that if the current user has permission to bypass RLS,
> >> and they have set row_security to OFF, then it should be off for all
> >> tables that they have access to, regardless of how they access those
> >> tables (directly or through a view). If it really is intentional that
> >> RLS remains active when querying through a view not owned by the
> >> table's owner, then the other calls to check_enable_rls() should
> >> probably be left as they were, since the table might have been updated
> >> through such a view and that code can't really tell at that point.
> >
> > Joe and I were discussing this earlier and it was certainly intentional
> > that RLS still be enabled if you're querying through a view as the RLS
> > rights of the view owner are used, not your own.  Note that we don't
> > allow a user to assume the BYPASSRLS right of the view owner though,
> > also intentionally.
>
> That seems inconsistent.  If querying through a view doesn't adopt the
> BYPASSRLS right (or lack thereof) of the view owner, and I agree that
> it shouldn't, then it also shouldn't disregard the fact that the
> person issuing the query has that right.

That's not how other role attributes currently work though, specifically
thinking of superuser.  Even when running a query as a superuser you can
get a permission denied if you're querying a view where the view owner
hasn't got access to the relation under the view.

=# create role r1;
CREATE ROLE
=*# create role r2;
CREATE ROLE
=*# create table t1 (c1 int);
CREATE TABLE
=*# alter table t1 owner to r1;
ALTER TABLE
=*# create view v1 as select * from t1;
CREATE VIEW
=*# alter view v1 owner to r2;
ALTER VIEW
=*# select * from v1;
ERROR:  permission denied for relation t1

> In other words, we've made a decision, when going through views, to
> test ACLs based on who owns the view.  We do that in all cases, not
> only sometimes.  Now, when querying a view, whose BYPASSRLS privilege
> do we consult?  It should either be the person issuing the query in
> all cases, or the view owner in all cases, not some hybrid of the two.

For superuser (the only similar precedent that we have, I believe), we
go based on the view owner, but that isn't quite the same as BYPASSRLS.

The reason this doesn't hold is that you have to use a combination of
BYPASSRLS and row_security=off to actually bypass RLS, unlike the
superuser role attribute which is just always "on" if you've got it.  If
having BYPASSRLS simply always meant "don't do any RLS" then we could
use the superuser precedent to use what the view owner has, but at least
for my part, I'm a lot happier with BYPASSRLS and row_security than with
superuser and would rather we continue in that direction, where the user
has the choice of if they want their role attribute to be in effect or
not.
Thanks,
    Stephen