Java: model Spring R2DBC DatabaseClient and io.r2dbc.spi as SQL injection sinks

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"]

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<String>)` 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<String>)`, 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"]

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:

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 |

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

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<String>)` 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<String>)
+  }
+}

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();
+  }
+}

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();
+  }
+}

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();
+}

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();
+}

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 {
+}

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();
+}

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<String> sqlSupplier);
+
+  interface GenericExecuteSpec {
+    GenericExecuteSpec bind(int index, Object value);
+
+    GenericExecuteSpec bind(String name, Object value);
+
+    FetchSpec fetch();
+  }
+
+  interface FetchSpec {
+    Object all();
+  }
+}