From 3945ce1c7e5fe31baa37ca23e6011f0cee22e16a Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Sat, 25 Sep 2021 18:58:33 -0500 Subject: [PATCH 06/15] Refactor parent ACL check selfuncs.c is 8k lines long, and this makes it 30 LOC shorter. --- src/backend/utils/adt/selfuncs.c | 140 ++++++++++++------------------- 1 file changed, 52 insertions(+), 88 deletions(-) diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c index 19b4aaf7eb..54324a71c0 100644 --- a/src/backend/utils/adt/selfuncs.c +++ b/src/backend/utils/adt/selfuncs.c @@ -187,6 +187,8 @@ static char *convert_string_datum(Datum value, Oid typid, Oid collid, bool *failure); static double convert_timevalue_to_scalar(Datum value, Oid typid, bool *failure); +static void recheck_parent_acl(PlannerInfo *root, VariableStatData *vardata, + Oid relid); static void examine_simple_variable(PlannerInfo *root, Var *var, VariableStatData *vardata); static bool get_variable_range(PlannerInfo *root, VariableStatData *vardata, @@ -5152,51 +5154,7 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid, (pg_class_aclcheck(rte->relid, userid, ACL_SELECT) == ACLCHECK_OK); - /* - * If the user doesn't have permissions to - * access an inheritance child relation, check - * the permissions of the table actually - * mentioned in the query, since most likely - * the user does have that permission. Note - * that whole-table select privilege on the - * parent doesn't quite guarantee that the - * user could read all columns of the child. - * But in practice it's unlikely that any - * interesting security violation could result - * from allowing access to the expression - * index's stats, so we allow it anyway. See - * similar code in examine_simple_variable() - * for additional comments. - */ - if (!vardata->acl_ok && - root->append_rel_array != NULL) - { - AppendRelInfo *appinfo; - Index varno = index->rel->relid; - - appinfo = root->append_rel_array[varno]; - while (appinfo && - planner_rt_fetch(appinfo->parent_relid, - root)->rtekind == RTE_RELATION) - { - varno = appinfo->parent_relid; - appinfo = root->append_rel_array[varno]; - } - if (varno != index->rel->relid) - { - /* Repeat access check on this rel */ - rte = planner_rt_fetch(varno, root); - Assert(rte->rtekind == RTE_RELATION); - - userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); - - vardata->acl_ok = - rte->securityQuals == NIL && - (pg_class_aclcheck(rte->relid, - userid, - ACL_SELECT) == ACLCHECK_OK); - } - } + recheck_parent_acl(root, vardata, index->rel->relid); } else { @@ -5287,49 +5245,7 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid, (pg_class_aclcheck(rte->relid, userid, ACL_SELECT) == ACLCHECK_OK); - /* - * If the user doesn't have permissions to access an - * inheritance child relation, check the permissions of - * the table actually mentioned in the query, since most - * likely the user does have that permission. Note that - * whole-table select privilege on the parent doesn't - * quite guarantee that the user could read all columns of - * the child. But in practice it's unlikely that any - * interesting security violation could result from - * allowing access to the expression stats, so we allow it - * anyway. See similar code in examine_simple_variable() - * for additional comments. - */ - if (!vardata->acl_ok && - root->append_rel_array != NULL) - { - AppendRelInfo *appinfo; - Index varno = onerel->relid; - - appinfo = root->append_rel_array[varno]; - while (appinfo && - planner_rt_fetch(appinfo->parent_relid, - root)->rtekind == RTE_RELATION) - { - varno = appinfo->parent_relid; - appinfo = root->append_rel_array[varno]; - } - if (varno != onerel->relid) - { - /* Repeat access check on this rel */ - rte = planner_rt_fetch(varno, root); - Assert(rte->rtekind == RTE_RELATION); - - userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); - - vardata->acl_ok = - rte->securityQuals == NIL && - (pg_class_aclcheck(rte->relid, - userid, - ACL_SELECT) == ACLCHECK_OK); - } - } - + recheck_parent_acl(root, vardata, onerel->relid); break; } @@ -5339,6 +5255,54 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid, } } +/* + * If the user doesn't have permissions to access an inheritance child + * relation, check the permissions of the table actually mentioned in the + * query, since most likely the user does have that permission. Note that + * whole-table select privilege on the parent doesn't quite guarantee that the + * user could read all columns of the child. But in practice it's unlikely + * that any interesting security violation could result from allowing access to + * the expression stats, so we allow it anyway. See similar code in + * examine_simple_variable() for additional comments. + */ +static void +recheck_parent_acl(PlannerInfo *root, VariableStatData *vardata, Oid relid) +{ + RangeTblEntry *rte; + Oid userid; + + if (!vardata->acl_ok && + root->append_rel_array != NULL) + { + AppendRelInfo *appinfo; + Index varno = relid; + + appinfo = root->append_rel_array[varno]; + while (appinfo && + planner_rt_fetch(appinfo->parent_relid, + root)->rtekind == RTE_RELATION) + { + varno = appinfo->parent_relid; + appinfo = root->append_rel_array[varno]; + } + + if (varno != relid) + { + /* Repeat access check on this rel */ + rte = planner_rt_fetch(varno, root); + Assert(rte->rtekind == RTE_RELATION); + + userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); + + vardata->acl_ok = + rte->securityQuals == NIL && + (pg_class_aclcheck(rte->relid, + userid, + ACL_SELECT) == ACLCHECK_OK); + } + } +} + /* * examine_simple_variable * Handle a simple Var for examine_variable -- 2.17.0