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.

diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 5d4a3244c1129b87291a52deb5d505237b2425a0..02db1cd20a1ff793cb3305b51cca7897ee4b4e2e 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.256 2010/07/06 19:18:57 momjian Exp $
+ *   $PostgreSQL: pgsql/src/backend/parser/parse_expr.c,v 1.257 2010/07/29 23:16:33 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 5de08dc78c82ed00a2b004eefcd1d555cbfd9f76..8e0f42fc4274453f95fe6436dd3f15f155d25b84 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.224 2010/05/30 18:10:40 tgl Exp $
+ *   $PostgreSQL: pgsql/src/backend/parser/parse_func.c,v 1.225 2010/07/29 23:16:33 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 f40f9af348e4d5954a07bbaa56f23f9084dfb02d..d7316ef9b624b3a9ebc3a21a3ed392387b67201d 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.113 2010/02/26 02:00:52 momjian Exp $
+ *   $PostgreSQL: pgsql/src/backend/parser/parse_oper.c,v 1.114 2010/07/29 23:16:33 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 3800f28759803d78bbafb9c89b528bf689a1500e..8b2fd94bbaa90b0904de6570f7e03b9cbae21329 100644 (file)
--- 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.69 2010/07/29 23:16:33 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 */