Merge pull request #18025 from geoffw0/sql1

Rust: SQL Injection Query
This commit is contained in:
Geoffrey White
2024-11-21 22:48:54 +00:00
committed by GitHub
16 changed files with 550 additions and 0 deletions

View File

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

View File

@@ -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() }
}
}

View File

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

View File

@@ -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>

View File

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

View File

@@ -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)

View File

@@ -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)

View File

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

View File

@@ -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"
}

View File

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

View File

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

View File

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

View File

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

View File

@@ -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");

View File

@@ -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" }

View File

@@ -0,0 +1,218 @@
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 with the provided `cargo.toml.manual`:
* ```
* cp cargo.toml.manual cargo.toml
* 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::<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 + "'";
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),
}
}