Skip to content

[DO NOT MERGE][DUMMY] Fix security checks in selectivity estimation functions. #614

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: BABEL_5_X_DEV__PG_17_X
Choose a base branch
from
Open

[DO NOT MERGE][DUMMY] Fix security checks in selectivity estimation functions. #614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 1 addition & 10 deletions src/backend/executor/execMain.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ static void ExecutePlan(QueryDesc *queryDesc,
uint64 numberTuples,
ScanDirection direction,
DestReceiver *dest);
static bool ExecCheckOneRelPerms(RTEPermissionInfo *perminfo);
static bool ExecCheckPermissionsModified(Oid relOid, Oid userid,
Bitmapset *modifiedCols,
AclMode requiredPerms);
Expand Down Expand Up @@ -638,7 +637,7 @@ ExecCheckPermissions(List *rangeTable, List *rteperminfos,
* ExecCheckOneRelPerms
* Check access permissions for a single relation.
*/
static bool
bool
ExecCheckOneRelPerms(RTEPermissionInfo *perminfo)
{
AclMode requiredPerms;
Expand Down Expand Up @@ -3061,11 +3060,3 @@ EvalPlanQualEnd(EPQState *epqstate)
epqstate->relsubs_blocked = NULL;
}

/*
* ExecCheckOneRelPerms_wrapper - wrapper around ExecCheckOneRelPerms
*/
bool
ExecCheckOneRelPerms_wrapper(RTEPermissionInfo *perminfo)
{
return ExecCheckOneRelPerms(perminfo);
}
33 changes: 33 additions & 0 deletions src/backend/optimizer/plan/planner.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
#include "parser/parse_relation.h"
#include "parser/parsetree.h"
#include "partitioning/partdesc.h"
#include "utils/acl.h"
#include "utils/lsyscache.h"
#include "utils/rel.h"
#include "utils/selfuncs.h"
Expand Down Expand Up @@ -821,6 +822,38 @@ subquery_planner(PlannerGlobal *glob, Query *parse, PlannerInfo *parent_root,
bms_make_singleton(parse->resultRelation);
}

/*
* This would be a convenient time to check access permissions for all
* relations mentioned in the query, since it would be better to fail now,
* before doing any detailed planning. However, for historical reasons,
* we leave this to be done at executor startup.
*
* Note, however, that we do need to check access permissions for any view
* relations mentioned in the query, in order to prevent information being
* leaked by selectivity estimation functions, which only check view owner
* permissions on underlying tables (see all_rows_selectable() and its
* callers). This is a little ugly, because it means that access
* permissions for views will be checked twice, which is another reason
* why it would be better to do all the ACL checks here.
*/
foreach(l, parse->rtable)
{
RangeTblEntry *rte = lfirst_node(RangeTblEntry, l);

if (rte->perminfoindex != 0 &&
rte->relkind == RELKIND_VIEW)
{
RTEPermissionInfo *perminfo;
bool result;

perminfo = getRTEPermissionInfo(parse->rteperminfos, rte);
result = ExecCheckOneRelPerms(perminfo);
if (!result)
aclcheck_error(ACLCHECK_NO_PRIV, OBJECT_VIEW,
get_rel_name(perminfo->relid));
}
}

/*
* Preprocess RowMark information. We need to do this after subquery
* pullup, so that all base relations are present.
Expand Down
108 changes: 44 additions & 64 deletions src/backend/statistics/extended_stats.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1320,14 +1320,17 @@ choose_best_statistics(List *stats, char requiredkind, bool inh,
* so we can't cope with system columns.
* *exprs: input/output parameter collecting primitive subclauses within
* the clause tree
* *leakproof: input/output parameter recording the leakproofness of the
* clause tree. This should be true initially, and will be set to false
* if any operator function used in an OpExpr is not leakproof.
*
* 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,
Index relid, Bitmapset **attnums,
List **exprs)
List **exprs, bool *leakproof)
{
/* Look inside any binary-compatible relabeling (as in examine_variable) */
if (IsA(clause, RelabelType))
Expand Down Expand Up @@ -1362,7 +1365,6 @@ statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause,
/* (Var/Expr op Const) or (Const op Var/Expr) */
if (is_opclause(clause))
{
RangeTblEntry *rte = root->simple_rte_array[relid];
OpExpr *expr = (OpExpr *) clause;
Node *clause_expr;

Expand Down Expand Up @@ -1397,24 +1399,15 @@ statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause,
return false;
}

/*
* If there are any securityQuals on the RTE from security barrier
* views or RLS policies, then the user may not have access to all the
* table's data, and we must check that the operator is leak-proof.
*
* If the operator is leaky, then we must ignore this clause for the
* purposes of estimating with MCV lists, otherwise the operator might
* reveal values from the MCV list that the user doesn't have
* permission to see.
*/
if (rte->securityQuals != NIL &&
!get_func_leakproof(get_opcode(expr->opno)))
return false;
/* Check if the operator is leakproof */
if (*leakproof)
*leakproof = get_func_leakproof(get_opcode(expr->opno));

/* Check (Var op Const) or (Const op Var) clauses by recursing. */
if (IsA(clause_expr, Var))
return statext_is_compatible_clause_internal(root, clause_expr,
relid, attnums, exprs);
relid, attnums,
exprs, leakproof);

/* Otherwise we have (Expr op Const) or (Const op Expr). */
*exprs = lappend(*exprs, clause_expr);
Expand All @@ -1424,7 +1417,6 @@ statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause,
/* Var/Expr IN Array */
if (IsA(clause, ScalarArrayOpExpr))
{
RangeTblEntry *rte = root->simple_rte_array[relid];
ScalarArrayOpExpr *expr = (ScalarArrayOpExpr *) clause;
Node *clause_expr;
bool expronleft;
Expand Down Expand Up @@ -1464,24 +1456,15 @@ statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause,
return false;
}

/*
* If there are any securityQuals on the RTE from security barrier
* views or RLS policies, then the user may not have access to all the
* table's data, and we must check that the operator is leak-proof.
*
* If the operator is leaky, then we must ignore this clause for the
* purposes of estimating with MCV lists, otherwise the operator might
* reveal values from the MCV list that the user doesn't have
* permission to see.
*/
if (rte->securityQuals != NIL &&
!get_func_leakproof(get_opcode(expr->opno)))
return false;
/* Check if the operator is leakproof */
if (*leakproof)
*leakproof = get_func_leakproof(get_opcode(expr->opno));

/* Check Var IN Array clauses by recursing. */
if (IsA(clause_expr, Var))
return statext_is_compatible_clause_internal(root, clause_expr,
relid, attnums, exprs);
relid, attnums,
exprs, leakproof);

/* Otherwise we have Expr IN Array. */
*exprs = lappend(*exprs, clause_expr);
Expand Down Expand Up @@ -1518,7 +1501,8 @@ statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause,
*/
if (!statext_is_compatible_clause_internal(root,
(Node *) lfirst(lc),
relid, attnums, exprs))
relid, attnums, exprs,
leakproof))
return false;
}

Expand All @@ -1532,8 +1516,10 @@ statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause,

/* Check Var IS NULL clauses by recursing. */
if (IsA(nt->arg, Var))
return statext_is_compatible_clause_internal(root, (Node *) (nt->arg),
relid, attnums, exprs);
return statext_is_compatible_clause_internal(root,
(Node *) (nt->arg),
relid, attnums,
exprs, leakproof);

/* Otherwise we have Expr IS NULL. */
*exprs = lappend(*exprs, nt->arg);
Expand Down Expand Up @@ -1572,11 +1558,9 @@ static bool
statext_is_compatible_clause(PlannerInfo *root, Node *clause, Index relid,
Bitmapset **attnums, List **exprs)
{
RangeTblEntry *rte = root->simple_rte_array[relid];
RelOptInfo *rel = root->simple_rel_array[relid];
RestrictInfo *rinfo;
int clause_relid;
Oid userid;
bool leakproof;

/*
* Special-case handling for bare BoolExpr AND clauses, because the
Expand Down Expand Up @@ -1616,18 +1600,31 @@ statext_is_compatible_clause(PlannerInfo *root, Node *clause, Index relid,
clause_relid != relid)
return false;

/* Check the clause and determine what attributes it references. */
/*
* Check the clause, determine what attributes it references, and whether
* it includes any non-leakproof operators.
*/
leakproof = true;
if (!statext_is_compatible_clause_internal(root, (Node *) rinfo->clause,
relid, attnums, exprs))
relid, attnums, exprs,
&leakproof))
return false;

/*
* Check that the user has permission to read all required attributes.
* If the clause includes any non-leakproof operators, check that the user
* has permission to read all required attributes, otherwise the operators
* might reveal values from the MCV list that the user doesn't have
* permission to see. We require all rows to be selectable --- there must
* be no securityQuals from security barrier views or RLS policies. See
* similar code in examine_variable(), examine_simple_variable(), and
* statistic_proc_security_check().
*
* Note that for an inheritance child, the permission checks are performed
* on the inheritance root parent, and whole-table select privilege on the
* parent doesn't guarantee that the user could read all columns of the
* child. Therefore we must check all referenced columns.
*/
userid = OidIsValid(rel->userid) ? rel->userid : GetUserId();

/* Table-level SELECT privilege is sufficient for all columns */
if (pg_class_aclcheck(rte->relid, userid, ACL_SELECT) != ACLCHECK_OK)
if (!leakproof)
{
Bitmapset *clause_attnums = NULL;
int attnum = -1;
Expand All @@ -1652,26 +1649,9 @@ statext_is_compatible_clause(PlannerInfo *root, Node *clause, Index relid,
if (*exprs != NIL)
pull_varattnos((Node *) *exprs, relid, &clause_attnums);

attnum = -1;
while ((attnum = bms_next_member(clause_attnums, attnum)) >= 0)
{
/* Undo the offset */
AttrNumber attno = attnum + FirstLowInvalidHeapAttributeNumber;

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, attno, userid,
ACL_SELECT) != ACLCHECK_OK)
return false;
}
}
/* Must have permission to read all rows from these columns */
if (!all_rows_selectable(root, relid, clause_attnums))
return false;
}

/* If we reach here, the clause is OK */
Expand Down
Loading