From 3673c7c813cec3c382c29733174a6425890dd565 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Wed, 30 Oct 2024 17:56:20 +0000 Subject: [PATCH 01/13] Rust: Add SQL injection test cases (complete and functioning). --- .../query-tests/security/CWE-089/.gitignore | 2 + .../query-tests/security/CWE-089/cargo.toml | 15 ++ .../migrations/20241031153051_create.sql | 12 + .../test/query-tests/security/CWE-089/sqlx.rs | 217 ++++++++++++++++++ 4 files changed, 246 insertions(+) create mode 100644 rust/ql/test/query-tests/security/CWE-089/.gitignore create mode 100644 rust/ql/test/query-tests/security/CWE-089/cargo.toml create mode 100644 rust/ql/test/query-tests/security/CWE-089/migrations/20241031153051_create.sql create mode 100644 rust/ql/test/query-tests/security/CWE-089/sqlx.rs diff --git a/rust/ql/test/query-tests/security/CWE-089/.gitignore b/rust/ql/test/query-tests/security/CWE-089/.gitignore new file mode 100644 index 000000000000..bdcfa82b5d5c --- /dev/null +++ b/rust/ql/test/query-tests/security/CWE-089/.gitignore @@ -0,0 +1,2 @@ +# sqlite database +*.db* diff --git a/rust/ql/test/query-tests/security/CWE-089/cargo.toml b/rust/ql/test/query-tests/security/CWE-089/cargo.toml new file mode 100644 index 000000000000..0f9f0b75510e --- /dev/null +++ b/rust/ql/test/query-tests/security/CWE-089/cargo.toml @@ -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" diff --git a/rust/ql/test/query-tests/security/CWE-089/migrations/20241031153051_create.sql b/rust/ql/test/query-tests/security/CWE-089/migrations/20241031153051_create.sql new file mode 100644 index 000000000000..c7d989a7258c --- /dev/null +++ b/rust/ql/test/query-tests/security/CWE-089/migrations/20241031153051_create.sql @@ -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"); diff --git a/rust/ql/test/query-tests/security/CWE-089/sqlx.rs b/rust/ql/test/query-tests/security/CWE-089/sqlx.rs new file mode 100644 index 000000000000..bf4cd7a96aec --- /dev/null +++ b/rust/ql/test/query-tests/security/CWE-089/sqlx.rs @@ -0,0 +1,217 @@ +use sqlx::Connection; +use sqlx::Executor; + +/** + * This test is designed to be "run" in two ways: + * - you can extract and analyze the code here using the CodeQL test runner in the usual way, + * verifying the that various vulnerabilities are detected. + * - you can compile and run the code using `cargo`, verifying that it really is a complete + * program that compiles, runs and executes SQL commands (the sqlite ones, at least). + * + * To do the latter: + * + * Install `sqlx`: + * ``` + * cargo install sqlx-cli + * ``` + * + * Create the database: + * ``` + * export DATABASE_URL="sqlite:sqlite_database.db" + * sqlx db create + * sqlx migrate run + * ``` + * + * Build and run: + * ``` + * cargo run + * ``` + * + * You can also rebuild the sqlx 'query cache' in the `.sqlx` subdirectory + * with: + * ``` + * cargo sqlx prepare + * ``` + * This allows the code (in particular the `prepare!` macro) to be built + * in the test without setting `DATABASE_URL` first. + */ + +async fn test_sqlx_mysql(url: &str, enable_remote: bool) -> Result<(), sqlx::Error> { + // connect through a MySql connection pool + let pool = sqlx::mysql::MySqlPool::connect(url).await?; + let mut conn = pool.acquire().await?; + + // 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 remote_number = remote_string.parse::().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 + "'"; + let safe_query_3 = format!("SELECT * FROM people WHERE firstname='{remote_number}'"); + let unsafe_query_1 = &arg_string; + let unsafe_query_2 = &remote_string; + let unsafe_query_3 = String::from("SELECT * FROM people WHERE firstname='") + &remote_string + "'"; + let unsafe_query_4 = format!("SELECT * FROM people WHERE firstname='{remote_string}'"); + 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 + 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 + } + + // 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 + 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(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?; + 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?; + } + + Ok(()) +} + +async fn test_sqlx_sqlite(url: &str, enable_remote: bool) -> Result<(), sqlx::Error> { + // connect through Sqlite, no connection pool + let mut conn = sqlx::sqlite::SqliteConnection::connect(url).await?; + + // 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 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?; + if enable_remote { + let _ = conn.execute(unsafe_query_1.as_str()).await?; // $ MISSING Alert[sql-injection]=remote2 + } + // ... + let _ = sqlx::raw_sql(safe_query_1.as_str()).execute(&mut conn).await?; + if enable_remote { + let _ = sqlx::raw_sql(unsafe_query_1.as_str()).execute(&mut conn).await?; // $ 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?; + 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(safe_query_1.as_str()).fetch(&mut conn); + let _ = sqlx::query(prepared_query_1.as_str()).bind(&const_string).fetch(&mut conn); + 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 row1: (i64, String, String) = sqlx::query_as(safe_query_1.as_str()).fetch_one(&mut conn).await?; + println!(" row1 = {:?}", row1); + let row2: (i64, String, String) = sqlx::query_as(prepared_query_1.as_str()).bind(&const_string).fetch_one(&mut conn).await?; + 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 row3: (i64, String, String) = sqlx::query_as(safe_query_1.as_str()).fetch_optional(&mut conn).await?.expect("no data"); + 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"); + 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 _ = 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?; + 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!("SELECT * FROM people WHERE firstname=$1", const_string).fetch_all(&mut conn).await?; // (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?; + } + + Ok(()) +} + +async fn test_sqlx_postgres(url: &str, enable_remote: bool) -> Result<(), sqlx::Error> { + // connect through a PostGres connection pool + let pool = sqlx::postgres::PgPool::connect(url).await?; + let mut conn = pool.acquire().await?; + + // 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 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?; + if enable_remote { + let _ = conn.execute(unsafe_query_1.as_str()).await?; // $ 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?; + 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?; + } + + Ok(()) +} + +fn main() { + println!("--- CWE-089 sqlx.rs test ---"); + + // we don't *actually* use data from a remote source unless we're explicitly told to at the + // command line; that's because this test is designed to be runnable, and we don't really + // want to expose the test database to potential SQL injection from http://example.com/ - + // no matter how unlikely, local and compartmentalized that may seem. + let enable_remote = std::env::args().nth(1) == Some(String::from("ENABLE_REMOTE")); + println!("enable_remote = {enable_remote}"); + + println!("test_sqlx_mysql..."); + match futures::executor::block_on(test_sqlx_mysql("", enable_remote)) { + Ok(_) => println!(" successful!"), + Err(e) => println!(" error: {}", e), + } + + println!("test_sqlx_sqlite..."); + match futures::executor::block_on(test_sqlx_sqlite("sqlite:sqlite_database.db", enable_remote)) { + Ok(_) => println!(" successful!"), + Err(e) => println!(" error: {}", e), + } + + println!("test_sqlx_postgres..."); + match futures::executor::block_on(test_sqlx_postgres("", enable_remote)) { + Ok(_) => println!(" successful!"), + Err(e) => println!(" error: {}", e), + } +} From 28d0ad94d503d76bc95cf88de7605b97a372875a Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 31 Oct 2024 17:56:17 +0000 Subject: [PATCH 02/13] Rust: Add placeholder SQL injection query, linked in test. --- .../queries/security/CWE-089/SqlInjection.ql | 22 +++++++++++++++++++ .../security/CWE-089/SqlInjection.expected | 0 .../security/CWE-089/SqlInjection.qlref | 2 ++ 3 files changed, 24 insertions(+) create mode 100644 rust/ql/src/queries/security/CWE-089/SqlInjection.ql create mode 100644 rust/ql/test/query-tests/security/CWE-089/SqlInjection.expected create mode 100644 rust/ql/test/query-tests/security/CWE-089/SqlInjection.qlref diff --git a/rust/ql/src/queries/security/CWE-089/SqlInjection.ql b/rust/ql/src/queries/security/CWE-089/SqlInjection.ql new file mode 100644 index 000000000000..7d24d5d12070 --- /dev/null +++ b/rust/ql/src/queries/security/CWE-089/SqlInjection.ql @@ -0,0 +1,22 @@ +/** + * @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 the user. + * @kind path-problem + * @problem.severity error + * @security-severity 8.8 + * @precision high + * @id rust/sql-injection + * @tags security + * external/cwe/cwe-089 + */ + +import codeql.rust.dataflow.DataFlow +/*import codeql.rust.security.SqlInjectionQuery +import SqlInjectionFlow::PathGraph + +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" +*/ +select 0 diff --git a/rust/ql/test/query-tests/security/CWE-089/SqlInjection.expected b/rust/ql/test/query-tests/security/CWE-089/SqlInjection.expected new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/rust/ql/test/query-tests/security/CWE-089/SqlInjection.qlref b/rust/ql/test/query-tests/security/CWE-089/SqlInjection.qlref new file mode 100644 index 000000000000..504d27ff30cc --- /dev/null +++ b/rust/ql/test/query-tests/security/CWE-089/SqlInjection.qlref @@ -0,0 +1,2 @@ +query: queries/security/CWE-089/SqlInjection.ql +postprocess: utils/InlineExpectationsTestQuery.ql From 2df565c84f1a5e8a886797b6cbe9d1ed315c4c3d Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Mon, 18 Nov 2024 15:24:33 +0000 Subject: [PATCH 03/13] Rust: Add options.yml and sqlx 'query cache' (result of 'sqlx prepare') so that the query test can function. --- ...b6da5ed65c28f666a68c4d73a1918f0eaa6f6.json | 32 +++++++++++++++++++ .../security/CWE-089/SqlInjection.expected | 1 + .../CWE-089/{cargo.toml => cargo.toml.manual} | 0 .../query-tests/security/CWE-089/options.yml | 5 +++ .../test/query-tests/security/CWE-089/sqlx.rs | 3 +- 5 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 rust/ql/test/query-tests/security/CWE-089/.sqlx/query-c996a36820ff0b98021fa553b09b6da5ed65c28f666a68c4d73a1918f0eaa6f6.json rename rust/ql/test/query-tests/security/CWE-089/{cargo.toml => cargo.toml.manual} (100%) create mode 100644 rust/ql/test/query-tests/security/CWE-089/options.yml diff --git a/rust/ql/test/query-tests/security/CWE-089/.sqlx/query-c996a36820ff0b98021fa553b09b6da5ed65c28f666a68c4d73a1918f0eaa6f6.json b/rust/ql/test/query-tests/security/CWE-089/.sqlx/query-c996a36820ff0b98021fa553b09b6da5ed65c28f666a68c4d73a1918f0eaa6f6.json new file mode 100644 index 000000000000..a4493e90c37d --- /dev/null +++ b/rust/ql/test/query-tests/security/CWE-089/.sqlx/query-c996a36820ff0b98021fa553b09b6da5ed65c28f666a68c4d73a1918f0eaa6f6.json @@ -0,0 +1,32 @@ +{ + "db_name": "SQLite", + "query": "SELECT * FROM people WHERE firstname=$1", + "describe": { + "columns": [ + { + "name": "id", + "ordinal": 0, + "type_info": "Integer" + }, + { + "name": "firstname", + "ordinal": 1, + "type_info": "Text" + }, + { + "name": "lastname", + "ordinal": 2, + "type_info": "Text" + } + ], + "parameters": { + "Right": 1 + }, + "nullable": [ + false, + false, + false + ] + }, + "hash": "c996a36820ff0b98021fa553b09b6da5ed65c28f666a68c4d73a1918f0eaa6f6" +} diff --git a/rust/ql/test/query-tests/security/CWE-089/SqlInjection.expected b/rust/ql/test/query-tests/security/CWE-089/SqlInjection.expected index e69de29bb2d1..f082a67fcf66 100644 --- a/rust/ql/test/query-tests/security/CWE-089/SqlInjection.expected +++ b/rust/ql/test/query-tests/security/CWE-089/SqlInjection.expected @@ -0,0 +1 @@ +| 0 | diff --git a/rust/ql/test/query-tests/security/CWE-089/cargo.toml b/rust/ql/test/query-tests/security/CWE-089/cargo.toml.manual similarity index 100% rename from rust/ql/test/query-tests/security/CWE-089/cargo.toml rename to rust/ql/test/query-tests/security/CWE-089/cargo.toml.manual diff --git a/rust/ql/test/query-tests/security/CWE-089/options.yml b/rust/ql/test/query-tests/security/CWE-089/options.yml new file mode 100644 index 000000000000..24744b7dfb45 --- /dev/null +++ b/rust/ql/test/query-tests/security/CWE-089/options.yml @@ -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" } diff --git a/rust/ql/test/query-tests/security/CWE-089/sqlx.rs b/rust/ql/test/query-tests/security/CWE-089/sqlx.rs index bf4cd7a96aec..b5cc25000f99 100644 --- a/rust/ql/test/query-tests/security/CWE-089/sqlx.rs +++ b/rust/ql/test/query-tests/security/CWE-089/sqlx.rs @@ -22,8 +22,9 @@ use sqlx::Executor; * sqlx migrate run * ``` * - * Build and run: + * Build and run with the provided `cargo.toml.manual`: * ``` + * cp cargo.toml.manual cargo.toml * cargo run * ``` * From 6a7fb060866a702e774951f302b234a9a3aa4db0 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 31 Oct 2024 18:24:05 +0000 Subject: [PATCH 04/13] Rust: Add .qhelp and examples. --- .../security/CWE-089/SqlInjection.qhelp | 39 +++++++++++++++++++ .../security/CWE-089/SqlInjectionBad.rs | 7 ++++ .../security/CWE-089/SqlInjectionGood.rs | 5 +++ 3 files changed, 51 insertions(+) create mode 100644 rust/ql/src/queries/security/CWE-089/SqlInjection.qhelp create mode 100644 rust/ql/src/queries/security/CWE-089/SqlInjectionBad.rs create mode 100644 rust/ql/src/queries/security/CWE-089/SqlInjectionGood.rs diff --git a/rust/ql/src/queries/security/CWE-089/SqlInjection.qhelp b/rust/ql/src/queries/security/CWE-089/SqlInjection.qhelp new file mode 100644 index 000000000000..bcf54fca5828 --- /dev/null +++ b/rust/ql/src/queries/security/CWE-089/SqlInjection.qhelp @@ -0,0 +1,39 @@ + + + + +

+If a database query (such as a 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. +

+ +
+ + +

+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. +

+ +
+ + +

+In the following examples, an SQL query is prepared using string formatting to directly include a user-controlled value remote_controlled_string. An attacker could craft remote_controlled_string to change the overall meaning of the SQL query. +

+ + + +

A better way to do this is with a prepared statement, binding remote_controlled_string to a parameter of that statement. An attacker who controls remote_controlled_string now cannot change the overall meaning of the query. +

+ + + +
+ + +
  • Wikipedia: SQL injection.
  • +
  • OWASP: SQL Injection Prevention Cheat Sheet.
  • + +
    +
    diff --git a/rust/ql/src/queries/security/CWE-089/SqlInjectionBad.rs b/rust/ql/src/queries/security/CWE-089/SqlInjectionBad.rs new file mode 100644 index 000000000000..cd0086b7eb27 --- /dev/null +++ b/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) diff --git a/rust/ql/src/queries/security/CWE-089/SqlInjectionGood.rs b/rust/ql/src/queries/security/CWE-089/SqlInjectionGood.rs new file mode 100644 index 000000000000..238665cc2739 --- /dev/null +++ b/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) From c7c6924fdacb8779a717b1662e3fe0d6108b34e9 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Tue, 19 Nov 2024 09:08:00 +0000 Subject: [PATCH 05/13] Rust: Implement query, source/sink/barrier classes and concepts. All of this is framework, nothing is concretely modelled yet. --- rust/ql/lib/codeql/rust/Concepts.qll | 138 ++++++++++++++++++ .../rust/security/SqlInjectionExtensions.qll | 50 +++++++ rust/ql/lib/qlpack.yml | 1 + .../queries/security/CWE-089/SqlInjection.ql | 22 ++- .../security/CWE-089/SqlInjection.expected | 5 +- 5 files changed, 212 insertions(+), 4 deletions(-) create mode 100644 rust/ql/lib/codeql/rust/Concepts.qll create mode 100644 rust/ql/lib/codeql/rust/security/SqlInjectionExtensions.qll diff --git a/rust/ql/lib/codeql/rust/Concepts.qll b/rust/ql/lib/codeql/rust/Concepts.qll new file mode 100644 index 000000000000..f64d2444573c --- /dev/null +++ b/rust/ql/lib/codeql/rust/Concepts.qll @@ -0,0 +1,138 @@ +/** + * 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. + */ +class ThreatModelSource extends DataFlow::Node instanceof ThreatModelSource::Range { + /** + * 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 + */ + string getThreatModel() { result = super.getThreatModel() } + + /** + * Gets a string that describes the type of this threat-model source. + */ + string getSourceType() { result = super.getSourceType() } +} + +/** + * 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. + */ + 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. + */ +class SqlConstruction extends DataFlow::Node instanceof SqlConstruction::Range { + /** + * Gets the argument that specifies the SQL statements to be constructed. + */ + DataFlow::Node getSql() { result = super.getSql() } +} + +/** + * 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. + */ +class SqlExecution extends DataFlow::Node instanceof SqlExecution::Range { + /** + * Gets the argument that specifies the SQL statements to be executed. + */ + DataFlow::Node getSql() { result = super.getSql() } +} + +/** + * 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. + */ +class SqlSanitization extends DataFlow::Node instanceof 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 { } +} diff --git a/rust/ql/lib/codeql/rust/security/SqlInjectionExtensions.qll b/rust/ql/lib/codeql/rust/security/SqlInjectionExtensions.qll new file mode 100644 index 000000000000..db6239d78110 --- /dev/null +++ b/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() } + } +} diff --git a/rust/ql/lib/qlpack.yml b/rust/ql/lib/qlpack.yml index 649d02c8e3aa..53ccf6dfced4 100644 --- a/rust/ql/lib/qlpack.yml +++ b/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} diff --git a/rust/ql/src/queries/security/CWE-089/SqlInjection.ql b/rust/ql/src/queries/security/CWE-089/SqlInjection.ql index 7d24d5d12070..c8db4569e599 100644 --- a/rust/ql/src/queries/security/CWE-089/SqlInjection.ql +++ b/rust/ql/src/queries/security/CWE-089/SqlInjection.ql @@ -10,13 +10,29 @@ * external/cwe/cwe-089 */ +import rust import codeql.rust.dataflow.DataFlow -/*import codeql.rust.security.SqlInjectionQuery +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 } +} + +/** + * Detect taint flow of tainted data that reaches a SQL sink. + */ +module SqlInjectionFlow = TaintTracking::Global; + 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" -*/ -select 0 diff --git a/rust/ql/test/query-tests/security/CWE-089/SqlInjection.expected b/rust/ql/test/query-tests/security/CWE-089/SqlInjection.expected index f082a67fcf66..58f42bec0c84 100644 --- a/rust/ql/test/query-tests/security/CWE-089/SqlInjection.expected +++ b/rust/ql/test/query-tests/security/CWE-089/SqlInjection.expected @@ -1 +1,4 @@ -| 0 | +#select +edges +nodes +subpaths From 49eefccde1a73cc3e9d2bce670556652f2beac72 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Tue, 19 Nov 2024 11:49:02 +0000 Subject: [PATCH 06/13] Rust: Autoformat. --- rust/ql/lib/codeql/rust/Concepts.qll | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/rust/ql/lib/codeql/rust/Concepts.qll b/rust/ql/lib/codeql/rust/Concepts.qll index f64d2444573c..62cd24467d1c 100644 --- a/rust/ql/lib/codeql/rust/Concepts.qll +++ b/rust/ql/lib/codeql/rust/Concepts.qll @@ -53,9 +53,7 @@ module ThreatModelSource { * A data flow source that is enabled in the current threat model configuration. */ class ActiveThreatModelSource extends ThreatModelSource { - ActiveThreatModelSource() { - currentThreatModel(this.getThreatModel()) - } + ActiveThreatModelSource() { currentThreatModel(this.getThreatModel()) } } /** From 758092b1d63dcdcd58f8dcdc642c50f23ede2c77 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Tue, 19 Nov 2024 12:04:15 +0000 Subject: [PATCH 07/13] Rust: Add consistency check failures. --- .../security/CWE-089/CONSISTENCY/DataFlowConsistency.expected | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 rust/ql/test/query-tests/security/CWE-089/CONSISTENCY/DataFlowConsistency.expected diff --git a/rust/ql/test/query-tests/security/CWE-089/CONSISTENCY/DataFlowConsistency.expected b/rust/ql/test/query-tests/security/CWE-089/CONSISTENCY/DataFlowConsistency.expected new file mode 100644 index 000000000000..d9a60435a6f9 --- /dev/null +++ b/rust/ql/test/query-tests/security/CWE-089/CONSISTENCY/DataFlowConsistency.expected @@ -0,0 +1,3 @@ +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. | From d828941b7c026df222df78f02832b44e9613bad3 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Wed, 20 Nov 2024 22:37:46 +0000 Subject: [PATCH 08/13] Rust: Address review comments. --- rust/ql/src/queries/security/CWE-089/SqlInjection.ql | 3 --- rust/ql/src/queries/security/CWE-089/SqlInjectionBad.rs | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/rust/ql/src/queries/security/CWE-089/SqlInjection.ql b/rust/ql/src/queries/security/CWE-089/SqlInjection.ql index c8db4569e599..cf1ea5534aa6 100644 --- a/rust/ql/src/queries/security/CWE-089/SqlInjection.ql +++ b/rust/ql/src/queries/security/CWE-089/SqlInjection.ql @@ -27,9 +27,6 @@ module SqlInjectionConfig implements DataFlow::ConfigSig { predicate isBarrier(DataFlow::Node barrier) { barrier instanceof SqlInjection::Barrier } } -/** - * Detect taint flow of tainted data that reaches a SQL sink. - */ module SqlInjectionFlow = TaintTracking::Global; from SqlInjectionFlow::PathNode sourceNode, SqlInjectionFlow::PathNode sinkNode diff --git a/rust/ql/src/queries/security/CWE-089/SqlInjectionBad.rs b/rust/ql/src/queries/security/CWE-089/SqlInjectionBad.rs index cd0086b7eb27..4184399a4f18 100644 --- a/rust/ql/src/queries/security/CWE-089/SqlInjectionBad.rs +++ b/rust/ql/src/queries/security/CWE-089/SqlInjectionBad.rs @@ -4,4 +4,4 @@ let unsafe_query = format!("SELECT * FROM people WHERE firstname='{remote_contro 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) +let _ = sqlx::query(unsafe_query.as_str()).fetch_all(&mut conn).await?; // BAD (arbitrary SQL injection is possible) From 636c047c415af7b43e4577bb675ed72fd2bc0796 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 21 Nov 2024 13:53:49 +0000 Subject: [PATCH 09/13] Rust: Use final class. --- rust/ql/lib/codeql/rust/Concepts.qll | 32 ++++------------------------ 1 file changed, 4 insertions(+), 28 deletions(-) diff --git a/rust/ql/lib/codeql/rust/Concepts.qll b/rust/ql/lib/codeql/rust/Concepts.qll index 62cd24467d1c..402c720ab6f3 100644 --- a/rust/ql/lib/codeql/rust/Concepts.qll +++ b/rust/ql/lib/codeql/rust/Concepts.qll @@ -13,21 +13,7 @@ private import codeql.threatmodels.ThreatModels * Extend this class to refine existing API models. If you want to model new APIs, * extend `ThreatModelSource::Range` instead. */ -class ThreatModelSource extends DataFlow::Node instanceof ThreatModelSource::Range { - /** - * 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 - */ - string getThreatModel() { result = super.getThreatModel() } - - /** - * Gets a string that describes the type of this threat-model source. - */ - string getSourceType() { result = super.getSourceType() } -} +final class ThreatModelSource = ThreatModelSource::Range; /** * Provides a class for modeling new sources for specific threat-models. @@ -67,12 +53,7 @@ class ActiveThreatModelSource extends ThreatModelSource { * Extend this class to refine existing API models. If you want to model new APIs, * extend `SqlConstruction::Range` instead. */ -class SqlConstruction extends DataFlow::Node instanceof SqlConstruction::Range { - /** - * Gets the argument that specifies the SQL statements to be constructed. - */ - DataFlow::Node getSql() { result = super.getSql() } -} +final class SqlConstruction = SqlConstruction::Range; /** * Provides a class for modeling new SQL execution APIs. @@ -98,12 +79,7 @@ module SqlConstruction { * Extend this class to refine existing API models. If you want to model new APIs, * extend `SqlExecution::Range` instead. */ -class SqlExecution extends DataFlow::Node instanceof SqlExecution::Range { - /** - * Gets the argument that specifies the SQL statements to be executed. - */ - DataFlow::Node getSql() { result = super.getSql() } -} +final class SqlExecution = SqlExecution::Range; /** * Provides a class for modeling new SQL execution APIs. @@ -123,7 +99,7 @@ module SqlExecution { /** * A data-flow node that performs SQL sanitization. */ -class SqlSanitization extends DataFlow::Node instanceof SqlSanitization::Range { } +final class SqlSanitization = SqlSanitization::Range; /** * Provides a class for modeling new SQL sanitization APIs. From 49e002843e78f7c0d4e9da70aeeeb378447add50 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 21 Nov 2024 13:57:45 +0000 Subject: [PATCH 10/13] Rust: Restore some documentation. --- rust/ql/lib/codeql/rust/Concepts.qll | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/rust/ql/lib/codeql/rust/Concepts.qll b/rust/ql/lib/codeql/rust/Concepts.qll index 402c720ab6f3..5befe006bc69 100644 --- a/rust/ql/lib/codeql/rust/Concepts.qll +++ b/rust/ql/lib/codeql/rust/Concepts.qll @@ -25,6 +25,10 @@ module ThreatModelSource { 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(); From b7c7a9ee7c2d5303ecc52c242f05e43dc4dae6d1 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 21 Nov 2024 14:23:51 +0000 Subject: [PATCH 11/13] Rust: Accept consistency check failures. --- .../security/CWE-089/CONSISTENCY/DataFlowConsistency.expected | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/rust/ql/test/query-tests/security/CWE-089/CONSISTENCY/DataFlowConsistency.expected b/rust/ql/test/query-tests/security/CWE-089/CONSISTENCY/DataFlowConsistency.expected index d9a60435a6f9..6e35019c1cbf 100644 --- a/rust/ql/test/query-tests/security/CWE-089/CONSISTENCY/DataFlowConsistency.expected +++ b/rust/ql/test/query-tests/security/CWE-089/CONSISTENCY/DataFlowConsistency.expected @@ -1,3 +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. | From f8fff4b18c5c93da643e055a5965b8d570875199 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 21 Nov 2024 17:06:32 +0000 Subject: [PATCH 12/13] Update rust/ql/src/queries/security/CWE-089/SqlInjection.ql Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com> --- rust/ql/src/queries/security/CWE-089/SqlInjection.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/ql/src/queries/security/CWE-089/SqlInjection.ql b/rust/ql/src/queries/security/CWE-089/SqlInjection.ql index cf1ea5534aa6..ee2a3d144868 100644 --- a/rust/ql/src/queries/security/CWE-089/SqlInjection.ql +++ b/rust/ql/src/queries/security/CWE-089/SqlInjection.ql @@ -1,6 +1,6 @@ /** * @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 the user. + * @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 From 01cddcc0428a5f4ae926e4dcd9e22592795ed719 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 21 Nov 2024 16:54:47 +0000 Subject: [PATCH 13/13] Rust: Suggestions from docs review. --- rust/ql/src/queries/security/CWE-089/SqlInjection.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/ql/src/queries/security/CWE-089/SqlInjection.qhelp b/rust/ql/src/queries/security/CWE-089/SqlInjection.qhelp index bcf54fca5828..0b56ac54d0de 100644 --- a/rust/ql/src/queries/security/CWE-089/SqlInjection.qhelp +++ b/rust/ql/src/queries/security/CWE-089/SqlInjection.qhelp @@ -5,7 +5,7 @@

    -If a database query (such as a 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. +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.