From 8ab6a6b4562efcd9f320353d5438fdbe10dbf9c5 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 3 Sep 2010 01:26:52 +0000 Subject: [PATCH] In HEAD only, revert kluge solution for preventing misuse of pg_get_expr(). A data-type-based solution, which is much cleaner and more bulletproof, will follow shortly. It seemed best to make this a separate commit though. --- src/backend/parser/parse_func.c | 113 +------------------------------- src/backend/parser/parse_oper.c | 8 +-- src/backend/tcop/fastpath.c | 13 +--- src/include/parser/parse_func.h | 4 +- 4 files changed, 4 insertions(+), 134 deletions(-) diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c index 88d31a3420..9b869c14c8 100644 --- a/src/backend/parser/parse_func.c +++ b/src/backend/parser/parse_func.c @@ -8,18 +8,15 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/parser/parse_func.c,v 1.226 2010/08/05 21:45:35 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/parser/parse_func.c,v 1.227 2010/09/03 01:26:52 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" @@ -29,7 +26,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/syscache.h" @@ -511,9 +507,6 @@ 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; } @@ -1600,107 +1593,3 @@ 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 d7316ef9b6..f72b692db5 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.114 2010/07/29 23:16:33 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/parser/parse_oper.c,v 1.115 2010/09/03 01:26:52 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -869,9 +869,6 @@ 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; } @@ -1000,9 +997,6 @@ 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/backend/tcop/fastpath.c b/src/backend/tcop/fastpath.c index 5870081fe3..a5c5a30982 100644 --- a/src/backend/tcop/fastpath.c +++ b/src/backend/tcop/fastpath.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/tcop/fastpath.c,v 1.105 2010/07/06 19:18:57 momjian Exp $ + * $PostgreSQL: pgsql/src/backend/tcop/fastpath.c,v 1.106 2010/09/03 01:26:52 tgl Exp $ * * NOTES * This cruft is the server side of PQfn. @@ -29,7 +29,6 @@ #include "tcop/fastpath.h" #include "tcop/tcopprot.h" #include "utils/acl.h" -#include "utils/fmgroids.h" #include "utils/lsyscache.h" #include "utils/snapmgr.h" #include "utils/syscache.h" @@ -348,16 +347,6 @@ HandleFunctionRequest(StringInfo msgBuf) aclcheck_error(aclresult, ACL_KIND_PROC, get_func_name(fid)); - /* - * Restrict access to pg_get_expr(). This reflects the hack in - * transformFuncCall() in parse_expr.c, see comments there for an - * explanation. - */ - if ((fid == F_PG_GET_EXPR || fid == F_PG_GET_EXPR_EXT) && !superuser()) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("argument to pg_get_expr() must come from system catalogs"))); - /* * Prepare function call info block and insert arguments. */ diff --git a/src/include/parser/parse_func.h b/src/include/parser/parse_func.h index 8b2fd94bba..50e69537a3 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.69 2010/07/29 23:16:33 tgl Exp $ + * $PostgreSQL: pgsql/src/include/parser/parse_func.h,v 1.70 2010/09/03 01:26:52 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -82,6 +82,4 @@ 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 */