Merge pull request #18129 from geoffw0/sinkmodels

Rust: Sink models for rust/sql-injection

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

@@ -105,7 +105,7 @@ module RemoteSource {
 }
 
 /**
- * A data-flow node that constructs a SQL statement.
+ * A data-flow node that constructs a SQL statement (for later execution).
  *
  * Often, it is worthy of an alert if a SQL statement is constructed such that
  * executing it would be a security risk.
@@ -133,10 +133,10 @@ module SqlConstruction {
 }
 
 /**
- * A data-flow node that executes SQL statements.
+ * A data-flow node that constructs and executes SQL statements.
  *
  * If the context of interest is such that merely constructing a SQL statement
- * would be valuable to report, consider using `SqlConstruction`.
+ * would be valuable to report, consider also using `SqlConstruction`.
  *
  * Extend this class to refine existing API models. If you want to model new APIs,
  * extend `SqlExecution::Range` instead.

rust/ql/lib/codeql/rust/Frameworks.qll

@@ -4,3 +4,4 @@
 
 private import codeql.rust.frameworks.Reqwest
 private import codeql.rust.frameworks.stdlib.Env
+private import codeql.rust.frameworks.Sqlx

rust/ql/lib/codeql/rust/frameworks/Sqlx.qll

@@ -0,0 +1,40 @@
+/**
+ * Provides modeling for the `SQLx` library.
+ */
+
+private import rust
+private import codeql.rust.Concepts
+private import codeql.rust.dataflow.DataFlow
+
+/**
+ * A call to `sqlx::query` and variations.
+ */
+private class SqlxQuery extends SqlConstruction::Range {
+  CallExpr call;
+
+  SqlxQuery() {
+    this.asExpr().getExpr() = call and
+    call.getFunction().(PathExpr).getPath().getResolvedPath() =
+      [
+        "crate::query::query", "crate::query_as::query_as", "crate::query_with::query_with",
+        "crate::query_as_with::query_as_with", "crate::query_scalar::query_scalar",
+        "crate::query_scalar_with::query_scalar_with", "crate::raw_sql::raw_sql"
+      ]
+  }
+
+  override DataFlow::Node getSql() { result.asExpr().getExpr() = call.getArgList().getArg(0) }
+}
+
+/**
+ * A call to `sqlx::Executor::execute`.
+ */
+private class SqlxExecute extends SqlExecution::Range {
+  MethodCallExpr call;
+
+  SqlxExecute() {
+    this.asExpr().getExpr() = call and
+    call.(Resolvable).getResolvedPath() = "crate::executor::Executor::execute"
+  }
+
+  override DataFlow::Node getSql() { result.asExpr().getExpr() = call.getArgList().getArg(0) }
+}

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

@@ -0,0 +1,2 @@
+testFailures
+failures

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

@@ -0,0 +1,19 @@
+import rust
+import codeql.rust.security.SqlInjectionExtensions
+import utils.InlineExpectationsTest
+
+module SqlSinksTest implements TestSig {
+  string getARelevantTag() { result = "sql-sink" }
+
+  predicate hasActualResult(Location location, string element, string tag, string value) {
+    exists(SqlInjection::Sink sink |
+      location = sink.getLocation() and
+      location.getFile().getBaseName() != "" and
+      element = sink.toString() and
+      tag = "sql-sink" and
+      value = ""
+    )
+  }
+}
+
+import MakeTest<SqlSinksTest>

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

@@ -44,8 +44,8 @@ async fn test_sqlx_mysql(url: &str, enable_remote: bool) -> Result<(), sqlx::Err
 
     // construct queries (with extra variants)
     let const_string = String::from("Alice");
-    let arg_string = std::env::args().nth(1).unwrap_or(String::from("Alice")); // $ MISSING Source=args1
-    let remote_string = reqwest::blocking::get("http://example.com/").unwrap().text().unwrap_or(String::from("Alice")); // $ MISSING Source=remote1
+    let arg_string = std::env::args().nth(1).unwrap_or(String::from("Alice")); // $ MISSING: Source=args1
+    let remote_string = reqwest::blocking::get("http://example.com/").unwrap().text().unwrap_or(String::from("Alice")); // $ MISSING: Source=remote1
     let remote_number = remote_string.parse::<i32>().unwrap_or(0);
     let safe_query_1 = String::from("SELECT * FROM people WHERE firstname='Alice'");
     let safe_query_2 = String::from("SELECT * FROM people WHERE firstname='") + &const_string + "'";
@@ -57,31 +57,31 @@ async fn test_sqlx_mysql(url: &str, enable_remote: bool) -> Result<(), sqlx::Err
     let prepared_query_1 = String::from("SELECT * FROM people WHERE firstname=?"); // (prepared arguments are safe)
 
     // direct execution
-    let _ = conn.execute(safe_query_1.as_str()).await?;
-    let _ = conn.execute(safe_query_2.as_str()).await?;
-    let _ = conn.execute(safe_query_3.as_str()).await?;
-    let _ = conn.execute(unsafe_query_1.as_str()).await?; // $ MISSING Alert[sql-injection]=args1
+    let _ = conn.execute(safe_query_1.as_str()).await?; // $ sql-sink
+    let _ = conn.execute(safe_query_2.as_str()).await?; // $ sql-sink
+    let _ = conn.execute(safe_query_3.as_str()).await?; // $ sql-sink
+    let _ = conn.execute(unsafe_query_1.as_str()).await?; // $ sql-sink MISSING: Alert[sql-injection]=args1
     if enable_remote {
-        let _ = conn.execute(unsafe_query_2.as_str()).await?; // $ MISSING Alert[sql-injection]=remote1
-        let _ = conn.execute(unsafe_query_3.as_str()).await?; // $ MISSING Alert[sql-injection]=remote1
-        let _ = conn.execute(unsafe_query_4.as_str()).await?; // $ MISSING Alert[sql-injection]=remote1
+        let _ = conn.execute(unsafe_query_2.as_str()).await?; // $ sql-sink MISSING: Alert[sql-injection]=remote1
+        let _ = conn.execute(unsafe_query_3.as_str()).await?; // $ sql-sink MISSING: Alert[sql-injection]=remote1
+        let _ = conn.execute(unsafe_query_4.as_str()).await?; // $ sql-sink MISSING: Alert[sql-injection]=remote1
     }
 
     // prepared queries
-    let _ = sqlx::query(safe_query_1.as_str()).execute(&pool).await?;
-    let _ = sqlx::query(safe_query_2.as_str()).execute(&pool).await?;
-    let _ = sqlx::query(safe_query_3.as_str()).execute(&pool).await?;
-    let _ = sqlx::query(unsafe_query_1.as_str()).execute(&pool).await?; // $ MISSING Alert[sql-injection]=args1
+    let _ = sqlx::query(safe_query_1.as_str()).execute(&pool).await?; // $ sql-sink
+    let _ = sqlx::query(safe_query_2.as_str()).execute(&pool).await?; // $ sql-sink
+    let _ = sqlx::query(safe_query_3.as_str()).execute(&pool).await?; // $ sql-sink
+    let _ = sqlx::query(unsafe_query_1.as_str()).execute(&pool).await?; // $ sql-sink MISSING: Alert[sql-injection]=args1
     if enable_remote {
-        let _ = sqlx::query(unsafe_query_2.as_str()).execute(&pool).await?; // $ MISSING Alert[sql-injection]=remote1
-        let _ = sqlx::query(unsafe_query_3.as_str()).execute(&pool).await?; // $ MISSING Alert[sql-injection]=remote1
-        let _ = sqlx::query(unsafe_query_4.as_str()).execute(&pool).await?; // $ MISSING Alert[sql-injection]=remote1
+        let _ = sqlx::query(unsafe_query_2.as_str()).execute(&pool).await?; // $ sql-sink MISSING: Alert[sql-injection]=remote1
+        let _ = sqlx::query(unsafe_query_3.as_str()).execute(&pool).await?; // $ sql-sink MISSING: Alert[sql-injection]=remote1
+        let _ = sqlx::query(unsafe_query_4.as_str()).execute(&pool).await?; // $ sql-sink MISSING: Alert[sql-injection]=remote1
     }
-    let _ = sqlx::query(prepared_query_1.as_str()).bind(const_string).execute(&pool).await?;
-    let _ = sqlx::query(prepared_query_1.as_str()).bind(arg_string).execute(&pool).await?;
+    let _ = sqlx::query(prepared_query_1.as_str()).bind(const_string).execute(&pool).await?; // $ sql-sink
+    let _ = sqlx::query(prepared_query_1.as_str()).bind(arg_string).execute(&pool).await?; // $ sql-sink
     if enable_remote {
-        let _ = sqlx::query(prepared_query_1.as_str()).bind(remote_string).execute(&pool).await?;
-        let _ = sqlx::query(prepared_query_1.as_str()).bind(remote_number).execute(&pool).await?;
+        let _ = sqlx::query(prepared_query_1.as_str()).bind(remote_string).execute(&pool).await?; // $ sql-sink
+        let _ = sqlx::query(prepared_query_1.as_str()).bind(remote_number).execute(&pool).await?; // $ sql-sink
     }
 
     Ok(())
@@ -93,67 +93,67 @@ async fn test_sqlx_sqlite(url: &str, enable_remote: bool) -> Result<(), sqlx::Er
 
     // construct queries
     let const_string = String::from("Alice");
-    let remote_string = reqwest::blocking::get("http://example.com/").unwrap().text().unwrap_or(String::from("Alice")); // $ MISSING Source=remote2
+    let remote_string = reqwest::blocking::get("http://example.com/").unwrap().text().unwrap_or(String::from("Alice")); // $ MISSING: Source=remote2
     let safe_query_1 = String::from("SELECT * FROM people WHERE firstname='") + &const_string + "'";
     let unsafe_query_1 = String::from("SELECT * FROM people WHERE firstname='") + &remote_string + "'";
     let prepared_query_1 = String::from("SELECT * FROM people WHERE firstname=?"); // (prepared arguments are safe)
 
     // direct execution (with extra variants)
-    let _ = conn.execute(safe_query_1.as_str()).await?;
+    let _ = conn.execute(safe_query_1.as_str()).await?; // $ sql-sink
     if enable_remote {
-        let _ = conn.execute(unsafe_query_1.as_str()).await?; // $ MISSING Alert[sql-injection]=remote2
+        let _ = conn.execute(unsafe_query_1.as_str()).await?; // $ sql-sink MISSING: Alert[sql-injection]=remote2
     }
     // ...
-    let _ = sqlx::raw_sql(safe_query_1.as_str()).execute(&mut conn).await?;
+    let _ = sqlx::raw_sql(safe_query_1.as_str()).execute(&mut conn).await?; // $ sql-sink
     if enable_remote {
-        let _ = sqlx::raw_sql(unsafe_query_1.as_str()).execute(&mut conn).await?; // $ MISSING Alert[sql-injection]=remote2
+        let _ = sqlx::raw_sql(unsafe_query_1.as_str()).execute(&mut conn).await?; // $ sql-sink MISSING: Alert[sql-injection]=remote2
     }
 
     // prepared queries (with extra variants)
-    let _ = sqlx::query(safe_query_1.as_str()).execute(&mut conn).await?;
-    let _ = sqlx::query(prepared_query_1.as_str()).bind(&const_string).execute(&mut conn).await?;
+    let _ = sqlx::query(safe_query_1.as_str()).execute(&mut conn).await?; // $ sql-sink
+    let _ = sqlx::query(prepared_query_1.as_str()).bind(&const_string).execute(&mut conn).await?; // $ sql-sink
     if enable_remote {
-        let _ = sqlx::query(unsafe_query_1.as_str()).execute(&mut conn).await?; // $ MISSING Alert[sql-injection]=remote2
-        let _ = sqlx::query(prepared_query_1.as_str()).bind(&remote_string).execute(&mut conn).await?;
+        let _ = sqlx::query(unsafe_query_1.as_str()).execute(&mut conn).await?; // $ sql-sink MISSING: Alert[sql-injection]=remote2
+        let _ = sqlx::query(prepared_query_1.as_str()).bind(&remote_string).execute(&mut conn).await?; // $ sql-sink
     }
     // ...
-    let _ = sqlx::query(safe_query_1.as_str()).fetch(&mut conn);
-    let _ = sqlx::query(prepared_query_1.as_str()).bind(&const_string).fetch(&mut conn);
+    let _ = sqlx::query(safe_query_1.as_str()).fetch(&mut conn); // $ sql-sink
+    let _ = sqlx::query(prepared_query_1.as_str()).bind(&const_string).fetch(&mut conn); // $ sql-sink
     if enable_remote {
-        let _ = sqlx::query(unsafe_query_1.as_str()).fetch(&mut conn); // $ MISSING Alert[sql-injection]=remote2
-        let _ = sqlx::query(prepared_query_1.as_str()).bind(&remote_string).fetch(&mut conn);
+        let _ = sqlx::query(unsafe_query_1.as_str()).fetch(&mut conn); // $ sql-sink MISSING: Alert[sql-injection]=remote2
+        let _ = sqlx::query(prepared_query_1.as_str()).bind(&remote_string).fetch(&mut conn); // $ sql-sink
     }
     // ...
-    let row1: (i64, String, String) = sqlx::query_as(safe_query_1.as_str()).fetch_one(&mut conn).await?;
+    let row1: (i64, String, String) = sqlx::query_as(safe_query_1.as_str()).fetch_one(&mut conn).await?; // $ sql-sink
     println!("  row1 = {:?}", row1);
-    let row2: (i64, String, String) = sqlx::query_as(prepared_query_1.as_str()).bind(&const_string).fetch_one(&mut conn).await?;
+    let row2: (i64, String, String) = sqlx::query_as(prepared_query_1.as_str()).bind(&const_string).fetch_one(&mut conn).await?; // $ sql-sink
     println!("  row2 = {:?}", row2);
     if enable_remote {
-        let _: (i64, String, String) = sqlx::query_as(unsafe_query_1.as_str()).fetch_one(&mut conn).await?; // $ MISSING Alert[sql-injection]=remote2
-        let _: (i64, String, String) = sqlx::query_as(prepared_query_1.as_str()).bind(&remote_string).fetch_one(&mut conn).await?;
+        let _: (i64, String, String) = sqlx::query_as(unsafe_query_1.as_str()).fetch_one(&mut conn).await?; // $ sql-sink MISSING: Alert[sql-injection]=remote2
+        let _: (i64, String, String) = sqlx::query_as(prepared_query_1.as_str()).bind(&remote_string).fetch_one(&mut conn).await?; // $ sql-sink
     }
     // ...
-    let row3: (i64, String, String) = sqlx::query_as(safe_query_1.as_str()).fetch_optional(&mut conn).await?.expect("no data");
+    let row3: (i64, String, String) = sqlx::query_as(safe_query_1.as_str()).fetch_optional(&mut conn).await?.expect("no data"); // $ sql-sink
     println!("  row3 = {:?}", row3);
-    let row4: (i64, String, String) = sqlx::query_as(prepared_query_1.as_str()).bind(&const_string).fetch_optional(&mut conn).await?.expect("no data");
+    let row4: (i64, String, String) = sqlx::query_as(prepared_query_1.as_str()).bind(&const_string).fetch_optional(&mut conn).await?.expect("no data"); // $ sql-sink
     println!("  row4 = {:?}", row4);
     if enable_remote {
-        let _: (i64, String, String) = sqlx::query_as(unsafe_query_1.as_str()).fetch_optional(&mut conn).await?.expect("no data"); // $ MISSING Alert[sql-injection]=remote2
-        let _: (i64, String, String) = sqlx::query_as(prepared_query_1.as_str()).bind(&remote_string).fetch_optional(&mut conn).await?.expect("no data");
+        let _: (i64, String, String) = sqlx::query_as(unsafe_query_1.as_str()).fetch_optional(&mut conn).await?.expect("no data"); // $ sql-sink $ MISSING: Alert[sql-injection]=remote2
+        let _: (i64, String, String) = sqlx::query_as(prepared_query_1.as_str()).bind(&remote_string).fetch_optional(&mut conn).await?.expect("no data"); // $ sql-sink
     }
     // ...
-    let _ = sqlx::query(safe_query_1.as_str()).fetch_all(&mut conn).await?;
-    let _ = sqlx::query(prepared_query_1.as_str()).bind(&const_string).fetch_all(&mut conn).await?;
-    let _ = sqlx::query("SELECT * FROM people WHERE firstname=?").bind(&const_string).fetch_all(&mut conn).await?;
+    let _ = sqlx::query(safe_query_1.as_str()).fetch_all(&mut conn).await?; // $ sql-sink
+    let _ = sqlx::query(prepared_query_1.as_str()).bind(&const_string).fetch_all(&mut conn).await?; // $ sql-sink
+    let _ = sqlx::query("SELECT * FROM people WHERE firstname=?").bind(&const_string).fetch_all(&mut conn).await?; // $ sql-sink
     if enable_remote {
-        let _ = sqlx::query(unsafe_query_1.as_str()).fetch_all(&mut conn).await?; // $ MISSING Alert[sql-injection]=remote2
-        let _ = sqlx::query(prepared_query_1.as_str()).bind(&remote_string).fetch_all(&mut conn).await?;
-        let _ = sqlx::query("SELECT * FROM people WHERE firstname=?").bind(&remote_string).fetch_all(&mut conn).await?;
+        let _ = sqlx::query(unsafe_query_1.as_str()).fetch_all(&mut conn).await?; // $ sql-sink MISSING: Alert[sql-injection]=remote2
+        let _ = sqlx::query(prepared_query_1.as_str()).bind(&remote_string).fetch_all(&mut conn).await?; // $ sql-sink
+        let _ = sqlx::query("SELECT * FROM people WHERE firstname=?").bind(&remote_string).fetch_all(&mut conn).await?; // $ sql-sink
     }
     // ...
-    let _ = sqlx::query!("SELECT * FROM people WHERE firstname=$1", const_string).fetch_all(&mut conn).await?; // (only takes string literals, so can't be vulnerable)
+    let _ = sqlx::query!("SELECT * FROM people WHERE firstname=$1", const_string).fetch_all(&mut conn).await?; // $ MISSING: sql-sink (only takes string literals, so can't be vulnerable)
     if enable_remote {
-        let _ = sqlx::query!("SELECT * FROM people WHERE firstname=$1", remote_string).fetch_all(&mut conn).await?;
+        let _ = sqlx::query!("SELECT * FROM people WHERE firstname=$1", remote_string).fetch_all(&mut conn).await?; // $ MISSING: sql-sink
     }
 
     Ok(())
@@ -166,23 +166,23 @@ async fn test_sqlx_postgres(url: &str, enable_remote: bool) -> Result<(), sqlx::
 
     // construct queries
     let const_string = String::from("Alice");
-    let remote_string = reqwest::blocking::get("http://example.com/").unwrap().text().unwrap_or(String::from("Alice")); // $ MISSING Source=remote3
+    let remote_string = reqwest::blocking::get("http://example.com/").unwrap().text().unwrap_or(String::from("Alice")); // $ MISSING: Source=remote3
     let safe_query_1 = String::from("SELECT * FROM people WHERE firstname='") + &const_string + "'";
     let unsafe_query_1 = String::from("SELECT * FROM people WHERE firstname='") + &remote_string + "'";
     let prepared_query_1 = String::from("SELECT * FROM people WHERE firstname=$1"); // (prepared arguments are safe)
 
     // direct execution
-    let _ = conn.execute(safe_query_1.as_str()).await?;
+    let _ = conn.execute(safe_query_1.as_str()).await?; // $ sql-sink
     if enable_remote {
-        let _ = conn.execute(unsafe_query_1.as_str()).await?; // $ MISSING Alert[sql-injection]=remote3
+        let _ = conn.execute(unsafe_query_1.as_str()).await?; // $ sql-sink MISSING: Alert[sql-injection]=remote3
     }
 
     // prepared queries
-    let _ = sqlx::query(safe_query_1.as_str()).execute(&pool).await?;
-    let _ = sqlx::query(prepared_query_1.as_str()).bind(&const_string).execute(&pool).await?;
+    let _ = sqlx::query(safe_query_1.as_str()).execute(&pool).await?; // $ sql-sink
+    let _ = sqlx::query(prepared_query_1.as_str()).bind(&const_string).execute(&pool).await?; // $ sql-sink
     if enable_remote {
-        let _ = sqlx::query(unsafe_query_1.as_str()).execute(&pool).await?; // $ MISSING Alert[sql-injection]=remote3
-        let _ = sqlx::query(prepared_query_1.as_str()).bind(&remote_string).execute(&pool).await?;
+        let _ = sqlx::query(unsafe_query_1.as_str()).execute(&pool).await?; // $ sql-sink MISSING: Alert[sql-injection]=remote3
+        let _ = sqlx::query(prepared_query_1.as_str()).bind(&remote_string).execute(&pool).await?; // $ sql-sink
     }
 
     Ok(())