Re: [Patch] Invalid permission check in pg_stats for functional indexes - Mailing list pgsql-hackers

Awhile back I wrote:
> Actually ... maybe we don't need to change the view definition at all,
> but instead just make has_column_privilege() do something different
> for indexes than it does for other relation types.  It's dubious that
> applying that function to an index yields anything meaningful today,
> so we could redefine what it returns without (probably) breaking
> anything.  That would at least give us an option to back-patch, too,
> though the end result might be complex enough that we don't care to
> risk it.

In hopes of resurrecting this thread, here's a draft patch that does
it like that (and also fixes row_security_active(), as otherwise this
probably creates a security hole in pg_stats).

It's definitely not commit quality as-is, for several reasons:

* No regression tests.

* I didn't bother to flesh out logic for looking at the individual
column privileges.  I'm not sure if that's worth doing.  If it is,
we should also look at BuildIndexValueDescription() which is worrying
about largely the same thing, and likewise is punting on the hardest
cases; and selfuncs.c's examine_variable, ditto; and maybe other places.
They should all be able to share one implementation of a check for
whether the user can read all the columns the index depends on.

* There's still the issue of whether any of the other nearby privilege
checking functions need to be synchronized with this, for consistency's
sake.  The pg_stats view doesn't care about the others, but I think
it's a bit weird if has_column_privilege works like this but, say,
has_table_privilege doesn't.

More generally, does anyone want to object to the whole concept of
redefining the has_xxx_privilege functions' behavior when applied
to indexes?

            regards, tom lane

diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index 4ccb3c0..696524a 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -22,6 +22,7 @@
 #include "catalog/pg_auth_members.h"
 #include "catalog/pg_authid.h"
 #include "catalog/pg_class.h"
+#include "catalog/pg_index.h"
 #include "catalog/pg_type.h"
 #include "commands/dbcommands.h"
 #include "commands/proclang.h"
@@ -98,6 +99,8 @@ static AclMode convert_any_priv_string(text *priv_type_text,
 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 int    index_column_privilege_check(Oid indexoid, AttrNumber attnum,
+                                         Oid roleid, AclMode mode);
 static AttrNumber convert_column_name(Oid tableoid, text *column);
 static AclMode convert_column_priv_string(text *priv_type_text);
 static Oid    convert_database_name(text *databasename);
@@ -2458,6 +2461,7 @@ static int
 column_privilege_check(Oid tableoid, AttrNumber attnum,
                        Oid roleid, AclMode mode)
 {
+    char        relkind;
     AclResult    aclresult;
     HeapTuple    attTuple;
     Form_pg_attribute attributeForm;
@@ -2469,16 +2473,28 @@ column_privilege_check(Oid tableoid, AttrNumber attnum,
         return -1;

     /*
-     * First check if we have the privilege at the table level.  We check
-     * existence of the pg_class row before risking calling pg_class_aclcheck.
+     * Check existence of the pg_class row, and find out what relkind it is.
+     * We rely here on get_rel_relkind() returning '\0' if the syscache lookup
+     * fails.
+     */
+    relkind = get_rel_relkind(tableoid);
+    if (relkind == '\0')
+        return -1;                /* table doesn't exist */
+
+    /*
+     * Special case for indexes.
+     */
+    if (relkind == RELKIND_INDEX)
+        return index_column_privilege_check(tableoid, attnum, roleid, mode);
+
+    /*
+     * Now we can check if we have the privilege at the table level.
+     *
      * Note: it might seem there's a race condition against concurrent DROP,
      * but really it's safe because there will be no syscache flush between
-     * here and there.  So if we see the row in the syscache, so will
-     * pg_class_aclcheck.
+     * get_rel_relkind() and pg_class_aclcheck().  So if the former saw the
+     * row in the syscache, so will the latter.
      */
-    if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(tableoid)))
-        return -1;
-
     aclresult = pg_class_aclcheck(tableoid, roleid, mode);

     if (aclresult == ACLCHECK_OK)
@@ -2508,6 +2524,68 @@ column_privilege_check(Oid tableoid, AttrNumber attnum,
 }

 /*
+ * Column privilege check on an index.
+ *
+ * Normally, indexes have no privileges, since one cannot access them via
+ * SQL DML statements.  However, it's useful for us to report that SELECT
+ * privilege exists if the user has SELECT on the underlying table, as that
+ * will allow the pg_stats view to show index expression statistics to
+ * unprivileged users.
+ *
+ * Returns 1 if have the privilege, 0 if not, -1 if dropped column/table.
+ */
+static int
+index_column_privilege_check(Oid indexoid, AttrNumber attnum,
+                             Oid roleid, AclMode mode)
+{
+    HeapTuple    tuple;
+    Form_pg_index indexForm;
+    Oid            tableoid;
+    AclResult    aclresult;
+
+    /* No privilege unless it's SELECT */
+    if (mode != ACL_SELECT)
+        return 0;
+
+    /* Identify the index's underlying table. */
+    tuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexoid));
+    if (!HeapTupleIsValid(tuple))
+        return -1;                /* index was dropped? */
+    indexForm = (Form_pg_index) GETSTRUCT(tuple);
+
+    tableoid = indexForm->indrelid;
+
+    /*
+     * Before we risk calling pg_class_aclcheck, ensure the pg_class row is in
+     * syscache, much as in column_privilege_check.
+     */
+    if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(tableoid)))
+    {
+        ReleaseSysCache(tuple);
+        return -1;
+    }
+
+    /* If we have the privilege for the whole table, we're good. */
+    aclresult = pg_class_aclcheck(tableoid, roleid, mode);
+
+    if (aclresult == ACLCHECK_OK)
+    {
+        ReleaseSysCache(tuple);
+        return 1;
+    }
+
+    /*
+     * In principle, we could examine the pg_index entry to see which table
+     * column or columns the index column depends on, and return success if
+     * the user has column-level privileges for all those columns.  For now,
+     * we just say "no privilege".
+     */
+
+    ReleaseSysCache(tuple);
+    return 0;
+}
+
+/*
  * has_column_privilege_name_name_name
  *        Check user privileges on a column given
  *        name username, text tablename, text colname, and text priv name.
diff --git a/src/backend/utils/misc/rls.c b/src/backend/utils/misc/rls.c
index 4f35911..29c2492 100644
--- a/src/backend/utils/misc/rls.c
+++ b/src/backend/utils/misc/rls.c
@@ -17,6 +17,7 @@
 #include "access/htup.h"
 #include "access/htup_details.h"
 #include "access/transam.h"
+#include "catalog/index.h"
 #include "catalog/namespace.h"
 #include "catalog/pg_class.h"
 #include "miscadmin.h"
@@ -54,6 +55,7 @@ check_enable_rls(Oid relid, Oid checkAsUser, bool noError)
     Oid            user_id = checkAsUser ? checkAsUser : GetUserId();
     HeapTuple    tuple;
     Form_pg_class classform;
+    char        relkind;
     bool        relrowsecurity;
     bool        relforcerowsecurity;
     bool        amowner;
@@ -62,17 +64,32 @@ check_enable_rls(Oid relid, Oid checkAsUser, bool noError)
     if (relid < (Oid) FirstNormalObjectId)
         return RLS_NONE;

-    /* Fetch relation's relrowsecurity and relforcerowsecurity flags */
+    /* Fetch info from the relation's pg_class row */
     tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
     if (!HeapTupleIsValid(tuple))
         return RLS_NONE;
     classform = (Form_pg_class) GETSTRUCT(tuple);

+    relkind = classform->relkind;
     relrowsecurity = classform->relrowsecurity;
     relforcerowsecurity = classform->relforcerowsecurity;

     ReleaseSysCache(tuple);

+    /*
+     * If it's an index, transfer our attention to the underlying table.  This
+     * allows the pg_stats view to do the right thing with statistics for
+     * indexes.
+     */
+    if (relkind == RELKIND_INDEX)
+    {
+        Oid            tableoid = IndexGetRelation(relid, true);
+
+        if (!OidIsValid(tableoid))
+            return RLS_NONE;    /* index was just dropped? */
+        return check_enable_rls(tableoid, user_id, noError);
+    }
+
     /* Nothing to do if the relation does not have RLS */
     if (!relrowsecurity)
         return RLS_NONE;