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.

Completion of back-port of my patch of yesterday.

diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 8884afc11aa164d8f255637d760ab85628cc7864..9f68d262dc4b0d019150d575278ba74eb8d031e5 100644 (file)
--- 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.241.2.5 2010/06/30 18:10:37 heikki Exp $
+ *       $PostgreSQL: pgsql/src/backend/parser/parse_expr.c,v 1.241.2.6 2010/07/30 17:56:59 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"
 
@@ -1103,7 +1099,6 @@ transformFuncCall(ParseState *pstate, FuncCall *fn)
 {
        List       *targs;
        ListCell   *args;
-       Node       *result;
 
        /* Transform the list of arguments ... */
        targs = NIL;
@@ -1114,7 +1109,7 @@ transformFuncCall(ParseState *pstate, FuncCall *fn)
        }
 
        /* ... and hand off to ParseFuncOrColumn */
-       result = ParseFuncOrColumn(pstate,
+       return ParseFuncOrColumn(pstate,
                                                         fn->funcname,
                                                         targs,
                                                         fn->agg_star,
@@ -1123,86 +1118,6 @@ transformFuncCall(ParseState *pstate, FuncCall *fn)
                                                         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;
 }
 
 static Node *

diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index fd0706e96086b5237016f3a0a3580a8fb2f9c3fd..8e7ccae12a4687f5884e8fadab7eb028758a1568 100644 (file)
--- 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.216 2009/06/11 14:49:00 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/parser/parse_func.c,v 1.216.2.1 2010/07/30 17:56:59 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"
 
@@ -412,6 +416,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;
 }
 
@@ -1430,3 +1437,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 16ed3a11ed5e8c4260f3a23e52c7a717ebd937d8..198a2bc4e9a5ba6262537a91a52512af674bc09b 100644 (file)
--- 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.109 2009/06/13 15:42:09 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/parser/parse_oper.c,v 1.109.2.1 2010/07/30 17:56:59 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -880,6 +880,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;
 }
 
@@ -1008,6 +1011,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 7905f96e86c8e3b90bf2f592b490e94196493f2c..0b0b81dd12aec3a1b3f3531377c742b89e21404f 100644 (file)
--- a/src/include/parser/parse_func.h
+++ b/src/include/parser/parse_func.h
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2009, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/parser/parse_func.h,v 1.65 2009/05/12 00:56:05 tgl Exp $
+ * $PostgreSQL: pgsql/src/include/parser/parse_func.h,v 1.65.2.1 2010/07/30 17:56:59 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -80,4 +80,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 */