Skip to content

Commit 1eef280

Browse files
tglsfdctglsfdc
committed
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.
1 parent 8e21bf5 commit 1eef280

File tree

4 files changed

+115
-84
lines changed

4 files changed

+115
-84
lines changed

‎src/backend/parser/parse_expr.c

Lines changed: 1 addition & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -8,16 +8,13 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/parser/parse_expr.c,v 1.179.4.4 2010/06/30 18:11:31 heikki Exp $
11+
* $PostgreSQL: pgsql/src/backend/parser/parse_expr.c,v 1.179.4.5 2010/07/30 17:57:24 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
1515

1616
#include "postgres.h"
1717

18-
#include "catalog/catname.h"
19-
#include "catalog/pg_attrdef.h"
20-
#include "catalog/pg_constraint.h"
2118
#include "catalog/pg_operator.h"
2219
#include "catalog/pg_proc.h"
2320
#include "commands/dbcommands.h"
@@ -35,7 +32,6 @@
3532
#include "parser/parse_relation.h"
3633
#include "parser/parse_type.h"
3734
#include "utils/builtins.h"
38-
#include "utils/fmgroids.h"
3935
#include "utils/lsyscache.h"
4036
#include "utils/syscache.h"
4137

@@ -439,82 +435,6 @@ transformExpr(ParseState *pstate, Node *expr)
439435
fn->agg_star,
440436
fn->agg_distinct,
441437
false);
442-
443-
/*
444-
* pg_get_expr() is a system function that exposes the
445-
* expression deparsing functionality in ruleutils.c to users.
446-
* Very handy, but it was later realized that the functions in
447-
* ruleutils.c don't check the input rigorously, assuming it to
448-
* come from system catalogs and to therefore be valid. That
449-
* makes it easy for a user to crash the backend by passing a
450-
* maliciously crafted string representation of an expression
451-
* to pg_get_expr().
452-
*
453-
* There's a lot of code in ruleutils.c, so it's not feasible
454-
* to add water-proof input checking after the fact. Even if
455-
* we did it once, it would need to be taken into account in
456-
* any future patches too.
457-
*
458-
* Instead, we restrict pg_rule_expr() to only allow input from
459-
* system catalogs instead. This is a hack, but it's the most
460-
* robust and easiest to backpatch way of plugging the
461-
* vulnerability.
462-
*
463-
* This is transparent to the typical usage pattern of
464-
* "pg_get_expr(systemcolumn, ...)", but will break
465-
* "pg_get_expr('foo', ...)", even if 'foo' is a valid
466-
* expression fetched earlier from a system catalog. Hopefully
467-
* there's isn't many clients doing that out there.
468-
*/
469-
if (result && IsA(result, FuncExpr) && !superuser())
470-
{
471-
FuncExpr *fe = (FuncExpr *) result;
472-
if (fe->funcid == F_PG_GET_EXPR ||
473-
fe->funcid == F_PG_GET_EXPR_EXT)
474-
{
475-
Expr *arg = linitial(fe->args);
476-
bool allowed = false;
477-
478-
/*
479-
* Check that the argument came directly from one of the
480-
* allowed system catalog columns
481-
*/
482-
if (IsA(arg, Var))
483-
{
484-
Var *var = (Var *) arg;
485-
RangeTblEntry *rte;
486-
487-
rte = GetRTEByRangeTablePosn(pstate,
488-
var->varno, var->varlevelsup);
489-
490-
if (rte->relid == get_system_catalog_relid(IndexRelationName))
491-
{
492-
if (var->varattno == Anum_pg_index_indexprs ||
493-
var->varattno == Anum_pg_index_indpred)
494-
allowed = true;
495-
}
496-
else if (rte->relid == get_system_catalog_relid(AttrDefaultRelationName))
497-
{
498-
if (var->varattno == Anum_pg_attrdef_adbin)
499-
allowed = true;
500-
}
501-
else if (rte->relid == get_system_catalog_relid(ConstraintRelationName))
502-
{
503-
if (var->varattno == Anum_pg_constraint_conbin)
504-
allowed = true;
505-
}
506-
else if (rte->relid == get_system_catalog_relid(TypeRelationName))
507-
{
508-
if (var->varattno == Anum_pg_type_typdefaultbin)
509-
allowed = true;
510-
}
511-
}
512-
if (!allowed)
513-
ereport(ERROR,
514-
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
515-
errmsg("argument to pg_get_expr() must come from system catalogs")));
516-
}
517-
}
518438
break;
519439
}
520440
case T_SubLink:

‎src/backend/parser/parse_func.c

Lines changed: 104 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,17 +8,20 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/parser/parse_func.c,v 1.175 2004/12/31 22:00:27 pgsql Exp $
11+
* $PostgreSQL: pgsql/src/backend/parser/parse_func.c,v 1.175.4.1 2010/07/30 17:57:24 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
1515
#include "postgres.h"
1616

1717
#include "access/heapam.h"
1818
#include "catalog/catname.h"
19+
#include "catalog/pg_attrdef.h"
20+
#include "catalog/pg_constraint.h"
1921
#include "catalog/pg_inherits.h"
2022
#include "catalog/pg_proc.h"
2123
#include "lib/stringinfo.h"
24+
#include "miscadmin.h"
2225
#include "nodes/makefuncs.h"
2326
#include "parser/parse_agg.h"
2427
#include "parser/parse_coerce.h"
@@ -255,6 +258,9 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
255258
errmsg("aggregates may not return sets")));
256259
}
257260

261+
/* Hack to protect pg_get_expr() against misuse */
262+
check_pg_get_expr_args(pstate, funcid, fargs);
263+
258264
return retval;
259265
}
260266

@@ -1406,3 +1412,100 @@ LookupFuncNameTypeNames(List *funcname, List *argtypes, bool noError)
14061412

14071413
return LookupFuncName(funcname, argcount, argoids, noError);
14081414
}
1415+
1416+
1417+
/*
1418+
* pg_get_expr() is a system function that exposes the expression
1419+
* deparsing functionality in ruleutils.c to users. Very handy, but it was
1420+
* later realized that the functions in ruleutils.c don't check the input
1421+
* rigorously, assuming it to come from system catalogs and to therefore
1422+
* be valid. That makes it easy for a user to crash the backend by passing
1423+
* a maliciously crafted string representation of an expression to
1424+
* pg_get_expr().
1425+
*
1426+
* There's a lot of code in ruleutils.c, so it's not feasible to add
1427+
* water-proof input checking after the fact. Even if we did it once, it
1428+
* would need to be taken into account in any future patches too.
1429+
*
1430+
* Instead, we restrict pg_rule_expr() to only allow input from system
1431+
* catalogs. This is a hack, but it's the most robust and easiest
1432+
* to backpatch way of plugging the vulnerability.
1433+
*
1434+
* This is transparent to the typical usage pattern of
1435+
* "pg_get_expr(systemcolumn, ...)", but will break "pg_get_expr('foo',
1436+
* ...)", even if 'foo' is a valid expression fetched earlier from a
1437+
* system catalog. Hopefully there aren't many clients doing that out there.
1438+
*/
1439+
void
1440+
check_pg_get_expr_args(ParseState *pstate, Oid fnoid, List *args)
1441+
{
1442+
bool allowed = false;
1443+
Node *arg;
1444+
int netlevelsup;
1445+
1446+
/* if not being called for pg_get_expr, do nothing */
1447+
if (fnoid != F_PG_GET_EXPR && fnoid != F_PG_GET_EXPR_EXT)
1448+
return;
1449+
1450+
/* superusers are allowed to call it anyway (dubious) */
1451+
if (superuser())
1452+
return;
1453+
1454+
/*
1455+
* The first argument must be a Var referencing one of the allowed
1456+
* system-catalog columns. It could be a join alias Var, though.
1457+
*/
1458+
Assert(list_length(args) > 1);
1459+
arg = (Node *) linitial(args);
1460+
netlevelsup = 0;
1461+
1462+
restart:
1463+
if (IsA(arg, Var))
1464+
{
1465+
Var *var = (Var *) arg;
1466+
RangeTblEntry *rte;
1467+
1468+
netlevelsup += var->varlevelsup;
1469+
rte = GetRTEByRangeTablePosn(pstate, var->varno, netlevelsup);
1470+
1471+
if (rte->rtekind == RTE_JOIN)
1472+
{
1473+
/* Expand join alias reference */
1474+
if (var->varattno > 0 &&
1475+
var->varattno <= list_length(rte->joinaliasvars))
1476+
{
1477+
arg = (Node *) list_nth(rte->joinaliasvars, var->varattno - 1);
1478+
goto restart;
1479+
}
1480+
}
1481+
else if (rte->rtekind == RTE_RELATION)
1482+
{
1483+
if (rte->relid == get_system_catalog_relid(IndexRelationName))
1484+
{
1485+
if (var->varattno == Anum_pg_index_indexprs ||
1486+
var->varattno == Anum_pg_index_indpred)
1487+
allowed = true;
1488+
}
1489+
else if (rte->relid == get_system_catalog_relid(AttrDefaultRelationName))
1490+
{
1491+
if (var->varattno == Anum_pg_attrdef_adbin)
1492+
allowed = true;
1493+
}
1494+
else if (rte->relid == get_system_catalog_relid(ConstraintRelationName))
1495+
{
1496+
if (var->varattno == Anum_pg_constraint_conbin)
1497+
allowed = true;
1498+
}
1499+
else if (rte->relid == get_system_catalog_relid(TypeRelationName))
1500+
{
1501+
if (var->varattno == Anum_pg_type_typdefaultbin)
1502+
allowed = true;
1503+
}
1504+
}
1505+
}
1506+
1507+
if (!allowed)
1508+
ereport(ERROR,
1509+
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
1510+
errmsg("argument to pg_get_expr() must come from system catalogs")));
1511+
}

‎src/backend/parser/parse_oper.c

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/parser/parse_oper.c,v 1.81 2004/12/31 22:00:27 pgsql Exp $
11+
* $PostgreSQL: pgsql/src/backend/parser/parse_oper.c,v 1.81.4.1 2010/07/30 17:57:24 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -932,6 +932,9 @@ make_scalar_array_op(ParseState *pstate, List *opname,
932932
result->useOr = useOr;
933933
result->args = args;
934934

935+
/* Hack to protect pg_get_expr() against misuse */
936+
check_pg_get_expr_args(pstate, opform->oprcode, args);
937+
935938
ReleaseSysCache(tup);
936939

937940
return (Expr *) result;
@@ -1005,5 +1008,8 @@ make_op_expr(ParseState *pstate, Operator op,
10051008
result->opretset = get_func_retset(opform->oprcode);
10061009
result->args = args;
10071010

1011+
/* Hack to protect pg_get_expr() against misuse */
1012+
check_pg_get_expr_args(pstate, opform->oprcode, args);
1013+
10081014
return (Expr *) result;
10091015
}

‎src/include/parser/parse_func.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group
88
* Portions Copyright (c) 1994, Regents of the University of California
99
*
10-
* $PostgreSQL: pgsql/src/include/parser/parse_func.h,v 1.53 2004/12/31 22:03:38 pgsql Exp $
10+
* $PostgreSQL: pgsql/src/include/parser/parse_func.h,v 1.53.4.1 2010/07/30 17:57:25 tgl Exp $
1111
*
1212
*-------------------------------------------------------------------------
1313
*/
@@ -78,4 +78,6 @@ extern Oid LookupFuncName(List *funcname, int nargs, const Oid *argtypes,
7878
extern Oid LookupFuncNameTypeNames(List *funcname, List *argtypes,
7979
bool noError);
8080

81+
extern void check_pg_get_expr_args(ParseState *pstate, Oid fnoid, List *args);
82+
8183
#endif /* PARSE_FUNC_H */

0 commit comments

Comments
 (0)