Commit 596b69fe authored by effulgentsia's avatar effulgentsia

Issue #2489672 by pwolanin, mpdonadio, zniki.ru, bzrudi71, Crell: Limit all DB...

Issue #2489672 by pwolanin, mpdonadio, zniki.ru, bzrudi71, Crell: Limit all DB drivers to executing single statements by checking for delimiter
parent 6e4a62cc
......@@ -239,6 +239,12 @@ public function destroy() {
* further up the call chain can take an appropriate action. To suppress
* that behavior and simply return NULL on failure, set this option to
* FALSE.
* - allow_delimiter_in_query: By default, queries which have the ; delimiter
* any place in them will cause an exception. This reduces the chance of SQL
* injection attacks that terminate the original query and add one or more
* additional queries (such as inserting new user accounts). In rare cases,
* such as creating an SQL function, a ; is needed and can be allowed by
* changing this option to TRUE.
*
* @return array
* An array of default query options.
......@@ -249,6 +255,7 @@ protected function defaultOptions() {
'fetch' => \PDO::FETCH_OBJ,
'return' => Database::RETURN_STATEMENT,
'throw_exception' => TRUE,
'allow_delimiter_in_query' => FALSE,
);
}
......@@ -491,7 +498,7 @@ public function makeComment($comments) {
return '';
// Flatten the array of comments.
$comment = implode('; ', $comments);
$comment = implode('. ', $comments);
// Sanitize the comment string so as to avoid SQL injection attacks.
return '/* ' . $this->filterComment($comment) . ' */ ';
......@@ -516,7 +523,7 @@ public function makeComment($comments) {
*
* Would result in the following SQL statement being generated:
* @code
* "/ * Exploit * / DROP TABLE node; -- * / UPDATE example SET field2=..."
* "/ * Exploit * / DROP TABLE node. -- * / UPDATE example SET field2=..."
* @endcode
*
* Unless the comment is sanitised first, the SQL server would drop the
......@@ -529,7 +536,8 @@ public function makeComment($comments) {
* A sanitized version of the query comment string.
*/
protected function filterComment($comment = '') {
return strtr($comment, ['*' => ' * ']);
// Change semicolons to period to avoid triggering multi-statement check.
return strtr($comment, ['*' => ' * ', ';' => '.']);
}
/**
......@@ -593,6 +601,16 @@ public function query($query, array $args = array(), $options = array()) {
}
else {
$this->expandArguments($query, $args);
// To protect against SQL injection, Drupal only supports executing one
// statement at a time. Thus, the presence of a SQL delimiter (the
// semicolon) is not allowed unless the option is set. Allowing
// semicolons should only be needed for special cases like defining a
// function or stored procedure in SQL. Trim any trailing delimiter to
// minimize false positives.
$query = rtrim($query, "; \t\n\r\0\x0B");
if (strpos($query, ';') !== FALSE && empty($options['allow_delimiter_in_query'])) {
throw new \InvalidArgumentException('; is not supported in SQL strings. Use only one statement at a time.');
}
$stmt = $this->prepareQuery($query);
$stmt->execute($args, $options);
}
......
......@@ -539,7 +539,8 @@ public function prepareComment($comment, $length = NULL) {
// Add table prefixes before truncating.
$comment = Unicode::truncate($this->connection->prefixTables($comment), $length, TRUE, TRUE);
}
// Remove semicolons to avoid triggering multi-statement check.
$comment = strtr($comment, array(';' => '.'));
return $this->connection->quote($comment);
}
......
......@@ -250,18 +250,23 @@ function initializeDatabase() {
// At the same time checking for the existence of the function fixes
// concurrency issues, when both try to update at the same time.
try {
$connection = Database::getConnection();
// Don't use {} around pg_proc table.
if (!db_query("SELECT COUNT(*) FROM pg_proc WHERE proname = 'rand'")->fetchField()) {
db_query('CREATE OR REPLACE FUNCTION "rand"() RETURNS float AS
if (!$connection->query("SELECT COUNT(*) FROM pg_proc WHERE proname = 'rand'")->fetchField()) {
$connection->query('CREATE OR REPLACE FUNCTION "rand"() RETURNS float AS
\'SELECT random();\'
LANGUAGE \'sql\''
LANGUAGE \'sql\'',
[],
[ 'allow_delimiter_in_query' => TRUE ]
);
}
if (!db_query("SELECT COUNT(*) FROM pg_proc WHERE proname = 'substring_index'")->fetchField()) {
db_query('CREATE OR REPLACE FUNCTION "substring_index"(text, text, integer) RETURNS text AS
if (!$connection->query("SELECT COUNT(*) FROM pg_proc WHERE proname = 'substring_index'")->fetchField()) {
$connection->query('CREATE OR REPLACE FUNCTION "substring_index"(text, text, integer) RETURNS text AS
\'SELECT array_to_string((string_to_array($1, $2)) [1:$3], $2);\'
LANGUAGE \'sql\''
LANGUAGE \'sql\'',
[],
[ 'allow_delimiter_in_query' => TRUE ]
);
}
......
......@@ -48,7 +48,7 @@ public function fieldExists($table, $column) {
*/
public function createTableSql($name, $table) {
$sql = array();
$sql[] = "CREATE TABLE {" . $name . "} (\n" . $this->createColumnsSql($name, $table) . "\n);\n";
$sql[] = "CREATE TABLE {" . $name . "} (\n" . $this->createColumnsSql($name, $table) . "\n)\n";
return array_merge($sql, $this->createIndexSql($name, $table));
}
......@@ -60,12 +60,12 @@ protected function createIndexSql($tablename, $schema) {
$info = $this->getPrefixInfo($tablename);
if (!empty($schema['unique keys'])) {
foreach ($schema['unique keys'] as $key => $fields) {
$sql[] = 'CREATE UNIQUE INDEX ' . $info['schema'] . '.' . $info['table'] . '_' . $key . ' ON ' . $info['table'] . ' (' . $this->createKeySql($fields) . "); \n";
$sql[] = 'CREATE UNIQUE INDEX ' . $info['schema'] . '.' . $info['table'] . '_' . $key . ' ON ' . $info['table'] . ' (' . $this->createKeySql($fields) . ")\n";
}
}
if (!empty($schema['indexes'])) {
foreach ($schema['indexes'] as $key => $fields) {
$sql[] = 'CREATE INDEX ' . $info['schema'] . '.' . $info['table'] . '_' . $key . ' ON ' . $info['table'] . ' (' . $this->createKeySql($fields) . "); \n";
$sql[] = 'CREATE INDEX ' . $info['schema'] . '.' . $info['table'] . '_' . $key . ' ON ' . $info['table'] . ' (' . $this->createKeySql($fields) . ")\n";
}
}
return $sql;
......
......@@ -640,6 +640,8 @@ public function fieldNames($fields) {
* The prepared comment.
*/
public function prepareComment($comment, $length = NULL) {
// Remove semicolons to avoid triggering multi-statement check.
$comment = strtr($comment, [';' => '.']);
return $this->connection->quote($comment);
}
......
......@@ -8,6 +8,7 @@
namespace Drupal\system\Tests\Database;
use Drupal\Core\Database\Database;
use Drupal\Core\Database\DatabaseExceptionWrapper;
/**
* Tests of the core database system.
......@@ -122,22 +123,41 @@ function testConnectionOptions() {
* Ensure that you cannot execute multiple statements on phpversion() > 5.5.21 or > 5.6.5.
*/
public function testMultipleStatementsForNewPhp() {
// This just tests mysql, as other PDO integrations don't allow to disable
// This just tests mysql, as other PDO integrations don't allow disabling
// multiple statements.
if (Database::getConnection()->databaseType() !== 'mysql' || !defined('\PDO::MYSQL_ATTR_MULTI_STATEMENTS')) {
return;
}
$db = Database::getConnection('default', 'default');
// Disable the protection at the PHP level.
try {
$db->query('SELECT * FROM {test}; SELECT * FROM {test_people}')->execute();
$this->fail('NO PDO exception thrown for multiple statements.');
$db->query('SELECT * FROM {test}; SELECT * FROM {test_people}',
[],
[ 'allow_delimiter_in_query' => TRUE ]
);
$this->fail('No PDO exception thrown for multiple statements.');
}
catch (\Exception $e) {
catch (DatabaseExceptionWrapper $e) {
$this->pass('PDO exception thrown for multiple statements.');
}
}
/**
* Ensure that you cannot execute multiple statements.
*/
public function testMultipleStatements() {
$db = Database::getConnection('default', 'default');
try {
$db->query('SELECT * FROM {test}; SELECT * FROM {test_people}');
$this->fail('No exception thrown for multiple statements.');
}
catch (\InvalidArgumentException $e) {
$this->pass('Exception thrown for multiple statements.');
}
}
/**
* Test the escapeTable(), escapeField() and escapeAlias() methods with all possible reserved words in PostgreSQL.
*/
......
......@@ -58,7 +58,7 @@ function testVulnerableComment() {
$records = $result->fetchAll();
$query = (string) $query;
$expected = "/* Testing query comments * / SELECT nid FROM {node}; -- */ SELECT test.name AS name, test.age AS age\nFROM \n{test} test";
$expected = "/* Testing query comments * / SELECT nid FROM {node}. -- */ SELECT test.name AS name, test.age AS age\nFROM \n{test} test";
$this->assertEqual(count($records), 4, 'Returned the correct number of rows.');
$this->assertNotIdentical(FALSE, strpos($query, $expected), 'The flattened query contains the sanitised comment string.');
......@@ -81,21 +81,21 @@ function makeCommentsProvider() {
],
// Try and close the comment early.
[
'/* Exploit * / DROP TABLE node; -- */ ',
'/* Exploit * / DROP TABLE node. -- */ ',
['Exploit */ DROP TABLE node; --'],
],
// Variations on comment closing.
[
'/* Exploit * / * / DROP TABLE node; -- */ ',
'/* Exploit * / * / DROP TABLE node. -- */ ',
['Exploit */*/ DROP TABLE node; --'],
],
[
'/* Exploit * * // DROP TABLE node; -- */ ',
'/* Exploit * * // DROP TABLE node. -- */ ',
['Exploit **// DROP TABLE node; --'],
],
// Try closing the comment in the second string which is appended.
[
'/* Exploit * / DROP TABLE node; --; Another try * / DROP TABLE node; -- */ ',
'/* Exploit * / DROP TABLE node. --. Another try * / DROP TABLE node. -- */ ',
['Exploit */ DROP TABLE node; --', 'Another try */ DROP TABLE node; --'],
],
];
......
......@@ -254,11 +254,11 @@ public function providerMakeComments() {
array(''),
),
array(
'/* Exploit * / DROP TABLE node; -- */ ',
'/* Exploit * / DROP TABLE node. -- */ ',
array('Exploit * / DROP TABLE node; --'),
),
array(
'/* Exploit * / DROP TABLE node; --; another comment */ ',
'/* Exploit * / DROP TABLE node. --. another comment */ ',
array('Exploit * / DROP TABLE node; --', 'another comment'),
),
);
......@@ -286,8 +286,8 @@ public function testMakeComments($expected, $comment_array) {
public function providerFilterComments() {
return array(
array('', ''),
array('Exploit * / DROP TABLE node; --', 'Exploit * / DROP TABLE node; --'),
array('Exploit * / DROP TABLE node; --', 'Exploit */ DROP TABLE node; --'),
array('Exploit * / DROP TABLE node. --', 'Exploit * / DROP TABLE node; --'),
array('Exploit * / DROP TABLE node. --', 'Exploit */ DROP TABLE node; --'),
);
}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment