mirrors/postgres
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fix incorrect permissions-checking code for extended statistics.

Browse Source 
Commit a4d75c86b improved the extended-stats logic to allow extended
stats to be collected on expressions not just bare Vars.  To apply
such stats, we first verify that the user has permissions to read all
columns used in the stats.  (If not, the query will likely fail at
runtime, but the planner ought not do so.)  That had to get extended
to check permissions of columns appearing within such expressions,
but the code for that was completely wrong: it applied pull_varattnos
to the wrong pointer, leading to "unrecognized node type" failures.
Furthermore, although you couldn't get to this because of that bug,
it failed to account for the attnum offset applied by pull_varattnos.

This escaped recognition so far because the code in question is not
reached when the user has whole-table SELECT privilege (which is the
common case), and because only subexpressions not specially handled
by statext_is_compatible_clause_internal() are at risk.

I think a large part of the reason for this bug is under-documentation
of what statext_is_compatible_clause() is doing and what its arguments
are, so do some work on the comments to try to improve that.

Per bug #17570 from Alexander Kozhemyakin.  Patch by Richard Guo;
comments and other cosmetic improvements by me.  (Thanks also to
Japin Li for diagnosis.)  Back-patch to v14 where the bug came in.

Discussion: https://postgr.es/m/17570-f2f2e0f4bccf0965@postgresql.org
This commit is contained in:
Tom Lane 2022-08-05 12:46:34 -04:00
parent 541f41d4fa
commit 7c6ce0475a
3 changed files with 95 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

126
src/backend/statistics/extended_stats.c
View File

@ -1316,10 +1316,38 @@ choose_best_statistics(List *stats, char requiredkind,
* statext_is_compatible_clause_internal
* Determines if the clause is compatible with MCV lists.
*
* Does the heavy lifting of actually inspecting the clauses for
* statext_is_compatible_clause. It needs to be split like this because
* of recursion. The attnums bitmap is an input/output parameter collecting
* attribute numbers from all compatible clauses (recursively).
* To be compatible, the given clause must be a combination of supported
* clauses built from Vars or sub-expressions (where a sub-expression is
* something that exactly matches an expression found in statistics objects).
* This function recursively examines the clause and extracts any
* sub-expressions that will need to be matched against statistics.
*
* Currently, we only support the following types of clauses:
*
* (a) OpExprs of the form (Var/Expr op Const), or (Const op Var/Expr), where
* the op is one of ("=", "<", ">", ">=", "<=")
*
* (b) (Var/Expr IS [NOT] NULL)
*
* (c) combinations using AND/OR/NOT
*
* (d) ScalarArrayOpExprs of the form (Var/Expr op ANY (array)) or (Var/Expr
* op ALL (array))
*
* In the future, the range of supported clauses may be expanded to more
* complex cases, for example (Var op Var).
*
* Arguments:
* clause: (sub)clause to be inspected (bare clause, not a RestrictInfo)
* relid: rel that all Vars in clause must belong to
* *attnums: input/output parameter collecting attribute numbers of all
* mentioned Vars. Note that we do not offset the attribute numbers,
* so we can't cope with system columns.
* *exprs: input/output parameter collecting primitive subclauses within
* the clause tree
*
* Returns false if there is something we definitively can't handle.
* On true return, we can proceed to match the *exprs against statistics.
*/
static bool
statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause,
@ -1343,10 +1371,14 @@ statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause,
if (var->varlevelsup > 0)
return false;
/* Also skip system attributes (we don't allow stats on those). */
/*
* Also reject system attributes and whole-row Vars (we don't allow
* stats on those).
*/
if (!AttrNumberIsForUserDefinedAttr(var->varattno))
return false;
/* OK, record the attnum for later permissions checks. */
*attnums = bms_add_member(*attnums, var->varattno);
return true;
@ -1501,7 +1533,7 @@ statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause,
foreach(lc, expr->args)
{
/*
* Had we found incompatible clause in the arguments, treat the
* If we find an incompatible clause in the arguments, treat the
* whole clause as incompatible.
*/
if (!statext_is_compatible_clause_internal(root,
@ -1540,27 +1572,28 @@ statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause,
* statext_is_compatible_clause
* Determines if the clause is compatible with MCV lists.
*
* Currently, we only support the following types of clauses:
* See statext_is_compatible_clause_internal, above, for the basic rules.
* This layer deals with RestrictInfo superstructure and applies permissions
* checks to verify that it's okay to examine all mentioned Vars.
*
* (a) OpExprs of the form (Var/Expr op Const), or (Const op Var/Expr), where
* the op is one of ("=", "<", ">", ">=", "<=")
* Arguments:
* clause: clause to be inspected (in RestrictInfo form)
* relid: rel that all Vars in clause must belong to
* *attnums: input/output parameter collecting attribute numbers of all
* mentioned Vars. Note that we do not offset the attribute numbers,
* so we can't cope with system columns.
* *exprs: input/output parameter collecting primitive subclauses within
* the clause tree
*
* (b) (Var/Expr IS [NOT] NULL)
*
* (c) combinations using AND/OR/NOT
*
* (d) ScalarArrayOpExprs of the form (Var/Expr op ANY (array)) or (Var/Expr
* op ALL (array))
*
* In the future, the range of supported clauses may be expanded to more
* complex cases, for example (Var op Var).
* Returns false if there is something we definitively can't handle.
* On true return, we can proceed to match the *exprs against statistics.
*/
static bool
statext_is_compatible_clause(PlannerInfo *root, Node *clause, Index relid,
Bitmapset **attnums, List **exprs)
{
RangeTblEntry *rte = root->simple_rte_array[relid];
RestrictInfo *rinfo = (RestrictInfo *) clause;
RestrictInfo *rinfo;
int clause_relid;
Oid userid;
@ -1589,8 +1622,9 @@ statext_is_compatible_clause(PlannerInfo *root, Node *clause, Index relid,
}
/* Otherwise it must be a RestrictInfo. */
if (!IsA(rinfo, RestrictInfo))
if (!IsA(clause, RestrictInfo))
return false;
rinfo = (RestrictInfo *) clause;
/* Pseudoconstants are not really interesting here. */
if (rinfo->pseudoconstant)
@ -1612,34 +1646,48 @@ statext_is_compatible_clause(PlannerInfo *root, Node *clause, Index relid,
*/
userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
/* Table-level SELECT privilege is sufficient for all columns */
if (pg_class_aclcheck(rte->relid, userid, ACL_SELECT) != ACLCHECK_OK)
{
Bitmapset *clause_attnums = NULL;
int attnum = -1;
/* Don't have table privilege, must check individual columns */
/*
* We have to check per-column privileges. *attnums has the attnums
* for individual Vars we saw, but there may also be Vars within
* subexpressions in *exprs. We can use pull_varattnos() to extract
* those, but there's an impedance mismatch: attnums returned by
* pull_varattnos() are offset by FirstLowInvalidHeapAttributeNumber,
* while attnums within *attnums aren't. Convert *attnums to the
* offset style so we can combine the results.
*/
while ((attnum = bms_next_member(*attnums, attnum)) >= 0)
{
clause_attnums =
bms_add_member(clause_attnums,
attnum - FirstLowInvalidHeapAttributeNumber);
}
/* Now merge attnums from *exprs into clause_attnums */
if (*exprs != NIL)
{
pull_varattnos((Node *) exprs, relid, &clause_attnums);
clause_attnums = bms_add_members(clause_attnums, *attnums);
}
else
clause_attnums = *attnums;
pull_varattnos((Node *) *exprs, relid, &clause_attnums);
if (bms_is_member(InvalidAttrNumber, clause_attnums))
attnum = -1;
while ((attnum = bms_next_member(clause_attnums, attnum)) >= 0)
{
/* Have a whole-row reference, must have access to all columns */
if (pg_attribute_aclcheck_all(rte->relid, userid, ACL_SELECT,
ACLMASK_ALL) != ACLCHECK_OK)
return false;
}
else
{
/* Check the columns referenced by the clause */
int attnum = -1;
/* Undo the offset */
AttrNumber attno = attnum + FirstLowInvalidHeapAttributeNumber;
while ((attnum = bms_next_member(clause_attnums, attnum)) >= 0)
if (attno == InvalidAttrNumber)
{
if (pg_attribute_aclcheck(rte->relid, attnum, userid,
/* Whole-row reference, so must have access to all columns */
if (pg_attribute_aclcheck_all(rte->relid, userid, ACL_SELECT,
ACLMASK_ALL) != ACLCHECK_OK)
return false;
}
else
{
if (pg_attribute_aclcheck(rte->relid, attno, userid,
ACL_SELECT) != ACLCHECK_OK)
return false;
}

4
src/test/regress/expected/stats_ext.out
View File

@ -3183,6 +3183,10 @@ GRANT USAGE ON SCHEMA tststats TO regress_stats_user1;
SET SESSION AUTHORIZATION regress_stats_user1;
SELECT * FROM tststats.priv_test_tbl; -- Permission denied
ERROR: permission denied for table priv_test_tbl
-- Check individual columns if we don't have table privilege
SELECT * FROM tststats.priv_test_tbl
WHERE a = 1 and tststats.priv_test_tbl.* > (1, 1) is not null;
ERROR: permission denied for table priv_test_tbl
-- Attempt to gain access using a leaky operator
CREATE FUNCTION op_leak(int, int) RETURNS bool
AS 'BEGIN RAISE NOTICE ''op_leak => %, %'', $1, $2; RETURN $1 < $2; END'

4
src/test/regress/sql/stats_ext.sql
View File

@ -1600,6 +1600,10 @@ GRANT USAGE ON SCHEMA tststats TO regress_stats_user1;
SET SESSION AUTHORIZATION regress_stats_user1;
SELECT * FROM tststats.priv_test_tbl; -- Permission denied
-- Check individual columns if we don't have table privilege
SELECT * FROM tststats.priv_test_tbl
WHERE a = 1 and tststats.priv_test_tbl.* > (1, 1) is not null;
-- Attempt to gain access using a leaky operator
CREATE FUNCTION op_leak(int, int) RETURNS bool
AS 'BEGIN RAISE NOTICE ''op_leak => %, %'', $1, $2; RETURN $1 < $2; END'