diff --git a/includes/bootstrap.inc b/includes/bootstrap.inc index dc569b1..ff25baf 100644 --- a/includes/bootstrap.inc +++ b/includes/bootstrap.inc @@ -1145,6 +1145,31 @@ function _drupal_trigger_error_with_delayed_logging($error_msg, $error_type = E_ } /** + * Invoke trigger_error() using a fatal error that will terminate the request. + * + * Normally, Drupal's error handler does not terminate script execution on + * user-level errors, even if the error is of type E_USER_ERROR. This function + * triggers an error of type E_USER_ERROR that is explicitly forced to be a + * fatal error which terminates script execution. + * + * @param string $error_msg + * The error message to trigger. As with trigger_error() itself, this is + * limited to 1024 bytes; additional characters beyond that will be removed. + * + * @see _drupal_error_handler_real() + */ +function drupal_trigger_fatal_error($error_msg) { + $fatal_error = &drupal_static(__FUNCTION__, FALSE); + $fatal_error = TRUE; + trigger_error($error_msg, E_USER_ERROR); + $fatal_error = FALSE; + // The standard Drupal error handler should have treated this as a fatal + // error and already ended the page request. But in case another error + // handler is being used, terminate execution explicitly here also. + exit; +} + +/** * Writes the file scan cache to the persistent cache. * * This cache stores all files marked as missing or moved after a file scan diff --git a/includes/database/database.inc b/includes/database/database.inc index 6879f69..3cc27cc 100644 --- a/includes/database/database.inc +++ b/includes/database/database.inc @@ -1874,6 +1874,11 @@ class DatabaseTransactionOutOfOrderException extends Exception { } class InvalidMergeQueryException extends Exception {} /** + * Exception thrown if an invalid query condition is specified. + */ +class InvalidQueryConditionOperatorException extends Exception {} + +/** * Exception thrown if an insert query specifies a field twice. * * It is not allowed to specify a field as default and insert field, this diff --git a/includes/database/query.inc b/includes/database/query.inc index 048c8a2..334e805 100644 --- a/includes/database/query.inc +++ b/includes/database/query.inc @@ -871,8 +871,14 @@ class DeleteQuery extends Query implements QueryConditionInterface { $query = $comments . 'DELETE FROM {' . $this->connection->escapeTable($this->table) . '} '; if (count($this->condition)) { - - $this->condition->compile($this->connection, $this); + try { + $this->condition->compile($this->connection, $this); + } + // PHP does not allow exceptions to be thrown in __toString(), so trigger + // a fatal error instead. + catch (InvalidQueryConditionOperatorException $e) { + drupal_trigger_fatal_error($e->getMessage()); + } $query .= "\nWHERE " . $this->condition; } @@ -1204,7 +1210,14 @@ class UpdateQuery extends Query implements QueryConditionInterface { $query = $comments . 'UPDATE {' . $this->connection->escapeTable($this->table) . '} SET ' . implode(', ', $update_fields); if (count($this->condition)) { - $this->condition->compile($this->connection, $this); + try { + $this->condition->compile($this->connection, $this); + } + // PHP does not allow exceptions to be thrown in __toString(), so trigger + // a fatal error instead. + catch (InvalidQueryConditionOperatorException $e) { + drupal_trigger_fatal_error($e->getMessage()); + } // There is an implicit string cast on $this->condition. $query .= "\nWHERE " . $this->condition; } @@ -1789,6 +1802,8 @@ class DatabaseCondition implements QueryConditionInterface, Countable { /** * Implements QueryConditionInterface::compile(). + * + * @throws InvalidQueryConditionOperatorException */ public function compile(DatabaseConnection $connection, QueryPlaceholderInterface $queryPlaceholder) { // Re-compile if this condition changed or if we are compiled against a @@ -1819,6 +1834,12 @@ class DatabaseCondition implements QueryConditionInterface, Countable { $arguments += $condition['field']->arguments(); } else { + // If the operator contains an invalid character, throw an + // exception to protect against SQL injection attempts. + if (stripos($condition['operator'], 'UNION') !== FALSE || strpbrk($condition['operator'], '[-\'"();') !== FALSE) { + throw new InvalidQueryConditionOperatorException('Invalid characters in query operator: ' . $condition['operator']); + } + // For simplicity, we treat all operators as the same data structure. // In the typical degenerate case, this won't get changed. $operator_defaults = array( diff --git a/includes/database/select.inc b/includes/database/select.inc index 8d84460..9fa9603 100644 --- a/includes/database/select.inc +++ b/includes/database/select.inc @@ -1504,7 +1504,14 @@ class SelectQuery extends Query implements SelectQueryInterface { // the query will be executed, it will be recompiled using the proper // placeholder generator anyway. if (!$this->compiled()) { - $this->compile($this->connection, $this); + try { + $this->compile($this->connection, $this); + } + // PHP does not allow exceptions to be thrown in __toString(), so trigger + // a fatal error instead. + catch (InvalidQueryConditionOperatorException $e) { + drupal_trigger_fatal_error($e->getMessage()); + } } // Create a sanitized comment string to prepend to the query. diff --git a/includes/errors.inc b/includes/errors.inc index 3548d1f..4465f8f 100644 --- a/includes/errors.inc +++ b/includes/errors.inc @@ -62,7 +62,8 @@ function _drupal_error_handler_real($error_level, $message, $filename, $line, $c require_once DRUPAL_ROOT . '/includes/common.inc'; } - // We treat recoverable errors as fatal. + // We treat recoverable errors as fatal, and also allow fatal errors to be + // explicitly triggered by drupal_trigger_fatal_error(). _drupal_log_error(array( '%type' => isset($types[$error_level]) ? $severity_msg : 'Unknown error', // The standard PHP error handler considers that the error messages @@ -72,7 +73,7 @@ function _drupal_error_handler_real($error_level, $message, $filename, $line, $c '%file' => $caller['file'], '%line' => $caller['line'], 'severity_level' => $severity_level, - ), $error_level == E_RECOVERABLE_ERROR); + ), $error_level == E_RECOVERABLE_ERROR || drupal_static('drupal_trigger_fatal_error')); } } @@ -269,7 +270,7 @@ function _drupal_log_error($error, $fatal = FALSE) { function _drupal_get_last_caller($backtrace) { // Errors that occur inside PHP internal functions do not generate // information about file and line. Ignore black listed functions. - $blacklist = array('debug', '_drupal_error_handler', '_drupal_exception_handler'); + $blacklist = array('debug', '_drupal_error_handler', '_drupal_exception_handler', 'drupal_trigger_fatal_error'); while (($backtrace && !isset($backtrace[0]['line'])) || (isset($backtrace[1]['function']) && in_array($backtrace[1]['function'], $blacklist))) { array_shift($backtrace); diff --git a/modules/simpletest/tests/database_test.install b/modules/simpletest/tests/database_test.install index 1136115..ece2463 100644 --- a/modules/simpletest/tests/database_test.install +++ b/modules/simpletest/tests/database_test.install @@ -217,5 +217,7 @@ function database_test_schema() { ), ); + $schema['TEST_UPPERCASE'] = $schema['test']; + return $schema; } diff --git a/modules/simpletest/tests/database_test.test b/modules/simpletest/tests/database_test.test index 59d2e5d..3d8b28a 100644 --- a/modules/simpletest/tests/database_test.test +++ b/modules/simpletest/tests/database_test.test @@ -27,6 +27,7 @@ class DatabaseTestCase extends DrupalWebTestCase { $schema['test_one_blob'] = drupal_get_schema('test_one_blob'); $schema['test_two_blobs'] = drupal_get_schema('test_two_blobs'); $schema['test_task'] = drupal_get_schema('test_task'); + $schema['TEST_UPPERCASE'] = drupal_get_schema('TEST_UPPERCASE'); $this->installTables($schema); @@ -3458,6 +3459,67 @@ class DatabaseQueryTestCase extends DatabaseTestCase { $this->assertFalse($result, 'SQL injection attempt did not result in a row being inserted in the database table.'); } + /** + * Tests SQL injection via condition operator. + */ + public function testConditionOperatorArgumentsSQLInjection() { + + $injection = "IS NOT NULL); INSERT INTO {test} (name) VALUES ('test12345678'); -- "; + try { + $result = db_select('test', 't') + ->fields('t') + ->condition('name', 1, $injection) + ->execute(); + $this->fail('Should not be able to attempt SQL injection via condition operator.'); + } + catch (InvalidQueryConditionOperatorException $e) { + $this->pass('SQL injection attempt via condition arguments should result in a database exception.'); + } + + // Test that the insert query that was used in the SQL injection attempt did + // not result in a row being inserted in the database. + $result = db_select('test') + ->condition('name', 'test12345678') + ->countQuery() + ->execute() + ->fetchField(); + $this->assertFalse($result, 'SQL injection attempt did not result in a row being inserted in the database table.'); + + // Attempt SQLi via union query with no unsafe characters. + db_insert('test') + ->fields(array('name' => '123456')) + ->execute(); + + $injection = "= 1 UNION ALL SELECT password FROM user WHERE uid ="; + try { + $result = db_select('test', 't') + ->fields('t', array('name', 'name')) + ->condition('name', 1, $injection) + ->execute(); + $this->fail('Should not be able to attempt SQL injection via operator.'); + } + catch (InvalidQueryConditionOperatorException $e) { + $this->pass('SQL injection attempt via condition arguments should result in a database exception.'); + } + + // Attempt SQLi via union query - uppercase tablename. + db_insert('TEST_UPPERCASE') + ->fields(array('name' => 'secrets')) + ->execute(); + + $injection = "IS NOT NULL) UNION ALL SELECT name FROM {TEST_UPPERCASE} -- "; + try { + $result = db_select('test', 't') + ->fields('t', array('name')) + ->condition('name', 1, $injection) + ->execute(); + $this->fail('Should not be able to attempt SQL injection via operator.'); + } + catch (InvalidQueryConditionOperatorException $e) { + $this->pass('SQL injection attempt via condition arguments should result in a database exception.'); + } + } + } /**