From c902ad9ca950fa1e6577ee3d13d96d4e78e84606 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 29 Jul 2010 23:16:41 +0000 Subject: [PATCH] Improved version of patch to protect pg_get_expr() against misuse: look through join alias Vars to avoid breaking join queries, and move the test to someplace where it will catch more possible ways of calling a function. We still ought to throw away the whole thing in favor of a data-type-based solution, but that's not feasible in the back branches. This needs to be back-patched further than 9.0, but I don't have time to do so today. Committing now so that the fix gets into 9.0beta4. --- src/backend/parser/parse_expr.c | 108 ++++---------------------------------- src/backend/parser/parse_func.c | 113 +++++++++++++++++++++++++++++++++++++++- src/backend/parser/parse_oper.c | 8 ++- src/include/parser/parse_func.h | 4 +- 4 files changed, 133 insertions(+), 100 deletions(-) diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c index 5d4a3244c1..f839a965a7 100644 --- a/src/backend/parser/parse_expr.c +++ b/src/backend/parser/parse_expr.c @@ -8,16 +8,13 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/parser/parse_expr.c,v 1.256 2010/07/06 19:18:57 momjian Exp $ + * $PostgreSQL: pgsql/src/backend/parser/parse_expr.c,v 1.256.2.1 2010/07/29 23:16:41 tgl Exp $ * *------------------------------------------------------------------------- */ #include "postgres.h" -#include "catalog/pg_attrdef.h" -#include "catalog/pg_constraint.h" -#include "catalog/pg_proc.h" #include "catalog/pg_type.h" #include "commands/dbcommands.h" #include "miscadmin.h" @@ -33,7 +30,6 @@ #include "parser/parse_target.h" #include "parser/parse_type.h" #include "utils/builtins.h" -#include "utils/fmgroids.h" #include "utils/lsyscache.h" #include "utils/xml.h" @@ -1214,7 +1210,6 @@ transformFuncCall(ParseState *pstate, FuncCall *fn) { List *targs; ListCell *args; - Node *result; /* Transform the list of arguments ... */ targs = NIL; @@ -1225,97 +1220,16 @@ transformFuncCall(ParseState *pstate, FuncCall *fn) } /* ... and hand off to ParseFuncOrColumn */ - result = ParseFuncOrColumn(pstate, - fn->funcname, - targs, - fn->agg_order, - fn->agg_star, - fn->agg_distinct, - fn->func_variadic, - fn->over, - false, - fn->location); - - /* - * pg_get_expr() is a system function that exposes the expression - * deparsing functionality in ruleutils.c to users. Very handy, but it was - * later realized that the functions in ruleutils.c don't check the input - * rigorously, assuming it to come from system catalogs and to therefore - * be valid. That makes it easy for a user to crash the backend by passing - * a maliciously crafted string representation of an expression to - * pg_get_expr(). - * - * There's a lot of code in ruleutils.c, so it's not feasible to add - * water-proof input checking after the fact. Even if we did it once, it - * would need to be taken into account in any future patches too. - * - * Instead, we restrict pg_rule_expr() to only allow input from system - * catalogs instead. This is a hack, but it's the most robust and easiest - * to backpatch way of plugging the vulnerability. - * - * This is transparent to the typical usage pattern of - * "pg_get_expr(systemcolumn, ...)", but will break "pg_get_expr('foo', - * ...)", even if 'foo' is a valid expression fetched earlier from a - * system catalog. Hopefully there's isn't many clients doing that out - * there. - */ - if (result && IsA(result, FuncExpr) &&!superuser()) - { - FuncExpr *fe = (FuncExpr *) result; - - if (fe->funcid == F_PG_GET_EXPR || fe->funcid == F_PG_GET_EXPR_EXT) - { - Expr *arg = linitial(fe->args); - bool allowed = false; - - /* - * Check that the argument came directly from one of the allowed - * system catalog columns - */ - if (IsA(arg, Var)) - { - Var *var = (Var *) arg; - RangeTblEntry *rte; - - rte = GetRTEByRangeTablePosn(pstate, - var->varno, var->varlevelsup); - - switch (rte->relid) - { - case IndexRelationId: - if (var->varattno == Anum_pg_index_indexprs || - var->varattno == Anum_pg_index_indpred) - allowed = true; - break; - - case AttrDefaultRelationId: - if (var->varattno == Anum_pg_attrdef_adbin) - allowed = true; - break; - - case ProcedureRelationId: - if (var->varattno == Anum_pg_proc_proargdefaults) - allowed = true; - break; - - case ConstraintRelationId: - if (var->varattno == Anum_pg_constraint_conbin) - allowed = true; - break; - - case TypeRelationId: - if (var->varattno == Anum_pg_type_typdefaultbin) - allowed = true; - break; - } - } - if (!allowed) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("argument to pg_get_expr() must come from system catalogs"))); - } - } - return result; + return ParseFuncOrColumn(pstate, + fn->funcname, + targs, + fn->agg_order, + fn->agg_star, + fn->agg_distinct, + fn->func_variadic, + fn->over, + false, + fn->location); } static Node * diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c index 5de08dc78c..7bc865e398 100644 --- a/src/backend/parser/parse_func.c +++ b/src/backend/parser/parse_func.c @@ -8,15 +8,18 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/parser/parse_func.c,v 1.224 2010/05/30 18:10:40 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/parser/parse_func.c,v 1.224.2.1 2010/07/29 23:16:41 tgl Exp $ * *------------------------------------------------------------------------- */ #include "postgres.h" +#include "catalog/pg_attrdef.h" +#include "catalog/pg_constraint.h" #include "catalog/pg_proc.h" #include "catalog/pg_type.h" #include "funcapi.h" +#include "miscadmin.h" #include "nodes/makefuncs.h" #include "nodes/nodeFuncs.h" #include "parser/parse_agg.h" @@ -26,6 +29,7 @@ #include "parser/parse_target.h" #include "parser/parse_type.h" #include "utils/builtins.h" +#include "utils/fmgroids.h" #include "utils/lsyscache.h" #include "utils/syscache.h" @@ -494,6 +498,9 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs, retval = (Node *) wfunc; } + /* Hack to protect pg_get_expr() against misuse */ + check_pg_get_expr_args(pstate, funcid, fargs); + return retval; } @@ -1580,3 +1587,107 @@ LookupAggNameTypeNames(List *aggname, List *argtypes, bool noError) return oid; } + + +/* + * pg_get_expr() is a system function that exposes the expression + * deparsing functionality in ruleutils.c to users. Very handy, but it was + * later realized that the functions in ruleutils.c don't check the input + * rigorously, assuming it to come from system catalogs and to therefore + * be valid. That makes it easy for a user to crash the backend by passing + * a maliciously crafted string representation of an expression to + * pg_get_expr(). + * + * There's a lot of code in ruleutils.c, so it's not feasible to add + * water-proof input checking after the fact. Even if we did it once, it + * would need to be taken into account in any future patches too. + * + * Instead, we restrict pg_rule_expr() to only allow input from system + * catalogs. This is a hack, but it's the most robust and easiest + * to backpatch way of plugging the vulnerability. + * + * This is transparent to the typical usage pattern of + * "pg_get_expr(systemcolumn, ...)", but will break "pg_get_expr('foo', + * ...)", even if 'foo' is a valid expression fetched earlier from a + * system catalog. Hopefully there aren't many clients doing that out there. + */ +void +check_pg_get_expr_args(ParseState *pstate, Oid fnoid, List *args) +{ + bool allowed = false; + Node *arg; + int netlevelsup; + + /* if not being called for pg_get_expr, do nothing */ + if (fnoid != F_PG_GET_EXPR && fnoid != F_PG_GET_EXPR_EXT) + return; + + /* superusers are allowed to call it anyway (dubious) */ + if (superuser()) + return; + + /* + * The first argument must be a Var referencing one of the allowed + * system-catalog columns. It could be a join alias Var, though. + */ + Assert(list_length(args) > 1); + arg = (Node *) linitial(args); + netlevelsup = 0; + +restart: + if (IsA(arg, Var)) + { + Var *var = (Var *) arg; + RangeTblEntry *rte; + + netlevelsup += var->varlevelsup; + rte = GetRTEByRangeTablePosn(pstate, var->varno, netlevelsup); + + if (rte->rtekind == RTE_JOIN) + { + /* Expand join alias reference */ + if (var->varattno > 0 && + var->varattno <= list_length(rte->joinaliasvars)) + { + arg = (Node *) list_nth(rte->joinaliasvars, var->varattno - 1); + goto restart; + } + } + else if (rte->rtekind == RTE_RELATION) + { + switch (rte->relid) + { + case IndexRelationId: + if (var->varattno == Anum_pg_index_indexprs || + var->varattno == Anum_pg_index_indpred) + allowed = true; + break; + + case AttrDefaultRelationId: + if (var->varattno == Anum_pg_attrdef_adbin) + allowed = true; + break; + + case ProcedureRelationId: + if (var->varattno == Anum_pg_proc_proargdefaults) + allowed = true; + break; + + case ConstraintRelationId: + if (var->varattno == Anum_pg_constraint_conbin) + allowed = true; + break; + + case TypeRelationId: + if (var->varattno == Anum_pg_type_typdefaultbin) + allowed = true; + break; + } + } + } + + if (!allowed) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("argument to pg_get_expr() must come from system catalogs"))); +} diff --git a/src/backend/parser/parse_oper.c b/src/backend/parser/parse_oper.c index f40f9af348..15a85d3e53 100644 --- a/src/backend/parser/parse_oper.c +++ b/src/backend/parser/parse_oper.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/parser/parse_oper.c,v 1.113 2010/02/26 02:00:52 momjian Exp $ + * $PostgreSQL: pgsql/src/backend/parser/parse_oper.c,v 1.113.4.1 2010/07/29 23:16:41 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -869,6 +869,9 @@ make_op(ParseState *pstate, List *opname, Node *ltree, Node *rtree, ReleaseSysCache(tup); + /* Hack to protect pg_get_expr() against misuse */ + check_pg_get_expr_args(pstate, result->opfuncid, args); + return (Expr *) result; } @@ -997,6 +1000,9 @@ make_scalar_array_op(ParseState *pstate, List *opname, ReleaseSysCache(tup); + /* Hack to protect pg_get_expr() against misuse */ + check_pg_get_expr_args(pstate, result->opfuncid, args); + return (Expr *) result; } diff --git a/src/include/parser/parse_func.h b/src/include/parser/parse_func.h index 3800f28759..0b68c0e0a1 100644 --- a/src/include/parser/parse_func.h +++ b/src/include/parser/parse_func.h @@ -7,7 +7,7 @@ * Portions Copyright (c) 1996-2010, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/include/parser/parse_func.h,v 1.68 2010/01/02 16:58:07 momjian Exp $ + * $PostgreSQL: pgsql/src/include/parser/parse_func.h,v 1.68.6.1 2010/07/29 23:16:41 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -82,4 +82,6 @@ extern Oid LookupFuncNameTypeNames(List *funcname, List *argtypes, extern Oid LookupAggNameTypeNames(List *aggname, List *argtypes, bool noError); +extern void check_pg_get_expr_args(ParseState *pstate, Oid fnoid, List *args); + #endif /* PARSE_FUNC_H */ -- 2.11.0