From e2c84d01afc7312a6a87329f9ac0592f413ac7eb Mon Sep 17 00:00:00 2001 From: Dries Buytaert Date: Sat, 19 Jul 2008 12:31:14 +0000 Subject: - Patch #280631 by pwolanin et al: rethink numeric data-type for db_placeholders(). --- includes/database.inc | 21 ++--- includes/tests/actions.test | 1 + includes/tests/bootstrap.test | 1 + includes/tests/cache.test | 2 + includes/tests/common.test | 1 + includes/tests/database.test | 182 ++++++++++++++++++++++++++++++++++++++++++ includes/tests/registry.test | 1 + 7 files changed, 200 insertions(+), 9 deletions(-) create mode 100644 includes/tests/database.test (limited to 'includes') diff --git a/includes/database.inc b/includes/database.inc index f662fc3d1..e607a525c 100644 --- a/includes/database.inc +++ b/includes/database.inc @@ -216,6 +216,11 @@ function _db_query_callback($match, $init = FALSE) { return (int) array_shift($args); // We don't need db_escape_string as numbers are db-safe case '%s': return db_escape_string(array_shift($args)); + case '%n': + // Numeric values have arbitrary precision, so can't be treated as float. + // is_numeric() allows hex values (0xFF), but they are not valid. + $value = trim(array_shift($args)); + return (is_numeric($value) && !stripos($value, 'x')) ? $value : '0'; case '%%': return '%'; case '%f': @@ -244,7 +249,7 @@ function db_placeholders($arguments, $type = 'int') { /** * Indicates the place holders that should be replaced in _db_query_callback(). */ -define('DB_QUERY_REGEXP', '/(%d|%s|%%|%f|%b)/'); +define('DB_QUERY_REGEXP', '/(%d|%s|%%|%f|%b|%n)/'); /** * Helper function for db_rewrite_sql. @@ -546,16 +551,14 @@ function db_type_placeholder($type) { case 'char': case 'text': case 'datetime': - return '\'%s\''; + return "'%s'"; case 'numeric': - // For 'numeric' values, we use '%s', not '\'%s\'' as with - // string types, because numeric values should not be enclosed - // in quotes in queries (though they can be, at least on mysql - // and pgsql). Numerics should only have [0-9.+-] and - // presumably no db's "escape string" function will mess with - // those characters. - return '%s'; + // Numeric values are arbitrary precision numbers. Syntacically, numerics + // should be specified directly in SQL. However, without single quotes + // the %s placeholder does not protect against non-numeric characters such + // as spaces which would expose us to SQL injection. + return '%n'; case 'serial': case 'int': diff --git a/includes/tests/actions.test b/includes/tests/actions.test index fc829a8e1..68b7a1b98 100644 --- a/includes/tests/actions.test +++ b/includes/tests/actions.test @@ -1,4 +1,5 @@ t('Database placeholders'), + 'description' => t('Make sure that invalid values do not get passed through the %n, %d, or %f placeholders.'), + 'group' => t('System') + ); + } + + function testPlaceholders() { + // First test the numeric type + $valid = array( + '0' => 0, + '1' => 1, + '543.21' => 543.21, + '123.456' => 123.46, + '+0.1e3' => 0.1e3, + ); + $not_valid = array( + '1x' => 0, + '4.4 OR 1=1' => 0, + '9 9' => 0, + '0xff' => 0, + 'XXX' => 0, + '0Xaa' => 0, + 'e' => 0, + '--1' => 0, + 'DROP TABLE' => 0, + '44-66' => 0, + '' => 0, + '.' => 0, + '%88' => 0, + ); + + $schema = array( + 'fields' => array( + 'n' => array( + 'type' => 'numeric', + 'precision' => 5, + 'scale' => 2, + 'not null' => TRUE, + ), + ) + ); + + $ret = array(); + db_create_table($ret, 'test_numeric', $schema); + $insert_query = 'INSERT INTO {test_numeric} (n) VALUES (' . db_type_placeholder('numeric') . ')'; + foreach ($valid as $insert => $select) { + db_query('DELETE FROM {test_numeric}'); + db_query($insert_query, $insert); + $count = db_result(db_query('SELECT COUNT(*) FROM {test_numeric}')); + $this->assertEqual(1, $count, "[numeric] One row ($count) after inserting $insert"); + $test = db_result(db_query('SELECT n FROM {test_numeric}')); + $this->assertEqual($select, $test, "[numeric] Got $select ($test) after inserting valid value $insert"); + } + foreach ($not_valid as $insert => $select) { + db_query('DELETE FROM {test_numeric}'); + db_query($insert_query, $insert); + $count = db_result(db_query('SELECT COUNT(*) FROM {test_numeric}')); + $this->assertEqual(1, $count, "[numeric] One row ($count) after inserting $insert"); + $test = db_result(db_query('SELECT n FROM {test_numeric}')); + $this->assertEqual(0, $test, "[numeric] Got $select ($test) after inserting invalid value $insert"); + } + + // Test ints + $valid = array( + '0' => 0, + '1' => 1, + '543.21' => 543, + '123.456' => 123, + '22' => 22, + ); + $not_valid = array( + '+0.1e3' => 0, + '0xff' => 0, + '0Xaa' => 0, + '1x' => 1, + '4.4 OR 1=1' => 4, + '9 9' => 9, + 'XXX' => 0, + 'e' => 0, + '--1' => 0, + 'DROP TABLE' => 0, + '44-66' => 44, + '' => 0, + '.' => 0, + '%88' => 0, + ); + + $schema = array( + 'fields' => array( + 'n' => array( + 'type' => 'int', + 'not null' => TRUE, + ), + ) + ); + + $ret = array(); + db_create_table($ret, 'test_int', $schema); + $insert_query = 'INSERT INTO {test_int} (n) VALUES (' . db_type_placeholder('int') . ')'; + foreach ($valid as $insert => $select) { + db_query('DELETE FROM {test_int}'); + db_query($insert_query, $insert); + $count = db_result(db_query('SELECT COUNT(*) FROM {test_int}')); + $this->assertEqual(1, $count, "[int] One row ($count) after inserting $insert"); + $test = db_result(db_query('SELECT n FROM {test_int}')); + $this->assertEqual($select, $test, "[int] Got $select ($test) after inserting valid value $insert"); + } + foreach ($not_valid as $insert => $select) { + db_query('DELETE FROM {test_int}'); + db_query($insert_query, $insert); + $count = db_result(db_query('SELECT COUNT(*) FROM {test_int}')); + $this->assertEqual(1, $count, "[int] One row ($count) after inserting $insert"); + $test = db_result(db_query('SELECT n FROM {test_int}')); + $this->assertEqual($select, $test, "[int] Got $select ($test) after inserting invalid value $insert"); + } + + // Test floats + $valid = array( + '0' => 0, + '1' => 1, + '543.21' => 543.21, + '123.456' => 123.456, + '22' => 22, + '+0.1e3' => 100, + ); + $not_valid = array( + '0xff' => 0, + '0Xaa' => 0, + '1x' => 1, + '4.4 OR 1=1' => 4.4, + '9 9' => 9, + 'XXX' => 0, + 'e' => 0, + '--1' => 0, + 'DROP TABLE' => 0, + '44-66' => 44, + '' => 0, + '.' => 0, + '%88' => 0, + ); + + $schema = array( + 'fields' => array( + 'n' => array( + 'type' => 'float', + 'not null' => TRUE, + ), + ) + ); + + $ret = array(); + db_create_table($ret, 'test_float', $schema); + $insert_query = 'INSERT INTO {test_float} (n) VALUES (' . db_type_placeholder('float') . ')'; + foreach ($valid as $insert => $select) { + db_query('DELETE FROM {test_float}'); + db_query($insert_query, $insert); + $count = db_result(db_query('SELECT COUNT(*) FROM {test_float}')); + $this->assertEqual(1, $count, "[float] One row ($count) after inserting $insert"); + $test = db_result(db_query('SELECT n FROM {test_float}')); + $this->assertEqual($select, $test, "[float] Got $select ($test) after inserting valid value $insert"); + } + foreach ($not_valid as $insert => $select) { + db_query('DELETE FROM {test_float}'); + db_query($insert_query, $insert); + $count = db_result(db_query('SELECT COUNT(*) FROM {test_float}')); + $this->assertEqual(1, $count, "[float] One row ($count) after inserting $insert"); + $test = db_result(db_query('SELECT n FROM {test_float}')); + $this->assertEqual($select, $test, "[float] Got $select ($test) after inserting invalid value $insert"); + } + + } +} diff --git a/includes/tests/registry.test b/includes/tests/registry.test index 93fea4009..13bfa9074 100644 --- a/includes/tests/registry.test +++ b/includes/tests/registry.test @@ -1,4 +1,5 @@