Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
7 changes: 7 additions & 0 deletions rust/ql/lib/codeql/rust/frameworks/reqwest.model.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,13 @@ extensions:
data:
- ["repo:https://github.com/seanmonstar/reqwest:reqwest", "crate::get", "ReturnValue.Field[crate::result::Result::Ok(0)]", "remote", "manual"]
- ["repo:https://github.com/seanmonstar/reqwest:reqwest", "crate::blocking::get", "ReturnValue.Field[crate::result::Result::Ok(0)]", "remote", "manual"]
- addsTo:
pack: codeql/rust-all
extensible: sinkModel
data:
- ["repo:https://github.com/seanmonstar/reqwest:reqwest", "crate::blocking::get", "Argument[0]", "transmission", "manual"]
- ["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:
pack: codeql/rust-all
extensible: summaryModel
Expand Down
7 changes: 7 additions & 0 deletions rust/ql/lib/codeql/rust/frameworks/url.model.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Models for the `url` crate
extensions:
- addsTo:
pack: codeql/rust-all
extensible: summaryModel
data:
- ["repo:https://github.com/servo/rust-url:url", "<crate::Url>::parse", "Argument[0].Reference", "ReturnValue.Field[crate::result::Result::Ok(0)]", "taint", "manual"]
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/**
* Provides classes and predicates for reasoning about cleartext transmission
* vulnerabilities.
*/

private import codeql.util.Unit
private import rust
private import codeql.rust.dataflow.DataFlow
private import codeql.rust.dataflow.FlowSink

/**
* A data flow sink for cleartext transmission vulnerabilities. That is,
* a `DataFlow::Node` of something that is transmitted over a network.
*/
abstract class CleartextTransmissionSink extends DataFlow::Node { }
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this ought to extend QuerySink::Range instead (cc @geoffw0 ).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it should, but that class is new. I'll make the change and check all the other merged queries do this now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here: #19103


/**
* A barrier for cleartext transmission vulnerabilities.
*/
abstract class CleartextTransmissionBarrier extends DataFlow::Node { }

/**
* A unit class for adding additional flow steps.
*/
class CleartextTransmissionAdditionalFlowStep extends Unit {
/**
* Holds if the step from `node1` to `node2` should be considered a flow
* step for paths related to cleartext transmission vulnerabilities.
*/
abstract predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo);
}

/**
* A sink defined through MaD.
*/
private class MadCleartextTransmissionSink extends CleartextTransmissionSink {
MadCleartextTransmissionSink() { sinkNode(this, "transmission") }
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# THIS FILE IS AN AUTO-GENERATED MODELS AS DATA FILE. DO NOT EDIT.
extensions:
- addsTo:
pack: codeql/rust-all
extensible: sinkModel
data:
- ["repo:https://github.com/seanmonstar/reqwest:reqwest", "<crate::async_impl::client::Client>::delete", "Argument[0]", "transmission", "df-generated"]
- ["repo:https://github.com/seanmonstar/reqwest:reqwest", "<crate::async_impl::client::Client>::get", "Argument[0]", "transmission", "df-generated"]
- ["repo:https://github.com/seanmonstar/reqwest:reqwest", "<crate::async_impl::client::Client>::head", "Argument[0]", "transmission", "df-generated"]
- ["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"]
- ["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::get", "Argument[0]", "transmission", "df-generated"]
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
<qhelp>
<overview>

<p>
Sensitive information that is transmitted without encryption may be accessible
to an attacker.
</p>

</overview>
<recommendation>

<p>
Ensure that sensitive information is always encrypted before being transmitted
over the network. In general, decrypt sensitive information only at the point
where it is necessary for it to be used in cleartext. Avoid transmitting
sensitive information when it is not necessary to.
</p>

</recommendation>
<example>

<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.
</p>

<sample src="CleartextTransmission.rs" />

</example>
<references>

<li>
OWASP Top 10:2021:
<a href="https://owasp.org/Top10/A02_2021-Cryptographic_Failures/">A02:2021 � Cryptographic Failures</a>.
</li>
<li>
OWASP:
<a href="https://cheatsheetseries.owasp.org/cheatsheets/Key_Management_Cheat_Sheet.html">Key Management Cheat Sheet</a>.
</li>

</references>
</qhelp>
50 changes: 50 additions & 0 deletions rust/ql/src/queries/security/CWE-311/CleartextTransmission.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/**
* @name Cleartext transmission of sensitive information
* @description Transmitting sensitive information across a network in
* cleartext can expose it to an attacker.
* @kind path-problem
* @problem.severity warning
* @security-severity 7.5
* @precision high
* @id rust/cleartext-transmission
* @tags security
* external/cwe/cwe-319
*/

import rust
import codeql.rust.dataflow.DataFlow
import codeql.rust.security.SensitiveData
import codeql.rust.dataflow.TaintTracking
import codeql.rust.security.CleartextTransmissionExtensions

/**
* A taint configuration from sensitive information to expressions that are
* transmitted over a network.
*/
module CleartextTransmissionConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node node) { node instanceof SensitiveData }

predicate isSink(DataFlow::Node node) { node instanceof CleartextTransmissionSink }

predicate isBarrier(DataFlow::Node barrier) { barrier instanceof CleartextTransmissionBarrier }

predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
any(CleartextTransmissionAdditionalFlowStep s).step(nodeFrom, nodeTo)
}

predicate isBarrierIn(DataFlow::Node node) {
// make sources barriers so that we only report the closest instance
isSource(node)
}
}

module CleartextTransmissionFlow = TaintTracking::Global<CleartextTransmissionConfig>;

import CleartextTransmissionFlow::PathGraph

from CleartextTransmissionFlow::PathNode sourceNode, CleartextTransmissionFlow::PathNode sinkNode
where CleartextTransmissionFlow::flowPath(sourceNode, sinkNode)
select sinkNode.getNode(), sourceNode, sinkNode,
"The operation '" + sinkNode.getNode().toString() +
"', transmits data which may contain unencrypted sensitive data from $@.", sourceNode,
sourceNode.getNode().toString()
15 changes: 15 additions & 0 deletions rust/ql/src/queries/security/CWE-311/CleartextTransmission.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
func getData() {
// ...

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

// BAD: sensitive information sent in cleartext
Copy link
Contributor

Choose a reason for hiding this comment

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

Strictly speaking I think HTTPS URL parameters are encrypted as part of the HTTPS protocol - though there's a significant danger they get logged outside of that encrypted tunnel, e.g. in a cache or log, or possibly the connection isn't as secure as it seems (as described in the first reference linked from the .qhelp). So I think it is indeed BAD but we might want to rephrase our claim as to why? Alternatively, make the example http:// to avoid introducing confusion on this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I actually thought the whole URL would be sent unencrypted. TIL.

Whether to change the description or use http://: I'm not actually entirely sure about the scope of CWE-311. Is sending a password in the URL of an HTTPS request within the scope given that is encrypted in transmission? Personally, I would think the query would be more useful if we just changed the description, as it definitely catches a real problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I'm definitely happy that we catch these results, but in the .qhelp it's important we try to be as clear as possible about what the problem is and to recommend good practices. Passwords are a bit funny because for several reasons they usually require even more care than other kinds of sensitive information, which complicates recommending good practices if we use that as as example.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy with the changes you've made already, by the way. If you're happy to you can ask for a docs review.

let body = reqwest::get(format!("https://example.com/data?password={password}")).await?.text().await?;

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

// ...
}
Original file line number Diff line number Diff line change
Expand Up @@ -2216,6 +2216,7 @@ storeStep
| file://:0:0:0:0 | [summary] to write: ReturnValue.Field[crate::result::Result::Ok(0)] in repo:https://github.com/seanmonstar/reqwest:reqwest::_::<crate::response::Response>::chunk | Ok | file://:0:0:0:0 | [summary] to write: ReturnValue in repo:https://github.com/seanmonstar/reqwest:reqwest::_::<crate::response::Response>::chunk |
| file://:0:0:0:0 | [summary] to write: ReturnValue.Field[crate::result::Result::Ok(0)] in repo:https://github.com/seanmonstar/reqwest:reqwest::_::<crate::response::Response>::text | Ok | file://:0:0:0:0 | [summary] to write: ReturnValue in repo:https://github.com/seanmonstar/reqwest:reqwest::_::<crate::response::Response>::text |
| file://:0:0:0:0 | [summary] to write: ReturnValue.Field[crate::result::Result::Ok(0)] in repo:https://github.com/seanmonstar/reqwest:reqwest::_::<crate::response::Response>::text_with_charset | Ok | file://:0:0:0:0 | [summary] to write: ReturnValue in repo:https://github.com/seanmonstar/reqwest:reqwest::_::<crate::response::Response>::text_with_charset |
| file://:0:0:0:0 | [summary] to write: ReturnValue.Field[crate::result::Result::Ok(0)] in repo:https://github.com/servo/rust-url:url::_::<crate::Url>::parse | Ok | file://:0:0:0:0 | [summary] to write: ReturnValue in repo:https://github.com/servo/rust-url:url::_::<crate::Url>::parse |
| file://:0:0:0:0 | [summary] to write: ReturnValue.Field[crate::result::Result::Ok(0)].Field[0] in lang:alloc::_::<crate::collections::btree::node::NodeRef>::search_tree_for_bifurcation | tuple.0 | file://:0:0:0:0 | [summary] to write: ReturnValue.Field[crate::result::Result::Ok(0)] in lang:alloc::_::<crate::collections::btree::node::NodeRef>::search_tree_for_bifurcation |
| file://:0:0:0:0 | [summary] to write: ReturnValue.Field[crate::result::Result::Ok(0)].Field[0] in lang:std::_::<crate::sync::poison::condvar::Condvar>::wait_timeout | tuple.0 | file://:0:0:0:0 | [summary] to write: ReturnValue.Field[crate::result::Result::Ok(0)] in lang:std::_::<crate::sync::poison::condvar::Condvar>::wait_timeout |
| file://:0:0:0:0 | [summary] to write: ReturnValue.Field[crate::result::Result::Ok(0)].Field[0] in lang:std::_::<crate::sync::poison::condvar::Condvar>::wait_timeout_ms | tuple.0 | file://:0:0:0:0 | [summary] to write: ReturnValue.Field[crate::result::Result::Ok(0)] in lang:std::_::<crate::sync::poison::condvar::Condvar>::wait_timeout_ms |
Expand Down Expand Up @@ -2494,6 +2495,7 @@ readStep
| file://:0:0:0:0 | [summary param] 0 in lang:std::_::crate::thread::current::try_with_current | function return | file://:0:0:0:0 | [summary] read: Argument[0].ReturnValue in lang:std::_::crate::thread::current::try_with_current |
| file://:0:0:0:0 | [summary param] 0 in lang:std::_::crate::thread::with_current_name | function return | file://:0:0:0:0 | [summary] read: Argument[0].ReturnValue in lang:std::_::crate::thread::with_current_name |
| file://:0:0:0:0 | [summary param] 0 in repo:https://github.com/rust-lang/regex:regex::_::crate::escape | &ref | file://:0:0:0:0 | [summary] read: Argument[0].Reference in repo:https://github.com/rust-lang/regex:regex::_::crate::escape |
| file://:0:0:0:0 | [summary param] 0 in repo:https://github.com/servo/rust-url:url::_::<crate::Url>::parse | &ref | file://:0:0:0:0 | [summary] read: Argument[0].Reference in repo:https://github.com/servo/rust-url:url::_::<crate::Url>::parse |
| file://:0:0:0:0 | [summary param] 1 in lang:alloc::_::<crate::vec::into_iter::IntoIter as crate::iter::traits::iterator::Iterator>::fold | function return | file://:0:0:0:0 | [summary] read: Argument[1].ReturnValue in lang:alloc::_::<crate::vec::into_iter::IntoIter as crate::iter::traits::iterator::Iterator>::fold |
| file://:0:0:0:0 | [summary param] 1 in lang:alloc::_::crate::collections::btree::mem::replace | function return | file://:0:0:0:0 | [summary] read: Argument[1].ReturnValue in lang:alloc::_::crate::collections::btree::mem::replace |
| file://:0:0:0:0 | [summary param] 1 in lang:alloc::_::crate::collections::btree::mem::take_mut | function return | file://:0:0:0:0 | [summary] read: Argument[1].ReturnValue in lang:alloc::_::crate::collections::btree::mem::take_mut |
Expand Down
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:60 |
| main.rs:4:20:4:40 | ...::var(...) [Ok] | main.rs:4:20:4:66 | ... .unwrap_or(...) | provenance | MaD:1573 |
| 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: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:64 |
| main.rs:5:25:5:44 | { ... } | main.rs:5:25:5:44 | ...::must_use(...) | provenance | MaD:2996 |
| 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: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