Обсуждение: [Patch] Invalid permission check in pg_stats for functional indexes
Hi When using a functional index on a table, we realized that the permission check done in pg_stats was incorrect and thus preventing valid access to the statistics from users. How to reproduce: create table tbl1 (a integer, b integer); insert into tbl1 select x, x % 50 from generate_series(1, 200000) x; create index on tbl1 using btree ((a % (b + 1))); analyze ; create user demo_priv encrypted password 'demo'; revoke ALL on SCHEMA public from PUBLIC ; grant select on tbl1 to demo_priv; grant usage on schema public to demo_priv; And as demo_priv user: select tablename, attname from pg_stats where tablename like 'tbl1%'; Returns: tablename | attname -----------+--------- tbl1 | a tbl1 | b (2 rows) Expected: tablename | attname ---------------+--------- tbl1 | a tbl1 | b tbl1_expr_idx | expr (3 rows) The attached patch fixes this by introducing a second path in privilege check in pg_stats view. I have not written a regression test yet, mainly because I'm not 100% certain where to write it. Given some hints, I would happily add it to this patch. Regards Pierre Ducroquet
Вложения
Hello Pierre, > When using a functional index on a table, we realized that the permission > check done in pg_stats was incorrect and thus preventing valid access to the > statistics from users. > > The attached patch fixes this by introducing a second path in privilege check > in pg_stats view. The patch doesn't apply on the latest HEAD [1]. IIUC, the patch introduces an additional privilege check for the underlying objects involved in the expression/functional index. If the user has 'select' privileges on all of the columns/objects included in the expression/functional index, then it should be visible in pg_stats view. I've applied the patch manually and tested the feature. It works as expected. > I have not written a regression test yet, mainly because I'm not 100% certain > where to write it. Given some hints, I would happily add it to this patch. > Yeah, it'll be good to have some regression tests for the same. I'm also not sure which regression file best suites for these tests. [1] http://cfbot.cputube.org/patch_24_2274.log -- Thanks & Regards, Kuntal Ghosh EnterpriseDB: http://www.enterprisedb.com
On Tuesday, September 3, 2019 12:39:51 PM CEST Kuntal Ghosh wrote: > Hello Pierre, Hello Kuntal > > > When using a functional index on a table, we realized that the permission > > check done in pg_stats was incorrect and thus preventing valid access to > > the statistics from users. > > > > The attached patch fixes this by introducing a second path in privilege > > check in pg_stats view. > > The patch doesn't apply on the latest HEAD [1]. All my apologies for that. I submitted this patch some time ago but forgot to add it to the commit fest. Attached to this mail is a rebased version. > IIUC, the patch introduces an additional privilege check for the > underlying objects involved in the expression/functional index. If the > user has 'select' privileges on all of the columns/objects included in > the expression/functional index, then it should be visible in pg_stats > view. I've applied the patch manually and tested the feature. It works > as expected. Indeed, you understood correctly. I have not digged around to find out the origin of the current situation, but it does not look like an intentional behaviour, more like a small oversight. > > I have not written a regression test yet, mainly because I'm not 100% > > certain where to write it. Given some hints, I would happily add it to > > this patch. > Yeah, it'll be good to have some regression tests for the same. I'm > also not sure which regression file best suites for these tests. Thank you very much for your review Pierre
Вложения
On Wed, Sep 4, 2019 at 12:23 AM Pierre Ducroquet <p.psql@pinaraf.info> wrote: > > All my apologies for that. I submitted this patch some time ago but forgot to > add it to the commit fest. Attached to this mail is a rebased version. > Thank you for the new version of the patch. It looks good to me. Moving the status to ready for committer. -- Thanks & Regards, Kuntal Ghosh EnterpriseDB: http://www.enterprisedb.com
On 2019-Sep-03, Pierre Ducroquet wrote:
> > IIUC, the patch introduces an additional privilege check for the
> > underlying objects involved in the expression/functional index. If the
> > user has 'select' privileges on all of the columns/objects included in
> > the expression/functional index, then it should be visible in pg_stats
> > view. I've applied the patch manually and tested the feature. It works
> > as expected.
>
> Indeed, you understood correctly. I have not digged around to find out the
> origin of the current situation, but it does not look like an intentional
> behaviour, more like a small oversight.
Hmm. This seems to create a large performance drop. I created your
view as pg_stats2 alongside pg_stats, and ran EXPLAIN on both for the
query you posted. Look at the plan with the original query:
55432 13devel 10881=# explain select tablename, attname from pg_stats where tablename like 'tbl1%';
QUERY PLAN
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Subquery Scan on pg_stats (cost=129.79..156.46 rows=1 width=128)
Filter: (pg_stats.tablename ~~ 'tbl1%'::text)
-> Hash Join (cost=129.79..156.39 rows=5 width=401)
Hash Cond: ((s.starelid = c.oid) AND (s.staattnum = a.attnum))
-> Index Only Scan using pg_statistic_relid_att_inh_index on pg_statistic s (cost=0.27..22.60 rows=422
width=6)
-> Hash (cost=114.88..114.88 rows=976 width=138)
-> Hash Join (cost=22.90..114.88 rows=976 width=138)
Hash Cond: (a.attrelid = c.oid)
Join Filter: has_column_privilege(c.oid, a.attnum, 'select'::text)
-> Seq Scan on pg_attribute a (cost=0.00..84.27 rows=2927 width=70)
Filter: (NOT attisdropped)
-> Hash (cost=17.95..17.95 rows=396 width=72)
-> Seq Scan on pg_class c (cost=0.00..17.95 rows=396 width=72)
Filter: ((NOT relrowsecurity) OR (NOT row_security_active(oid)))
(14 filas)
and here's the plan with your modified view:
55432 13devel 10881=# explain select tablename, attname from pg_stats2 where tablename like 'tbl1%';
QUERY PLAN
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Subquery Scan on pg_stats2 (cost=128.72..6861.85 rows=1 width=128)
Filter: (pg_stats2.tablename ~~ 'tbl1%'::text)
-> Nested Loop (cost=128.72..6861.80 rows=4 width=401)
Join Filter: (s.starelid = c.oid)
-> Hash Join (cost=128.45..152.99 rows=16 width=74)
Hash Cond: ((s.starelid = a.attrelid) AND (s.staattnum = a.attnum))
-> Index Only Scan using pg_statistic_relid_att_inh_index on pg_statistic s (cost=0.27..22.60 rows=422
width=6)
-> Hash (cost=84.27..84.27 rows=2927 width=70)
-> Seq Scan on pg_attribute a (cost=0.00..84.27 rows=2927 width=70)
Filter: (NOT attisdropped)
-> Index Scan using pg_class_oid_index on pg_class c (cost=0.27..419.29 rows=1 width=73)
Index Cond: (oid = a.attrelid)
Filter: (((NOT relrowsecurity) OR (NOT row_security_active(oid))) AND ((relkind = 'r'::"char") OR
((relkind= 'i'::"char") AND (NOT (alternatives: SubPlan 1 or hashed SubPlan 2)))) AND (((relkind = 'r'::"char") AND
has_column_privilege(oid,a.attnum, 'select'::text)) OR ((relkind = 'i'::"char") AND (NOT (alternatives: SubPlan 1 or
hashedSubPlan 2)))))
SubPlan 1
-> Seq Scan on pg_depend (cost=0.00..209.48 rows=1 width=0)
Filter: ((refobjsubid > 0) AND (objid = c.oid) AND (NOT has_column_privilege(refobjid,
(refobjsubid)::smallint,'select'::text)))
SubPlan 2
-> Seq Scan on pg_depend pg_depend_1 (cost=0.00..190.42 rows=176 width=4)
Filter: ((refobjsubid > 0) AND (NOT has_column_privilege(refobjid, (refobjsubid)::smallint,
'select'::text)))
(19 filas)
You forgot to add a condition `pg_depend.classid =
'pg_catalog.pg_class'::pg_catalog.regclass` in your subquery (fixing
that probably improves the plan a lot); but more generally I'm not sure
that querying pg_depend is an acceptable way to go about this. I have
to admit I don't see any other way to get a list of columns involved in
an expression, though. Maybe we need to introduce a function that
returns the set of columns involved in an index (which should include
the column in a WHERE clause if any, I suppose.)
What about relkind='m'?
I'm not sure about a writing a test for this. Do we have any tests for
privileges here?
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Hmm. This seems to create a large performance drop.
Yeah. It's also flat out wrong for indexes that depend on whole-row
variables. For that case, we really need to insist that the user
have select privilege on all the table columns, but this won't
accomplish that. It ignores pg_depend entries with refobjsubid = 0,
and there's no good way to expand those even if it didn't.
> You forgot to add a condition `pg_depend.classid =
> 'pg_catalog.pg_class'::pg_catalog.regclass` in your subquery (fixing
> that probably improves the plan a lot); but more generally I'm not sure
> that querying pg_depend is an acceptable way to go about this.
pg_depend isn't ideal for this, I agree. It serves other masters.
> I have
> to admit I don't see any other way to get a list of columns involved in
> an expression, though. Maybe we need to introduce a function that
> returns the set of columns involved in an index (which should include
> the column in a WHERE clause if any, I suppose.)
I agree that some C function that inspects the index definition is
probably needed here. Not sure exactly what it should look like.
We might be well advised to bury the whole business in a function
like "has_index_column_privilege(index_oid, col, priv_type)" rather
than implementing that partly in SQL and partly in C. The performance
would be better, and we'd have more flexibility to fix issues without
forcing new initdb's.
On the other hand, a SQL function that just parses the index definition
and returns relevant column number(s) might be useful for other
purposes, so maybe we should write that alongside this.
> What about relkind='m'?
As coded, this certainly breaks pg_stat for those, and for foreign tables
as well. Likely better to write something like
"case when relkind = 'i' then do-something-for-indexes else old-code end".
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.
I wonder which of the other has_xxx_privilege tests are likewise
in need of rethink for indexes.
> I'm not sure about a writing a test for this. Do we have any tests for
> privileges here?
I don't think we have any meaningful tests for the info-schema views
as such. However, if we redefine the problem as "has_column_privilege
on an index does the wrong thing", then privileges.sql is a natural
home for testing that because it already has test cases for that
function.
regards, tom lane
On Thu, Sep 05, 2019 at 04:56:40PM -0400, Tom Lane wrote: > As coded, this certainly breaks pg_stat for those, and for foreign tables > as well. Likely better to write something like > "case when relkind = 'i' then do-something-for-indexes else old-code end". Pierre, as an author of the patch currently waiting on author for a couple of months now, are you planning to work more on that and address the comments provided? -- Michael
Вложения
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;
Hi Pierre, On 12/26/19 6:46 PM, Tom Lane wrote: > 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). Do you know when you will have a chance to look at this patch? Tom made a suggestion up-thread about where the regression tests could go. Regards, -- -David david@pgmasters.net
> On 25 Mar 2020, at 15:52, David Steele <david@pgmasters.net> wrote: > On 12/26/19 6:46 PM, Tom Lane wrote: >> 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). > > Do you know when you will have a chance to look at this patch? > > Tom made a suggestion up-thread about where the regression tests could go. This patch still hasn't progressed since Tom's draft patch, is anyone still interested in pursuing it or should we close it for now? cheers ./daniel
Daniel Gustafsson <daniel@yesql.se> writes:
> This patch still hasn't progressed since Tom's draft patch, is anyone still
> interested in pursuing it or should we close it for now?
It seems clearly reasonable to me to close the CF item as RWF,
expecting that a new one can be made whenever somebody re-tackles
this problem. The CF list is not a to-do list.
regards, tom lane
> On 5 Jul 2020, at 16:16, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Daniel Gustafsson <daniel@yesql.se> writes: >> This patch still hasn't progressed since Tom's draft patch, is anyone still >> interested in pursuing it or should we close it for now? > > It seems clearly reasonable to me to close the CF item as RWF, > expecting that a new one can be made whenever somebody re-tackles > this problem. Thanks for confirmation, done. > The CF list is not a to-do list. Yes. Multiple +1's. cheers ./daniel