From 62ee70331336161cb44733b6c3e0811696d962aa Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 8 Dec 2020 17:50:54 -0500 Subject: [PATCH] Teach contain_leaked_vars that assignment SubscriptingRefs are leaky. array_get_element and array_get_slice qualify as leakproof, since they will silently return NULL for bogus subscripts. But array_set_element and array_set_slice throw errors for such cases, making them clearly not leakproof. contain_leaked_vars was evidently written with only the former case in mind, as it gave the wrong answer for assignment SubscriptingRefs (nee ArrayRefs). This would be a live security bug, were it not that assignment SubscriptingRefs can only occur in INSERT and UPDATE target lists, while we only care about leakproofness for qual expressions; so the wrong answer can't occur in practice. Still, that's a rather shaky answer for a security-related question; and maybe in future somebody will want to ask about leakproofness of a tlist. So it seems wise to fix and even back-patch this correction. (We would need some change here anyway for the upcoming generic-subscripting patch, since extensions might make different tradeoffs about whether to throw errors. Commit 558d77f20 attempted to lay groundwork for that by asking check_functions_in_node whether a SubscriptingRef contains leaky functions; but that idea fails now that the implementation methods of a SubscriptingRef are not SQL-visible functions that could be marked leakproof or not.) Back-patch to 9.6. While 9.5 has the same issue, the code's a bit different. It seems quite unlikely that we'd introduce any actual bug in the short time 9.5 has left to live, so the work/risk/reward balance isn't attractive for changing 9.5. Discussion: https://postgr.es/m/3143742.1607368115@sss.pgh.pa.us --- src/backend/optimizer/util/clauses.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 587d494c34..cb7fa66180 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -1121,7 +1121,6 @@ contain_leaked_vars_walker(Node *node, void *context) case T_ScalarArrayOpExpr: case T_CoerceViaIO: case T_ArrayCoerceExpr: - case T_SubscriptingRef: /* * If node contains a leaky function call, and there's any Var @@ -1133,6 +1132,23 @@ contain_leaked_vars_walker(Node *node, void *context) return true; break; + case T_SubscriptingRef: + { + SubscriptingRef *sbsref = (SubscriptingRef *) node; + + /* + * subscripting assignment is leaky, but subscripted fetches + * are not + */ + if (sbsref->refassgnexpr != NULL) + { + /* Node is leaky, so reject if it contains Vars */ + if (contain_var_clause(node)) + return true; + } + } + break; + case T_RowCompareExpr: { /*