From 44d566faf2ba28ae7b01e46f2b8a13009af01573 Mon Sep 17 00:00:00 2001 From: Chad Bentz <1760475+felickz@users.noreply.github.com> Date: Mon, 3 Aug 2026 12:25:21 -0400 Subject: [PATCH] Add Java SQL injection sinks for Spring R2DBC and io.r2dbc.spi Models Spring Data R2DBC's DatabaseClient and the underlying io.r2dbc.spi SPI as SQL injection sinks in codeql/java-all, closing a false negative where unvalidated values concatenated into SQL text and executed via DatabaseClient.sql(String) were not flagged by java/sql-injection. Adds 4 new sql-injection sink rows across two manual data extension files: - java/ext/manual/org.springframework.r2dbc.model.yml - DatabaseClient.sql(String) -> Argument[0] - DatabaseClient.sql(Supplier) -> the deferred/lazy overload, modeled via a summaryModel that bridges taint from the supplier's return value onto the GenericExecuteSpec object, with the sink placed on the terminal fetch() call's receiver - java/ext/manual/io.r2dbc.spi.model.yml - Connection.createStatement(String) -> Argument[0] - Batch.add(String) -> Argument[0] io.r2dbc.spi is the low-level, driver-agnostic SPI that DatabaseClient (and every other reactive relational driver) is built on top of, so both files are needed for full coverage of code that either uses Spring's higher-level API or drops down to the raw R2DBC SPI directly. Adds java/test/security/CWE-089/spring-r2dbc/ with self-contained stub sources for both APIs and a DatabaseClientSqlInjection.ql test validating all 4 sinks (plus safe/negative parameter-binding cases) via the real QueryInjectionFlow::flow used by the standard java/sql-injection query. Bumps java/ext's qlpack.yml version (0.7.1 -> 0.8.0) per CONTRIBUTING.md. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f02ef23b-e35d-4536-bd58-f2fbc2976ba9 --- java/ext/manual/io.r2dbc.spi.model.yml | 21 +++++++ .../org.springframework.r2dbc.model.yml | 28 ++++++++++ java/ext/qlpack.yml | 2 +- .../DatabaseClientSqlInjection.expected | 6 ++ .../DatabaseClientSqlInjection.ql | 28 ++++++++++ .../com/example/DeferredQueryHandler.java | 30 ++++++++++ .../com/example/RawR2dbcHandler.java | 50 +++++++++++++++++ .../com/example/StringConcatQueryHandler.java | 55 +++++++++++++++++++ .../spring-r2dbc/io/r2dbc/spi/Batch.java | 12 ++++ .../spring-r2dbc/io/r2dbc/spi/Connection.java | 12 ++++ .../spring-r2dbc/io/r2dbc/spi/Result.java | 8 +++ .../spring-r2dbc/io/r2dbc/spi/Statement.java | 20 +++++++ .../r2dbc/core/DatabaseClient.java | 26 +++++++++ 13 files changed, 297 insertions(+), 1 deletion(-) create mode 100644 java/ext/manual/io.r2dbc.spi.model.yml create mode 100644 java/ext/manual/org.springframework.r2dbc.model.yml create mode 100644 java/test/security/CWE-089/spring-r2dbc/DatabaseClientSqlInjection.expected create mode 100644 java/test/security/CWE-089/spring-r2dbc/DatabaseClientSqlInjection.ql create mode 100644 java/test/security/CWE-089/spring-r2dbc/com/example/DeferredQueryHandler.java create mode 100644 java/test/security/CWE-089/spring-r2dbc/com/example/RawR2dbcHandler.java create mode 100644 java/test/security/CWE-089/spring-r2dbc/com/example/StringConcatQueryHandler.java create mode 100644 java/test/security/CWE-089/spring-r2dbc/io/r2dbc/spi/Batch.java create mode 100644 java/test/security/CWE-089/spring-r2dbc/io/r2dbc/spi/Connection.java create mode 100644 java/test/security/CWE-089/spring-r2dbc/io/r2dbc/spi/Result.java create mode 100644 java/test/security/CWE-089/spring-r2dbc/io/r2dbc/spi/Statement.java create mode 100644 java/test/security/CWE-089/spring-r2dbc/org/springframework/r2dbc/core/DatabaseClient.java diff --git a/java/ext/manual/io.r2dbc.spi.model.yml b/java/ext/manual/io.r2dbc.spi.model.yml new file mode 100644 index 00000000..4173ce28 --- /dev/null +++ b/java/ext/manual/io.r2dbc.spi.model.yml @@ -0,0 +1,21 @@ +extensions: + - addsTo: + pack: codeql/java-all + extensible: sinkModel + data: + # `io.r2dbc.spi` is the low-level, driver-agnostic R2DBC SPI that every + # reactive relational driver (Postgres, MySQL, H2, SQL Server, ...) + # implements, and that higher-level APIs such as Spring's + # `DatabaseClient` are themselves built on top of. Modeling these two + # sinks closes the false negative for any code (Spring-based or not) + # that drops down to the raw R2DBC SPI to build/execute SQL text. + # + # `Connection.createStatement(String)` is the R2DBC equivalent of + # `java.sql.Connection.prepareStatement(String)`: the SQL text is fixed + # at statement-creation time and executed (with any bound parameters) + # via `Statement.execute()`. + - ["io.r2dbc.spi", "Connection", True, "createStatement", "(String)", "", "Argument[0]", "sql-injection", "manual"] + # `Batch.add(String)` is the R2DBC equivalent of + # `java.sql.Statement.addBatch(String)`: it appends a raw SQL statement + # to a batch that is later executed via `Batch.execute()`. + - ["io.r2dbc.spi", "Batch", True, "add", "(String)", "", "Argument[0]", "sql-injection", "manual"] diff --git a/java/ext/manual/org.springframework.r2dbc.model.yml b/java/ext/manual/org.springframework.r2dbc.model.yml new file mode 100644 index 00000000..ba96da19 --- /dev/null +++ b/java/ext/manual/org.springframework.r2dbc.model.yml @@ -0,0 +1,28 @@ +extensions: + - addsTo: + pack: codeql/java-all + extensible: sinkModel + data: + # `DatabaseClient.sql(String)` executes the given SQL text. Neither the + # SQL text nor any subsequently bound parameters are validated by the + # framework, so an unvalidated string reaching this argument is a SQL + # injection sink. + - ["org.springframework.r2dbc.core", "DatabaseClient", True, "sql", "(String)", "", "Argument[0]", "sql-injection", "manual"] + # `DatabaseClient.sql(Supplier)` defers resolution of the SQL + # text until execution. The supplier's return value is not itself an + # argument/return position that MaD can target directly as a sink, so + # taint is instead propagated (via the summaryModel below) onto the + # `GenericExecuteSpec` object returned by `sql(...)`. The sink is + # placed on `fetch()`, the point where the deferred query is finalized + # and enters the fluent execution chain; actual execution remains + # reactive/deferred until the returned publisher is subscribed to, but + # the tainted SQL text has already been captured by that point. + - ["org.springframework.r2dbc.core", "DatabaseClient$GenericExecuteSpec", True, "fetch", "", "", "Argument[this]", "sql-injection", "manual"] + - addsTo: + pack: codeql/java-all + extensible: summaryModel + data: + # Propagate taint from the deferred SQL supplier's return value onto + # the `GenericExecuteSpec` returned by `sql(Supplier)`, so it + # can reach the `fetch()` sink modeled above. + - ["org.springframework.r2dbc.core", "DatabaseClient", True, "sql", "(java.util.function.Supplier)", "", "Argument[0].ReturnValue", "ReturnValue", "taint", "manual"] diff --git a/java/ext/qlpack.yml b/java/ext/qlpack.yml index 77f05cb4..767f96f3 100644 --- a/java/ext/qlpack.yml +++ b/java/ext/qlpack.yml @@ -1,6 +1,6 @@ library: true name: githubsecuritylab/codeql-java-extensions -version: 0.7.1 +version: 0.8.0 extensionTargets: codeql/java-all: '9.2.1' dataExtensions: diff --git a/java/test/security/CWE-089/spring-r2dbc/DatabaseClientSqlInjection.expected b/java/test/security/CWE-089/spring-r2dbc/DatabaseClientSqlInjection.expected new file mode 100644 index 00000000..8b6f4acb --- /dev/null +++ b/java/test/security/CWE-089/spring-r2dbc/DatabaseClientSqlInjection.expected @@ -0,0 +1,6 @@ +| com/example/DeferredQueryHandler.java:24:23:24:30 | source(...) | com/example/DeferredQueryHandler.java:25:12:26:77 | sql(...) | +| com/example/RawR2dbcHandler.java:28:23:28:30 | source(...) | com/example/RawR2dbcHandler.java:31:13:31:69 | ... + ... | +| com/example/RawR2dbcHandler.java:36:23:36:30 | source(...) | com/example/RawR2dbcHandler.java:38:15:38:71 | ... + ... | +| com/example/StringConcatQueryHandler.java:39:23:39:30 | source(...) | com/example/StringConcatQueryHandler.java:43:31:43:35 | query | +| com/example/StringConcatQueryHandler.java:40:21:40:28 | source(...) | com/example/StringConcatQueryHandler.java:43:31:43:35 | query | +| com/example/StringConcatQueryHandler.java:41:24:41:31 | source(...) | com/example/StringConcatQueryHandler.java:43:31:43:35 | query | diff --git a/java/test/security/CWE-089/spring-r2dbc/DatabaseClientSqlInjection.ql b/java/test/security/CWE-089/spring-r2dbc/DatabaseClientSqlInjection.ql new file mode 100644 index 00000000..438ab906 --- /dev/null +++ b/java/test/security/CWE-089/spring-r2dbc/DatabaseClientSqlInjection.ql @@ -0,0 +1,28 @@ +/** + * Test that the standard `java/sql-injection` query (`QueryInjectionFlow`, + * from `semmle.code.java.security.SqlInjectionQuery`) recognizes taint + * reaching the Spring R2DBC (`org.springframework.r2dbc.core.DatabaseClient`) + * and raw R2DBC SPI (`io.r2dbc.spi`) sinks modeled in + * `java/ext/manual/org.springframework.r2dbc.model.yml` and + * `java/ext/manual/io.r2dbc.spi.model.yml`. + */ + +import java +import semmle.code.java.dataflow.FlowSources +import semmle.code.java.security.SqlInjectionQuery + +/** + * A fake remote flow source used by the test code to mark a value as + * tainted, matching calls to a `source()` method. This isolates the test + * from real-world remote-flow-source modeling (e.g. `@RequestParam`) so it + * focuses purely on validating the new sink models. + */ +private class SourceMethodSource extends RemoteFlowSource { + SourceMethodSource() { this.asExpr().(MethodCall).getMethod().hasName("source") } + + override string getSourceType() { result = "source" } +} + +from DataFlow::Node source, DataFlow::Node sink +where QueryInjectionFlow::flow(source, sink) +select source, sink diff --git a/java/test/security/CWE-089/spring-r2dbc/com/example/DeferredQueryHandler.java b/java/test/security/CWE-089/spring-r2dbc/com/example/DeferredQueryHandler.java new file mode 100644 index 00000000..14bc3ce0 --- /dev/null +++ b/java/test/security/CWE-089/spring-r2dbc/com/example/DeferredQueryHandler.java @@ -0,0 +1,30 @@ +package com.example; + +import org.springframework.r2dbc.core.DatabaseClient; + +/** + * Test source for the `DatabaseClient.sql(Supplier)` overload: SQL + * text is deferred behind a lambda instead of being passed directly as a + * String. + */ +public class DeferredQueryHandler { + + // Fake remote flow source, recognized by DatabaseClientSqlInjection.ql. + public static String source() { + return null; + } + + private final DatabaseClient databaseClient; + + public DeferredQueryHandler(DatabaseClient databaseClient) { + this.databaseClient = databaseClient; + } + + public Object runDeferredQuery() { + String category = source(); + return databaseClient + .sql(() -> "SELECT * FROM items WHERE category = '" + category + "'") + .fetch() + .all(); // sink 2: sql(Supplier) + } +} diff --git a/java/test/security/CWE-089/spring-r2dbc/com/example/RawR2dbcHandler.java b/java/test/security/CWE-089/spring-r2dbc/com/example/RawR2dbcHandler.java new file mode 100644 index 00000000..7bf72133 --- /dev/null +++ b/java/test/security/CWE-089/spring-r2dbc/com/example/RawR2dbcHandler.java @@ -0,0 +1,50 @@ +package com.example; + +import io.r2dbc.spi.Batch; +import io.r2dbc.spi.Connection; +import io.r2dbc.spi.Result; +import io.r2dbc.spi.Statement; + +/** + * Test source for the low-level `io.r2dbc.spi` SPI that Spring's + * `DatabaseClient` (and every other reactive relational driver) is built on + * top of. Code that drops down to this API to build/execute SQL text bypasses + * `DatabaseClient` entirely, so it needs its own sink models. + */ +public class RawR2dbcHandler { + + // Fake remote flow source, recognized by DatabaseClientSqlInjection.ql. + public static String source() { + return null; + } + + private final Connection connection; + + public RawR2dbcHandler(Connection connection) { + this.connection = connection; + } + + public Result runStatement() { + String category = source(); + Statement statement = + connection.createStatement( // sink 3: Connection.createStatement(String) + "SELECT * FROM items WHERE category = '" + category + "'"); + return statement.execute(); + } + + public Result runBatch() { + String category = source(); + Batch batch = connection.createBatch(); + batch.add("SELECT * FROM items WHERE category = '" + category + "'"); // sink 4: Batch.add(String) + return batch.execute(); + } + + public Result runStatementSafe() { + // Parameter binding, not string concatenation - should NOT be flagged. + String category = source(); + Statement statement = + connection.createStatement("SELECT * FROM items WHERE category = $1"); + statement.bind(0, category); + return statement.execute(); + } +} diff --git a/java/test/security/CWE-089/spring-r2dbc/com/example/StringConcatQueryHandler.java b/java/test/security/CWE-089/spring-r2dbc/com/example/StringConcatQueryHandler.java new file mode 100644 index 00000000..f160f048 --- /dev/null +++ b/java/test/security/CWE-089/spring-r2dbc/com/example/StringConcatQueryHandler.java @@ -0,0 +1,55 @@ +package com.example; + +import org.springframework.r2dbc.core.DatabaseClient; + +/** + * Test source for `DatabaseClient.sql(String)`: request-derived values are + * concatenated into a SQL string via a `StringBuilder`, then executed + * directly. + */ +public class StringConcatQueryHandler { + + // Fake remote flow source, recognized by DatabaseClientSqlInjection.ql. + public static String source() { + return null; + } + + private final DatabaseClient databaseClient; + + public StringConcatQueryHandler(DatabaseClient databaseClient) { + this.databaseClient = databaseClient; + } + + private void appendFilters(StringBuilder sql, String category, String sortBy, String sortOrder) { + if (category != null) { + sql.append(" AND category = '").append(category).append("'"); + } + if (sortBy != null && sortOrder != null) { + sql.append(" ORDER BY ").append(sortBy).append(" ").append(sortOrder); + } + } + + private String buildQuery(String category, String sortBy, String sortOrder) { + StringBuilder sql = new StringBuilder("SELECT * FROM items WHERE 1=1"); + appendFilters(sql, category, sortBy, sortOrder); + return sql.toString(); + } + + public Object runQuery() { + String category = source(); + String sortBy = source(); + String sortOrder = source(); + String query = buildQuery(category, sortBy, sortOrder); + return databaseClient.sql(query).fetch().all(); // sink 1: sql(String) + } + + public Object runQuerySafe() { + // Parameter binding, not string concatenation - should NOT be flagged. + String category = source(); + return databaseClient + .sql("SELECT * FROM items WHERE category = :category") + .bind("category", category) + .fetch() + .all(); + } +} diff --git a/java/test/security/CWE-089/spring-r2dbc/io/r2dbc/spi/Batch.java b/java/test/security/CWE-089/spring-r2dbc/io/r2dbc/spi/Batch.java new file mode 100644 index 00000000..63d0d7f7 --- /dev/null +++ b/java/test/security/CWE-089/spring-r2dbc/io/r2dbc/spi/Batch.java @@ -0,0 +1,12 @@ +/* + * Minimal stub of the `io.r2dbc.spi` SPI, used only to compile the test + * source in this directory. Not the real implementation. + */ +package io.r2dbc.spi; + +public interface Batch { + + Batch add(String sql); + + Result execute(); +} diff --git a/java/test/security/CWE-089/spring-r2dbc/io/r2dbc/spi/Connection.java b/java/test/security/CWE-089/spring-r2dbc/io/r2dbc/spi/Connection.java new file mode 100644 index 00000000..bfec8d88 --- /dev/null +++ b/java/test/security/CWE-089/spring-r2dbc/io/r2dbc/spi/Connection.java @@ -0,0 +1,12 @@ +/* + * Minimal stub of the `io.r2dbc.spi` SPI, used only to compile the test + * source in this directory. Not the real implementation. + */ +package io.r2dbc.spi; + +public interface Connection { + + Statement createStatement(String sql); + + Batch createBatch(); +} diff --git a/java/test/security/CWE-089/spring-r2dbc/io/r2dbc/spi/Result.java b/java/test/security/CWE-089/spring-r2dbc/io/r2dbc/spi/Result.java new file mode 100644 index 00000000..c8fc9764 --- /dev/null +++ b/java/test/security/CWE-089/spring-r2dbc/io/r2dbc/spi/Result.java @@ -0,0 +1,8 @@ +/* + * Minimal stub of the `io.r2dbc.spi` SPI, used only to compile the test + * source in this directory. Not the real implementation. + */ +package io.r2dbc.spi; + +public interface Result { +} diff --git a/java/test/security/CWE-089/spring-r2dbc/io/r2dbc/spi/Statement.java b/java/test/security/CWE-089/spring-r2dbc/io/r2dbc/spi/Statement.java new file mode 100644 index 00000000..f399c340 --- /dev/null +++ b/java/test/security/CWE-089/spring-r2dbc/io/r2dbc/spi/Statement.java @@ -0,0 +1,20 @@ +/* + * Minimal stub of the `io.r2dbc.spi` SPI, used only to compile the test + * source in this directory. Not the real implementation. + */ +package io.r2dbc.spi; + +public interface Statement { + + Statement bind(int index, Object value); + + Statement bind(String name, Object value); + + Statement bindNull(int index, Class type); + + Statement bindNull(String name, Class type); + + Statement add(); + + Result execute(); +} diff --git a/java/test/security/CWE-089/spring-r2dbc/org/springframework/r2dbc/core/DatabaseClient.java b/java/test/security/CWE-089/spring-r2dbc/org/springframework/r2dbc/core/DatabaseClient.java new file mode 100644 index 00000000..8f5f8448 --- /dev/null +++ b/java/test/security/CWE-089/spring-r2dbc/org/springframework/r2dbc/core/DatabaseClient.java @@ -0,0 +1,26 @@ +/* + * Minimal stub of the Spring Data R2DBC `DatabaseClient` API, used only to + * compile the test source in this directory. Not the real implementation. + */ +package org.springframework.r2dbc.core; + +import java.util.function.Supplier; + +public interface DatabaseClient { + + GenericExecuteSpec sql(String sql); + + GenericExecuteSpec sql(Supplier sqlSupplier); + + interface GenericExecuteSpec { + GenericExecuteSpec bind(int index, Object value); + + GenericExecuteSpec bind(String name, Object value); + + FetchSpec fetch(); + } + + interface FetchSpec { + Object all(); + } +}