diff --git a/CLAUDE.md b/CLAUDE.md index c4ef6ef..7900810 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -702,6 +702,24 @@ it against a real database — not a theoretical hardening pass. the parse tree (`detectSelectWrite`) **and** runs a text backstop (`detectHiddenWrite`) so an unparseable variant fails closed instead of falling through to the keyword path. +- **A verb-based guard cannot see a dangerous *function*, and `setReadOnly(true)` cannot stop + one that leaves the session.** `SELECT dblink_exec('dbname=app …','DELETE FROM orders')` + defeated **both** layers at once: it begins `SELECT` so the allowlist passes it and no + forbidden verb appears, and `dblink` opens a **new outbound connection** whose transaction is + not read-only — the flag constrains the session it is set on, never one the query dials out + and creates. Reproduced against a real PostgreSQL: inside `BEGIN TRANSACTION READ ONLY`, the + statement reported `DELETE 3` and the table went from 3 rows to 0; `pg_read_file` likewise + read the server's filesystem under read-only. Until public dashboard queries were bound to + published shapes this was reachable **unauthenticated** through the share endpoint, which + runs the same executor. `DANGEROUS_SQL_FUNCTIONS` now denies dblink*, `pg_read_file`, + `pg_read_binary_file`, `pg_ls_dir`, `pg_stat_file`, `lo_import`, `lo_export` and MySQL + `LOAD_FILE`. A denylist is the right shape *here* only because the allowlist governs verbs + and there is no allowlist of functions that may sit inside a `SELECT`. **Match the call, not + the name** — `(?Every check above classifies by statement verb, so a {@code SELECT} that calls + * one of these passes cleanly: the allowlist sees SELECT and no forbidden verb appears. + * {@code connection.setReadOnly(true)} does not stop them either — {@code dblink} opens a + * new outbound connection whose transaction is not read-only, so the flag + * constrains the session it is set on but never one the query dials out and creates. + * + *

Verified against a real PostgreSQL, not inferred: inside an explicitly + * {@code BEGIN TRANSACTION READ ONLY}, {@code SELECT dblink_exec(..., 'DELETE FROM t')} + * reported {@code DELETE 3} and the table went from three rows to zero; + * {@code pg_read_file('/etc/hostname')} returned its contents from the database server's + * filesystem. Both of the product's layers failed at once. + * + *

A denylist is the wrong shape in general, but it is the right shape here: the guard's + * allowlist governs verbs, and there is no allowlist of functions to sit inside + * a SELECT. Names are matched as calls (see {@link #DANGEROUS_FUNCTION_CALL}) so an + * ordinary identifier of the same name still works. + */ + private static final List DANGEROUS_SQL_FUNCTIONS = List.of( + // outbound connections — escape the read-only session entirely + "dblink", "dblink_exec", "dblink_connect", "dblink_open", "dblink_send_query", + // server-side file access + "pg_read_file", "pg_read_binary_file", "pg_ls_dir", "pg_stat_file", + "lo_import", "lo_export", + // MySQL equivalents + "load_file" + ); + + private static final String DANGEROUS_FUNCTION_ALTERNATION = + String.join("|", DANGEROUS_SQL_FUNCTIONS); + + /** + * A dangerous function being called: the name, optional whitespace, then an open + * paren. Matching the bare name would reject ordinary identifiers — plenty of schemas have + * a {@code dblink_audit} table or a {@code load_file_name} column, the same mistake + * CLAUDE.md records for the old {@code \bCOMMENT\b} rule that rejected + * {@code SELECT * FROM comment}. A leading word-boundary check keeps {@code my_dblink(} — + * a different function — from matching. + */ + private static final Pattern DANGEROUS_FUNCTION_CALL = Pattern.compile( + "(? FORBIDDEN_SQL_KEYWORD_SET = Set.copyOf(FORBIDDEN_SQL_KEYWORDS); private static final String FORBIDDEN_ALTERNATION = String.join("|", FORBIDDEN_SQL_KEYWORDS); @@ -102,9 +147,29 @@ public ValidationOutcome validateReadOnlySql(String sql, boolean allowExplain) { ); } + String dangerousFunction = containsDangerousFunction(statement); + if (dangerousFunction != null) { + return ValidationOutcome.invalid( + "Blocked SQL function that reads or writes outside this session: " + + dangerousFunction + "." + ); + } + return ValidationOutcome.valid(stripTrailingSemicolons(sql), keyword); } + /** + * The first dangerous function call in the statement, or null. + * + *

Inspected with comments and string literals stripped, so neither + * {@code /*x*} + {@code /dblink_exec(} nor a name mentioned inside a quoted literal can + * hide or falsely trigger a match. + */ + String containsDangerousFunction(String statement) { + var matcher = DANGEROUS_FUNCTION_CALL.matcher(normalizeSqlForInspection(statement)); + return matcher.find() ? matcher.group(1).toLowerCase(Locale.ROOT) : null; + } + String normalizeSqlForInspection(String sql) { return compactWhitespace(stripSqlStringLiterals(stripSqlComments(sql))); } diff --git a/backend/src/test/java/com/dbaagent/service/McpSqlGuardServiceTest.java b/backend/src/test/java/com/dbaagent/service/McpSqlGuardServiceTest.java index f874a67..1f32764 100644 --- a/backend/src/test/java/com/dbaagent/service/McpSqlGuardServiceTest.java +++ b/backend/src/test/java/com/dbaagent/service/McpSqlGuardServiceTest.java @@ -122,4 +122,95 @@ void rejectsExplainOfDeleteButAllowsExplainOfCommentTable() { var commentPlan = service.validateReadOnlySql("EXPLAIN SELECT * FROM comment", true); assertTrue(commentPlan.ok()); } + + // ── dangerous functions ─────────────────────────────────────────────────── + // + // The guard classifies by statement *verb*, so a SELECT that calls a dangerous + // function passes every check: the allowlist sees SELECT, and no forbidden verb + // appears anywhere. Verified against a real PostgreSQL 17 — inside an explicitly + // READ ONLY transaction, `SELECT dblink_exec(..., 'DELETE FROM t')` reported + // `DELETE 3` and the table went from 3 rows to 0. + // + // connection.setReadOnly(true) cannot stop it either: dblink opens a *new outbound + // connection* whose transaction is not read-only. The read-only flag constrains the + // session it is set on, never one the query dials out and creates. So both of the + // product's layers fail at once, and until the public dashboard path was bound to + // published query shapes this was reachable anonymously. + + @Test + void rejectsDblinkExec() { + var result = service.validateReadOnlySql( + "SELECT dblink_exec('dbname=app user=postgres host=127.0.0.1','DELETE FROM orders')", true); + + assertFalse(result.ok()); + assertTrue(result.reason().toLowerCase().contains("dblink"), + "reason should name the function it refused, was: " + result.reason()); + } + + @Test + void rejectsEveryDblinkEntryPoint() { + for (String sql : new String[] { + "SELECT * FROM dblink('dbname=app','SELECT 1') AS t(a int)", + "SELECT dblink_connect('dbname=app')", + "SELECT dblink_send_query('conn','DELETE FROM orders')", + "SELECT dblink_open('conn','cur','SELECT 1')" + }) { + assertFalse(service.validateReadOnlySql(sql, true).ok(), "should refuse: " + sql); + } + } + + @Test + void rejectsServerSideFileReads() { + for (String sql : new String[] { + "SELECT pg_read_file('/etc/passwd')", + "SELECT pg_read_binary_file('/etc/passwd')", + "SELECT pg_ls_dir('/var/lib/postgresql/data')", + "SELECT pg_stat_file('/etc/passwd')", + "SELECT lo_import('/etc/passwd')", + "SELECT lo_export(1,'/tmp/out')" + }) { + assertFalse(service.validateReadOnlySql(sql, true).ok(), "should refuse: " + sql); + } + } + + @Test + void rejectsMySqlFileReads() { + assertFalse(service.validateReadOnlySql("SELECT LOAD_FILE('/etc/passwd')", true).ok()); + } + + @Test + void rejectsDangerousFunctionRegardlessOfSpacingOrCase() { + for (String sql : new String[] { + "SELECT DBLINK_EXEC('x','DELETE FROM t')", + "SELECT dblink_exec ('x','DELETE FROM t')", + "SELECT pg_read_file\n('/etc/passwd')", + "WITH x AS (SELECT pg_read_file('/etc/passwd') AS f) SELECT * FROM x" + }) { + assertFalse(service.validateReadOnlySql(sql, true).ok(), "should refuse: " + sql); + } + } + + /** + * The guard must match a function *call*, not a name that merely appears. Column and + * table names are ordinary identifiers and plenty of schemas contain them — the same + * mistake CLAUDE.md records for the old \\bCOMMENT\\b rule, which rejected + * `SELECT * FROM comment`. + */ + @Test + void stillAllowsIdentifiersThatMerelyResembleADangerousFunction() { + for (String sql : new String[] { + "SELECT * FROM public.dblink_audit", + "SELECT t.pg_read_file_count FROM public.stats t", + "SELECT load_file_name FROM public.imports", + "SELECT * FROM comment" + }) { + assertTrue(service.validateReadOnlySql(sql, true).ok(), "should allow: " + sql); + } + } + + @Test + void stillAllowsOrdinaryAnalyticQueries() { + assertTrue(service.validateReadOnlySql( + "SELECT count(*), sum(o.total) FROM public.orders o WHERE o.created_at >= '2026-01-01'", true).ok()); + } } diff --git a/docs/security/2026-09-11-sql-guard-dangerous-functions.md b/docs/security/2026-09-11-sql-guard-dangerous-functions.md new file mode 100644 index 0000000..894309a --- /dev/null +++ b/docs/security/2026-09-11-sql-guard-dangerous-functions.md @@ -0,0 +1,125 @@ +# A SELECT could write through both read-only layers + +*Found 2026-09-10 in a repository-wide security audit; reproduced against a live PostgreSQL +2026-09-11. Severity: critical.* + +## What was wrong + +Both of the product's read-only defences were bypassed by one statement: + +```sql +SELECT dblink_exec('dbname=app user=postgres host=127.0.0.1', 'DELETE FROM orders') +``` + +**The guard failed** because `McpSqlGuardService` classifies by statement *verb*. The statement +begins `SELECT`, which is on `ALLOWED_READ_ONLY_KEYWORDS`, and none of the 15 +`FORBIDDEN_SQL_KEYWORDS` appears anywhere in it. `dblink_exec` is a *function call* — invisible +to a verb-based parser. + +**`connection.setReadOnly(true)` failed** for a subtler reason, and this is the part worth +understanding: `dblink` opens a **new outbound connection** to the database. That second +connection runs its own transaction, which is not read-only. The read-only flag constrains the +session it is set on; it cannot constrain a session the query itself dials out and creates. + +CLAUDE.md describes `setReadOnly(true)` as the backstop that "keeps the *next* parser gap from +becoming data loss". That holds for ordinary writes — `SELECT … INTO`, `nextval`, `lo_import` +and volatile writer functions are all correctly refused by it. It does not hold for a function +that leaves the session. + +## Reproduced, not inferred + +Against a real PostgreSQL, in an isolated `zz_sec` schema (created and dropped for the test): + +``` +rows before 3 +BEGIN TRANSACTION READ ONLY +SELECT dblink_exec('dbname=dba_agent …','DELETE FROM zz_sec.victim') + dblink_exec +------------- + DELETE 3 +COMMIT +rows after 0 +``` + +A `DELETE` ran to completion inside an explicitly read-only transaction. + +The same class of function reads the database server's filesystem, also under read-only: + +``` +BEGIN TRANSACTION READ ONLY +SELECT length(pg_read_file('/etc/hostname')) -> 13 +``` + +And the guard permitted every one of them. Running the shipped `validateReadOnlySql` over the +payloads directly returned `ALLOWED` for `dblink_exec`, `dblink`, `pg_read_file`, `pg_ls_dir`, +`pg_read_binary_file`, `lo_import` and `LOAD_FILE`. + +Until public dashboard queries were bound to their published shapes, this was reachable from +the **unauthenticated** share endpoint, which runs through the same executor. + +## The fix + +A denylist of functions that read or write outside the session, checked after the verb checks +in both guards: + +```java +private static final List DANGEROUS_SQL_FUNCTIONS = List.of( + "dblink", "dblink_exec", "dblink_connect", "dblink_open", "dblink_send_query", + "pg_read_file", "pg_read_binary_file", "pg_ls_dir", "pg_stat_file", + "lo_import", "lo_export", + "load_file"); +``` + +A denylist is usually the wrong shape. It is the right shape *here* because the guard's +allowlist governs **verbs**, and there is no allowlist of functions that may appear inside a +`SELECT` — the set of legitimate functions is open-ended, while the set that escapes the +session is small and nameable. + +**Matched as a call, not as a name.** The pattern requires the name, optional whitespace, then +an open paren, with a leading boundary check: + +```java +"(? { assert.equal(result, "http://localhost:8080/api/connections/123/schema"); }); +// Dangerous functions. The guard classifies by statement verb, so a SELECT calling one of +// these passed every check; connection.setReadOnly(true) does not stop them either, because +// dblink opens a new outbound connection whose transaction is not read-only. Verified against +// a real PostgreSQL: inside BEGIN TRANSACTION READ ONLY, dblink_exec(..., 'DELETE FROM t') +// reported DELETE 3 and the table went from three rows to zero. +test("validateReadOnlySql blocks dblink and other outbound-connection functions", () => { + for (const sql of [ + "SELECT dblink_exec('dbname=app','DELETE FROM orders')", + "SELECT * FROM dblink('dbname=app','SELECT 1') AS t(a int)", + "SELECT dblink_connect('dbname=app')", + "SELECT dblink_send_query('c','DELETE FROM orders')", + ]) { + assert.equal(validateReadOnlySql(sql).ok, false, `should refuse: ${sql}`); + } +}); + +test("validateReadOnlySql blocks server-side file reads", () => { + for (const sql of [ + "SELECT pg_read_file('/etc/passwd')", + "SELECT pg_read_binary_file('/etc/passwd')", + "SELECT pg_ls_dir('/tmp')", + "SELECT pg_stat_file('/etc/passwd')", + "SELECT lo_import('/etc/passwd')", + "SELECT LOAD_FILE('/etc/passwd')", + ]) { + assert.equal(validateReadOnlySql(sql).ok, false, `should refuse: ${sql}`); + } +}); + +// The name must be matched as a *call*, not wherever it appears: plenty of schemas have a +// dblink_audit table or a load_file_name column. Same mistake the old \bCOMMENT\b rule made +// when it rejected "SELECT * FROM comment". +test("validateReadOnlySql still allows identifiers that resemble a dangerous function", () => { + for (const sql of [ + "SELECT * FROM public.dblink_audit", + "SELECT t.pg_read_file_count FROM public.stats t", + "SELECT load_file_name FROM public.imports", + "SELECT * FROM comment", + ]) { + assert.equal(validateReadOnlySql(sql).ok, true, `should allow: ${sql}`); + } +}); + test("validateReadOnlySql accepts a simple select", () => { const result = validateReadOnlySql("SELECT * FROM orders LIMIT 10;"); assert.equal(result.ok, true);