Split SQL batches correctly around dollar quotes and comments
splitStatements() only knew 'strings' and "identifiers", so a DO block or function body ($$ ... ; ... $$) was cut at its inner semicolons and failed, and a quote inside a comment (-- don't) opened a string that never closed. It now also skips dollar-quoted bodies, E'...' strings with backslash escapes, and comments. It moves to SqlGuard so it is tested with the other checks. OccController::run() now catches Throwable, so an Error thrown by a command is logged and returned like an Exception instead of ending in an HTTP 500. Not changed on purpose: showing rows for statements with RETURNING or data-modifying CTEs, and failure handling in the Makefile release targets. Both came up in the same review but are unrelated to the security fixes in this release.
This commit is contained in:
@@ -41,59 +41,6 @@ class DbController extends Controller
|
||||
$this->logger = OC::$server->get(LoggerInterface::class);
|
||||
}
|
||||
|
||||
/**
|
||||
* Splits a batch of queries on ";" while respecting single and double
|
||||
* quotes (strings and Postgres-escaped identifiers), so that a
|
||||
* semicolon inside a 'string' or "identifier" (including '' / "" as
|
||||
* an escaped quote) doesn't break up the split.
|
||||
*/
|
||||
private function splitStatements($sql)
|
||||
{
|
||||
$statements = [];
|
||||
$current = '';
|
||||
$len = strlen($sql);
|
||||
$quoteChar = null;
|
||||
|
||||
for ($i = 0; $i < $len; $i++) {
|
||||
$ch = $sql[$i];
|
||||
|
||||
if ($quoteChar !== null) {
|
||||
if ($ch === $quoteChar) {
|
||||
if ($i + 1 < $len && $sql[$i + 1] === $quoteChar) {
|
||||
$current .= $quoteChar . $quoteChar;
|
||||
$i++;
|
||||
continue;
|
||||
}
|
||||
$quoteChar = null;
|
||||
}
|
||||
$current .= $ch;
|
||||
continue;
|
||||
}
|
||||
|
||||
if ($ch === "'" || $ch === '"') {
|
||||
$quoteChar = $ch;
|
||||
$current .= $ch;
|
||||
continue;
|
||||
}
|
||||
|
||||
if ($ch === ';') {
|
||||
$statements[] = trim($current);
|
||||
$current = '';
|
||||
continue;
|
||||
}
|
||||
|
||||
$current .= $ch;
|
||||
}
|
||||
|
||||
if (trim($current) !== '') {
|
||||
$statements[] = trim($current);
|
||||
}
|
||||
|
||||
return array_values(array_filter($statements, function ($s) {
|
||||
return $s !== '';
|
||||
}));
|
||||
}
|
||||
|
||||
public function query()
|
||||
{
|
||||
// Deliberate defense-in-depth (see the equivalent check in
|
||||
@@ -138,7 +85,7 @@ class DbController extends Controller
|
||||
// All queries run sequentially on the same DB connection (within a
|
||||
// single HTTP request), so SET retains its value for
|
||||
// current_setting() in the following queries of the same batch.
|
||||
$queries = $this->splitStatements($sql);
|
||||
$queries = SqlGuard::splitStatements($sql);
|
||||
|
||||
// Access to the server's filesystem / running programs via SQL is
|
||||
// blocked entirely — this isn't about data loss (like DELETE), it's
|
||||
|
||||
@@ -91,7 +91,7 @@ class OccController extends Controller
|
||||
try {
|
||||
$this->application->run($input, $this->output);
|
||||
return $this->output->fetch();
|
||||
} catch (Exception $ex) {
|
||||
} catch (\Throwable $ex) {
|
||||
$this->logger->error($ex->getMessage(), ['exception' => $ex]);
|
||||
return "error: " . $ex->getMessage();
|
||||
}
|
||||
|
||||
@@ -49,6 +49,76 @@ class SqlGuard
|
||||
'/\b(pg_terminate_backend|pg_cancel_backend|pg_reload_conf|set_config|setval|lo_create|lo_put|lo_unlink|pg_switch_wal|pg_create_restore_point|pg_advisory_lock)\s*\(/i',
|
||||
];
|
||||
|
||||
/**
|
||||
* Splits a batch on ";" outside of 'strings', "identifiers",
|
||||
* $tag$dollar-quoted bodies$tag$ and comments, so a semicolon in a
|
||||
* DO block or a quote in a comment doesn't break up the split.
|
||||
*
|
||||
* @return string[]
|
||||
*/
|
||||
public static function splitStatements(string $sql): array
|
||||
{
|
||||
$statements = [];
|
||||
$current = '';
|
||||
$len = strlen($sql);
|
||||
$i = 0;
|
||||
|
||||
while ($i < $len) {
|
||||
$ch = $sql[$i];
|
||||
$end = null;
|
||||
|
||||
if ($ch === "'" || $ch === '"') {
|
||||
$backslashEscapes = $ch === "'" && $i > 0 && ($sql[$i - 1] === 'E' || $sql[$i - 1] === 'e')
|
||||
&& ($i === 1 || !preg_match('/[A-Za-z0-9_]/', $sql[$i - 2]));
|
||||
$end = $i + 1;
|
||||
while ($end < $len) {
|
||||
if ($backslashEscapes && $sql[$end] === '\\') {
|
||||
$end += 2;
|
||||
continue;
|
||||
}
|
||||
if ($sql[$end] === $ch) {
|
||||
if ($end + 1 < $len && $sql[$end + 1] === $ch) {
|
||||
$end += 2;
|
||||
continue;
|
||||
}
|
||||
break;
|
||||
}
|
||||
$end++;
|
||||
}
|
||||
$end = min($end + 1, $len);
|
||||
} elseif ($ch === '$' && preg_match('/\G\$([A-Za-z_][A-Za-z0-9_]*)?\$/', $sql, $m, 0, $i)) {
|
||||
$close = strpos($sql, $m[0], $i + strlen($m[0]));
|
||||
$end = $close === false ? $len : $close + strlen($m[0]);
|
||||
} elseif ($ch === '-' && substr($sql, $i, 2) === '--') {
|
||||
$newline = strpos($sql, "\n", $i);
|
||||
$end = $newline === false ? $len : $newline;
|
||||
} elseif ($ch === '/' && substr($sql, $i, 2) === '/*') {
|
||||
$close = strpos($sql, '*/', $i + 2);
|
||||
$end = $close === false ? $len : $close + 2;
|
||||
}
|
||||
|
||||
if ($end !== null) {
|
||||
$current .= substr($sql, $i, $end - $i);
|
||||
$i = $end;
|
||||
continue;
|
||||
}
|
||||
|
||||
if ($ch === ';') {
|
||||
$statements[] = trim($current);
|
||||
$current = '';
|
||||
} else {
|
||||
$current .= $ch;
|
||||
}
|
||||
$i++;
|
||||
}
|
||||
|
||||
$statements[] = trim($current);
|
||||
|
||||
return array_values(array_filter($statements, function ($s) {
|
||||
return $s !== '';
|
||||
}));
|
||||
}
|
||||
|
||||
/**
|
||||
* Replaces comments with a space. A comment separates tokens, so
|
||||
* dropping it without a space would glue "TO/ ** /PROGRAM" into one
|
||||
|
||||
@@ -125,4 +125,29 @@ class SqlGuardTest extends TestCase
|
||||
$this->assertSame('SELECT', SqlGuard::statementType('(SELECT 1)'));
|
||||
$this->assertSame('', SqlGuard::statementType(''));
|
||||
}
|
||||
|
||||
public static function splitProvider(): array
|
||||
{
|
||||
return [
|
||||
['SELECT 1; SELECT 2', ['SELECT 1', 'SELECT 2']],
|
||||
["SELECT ';'; SELECT 2", ["SELECT ';'", 'SELECT 2']],
|
||||
["SELECT 'it''s; x'", ["SELECT 'it''s; x'"]],
|
||||
['SELECT "a;b" FROM t', ['SELECT "a;b" FROM t']],
|
||||
["SELECT E'it\\'s; x'; SELECT 2", ["SELECT E'it\\'s; x'", 'SELECT 2']],
|
||||
["DO \$\$BEGIN PERFORM 1; PERFORM 2; END\$\$; SELECT 3", ["DO \$\$BEGIN PERFORM 1; PERFORM 2; END\$\$", 'SELECT 3']],
|
||||
["SELECT \$f\$a;\$\$;b\$f\$; SELECT 2", ["SELECT \$f\$a;\$\$;b\$f\$", 'SELECT 2']],
|
||||
["-- don't; split\nSELECT 1; SELECT 2", ["-- don't; split\nSELECT 1", 'SELECT 2']],
|
||||
["/* it's; here */ SELECT 1; SELECT 2", ["/* it's; here */ SELECT 1", 'SELECT 2']],
|
||||
['SELECT \$1; SELECT 2', ['SELECT \$1', 'SELECT 2']],
|
||||
[' ; ;SELECT 1;', ['SELECT 1']],
|
||||
];
|
||||
}
|
||||
|
||||
/**
|
||||
* @dataProvider splitProvider
|
||||
*/
|
||||
public function testSplitStatements(string $sql, array $expected): void
|
||||
{
|
||||
$this->assertSame($expected, SqlGuard::splitStatements($sql));
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user