From ef6f19df618fa8e95a2f039efdba34f0f0b717c8 Mon Sep 17 00:00:00 2001 From: Stefan Bischof Date: Tue, 25 Aug 2026 10:58:29 +0200 Subject: [PATCH] harden(guard): block side-effecting selects and document the contract Build on the data-modifying-CTE fix already on main. Extend SqlGuard's read-only enforcement to the rest of the "parses as Select but has a side effect" class, and document the contract: - reject SELECT ... INTO (writes a table) and FOR UPDATE/FOR SHARE (locks); - fail closed: any unexpected RuntimeException during validation now becomes a GuardException instead of propagating or passing through; - add javadoc to SqlGuard.guard()/SqlGuardFactory.create() and a guard/README.md covering the read-only contract and the function whitelist (full-match case-sensitive regex, empty = deny-all). Extends ReadOnlyStatementValidator and adds integration tests for each new case. Signed-off-by: Stefan Bischof --- guard/README.md | 88 +++++++++++++++++++ .../daanse/sql/guard/api/SqlGuard.java | 34 +++++++ .../daanse/sql/guard/api/SqlGuardFactory.java | 33 +++++++ .../ReadOnlyStatementValidator.java | 64 +++++++++++--- .../jsqltranspiler/TranspilerSqlGuard.java | 6 ++ .../integration/SqlGuardTest.java | 57 ++++++++++++ 6 files changed, 268 insertions(+), 14 deletions(-) create mode 100644 guard/README.md diff --git a/guard/README.md b/guard/README.md new file mode 100644 index 0000000..4a08e0f --- /dev/null +++ b/guard/README.md @@ -0,0 +1,88 @@ +# Daanse SQL Guard + +`SqlGuard` narrows an **untrusted** SQL string down to a **read-only query over an +explicitly whitelisted catalog**. It is the trust boundary between caller-supplied +SQL (for example a drill-through / pass-through query from an analytics client) and +the database. + +- **API:** `guard/api` — `SqlGuard`, `SqlGuardFactory`, the `elements` catalog model, + and the `GuardException` hierarchy. +- **Implementation:** `guard/jsqltranspiler` — `TranspilerSqlGuard`, built on JSqlParser + and JSQLTranspiler. + +## What the guard guarantees + +`SqlGuard.guard(String)` either returns an equivalent read-only `SELECT` — with every +table and column fully qualified against the whitelisted catalog and rendered in the +configured dialect — or throws a `GuardException`. It rejects, by construction: + +- any non-`SELECT` statement (INSERT / UPDATE / DELETE / DDL / …); +- a **data-modifying CTE**, e.g. `WITH x AS (UPDATE foo …) SELECT …`; +- `SELECT … INTO` (writes a table) and `SELECT … FOR UPDATE` / `FOR SHARE` (row locks); +- any reference to a table or column **outside the whitelisted catalog**; +- any **function not permitted by the function whitelist** (see below); +- empty or unparsable input. + +The guard **fails closed**: any unexpected error during validation results in a +`GuardException`, never an approved statement. + +## The function whitelist + +The whitelist is the control for which functions a query may call. It is a +`List` of **full-match, case-sensitive regular expressions** (evaluated with +`java.util.regex.Pattern.matches`). Every function name appearing anywhere in the +query — SELECT list, `WHERE`, `HAVING`, subqueries, and so on — must be matched in +full by at least one pattern, or the whole statement is rejected with +`GuardException("Query has disallowed functions.")`. + +```java +// allow three aggregate functions, nothing else +SqlGuard guard = factory.create("", "sch", catalog, + List.of("sum", "count", "avg"), dialect); +``` + +Key points: + +- **An empty list rejects every function** — `List.of()` is the secure default, and + the guard is safe out of the box. +- Matching is **case-sensitive and anchored**: `"sum"` matches only `sum`, not `SUM`. + Use `"(?i)sum"`, or list both spellings, when you need case-insensitivity. +- `List.of(".*")` allows **every** function and is strongly discouraged — see below. + +## Keep the whitelist tight + +Allow only the specific aggregate/scalar functions your use case needs. In particular, +**never** allow function families that have side effects or enable denial of service or +data exfiltration, for example: + +| Category | Examples | +|---|---| +| Sequence writes | `nextval`, `setval` | +| Denial of service | `pg_sleep`, `benchmark` | +| File / network I/O, RCE | `dblink`, `lo_import`, `lo_export`, `pg_read_file`, `pg_ls_dir`, `xp_cmdshell`, `load_file` | +| Backend control | `pg_terminate_backend`, `pg_cancel_backend` | + +A broad pattern such as `.*` re-enables all of these at once. Prefer an explicit, +enumerated list over any catch-all. + +## Why there is no built-in denylist + +The guard does **not** ship a hardcoded list of "dangerous" functions to block. Such a +denylist is: + +- **inherently incomplete** — the set of side-effecting functions differs by database, + extension, and version, and grows over time; +- a source of **false confidence** — a denylist that looks comprehensive invites callers + to relax the whitelist, which is exactly the wrong direction. + +The whitelist is the authoritative control, and it is already default-deny. The correct +lever is keeping that whitelist tight, not maintaining a denylist that can never be +complete. + +## Defense in depth (outside this library) + +Treat the guard as one layer, not the only one. Execute guarded SQL under a **read-only +database role** and/or inside a **read-only transaction** (for example +`SET TRANSACTION READ ONLY`, or `default_transaction_read_only` on PostgreSQL). Then even +a future bypass — a construct no one anticipated — still cannot write. The parser-level +guard should never be the sole barrier between untrusted SQL and the database. diff --git a/guard/api/src/main/java/org/eclipse/daanse/sql/guard/api/SqlGuard.java b/guard/api/src/main/java/org/eclipse/daanse/sql/guard/api/SqlGuard.java index 49db381..cc6a92f 100644 --- a/guard/api/src/main/java/org/eclipse/daanse/sql/guard/api/SqlGuard.java +++ b/guard/api/src/main/java/org/eclipse/daanse/sql/guard/api/SqlGuard.java @@ -15,8 +15,42 @@ import org.eclipse.daanse.sql.guard.api.exception.GuardException; +/** + * Narrows an untrusted SQL string to a read-only query over an explicitly + * whitelisted catalog. The guard is the trust boundary between caller-supplied + * SQL and the database. + */ public interface SqlGuard { + /** + * Validates {@code sql} and, if it is accepted, returns an equivalent read-only + * {@code SELECT} with every table and column fully qualified against the + * whitelisted catalog and rendered in the configured dialect. + *

+ * The statement is rejected with a {@link GuardException} unless it is a plain, + * read-only {@code SELECT}. In particular the guard rejects: + *

    + *
  • any non-{@code SELECT} statement (INSERT/UPDATE/DELETE/DDL/…) — + * {@link org.eclipse.daanse.sql.guard.api.exception.UnallowedStatementTypeGuardException};
    • + *
  • a data-modifying CTE, e.g. {@code WITH x AS (UPDATE foo ...) SELECT ...};
    • + *
  • {@code SELECT ... INTO} (writes a table) and {@code SELECT ... FOR UPDATE} + * / {@code FOR SHARE} (row locks);
    • + *
  • any reference to a table or column outside the whitelisted catalog — + * {@link org.eclipse.daanse.sql.guard.api.exception.UnresolvableObjectsGuardException};
    • + *
  • any function not permitted by the configured function whitelist (see + * {@link SqlGuardFactory#create});
    • + *
  • empty input + * ({@link org.eclipse.daanse.sql.guard.api.exception.EmptyStatementGuardException}) + * and unparsable input + * ({@link org.eclipse.daanse.sql.guard.api.exception.UnparsableStatementGuardException}).
    • + *
+ * The guard fails closed: any unexpected error during validation results in a + * {@link GuardException} rather than an approved statement. + * + * @param sql the untrusted SQL to validate. + * @return the rewritten, fully qualified read-only statement. + * @throws GuardException if the statement is not an allowed read-only query. + */ String guard(String sql) throws GuardException; } diff --git a/guard/api/src/main/java/org/eclipse/daanse/sql/guard/api/SqlGuardFactory.java b/guard/api/src/main/java/org/eclipse/daanse/sql/guard/api/SqlGuardFactory.java index 46b5acb..bcda15c 100644 --- a/guard/api/src/main/java/org/eclipse/daanse/sql/guard/api/SqlGuardFactory.java +++ b/guard/api/src/main/java/org/eclipse/daanse/sql/guard/api/SqlGuardFactory.java @@ -20,5 +20,38 @@ public interface SqlGuardFactory { + /** + * Creates a {@link SqlGuard} bound to a whitelisted catalog and a function + * whitelist. + * + * @param currentCatalogName the catalog assumed for unqualified names. + * @param currentSchemaName the schema assumed for unqualified names. + * @param databaseCatalog the catalog, schemas, tables and columns that + * queries are allowed to reference; anything not + * described here is rejected. + * @param whitelistFunctionsPatterns the functions a query may call, as a list of + * full-match, case-sensitive regular expressions + * (evaluated with {@link java.util.regex.Pattern#matches}). A pattern + * is tested against every function name appearing anywhere in the + * query. A function passes only if at least one pattern matches its + * whole name; a single unmatched function rejects the statement. + *
    + *
  • An empty list rejects every function — this is + * the secure default.
    • + *
  • {@code "sum"} matches only {@code sum}, not {@code SUM}; use + * {@code "(?i)sum"} or list both spellings for case-insensitivity.
    • + *
  • {@code List.of(".*")} allows every function and is + * strongly discouraged: it re-enables side-effecting and + * denial-of-service functions (e.g. {@code nextval}, {@code setval}, + * {@code pg_sleep}, {@code dblink}, {@code pg_read_file}, + * {@code xp_cmdshell}).
    • + *
+ * Keep this list as tight as the use case allows. See + * {@code guard/README.md} for the rationale and guidance. + * @param dialect the SQL dialect used to render the rewritten, + * fully qualified output. + * @return a guard that enforces the read-only contract described on + * {@link SqlGuard#guard(String)}. + */ SqlGuard create(String currentCatalogName, String currentSchemaName, DatabaseCatalog databaseCatalog, List whitelistFunctionsPatterns, Dialect dialect); } diff --git a/guard/jsqltranspiler/src/main/java/org/eclipse/daanse/sql/guard/jsqltranspiler/ReadOnlyStatementValidator.java b/guard/jsqltranspiler/src/main/java/org/eclipse/daanse/sql/guard/jsqltranspiler/ReadOnlyStatementValidator.java index 9259e26..9ba0fc8 100644 --- a/guard/jsqltranspiler/src/main/java/org/eclipse/daanse/sql/guard/jsqltranspiler/ReadOnlyStatementValidator.java +++ b/guard/jsqltranspiler/src/main/java/org/eclipse/daanse/sql/guard/jsqltranspiler/ReadOnlyStatementValidator.java @@ -19,7 +19,9 @@ import net.sf.jsqlparser.statement.ParenthesedStatement; import net.sf.jsqlparser.statement.Statement; import net.sf.jsqlparser.statement.select.ParenthesedSelect; +import net.sf.jsqlparser.statement.select.PlainSelect; import net.sf.jsqlparser.statement.select.Select; +import net.sf.jsqlparser.statement.select.SetOperationList; import net.sf.jsqlparser.statement.select.WithItem; import net.sf.jsqlparser.statement.delete.ParenthesedDelete; import net.sf.jsqlparser.statement.insert.ParenthesedInsert; @@ -27,20 +29,23 @@ import net.sf.jsqlparser.util.TablesNamesFinder; /** - * Walks a parsed {@link Select} tree and detects data-modifying statements that - * are wrapped in a {@code WITH} item (a data-modifying CTE), such as - * {@code WITH x AS (UPDATE foo ...) SELECT ...}. - *

- * Such statements parse as a {@code Select} and therefore pass the top-level - * {@code instanceof Select} gate, yet on PostgreSQL/MariaDB they execute the - * wrapped {@code UPDATE}/{@code INSERT}/{@code DELETE}. The upstream resolver - * does not populate its update/insert column lists for a {@code WithItem}, so it - * cannot be relied on to detect this case. - *

- * This validator reuses {@link TablesNamesFinder}'s full recursive traversal - * (subqueries, joins, set operations, nested {@code WITH} clauses) and inspects - * every {@code WithItem}. A {@code WithItem} is only permitted to wrap a - * {@link ParenthesedSelect}; anything else is a violation. + * Walks a parsed {@link Select} tree and rejects statements that parse as a + * {@code Select} yet carry a side effect. Several SQL constructs pass the naive + * {@code instanceof Select} gate but are not read-only: + *

    + *
  • a data-modifying CTE, e.g. {@code WITH x AS (UPDATE foo ...) SELECT ...} — + * on PostgreSQL/MariaDB the wrapped {@code UPDATE}/{@code INSERT}/{@code DELETE} + * executes. The upstream resolver does not populate its update/insert column + * lists for a {@code WithItem}, so it cannot be relied on to detect this;
    • + *
  • {@code SELECT ... INTO newtable} — creates/writes a table + * (PostgreSQL, SQL Server);
    • + *
  • {@code SELECT ... FOR UPDATE}/{@code FOR SHARE} — takes row locks.
    • + *
+ * The validator reuses {@link TablesNamesFinder}'s full recursive traversal + * (subqueries, joins, set operations, nested {@code WITH} clauses). A + * {@code WithItem} is only permitted to wrap a {@link ParenthesedSelect}; a + * {@code SELECT INTO} or a {@code FOR UPDATE}/{@code FOR SHARE} clause anywhere + * in the tree is a violation. */ class ReadOnlyStatementValidator extends TablesNamesFinder { @@ -48,6 +53,8 @@ class ReadOnlyStatementValidator extends TablesNamesFinder { static final String UPDATE_IS_NOT_PERMITTED = "UPDATE is not permitted."; static final String INSERT_IS_NOT_PERMITTED = "INSERT is not permitted."; static final String STATEMENT_IS_NOT_PERMITTED = "Statement is not permitted."; + static final String SELECT_INTO_IS_NOT_PERMITTED = "SELECT INTO is not permitted."; + static final String ROW_LOCKING_IS_NOT_PERMITTED = "Row locking (FOR UPDATE/SHARE) is not permitted."; private String violation; @@ -62,6 +69,35 @@ Optional firstViolation(Select select) { return Optional.ofNullable(violation); } + private void record(String message) { + if (violation == null) { + violation = message; + } + } + + /** A FOR UPDATE / FOR SHARE clause can hang off any Select node. */ + private void checkRowLocking(Select select) { + if (select.getForMode() != null || select.getForUpdateTable() != null) { + record(ROW_LOCKING_IS_NOT_PERMITTED); + } + } + + @Override + public Void visit(PlainSelect plainSelect, S context) { + if (plainSelect.getIntoTables() != null && !plainSelect.getIntoTables().isEmpty() + || plainSelect.getIntoTempTable() != null) { + record(SELECT_INTO_IS_NOT_PERMITTED); + } + checkRowLocking(plainSelect); + return super.visit(plainSelect, context); + } + + @Override + public Void visit(SetOperationList setOpList, S context) { + checkRowLocking(setOpList); + return super.visit(setOpList, context); + } + @Override public Void visit(WithItem withItem, S context) { ParenthesedStatement statement = withItem.getParenthesedStatement(); diff --git a/guard/jsqltranspiler/src/main/java/org/eclipse/daanse/sql/guard/jsqltranspiler/TranspilerSqlGuard.java b/guard/jsqltranspiler/src/main/java/org/eclipse/daanse/sql/guard/jsqltranspiler/TranspilerSqlGuard.java index 81db60f..36c4c94 100644 --- a/guard/jsqltranspiler/src/main/java/org/eclipse/daanse/sql/guard/jsqltranspiler/TranspilerSqlGuard.java +++ b/guard/jsqltranspiler/src/main/java/org/eclipse/daanse/sql/guard/jsqltranspiler/TranspilerSqlGuard.java @@ -57,6 +57,7 @@ public class TranspilerSqlGuard implements SqlGuard { private static final String INSERT_IS_NOT_PERMITTED = "INSERT is not permitted."; private static final String NOTHING_WAS_SELECTED = "Nothing was selected."; private static final String QUERY_HAS_DISAllOWED_FUNCTIONS = "Query has disallowed functions."; + private static final String COULD_NOT_VALIDATE = "Statement could not be validated."; private static final Logger LOGGER = LoggerFactory.getLogger(TranspilerSqlGuard.class); private JdbcMetaData jdbcMetaDataToCopy; private List whitelistFunctionsPatterns = new ArrayList(); @@ -176,6 +177,11 @@ public String guard(String sqlStr) throws GuardException { } catch (CatalogNotFoundException | ColumnNotFoundException | SchemaNotFoundException | TableNotDeclaredException | TableNotFoundException ex) { throw new UnresolvableObjectsGuardException(ex.getMessage()); + } catch (RuntimeException ex) { + // Fail closed: any unexpected error while validating untrusted SQL must + // deny the statement, never let it through or leak an internal exception. + LOGGER.atWarn().setCause(ex).log(COULD_NOT_VALIDATE); + throw new GuardException(COULD_NOT_VALIDATE); } } diff --git a/guard/jsqltranspiler/src/test/java/org/eclipse/daanse/sql/guard/jsqltranspiler/integration/SqlGuardTest.java b/guard/jsqltranspiler/src/test/java/org/eclipse/daanse/sql/guard/jsqltranspiler/integration/SqlGuardTest.java index 7f788dc..215e244 100644 --- a/guard/jsqltranspiler/src/test/java/org/eclipse/daanse/sql/guard/jsqltranspiler/integration/SqlGuardTest.java +++ b/guard/jsqltranspiler/src/test/java/org/eclipse/daanse/sql/guard/jsqltranspiler/integration/SqlGuardTest.java @@ -269,6 +269,19 @@ y AS (UPDATE foo SET name='b' RETURNING id) private static final String INSERT_IS_NOT_PERMITTED = "INSERT is not permitted."; private static final String DELETE_IS_NOT_PERMITTED = "DELETE is not permitted."; + + private static final String SQL_SELECT_INTO = "SELECT foo.id INTO bar FROM foo"; + + private static final String SQL_SELECT_FOR_UPDATE = "SELECT foo.id FROM foo FOR UPDATE"; + + private static final String SQL_SELECT_FOR_SHARE = "SELECT foo.id FROM foo FOR SHARE"; + + private static final String SQL_UNION_FOR_UPDATE = """ + SELECT foo.id FROM foo UNION SELECT fooFact.id FROM fooFact FOR UPDATE"""; + + private static final String SELECT_INTO_IS_NOT_PERMITTED = "SELECT INTO is not permitted."; + + private static final String ROW_LOCKING_IS_NOT_PERMITTED = "Row locking (FOR UPDATE/SHARE) is not permitted."; private static final String SQL_LEFT_JOIN_ON_WRONG_COLUMN = """ SELECT foo.id FROM foo LEFT JOIN fooFact ON foo.wrongCol = fooFact.id"""; @@ -1089,6 +1102,50 @@ void testValidCteStillPasses(@InjectService SqlGuardFactory sqlGuardFactory) thr } } + @Nested + @DisplayName("Side-Effecting Select Security Tests (SELECT INTO / FOR UPDATE)") + class SideEffectingSelectSecurityTests { + + // These parse as a Select but are not read-only: SELECT INTO writes a table, + // FOR UPDATE/SHARE takes row locks. They must be rejected by the read-only gate. + + @Test + void testSelectInto(@InjectService SqlGuardFactory sqlGuardFactory) throws Exception { + DatabaseCatalog databaseCatalog = schemaWithTwoTableTwoCol(); + SqlGuard guard = sqlGuardFactory.create("", SCH, databaseCatalog, List.of(), dialect); + assertThatThrownBy(() -> guard.guard(SQL_SELECT_INTO)) + .isInstanceOf(GuardException.class) + .hasMessage(SELECT_INTO_IS_NOT_PERMITTED); + } + + @Test + void testSelectForUpdate(@InjectService SqlGuardFactory sqlGuardFactory) throws Exception { + DatabaseCatalog databaseCatalog = schemaWithTwoTableTwoCol(); + SqlGuard guard = sqlGuardFactory.create("", SCH, databaseCatalog, List.of(), dialect); + assertThatThrownBy(() -> guard.guard(SQL_SELECT_FOR_UPDATE)) + .isInstanceOf(GuardException.class) + .hasMessage(ROW_LOCKING_IS_NOT_PERMITTED); + } + + @Test + void testSelectForShare(@InjectService SqlGuardFactory sqlGuardFactory) throws Exception { + DatabaseCatalog databaseCatalog = schemaWithTwoTableTwoCol(); + SqlGuard guard = sqlGuardFactory.create("", SCH, databaseCatalog, List.of(), dialect); + assertThatThrownBy(() -> guard.guard(SQL_SELECT_FOR_SHARE)) + .isInstanceOf(GuardException.class) + .hasMessage(ROW_LOCKING_IS_NOT_PERMITTED); + } + + @Test + void testUnionForUpdate(@InjectService SqlGuardFactory sqlGuardFactory) throws Exception { + DatabaseCatalog databaseCatalog = schemaWithTwoTableTwoCol(); + SqlGuard guard = sqlGuardFactory.create("", SCH, databaseCatalog, List.of(), dialect); + assertThatThrownBy(() -> guard.guard(SQL_UNION_FOR_UPDATE)) + .isInstanceOf(GuardException.class) + .hasMessage(ROW_LOCKING_IS_NOT_PERMITTED); + } + } + @Nested @DisplayName("JOIN Security Tests") class JoinSecurityTests {