On Sat, Jan 17, 2015 at 11:16 PM, Michael Paquier
<michael.paqu...@gmail.com> wrote:
> Patch is attached. Comments welcome.
So, I have been poking at this code a bit more and as the values of
the parameters are passed as-is to the SQL queries that connectby
generates internally (this is as well mentioned in the documentation
here: http://www.postgresql.org/docs/devel/static/tablefunc.html), you
can do quite fancy things by passing for example values of the type
"foo FROM table; --" or similar. Particularly, by enforcing a query
returning only one column, or NULL values I am even able to crash the
server. The interesting part is that even if compatConnectbyTupleDescs
is enabled for each level, it is still possible to crash the server by
passing for example NULL values casted to the same type, like that
'NULL::text, NULL::text; --'.
The attached patch fixes all those things, I have also enabled
compatConnectbyTupleDescs to run at each level. I'll add it to the
next CF as well to not lose track of it. This behavior has been like
that forever...
--
Michael
From 40ef6b9fa378af588712c0fc98d4e44047f9756c Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@otacoo.com>
Date: Mon, 26 Jan 2015 15:17:23 +0900
Subject: [PATCH] Fix crashes and tuple compatibility checks in connectby()
Coverity has pointed out that compatConnectbyTupleDescs has been dead code
since commit 08c33c4 of 2003. However, after further analysis, it happens
that it is even possible to crash the server as values passed by connectby
are used as-is by the process generating SELECT queries, with for example
values like that '1; --' or that 'column::text, NULL FROM foo; --'. It is
even possible to bypass compatConnectbyTupleDescs by enforcing the types
of the attributes returned as with NULL values as key and parent key
processing did not have checks to see if a value could be NULL. It is already
specified in the documentation that any kind of values can be used by callers
of connectby(), so this commit tightens check around tuple compatibility to
ensure that no callers can crash the server by either enforcing NULL values,
in which case value used is really NULL or when having a query returning only
one column, in which case process simply errors out because a parent key cannot
be defined.
---
contrib/tablefunc/expected/tablefunc.out | 22 +++++++
contrib/tablefunc/sql/tablefunc.sql | 16 +++++
contrib/tablefunc/tablefunc.c | 101 +++++++++++++++++++------------
3 files changed, 101 insertions(+), 38 deletions(-)
diff --git a/contrib/tablefunc/expected/tablefunc.out b/contrib/tablefunc/expected/tablefunc.out
index 0437ecf..7bf7d6d 100644
--- a/contrib/tablefunc/expected/tablefunc.out
+++ b/contrib/tablefunc/expected/tablefunc.out
@@ -376,6 +376,28 @@ SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 4, '~') A
11 | 10 | 4 | 2~5~9~10~11
(8 rows)
+-- should fail as first two columns must have the same type
+SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') AS t(keyid text, parent_keyid int, level int, branch text);
+ERROR: invalid return type
+DETAIL: First two columns must be the same type.
+-- should fail as key field datatype should match return datatype
+SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') AS t(keyid json, parent_keyid json, level int, branch text);
+ERROR: invalid return type
+DETAIL: Failed to cast integer to json
+-- tests for values using custom queries
+-- query with one column - failed
+SELECT * FROM connectby('connectby_int', '1; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
+ERROR: invalid return type
+DETAIL: Return tuple needs at least two columns.
+-- query with two columns first value as NULL - infinite recursion
+SELECT * FROM connectby('connectby_int', 'NULL::text, 1::text; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
+ERROR: infinite recursion detected
+-- query with two columns first value as NULL - infinite recursion
+SELECT * FROM connectby('connectby_int', '1::text, NULL::text; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
+ERROR: infinite recursion detected
+-- query with two columns, both values as NULL - infinite recursion
+SELECT * FROM connectby('connectby_int', 'NULL::text, NULL::text; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
+ERROR: infinite recursion detected
-- test for falsely detected recursion
DROP TABLE connectby_int;
CREATE TABLE connectby_int(keyid int, parent_keyid int);
diff --git a/contrib/tablefunc/sql/tablefunc.sql b/contrib/tablefunc/sql/tablefunc.sql
index bf874f2..835eb28 100644
--- a/contrib/tablefunc/sql/tablefunc.sql
+++ b/contrib/tablefunc/sql/tablefunc.sql
@@ -179,6 +179,22 @@ SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') A
-- infinite recursion failure avoided by depth limit
SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 4, '~') AS t(keyid int, parent_keyid int, level int, branch text);
+-- should fail as first two columns must have the same type
+SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') AS t(keyid text, parent_keyid int, level int, branch text);
+
+-- should fail as key field datatype should match return datatype
+SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') AS t(keyid json, parent_keyid json, level int, branch text);
+
+-- tests for values using custom queries
+-- query with one column - failed
+SELECT * FROM connectby('connectby_int', '1; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
+-- query with two columns first value as NULL - infinite recursion
+SELECT * FROM connectby('connectby_int', 'NULL::text, 1::text; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
+-- query with two columns first value as NULL - infinite recursion
+SELECT * FROM connectby('connectby_int', '1::text, NULL::text; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
+-- query with two columns, both values as NULL - infinite recursion
+SELECT * FROM connectby('connectby_int', 'NULL::text, NULL::text; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
+
-- test for falsely detected recursion
DROP TABLE connectby_int;
CREATE TABLE connectby_int(keyid int, parent_keyid int);
diff --git a/contrib/tablefunc/tablefunc.c b/contrib/tablefunc/tablefunc.c
index 3388fab..2260743 100644
--- a/contrib/tablefunc/tablefunc.c
+++ b/contrib/tablefunc/tablefunc.c
@@ -40,7 +40,10 @@
#include "funcapi.h"
#include "lib/stringinfo.h"
#include "miscadmin.h"
+#include "nodes/makefuncs.h"
+#include "parser/parse_coerce.h"
#include "utils/builtins.h"
+#include "utils/lsyscache.h"
#include "tablefunc.h"
@@ -54,7 +57,7 @@ static Tuplestorestate *get_crosstab_tuplestore(char *sql,
bool randomAccess);
static void validateConnectbyTupleDesc(TupleDesc tupdesc, bool show_branch, bool show_serial);
static bool compatCrosstabTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2);
-static bool compatConnectbyTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2);
+static void compatConnectbyTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2);
static void get_normal_pair(float8 *x1, float8 *x2);
static Tuplestorestate *connectby(char *relname,
char *key_fld,
@@ -1316,22 +1319,11 @@ build_tuplestore_recursively(char *key_fld,
StringInfoData chk_branchstr;
StringInfoData chk_current_key;
- /* First time through, do a little more setup */
- if (level == 0)
- {
- /*
- * Check that return tupdesc is compatible with the one we got
- * from the query, but only at level 0 -- no need to check more
- * than once
- */
-
- if (!compatConnectbyTupleDescs(tupdesc, spi_tupdesc))
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("invalid return type"),
- errdetail("Return and SQL tuple descriptions are " \
- "incompatible.")));
- }
+ /*
+ * Check that return tupdesc is compatible with the one we got
+ * from the query.
+ */
+ compatConnectbyTupleDescs(tupdesc, spi_tupdesc);
initStringInfo(&branchstr);
initStringInfo(&chk_branchstr);
@@ -1346,10 +1338,17 @@ build_tuplestore_recursively(char *key_fld,
/* get the next sql result tuple */
spi_tuple = tuptable->vals[i];
- /* get the current key and parent */
+ /* get the current key */
current_key = SPI_getvalue(spi_tuple, spi_tupdesc, 1);
- appendStringInfo(&chk_current_key, "%s%s%s", branch_delim, current_key, branch_delim);
- current_key_parent = pstrdup(SPI_getvalue(spi_tuple, spi_tupdesc, 2));
+ if (current_key)
+ appendStringInfo(&chk_current_key, "%s%s%s",
+ branch_delim, current_key, branch_delim);
+
+ /*
+ * get the parent key, using NULL if result tuple does not have at
+ * least two columns.
+ */
+ current_key_parent = SPI_getvalue(spi_tuple, spi_tupdesc, 2);
/* get the current level */
sprintf(current_level, "%d", level);
@@ -1359,12 +1358,13 @@ build_tuplestore_recursively(char *key_fld,
elog(ERROR, "infinite recursion detected");
/* OK, extend the branch */
- appendStringInfo(&branchstr, "%s%s", branch_delim, current_key);
+ if (current_key)
+ appendStringInfo(&branchstr, "%s%s", branch_delim, current_key);
current_branch = branchstr.data;
/* build a tuple */
- values[0] = pstrdup(current_key);
- values[1] = current_key_parent;
+ values[0] = current_key ? pstrdup(current_key) : NULL;
+ values[1] = current_key_parent ? pstrdup(current_key_parent) : NULL;
values[2] = current_level;
if (show_branch)
values[3] = current_branch;
@@ -1486,36 +1486,61 @@ validateConnectbyTupleDesc(TupleDesc tupdesc, bool show_branch, bool show_serial
}
/*
- * Check if spi sql tupdesc and return tupdesc are compatible
+ * Check if spi sql tupdesc and return tupdesc are compatible and
+ * if implicit casts can be used.
*/
-static bool
+static void
compatConnectbyTupleDescs(TupleDesc ret_tupdesc, TupleDesc sql_tupdesc)
{
Oid ret_atttypid;
Oid sql_atttypid;
+ int32 ret_typmod;
+ Oid ret_basetype;
+ Oid ret_basecoll;
+ Expr *defval;
- /* check the key_fld types match */
- ret_atttypid = ret_tupdesc->attrs[0]->atttypid;
- sql_atttypid = sql_tupdesc->attrs[0]->atttypid;
- if (ret_atttypid != sql_atttypid)
+ /*
+ * Return result must have at least 2 columns.
+ */
+ if (sql_tupdesc->natts < 2)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("invalid return type"),
- errdetail("SQL key field datatype does " \
- "not match return key field datatype.")));
+ errdetail("Return tuple needs at least two columns.")));
- /* check the parent_key_fld types match */
- ret_atttypid = ret_tupdesc->attrs[1]->atttypid;
- sql_atttypid = sql_tupdesc->attrs[1]->atttypid;
- if (ret_atttypid != sql_atttypid)
+ /*
+ * Attribute type of the two first columns must match (see
+ * validateConnectbyTupleDesc).
+ */
+ if (sql_tupdesc->attrs[0]->atttypid != sql_tupdesc->attrs[1]->atttypid)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("invalid return type"),
- errdetail("SQL parent key field datatype does " \
- "not match return parent key field datatype.")));
+ errdetail("The two first columns of the result query " \
+ "must be identical")));
+
+ /* check compatibility of the the key fields */
+ ret_atttypid = ret_tupdesc->attrs[0]->atttypid;
+ sql_atttypid = sql_tupdesc->attrs[0]->atttypid;
+ ret_basetype = getBaseTypeAndTypmod(ret_atttypid, &ret_typmod);
+ ret_basecoll = get_typcollation(ret_basetype);
+ defval = (Expr *) makeNullConst(ret_basetype, ret_typmod, ret_basecoll);
+ defval = (Expr *) coerce_to_target_type(NULL, (Node *) defval,
+ sql_atttypid,
+ ret_basetype, ret_typmod,
+ COERCION_EXPLICIT,
+ COERCE_EXPLICIT_CAST,
+ -1);
+
+ if (defval == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("invalid return type"),
+ errdetail("Failed to cast %s to %s",
+ format_type_be(sql_atttypid),
+ format_type_be(ret_atttypid))));
/* OK, the two tupdescs are compatible for our purposes */
- return true;
}
/*
--
2.2.2
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
