]> granicus.if.org Git - postgresql/commitdiff
Plug RLS related information leak in pg_stats view.
authorJoe Conway <mail@joeconway.com>
Tue, 28 Jul 2015 20:21:37 +0000 (13:21 -0700)
committerJoe Conway <mail@joeconway.com>
Tue, 28 Jul 2015 20:21:37 +0000 (13:21 -0700)
The pg_stats view is supposed to be restricted to only show rows
about tables the user can read. However, it sometimes can leak
information which could not otherwise be seen when row level security
is enabled. Fix that by not showing pg_stats rows to users that would
be subject to RLS on the table the row is related to. This is done
by creating/using the newly introduced SQL visible function,
row_security_active().

Along the way, clean up three call sites of check_enable_rls(). The second
argument of that function should only be specified as other than
InvalidOid when we are checking as a different user than the current one,
as in when querying through a view. These sites were passing GetUserId()
instead of InvalidOid, which can cause the function to return incorrect
results if the current user has the BYPASSRLS privilege and row_security
has been set to OFF.

Additionally fix a bug causing RI Trigger error messages to unintentionally
leak information when RLS is enabled, and other minor cleanup and
improvements. Also add WITH (security_barrier) to the definition of pg_stats.

Bumped CATVERSION due to new SQL functions and pg_stats view definition.

Back-patch to 9.5 where RLS was introduced. Reported by Yaroslav.
Patch by Joe Conway and Dean Rasheed with review and input by
Michael Paquier and Stephen Frost.

16 files changed:
doc/src/sgml/func.sgml
src/backend/access/index/genam.c
src/backend/catalog/system_views.sql
src/backend/executor/execMain.c
src/backend/rewrite/rowsecurity.c
src/backend/utils/adt/ri_triggers.c
src/backend/utils/cache/plancache.c
src/backend/utils/init/miscinit.c
src/backend/utils/misc/rls.c
src/include/catalog/catversion.h
src/include/catalog/pg_proc.h
src/include/miscadmin.h
src/include/utils/builtins.h
src/test/regress/expected/rowsecurity.out
src/test/regress/expected/rules.out
src/test/regress/sql/rowsecurity.sql

index ef50fa581135b3b1748bddf26e624c9bfb010fd0..17aa1d77c9f077297dbd064cf015c221d7402588 100644 (file)
@@ -15244,6 +15244,12 @@ SET search_path TO <replaceable>schema</> <optional>, <replaceable>schema</>, ..
        <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>
@@ -15284,6 +15290,9 @@ SET search_path TO <replaceable>schema</> <optional>, <replaceable>schema</>, ..
    <indexterm>
     <primary>pg_has_role</primary>
    </indexterm>
+   <indexterm>
+    <primary>row_security_active</primary>
+   </indexterm>
 
    <para>
     <function>has_table_privilege</function> checks whether a user
@@ -15447,6 +15456,13 @@ SELECT has_function_privilege('joeuser', 'myfunc(int, text)', 'execute');
     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
index 1043362f914e2dd5bab62c3c39cfa5a18b532d6e..aa5b28c61a07c4d1dde7de4932835ebb21753bd2 100644 (file)
@@ -204,7 +204,7 @@ BuildIndexValueDescription(Relation indexRelation,
        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)
+       if (check_enable_rls(indrelid, InvalidOid, true) == RLS_ENABLED)
        {
                ReleaseSysCache(ht_idx);
                return NULL;
index e82a53aee93649898bdbc39d8e0f9d398a0eaecf..c0bd6fa96b750a07d5a2e970fc4dbb3f93243e85 100644 (file)
@@ -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;
 
index a1561ce0cc0ff69be73f90f9f6c92bbcca754a29..2c65a901d945d07f6a79b8f56d91c62b2d62191e 100644 (file)
@@ -1874,7 +1874,7 @@ ExecBuildSlotValueDescription(Oid reloid,
         * then don't return anything.  Otherwise, go through normal permission
         * checks.
         */
-       if (check_enable_rls(reloid, GetUserId(), true) == RLS_ENABLED)
+       if (check_enable_rls(reloid, InvalidOid, true) == RLS_ENABLED)
                return NULL;
 
        initStringInfo(&buf);
index aaf0061164b2969f727ec9adad2272280f4f622a..2386cf016fbc52cbe4f9ac6cc64a4abc1f3e7158 100644 (file)
@@ -107,7 +107,6 @@ get_row_security_policies(Query *root, CmdType commandType, RangeTblEntry *rte,
 
        Relation        rel;
        Oid                     user_id;
-       int                     sec_context;
        int                     rls_status;
        bool            defaultDeny = false;
 
@@ -117,22 +116,13 @@ get_row_security_policies(Query *root, CmdType commandType, RangeTblEntry *rte,
        *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);
 
index 88dd3faf2d9a024fa1b875a8774a4ed21e327d7e..61edde9c5d35a4ccdae954d7e25329d208ab15e8 100644 (file)
@@ -3243,7 +3243,7 @@ ri_ReportViolation(const RI_ConstraintInfo *riinfo,
         * privileges.
         */
 
-       if (check_enable_rls(rel_oid, GetUserId(), true) != RLS_ENABLED)
+       if (check_enable_rls(rel_oid, InvalidOid, true) != RLS_ENABLED)
        {
                aclresult = pg_class_aclcheck(rel_oid, GetUserId(), ACL_SELECT);
                if (aclresult != ACLCHECK_OK)
@@ -3264,6 +3264,8 @@ ri_ReportViolation(const RI_ConstraintInfo *riinfo,
                        }
                }
        }
+       else
+               has_perm = false;
 
        if (has_perm)
        {
index e6808e75763593411ef9c964d3c4ead343d390c0..525794fb64450407504bab67e2da2da655a44eb8 100644 (file)
@@ -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;
 
index acc4752015b3217ce8bd9a6e49f6d75fbce4f2dd..ac3e764e8b8c2bb6cb0d3211a1aa1452e65aed67 100644 (file)
@@ -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
index 44cb3743034a16fcba4f5bac39160e743fd00136..7b8d51d956f2961a081fede0622fd123cf2f6ff2 100644 (file)
 
 #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"
@@ -37,7 +40,10 @@ extern int   check_enable_rls(Oid relid, Oid checkAsUser, bool noError);
  * 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).
+ * 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
@@ -53,6 +59,17 @@ check_enable_rls(Oid relid, Oid checkAsUser, bool noError)
        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;
@@ -111,3 +128,37 @@ check_enable_rls(Oid relid, Oid checkAsUser, bool noError)
        /* 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);
+}
index 349dd2531028aea84fddfc5eeb196975b2ab38f9..0bc1ee222a3db308d556075e6be9ae39fb316bfa 100644 (file)
@@ -53,6 +53,6 @@
  */
 
 /*                                                     yyyymmddN */
-#define CATALOG_VERSION_NO     201507251
+#define CATALOG_VERSION_NO     201507281
 
 #endif
index be55666dd076bd802be8b85f83f995552da9b307..9a27399c5128e0e0b891c66b32163c1a9244ac4e 100644 (file)
@@ -5337,6 +5337,12 @@ DESCR("get progress for all replication origins");
 #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
index b5391673609b7332e15134ab1c30f799eb266188..e0cc69f27ef1e39f182abb0796490262940c02eb 100644 (file)
@@ -305,6 +305,7 @@ extern void GetUserIdAndSecContext(Oid *userid, int *sec_context);
 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);
index 07caf22f962f4a4ae800a87720f708467b007a4a..95f2a848d39d38a5b1b8601c6e463fc8b2a80bc3 100644 (file)
@@ -1120,6 +1120,10 @@ extern Datum set_config_by_name(PG_FUNCTION_ARGS);
 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);
index 72361e82a5f239b9469f7a24346d92a4c37cbb55..fd8e180f8a8e88b18fdb4898d708ae62da4c25b2 100644 (file)
@@ -307,7 +307,7 @@ SELECT * FROM document d FULL OUTER JOIN category c on d.cid = c.cid;
 
 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".
+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;
@@ -2886,11 +2886,45 @@ SELECT * FROM current_check;
 (1 row)
 
 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;
index 1e5b0b9a2c43a522d088417dfa249168b3e5eeab..6206c819cd872adf67db7133dd06ffb3db65641e 100644 (file)
@@ -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,
index f588fa2337738c4167d5f6ed7c542eaaa22185cd..32f10d8649f1e12502e40c7a5486cba4134ece3e 100644 (file)
@@ -1189,11 +1189,26 @@ 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;