- Patch #280631 by pwolanin et al: rethink numeric data-type for db_placeholders().

7 files changed, 200 insertions, 9 deletions

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 @@
 <?php
+// $Id$
 
 class ActionsConfigurationTestCase extends DrupalWebTestCase {
   /**
diff --git a/includes/tests/bootstrap.test b/includes/tests/bootstrap.test
index 3bb197aae..de833f117 100644
--- a/includes/tests/bootstrap.test
+++ b/includes/tests/bootstrap.test
@@ -1,4 +1,5 @@
 <?php
+// $Id$
 
 class BootstrapIPAddressTestCase extends DrupalWebTestCase {
 
diff --git a/includes/tests/cache.test b/includes/tests/cache.test
index a6ff1e0b7..78b7886d8 100644
--- a/includes/tests/cache.test
+++ b/includes/tests/cache.test
@@ -1,4 +1,6 @@
 <?php
+// $Id$
+
 class CacheTestCase extends DrupalWebTestCase {
   protected $default_table = 'cache';
   protected $default_cid = 'test_temporary';
diff --git a/includes/tests/common.test b/includes/tests/common.test
index d3f907608..7140128a9 100644
--- a/includes/tests/common.test
+++ b/includes/tests/common.test
@@ -1,4 +1,5 @@
 <?php
+// $Id$
 
 class CommonFormatSizeTestCase extends DrupalWebTestCase {
 
diff --git a/includes/tests/database.test b/includes/tests/database.test
new file mode 100644
index 000000000..83bb3a123
--- /dev/null
+++ b/includes/tests/database.test
@@ -0,0 +1,182 @@
+<?php
+// $Id$
+
+class DatabaseSecurityTestCase extends DrupalWebTestCase {
+
+  /**
+   * Implementation of getInfo().
+   */
+  function getInfo() {
+    return array(
+      'name' => 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 @@
 <?php
+// $Id$
 
 class RegistryParseFileTestCase extends DrupalWebTestCase {