8000 Rust: Update SqlxQuery, SqlxExecute to use getCanonicalPath by geoffw0 · Pull Request #19802 · github/codeql · GitHub
[go: up one dir, main page]
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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Rust: Update SqlxQuery, SqlxExecute to use getCanonicalPath #19802

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
dc08274
Rust: Update SqlxQuery, SqlxExecute from getResolvedPath -> getCanoni…
geoffw0 Jun 17, 2025
f3b5cc7
Merge branch 'main' into sqlx
geoffw0 Jul 8, 2025
6f5e4ef
Merge branch 'main' into sqlx
geoffw0 Jul 16, 2025
87deab8
Rust: Remove Sqlx.qll.
geoffw0 Jul 16, 2025
62b7d84
Rust: Add Sqlx as MaD sinks instead.
geoffw0 Jul 16, 2025
944fd2a
Rust: Add explicit types in some (not all) of the test cases.
geoffw0 Jul 16, 2025
69064b7
Rust: Update the model.
geoffw0 Jul 17, 2025
27bea33
Rust: Accept consistency check change.
geoffw0 Jul 17, 2025
67c170f
Merge branch 'main' into sqlx
geoffw0 Jul 24, 2025
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
Next Next commit
Rust: Update SqlxQuery, SqlxExecute from getResolvedPath -> getCanoni… 
…calPath.
  • Loading branch information
@geoffw0
geoffw0 committed Jun 17, 2025
commit dc08274aa294bae0d404573370d6221dcca3e89b
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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this should be something like

Suggested change
"sqlx_core::executor::Executor::execute"
"<sqlx_core::executor::Executor>::execute"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's difficult to be confident without the test working, but <_ as sqlx_core::executor::Executor>::execute does at least exist as the canonical path of a function in the database, so I'm going with that for now.

}

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we are able to infer that conn has type PoolConnection, we cannot currently infer that is in fact has type PoolConnection<MySql> (#19954 or similar is needed for that).

Once we are able to do that, we need implicit dereferencing via

impl<DB: Database> Deref for PoolConnection<DB> {
    type Target = DB::Connection;

    fn deref(&self) -> &Self::Target {
        &self.live.as_ref().expect(EXPECT_MSG).raw
    }
}

to get to the type &MySqlConnection, and then via impl<'c> Executor<'c> for &'c mut MySqlConnection we can find execute.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added explicit types to some of the test cases and that is indeed not enough by itself, but it should increase the chances we see a change in test results as improvements are made.

I'm not sure whether we should aim to merge this PR (i.e. accept the regression for now) or wait for type inference to catch up. Guess I'll bring it out of draft and start a DCA run to help decide...

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"); 59EC // (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
0