Skip to content

Rust: Update SqlxQuery, SqlxExecute to use getCanonicalPath #19802

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Rust: Update SqlxQuery, SqlxExecute to use getCanonicalPath #19802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions rust/ql/lib/codeql/rust/frameworks/Sqlx.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
private import rust
private import codeql.rust.Concepts
private import codeql.rust.dataflow.DataFlow
private import codeql.rust.internal.TypeInference
private import codeql.rust.internal.Type

/**
* A call to `sqlx::query` and variations.
Expand All @@ -14,11 +16,12 @@ private class SqlxQuery extends SqlConstruction::Range {

SqlxQuery() {
this.asExpr().getExpr() = call and
call.getFunction().(PathExpr).getResolvedPath() =
call.getStaticTarget().(Addressable).getCanonicalPath() =
[
"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"
"sqlx_core::query::query", "sqlx_core::query_as::query_as",
"sqlx_core::query_with::query_with", "sqlx_core::query_as_with::query_as_with",
"sqlx_core::query_scalar::query_scalar", "sqlx_core::query_scalar_with::query_scalar_with",
"sqlx_core::raw_sql::raw_sql"
]
}

Expand All @@ -33,7 +36,8 @@ private class SqlxExecute extends SqlExecution::Range {

SqlxExecute() {
this.asExpr().getExpr() = call and
call.(Resolvable).getResolvedPath() = "crate::executor::Executor::execute"
call.getStaticTarget().(Addressable).getCanonicalPath() =
"sqlx_core::executor::Executor::execute"
}

override DataFlow::Node getSql() { result.asExpr().getExpr() = call.getArgList().getArg(0) }
Expand Down
12 changes: 0 additions & 12 deletions rust/ql/test/query-tests/security/CWE-089/SqlInjection.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,4 @@
#select
| sqlx.rs:66:26:66:46 | safe_query_3.as_str() | sqlx.rs:48:25:48:46 | ...::get | sqlx.rs:66:26:66:46 | safe_query_3.as_str() | This query depends on a $@. | sqlx.rs:48:25:48:46 | ...::get | user-provided value |
| sqlx.rs:67:26:67:48 | unsafe_query_1.as_str() | sqlx.rs:47:22:47:35 | ...::args | sqlx.rs:67:26:67:48 | unsafe_query_1.as_str() | This query depends on a $@. | sqlx.rs:47:22:47:35 | ...::args | user-provided value |
| sqlx.rs:69:30:69:52 | unsafe_query_2.as_str() | sqlx.rs:48:25:48:46 | ...::get | sqlx.rs:69:30:69:52 | unsafe_query_2.as_str() | This query depends on a $@. | sqlx.rs:48:25:48:46 | ...::get | user-provided value |
| sqlx.rs:71:30:71:52 | unsafe_query_4.as_str() | sqlx.rs:48:25:48:46 | ...::get | sqlx.rs:71:30:71:52 | unsafe_query_4.as_str() | This query depends on a $@. | sqlx.rs:48:25:48:46 | ...::get | user-provided value |
| sqlx.rs:77:25:77:45 | safe_query_3.as_str() | sqlx.rs:48:25:48:46 | ...::get | sqlx.rs:77:25:77:45 | safe_query_3.as_str() | This query depends on a $@. | sqlx.rs:48:25:48:46 | ...::get | user-provided value |
| sqlx.rs:78:25:78:47 | unsafe_query_1.as_str() | sqlx.rs:47:22:47:35 | ...::args | sqlx.rs:78:25:78:47 | unsafe_query_1.as_str() | This query depends on a $@. | sqlx.rs:47:22:47:35 | ...::args | user-provided value |
| sqlx.rs:80:29:80:51 | unsafe_query_2.as_str() | sqlx.rs:48:25:48:46 | ...::get | sqlx.rs:80:29:80:51 | unsafe_query_2.as_str() | This query depends on a $@. | sqlx.rs:48:25:48:46 | ...::get | user-provided value |
Expand All @@ -24,22 +20,18 @@ edges
| sqlx.rs:49:9:49:21 | remote_number | sqlx.rs:52:32:52:87 | MacroExpr | provenance | |
| sqlx.rs:49:25:49:52 | remote_string.parse() [Ok] | sqlx.rs:49:25:49:65 | ... .unwrap_or(...) | provenance | MaD:7 |
| sqlx.rs:49:25:49:65 | ... .unwrap_or(...) | sqlx.rs:49:9:49:21 | remote_number | provenance | |
| sqlx.rs:52:9:52:20 | safe_query_3 | sqlx.rs:66:26:66:46 | safe_query_3.as_str() | provenance | MaD:3 |
| sqlx.rs:52:9:52:20 | safe_query_3 | sqlx.rs:77:25:77:45 | safe_query_3.as_str() | provenance | MaD:3 |
| sqlx.rs:52:24:52:88 | res | sqlx.rs:52:32:52:87 | { ... } | provenance | |
| sqlx.rs:52:32:52:87 | ...::format(...) | sqlx.rs:52:24:52:88 | res | provenance | |
| sqlx.rs:52:32:52:87 | ...::must_use(...) | sqlx.rs:52:9:52:20 | safe_query_3 | provenance | |
| sqlx.rs:52:32:52:87 | MacroExpr | sqlx.rs:52:32:52:87 | ...::format(...) | provenance | MaD:4 |
| sqlx.rs:52:32:52:87 | { ... } | sqlx.rs:52:32:52:87 | ...::must_use(...) | provenance | MaD:9 |
| sqlx.rs:53:9:53:22 | unsafe_query_1 [&ref] | sqlx.rs:67:26:67:48 | unsafe_query_1.as_str() | provenance | MaD:3 |
| sqlx.rs:53:9:53:22 | unsafe_query_1 [&ref] | sqlx.rs:78:25:78:47 | unsafe_query_1.as_str() | provenance | MaD:3 |
| sqlx.rs:53:26:53:36 | &arg_string [&ref] | sqlx.rs:53:9:53:22 | unsafe_query_1 [&ref] | provenance | |
| sqlx.rs:53:27:53:36 | arg_string | sqlx.rs:53:26:53:36 | &arg_string [&ref] | provenance | |
| sqlx.rs:54:9:54:22 | unsafe_query_2 [&ref] | sqlx.rs:69:30:69:52 | unsafe_query_2.as_str() | provenance | MaD:3 |
| sqlx.rs:54:9:54:22 | unsafe_query_2 [&ref] | sqlx.rs:80:29:80:51 | unsafe_query_2.as_str() | provenance | MaD:3 |
| sqlx.rs:54:26:54:39 | &remote_string [&ref] | sqlx.rs:54:9:54:22 | unsafe_query_2 [&ref] | provenance | |
| sqlx.rs:54:27:54:39 | remote_string | sqlx.rs:54:26:54:39 | &remote_string [&ref] | provenance | |
| sqlx.rs:56:9:56:22 | unsafe_query_4 | sqlx.rs:71:30:71:52 | unsafe_query_4.as_str() | provenance | MaD:3 |
| sqlx.rs:56:9:56:22 | unsafe_query_4 | sqlx.rs:82:29:82:51 | unsafe_query_4.as_str() | provenance | MaD:3 |
| sqlx.rs:59:9:59:73 | res | sqlx.rs:59:17:59:72 | { ... } | provenance | |
| sqlx.rs:59:17:59:72 | ...::format(...) | sqlx.rs:59:9:59:73 | res | provenance | |
Expand Down Expand Up @@ -91,10 +83,6 @@ nodes
| sqlx.rs:59:17:59:72 | ...::must_use(...) | semmle.label | ...::must_use(...) |
| sqlx.rs:59:17:59:72 | MacroExpr | semmle.label | MacroExpr |
| sqlx.rs:59:17:59:72 | { ... } | semmle.label | { ... } |
| sqlx.rs:66:26:66:46 | safe_query_3.as_str() | semmle.label | safe_query_3.as_str() |
| sqlx.rs:67:26:67:48 | unsafe_query_1.as_str() | semmle.label | unsafe_query_1.as_str() |
| sqlx.rs:69:30:69:52 | unsafe_query_2.as_str() | semmle.label | unsafe_query_2.as_str() |
| sqlx.rs:71:30:71:52 | unsafe_query_4.as_str() | semmle.label | unsafe_query_4.as_str() |
| sqlx.rs:77:25:77:45 | safe_query_3.as_str() | semmle.label | safe_query_3.as_str() |
| sqlx.rs:78:25:78:47 | unsafe_query_1.as_str() | semmle.label | unsafe_query_1.as_str() |
| sqlx.rs:80:29:80:51 | unsafe_query_2.as_str() | semmle.label | unsafe_query_2.as_str() |
Expand Down
22 changes: 11 additions & 11 deletions rust/ql/test/query-tests/security/CWE-089/sqlx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,14 @@ 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?; // $ sql-sink
let _ = conn.execute(safe_query_2.as_str()).await?; // $ sql-sink
let _ = conn.execute(safe_query_3.as_str()).await?; // $ sql-sink SPURIOUS: Alert[rust/sql-injection]=remote1
let _ = conn.execute(unsafe_query_1.as_str()).await?; // $ sql-sink Alert[rust/sql-injection]=args1
let _ = conn.execute(safe_query_1.as_str()).await?; // $ MISSING: sql-sink
let _ = conn.execute(safe_query_2.as_str()).await?; // $ MISSING: sql-sink
let _ = conn.execute(safe_query_3.as_str()).await?; // $ MISSING: sql-sink
let _ = conn.execute(unsafe_query_1.as_str()).await?; // $ MISSING: sql-sink Alert[rust/sql-injection]=args1
if enable_remote {
let _ = conn.execute(unsafe_query_2.as_str()).await?; // $ sql-sink Alert[rust/sql-injection]=remote1
let _ = conn.execute(unsafe_query_3.as_str()).await?; // $ sql-sink MISSING: Alert[rust/sql-injection]=remote1
let _ = conn.execute(unsafe_query_4.as_str()).await?; // $ sql-sink Alert[rust/sql-injection]=remote1
let _ = conn.execute(unsafe_query_2.as_str()).await?; // $ MISSING: sql-sink Alert[rust/sql-injection]=remote1
let _ = conn.execute(unsafe_query_3.as_str()).await?; // $ MISSING: sql-sink Alert[rust/sql-injection]=remote1
let _ = conn.execute(unsafe_query_4.as_str()).await?; // $ MISSING: sql-sink Alert[rust/sql-injection]=remote1
}

// prepared queries
Expand Down Expand Up @@ -103,9 +103,9 @@ async fn test_sqlx_sqlite(url: &str, enable_remote: bool) -> Result<(), sqlx::Er
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?; // $ sql-sink
let _ = conn.execute(safe_query_1.as_str()).await?; // $ MISSING: sql-sink
if enable_remote {
let _ = conn.execute(unsafe_query_1.as_str()).await?; // $ sql-sink MISSING: Alert[rust/sql-injection]=remote2
let _ = conn.execute(unsafe_query_1.as_str()).await?; // $ MISSING: sql-sink Alert[rust/sql-injection]=remote2
}
// ...
let _ = sqlx::raw_sql(safe_query_1.as_str()).execute(&mut conn).await?; // $ sql-sink
Expand Down Expand Up @@ -176,9 +176,9 @@ async fn test_sqlx_postgres(url: &str, enable_remote: bool) -> Result<(), sqlx::
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?; // $ sql-sink
let _ = conn.execute(safe_query_1.as_str()).await?; // $ MISSING: sql-sink
if enable_remote {
let _ = conn.execute(unsafe_query_1.as_str()).await?; // $ sql-sink MISSING: Alert[rust/sql-injection]=remote3
let _ = conn.execute(unsafe_query_1.as_str()).await?; // $ MISSING: sql-sink Alert[rust/sql-injection]=remote3
}

// prepared queries
Expand Down