Re: [BUGS] Server crash while trying to read expression using pg_get_expr()

On 10/06/10 00:24, Tom Lane wrote:
> I wrote:
>> [ thinks for awhile... ] I wonder whether there is any way of locking
>> down pg_get_expr so that it throws an error if called with anything
>> except a suitable field from one of the system catalogs.
>
> I did a bit of research into this idea. It looks at least somewhat
> feasible:
>
> * All PG versions back to 7.4 will pass the calling expression tree via
> fcinfo->flinfo->fn_expr. Lack of that would be a showstopper, because
> we can't change the FmgrInfo struct in back branches (ABI break). So we
> can get the arguments and determine whether they are Vars.
>
> * To determine which table a Var actually refers to, we must have access
> to the rangetable, and in join cases we also need access to the plan
> tree node containing the Var (since we have to drill down to the plan
> node's inputs to resolve OUTER and INNER references). The rangetable is
> reachable from the PlanState node, so it's sufficient to make that one
> pointer available to functions. The obvious way to handle this is to
> add a field to FmgrInfo, and I would suggest doing it that way as of
> 9.0. In the back branches, we could probably hack it without an ABI
> break by having fn_expr point to some intermediate node type that
> contains both the actual expression tree and the PlanState pointer
> (probably, we'd make it point to FuncExprState instead of directly to
> the FuncExpr, and then add the field to FuncExprState, which is far less
> dangerous than redefining struct FmgrInfo). Now this depends on the
> assumption that no external functions are doing anything with fn_expr
> except passing it to the related fmgr.c routines, which we would teach
> about this convention.
>
> * Once we've got the above data it's a simple matter of adapting the
> Var-interpretation logic used by EXPLAIN in order to find out the table
> OID and column number, if any, represented by the Var. I'd suggest
> adding such functions in fmgr.c to extend the API currently offered by
> get_fn_expr_argtype() and friends. It seems possible that other
> functions besides pg_get_expr might have use for such a capability.
>
>
> What I'm suggesting is definitely not a trivial patch, and I would never
> consider back-patching it if it weren't a security matter. But I think
> it's doable and we'd be fixing the hole with a determinate amount of
> work, whereas trying to verify the validity of pg_get_expr input
> directly would be a never-ending source of more security bugs.

Yeah, seems like it would work.

You could avoid changing the meaning of fn_expr by putting the check in
the parse analysis phase, into transformFuncCall(). That would feel
safer at least for back-branches.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com