Merge pull request #18025 from geoffw0/sql1

Rust: SQL Injection Query

Author: geoffw0

rust/ql/lib/codeql/rust/Concepts.qll

@@ -0,0 +1,116 @@
+/**
+ * Provides abstract classes representing generic concepts such as file system
+ * access or system command execution, for which individual framework libraries
+ * provide concrete subclasses.
+ */
+
+private import codeql.rust.dataflow.DataFlow
+private import codeql.threatmodels.ThreatModels
+
+/**
+ * A data flow source for a specific threat-model.
+ *
+ * Extend this class to refine existing API models. If you want to model new APIs,
+ * extend `ThreatModelSource::Range` instead.
+ */
+final class ThreatModelSource = ThreatModelSource::Range;
+
+/**
+ * Provides a class for modeling new sources for specific threat-models.
+ */
+module ThreatModelSource {
+  /**
+   * A data flow source, for a specific threat-model.
+   */
+  abstract class Range extends DataFlow::Node {
+    /**
+     * Gets a string that represents the source kind with respect to threat modeling.
+     *
+     * See
+     * - https://github.com/github/codeql/blob/main/docs/codeql/reusables/threat-model-description.rst
+     * - https://github.com/github/codeql/blob/main/shared/threat-models/ext/threat-model-grouping.model.yml
+     */
+    abstract string getThreatModel();
+
+    /**
+     * Gets a string that describes the type of this threat-model source.
+     */
+    abstract string getSourceType();
+  }
+}
+
+/**
+ * A data flow source that is enabled in the current threat model configuration.
+ */
+class ActiveThreatModelSource extends ThreatModelSource {
+  ActiveThreatModelSource() { currentThreatModel(this.getThreatModel()) }
+}
+
+/**
+ * A data-flow node that constructs a SQL statement.
+ *
+ * Often, it is worthy of an alert if a SQL statement is constructed such that
+ * executing it would be a security risk.
+ *
+ * If it is important that the SQL statement is executed, use `SqlExecution`.
+ *
+ * Extend this class to refine existing API models. If you want to model new APIs,
+ * extend `SqlConstruction::Range` instead.
+ */
+final class SqlConstruction = SqlConstruction::Range;
+
+/**
+ * Provides a class for modeling new SQL execution APIs.
+ */
+module SqlConstruction {
+  /**
+   * A data-flow node that constructs a SQL statement.
+   */
+  abstract class Range extends DataFlow::Node {
+    /**
+     * Gets the argument that specifies the SQL statements to be constructed.
+     */
+    abstract DataFlow::Node getSql();
+  }
+}
+
+/**
+ * A data-flow node that executes SQL statements.
+ *
+ * If the context of interest is such that merely constructing a SQL statement
+ * would be valuable to report, consider using `SqlConstruction`.
+ *
+ * Extend this class to refine existing API models. If you want to model new APIs,
+ * extend `SqlExecution::Range` instead.
+ */
+final class SqlExecution = SqlExecution::Range;
+
+/**
+ * Provides a class for modeling new SQL execution APIs.
+ */
+module SqlExecution {
+  /**
+   * A data-flow node that executes SQL statements.
+   */
+  abstract class Range extends DataFlow::Node {
+    /**
+     * Gets the argument that specifies the SQL statements to be executed.
+     */
+    abstract DataFlow::Node getSql();
+  }
+}
+
+/**
+ * A data-flow node that performs SQL sanitization.
+ */
+final class SqlSanitization = SqlSanitization::Range;
+
+/**
+ * Provides a class for modeling new SQL sanitization APIs.
+ */
+module SqlSanitization {
+  /**
+   * A data-flow node that performs SQL sanitization.
+   */
+  abstract class Range extends DataFlow::Node { }
+}

rust/ql/lib/codeql/rust/security/SqlInjectionExtensions.qll

@@ -0,0 +1,50 @@
+/**
+ * Provides classes and predicates for reasoning about database
+ * queries built from user-controlled sources (that is, SQL injection
+ * vulnerabilities).
+ */
+
+import rust
+private import codeql.rust.dataflow.DataFlow
+private import codeql.rust.Concepts
+private import codeql.util.Unit
+
+/**
+ * Provides default sources, sinks and barriers for detecting SQL injection
+ * vulnerabilities, as well as extension points for adding your own.
+ */
+module SqlInjection {
+  /**
+   * A data flow source for SQL injection vulnerabilities.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A data flow sink for SQL injection vulnerabilities.
+   */
+  abstract class Sink extends DataFlow::Node { }
+
+  /**
+   * A barrier for SQL injection vulnerabilities.
+   */
+  abstract class Barrier extends DataFlow::Node { }
+
+  /**
+   * An active threat-model source, considered as a flow source.
+   */
+  private class ActiveThreatModelSourceAsSource extends Source, ActiveThreatModelSource { }
+
+  /**
+   * A flow sink that is the statement of an SQL construction.
+   */
+  class SqlConstructionAsSink extends Sink {
+    SqlConstructionAsSink() { this = any(SqlConstruction c).getSql() }
+  }
+
+  /**
+   * A flow sink that is the statement of an SQL execution.
+   */
+  class SqlExecutionAsSink extends Sink {
+    SqlExecutionAsSink() { this = any(SqlExecution e).getSql() }
+  }
+}

rust/ql/lib/qlpack.yml

@@ -8,6 +8,7 @@ dependencies:
   codeql/controlflow: ${workspace}
   codeql/dataflow: ${workspace}
   codeql/regex: ${workspace}
+  codeql/threat-models: ${workspace}
   codeql/mad: ${workspace}
   codeql/ssa: ${workspace}
   codeql/tutorial: ${workspace}

rust/ql/src/queries/security/CWE-089/SqlInjection.qhelp

@@ -0,0 +1,39 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+
+<p>
+If a database query (such as an SQL query) is built from user-provided data without sufficient sanitization, a user may be able to run malicious database queries. An attacker can craft the part of the query they control to change the overall meaning of the query.
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+Most database connector libraries offer a way to safely embed untrusted data into a query using query parameters or prepared statements. You should use these features to build queries, rather than string concatenation or similar methods. You can also escape (sanitize) user-controlled strings so that they can be included directly in an SQL command. A library function should be used for escaping, because this approach is only safe if the escaping function is robust against all possible inputs.
+</p>
+
+</recommendation>
+<example>
+
+<p>
+In the following examples, an SQL query is prepared using string formatting to directly include a user-controlled value <code>remote_controlled_string</code>. An attacker could craft <code>remote_controlled_string</code> to change the overall meaning of the SQL query.
+</p>
+
+<sample src="SqlInjectionBad.rs" />
+
+<p>A better way to do this is with a prepared statement, binding <code>remote_controlled_string</code> to a parameter of that statement. An attacker who controls <code>remote_controlled_string</code> now cannot change the overall meaning of the query.
+</p>
+
+<sample src="SqlInjectionGood.rs" />
+
+</example>
+<references>
+
+<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/SQL_injection">SQL injection</a>.</li>
+<li>OWASP: <a href="https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.html">SQL Injection Prevention Cheat Sheet</a>.</li>
+
+</references>
+</qhelp>

rust/ql/src/queries/security/CWE-089/SqlInjection.ql

@@ -0,0 +1,35 @@
+/**
+ * @name Database query built from user-controlled sources
+ * @description Building a database query from user-controlled sources is vulnerable to insertion of malicious code by attackers.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 8.8
+ * @precision high
+ * @id rust/sql-injection
+ * @tags security
+ *       external/cwe/cwe-089
+ */
+
+import rust
+import codeql.rust.dataflow.DataFlow
+import codeql.rust.dataflow.TaintTracking
+import codeql.rust.security.SqlInjectionExtensions
+import SqlInjectionFlow::PathGraph
+
+/**
+ * A taint configuration for tainted data that reaches a SQL sink.
+ */
+module SqlInjectionConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node node) { node instanceof SqlInjection::Source }
+
+  predicate isSink(DataFlow::Node node) { node instanceof SqlInjection::Sink }
+
+  predicate isBarrier(DataFlow::Node barrier) { barrier instanceof SqlInjection::Barrier }
+}
+
+module SqlInjectionFlow = TaintTracking::Global<SqlInjectionConfig>;
+
+from SqlInjectionFlow::PathNode sourceNode, SqlInjectionFlow::PathNode sinkNode
+where SqlInjectionFlow::flowPath(sourceNode, sinkNode)
+select sinkNode.getNode(), sourceNode, sinkNode, "This query depends on a $@.",
+  sourceNode.getNode(), "user-provided value"

rust/ql/src/queries/security/CWE-089/SqlInjectionBad.rs

@@ -0,0 +1,7 @@
+// with SQLx
+
+let unsafe_query = format!("SELECT * FROM people WHERE firstname='{remote_controlled_string}'");
+
+let _ = conn.execute(unsafe_query.as_str()).await?; // BAD (arbitrary SQL injection is possible)
+
+let _ = sqlx::query(unsafe_query.as_str()).fetch_all(&mut conn).await?; // BAD (arbitrary SQL injection is possible)

rust/ql/src/queries/security/CWE-089/SqlInjectionGood.rs

@@ -0,0 +1,5 @@
+// with SQLx
+
+let prepared_query = "SELECT * FROM people WHERE firstname=?";
+
+let _ = sqlx::query(prepared_query_1).bind(&remote_controlled_string).fetch_all(&mut conn).await?; // GOOD (prepared statement with bound parameter)

rust/ql/test/query-tests/security/CWE-089/.gitignore

@@ -0,0 +1,2 @@
+# sqlite database
+*.db*

rust/ql/test/query-tests/security/CWE-089/.sqlx/query-c996a36820ff0b98021fa553b09b6da5ed65c28f666a68c4d73a1918f0eaa6f6.json

@@ -0,0 +1,7 @@
+uniqueEnclosingCallable
+| sqlx.rs:52:72:52:84 | remote_number | Node should have one enclosing callable but has 0. |
+| sqlx.rs:56:74:56:86 | remote_string | Node should have one enclosing callable but has 0. |
+| sqlx.rs:199:32:199:44 | enable_remote | Node should have one enclosing callable but has 0. |
+uniqueNodeToString
+| sqlx.rs:154:13:154:81 | (no string representation) | Node should have one toString but has 0. |
+| sqlx.rs:156:17:156:86 | (no string representation) | Node should have one toString but has 0. |

rust/ql/test/query-tests/security/CWE-089/CONSISTENCY/DataFlowConsistency.expected

@@ -0,0 +1,4 @@
+#select
+edges
+nodes
+subpaths

rust/ql/test/query-tests/security/CWE-089/SqlInjection.expected

@@ -0,0 +1,2 @@
+query: queries/security/CWE-089/SqlInjection.ql
+postprocess: utils/InlineExpectationsTestQuery.ql

rust/ql/test/query-tests/security/CWE-089/SqlInjection.qlref

@@ -0,0 +1,15 @@
+[workspace]
+
+[package]
+name = "CWE-089-Test"
+version = "0.1.0"
+edition = "2021"
+
+[dependencies]
+reqwest = { version = "0.12.9", features = ["blocking"] }
+sqlx = { version = "0.8", features = ["mysql", "sqlite", "postgres", "runtime-async-std", "tls-native-tls"] }
+futures = { version = "0.3" }
+
+[[bin]]
+name = "sqlx"
+path = "./sqlx.rs"

rust/ql/test/query-tests/security/CWE-089/cargo.toml.manual

@@ -0,0 +1,12 @@
+CREATE TABLE IF NOT EXISTS people
+(
+    id INTEGER PRIMARY KEY NOT NULL,
+    firstname TEXT NOT NULL,
+    lastname TEXT NOT NULL
+);
+
+INSERT INTO people
+VALUES (1, "Alice", "Adams");
+
+INSERT INTO people
+VALUES (2, "Bob", "Becket");

rust/ql/test/query-tests/security/CWE-089/migrations/20241031153051_create.sql

@@ -0,0 +1,5 @@
+qltest_cargo_check: true
+qltest_dependencies:
+    - reqwest = { version = "0.12.9", features = ["blocking"] }
+    - sqlx = { version = "0.8", features = ["mysql", "sqlite", "postgres", "runtime-async-std", "tls-native-tls"] }
+    - futures = { version = "0.3" }