This is an automated email from the ASF dual-hosted git repository.
reshke pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/cloudberry.git
The following commit(s) were added to refs/heads/main by this push:
new 8887fb50cd3 Cherry-pick pg14.5 commit: Fix incorrect
permissions-checking code for extended statistics. (#1550)
8887fb50cd3 is described below
commit 8887fb50cd34a466d66492738bc733bc6ad982ff
Author: reshke <[email protected]>
AuthorDate: Thu Jan 29 00:44:27 2026 +0500
Cherry-pick pg14.5 commit: Fix incorrect permissions-checking code for
extended statistics. (#1550)
Done more clean cherry-pick of CVE fix postgres/postgres@afe38fb
Original commit message follows:
=====
* Fix incorrect permissions-checking code for extended statistics.
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/[email protected]
---------
Co-authored-by: Tom Lane <[email protected]>
---
src/backend/statistics/extended_stats.c | 124 +++++++++++++++-------
src/test/regress/expected/stats_ext.out | 4 +
src/test/regress/expected/stats_ext_optimizer.out | 4 +
src/test/regress/sql/stats_ext.sql | 4 +
4 files changed, 98 insertions(+), 38 deletions(-)
diff --git a/src/backend/statistics/extended_stats.c
b/src/backend/statistics/extended_stats.c
index d3561b779ab..aff0b0db05b 100644
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -1318,10 +1318,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,
@@ -1345,10 +1373,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;
@@ -1503,7 +1535,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,
@@ -1542,27 +1574,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;
@@ -1591,8 +1624,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)
@@ -1614,34 +1648,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 */
- if (*exprs != NIL)
+ /*
+ * 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)
{
- pull_varattnos((Node *) exprs, relid, &clause_attnums);
- clause_attnums = bms_add_members(clause_attnums,
*attnums);
+ clause_attnums =
+ bms_add_member(clause_attnums,
+ attnum -
FirstLowInvalidHeapAttributeNumber);
}
- else
- clause_attnums = *attnums;
- if (bms_is_member(InvalidAttrNumber, clause_attnums))
- {
- /* 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
+ /* Now merge attnums from *exprs into clause_attnums */
+ if (*exprs != NIL)
+ pull_varattnos((Node *) *exprs, relid, &clause_attnums);
+
+ attnum = -1;
+ while ((attnum = bms_next_member(clause_attnums, attnum)) >= 0)
{
- /* 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)
+ {
+ /* 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, attnum,
userid,
+ if (pg_attribute_aclcheck(rte->relid, attno,
userid,
ACL_SELECT) != ACLCHECK_OK)
return false;
}
diff --git a/src/test/regress/expected/stats_ext.out
b/src/test/regress/expected/stats_ext.out
index 3fc90553026..b752abfc4c6 100644
--- a/src/test/regress/expected/stats_ext.out
+++ b/src/test/regress/expected/stats_ext.out
@@ -3196,6 +3196,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'
diff --git a/src/test/regress/expected/stats_ext_optimizer.out
b/src/test/regress/expected/stats_ext_optimizer.out
index d19caa775d1..9f1b78af0b3 100644
--- a/src/test/regress/expected/stats_ext_optimizer.out
+++ b/src/test/regress/expected/stats_ext_optimizer.out
@@ -3231,6 +3231,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'
diff --git a/src/test/regress/sql/stats_ext.sql
b/src/test/regress/sql/stats_ext.sql
index 91edd3a5bba..6840818118d 100644
--- a/src/test/regress/sql/stats_ext.sql
+++ b/src/test/regress/sql/stats_ext.sql
@@ -1615,6 +1615,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'
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
