10000 Improved version of patch to protect pg_get_expr() against misuse: · liuhb86/postgres@10c0565 · GitHub
[go: up one dir, main page]
Skip to content

Commit 10c0565

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 678e0d9 commit 10c0565

File tree

4 files changed

+135
-93
lines changed

4 files changed

+135
-93
lines changed

‎src/backend/parser/parse_expr.c

Lines changed: 1 addition & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -8,16 +8,13 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $Header: /cvsroot/pgsql/src/backend/parser/parse_expr.c,v 1.163.2.5 2010/06/30 18:11:43 heikki Exp $
11+
* $Header: /cvsroot/pgsql/src/backend/parser/parse_expr.c,v 1.163.2.6 2010/07/30 17:57:32 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"
@@ -34,9 +31,7 @@
3431
#include "parser/parse_oper.h"
3532
#include "parser/parse_relation.h"
3633
#include "parser/parse_type.h"
37-
#include "parser/parsetree.h"
3834
#include "utils/builtins.h"
39-
#include "utils/fmgroids.h"
4035
#include "utils/lsyscache.h"
4136
#include "utils/syscache.h"
4237

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

‎src/backend/parser/parse_func.c

Lines changed: 124 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,18 +8,22 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $Header: /cvsroot/pgsql/src/backend/parser/parse_func.c,v 1.161 2003/09/29 00:05:25 petere Exp $
11+
* $Header: /cvsroot/pgsql/src/backend/parser/parse_func.c,v 1.161.2.1 2010/07/30 17:57:32 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"
26+
#include "parser/parsetree.h"
2327
#include "parser/parse_agg.h"
2428
#include "parser/parse_coerce.h"
2529
#include "parser/parse_expr.h"
@@ -371,6 +375,9 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
371375
errmsg("aggregates may not return sets")));
372376
}
373377

378+
/* Hack to protect pg_get_expr() against misuse */
379+
check_pg_get_expr_args(pstate, funcid, fargs);
380+
374381
return retval;
375382
}
376383

@@ -1531,3 +1538,119 @@ LookupFuncNameTypeNames(List *funcname, List *argtypes, bool noError)
15311538

15321539
return LookupFuncName(funcname, argcount, argoids, noError);
15331540
}
1541+
1542+
1543+
/*
1544+
* Given an RT index and nesting depth, find the corresponding RTE.
1545+
* This is the inverse of RTERangeTablePosn.
1546+
*/
1547+
static RangeTblEntry *
1548+
GetRTEByRangeTablePosn(ParseState *pstate,
1549+
int varno,
1550+
int sublevels_up)
1551+
{
1552+
while (sublevels_up-- > 0)
1553+
{
1554+
pstate = pstate->parentParseState;
1555+
Assert(pstate != NULL);
1556+
}
1557+
Assert(varno > 0 && varno <= length(pstate->p_rtable));
1558+
return rt_fetch(varno, pstate->p_rtable);
1559+
}
1560+
1561+
1562+
/*
1563+
* pg_get_expr() is a system function that exposes the expression
1564+
* deparsing functionality in ruleutils.c to users. Very handy, but it was
1565+
* later realized that the functions in ruleutils.c don't check the input
1566+
* rigorously, assuming it to come from system catalogs and to therefore
1567+
* be valid. That makes it easy for a user to crash the backend by passing
1568+
* a maliciously crafted string representation of an expression to
1569+
* pg_get_expr().
1570+
*
1571+
* There's a lot of code in ruleutils.c, so it's not feasible to add
1572+
* water-proof input checking after the fact. Even if we did it once, it
1573+
* would need to be taken into account in any future patches too.
1574+
*
1575+
* Instead, we restrict pg_rule_expr() to only allow input from system
1576+
* catalogs. This is a hack, but it's the most robust and easiest
1577+
* to backpatch way of plugging the vulnerability.
1578+
*
1579+
* This is transparent to the typical usage pattern of
1580+
* "pg_get_expr(systemcolumn, ...)", but will break "pg_get_expr('foo',
1581+
* ...)", even if 'foo' is a valid expression fetched earlier from a
1582+
* system catalog. Hopefully there aren't many clients doing that out there.
1583+
*/
1584+
void
1585+
check_pg_get_expr_args(ParseState *pstate, Oid fnoid, List *args)
1586+
{
1587+
bool allowed = false;
1588+
Node *arg;
1589+
int netlevelsup;
1590+
1591+
/* if not being called for pg_get_expr, do nothing */
1592+
if (fnoid != F_PG_GET_EXPR && fnoid != F_PG_GET_EXPR_EXT)
1593+
return;
1594+
1595+
/* superusers are allowed to call it anyway (dubious) */
1596+
if (superuser())
1597+
return;
1598+
1599+
/*
1600+
* The first argument must be a Var referencing one of the allowed
1601+
* system-catalog columns. It could be a join alias Var, though.
1602+
*/
1603+
Assert(args != NIL);
1604+
arg = (Node *) lfirst(args);
1605+
netlevelsup = 0;
1606+
1607+
restart:
1608+
if (IsA(arg, Var))
1609+
{
1610+
Var *var = (Var *) arg;
1611+
RangeTblEntry *rte;
1612+
1613+
netlevelsup += var->varlevelsup;
1614+
rte = GetRTEByRangeTablePosn(pstate, var->varno, netlevelsup);
1615+
1616+
if (rte->rtekind == RTE_JOIN)
1617+
{
1618+
/* Expand join alias reference */
1619+
if (var->varattno > 0 &&
1620+
var->varattno <= length(rte->joinaliasvars))
1621+
{
1622+
arg = (Node *) nth(var->varattno - 1, rte->joinaliasvars);
1623+
goto restart;
1624+
}
1625+
}
1626+
else if (rte->rtekind == RTE_RELATION)
1627+
{
1628+
if (rte->relid == get_system_catalog_relid(IndexRelationName))
1629+
{
1630+
if (var->varattno == Anum_pg_index_indexprs ||
1631+
var->varattno == Anum_pg_index_indpred)
1632+
allowed = true;
1633+
}
1634+
else if (rte->relid == get_system_catalog_relid(AttrDefaultRelationName))
1635+
{
1636+
if (var->varattno == Anum_pg_attrdef_adbin)
1637+
allowed = true;
1638+
}
1639+
else if (rte->relid == get_system_catalog_relid(ConstraintRelationName))
1640+
{
1641+
if (var->varattno == Anum_pg_constraint_conbin)
1642+
allowed = true;
1643+
}
1644+
else if (rte->relid == get_system_catalog_relid(TypeRelationName))
1645+
{
1646+
if (var->varattno == Anum_pg_type_typdefaultbin)
1647+
allowed = true;
1648+
}
1649+
}
1650+
}
1651+
1652+
if (!allowed)
1653+
ereport(ERROR,
1654+
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
1655+
errmsg("argument to pg_get_expr() must come from system catalogs")));
1656+
}

‎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-
* $Header: /cvsroot/pgsql/src/backend/parser/parse_oper.c,v 1.76 2003/10/06 20:09:47 tgl Exp $
11+
* $Header: /cvsroot/pgsql/src/backend/parser/parse_oper.c,v 1.76.2.1 2010/07/30 17:57:32 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-2003, PostgreSQL Global Development Group
88
* Portions Copyright (c) 1994, Regents of the University of California
99
*
10-
* $Id: parse_func.h,v 1.50 2003/08/04 02:40:14 momjian Exp $
10+
* $Id: parse_func.h,v 1.50.4.1 2010/07/30 17:57:32 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)
0