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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
20150721_rls_stats.patchtext/x-patch; charset=US-ASCII; name=20150721_rls_stats.patchDownload
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index e82a53a..3ecf948 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -211,7 +211,10 @@ CREATE VIEW pg_stats AS
FROM pg_statistic s JOIN pg_class c ON (c.oid = s.starelid)
JOIN pg_attribute a ON (c.oid = attrelid AND attnum = s.staattnum)
LEFT JOIN pg_namespace n ON (n.oid = c.relnamespace)
- WHERE NOT attisdropped AND has_column_privilege(c.oid, a.attnum, 'select');
+ WHERE NOT attisdropped AND
+ has_column_privilege(c.oid, a.attnum, 'select') AND
+ (c.relrowsecurity = false OR
+ c.relowner = current_user::regrole::oid);
REVOKE ALL on pg_statistic FROM public;
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index cd53375..67aa14a 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -2061,7 +2061,7 @@ pg_stats| SELECT n.nspname AS schemaname,
JOIN pg_class c ON ((c.oid = s.starelid)))
JOIN pg_attribute a ON (((c.oid = a.attrelid) AND (a.attnum = s.staattnum))))
LEFT JOIN pg_namespace n ON ((n.oid = c.relnamespace)))
- WHERE ((NOT a.attisdropped) AND has_column_privilege(c.oid, a.attnum, 'select'::text));
+ WHERE ((NOT a.attisdropped) AND has_column_privilege(c.oid, a.attnum, 'select'::text) AND ((c.relrowsecurity = false) OR (c.relowner = (("current_user"())::regrole)::oid)));
pg_tables| SELECT n.nspname AS schemaname,
c.relname AS tablename,
pg_get_userbyid(c.relowner) AS tableowner,
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
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:
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
20150725.1-rls-pg_stats.patchtext/x-diff; name=20150725.1-rls-pg_stats.patchDownload
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index e82a53a..c0bd6fa 100644
*** a/src/backend/catalog/system_views.sql
--- b/src/backend/catalog/system_views.sql
*************** CREATE VIEW pg_indexes AS
*** 150,156 ****
LEFT JOIN pg_tablespace T ON (T.oid = I.reltablespace)
WHERE C.relkind IN ('r', 'm') AND I.relkind = 'i';
! CREATE VIEW pg_stats AS
SELECT
nspname AS schemaname,
relname AS tablename,
--- 150,156 ----
LEFT JOIN pg_tablespace T ON (T.oid = I.reltablespace)
WHERE C.relkind IN ('r', 'm') AND I.relkind = 'i';
! CREATE VIEW pg_stats WITH (security_barrier) AS
SELECT
nspname AS schemaname,
relname AS tablename,
*************** CREATE VIEW pg_stats AS
*** 211,217 ****
FROM pg_statistic s JOIN pg_class c ON (c.oid = s.starelid)
JOIN pg_attribute a ON (c.oid = attrelid AND attnum = s.staattnum)
LEFT JOIN pg_namespace n ON (n.oid = c.relnamespace)
! WHERE NOT attisdropped AND has_column_privilege(c.oid, a.attnum, 'select');
REVOKE ALL on pg_statistic FROM public;
--- 211,219 ----
FROM pg_statistic s JOIN pg_class c ON (c.oid = s.starelid)
JOIN pg_attribute a ON (c.oid = attrelid AND attnum = s.staattnum)
LEFT JOIN pg_namespace n ON (n.oid = c.relnamespace)
! WHERE NOT attisdropped
! AND has_column_privilege(c.oid, a.attnum, 'select')
! AND (c.relrowsecurity = false OR NOT row_security_active(c.oid));
REVOKE ALL on pg_statistic FROM public;
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index 3ca168b..094ac33 100644
*** a/src/backend/utils/adt/acl.c
--- b/src/backend/utils/adt/acl.c
*************** static AclMode convert_priv_string(text
*** 92,98 ****
static AclMode convert_any_priv_string(text *priv_type_text,
const priv_map *privileges);
- static Oid convert_table_name(text *tablename);
static AclMode convert_table_priv_string(text *priv_type_text);
static AclMode convert_sequence_priv_string(text *priv_type_text);
static AttrNumber convert_column_name(Oid tableoid, text *column);
--- 92,97 ----
*************** has_table_privilege_id_id(PG_FUNCTION_AR
*** 1998,2004 ****
/*
* Given a table name expressed as a string, look it up and return Oid
*/
! static Oid
convert_table_name(text *tablename)
{
RangeVar *relrv;
--- 1997,2003 ----
/*
* Given a table name expressed as a string, look it up and return Oid
*/
! Oid
convert_table_name(text *tablename)
{
RangeVar *relrv;
diff --git a/src/backend/utils/misc/rls.c b/src/backend/utils/misc/rls.c
index 44cb374..cd1a521 100644
*** a/src/backend/utils/misc/rls.c
--- b/src/backend/utils/misc/rls.c
***************
*** 19,24 ****
--- 19,25 ----
#include "catalog/pg_class.h"
#include "miscadmin.h"
#include "utils/acl.h"
+ #include "utils/builtins.h"
#include "utils/elog.h"
#include "utils/rls.h"
#include "utils/syscache.h"
*************** check_enable_rls(Oid relid, Oid checkAsU
*** 111,113 ****
--- 112,141 ----
/* RLS should be fully enabled for this relation. */
return RLS_ENABLED;
}
+
+ /*
+ * row_security_active variants
+ *
+ * check_enable_rls wrapped as a SQL callable function except
+ * RLS_NONE_ENV and RLS_NONE are the same for this purpose.
+ */
+ Datum
+ row_security_active_name(PG_FUNCTION_ARGS)
+ {
+ text *tablename = PG_GETARG_TEXT_P(0);
+ Oid tableoid = convert_table_name(tablename);
+
+ if (check_enable_rls(tableoid, GetUserId(), true) == RLS_ENABLED)
+ return true;
+ else
+ return false;
+ }
+
+ Datum
+ row_security_active_id(PG_FUNCTION_ARGS)
+ {
+ if (check_enable_rls(PG_GETARG_OID(0), GetUserId(), true) == RLS_ENABLED)
+ return true;
+ else
+ return false;
+ }
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 8f6685f..5fe1930 100644
*** a/src/include/catalog/catversion.h
--- b/src/include/catalog/catversion.h
***************
*** 53,58 ****
*/
/* yyyymmddN */
! #define CATALOG_VERSION_NO 201507171
#endif
--- 53,58 ----
*/
/* yyyymmddN */
! #define CATALOG_VERSION_NO 201507250
#endif
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 1d68ad7..91b3b24 100644
*** a/src/include/catalog/pg_proc.h
--- b/src/include/catalog/pg_proc.h
*************** DESCR("tsm_bernoulli_reset(internal)");
*** 5348,5353 ****
--- 5348,5359 ----
DATA(insert OID = 3346 ( tsm_bernoulli_cost PGNSP PGUID 12 1 0 0 0 f f f f t f v 7 0 2278 "2281 2281 2281 2281 2281 2281 2281" _null_ _null_ _null_ _null_ _null_ tsm_bernoulli_cost _null_ _null_ _null_ ));
DESCR("tsm_bernoulli_cost(internal)");
+ /* rls */
+ DATA(insert OID = 3298 ( row_security_active PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 16 "25" _null_ _null_ _null_ _null_ _null_ row_security_active_name _null_ _null_ _null_ ));
+ DESCR("row security for current context active on table by table name");
+ DATA(insert OID = 3299 ( row_security_active PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 16 "26" _null_ _null_ _null_ _null_ _null_ row_security_active_id _null_ _null_ _null_ ));
+ DESCR("row security for current context active on table by table oid");
+
/*
* Symbolic values for provolatile column: these indicate whether the result
* of a function is dependent *only* on the values of its explicit arguments,
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index fcb0bf0..5d80de3 100644
*** a/src/include/utils/builtins.h
--- b/src/include/utils/builtins.h
***************
*** 22,27 ****
--- 22,28 ----
*/
/* acl.c */
+ extern Oid convert_table_name(text *tablename);
extern Datum has_any_column_privilege_name_name(PG_FUNCTION_ARGS);
extern Datum has_any_column_privilege_name_id(PG_FUNCTION_ARGS);
extern Datum has_any_column_privilege_id_name(PG_FUNCTION_ARGS);
*************** extern Datum set_config_by_name(PG_FUNCT
*** 1119,1124 ****
--- 1120,1129 ----
extern Datum show_all_settings(PG_FUNCTION_ARGS);
extern Datum show_all_file_settings(PG_FUNCTION_ARGS);
+ /* rls.c */
+ extern Datum row_security_active_name(PG_FUNCTION_ARGS);
+ extern Datum row_security_active_id(PG_FUNCTION_ARGS);
+
/* lockfuncs.c */
extern Datum pg_lock_status(PG_FUNCTION_ARGS);
extern Datum pg_advisory_lock_int8(PG_FUNCTION_ARGS);
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
index 414299a..b17c637 100644
*** a/src/test/regress/expected/rowsecurity.out
--- b/src/test/regress/expected/rowsecurity.out
*************** SELECT * FROM current_check;
*** 2837,2846 ****
COMMIT;
--
-- Collation support
--
BEGIN;
! SET row_security = force;
CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
--- 2837,2880 ----
COMMIT;
--
+ -- check pg_stats view filtering
+ --
+ SET row_security TO ON;
+ SET SESSION AUTHORIZATION rls_regress_user0;
+ ANALYZE current_check;
+ -- Stats visible
+ SELECT row_security_active('current_check');
+ row_security_active
+ ---------------------
+ f
+ (1 row)
+
+ SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
+ most_common_vals
+ ---------------------
+
+
+ {rls_regress_user1}
+ (3 rows)
+
+ SET SESSION AUTHORIZATION rls_regress_user1;
+ -- Stats not visible
+ SELECT row_security_active('current_check');
+ row_security_active
+ ---------------------
+ t
+ (1 row)
+
+ SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
+ most_common_vals
+ ------------------
+ (0 rows)
+
+ --
-- Collation support
--
BEGIN;
! SET row_security TO FORCE;
CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index cd53375..2fc05a5 100644
*** a/src/test/regress/expected/rules.out
--- b/src/test/regress/expected/rules.out
*************** pg_stats| SELECT n.nspname AS schemaname
*** 2061,2067 ****
JOIN pg_class c ON ((c.oid = s.starelid)))
JOIN pg_attribute a ON (((c.oid = a.attrelid) AND (a.attnum = s.staattnum))))
LEFT JOIN pg_namespace n ON ((n.oid = c.relnamespace)))
! WHERE ((NOT a.attisdropped) AND has_column_privilege(c.oid, a.attnum, 'select'::text));
pg_tables| SELECT n.nspname AS schemaname,
c.relname AS tablename,
pg_get_userbyid(c.relowner) AS tableowner,
--- 2061,2067 ----
JOIN pg_class c ON ((c.oid = s.starelid)))
JOIN pg_attribute a ON (((c.oid = a.attrelid) AND (a.attnum = s.staattnum))))
LEFT JOIN pg_namespace n ON ((n.oid = c.relnamespace)))
! WHERE ((NOT a.attisdropped) AND has_column_privilege(c.oid, a.attnum, 'select'::text) AND ((c.relrowsecurity = false) OR (NOT row_security_active(c.oid))));
pg_tables| SELECT n.nspname AS schemaname,
c.relname AS tablename,
pg_get_userbyid(c.relowner) AS tableowner,
diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
index 039070b..7c377ca 100644
*** a/src/test/regress/sql/rowsecurity.sql
--- b/src/test/regress/sql/rowsecurity.sql
*************** SELECT * FROM current_check;
*** 1137,1146 ****
COMMIT;
--
-- Collation support
--
BEGIN;
! SET row_security = force;
CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
--- 1137,1161 ----
COMMIT;
--
+ -- check pg_stats view filtering
+ --
+ SET row_security TO ON;
+ SET SESSION AUTHORIZATION rls_regress_user0;
+ ANALYZE current_check;
+ -- Stats visible
+ SELECT row_security_active('current_check');
+ SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
+
+ SET SESSION AUTHORIZATION rls_regress_user1;
+ -- Stats not visible
+ SELECT row_security_active('current_check');
+ SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
+
+ --
-- Collation support
--
BEGIN;
! SET row_security TO FORCE;
CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
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
Attachments:
rls-pg-stats.v2.patchapplication/octet-stream; name=rls-pg-stats.v2.patchDownload
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
new file mode 100644
index e82a53a..c0bd6fa
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -150,7 +150,7 @@ CREATE VIEW pg_indexes AS
LEFT JOIN pg_tablespace T ON (T.oid = I.reltablespace)
WHERE C.relkind IN ('r', 'm') AND I.relkind = 'i';
-CREATE VIEW pg_stats AS
+CREATE VIEW pg_stats WITH (security_barrier) AS
SELECT
nspname AS schemaname,
relname AS tablename,
@@ -211,7 +211,9 @@ CREATE VIEW pg_stats AS
FROM pg_statistic s JOIN pg_class c ON (c.oid = s.starelid)
JOIN pg_attribute a ON (c.oid = attrelid AND attnum = s.staattnum)
LEFT JOIN pg_namespace n ON (n.oid = c.relnamespace)
- WHERE NOT attisdropped AND has_column_privilege(c.oid, a.attnum, 'select');
+ WHERE NOT attisdropped
+ AND has_column_privilege(c.oid, a.attnum, 'select')
+ AND (c.relrowsecurity = false OR NOT row_security_active(c.oid));
REVOKE ALL on pg_statistic FROM public;
diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c
new file mode 100644
index aaf0061..2386cf0
--- a/src/backend/rewrite/rowsecurity.c
+++ b/src/backend/rewrite/rowsecurity.c
@@ -107,7 +107,6 @@ get_row_security_policies(Query *root, C
Relation rel;
Oid user_id;
- int sec_context;
int rls_status;
bool defaultDeny = false;
@@ -117,22 +116,13 @@ get_row_security_policies(Query *root, C
*hasRowSecurity = false;
*hasSubLinks = false;
- /* This is just to get the security context */
- GetUserIdAndSecContext(&user_id, &sec_context);
+ /* If this is not a normal relation, just return immediately */
+ if (rte->relkind != RELKIND_RELATION)
+ return;
/* Switch to checkAsUser if it's set */
user_id = rte->checkAsUser ? rte->checkAsUser : GetUserId();
- /*
- * If this is not a normal relation, or we have been told to explicitly
- * skip RLS (perhaps because this is an FK check) then just return
- * immediately.
- */
- if (rte->relid < FirstNormalObjectId
- || rte->relkind != RELKIND_RELATION
- || (sec_context & SECURITY_ROW_LEVEL_DISABLED))
- return;
-
/* Determine the state of RLS for this, pass checkAsUser explicitly */
rls_status = check_enable_rls(rte->relid, rte->checkAsUser, false);
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
new file mode 100644
index e6808e7..525794f
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -153,8 +153,6 @@ CreateCachedPlan(Node *raw_parse_tree,
CachedPlanSource *plansource;
MemoryContext source_context;
MemoryContext oldcxt;
- Oid user_id;
- int security_context;
Assert(query_string != NULL); /* required as of 8.4 */
@@ -177,8 +175,6 @@ CreateCachedPlan(Node *raw_parse_tree,
*/
oldcxt = MemoryContextSwitchTo(source_context);
- GetUserIdAndSecContext(&user_id, &security_context);
-
plansource = (CachedPlanSource *) palloc0(sizeof(CachedPlanSource));
plansource->magic = CACHEDPLANSOURCE_MAGIC;
plansource->raw_parse_tree = copyObject(raw_parse_tree);
@@ -208,8 +204,7 @@ CreateCachedPlan(Node *raw_parse_tree,
plansource->total_custom_cost = 0;
plansource->num_custom_plans = 0;
plansource->hasRowSecurity = false;
- plansource->rowSecurityDisabled
- = (security_context & SECURITY_ROW_LEVEL_DISABLED) != 0;
+ plansource->rowSecurityDisabled = InRowLevelSecurityDisabled();
plansource->row_security_env = row_security;
plansource->planUserId = InvalidOid;
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
new file mode 100644
index acc4752..ac3e764
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -341,7 +341,7 @@ GetAuthenticatedUserId(void)
* GetUserIdAndSecContext/SetUserIdAndSecContext - get/set the current user ID
* and the SecurityRestrictionContext flags.
*
- * Currently there are two valid bits in SecurityRestrictionContext:
+ * Currently there are three valid bits in SecurityRestrictionContext:
*
* SECURITY_LOCAL_USERID_CHANGE indicates that we are inside an operation
* that is temporarily changing CurrentUserId via these functions. This is
@@ -359,6 +359,9 @@ GetAuthenticatedUserId(void)
* where the called functions are really supposed to be side-effect-free
* anyway, such as VACUUM/ANALYZE/REINDEX.
*
+ * SECURITY_ROW_LEVEL_DISABLED indicates that we are inside an operation that
+ * needs to bypass row level security checks, for example FK checks.
+ *
* Unlike GetUserId, GetUserIdAndSecContext does *not* Assert that the current
* value of CurrentUserId is valid; nor does SetUserIdAndSecContext require
* the new value to be valid. In fact, these routines had better not
@@ -401,6 +404,15 @@ InSecurityRestrictedOperation(void)
return (SecurityRestrictionContext & SECURITY_RESTRICTED_OPERATION) != 0;
}
+/*
+ * InRowLevelSecurityDisabled - are we inside a RLS-disabled operation?
+ */
+bool
+InRowLevelSecurityDisabled(void)
+{
+ return (SecurityRestrictionContext & SECURITY_ROW_LEVEL_DISABLED) != 0;
+}
+
/*
* These are obsolete versions of Get/SetUserIdAndSecContext that are
diff --git a/src/backend/utils/misc/rls.c b/src/backend/utils/misc/rls.c
new file mode 100644
index 44cb374..bcdad7a
--- a/src/backend/utils/misc/rls.c
+++ b/src/backend/utils/misc/rls.c
@@ -17,8 +17,10 @@
#include "access/htup.h"
#include "access/htup_details.h"
#include "catalog/pg_class.h"
+#include "catalog/namespace.h"
#include "miscadmin.h"
#include "utils/acl.h"
+#include "utils/builtins.h"
#include "utils/elog.h"
#include "utils/rls.h"
#include "utils/syscache.h"
@@ -53,6 +55,18 @@ check_enable_rls(Oid relid, Oid checkAsU
bool relrowsecurity;
Oid user_id = checkAsUser ? checkAsUser : GetUserId();
+ /* Nothing to do for built-in relations */
+ if (relid < FirstNormalObjectId)
+ return RLS_NONE;
+
+ /*
+ * Check if we have been told to explicitly skip RLS (perhaps because this
+ * is a foreign key check)
+ */
+ if (InRowLevelSecurityDisabled())
+ return RLS_NONE;
+
+ /* Check if RLS is enabled on the relation */
tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
if (!HeapTupleIsValid(tuple))
return RLS_NONE;
@@ -111,3 +125,37 @@ check_enable_rls(Oid relid, Oid checkAsU
/* RLS should be fully enabled for this relation. */
return RLS_ENABLED;
}
+
+/*
+ * row_security_active
+ *
+ * check_enable_rls wrapped as a SQL callable function except
+ * RLS_NONE_ENV and RLS_NONE are the same for this purpose.
+ */
+Datum
+row_security_active(PG_FUNCTION_ARGS)
+{
+ /* By OID */
+ Oid tableoid = PG_GETARG_OID(0);
+ int rls_status;
+
+ rls_status = check_enable_rls(tableoid, InvalidOid, true);
+ PG_RETURN_BOOL(rls_status == RLS_ENABLED);
+}
+
+Datum
+row_security_active_name(PG_FUNCTION_ARGS)
+{
+ /* By qualified name */
+ text *tablename = PG_GETARG_TEXT_P(0);
+ RangeVar *tablerel;
+ Oid tableoid;
+ int rls_status;
+
+ /* Look up table name. Can't lock it - we might not have privileges. */
+ tablerel = makeRangeVarFromNameList(textToQualifiedNameList(tablename));
+ tableoid = RangeVarGetRelid(tablerel, NoLock, false);
+
+ rls_status = check_enable_rls(tableoid, InvalidOid, true);
+ PG_RETURN_BOOL(rls_status == RLS_ENABLED);
+}
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
new file mode 100644
index 09bf143..2563bb9
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -5343,6 +5343,12 @@ DESCR("get progress for all replication
#define PROVOLATILE_STABLE 's' /* does not change within a scan */
#define PROVOLATILE_VOLATILE 'v' /* can change even within a scan */
+/* rls */
+DATA(insert OID = 3298 ( row_security_active PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 16 "26" _null_ _null_ _null_ _null_ _null_ row_security_active _null_ _null_ _null_ ));
+DESCR("row security for current context active on table by table oid");
+DATA(insert OID = 3299 ( row_security_active PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 16 "25" _null_ _null_ _null_ _null_ _null_ row_security_active_name _null_ _null_ _null_ ));
+DESCR("row security for current context active on table by table name");
+
/*
* Symbolic values for proargmodes column. Note that these must agree with
* the FunctionParameterMode enum in parsenodes.h; we declare them here to
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
new file mode 100644
index b539167..e0cc69f
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -305,6 +305,7 @@ extern void GetUserIdAndSecContext(Oid *
extern void SetUserIdAndSecContext(Oid userid, int sec_context);
extern bool InLocalUserIdChange(void);
extern bool InSecurityRestrictedOperation(void);
+extern bool InRowLevelSecurityDisabled(void);
extern void GetUserIdAndContext(Oid *userid, bool *sec_def_context);
extern void SetUserIdAndContext(Oid userid, bool sec_def_context);
extern void InitializeSessionUserId(const char *rolename, Oid useroid);
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
new file mode 100644
index 49caa56..fc1679e
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -1121,6 +1121,10 @@ extern Datum set_config_by_name(PG_FUNCT
extern Datum show_all_settings(PG_FUNCTION_ARGS);
extern Datum show_all_file_settings(PG_FUNCTION_ARGS);
+/* rls.c */
+extern Datum row_security_active(PG_FUNCTION_ARGS);
+extern Datum row_security_active_name(PG_FUNCTION_ARGS);
+
/* lockfuncs.c */
extern Datum pg_lock_status(PG_FUNCTION_ARGS);
extern Datum pg_advisory_lock_int8(PG_FUNCTION_ARGS);
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
new file mode 100644
index e7c242c..98e36f2
--- a/src/test/regress/expected/rowsecurity.out
+++ b/src/test/regress/expected/rowsecurity.out
@@ -2839,10 +2839,44 @@ SELECT * FROM current_check;
COMMIT;
--
+-- check pg_stats view filtering
+--
+SET row_security TO ON;
+SET SESSION AUTHORIZATION rls_regress_user0;
+ANALYZE current_check;
+-- Stats visible
+SELECT row_security_active('current_check');
+ row_security_active
+---------------------
+ f
+(1 row)
+
+SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
+ most_common_vals
+---------------------
+
+
+ {rls_regress_user1}
+(3 rows)
+
+SET SESSION AUTHORIZATION rls_regress_user1;
+-- Stats not visible
+SELECT row_security_active('current_check');
+ row_security_active
+---------------------
+ t
+(1 row)
+
+SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
+ most_common_vals
+------------------
+(0 rows)
+
+--
-- Collation support
--
BEGIN;
-SET row_security = force;
+SET row_security TO FORCE;
CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
new file mode 100644
index 1e5b0b9..6206c81
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -2061,7 +2061,7 @@ pg_stats| SELECT n.nspname AS schemaname
JOIN pg_class c ON ((c.oid = s.starelid)))
JOIN pg_attribute a ON (((c.oid = a.attrelid) AND (a.attnum = s.staattnum))))
LEFT JOIN pg_namespace n ON ((n.oid = c.relnamespace)))
- WHERE ((NOT a.attisdropped) AND has_column_privilege(c.oid, a.attnum, 'select'::text));
+ WHERE ((NOT a.attisdropped) AND has_column_privilege(c.oid, a.attnum, 'select'::text) AND ((c.relrowsecurity = false) OR (NOT row_security_active(c.oid))));
pg_tables| SELECT n.nspname AS schemaname,
c.relname AS tablename,
pg_get_userbyid(c.relowner) AS tableowner,
diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
new file mode 100644
index e86f814..73cc020
--- a/src/test/regress/sql/rowsecurity.sql
+++ b/src/test/regress/sql/rowsecurity.sql
@@ -1141,10 +1141,25 @@ SELECT * FROM current_check;
COMMIT;
--
+-- check pg_stats view filtering
+--
+SET row_security TO ON;
+SET SESSION AUTHORIZATION rls_regress_user0;
+ANALYZE current_check;
+-- Stats visible
+SELECT row_security_active('current_check');
+SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
+
+SET SESSION AUTHORIZATION rls_regress_user1;
+-- Stats not visible
+SELECT row_security_active('current_check');
+SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
+
+--
-- Collation support
--
BEGIN;
-SET row_security = force;
+SET row_security TO FORCE;
CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
20150727.00-rls-pg-stats.v3.patchtext/x-diff; name=20150727.00-rls-pg-stats.v3.patchDownload
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 98d1497..fd82ea4 100644
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
*************** SET search_path TO <replaceable>schema</
*** 15259,15264 ****
--- 15259,15270 ----
<entry><type>boolean</type></entry>
<entry>does current user have privilege for role</entry>
</row>
+ <row>
+ <entry><literal><function>row_security_active</function>(<parameter>table</parameter>)</literal>
+ </entry>
+ <entry><type>boolean</type></entry>
+ <entry>does current user have row level security active for table</entry>
+ </row>
</tbody>
</tgroup>
</table>
*************** SET search_path TO <replaceable>schema</
*** 15299,15304 ****
--- 15305,15313 ----
<indexterm>
<primary>pg_has_role</primary>
</indexterm>
+ <indexterm>
+ <primary>row_security_active</primary>
+ </indexterm>
<para>
<function>has_table_privilege</function> checks whether a user
*************** SELECT has_function_privilege('joeuser',
*** 15462,15467 ****
--- 15471,15483 ----
are immediately available without doing <command>SET ROLE</>.
</para>
+ <para>
+ <function>row_security_active</function> checks whether row level
+ security is active for the specified table in the context of the
+ <function>current_user</function> and environment. The table can
+ be specified by name or by OID.
+ </para>
+
<para>
<xref linkend="functions-info-schema-table"> shows functions that
determine whether a certain object is <firstterm>visible</> in the
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index e82a53a..c0bd6fa 100644
*** a/src/backend/catalog/system_views.sql
--- b/src/backend/catalog/system_views.sql
*************** CREATE VIEW pg_indexes AS
*** 150,156 ****
LEFT JOIN pg_tablespace T ON (T.oid = I.reltablespace)
WHERE C.relkind IN ('r', 'm') AND I.relkind = 'i';
! CREATE VIEW pg_stats AS
SELECT
nspname AS schemaname,
relname AS tablename,
--- 150,156 ----
LEFT JOIN pg_tablespace T ON (T.oid = I.reltablespace)
WHERE C.relkind IN ('r', 'm') AND I.relkind = 'i';
! CREATE VIEW pg_stats WITH (security_barrier) AS
SELECT
nspname AS schemaname,
relname AS tablename,
*************** CREATE VIEW pg_stats AS
*** 211,217 ****
FROM pg_statistic s JOIN pg_class c ON (c.oid = s.starelid)
JOIN pg_attribute a ON (c.oid = attrelid AND attnum = s.staattnum)
LEFT JOIN pg_namespace n ON (n.oid = c.relnamespace)
! WHERE NOT attisdropped AND has_column_privilege(c.oid, a.attnum, 'select');
REVOKE ALL on pg_statistic FROM public;
--- 211,219 ----
FROM pg_statistic s JOIN pg_class c ON (c.oid = s.starelid)
JOIN pg_attribute a ON (c.oid = attrelid AND attnum = s.staattnum)
LEFT JOIN pg_namespace n ON (n.oid = c.relnamespace)
! WHERE NOT attisdropped
! AND has_column_privilege(c.oid, a.attnum, 'select')
! AND (c.relrowsecurity = false OR NOT row_security_active(c.oid));
REVOKE ALL on pg_statistic FROM public;
diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c
index aaf0061..2386cf0 100644
*** a/src/backend/rewrite/rowsecurity.c
--- b/src/backend/rewrite/rowsecurity.c
*************** get_row_security_policies(Query *root, C
*** 107,113 ****
Relation rel;
Oid user_id;
- int sec_context;
int rls_status;
bool defaultDeny = false;
--- 107,112 ----
*************** get_row_security_policies(Query *root, C
*** 117,138 ****
*hasRowSecurity = false;
*hasSubLinks = false;
! /* This is just to get the security context */
! GetUserIdAndSecContext(&user_id, &sec_context);
/* Switch to checkAsUser if it's set */
user_id = rte->checkAsUser ? rte->checkAsUser : GetUserId();
- /*
- * If this is not a normal relation, or we have been told to explicitly
- * skip RLS (perhaps because this is an FK check) then just return
- * immediately.
- */
- if (rte->relid < FirstNormalObjectId
- || rte->relkind != RELKIND_RELATION
- || (sec_context & SECURITY_ROW_LEVEL_DISABLED))
- return;
-
/* Determine the state of RLS for this, pass checkAsUser explicitly */
rls_status = check_enable_rls(rte->relid, rte->checkAsUser, false);
--- 116,128 ----
*hasRowSecurity = false;
*hasSubLinks = false;
! /* If this is not a normal relation, just return immediately */
! if (rte->relkind != RELKIND_RELATION)
! return;
/* Switch to checkAsUser if it's set */
user_id = rte->checkAsUser ? rte->checkAsUser : GetUserId();
/* Determine the state of RLS for this, pass checkAsUser explicitly */
rls_status = check_enable_rls(rte->relid, rte->checkAsUser, false);
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index e6808e7..525794f 100644
*** a/src/backend/utils/cache/plancache.c
--- b/src/backend/utils/cache/plancache.c
*************** CreateCachedPlan(Node *raw_parse_tree,
*** 153,160 ****
CachedPlanSource *plansource;
MemoryContext source_context;
MemoryContext oldcxt;
- Oid user_id;
- int security_context;
Assert(query_string != NULL); /* required as of 8.4 */
--- 153,158 ----
*************** CreateCachedPlan(Node *raw_parse_tree,
*** 177,184 ****
*/
oldcxt = MemoryContextSwitchTo(source_context);
- GetUserIdAndSecContext(&user_id, &security_context);
-
plansource = (CachedPlanSource *) palloc0(sizeof(CachedPlanSource));
plansource->magic = CACHEDPLANSOURCE_MAGIC;
plansource->raw_parse_tree = copyObject(raw_parse_tree);
--- 175,180 ----
*************** CreateCachedPlan(Node *raw_parse_tree,
*** 208,215 ****
plansource->total_custom_cost = 0;
plansource->num_custom_plans = 0;
plansource->hasRowSecurity = false;
! plansource->rowSecurityDisabled
! = (security_context & SECURITY_ROW_LEVEL_DISABLED) != 0;
plansource->row_security_env = row_security;
plansource->planUserId = InvalidOid;
--- 204,210 ----
plansource->total_custom_cost = 0;
plansource->num_custom_plans = 0;
plansource->hasRowSecurity = false;
! plansource->rowSecurityDisabled = InRowLevelSecurityDisabled();
plansource->row_security_env = row_security;
plansource->planUserId = InvalidOid;
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index acc4752..ac3e764 100644
*** a/src/backend/utils/init/miscinit.c
--- b/src/backend/utils/init/miscinit.c
*************** GetAuthenticatedUserId(void)
*** 341,347 ****
* GetUserIdAndSecContext/SetUserIdAndSecContext - get/set the current user ID
* and the SecurityRestrictionContext flags.
*
! * Currently there are two valid bits in SecurityRestrictionContext:
*
* SECURITY_LOCAL_USERID_CHANGE indicates that we are inside an operation
* that is temporarily changing CurrentUserId via these functions. This is
--- 341,347 ----
* GetUserIdAndSecContext/SetUserIdAndSecContext - get/set the current user ID
* and the SecurityRestrictionContext flags.
*
! * Currently there are three valid bits in SecurityRestrictionContext:
*
* SECURITY_LOCAL_USERID_CHANGE indicates that we are inside an operation
* that is temporarily changing CurrentUserId via these functions. This is
*************** GetAuthenticatedUserId(void)
*** 359,364 ****
--- 359,367 ----
* where the called functions are really supposed to be side-effect-free
* anyway, such as VACUUM/ANALYZE/REINDEX.
*
+ * SECURITY_ROW_LEVEL_DISABLED indicates that we are inside an operation that
+ * needs to bypass row level security checks, for example FK checks.
+ *
* Unlike GetUserId, GetUserIdAndSecContext does *not* Assert that the current
* value of CurrentUserId is valid; nor does SetUserIdAndSecContext require
* the new value to be valid. In fact, these routines had better not
*************** InSecurityRestrictedOperation(void)
*** 401,406 ****
--- 404,418 ----
return (SecurityRestrictionContext & SECURITY_RESTRICTED_OPERATION) != 0;
}
+ /*
+ * InRowLevelSecurityDisabled - are we inside a RLS-disabled operation?
+ */
+ bool
+ InRowLevelSecurityDisabled(void)
+ {
+ return (SecurityRestrictionContext & SECURITY_ROW_LEVEL_DISABLED) != 0;
+ }
+
/*
* These are obsolete versions of Get/SetUserIdAndSecContext that are
diff --git a/src/backend/utils/misc/rls.c b/src/backend/utils/misc/rls.c
index 44cb374..60dbec2 100644
*** a/src/backend/utils/misc/rls.c
--- b/src/backend/utils/misc/rls.c
***************
*** 16,24 ****
--- 16,27 ----
#include "access/htup.h"
#include "access/htup_details.h"
+ #include "access/transam.h"
#include "catalog/pg_class.h"
+ #include "catalog/namespace.h"
#include "miscadmin.h"
#include "utils/acl.h"
+ #include "utils/builtins.h"
#include "utils/elog.h"
#include "utils/rls.h"
#include "utils/syscache.h"
*************** extern int check_enable_rls(Oid relid, O
*** 37,43 ****
* for the table and the plan cache needs to be invalidated if the environment
* changes.
*
! * Handle checking as another role via checkAsUser (for views, etc).
*
* If noError is set to 'true' then we just return RLS_ENABLED instead of doing
* an ereport() if the user has attempted to bypass RLS and they are not
--- 40,49 ----
* for the table and the plan cache needs to be invalidated if the environment
* changes.
*
! * Handle checking as another role via checkAsUser (for views, etc). Note that
! * if *not* checking as another role, the caller should pass InvalidOid rather
! * than GetUserId(). Otherwise the check for row_security = OFF is skipped, and
! * so we may falsely report that RLS is active when the user has bypassed it.
*
* If noError is set to 'true' then we just return RLS_ENABLED instead of doing
* an ereport() if the user has attempted to bypass RLS and they are not
*************** check_enable_rls(Oid relid, Oid checkAsU
*** 53,58 ****
--- 59,76 ----
bool relrowsecurity;
Oid user_id = checkAsUser ? checkAsUser : GetUserId();
+ /* Nothing to do for built-in relations */
+ if (relid < FirstNormalObjectId)
+ return RLS_NONE;
+
+ /*
+ * Check if we have been told to explicitly skip RLS (perhaps because this
+ * is a foreign key check)
+ */
+ if (InRowLevelSecurityDisabled())
+ return RLS_NONE;
+
+ /* Check if RLS is enabled on the relation */
tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
if (!HeapTupleIsValid(tuple))
return RLS_NONE;
*************** check_enable_rls(Oid relid, Oid checkAsU
*** 111,113 ****
--- 129,165 ----
/* RLS should be fully enabled for this relation. */
return RLS_ENABLED;
}
+
+ /*
+ * row_security_active
+ *
+ * check_enable_rls wrapped as a SQL callable function except
+ * RLS_NONE_ENV and RLS_NONE are the same for this purpose.
+ */
+ Datum
+ row_security_active(PG_FUNCTION_ARGS)
+ {
+ /* By OID */
+ Oid tableoid = PG_GETARG_OID(0);
+ int rls_status;
+
+ rls_status = check_enable_rls(tableoid, InvalidOid, true);
+ PG_RETURN_BOOL(rls_status == RLS_ENABLED);
+ }
+
+ Datum
+ row_security_active_name(PG_FUNCTION_ARGS)
+ {
+ /* By qualified name */
+ text *tablename = PG_GETARG_TEXT_P(0);
+ RangeVar *tablerel;
+ Oid tableoid;
+ int rls_status;
+
+ /* Look up table name. Can't lock it - we might not have privileges. */
+ tablerel = makeRangeVarFromNameList(textToQualifiedNameList(tablename));
+ tableoid = RangeVarGetRelid(tablerel, NoLock, false);
+
+ rls_status = check_enable_rls(tableoid, InvalidOid, true);
+ PG_RETURN_BOOL(rls_status == RLS_ENABLED);
+ }
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 09bf143..2563bb9 100644
*** a/src/include/catalog/pg_proc.h
--- b/src/include/catalog/pg_proc.h
*************** DESCR("get progress for all replication
*** 5343,5348 ****
--- 5343,5354 ----
#define PROVOLATILE_STABLE 's' /* does not change within a scan */
#define PROVOLATILE_VOLATILE 'v' /* can change even within a scan */
+ /* rls */
+ DATA(insert OID = 3298 ( row_security_active PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 16 "26" _null_ _null_ _null_ _null_ _null_ row_security_active _null_ _null_ _null_ ));
+ DESCR("row security for current context active on table by table oid");
+ DATA(insert OID = 3299 ( row_security_active PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 16 "25" _null_ _null_ _null_ _null_ _null_ row_security_active_name _null_ _null_ _null_ ));
+ DESCR("row security for current context active on table by table name");
+
/*
* Symbolic values for proargmodes column. Note that these must agree with
* the FunctionParameterMode enum in parsenodes.h; we declare them here to
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index b539167..e0cc69f 100644
*** a/src/include/miscadmin.h
--- b/src/include/miscadmin.h
*************** extern void GetUserIdAndSecContext(Oid *
*** 305,310 ****
--- 305,311 ----
extern void SetUserIdAndSecContext(Oid userid, int sec_context);
extern bool InLocalUserIdChange(void);
extern bool InSecurityRestrictedOperation(void);
+ extern bool InRowLevelSecurityDisabled(void);
extern void GetUserIdAndContext(Oid *userid, bool *sec_def_context);
extern void SetUserIdAndContext(Oid userid, bool sec_def_context);
extern void InitializeSessionUserId(const char *rolename, Oid useroid);
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index 49caa56..fc1679e 100644
*** a/src/include/utils/builtins.h
--- b/src/include/utils/builtins.h
*************** extern Datum set_config_by_name(PG_FUNCT
*** 1121,1126 ****
--- 1121,1130 ----
extern Datum show_all_settings(PG_FUNCTION_ARGS);
extern Datum show_all_file_settings(PG_FUNCTION_ARGS);
+ /* rls.c */
+ extern Datum row_security_active(PG_FUNCTION_ARGS);
+ extern Datum row_security_active_name(PG_FUNCTION_ARGS);
+
/* lockfuncs.c */
extern Datum pg_lock_status(PG_FUNCTION_ARGS);
extern Datum pg_advisory_lock_int8(PG_FUNCTION_ARGS);
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
index e7c242c..98e36f2 100644
*** a/src/test/regress/expected/rowsecurity.out
--- b/src/test/regress/expected/rowsecurity.out
*************** SELECT * FROM current_check;
*** 2839,2848 ****
COMMIT;
--
-- Collation support
--
BEGIN;
! SET row_security = force;
CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
--- 2839,2882 ----
COMMIT;
--
+ -- check pg_stats view filtering
+ --
+ SET row_security TO ON;
+ SET SESSION AUTHORIZATION rls_regress_user0;
+ ANALYZE current_check;
+ -- Stats visible
+ SELECT row_security_active('current_check');
+ row_security_active
+ ---------------------
+ f
+ (1 row)
+
+ SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
+ most_common_vals
+ ---------------------
+
+
+ {rls_regress_user1}
+ (3 rows)
+
+ SET SESSION AUTHORIZATION rls_regress_user1;
+ -- Stats not visible
+ SELECT row_security_active('current_check');
+ row_security_active
+ ---------------------
+ t
+ (1 row)
+
+ SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
+ most_common_vals
+ ------------------
+ (0 rows)
+
+ --
-- Collation support
--
BEGIN;
! SET row_security TO FORCE;
CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 1e5b0b9..6206c81 100644
*** a/src/test/regress/expected/rules.out
--- b/src/test/regress/expected/rules.out
*************** pg_stats| SELECT n.nspname AS schemaname
*** 2061,2067 ****
JOIN pg_class c ON ((c.oid = s.starelid)))
JOIN pg_attribute a ON (((c.oid = a.attrelid) AND (a.attnum = s.staattnum))))
LEFT JOIN pg_namespace n ON ((n.oid = c.relnamespace)))
! WHERE ((NOT a.attisdropped) AND has_column_privilege(c.oid, a.attnum, 'select'::text));
pg_tables| SELECT n.nspname AS schemaname,
c.relname AS tablename,
pg_get_userbyid(c.relowner) AS tableowner,
--- 2061,2067 ----
JOIN pg_class c ON ((c.oid = s.starelid)))
JOIN pg_attribute a ON (((c.oid = a.attrelid) AND (a.attnum = s.staattnum))))
LEFT JOIN pg_namespace n ON ((n.oid = c.relnamespace)))
! WHERE ((NOT a.attisdropped) AND has_column_privilege(c.oid, a.attnum, 'select'::text) AND ((c.relrowsecurity = false) OR (NOT row_security_active(c.oid))));
pg_tables| SELECT n.nspname AS schemaname,
c.relname AS tablename,
pg_get_userbyid(c.relowner) AS tableowner,
diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
index e86f814..73cc020 100644
*** a/src/test/regress/sql/rowsecurity.sql
--- b/src/test/regress/sql/rowsecurity.sql
*************** SELECT * FROM current_check;
*** 1141,1150 ****
COMMIT;
--
-- Collation support
--
BEGIN;
! SET row_security = force;
CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
--- 1141,1165 ----
COMMIT;
--
+ -- check pg_stats view filtering
+ --
+ SET row_security TO ON;
+ SET SESSION AUTHORIZATION rls_regress_user0;
+ ANALYZE current_check;
+ -- Stats visible
+ SELECT row_security_active('current_check');
+ SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
+
+ SET SESSION AUTHORIZATION rls_regress_user1;
+ -- Stats not visible
+ SELECT row_security_active('current_check');
+ SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
+
+ --
-- Collation support
--
BEGIN;
! SET row_security TO FORCE;
CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
-----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-----
Attachments:
20150727.00-rls-pg-stats.v4.patchtext/x-diff; name=20150727.00-rls-pg-stats.v4.patchDownload
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 98d1497..fd82ea4 100644
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
*************** SET search_path TO <replaceable>schema</
*** 15259,15264 ****
--- 15259,15270 ----
<entry><type>boolean</type></entry>
<entry>does current user have privilege for role</entry>
</row>
+ <row>
+ <entry><literal><function>row_security_active</function>(<parameter>table</parameter>)</literal>
+ </entry>
+ <entry><type>boolean</type></entry>
+ <entry>does current user have row level security active for table</entry>
+ </row>
</tbody>
</tgroup>
</table>
*************** SET search_path TO <replaceable>schema</
*** 15299,15304 ****
--- 15305,15313 ----
<indexterm>
<primary>pg_has_role</primary>
</indexterm>
+ <indexterm>
+ <primary>row_security_active</primary>
+ </indexterm>
<para>
<function>has_table_privilege</function> checks whether a user
*************** SELECT has_function_privilege('joeuser',
*** 15462,15467 ****
--- 15471,15483 ----
are immediately available without doing <command>SET ROLE</>.
</para>
+ <para>
+ <function>row_security_active</function> checks whether row level
+ security is active for the specified table in the context of the
+ <function>current_user</function> and environment. The table can
+ be specified by name or by OID.
+ </para>
+
<para>
<xref linkend="functions-info-schema-table"> shows functions that
determine whether a certain object is <firstterm>visible</> in the
diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
index 1043362..aa5b28c 100644
*** a/src/backend/access/index/genam.c
--- b/src/backend/access/index/genam.c
*************** BuildIndexValueDescription(Relation inde
*** 204,210 ****
Assert(indexrelid == idxrec->indexrelid);
/* RLS check- if RLS is enabled then we don't return anything. */
! if (check_enable_rls(indrelid, GetUserId(), true) == RLS_ENABLED)
{
ReleaseSysCache(ht_idx);
return NULL;
--- 204,210 ----
Assert(indexrelid == idxrec->indexrelid);
/* RLS check- if RLS is enabled then we don't return anything. */
! if (check_enable_rls(indrelid, InvalidOid, true) == RLS_ENABLED)
{
ReleaseSysCache(ht_idx);
return NULL;
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index e82a53a..c0bd6fa 100644
*** a/src/backend/catalog/system_views.sql
--- b/src/backend/catalog/system_views.sql
*************** CREATE VIEW pg_indexes AS
*** 150,156 ****
LEFT JOIN pg_tablespace T ON (T.oid = I.reltablespace)
WHERE C.relkind IN ('r', 'm') AND I.relkind = 'i';
! CREATE VIEW pg_stats AS
SELECT
nspname AS schemaname,
relname AS tablename,
--- 150,156 ----
LEFT JOIN pg_tablespace T ON (T.oid = I.reltablespace)
WHERE C.relkind IN ('r', 'm') AND I.relkind = 'i';
! CREATE VIEW pg_stats WITH (security_barrier) AS
SELECT
nspname AS schemaname,
relname AS tablename,
*************** CREATE VIEW pg_stats AS
*** 211,217 ****
FROM pg_statistic s JOIN pg_class c ON (c.oid = s.starelid)
JOIN pg_attribute a ON (c.oid = attrelid AND attnum = s.staattnum)
LEFT JOIN pg_namespace n ON (n.oid = c.relnamespace)
! WHERE NOT attisdropped AND has_column_privilege(c.oid, a.attnum, 'select');
REVOKE ALL on pg_statistic FROM public;
--- 211,219 ----
FROM pg_statistic s JOIN pg_class c ON (c.oid = s.starelid)
JOIN pg_attribute a ON (c.oid = attrelid AND attnum = s.staattnum)
LEFT JOIN pg_namespace n ON (n.oid = c.relnamespace)
! WHERE NOT attisdropped
! AND has_column_privilege(c.oid, a.attnum, 'select')
! AND (c.relrowsecurity = false OR NOT row_security_active(c.oid));
REVOKE ALL on pg_statistic FROM public;
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index a1561ce..2c65a90 100644
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
*************** ExecBuildSlotValueDescription(Oid reloid
*** 1874,1880 ****
* then don't return anything. Otherwise, go through normal permission
* checks.
*/
! if (check_enable_rls(reloid, GetUserId(), true) == RLS_ENABLED)
return NULL;
initStringInfo(&buf);
--- 1874,1880 ----
* then don't return anything. Otherwise, go through normal permission
* checks.
*/
! if (check_enable_rls(reloid, InvalidOid, true) == RLS_ENABLED)
return NULL;
initStringInfo(&buf);
diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c
index aaf0061..2386cf0 100644
*** a/src/backend/rewrite/rowsecurity.c
--- b/src/backend/rewrite/rowsecurity.c
*************** get_row_security_policies(Query *root, C
*** 107,113 ****
Relation rel;
Oid user_id;
- int sec_context;
int rls_status;
bool defaultDeny = false;
--- 107,112 ----
*************** get_row_security_policies(Query *root, C
*** 117,138 ****
*hasRowSecurity = false;
*hasSubLinks = false;
! /* This is just to get the security context */
! GetUserIdAndSecContext(&user_id, &sec_context);
/* Switch to checkAsUser if it's set */
user_id = rte->checkAsUser ? rte->checkAsUser : GetUserId();
- /*
- * If this is not a normal relation, or we have been told to explicitly
- * skip RLS (perhaps because this is an FK check) then just return
- * immediately.
- */
- if (rte->relid < FirstNormalObjectId
- || rte->relkind != RELKIND_RELATION
- || (sec_context & SECURITY_ROW_LEVEL_DISABLED))
- return;
-
/* Determine the state of RLS for this, pass checkAsUser explicitly */
rls_status = check_enable_rls(rte->relid, rte->checkAsUser, false);
--- 116,128 ----
*hasRowSecurity = false;
*hasSubLinks = false;
! /* If this is not a normal relation, just return immediately */
! if (rte->relkind != RELKIND_RELATION)
! return;
/* Switch to checkAsUser if it's set */
user_id = rte->checkAsUser ? rte->checkAsUser : GetUserId();
/* Determine the state of RLS for this, pass checkAsUser explicitly */
rls_status = check_enable_rls(rte->relid, rte->checkAsUser, false);
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 88dd3fa..386837f 100644
*** a/src/backend/utils/adt/ri_triggers.c
--- b/src/backend/utils/adt/ri_triggers.c
*************** ri_ReportViolation(const RI_ConstraintIn
*** 3243,3249 ****
* privileges.
*/
! if (check_enable_rls(rel_oid, GetUserId(), true) != RLS_ENABLED)
{
aclresult = pg_class_aclcheck(rel_oid, GetUserId(), ACL_SELECT);
if (aclresult != ACLCHECK_OK)
--- 3243,3249 ----
* privileges.
*/
! if (check_enable_rls(rel_oid, InvalidOid, true) != RLS_ENABLED)
{
aclresult = pg_class_aclcheck(rel_oid, GetUserId(), ACL_SELECT);
if (aclresult != ACLCHECK_OK)
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index e6808e7..525794f 100644
*** a/src/backend/utils/cache/plancache.c
--- b/src/backend/utils/cache/plancache.c
*************** CreateCachedPlan(Node *raw_parse_tree,
*** 153,160 ****
CachedPlanSource *plansource;
MemoryContext source_context;
MemoryContext oldcxt;
- Oid user_id;
- int security_context;
Assert(query_string != NULL); /* required as of 8.4 */
--- 153,158 ----
*************** CreateCachedPlan(Node *raw_parse_tree,
*** 177,184 ****
*/
oldcxt = MemoryContextSwitchTo(source_context);
- GetUserIdAndSecContext(&user_id, &security_context);
-
plansource = (CachedPlanSource *) palloc0(sizeof(CachedPlanSource));
plansource->magic = CACHEDPLANSOURCE_MAGIC;
plansource->raw_parse_tree = copyObject(raw_parse_tree);
--- 175,180 ----
*************** CreateCachedPlan(Node *raw_parse_tree,
*** 208,215 ****
plansource->total_custom_cost = 0;
plansource->num_custom_plans = 0;
plansource->hasRowSecurity = false;
! plansource->rowSecurityDisabled
! = (security_context & SECURITY_ROW_LEVEL_DISABLED) != 0;
plansource->row_security_env = row_security;
plansource->planUserId = InvalidOid;
--- 204,210 ----
plansource->total_custom_cost = 0;
plansource->num_custom_plans = 0;
plansource->hasRowSecurity = false;
! plansource->rowSecurityDisabled = InRowLevelSecurityDisabled();
plansource->row_security_env = row_security;
plansource->planUserId = InvalidOid;
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index acc4752..ac3e764 100644
*** a/src/backend/utils/init/miscinit.c
--- b/src/backend/utils/init/miscinit.c
*************** GetAuthenticatedUserId(void)
*** 341,347 ****
* GetUserIdAndSecContext/SetUserIdAndSecContext - get/set the current user ID
* and the SecurityRestrictionContext flags.
*
! * Currently there are two valid bits in SecurityRestrictionContext:
*
* SECURITY_LOCAL_USERID_CHANGE indicates that we are inside an operation
* that is temporarily changing CurrentUserId via these functions. This is
--- 341,347 ----
* GetUserIdAndSecContext/SetUserIdAndSecContext - get/set the current user ID
* and the SecurityRestrictionContext flags.
*
! * Currently there are three valid bits in SecurityRestrictionContext:
*
* SECURITY_LOCAL_USERID_CHANGE indicates that we are inside an operation
* that is temporarily changing CurrentUserId via these functions. This is
*************** GetAuthenticatedUserId(void)
*** 359,364 ****
--- 359,367 ----
* where the called functions are really supposed to be side-effect-free
* anyway, such as VACUUM/ANALYZE/REINDEX.
*
+ * SECURITY_ROW_LEVEL_DISABLED indicates that we are inside an operation that
+ * needs to bypass row level security checks, for example FK checks.
+ *
* Unlike GetUserId, GetUserIdAndSecContext does *not* Assert that the current
* value of CurrentUserId is valid; nor does SetUserIdAndSecContext require
* the new value to be valid. In fact, these routines had better not
*************** InSecurityRestrictedOperation(void)
*** 401,406 ****
--- 404,418 ----
return (SecurityRestrictionContext & SECURITY_RESTRICTED_OPERATION) != 0;
}
+ /*
+ * InRowLevelSecurityDisabled - are we inside a RLS-disabled operation?
+ */
+ bool
+ InRowLevelSecurityDisabled(void)
+ {
+ return (SecurityRestrictionContext & SECURITY_ROW_LEVEL_DISABLED) != 0;
+ }
+
/*
* These are obsolete versions of Get/SetUserIdAndSecContext that are
diff --git a/src/backend/utils/misc/rls.c b/src/backend/utils/misc/rls.c
index 44cb374..60dbec2 100644
*** a/src/backend/utils/misc/rls.c
--- b/src/backend/utils/misc/rls.c
***************
*** 16,24 ****
--- 16,27 ----
#include "access/htup.h"
#include "access/htup_details.h"
+ #include "access/transam.h"
#include "catalog/pg_class.h"
+ #include "catalog/namespace.h"
#include "miscadmin.h"
#include "utils/acl.h"
+ #include "utils/builtins.h"
#include "utils/elog.h"
#include "utils/rls.h"
#include "utils/syscache.h"
*************** extern int check_enable_rls(Oid relid, O
*** 37,43 ****
* for the table and the plan cache needs to be invalidated if the environment
* changes.
*
! * Handle checking as another role via checkAsUser (for views, etc).
*
* If noError is set to 'true' then we just return RLS_ENABLED instead of doing
* an ereport() if the user has attempted to bypass RLS and they are not
--- 40,49 ----
* for the table and the plan cache needs to be invalidated if the environment
* changes.
*
! * Handle checking as another role via checkAsUser (for views, etc). Note that
! * if *not* checking as another role, the caller should pass InvalidOid rather
! * than GetUserId(). Otherwise the check for row_security = OFF is skipped, and
! * so we may falsely report that RLS is active when the user has bypassed it.
*
* If noError is set to 'true' then we just return RLS_ENABLED instead of doing
* an ereport() if the user has attempted to bypass RLS and they are not
*************** check_enable_rls(Oid relid, Oid checkAsU
*** 53,58 ****
--- 59,76 ----
bool relrowsecurity;
Oid user_id = checkAsUser ? checkAsUser : GetUserId();
+ /* Nothing to do for built-in relations */
+ if (relid < FirstNormalObjectId)
+ return RLS_NONE;
+
+ /*
+ * Check if we have been told to explicitly skip RLS (perhaps because this
+ * is a foreign key check)
+ */
+ if (InRowLevelSecurityDisabled())
+ return RLS_NONE;
+
+ /* Check if RLS is enabled on the relation */
tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
if (!HeapTupleIsValid(tuple))
return RLS_NONE;
*************** check_enable_rls(Oid relid, Oid checkAsU
*** 111,113 ****
--- 129,165 ----
/* RLS should be fully enabled for this relation. */
return RLS_ENABLED;
}
+
+ /*
+ * row_security_active
+ *
+ * check_enable_rls wrapped as a SQL callable function except
+ * RLS_NONE_ENV and RLS_NONE are the same for this purpose.
+ */
+ Datum
+ row_security_active(PG_FUNCTION_ARGS)
+ {
+ /* By OID */
+ Oid tableoid = PG_GETARG_OID(0);
+ int rls_status;
+
+ rls_status = check_enable_rls(tableoid, InvalidOid, true);
+ PG_RETURN_BOOL(rls_status == RLS_ENABLED);
+ }
+
+ Datum
+ row_security_active_name(PG_FUNCTION_ARGS)
+ {
+ /* By qualified name */
+ text *tablename = PG_GETARG_TEXT_P(0);
+ RangeVar *tablerel;
+ Oid tableoid;
+ int rls_status;
+
+ /* Look up table name. Can't lock it - we might not have privileges. */
+ tablerel = makeRangeVarFromNameList(textToQualifiedNameList(tablename));
+ tableoid = RangeVarGetRelid(tablerel, NoLock, false);
+
+ rls_status = check_enable_rls(tableoid, InvalidOid, true);
+ PG_RETURN_BOOL(rls_status == RLS_ENABLED);
+ }
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 09bf143..2563bb9 100644
*** a/src/include/catalog/pg_proc.h
--- b/src/include/catalog/pg_proc.h
*************** DESCR("get progress for all replication
*** 5343,5348 ****
--- 5343,5354 ----
#define PROVOLATILE_STABLE 's' /* does not change within a scan */
#define PROVOLATILE_VOLATILE 'v' /* can change even within a scan */
+ /* rls */
+ DATA(insert OID = 3298 ( row_security_active PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 16 "26" _null_ _null_ _null_ _null_ _null_ row_security_active _null_ _null_ _null_ ));
+ DESCR("row security for current context active on table by table oid");
+ DATA(insert OID = 3299 ( row_security_active PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 16 "25" _null_ _null_ _null_ _null_ _null_ row_security_active_name _null_ _null_ _null_ ));
+ DESCR("row security for current context active on table by table name");
+
/*
* Symbolic values for proargmodes column. Note that these must agree with
* the FunctionParameterMode enum in parsenodes.h; we declare them here to
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index b539167..e0cc69f 100644
*** a/src/include/miscadmin.h
--- b/src/include/miscadmin.h
*************** extern void GetUserIdAndSecContext(Oid *
*** 305,310 ****
--- 305,311 ----
extern void SetUserIdAndSecContext(Oid userid, int sec_context);
extern bool InLocalUserIdChange(void);
extern bool InSecurityRestrictedOperation(void);
+ extern bool InRowLevelSecurityDisabled(void);
extern void GetUserIdAndContext(Oid *userid, bool *sec_def_context);
extern void SetUserIdAndContext(Oid userid, bool sec_def_context);
extern void InitializeSessionUserId(const char *rolename, Oid useroid);
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index 49caa56..fc1679e 100644
*** a/src/include/utils/builtins.h
--- b/src/include/utils/builtins.h
*************** extern Datum set_config_by_name(PG_FUNCT
*** 1121,1126 ****
--- 1121,1130 ----
extern Datum show_all_settings(PG_FUNCTION_ARGS);
extern Datum show_all_file_settings(PG_FUNCTION_ARGS);
+ /* rls.c */
+ extern Datum row_security_active(PG_FUNCTION_ARGS);
+ extern Datum row_security_active_name(PG_FUNCTION_ARGS);
+
/* lockfuncs.c */
extern Datum pg_lock_status(PG_FUNCTION_ARGS);
extern Datum pg_advisory_lock_int8(PG_FUNCTION_ARGS);
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
index e7c242c..98e36f2 100644
*** a/src/test/regress/expected/rowsecurity.out
--- b/src/test/regress/expected/rowsecurity.out
*************** SELECT * FROM current_check;
*** 2839,2848 ****
COMMIT;
--
-- Collation support
--
BEGIN;
! SET row_security = force;
CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
--- 2839,2882 ----
COMMIT;
--
+ -- check pg_stats view filtering
+ --
+ SET row_security TO ON;
+ SET SESSION AUTHORIZATION rls_regress_user0;
+ ANALYZE current_check;
+ -- Stats visible
+ SELECT row_security_active('current_check');
+ row_security_active
+ ---------------------
+ f
+ (1 row)
+
+ SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
+ most_common_vals
+ ---------------------
+
+
+ {rls_regress_user1}
+ (3 rows)
+
+ SET SESSION AUTHORIZATION rls_regress_user1;
+ -- Stats not visible
+ SELECT row_security_active('current_check');
+ row_security_active
+ ---------------------
+ t
+ (1 row)
+
+ SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
+ most_common_vals
+ ------------------
+ (0 rows)
+
+ --
-- Collation support
--
BEGIN;
! SET row_security TO FORCE;
CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 1e5b0b9..6206c81 100644
*** a/src/test/regress/expected/rules.out
--- b/src/test/regress/expected/rules.out
*************** pg_stats| SELECT n.nspname AS schemaname
*** 2061,2067 ****
JOIN pg_class c ON ((c.oid = s.starelid)))
JOIN pg_attribute a ON (((c.oid = a.attrelid) AND (a.attnum = s.staattnum))))
LEFT JOIN pg_namespace n ON ((n.oid = c.relnamespace)))
! WHERE ((NOT a.attisdropped) AND has_column_privilege(c.oid, a.attnum, 'select'::text));
pg_tables| SELECT n.nspname AS schemaname,
c.relname AS tablename,
pg_get_userbyid(c.relowner) AS tableowner,
--- 2061,2067 ----
JOIN pg_class c ON ((c.oid = s.starelid)))
JOIN pg_attribute a ON (((c.oid = a.attrelid) AND (a.attnum = s.staattnum))))
LEFT JOIN pg_namespace n ON ((n.oid = c.relnamespace)))
! WHERE ((NOT a.attisdropped) AND has_column_privilege(c.oid, a.attnum, 'select'::text) AND ((c.relrowsecurity = false) OR (NOT row_security_active(c.oid))));
pg_tables| SELECT n.nspname AS schemaname,
c.relname AS tablename,
pg_get_userbyid(c.relowner) AS tableowner,
diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
index e86f814..73cc020 100644
*** a/src/test/regress/sql/rowsecurity.sql
--- b/src/test/regress/sql/rowsecurity.sql
*************** SELECT * FROM current_check;
*** 1141,1150 ****
COMMIT;
--
-- Collation support
--
BEGIN;
! SET row_security = force;
CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
--- 1141,1165 ----
COMMIT;
--
+ -- check pg_stats view filtering
+ --
+ SET row_security TO ON;
+ SET SESSION AUTHORIZATION rls_regress_user0;
+ ANALYZE current_check;
+ -- Stats visible
+ SELECT row_security_active('current_check');
+ SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
+
+ SET SESSION AUTHORIZATION rls_regress_user1;
+ -- Stats not visible
+ SELECT row_security_active('current_check');
+ SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
+
+ --
-- Collation support
--
BEGIN;
! SET row_security TO FORCE;
CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
On 27 July 2015 at 18:13, Joe Conway <mail@joeconway.com> wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1On 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
--
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 27 July 2015 at 18:13, Joe Conway <mail@joeconway.com> wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1On 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: SHA1On 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
-----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-----
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 27 July 2015 at 22:36, Joe Conway <mail@joeconway.com> wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1On 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Monday, July 27, 2015, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On 27 July 2015 at 22:36, Joe Conway <mail@joeconway.com <javascript:;>>
wrote:-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1On 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.
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.
Thanks!
Stephen
-----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-----
Attachments:
20150727.00-rls-pg-stats.v6.patchtext/x-diff; name=20150727.00-rls-pg-stats.v6.patchDownload
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 98d1497..fd82ea4 100644
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
*************** SET search_path TO <replaceable>schema</
*** 15259,15264 ****
--- 15259,15270 ----
<entry><type>boolean</type></entry>
<entry>does current user have privilege for role</entry>
</row>
+ <row>
+ <entry><literal><function>row_security_active</function>(<parameter>table</parameter>)</literal>
+ </entry>
+ <entry><type>boolean</type></entry>
+ <entry>does current user have row level security active for table</entry>
+ </row>
</tbody>
</tgroup>
</table>
*************** SET search_path TO <replaceable>schema</
*** 15299,15304 ****
--- 15305,15313 ----
<indexterm>
<primary>pg_has_role</primary>
</indexterm>
+ <indexterm>
+ <primary>row_security_active</primary>
+ </indexterm>
<para>
<function>has_table_privilege</function> checks whether a user
*************** SELECT has_function_privilege('joeuser',
*** 15462,15467 ****
--- 15471,15483 ----
are immediately available without doing <command>SET ROLE</>.
</para>
+ <para>
+ <function>row_security_active</function> checks whether row level
+ security is active for the specified table in the context of the
+ <function>current_user</function> and environment. The table can
+ be specified by name or by OID.
+ </para>
+
<para>
<xref linkend="functions-info-schema-table"> shows functions that
determine whether a certain object is <firstterm>visible</> in the
diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
index 1043362..2797c56 100644
*** a/src/backend/access/index/genam.c
--- b/src/backend/access/index/genam.c
*************** BuildIndexValueDescription(Relation inde
*** 203,209 ****
indrelid = idxrec->indrelid;
Assert(indexrelid == idxrec->indexrelid);
! /* RLS check- if RLS is enabled then we don't return anything. */
if (check_enable_rls(indrelid, GetUserId(), true) == RLS_ENABLED)
{
ReleaseSysCache(ht_idx);
--- 203,213 ----
indrelid = idxrec->indrelid;
Assert(indexrelid == idxrec->indexrelid);
! /*
! * RLS check - if RLS is enabled then we don't return anything.
! * Explicitly pass GetUserId() to ensure the result is not
! * dependent on the value of row_security for users with BYPASSRLS.
! */
if (check_enable_rls(indrelid, GetUserId(), true) == RLS_ENABLED)
{
ReleaseSysCache(ht_idx);
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index e82a53a..c0bd6fa 100644
*** a/src/backend/catalog/system_views.sql
--- b/src/backend/catalog/system_views.sql
*************** CREATE VIEW pg_indexes AS
*** 150,156 ****
LEFT JOIN pg_tablespace T ON (T.oid = I.reltablespace)
WHERE C.relkind IN ('r', 'm') AND I.relkind = 'i';
! CREATE VIEW pg_stats AS
SELECT
nspname AS schemaname,
relname AS tablename,
--- 150,156 ----
LEFT JOIN pg_tablespace T ON (T.oid = I.reltablespace)
WHERE C.relkind IN ('r', 'm') AND I.relkind = 'i';
! CREATE VIEW pg_stats WITH (security_barrier) AS
SELECT
nspname AS schemaname,
relname AS tablename,
*************** CREATE VIEW pg_stats AS
*** 211,217 ****
FROM pg_statistic s JOIN pg_class c ON (c.oid = s.starelid)
JOIN pg_attribute a ON (c.oid = attrelid AND attnum = s.staattnum)
LEFT JOIN pg_namespace n ON (n.oid = c.relnamespace)
! WHERE NOT attisdropped AND has_column_privilege(c.oid, a.attnum, 'select');
REVOKE ALL on pg_statistic FROM public;
--- 211,219 ----
FROM pg_statistic s JOIN pg_class c ON (c.oid = s.starelid)
JOIN pg_attribute a ON (c.oid = attrelid AND attnum = s.staattnum)
LEFT JOIN pg_namespace n ON (n.oid = c.relnamespace)
! WHERE NOT attisdropped
! AND has_column_privilege(c.oid, a.attnum, 'select')
! AND (c.relrowsecurity = false OR NOT row_security_active(c.oid));
REVOKE ALL on pg_statistic FROM public;
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index a1561ce..51f5cf4 100644
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
*************** ExecBuildSlotValueDescription(Oid reloid
*** 1872,1878 ****
/*
* Check if RLS is enabled and should be active for the relation; if so,
* then don't return anything. Otherwise, go through normal permission
! * checks.
*/
if (check_enable_rls(reloid, GetUserId(), true) == RLS_ENABLED)
return NULL;
--- 1872,1879 ----
/*
* Check if RLS is enabled and should be active for the relation; if so,
* then don't return anything. Otherwise, go through normal permission
! * checks. Explicitly pass GetUserId() to ensure the result is not
! * dependent on the value of row_security for users with BYPASSRLS.
*/
if (check_enable_rls(reloid, GetUserId(), true) == RLS_ENABLED)
return NULL;
diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c
index aaf0061..2386cf0 100644
*** a/src/backend/rewrite/rowsecurity.c
--- b/src/backend/rewrite/rowsecurity.c
*************** get_row_security_policies(Query *root, C
*** 107,113 ****
Relation rel;
Oid user_id;
- int sec_context;
int rls_status;
bool defaultDeny = false;
--- 107,112 ----
*************** get_row_security_policies(Query *root, C
*** 117,138 ****
*hasRowSecurity = false;
*hasSubLinks = false;
! /* This is just to get the security context */
! GetUserIdAndSecContext(&user_id, &sec_context);
/* Switch to checkAsUser if it's set */
user_id = rte->checkAsUser ? rte->checkAsUser : GetUserId();
- /*
- * If this is not a normal relation, or we have been told to explicitly
- * skip RLS (perhaps because this is an FK check) then just return
- * immediately.
- */
- if (rte->relid < FirstNormalObjectId
- || rte->relkind != RELKIND_RELATION
- || (sec_context & SECURITY_ROW_LEVEL_DISABLED))
- return;
-
/* Determine the state of RLS for this, pass checkAsUser explicitly */
rls_status = check_enable_rls(rte->relid, rte->checkAsUser, false);
--- 116,128 ----
*hasRowSecurity = false;
*hasSubLinks = false;
! /* If this is not a normal relation, just return immediately */
! if (rte->relkind != RELKIND_RELATION)
! return;
/* Switch to checkAsUser if it's set */
user_id = rte->checkAsUser ? rte->checkAsUser : GetUserId();
/* Determine the state of RLS for this, pass checkAsUser explicitly */
rls_status = check_enable_rls(rte->relid, rte->checkAsUser, false);
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 88dd3fa..d3a1520 100644
*** a/src/backend/utils/adt/ri_triggers.c
--- b/src/backend/utils/adt/ri_triggers.c
*************** ri_ReportViolation(const RI_ConstraintIn
*** 3237,3243 ****
* any of the key columns then we don't include the errdetail() below.
*
* Check if RLS is enabled on the relation first. If so, we don't return
! * any specifics to avoid leaking data.
*
* Check table-level permissions next and, failing that, column-level
* privileges.
--- 3237,3245 ----
* any of the key columns then we don't include the errdetail() below.
*
* Check if RLS is enabled on the relation first. If so, we don't return
! * any specifics to avoid leaking data. Explicitly pass GetUserId() to
! * ensure the result is not dependent on the value of row_security for
! * users with BYPASSRLS.
*
* Check table-level permissions next and, failing that, column-level
* privileges.
*************** ri_ReportViolation(const RI_ConstraintIn
*** 3264,3269 ****
--- 3266,3273 ----
}
}
}
+ else
+ has_perm = false;
if (has_perm)
{
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index e6808e7..525794f 100644
*** a/src/backend/utils/cache/plancache.c
--- b/src/backend/utils/cache/plancache.c
*************** CreateCachedPlan(Node *raw_parse_tree,
*** 153,160 ****
CachedPlanSource *plansource;
MemoryContext source_context;
MemoryContext oldcxt;
- Oid user_id;
- int security_context;
Assert(query_string != NULL); /* required as of 8.4 */
--- 153,158 ----
*************** CreateCachedPlan(Node *raw_parse_tree,
*** 177,184 ****
*/
oldcxt = MemoryContextSwitchTo(source_context);
- GetUserIdAndSecContext(&user_id, &security_context);
-
plansource = (CachedPlanSource *) palloc0(sizeof(CachedPlanSource));
plansource->magic = CACHEDPLANSOURCE_MAGIC;
plansource->raw_parse_tree = copyObject(raw_parse_tree);
--- 175,180 ----
*************** CreateCachedPlan(Node *raw_parse_tree,
*** 208,215 ****
plansource->total_custom_cost = 0;
plansource->num_custom_plans = 0;
plansource->hasRowSecurity = false;
! plansource->rowSecurityDisabled
! = (security_context & SECURITY_ROW_LEVEL_DISABLED) != 0;
plansource->row_security_env = row_security;
plansource->planUserId = InvalidOid;
--- 204,210 ----
plansource->total_custom_cost = 0;
plansource->num_custom_plans = 0;
plansource->hasRowSecurity = false;
! plansource->rowSecurityDisabled = InRowLevelSecurityDisabled();
plansource->row_security_env = row_security;
plansource->planUserId = InvalidOid;
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index acc4752..ac3e764 100644
*** a/src/backend/utils/init/miscinit.c
--- b/src/backend/utils/init/miscinit.c
*************** GetAuthenticatedUserId(void)
*** 341,347 ****
* GetUserIdAndSecContext/SetUserIdAndSecContext - get/set the current user ID
* and the SecurityRestrictionContext flags.
*
! * Currently there are two valid bits in SecurityRestrictionContext:
*
* SECURITY_LOCAL_USERID_CHANGE indicates that we are inside an operation
* that is temporarily changing CurrentUserId via these functions. This is
--- 341,347 ----
* GetUserIdAndSecContext/SetUserIdAndSecContext - get/set the current user ID
* and the SecurityRestrictionContext flags.
*
! * Currently there are three valid bits in SecurityRestrictionContext:
*
* SECURITY_LOCAL_USERID_CHANGE indicates that we are inside an operation
* that is temporarily changing CurrentUserId via these functions. This is
*************** GetAuthenticatedUserId(void)
*** 359,364 ****
--- 359,367 ----
* where the called functions are really supposed to be side-effect-free
* anyway, such as VACUUM/ANALYZE/REINDEX.
*
+ * SECURITY_ROW_LEVEL_DISABLED indicates that we are inside an operation that
+ * needs to bypass row level security checks, for example FK checks.
+ *
* Unlike GetUserId, GetUserIdAndSecContext does *not* Assert that the current
* value of CurrentUserId is valid; nor does SetUserIdAndSecContext require
* the new value to be valid. In fact, these routines had better not
*************** InSecurityRestrictedOperation(void)
*** 401,406 ****
--- 404,418 ----
return (SecurityRestrictionContext & SECURITY_RESTRICTED_OPERATION) != 0;
}
+ /*
+ * InRowLevelSecurityDisabled - are we inside a RLS-disabled operation?
+ */
+ bool
+ InRowLevelSecurityDisabled(void)
+ {
+ return (SecurityRestrictionContext & SECURITY_ROW_LEVEL_DISABLED) != 0;
+ }
+
/*
* These are obsolete versions of Get/SetUserIdAndSecContext that are
diff --git a/src/backend/utils/misc/rls.c b/src/backend/utils/misc/rls.c
index 44cb374..7b8d51d 100644
*** a/src/backend/utils/misc/rls.c
--- b/src/backend/utils/misc/rls.c
***************
*** 16,24 ****
--- 16,27 ----
#include "access/htup.h"
#include "access/htup_details.h"
+ #include "access/transam.h"
#include "catalog/pg_class.h"
+ #include "catalog/namespace.h"
#include "miscadmin.h"
#include "utils/acl.h"
+ #include "utils/builtins.h"
#include "utils/elog.h"
#include "utils/rls.h"
#include "utils/syscache.h"
*************** extern int check_enable_rls(Oid relid, O
*** 37,43 ****
* for the table and the plan cache needs to be invalidated if the environment
* changes.
*
! * Handle checking as another role via checkAsUser (for views, etc).
*
* If noError is set to 'true' then we just return RLS_ENABLED instead of doing
* an ereport() if the user has attempted to bypass RLS and they are not
--- 40,49 ----
* for the table and the plan cache needs to be invalidated if the environment
* changes.
*
! * Handle checking as another role via checkAsUser (for views, etc). Note that
! * if *not* checking as another role, the caller should pass InvalidOid rather
! * than GetUserId(). Otherwise the check for row_security = OFF is skipped, and
! * so we may falsely report that RLS is active when the user has bypassed it.
*
* If noError is set to 'true' then we just return RLS_ENABLED instead of doing
* an ereport() if the user has attempted to bypass RLS and they are not
*************** check_enable_rls(Oid relid, Oid checkAsU
*** 53,58 ****
--- 59,75 ----
bool relrowsecurity;
Oid user_id = checkAsUser ? checkAsUser : GetUserId();
+ /* Nothing to do for built-in relations */
+ if (relid < FirstNormalObjectId)
+ return RLS_NONE;
+
+ /*
+ * Check if we have been told to explicitly skip RLS (perhaps because this
+ * is a foreign key check)
+ */
+ if (InRowLevelSecurityDisabled())
+ return RLS_NONE;
+
tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
if (!HeapTupleIsValid(tuple))
return RLS_NONE;
*************** check_enable_rls(Oid relid, Oid checkAsU
*** 111,113 ****
--- 128,164 ----
/* RLS should be fully enabled for this relation. */
return RLS_ENABLED;
}
+
+ /*
+ * row_security_active
+ *
+ * check_enable_rls wrapped as a SQL callable function except
+ * RLS_NONE_ENV and RLS_NONE are the same for this purpose.
+ */
+ Datum
+ row_security_active(PG_FUNCTION_ARGS)
+ {
+ /* By OID */
+ Oid tableoid = PG_GETARG_OID(0);
+ int rls_status;
+
+ rls_status = check_enable_rls(tableoid, InvalidOid, true);
+ PG_RETURN_BOOL(rls_status == RLS_ENABLED);
+ }
+
+ Datum
+ row_security_active_name(PG_FUNCTION_ARGS)
+ {
+ /* By qualified name */
+ text *tablename = PG_GETARG_TEXT_P(0);
+ RangeVar *tablerel;
+ Oid tableoid;
+ int rls_status;
+
+ /* Look up table name. Can't lock it - we might not have privileges. */
+ tablerel = makeRangeVarFromNameList(textToQualifiedNameList(tablename));
+ tableoid = RangeVarGetRelid(tablerel, NoLock, false);
+
+ rls_status = check_enable_rls(tableoid, InvalidOid, true);
+ PG_RETURN_BOOL(rls_status == RLS_ENABLED);
+ }
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 09bf143..2563bb9 100644
*** a/src/include/catalog/pg_proc.h
--- b/src/include/catalog/pg_proc.h
*************** DESCR("get progress for all replication
*** 5343,5348 ****
--- 5343,5354 ----
#define PROVOLATILE_STABLE 's' /* does not change within a scan */
#define PROVOLATILE_VOLATILE 'v' /* can change even within a scan */
+ /* rls */
+ DATA(insert OID = 3298 ( row_security_active PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 16 "26" _null_ _null_ _null_ _null_ _null_ row_security_active _null_ _null_ _null_ ));
+ DESCR("row security for current context active on table by table oid");
+ DATA(insert OID = 3299 ( row_security_active PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 16 "25" _null_ _null_ _null_ _null_ _null_ row_security_active_name _null_ _null_ _null_ ));
+ DESCR("row security for current context active on table by table name");
+
/*
* Symbolic values for proargmodes column. Note that these must agree with
* the FunctionParameterMode enum in parsenodes.h; we declare them here to
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index b539167..e0cc69f 100644
*** a/src/include/miscadmin.h
--- b/src/include/miscadmin.h
*************** extern void GetUserIdAndSecContext(Oid *
*** 305,310 ****
--- 305,311 ----
extern void SetUserIdAndSecContext(Oid userid, int sec_context);
extern bool InLocalUserIdChange(void);
extern bool InSecurityRestrictedOperation(void);
+ extern bool InRowLevelSecurityDisabled(void);
extern void GetUserIdAndContext(Oid *userid, bool *sec_def_context);
extern void SetUserIdAndContext(Oid userid, bool sec_def_context);
extern void InitializeSessionUserId(const char *rolename, Oid useroid);
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index 49caa56..fc1679e 100644
*** a/src/include/utils/builtins.h
--- b/src/include/utils/builtins.h
*************** extern Datum set_config_by_name(PG_FUNCT
*** 1121,1126 ****
--- 1121,1130 ----
extern Datum show_all_settings(PG_FUNCTION_ARGS);
extern Datum show_all_file_settings(PG_FUNCTION_ARGS);
+ /* rls.c */
+ extern Datum row_security_active(PG_FUNCTION_ARGS);
+ extern Datum row_security_active_name(PG_FUNCTION_ARGS);
+
/* lockfuncs.c */
extern Datum pg_lock_status(PG_FUNCTION_ARGS);
extern Datum pg_advisory_lock_int8(PG_FUNCTION_ARGS);
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
index e7c242c..98e36f2 100644
*** a/src/test/regress/expected/rowsecurity.out
--- b/src/test/regress/expected/rowsecurity.out
*************** SELECT * FROM current_check;
*** 2839,2848 ****
COMMIT;
--
-- Collation support
--
BEGIN;
! SET row_security = force;
CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
--- 2839,2882 ----
COMMIT;
--
+ -- check pg_stats view filtering
+ --
+ SET row_security TO ON;
+ SET SESSION AUTHORIZATION rls_regress_user0;
+ ANALYZE current_check;
+ -- Stats visible
+ SELECT row_security_active('current_check');
+ row_security_active
+ ---------------------
+ f
+ (1 row)
+
+ SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
+ most_common_vals
+ ---------------------
+
+
+ {rls_regress_user1}
+ (3 rows)
+
+ SET SESSION AUTHORIZATION rls_regress_user1;
+ -- Stats not visible
+ SELECT row_security_active('current_check');
+ row_security_active
+ ---------------------
+ t
+ (1 row)
+
+ SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
+ most_common_vals
+ ------------------
+ (0 rows)
+
+ --
-- Collation support
--
BEGIN;
! SET row_security TO FORCE;
CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 1e5b0b9..6206c81 100644
*** a/src/test/regress/expected/rules.out
--- b/src/test/regress/expected/rules.out
*************** pg_stats| SELECT n.nspname AS schemaname
*** 2061,2067 ****
JOIN pg_class c ON ((c.oid = s.starelid)))
JOIN pg_attribute a ON (((c.oid = a.attrelid) AND (a.attnum = s.staattnum))))
LEFT JOIN pg_namespace n ON ((n.oid = c.relnamespace)))
! WHERE ((NOT a.attisdropped) AND has_column_privilege(c.oid, a.attnum, 'select'::text));
pg_tables| SELECT n.nspname AS schemaname,
c.relname AS tablename,
pg_get_userbyid(c.relowner) AS tableowner,
--- 2061,2067 ----
JOIN pg_class c ON ((c.oid = s.starelid)))
JOIN pg_attribute a ON (((c.oid = a.attrelid) AND (a.attnum = s.staattnum))))
LEFT JOIN pg_namespace n ON ((n.oid = c.relnamespace)))
! WHERE ((NOT a.attisdropped) AND has_column_privilege(c.oid, a.attnum, 'select'::text) AND ((c.relrowsecurity = false) OR (NOT row_security_active(c.oid))));
pg_tables| SELECT n.nspname AS schemaname,
c.relname AS tablename,
pg_get_userbyid(c.relowner) AS tableowner,
diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
index e86f814..73cc020 100644
*** a/src/test/regress/sql/rowsecurity.sql
--- b/src/test/regress/sql/rowsecurity.sql
*************** SELECT * FROM current_check;
*** 1141,1150 ****
COMMIT;
--
-- Collation support
--
BEGIN;
! SET row_security = force;
CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
--- 1141,1165 ----
COMMIT;
--
+ -- check pg_stats view filtering
+ --
+ SET row_security TO ON;
+ SET SESSION AUTHORIZATION rls_regress_user0;
+ ANALYZE current_check;
+ -- Stats visible
+ SELECT row_security_active('current_check');
+ SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
+
+ SET SESSION AUTHORIZATION rls_regress_user1;
+ -- Stats not visible
+ SELECT row_security_active('current_check');
+ SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
+
+ --
-- Collation support
--
BEGIN;
! SET row_security TO FORCE;
CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
On 28 July 2015 at 03:19, Joe Conway <mail@joeconway.com> wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1On 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
-----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-----
Attachments:
20150728.00-rls-pg-stats.v7.patchtext/x-diff; name=20150728.00-rls-pg-stats.v7.patchDownload
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 98d1497..fd82ea4 100644
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
*************** SET search_path TO <replaceable>schema</
*** 15259,15264 ****
--- 15259,15270 ----
<entry><type>boolean</type></entry>
<entry>does current user have privilege for role</entry>
</row>
+ <row>
+ <entry><literal><function>row_security_active</function>(<parameter>table</parameter>)</literal>
+ </entry>
+ <entry><type>boolean</type></entry>
+ <entry>does current user have row level security active for table</entry>
+ </row>
</tbody>
</tgroup>
</table>
*************** SET search_path TO <replaceable>schema</
*** 15299,15304 ****
--- 15305,15313 ----
<indexterm>
<primary>pg_has_role</primary>
</indexterm>
+ <indexterm>
+ <primary>row_security_active</primary>
+ </indexterm>
<para>
<function>has_table_privilege</function> checks whether a user
*************** SELECT has_function_privilege('joeuser',
*** 15462,15467 ****
--- 15471,15483 ----
are immediately available without doing <command>SET ROLE</>.
</para>
+ <para>
+ <function>row_security_active</function> checks whether row level
+ security is active for the specified table in the context of the
+ <function>current_user</function> and environment. The table can
+ be specified by name or by OID.
+ </para>
+
<para>
<xref linkend="functions-info-schema-table"> shows functions that
determine whether a certain object is <firstterm>visible</> in the
diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
index 1043362..aa5b28c 100644
*** a/src/backend/access/index/genam.c
--- b/src/backend/access/index/genam.c
*************** BuildIndexValueDescription(Relation inde
*** 204,210 ****
Assert(indexrelid == idxrec->indexrelid);
/* RLS check- if RLS is enabled then we don't return anything. */
! if (check_enable_rls(indrelid, GetUserId(), true) == RLS_ENABLED)
{
ReleaseSysCache(ht_idx);
return NULL;
--- 204,210 ----
Assert(indexrelid == idxrec->indexrelid);
/* RLS check- if RLS is enabled then we don't return anything. */
! if (check_enable_rls(indrelid, InvalidOid, true) == RLS_ENABLED)
{
ReleaseSysCache(ht_idx);
return NULL;
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index e82a53a..c0bd6fa 100644
*** a/src/backend/catalog/system_views.sql
--- b/src/backend/catalog/system_views.sql
*************** CREATE VIEW pg_indexes AS
*** 150,156 ****
LEFT JOIN pg_tablespace T ON (T.oid = I.reltablespace)
WHERE C.relkind IN ('r', 'm') AND I.relkind = 'i';
! CREATE VIEW pg_stats AS
SELECT
nspname AS schemaname,
relname AS tablename,
--- 150,156 ----
LEFT JOIN pg_tablespace T ON (T.oid = I.reltablespace)
WHERE C.relkind IN ('r', 'm') AND I.relkind = 'i';
! CREATE VIEW pg_stats WITH (security_barrier) AS
SELECT
nspname AS schemaname,
relname AS tablename,
*************** CREATE VIEW pg_stats AS
*** 211,217 ****
FROM pg_statistic s JOIN pg_class c ON (c.oid = s.starelid)
JOIN pg_attribute a ON (c.oid = attrelid AND attnum = s.staattnum)
LEFT JOIN pg_namespace n ON (n.oid = c.relnamespace)
! WHERE NOT attisdropped AND has_column_privilege(c.oid, a.attnum, 'select');
REVOKE ALL on pg_statistic FROM public;
--- 211,219 ----
FROM pg_statistic s JOIN pg_class c ON (c.oid = s.starelid)
JOIN pg_attribute a ON (c.oid = attrelid AND attnum = s.staattnum)
LEFT JOIN pg_namespace n ON (n.oid = c.relnamespace)
! WHERE NOT attisdropped
! AND has_column_privilege(c.oid, a.attnum, 'select')
! AND (c.relrowsecurity = false OR NOT row_security_active(c.oid));
REVOKE ALL on pg_statistic FROM public;
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index a1561ce..2c65a90 100644
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
*************** ExecBuildSlotValueDescription(Oid reloid
*** 1874,1880 ****
* then don't return anything. Otherwise, go through normal permission
* checks.
*/
! if (check_enable_rls(reloid, GetUserId(), true) == RLS_ENABLED)
return NULL;
initStringInfo(&buf);
--- 1874,1880 ----
* then don't return anything. Otherwise, go through normal permission
* checks.
*/
! if (check_enable_rls(reloid, InvalidOid, true) == RLS_ENABLED)
return NULL;
initStringInfo(&buf);
diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c
index aaf0061..2386cf0 100644
*** a/src/backend/rewrite/rowsecurity.c
--- b/src/backend/rewrite/rowsecurity.c
*************** get_row_security_policies(Query *root, C
*** 107,113 ****
Relation rel;
Oid user_id;
- int sec_context;
int rls_status;
bool defaultDeny = false;
--- 107,112 ----
*************** get_row_security_policies(Query *root, C
*** 117,138 ****
*hasRowSecurity = false;
*hasSubLinks = false;
! /* This is just to get the security context */
! GetUserIdAndSecContext(&user_id, &sec_context);
/* Switch to checkAsUser if it's set */
user_id = rte->checkAsUser ? rte->checkAsUser : GetUserId();
- /*
- * If this is not a normal relation, or we have been told to explicitly
- * skip RLS (perhaps because this is an FK check) then just return
- * immediately.
- */
- if (rte->relid < FirstNormalObjectId
- || rte->relkind != RELKIND_RELATION
- || (sec_context & SECURITY_ROW_LEVEL_DISABLED))
- return;
-
/* Determine the state of RLS for this, pass checkAsUser explicitly */
rls_status = check_enable_rls(rte->relid, rte->checkAsUser, false);
--- 116,128 ----
*hasRowSecurity = false;
*hasSubLinks = false;
! /* If this is not a normal relation, just return immediately */
! if (rte->relkind != RELKIND_RELATION)
! return;
/* Switch to checkAsUser if it's set */
user_id = rte->checkAsUser ? rte->checkAsUser : GetUserId();
/* Determine the state of RLS for this, pass checkAsUser explicitly */
rls_status = check_enable_rls(rte->relid, rte->checkAsUser, false);
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 88dd3fa..61edde9 100644
*** a/src/backend/utils/adt/ri_triggers.c
--- b/src/backend/utils/adt/ri_triggers.c
*************** ri_ReportViolation(const RI_ConstraintIn
*** 3243,3249 ****
* privileges.
*/
! if (check_enable_rls(rel_oid, GetUserId(), true) != RLS_ENABLED)
{
aclresult = pg_class_aclcheck(rel_oid, GetUserId(), ACL_SELECT);
if (aclresult != ACLCHECK_OK)
--- 3243,3249 ----
* privileges.
*/
! if (check_enable_rls(rel_oid, InvalidOid, true) != RLS_ENABLED)
{
aclresult = pg_class_aclcheck(rel_oid, GetUserId(), ACL_SELECT);
if (aclresult != ACLCHECK_OK)
*************** ri_ReportViolation(const RI_ConstraintIn
*** 3264,3269 ****
--- 3264,3271 ----
}
}
}
+ else
+ has_perm = false;
if (has_perm)
{
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index e6808e7..525794f 100644
*** a/src/backend/utils/cache/plancache.c
--- b/src/backend/utils/cache/plancache.c
*************** CreateCachedPlan(Node *raw_parse_tree,
*** 153,160 ****
CachedPlanSource *plansource;
MemoryContext source_context;
MemoryContext oldcxt;
- Oid user_id;
- int security_context;
Assert(query_string != NULL); /* required as of 8.4 */
--- 153,158 ----
*************** CreateCachedPlan(Node *raw_parse_tree,
*** 177,184 ****
*/
oldcxt = MemoryContextSwitchTo(source_context);
- GetUserIdAndSecContext(&user_id, &security_context);
-
plansource = (CachedPlanSource *) palloc0(sizeof(CachedPlanSource));
plansource->magic = CACHEDPLANSOURCE_MAGIC;
plansource->raw_parse_tree = copyObject(raw_parse_tree);
--- 175,180 ----
*************** CreateCachedPlan(Node *raw_parse_tree,
*** 208,215 ****
plansource->total_custom_cost = 0;
plansource->num_custom_plans = 0;
plansource->hasRowSecurity = false;
! plansource->rowSecurityDisabled
! = (security_context & SECURITY_ROW_LEVEL_DISABLED) != 0;
plansource->row_security_env = row_security;
plansource->planUserId = InvalidOid;
--- 204,210 ----
plansource->total_custom_cost = 0;
plansource->num_custom_plans = 0;
plansource->hasRowSecurity = false;
! plansource->rowSecurityDisabled = InRowLevelSecurityDisabled();
plansource->row_security_env = row_security;
plansource->planUserId = InvalidOid;
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index acc4752..ac3e764 100644
*** a/src/backend/utils/init/miscinit.c
--- b/src/backend/utils/init/miscinit.c
*************** GetAuthenticatedUserId(void)
*** 341,347 ****
* GetUserIdAndSecContext/SetUserIdAndSecContext - get/set the current user ID
* and the SecurityRestrictionContext flags.
*
! * Currently there are two valid bits in SecurityRestrictionContext:
*
* SECURITY_LOCAL_USERID_CHANGE indicates that we are inside an operation
* that is temporarily changing CurrentUserId via these functions. This is
--- 341,347 ----
* GetUserIdAndSecContext/SetUserIdAndSecContext - get/set the current user ID
* and the SecurityRestrictionContext flags.
*
! * Currently there are three valid bits in SecurityRestrictionContext:
*
* SECURITY_LOCAL_USERID_CHANGE indicates that we are inside an operation
* that is temporarily changing CurrentUserId via these functions. This is
*************** GetAuthenticatedUserId(void)
*** 359,364 ****
--- 359,367 ----
* where the called functions are really supposed to be side-effect-free
* anyway, such as VACUUM/ANALYZE/REINDEX.
*
+ * SECURITY_ROW_LEVEL_DISABLED indicates that we are inside an operation that
+ * needs to bypass row level security checks, for example FK checks.
+ *
* Unlike GetUserId, GetUserIdAndSecContext does *not* Assert that the current
* value of CurrentUserId is valid; nor does SetUserIdAndSecContext require
* the new value to be valid. In fact, these routines had better not
*************** InSecurityRestrictedOperation(void)
*** 401,406 ****
--- 404,418 ----
return (SecurityRestrictionContext & SECURITY_RESTRICTED_OPERATION) != 0;
}
+ /*
+ * InRowLevelSecurityDisabled - are we inside a RLS-disabled operation?
+ */
+ bool
+ InRowLevelSecurityDisabled(void)
+ {
+ return (SecurityRestrictionContext & SECURITY_ROW_LEVEL_DISABLED) != 0;
+ }
+
/*
* These are obsolete versions of Get/SetUserIdAndSecContext that are
diff --git a/src/backend/utils/misc/rls.c b/src/backend/utils/misc/rls.c
index 44cb374..7b8d51d 100644
*** a/src/backend/utils/misc/rls.c
--- b/src/backend/utils/misc/rls.c
***************
*** 16,24 ****
--- 16,27 ----
#include "access/htup.h"
#include "access/htup_details.h"
+ #include "access/transam.h"
#include "catalog/pg_class.h"
+ #include "catalog/namespace.h"
#include "miscadmin.h"
#include "utils/acl.h"
+ #include "utils/builtins.h"
#include "utils/elog.h"
#include "utils/rls.h"
#include "utils/syscache.h"
*************** extern int check_enable_rls(Oid relid, O
*** 37,43 ****
* for the table and the plan cache needs to be invalidated if the environment
* changes.
*
! * Handle checking as another role via checkAsUser (for views, etc).
*
* If noError is set to 'true' then we just return RLS_ENABLED instead of doing
* an ereport() if the user has attempted to bypass RLS and they are not
--- 40,49 ----
* for the table and the plan cache needs to be invalidated if the environment
* changes.
*
! * Handle checking as another role via checkAsUser (for views, etc). Note that
! * if *not* checking as another role, the caller should pass InvalidOid rather
! * than GetUserId(). Otherwise the check for row_security = OFF is skipped, and
! * so we may falsely report that RLS is active when the user has bypassed it.
*
* If noError is set to 'true' then we just return RLS_ENABLED instead of doing
* an ereport() if the user has attempted to bypass RLS and they are not
*************** check_enable_rls(Oid relid, Oid checkAsU
*** 53,58 ****
--- 59,75 ----
bool relrowsecurity;
Oid user_id = checkAsUser ? checkAsUser : GetUserId();
+ /* Nothing to do for built-in relations */
+ if (relid < FirstNormalObjectId)
+ return RLS_NONE;
+
+ /*
+ * Check if we have been told to explicitly skip RLS (perhaps because this
+ * is a foreign key check)
+ */
+ if (InRowLevelSecurityDisabled())
+ return RLS_NONE;
+
tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
if (!HeapTupleIsValid(tuple))
return RLS_NONE;
*************** check_enable_rls(Oid relid, Oid checkAsU
*** 111,113 ****
--- 128,164 ----
/* RLS should be fully enabled for this relation. */
return RLS_ENABLED;
}
+
+ /*
+ * row_security_active
+ *
+ * check_enable_rls wrapped as a SQL callable function except
+ * RLS_NONE_ENV and RLS_NONE are the same for this purpose.
+ */
+ Datum
+ row_security_active(PG_FUNCTION_ARGS)
+ {
+ /* By OID */
+ Oid tableoid = PG_GETARG_OID(0);
+ int rls_status;
+
+ rls_status = check_enable_rls(tableoid, InvalidOid, true);
+ PG_RETURN_BOOL(rls_status == RLS_ENABLED);
+ }
+
+ Datum
+ row_security_active_name(PG_FUNCTION_ARGS)
+ {
+ /* By qualified name */
+ text *tablename = PG_GETARG_TEXT_P(0);
+ RangeVar *tablerel;
+ Oid tableoid;
+ int rls_status;
+
+ /* Look up table name. Can't lock it - we might not have privileges. */
+ tablerel = makeRangeVarFromNameList(textToQualifiedNameList(tablename));
+ tableoid = RangeVarGetRelid(tablerel, NoLock, false);
+
+ rls_status = check_enable_rls(tableoid, InvalidOid, true);
+ PG_RETURN_BOOL(rls_status == RLS_ENABLED);
+ }
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 09bf143..2563bb9 100644
*** a/src/include/catalog/pg_proc.h
--- b/src/include/catalog/pg_proc.h
*************** DESCR("get progress for all replication
*** 5343,5348 ****
--- 5343,5354 ----
#define PROVOLATILE_STABLE 's' /* does not change within a scan */
#define PROVOLATILE_VOLATILE 'v' /* can change even within a scan */
+ /* rls */
+ DATA(insert OID = 3298 ( row_security_active PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 16 "26" _null_ _null_ _null_ _null_ _null_ row_security_active _null_ _null_ _null_ ));
+ DESCR("row security for current context active on table by table oid");
+ DATA(insert OID = 3299 ( row_security_active PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 16 "25" _null_ _null_ _null_ _null_ _null_ row_security_active_name _null_ _null_ _null_ ));
+ DESCR("row security for current context active on table by table name");
+
/*
* Symbolic values for proargmodes column. Note that these must agree with
* the FunctionParameterMode enum in parsenodes.h; we declare them here to
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index b539167..e0cc69f 100644
*** a/src/include/miscadmin.h
--- b/src/include/miscadmin.h
*************** extern void GetUserIdAndSecContext(Oid *
*** 305,310 ****
--- 305,311 ----
extern void SetUserIdAndSecContext(Oid userid, int sec_context);
extern bool InLocalUserIdChange(void);
extern bool InSecurityRestrictedOperation(void);
+ extern bool InRowLevelSecurityDisabled(void);
extern void GetUserIdAndContext(Oid *userid, bool *sec_def_context);
extern void SetUserIdAndContext(Oid userid, bool sec_def_context);
extern void InitializeSessionUserId(const char *rolename, Oid useroid);
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index 49caa56..fc1679e 100644
*** a/src/include/utils/builtins.h
--- b/src/include/utils/builtins.h
*************** extern Datum set_config_by_name(PG_FUNCT
*** 1121,1126 ****
--- 1121,1130 ----
extern Datum show_all_settings(PG_FUNCTION_ARGS);
extern Datum show_all_file_settings(PG_FUNCTION_ARGS);
+ /* rls.c */
+ extern Datum row_security_active(PG_FUNCTION_ARGS);
+ extern Datum row_security_active_name(PG_FUNCTION_ARGS);
+
/* lockfuncs.c */
extern Datum pg_lock_status(PG_FUNCTION_ARGS);
extern Datum pg_advisory_lock_int8(PG_FUNCTION_ARGS);
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
index 72361e8..fd8e180 100644
*** a/src/test/regress/expected/rowsecurity.out
--- b/src/test/regress/expected/rowsecurity.out
*************** SELECT * FROM document d FULL OUTER JOIN
*** 307,313 ****
DELETE FROM category WHERE cid = 33; -- fails with FK violation
ERROR: update or delete on table "category" violates foreign key constraint "document_cid_fkey" on table "document"
! DETAIL: Key (cid)=(33) is still referenced from table "document".
-- can insert FK referencing invisible PK
SET SESSION AUTHORIZATION rls_regress_user2;
SELECT * FROM document d FULL OUTER JOIN category c on d.cid = c.cid;
--- 307,313 ----
DELETE FROM category WHERE cid = 33; -- fails with FK violation
ERROR: update or delete on table "category" violates foreign key constraint "document_cid_fkey" on table "document"
! DETAIL: Key is still referenced from table "document".
-- can insert FK referencing invisible PK
SET SESSION AUTHORIZATION rls_regress_user2;
SELECT * FROM document d FULL OUTER JOIN category c on d.cid = c.cid;
*************** SELECT * FROM current_check;
*** 2887,2896 ****
COMMIT;
--
-- Collation support
--
BEGIN;
! SET row_security = force;
CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
--- 2887,2930 ----
COMMIT;
--
+ -- check pg_stats view filtering
+ --
+ SET row_security TO ON;
+ SET SESSION AUTHORIZATION rls_regress_user0;
+ ANALYZE current_check;
+ -- Stats visible
+ SELECT row_security_active('current_check');
+ row_security_active
+ ---------------------
+ f
+ (1 row)
+
+ SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
+ most_common_vals
+ ---------------------
+
+
+ {rls_regress_user1}
+ (3 rows)
+
+ SET SESSION AUTHORIZATION rls_regress_user1;
+ -- Stats not visible
+ SELECT row_security_active('current_check');
+ row_security_active
+ ---------------------
+ t
+ (1 row)
+
+ SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
+ most_common_vals
+ ------------------
+ (0 rows)
+
+ --
-- Collation support
--
BEGIN;
! SET row_security TO FORCE;
CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 1e5b0b9..6206c81 100644
*** a/src/test/regress/expected/rules.out
--- b/src/test/regress/expected/rules.out
*************** pg_stats| SELECT n.nspname AS schemaname
*** 2061,2067 ****
JOIN pg_class c ON ((c.oid = s.starelid)))
JOIN pg_attribute a ON (((c.oid = a.attrelid) AND (a.attnum = s.staattnum))))
LEFT JOIN pg_namespace n ON ((n.oid = c.relnamespace)))
! WHERE ((NOT a.attisdropped) AND has_column_privilege(c.oid, a.attnum, 'select'::text));
pg_tables| SELECT n.nspname AS schemaname,
c.relname AS tablename,
pg_get_userbyid(c.relowner) AS tableowner,
--- 2061,2067 ----
JOIN pg_class c ON ((c.oid = s.starelid)))
JOIN pg_attribute a ON (((c.oid = a.attrelid) AND (a.attnum = s.staattnum))))
LEFT JOIN pg_namespace n ON ((n.oid = c.relnamespace)))
! WHERE ((NOT a.attisdropped) AND has_column_privilege(c.oid, a.attnum, 'select'::text) AND ((c.relrowsecurity = false) OR (NOT row_security_active(c.oid))));
pg_tables| SELECT n.nspname AS schemaname,
c.relname AS tablename,
pg_get_userbyid(c.relowner) AS tableowner,
diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
index f588fa2..32f10d8 100644
*** a/src/test/regress/sql/rowsecurity.sql
--- b/src/test/regress/sql/rowsecurity.sql
*************** SELECT * FROM current_check;
*** 1190,1199 ****
COMMIT;
--
-- Collation support
--
BEGIN;
! SET row_security = force;
CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
--- 1190,1214 ----
COMMIT;
--
+ -- check pg_stats view filtering
+ --
+ SET row_security TO ON;
+ SET SESSION AUTHORIZATION rls_regress_user0;
+ ANALYZE current_check;
+ -- Stats visible
+ SELECT row_security_active('current_check');
+ SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
+
+ SET SESSION AUTHORIZATION rls_regress_user1;
+ -- Stats not visible
+ SELECT row_security_active('current_check');
+ SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
+
+ --
-- Collation support
--
BEGIN;
! SET row_security TO FORCE;
CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
* 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-----
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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