Ensembia/occweb
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Harden SQL mode checks and sanitize occ output

Browse Source 
A security review found three issues:

- Comments were removed without a space before matching the forbidden
  patterns, so "TO/**/PROGRAM" became "TOPROGRAM" and passed the
  COPY ... PROGRAM check while PostgreSQL still ran it. Comments are
  now replaced by a space, and the COPY file check also covers E'...',
  U&'...' and dollar-quoted file names.
- Only statements starting with DELETE or UPDATE asked for
  confirmation, so a leading block comment, a data-modifying CTE
  (WITH x AS (DELETE ...)) or DROP/TRUNCATE/INSERT ran without it.
  Every statement that is not a plain read now needs confirmation,
  decided on the server.
- occ output could contain escape sequences, backspaces and URLs from
  user-controlled text (display names, file names) that rewrite or hide
  text or show a clickable link. Only color codes are kept now, and
  URLs are no longer turned into links.

The checks move to SqlGuard so they can be tested on their own. They
remain defense in depth: dynamic SQL can always get around a text
check, so the database role should not have superuser rights,
pg_execute_server_program or pg_read/write_server_files. A read that
starts with WITH now shows its rows like a SELECT.
This commit is contained in:
Dillard Blom
2026-09-30 03:15:35 +02:00
parent 3fc8601344
commit f68b1658be
5 changed files with 290 additions and 101 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lib/Controller/DbController.php
+21 -93
View File
@@ -3,6 +3,7 @@
namespace OCA\OCCWeb\Controller;
use OC;
use OCA\OCCWeb\Service\SqlGuard;
use OCP\AppFramework\Controller;
use OCP\IRequest;
use OCP\IDBConnection;
@@ -19,26 +20,6 @@ class DbController extends Controller
/** Maximum size of submitted SQL text in bytes (protection against OOM/DoS). */
private const MAX_SQL_BYTES = 1048576; // 1 MB
/**
* Constructs that give access to the server's filesystem or allow
* launching external programs via SQL. These are blocked entirely,
* with no way to confirm past them — unlike DELETE, this isn't
* about data loss, it's about potential server takeover.
*/
private const FORBIDDEN_PATTERNS = [
'/\bCOPY\b[\s\S]*\bPROGRAM\b/i' => 'COPY ... PROGRAM (runs external commands)',
'/\bCOPY\b[\s\S]*\b(FROM|TO)\b\s*\'/i' => "COPY ... FROM/TO 'file' (access to the server's filesystem)",
'/\bpg_read_binary_file\s*\(/i' => 'pg_read_binary_file()',
'/\bpg_read_file\s*\(/i' => 'pg_read_file()',
'/\bpg_ls_dir\s*\(/i' => 'pg_ls_dir()',
'/\bpg_stat_file\s*\(/i' => 'pg_stat_file()',
'/\blo_import\s*\(/i' => 'lo_import()',
'/\blo_export\s*\(/i' => 'lo_export()',
'/\bdblink(_connect)?\s*\(/i' => 'dblink() (connects to arbitrary databases)',
'/\bLOAD_FILE\s*\(/i' => 'LOAD_FILE()',
'/\bINTO\s+(OUTFILE|DUMPFILE)\b/i' => 'INTO OUTFILE/DUMPFILE',
];
private $db;
private $groupManager;
private $userSession;
@@ -60,35 +41,6 @@ class DbController extends Controller
$this->logger = OC::$server->get(LoggerInterface::class);
}
/**
* Strips leading single-line comments ("-- ...") before a query.
* After splitting a batch, such a comment can end up stuck to the
* next query and interfere with detecting its type (SELECT/SET/DELETE).
*/
private function stripLeadingComments($query)
{
$query = ltrim($query);
while (preg_match('/^--[^\n]*\n/', $query)) {
$query = ltrim(preg_replace('/^--[^\n]*\n/', '', $query, 1));
}
return $query;
}
/**
* Strips ALL single-line (-- ...) and block-style C comments from a
* query, including ones sitting inside an expression (e.g. between a
* function name and its opening parenthesis — otherwise a forbidden
* construct could be hidden from findForbiddenConstruct that way).
* Used only for checking against forbidden constructs — the query
* that actually gets executed is left unmodified.
*/
private function removeAllComments($query)
{
$query = preg_replace('/--[^\n]*/', '', $query);
$query = preg_replace('/\/\*[\s\S]*?\*\//', '', $query);
return $query;
}
/**
* Splits a batch of queries on ";" while respecting single and double
* quotes (strings and Postgres-escaped identifiers), so that a
@@ -142,24 +94,6 @@ class DbController extends Controller
}));
}
/**
* Returns a description of the forbidden construct found (filesystem
* access, running programs), or null if the query is safe in that
* regard. The comment-stripped version of the query is checked —
* otherwise the construct could be hidden by inserting a comment
* between the function name and "(".
*/
private function findForbiddenConstruct($query)
{
$clean = $this->removeAllComments($query);
foreach (self::FORBIDDEN_PATTERNS as $pattern => $label) {
if (preg_match($pattern, $clean)) {
return $label;
}
}
return null;
}
public function query()
{
// Deliberate defense-in-depth (see the equivalent check in
@@ -210,7 +144,7 @@ class DbController extends Controller
// blocked entirely — this isn't about data loss (like DELETE), it's
// about potential server takeover, and confirmation doesn't bypass it.
foreach ($queries as $query) {
$forbidden = $this->findForbiddenConstruct($query);
$forbidden = SqlGuard::findForbiddenConstruct($query);
if ($forbidden !== null) {
$this->logger->error('[extended_occweb] Blocked forbidden construct ({construct}) from {user}: {sql}', [
'app' => 'extended_occweb',
@@ -225,34 +159,29 @@ class DbController extends Controller
}
}
// DELETE and UPDATE are irreversible (or hard to reverse), so we
// require explicit confirmation from the client (confirm=true) before
// executing a single query from the batch.
$deleteCount = 0;
$updateCount = 0;
// Anything that isn't a plain read (DELETE, UPDATE, INSERT, DROP,
// a data-modifying CTE, ...) needs explicit confirmation from the
// client (confirm=true) before a single query of the batch runs.
$writeTypes = [];
foreach ($queries as $query) {
$normalized = $this->stripLeadingComments($query);
if (stripos($normalized, 'DELETE') === 0) {
$deleteCount++;
} elseif (stripos($normalized, 'UPDATE') === 0) {
$updateCount++;
if (!SqlGuard::isReadOnly($query)) {
$type = SqlGuard::statementType($query) ?: 'UNKNOWN';
$writeTypes[$type] = ($writeTypes[$type] ?? 0) + 1;
}
}
if (($deleteCount > 0 || $updateCount > 0) && !$confirmed) {
if ($writeTypes !== [] && !$confirmed) {
$parts = [];
if ($deleteCount > 0) {
$parts[] = "{$deleteCount} DELETE";
}
if ($updateCount > 0) {
$parts[] = "{$updateCount} UPDATE";
foreach ($writeTypes as $type => $count) {
$parts[] = "{$count} {$type}";
}
return new JSONResponse([
'success' => false,
'requiresConfirmation' => true,
'deleteCount' => $deleteCount,
'updateCount' => $updateCount,
'error' => 'Batch contains ' . implode(' and ', $parts) . ' statement(s) and was not executed. Resend with confirm=true to proceed.'
'deleteCount' => $writeTypes['DELETE'] ?? 0,
'updateCount' => $writeTypes['UPDATE'] ?? 0,
'writeCount' => array_sum($writeTypes),
'error' => 'Batch contains statement(s) that change data or the schema (' . implode(', ', $parts) . ') and was not executed. Resend with confirm=true to proceed.'
]);
}
@@ -289,17 +218,16 @@ class DbController extends Controller
}
foreach ($queries as $query) {
$normalized = $this->stripLeadingComments($query);
$isSelect = stripos($normalized, 'SELECT') === 0;
$isSet = stripos($normalized, 'SET ') === 0;
$isDelete = stripos($normalized, 'DELETE') === 0;
$isUpdate = stripos($normalized, 'UPDATE') === 0;
$statementType = SqlGuard::statementType($query);
$isSet = $statementType === 'SET';
$isDelete = $statementType === 'DELETE';
$isUpdate = $statementType === 'UPDATE';
try {
$stmt = $this->db->prepare($query);
$stmt->execute();
if ($isSelect) {
if ($statementType === 'SELECT' || ($statementType === 'WITH' && SqlGuard::isReadOnly($query))) {
// Read row by row and stop at MAX_ROWS, instead of
// fetchAll() + array_slice — otherwise a SELECT without
// LIMIT on a huge table would still pull everything into