-----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-----
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;
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers