Fix false negative: full SQL-injection sink coverage for Spring R2DBC + io.r2dbc.spi

java/ext/manual/io.r2dbc.spi.model.yml

@@ -6,9 +6,10 @@ extensions:
       # `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.
+      # `DatabaseClient` are themselves built on top of. Modeling these 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 or
+      # identifiers.
       #
       # `Connection.createStatement(String)` is the R2DBC equivalent of
       # `java.sql.Connection.prepareStatement(String)`: the SQL text is fixed
@@ -19,3 +20,24 @@ extensions:
       # `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"]
+      # `Connection.createSavepoint`/`releaseSavepoint`/
+      # `rollbackTransactionToSavepoint` each take a raw `name` that
+      # identifies a savepoint. The SQL wire protocol has no parameter
+      # placeholder for identifiers (only for literal values), so drivers
+      # build the executed command by splicing `name` directly into text
+      # such as `SAVEPOINT <name>`. Verified against r2dbc-postgresql
+      # 1.0.2.RELEASE (`PostgresqlConnection`), which does exactly this via
+      # unescaped `String.format(...)` - a tainted savepoint name is
+      # classic SQL injection (e.g. `x; DROP TABLE users; --`).
+      - ["io.r2dbc.spi", "Connection", True, "createSavepoint", "(String)", "", "Argument[0]", "sql-injection", "manual"]
+      - ["io.r2dbc.spi", "Connection", True, "releaseSavepoint", "(String)", "", "Argument[0]", "sql-injection", "manual"]
+      - ["io.r2dbc.spi", "Connection", True, "rollbackTransactionToSavepoint", "(String)", "", "Argument[0]", "sql-injection", "manual"]
+      # `Statement.returnGeneratedValues(String...)` takes raw column names
+      # to request back after an INSERT/UPDATE/DELETE. For the same reason
+      # (no placeholder syntax for identifiers), drivers append these names
+      # directly into the executed SQL's `RETURNING` clause. Verified
+      # against r2dbc-postgresql 1.0.2.RELEASE
+      # (`GeneratedValuesUtils.augment`), which does
+      # `String.format("%s RETURNING %s", sql, String.join(", ", columns))`
+      # with zero escaping or validation of the column names.
+      - ["io.r2dbc.spi", "Statement", True, "returnGeneratedValues", "(String[])", "", "Argument[0]", "sql-injection", "manual"]

java/ext/manual/org.springframework.r2dbc.model.yml

@@ -12,17 +12,43 @@ extensions:
       # 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.
+      # `GenericExecuteSpec` object returned by `sql(...)`. Sinks are then
+      # placed on every method of `GenericExecuteSpec` that actually enters
+      # the execution stage (per Spring's own Javadoc wording) - `fetch()`,
+      # `then()`, `map(...)`/`mapValue(...)`/`mapProperties(...)`, and
+      # `flatMap(...)` - since any of them, called on a tainted receiver,
+      # means the tainted SQL text has already been captured. Actual
+      # execution remains reactive/deferred until the returned publisher is
+      # subscribed to, but that doesn't change the taint result.
       - ["org.springframework.r2dbc.core", "DatabaseClient$GenericExecuteSpec", True, "fetch", "", "", "Argument[this]", "sql-injection", "manual"]
+      - ["org.springframework.r2dbc.core", "DatabaseClient$GenericExecuteSpec", True, "then", "", "", "Argument[this]", "sql-injection", "manual"]
+      - ["org.springframework.r2dbc.core", "DatabaseClient$GenericExecuteSpec", True, "map", "(java.util.function.Function)", "", "Argument[this]", "sql-injection", "manual"]
+      - ["org.springframework.r2dbc.core", "DatabaseClient$GenericExecuteSpec", True, "map", "(java.util.function.BiFunction)", "", "Argument[this]", "sql-injection", "manual"]
+      - ["org.springframework.r2dbc.core", "DatabaseClient$GenericExecuteSpec", True, "mapValue", "(Class)", "", "Argument[this]", "sql-injection", "manual"]
+      - ["org.springframework.r2dbc.core", "DatabaseClient$GenericExecuteSpec", True, "mapProperties", "(Class)", "", "Argument[this]", "sql-injection", "manual"]
+      - ["org.springframework.r2dbc.core", "DatabaseClient$GenericExecuteSpec", True, "flatMap", "(java.util.function.Function)", "", "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<String>)`, so it
-      # can reach the `fetch()` sink modeled above.
+      # can reach the execution-stage sinks modeled above.
       - ["org.springframework.r2dbc.core", "DatabaseClient", True, "sql", "(java.util.function.Supplier)", "", "Argument[0].ReturnValue", "ReturnValue", "taint", "manual"]
+      # `GenericExecuteSpec`'s parameter-binding/filtering methods are
+      # fluent - they return (a possibly new) `GenericExecuteSpec` - and
+      # must NOT break the taint chain from the deferred-supplier summary
+      # above through to the execution-stage sinks. These propagate the
+      # receiver's own taint (from the deferred-supplier path) onto the
+      # returned `GenericExecuteSpec`; they do not, and should not, taint
+      # the receiver based on the *bound parameter value* itself, since
+      # bound parameters are safely escaped by the driver.
+      - ["org.springframework.r2dbc.core", "DatabaseClient$GenericExecuteSpec", True, "bind", "(int,Object)", "", "Argument[this]", "ReturnValue", "taint", "manual"]
+      - ["org.springframework.r2dbc.core", "DatabaseClient$GenericExecuteSpec", True, "bind", "(String,Object)", "", "Argument[this]", "ReturnValue", "taint", "manual"]
+      - ["org.springframework.r2dbc.core", "DatabaseClient$GenericExecuteSpec", True, "bindNull", "(int,Class)", "", "Argument[this]", "ReturnValue", "taint", "manual"]
+      - ["org.springframework.r2dbc.core", "DatabaseClient$GenericExecuteSpec", True, "bindNull", "(String,Class)", "", "Argument[this]", "ReturnValue", "taint", "manual"]
+      - ["org.springframework.r2dbc.core", "DatabaseClient$GenericExecuteSpec", True, "bindValues", "(List)", "", "Argument[this]", "ReturnValue", "taint", "manual"]
+      - ["org.springframework.r2dbc.core", "DatabaseClient$GenericExecuteSpec", True, "bindValues", "(Map)", "", "Argument[this]", "ReturnValue", "taint", "manual"]
+      - ["org.springframework.r2dbc.core", "DatabaseClient$GenericExecuteSpec", True, "bindProperties", "(Object)", "", "Argument[this]", "ReturnValue", "taint", "manual"]
+      - ["org.springframework.r2dbc.core", "DatabaseClient$GenericExecuteSpec", True, "filter", "(java.util.function.Function)", "", "Argument[this]", "ReturnValue", "taint", "manual"]
+      - ["org.springframework.r2dbc.core", "DatabaseClient$GenericExecuteSpec", True, "filter", "(org.springframework.r2dbc.core.StatementFilterFunction)", "", "Argument[this]", "ReturnValue", "taint", "manual"]

java/test/security/CWE-089/spring-r2dbc/DatabaseClientSqlInjection.expected

@@ -1,6 +1,21 @@
 | com/example/DeferredQueryHandler.java:24:23:24:30 | source(...) | com/example/DeferredQueryHandler.java:25:12:26:77 | sql(...) |
+| com/example/DeferredQueryHandler.java:33:23:33:30 | source(...) | com/example/DeferredQueryHandler.java:34:12:35:77 | sql(...) |
+| com/example/DeferredQueryHandler.java:42:23:42:30 | source(...) | com/example/DeferredQueryHandler.java:43:12:44:77 | sql(...) |
+| com/example/DeferredQueryHandler.java:52:23:52:30 | source(...) | com/example/DeferredQueryHandler.java:53:12:55:35 | bind(...) |
+| com/example/DeferredQueryHandler.java:64:23:64:30 | source(...) | com/example/DeferredQueryHandler.java:65:12:67:41 | bind(...) |
+| com/example/DeferredQueryHandler.java:75:23:75:30 | source(...) | com/example/DeferredQueryHandler.java:76:12:77:77 | sql(...) |
+| com/example/DeferredQueryHandler.java:83:23:83:30 | source(...) | com/example/DeferredQueryHandler.java:84:12:85:77 | sql(...) |
+| com/example/DeferredQueryHandler.java:94:23:94:30 | source(...) | com/example/DeferredQueryHandler.java:95:12:100:29 | filter(...) |
+| com/example/DeferredQueryHandler.java:107:23:107:30 | source(...) | com/example/DeferredQueryHandler.java:108:12:110:40 | bindNull(...) |
+| com/example/DeferredQueryHandler.java:117:23:117:30 | source(...) | com/example/DeferredQueryHandler.java:118:12:120:59 | bindValues(...) |
+| com/example/DeferredQueryHandler.java:127:23:127:30 | source(...) | com/example/DeferredQueryHandler.java:128:12:130:78 | filter(...) |
+| com/example/DeferredQueryHandler.java:137:23:137:30 | source(...) | com/example/DeferredQueryHandler.java:138:12:139: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/RawR2dbcHandler.java:54:19:54:26 | source(...) | com/example/RawR2dbcHandler.java:55:32:55:35 | name |
+| com/example/RawR2dbcHandler.java:59:19:59:26 | source(...) | com/example/RawR2dbcHandler.java:60:33:60:36 | name |
+| com/example/RawR2dbcHandler.java:64:19:64:26 | source(...) | com/example/RawR2dbcHandler.java:65:47:65:50 | name |
+| com/example/RawR2dbcHandler.java:72:21:72:28 | source(...) | com/example/RawR2dbcHandler.java:74:12:74:50 | new ..[] { .. } |
 | 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 |

java/test/security/CWE-089/spring-r2dbc/com/example/DeferredQueryHandler.java

@@ -27,4 +27,117 @@ public Object runDeferredQuery() {
         .fetch()
         .all(); // sink 2: sql(Supplier<String>)
   }
+
+  // Alternative terminal/execution-triggering method to fetch(): then().
+  public Object runDeferredQueryThen() {
+    String category = source();
+    return databaseClient
+        .sql(() -> "SELECT * FROM items WHERE category = '" + category + "'")
+        .then(); // sink: sql(Supplier<String>) via then()
+  }
+
+  // Alternative terminal method: map(...).all() instead of fetch().all(),
+  // matching the exact chain from the original real-world report.
+  public Object runDeferredQueryWithMap() {
+    String category = source();
+    return databaseClient
+        .sql(() -> "SELECT * FROM items WHERE category = '" + category + "'")
+        .map(row -> row)
+        .all(); // sink: sql(Supplier<String>) via map()
+  }
+
+  // Intermediate fluent bind() call between the deferred sql() and the
+  // terminal fetch() - taint must survive the fluent chain.
+  public Object runDeferredQueryWithBind() {
+    String category = source();
+    return databaseClient
+        .sql(() -> "SELECT * FROM items WHERE category = '" + category + "'")
+        .bind(0, "unrelated-value")
+        .fetch()
+        .all(); // sink: sql(Supplier<String>) via bind().fetch()
+  }
+
+  // Same as above but exercising the String-named bind() overload, to
+  // confirm the summary model's signature string matches the correct
+  // overload (not just the int-indexed one).
+  public Object runDeferredQueryWithBindByName() {
+    String category = source();
+    return databaseClient
+        .sql(() -> "SELECT * FROM items WHERE category = '" + category + "'")
+        .bind("param", "unrelated-value")
+        .fetch()
+        .all(); // sink: sql(Supplier<String>) via bind(String,Object).fetch()
+  }
+
+  // Alternative terminal method: flatMap(...), which returns Flux<R>
+  // directly instead of an intermediate RowsFetchSpec.
+  public Object runDeferredQueryWithFlatMap() {
+    String category = source();
+    return databaseClient
+        .sql(() -> "SELECT * FROM items WHERE category = '" + category + "'")
+        .flatMap(row -> row); // sink: sql(Supplier<String>) via flatMap()
+  }
+
+  // Alternative terminal method: map(BiFunction).
+  public Object runDeferredQueryWithMapBiFunction() {
+    String category = source();
+    return databaseClient
+        .sql(() -> "SELECT * FROM items WHERE category = '" + category + "'")
+        .map((row, meta) -> row)
+        .all(); // sink: sql(Supplier<String>) via map(BiFunction)
+  }
+
+  // Chains bindNull(int,Class), bindValues(List), bindProperties(Object),
+  // and filter(Function) before the mapValue(Class) terminal sink -
+  // validates the remaining summary/sink signature strings in one flow.
+  public Object runDeferredQueryWithBindNullAndFilter() {
+    String category = source();
+    return databaseClient
+        .sql(() -> "SELECT * FROM items WHERE category = '" + category + "'")
+        .bindNull(1, String.class)
+        .bindValues(java.util.List.of("unrelated"))
+        .bindProperties(new Object())
+        .filter(spec -> spec)
+        .mapValue(String.class)
+        .all(); // sink: sql(Supplier<String>) via mapValue() after bindNull/bindValues(List)/bindProperties/filter(Function)
+  }
+
+  // Isolated: bindNull(String,Class) alone.
+  public Object runDeferredQueryWithBindNullByName() {
+    String category = source();
+    return databaseClient
+        .sql(() -> "SELECT * FROM items WHERE category = '" + category + "'")
+        .bindNull("param", String.class)
+        .fetch()
+        .all(); // sink: sql(Supplier<String>) via bindNull(String,Class).fetch()
+  }
+
+  // Isolated: bindValues(Map) alone.
+  public Object runDeferredQueryWithBindValuesMap() {
+    String category = source();
+    return databaseClient
+        .sql(() -> "SELECT * FROM items WHERE category = '" + category + "'")
+        .bindValues(java.util.Map.of("param", "unrelated"))
+        .fetch()
+        .all(); // sink: sql(Supplier<String>) via bindValues(Map).fetch()
+  }
+
+  // Isolated: filter(StatementFilterFunction) alone.
+  public Object runDeferredQueryWithFilterOverload() {
+    String category = source();
+    return databaseClient
+        .sql(() -> "SELECT * FROM items WHERE category = '" + category + "'")
+        .filter((org.springframework.r2dbc.core.StatementFilterFunction) null)
+        .fetch()
+        .all(); // sink: sql(Supplier<String>) via filter(StatementFilterFunction).fetch()
+  }
+
+  // Isolated: mapProperties(Class) alone.
+  public Object runDeferredQueryWithMapProperties() {
+    String category = source();
+    return databaseClient
+        .sql(() -> "SELECT * FROM items WHERE category = '" + category + "'")
+        .mapProperties(String.class)
+        .all(); // sink: sql(Supplier<String>) via mapProperties()
+  }
 }

java/test/security/CWE-089/spring-r2dbc/com/example/RawR2dbcHandler.java

@@ -47,4 +47,30 @@ public Result runStatementSafe() {
     statement.bind(0, category);
     return statement.execute();
   }
+
+  public void runCreateSavepoint() {
+    // Real drivers (e.g. r2dbc-postgresql) build this as
+    // `String.format("SAVEPOINT %s", name)` with no escaping/validation.
+    String name = source();
+    connection.createSavepoint(name); // sink 5: Connection.createSavepoint(String)
+  }
+
+  public void runReleaseSavepoint() {
+    String name = source();
+    connection.releaseSavepoint(name); // sink 6: Connection.releaseSavepoint(String)
+  }
+
+  public void runRollbackTransactionToSavepoint() {
+    String name = source();
+    connection.rollbackTransactionToSavepoint(name); // sink 7: Connection.rollbackTransactionToSavepoint(String)
+  }
+
+  public Result runReturnGeneratedValues() {
+    // Real drivers (e.g. r2dbc-postgresql) build this as
+    // `String.format("%s RETURNING %s", sql, String.join(", ", columns))`
+    // with no escaping/validation of the column names.
+    String column = source();
+    Statement statement = connection.createStatement("INSERT INTO items (name) VALUES ($1)");
+    return statement.returnGeneratedValues(column).execute(); // sink 8: Statement.returnGeneratedValues(String...)
+  }
 }

java/test/security/CWE-089/spring-r2dbc/io/r2dbc/spi/Connection.java

@@ -9,4 +9,10 @@ public interface Connection {
   Statement createStatement(String sql);
 
   Batch createBatch();
+
+  void createSavepoint(String name);
+
+  void releaseSavepoint(String name);
+
+  void rollbackTransactionToSavepoint(String name);
 }

java/test/security/CWE-089/spring-r2dbc/io/r2dbc/spi/Statement.java

@@ -16,5 +16,7 @@ public interface Statement {
 
   Statement add();
 
+  Statement returnGeneratedValues(String... columns);
+
   Result execute();
 }

java/test/security/CWE-089/spring-r2dbc/org/springframework/r2dbc/core/DatabaseClient.java

@@ -4,6 +4,10 @@
  */
 package org.springframework.r2dbc.core;
 
+import java.util.List;
+import java.util.Map;
+import java.util.function.BiFunction;
+import java.util.function.Function;
 import java.util.function.Supplier;
 
 public interface DatabaseClient {
@@ -17,7 +21,43 @@ interface GenericExecuteSpec {
 
     GenericExecuteSpec bind(String name, Object value);
 
+    GenericExecuteSpec bindNull(int index, Class<?> type);
+
+    GenericExecuteSpec bindNull(String name, Class<?> type);
+
+    GenericExecuteSpec bindValues(List<Object> values);
+
+    GenericExecuteSpec bindValues(Map<String, Object> values);
+
+    GenericExecuteSpec bindProperties(Object source);
+
+    GenericExecuteSpec filter(Function<Object, Object> filterFunction);
+
+    GenericExecuteSpec filter(StatementFilterFunction filterFunction);
+
+    <R> RowsFetchSpec<R> map(Function<Object, R> mappingFunction);
+
+    <R> RowsFetchSpec<R> map(BiFunction<Object, Object, R> mappingFunction);
+
+    <R> RowsFetchSpec<R> mapValue(Class<R> mappedClass);
+
+    <R> RowsFetchSpec<R> mapProperties(Class<R> mappedClass);
+
     FetchSpec fetch();
+
+    // Real Spring's `then()`: an alternative terminal/execution-triggering
+    // method to `fetch()`, returning `Mono<Void>`. Stubbed as `Object` since
+    // this test doesn't need Reactor on the classpath.
+    Object then();
+
+    // Real Spring's `flatMap(...)`: another terminal/execution-triggering
+    // method, returning `Flux<R>` directly (no further `.all()` needed).
+    // Stubbed with `Object` result since this test doesn't need Reactor.
+    Object flatMap(Function<Object, Object> mappingFunction);
+  }
+
+  interface RowsFetchSpec<R> {
+    Object all();
   }
 
   interface FetchSpec {

java/test/security/CWE-089/spring-r2dbc/org/springframework/r2dbc/core/StatementFilterFunction.java

@@ -0,0 +1,9 @@
+/*
+ * Minimal stub of Spring Data R2DBC's `StatementFilterFunction`: a
+ * top-level interface in the real `org.springframework.r2dbc.core`
+ * package (not nested inside `DatabaseClient`), used only for the
+ * `GenericExecuteSpec.filter(StatementFilterFunction)` overload.
+ */
+package org.springframework.r2dbc.core;
+
+public interface StatementFilterFunction {}