[BUGS] Server crash while trying to read expressionusing pg_get_expr()
in [PgSql]
|
Prev: pgstatindex still throws ERROR: value "3220078592" is out of range for type integer
Next: [HACKERS] Performance of Bit String
From: Heikki Linnakangas on 9 Jun 2010 10:38 On 09/06/10 17:34, Tom Lane wrote: > Heikki Linnakangas<heikki.linnakangas(a)enterprisedb.com> writes: >> We have two options: > >> 1. Make pg_get_expr() handle arbitrary (possibly even malicious) input >> gracefully. > >> 2. Restrict pg_get_expr() to superusers only. > > I think #1 is a fool's errand. There is far too much structure to a > node tree that is outside the scope of what readfuncs.c is capable of > understanding. That's why I said that ruleutils.c will need to understand and complain about the rest. Are you thinking we should retrict pg_get_expr() to superusers then? -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Heikki Linnakangas on 9 Jun 2010 10:38 On 09/06/10 17:34, Tom Lane wrote: > Heikki Linnakangas<heikki.linnakangas(a)enterprisedb.com> writes: >> We have two options: > >> 1. Make pg_get_expr() handle arbitrary (possibly even malicious) input >> gracefully. > >> 2. Restrict pg_get_expr() to superusers only. > > I think #1 is a fool's errand. There is far too much structure to a > node tree that is outside the scope of what readfuncs.c is capable of > understanding. That's why I said that ruleutils.c will need to understand and complain about the rest. Are you thinking we should restrict pg_get_expr() to superusers then? -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Kris Jurka on 9 Jun 2010 13:28 On Wed, 9 Jun 2010, Heikki Linnakangas wrote: > Are you thinking we should retrict pg_get_expr() to superusers then? > That seems like it will cause problems for both pg_dump and drivers which want to return metadata as pg_get_expr has been the recommended way of fetching this information. The JDBC driver uses pg_get_expr to decode both pg_attrdef.adbin and pg_index.indpred. Kris Jurka -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Heikki Linnakangas on 15 Jun 2010 03:31 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Heikki Linnakangas on 21 Jun 2010 19:50
On 15/06/10 10:31, Heikki Linnakangas wrote: > 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. Here's a patch using that approach. I grepped through PostgreSQL and pgadmin source code to find the system columns where valid node-strings are stored: pg_index.indexprs pg_index.indprep pg_attrdef.adbin pg_proc.proargdefaults pg_constraint.conbin Am I missing anything? -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com |