Rust: New Query rust/disabled-certificate-check #20829
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| /** | ||
| * Provides classes and predicates for reasoning about disabled certificate | ||
| * check vulnerabilities. | ||
| */ | ||
|
|
||
| import rust | ||
| private import codeql.rust.dataflow.DataFlow | ||
| private import codeql.rust.dataflow.FlowSink | ||
| private import codeql.rust.Concepts | ||
|
|
||
| /** | ||
| * Provides default sinks for detecting disabled certificate check | ||
| * vulnerabilities, as well as extension points for adding your own. | ||
| */ | ||
| module DisabledCertificateCheckExtensions { | ||
| /** | ||
| * A data flow sink for disabled certificate check vulnerabilities. | ||
| */ | ||
| abstract class Sink extends QuerySink::Range { | ||
| override string getSinkType() { result = "DisabledCertificateCheck" } | ||
| } | ||
|
|
||
| /** | ||
| * A default sink for disabled certificate check vulnerabilities based on function names. | ||
| */ | ||
| private class DefaultSink extends Sink { | ||
| DefaultSink() { | ||
| exists(CallExprBase fc | | ||
| fc.getStaticTarget().(Function).getName().getText() = | ||
| ["danger_accept_invalid_certs", "danger_accept_invalid_hostnames"] and | ||
| fc.getArg(0) = this.asExpr().getExpr() | ||
|
Contributor
Author
There was a problem hiding this comment. I did try defining the sinks using models-as-data, but it proved to be less reliable in the absence of full modelling of the classes involved. For example in a chain
Contributor
There was a problem hiding this comment. I'm not sure I follow this. Since the sink here uses
Contributor
Author
There was a problem hiding this comment. Hmm, I think you're right, maybe something slightly different is going on. I will investigate...
Contributor
Author
There was a problem hiding this comment. I guess I must've made a mistake in my initial MaD implementation. My second attempt (now pushed) works fine. I did find a couple of additional sinks with a matching name and purpose in the MRVA-1000, so I've modelled them as well.
Contributor
Author
There was a problem hiding this comment. ... after experimenting with this some more, we still lose quite a lot of sinks with the modelled sinks compared to the old name matching (e.g. the MRVA-1000 alone would require 10+ more models above what I'd already added). And the name matching is quite precise due to those long and specific function names. So I've reinstated that, but I'm calling it a heuristic sink now. If you disable the heuristic sinks, we do still have reliable MaD sinks for the common cases. |
||
| ) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * A sink for disabled certificate check vulnerabilities from model data. | ||
| */ | ||
| private class ModelsAsDataSink extends Sink { | ||
| ModelsAsDataSink() { sinkNode(this, "disable-certificate") } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| --- | ||
| category: newQuery | ||
| --- | ||
| * Added a new query `rust/disabled-certificate-check`, to detect disabled TLS certificate checks. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| <!DOCTYPE qhelp PUBLIC | ||
| "-//Semmle//qhelp//EN" | ||
| "qhelp.dtd"> | ||
| <qhelp> | ||
|
|
||
| <overview> | ||
| <p> | ||
| The <code>danger_accept_invalid_certs</code> option on TLS connectors and HTTP clients controls whether certificate verification is performed. If this option is set to <code>true</code>, the client will accept any certificate, making it susceptible to man-in-the-middle attacks. | ||
| </p> | ||
| <p> | ||
| Similarly, the <code>danger_accept_invalid_hostnames</code> option controls whether hostname verification is performed. If this option is set to <code>true</code>, the client will accept any valid certificate regardless of the site that certificate is for, again making it susceptible to man-in-the-middle attacks. | ||
| </p> | ||
| </overview> | ||
|
|
||
| <recommendation> | ||
| <p> | ||
| Do not set <code>danger_accept_invalid_certs</code> or <code>danger_accept_invalid_hostnames</code> to <code>true</code>, except in controlled environments such as tests. In production, always ensure certificate and hostname verification is enabled to prevent security risks. | ||
| </p> | ||
| </recommendation> | ||
|
|
||
| <example> | ||
| <p> | ||
| The following code snippet shows a function that creates an HTTP client with certificate verification disabled: | ||
| </p> | ||
| <sample src="DisabledCertificateCheckBad.rs"/> | ||
| <p> | ||
| In production code, always configure clients to verify certificates: | ||
| </p> | ||
| <sample src="DisabledCertificateCheckGood.rs"/> | ||
| </example> | ||
| <references> | ||
| <li> | ||
| Rust native-tls crate: <a href="https://docs.rs/native-tls/latest/native_tls/struct.TlsConnectorBuilder.html">TlsConnectorBuilder</a>. | ||
| </li> | ||
| <li> | ||
| Rust reqwest crate: <a href="https://docs.rs/reqwest/latest/reqwest/struct.ClientBuilder.html">ClientBuilder</a>. | ||
| </li> | ||
| <li> | ||
| SSL.com: <a href="https://www.ssl.com/article/browsers-and-certificate-validation/">Browsers and Certificate Validation</a>. | ||
| </li> | ||
| </references> | ||
| </qhelp> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| /** | ||
| * @name Disabled TLS certificate check | ||
| * @description An application that disables TLS certificate checking is more vulnerable to | ||
| * man-in-the-middle attacks. | ||
| * @kind path-problem | ||
| * @problem.severity warning | ||
| * @security-severity 7.5 | ||
| * @precision high | ||
| * @id rust/disabled-certificate-check | ||
| * @tags security | ||
| * external/cwe/cwe-295 | ||
| */ | ||
|
|
||
| import rust | ||
| import codeql.rust.dataflow.DataFlow | ||
| import codeql.rust.security.DisabledCertificateCheckExtensions | ||
|
|
||
| /** | ||
| * A taint configuration for disabled TLS certificate checks. | ||
| */ | ||
| module DisabledCertificateCheckConfig implements DataFlow::ConfigSig { | ||
| import DisabledCertificateCheckExtensions | ||
|
|
||
| predicate isSource(DataFlow::Node node) { | ||
| node.asExpr().getExpr().(BooleanLiteralExpr).getTextValue() = "true" | ||
|
Contributor
There was a problem hiding this comment. Would it make sense to also have tainted user input as a sink? In such cases a user could make the value
Contributor
Author
There was a problem hiding this comment. Yep, makes sense - I'm not sure if it will add many more results in practice but I'll add taint sources as a flow source here.
Contributor
Author
There was a problem hiding this comment. Done. It was a bit tricky to test as we didn't have any (direct)
Contributor
Author
There was a problem hiding this comment. (second DCA run was done) |
||
| } | ||
|
|
||
| predicate isSink(DataFlow::Node node) { node instanceof Sink } | ||
|
|
||
| predicate observeDiffInformedIncrementalMode() { any() } | ||
| } | ||
|
|
||
| module DisabledCertificateCheckFlow = DataFlow::Global<DisabledCertificateCheckConfig>; | ||
|
|
||
| import DisabledCertificateCheckFlow::PathGraph | ||
|
|
||
| from | ||
| DisabledCertificateCheckFlow::PathNode sourceNode, DisabledCertificateCheckFlow::PathNode sinkNode | ||
| where DisabledCertificateCheckFlow::flowPath(sourceNode, sinkNode) | ||
| select sinkNode.getNode(), sourceNode, sinkNode, | ||
| "Disabling TLS certificate validation can expose the application to man-in-the-middle attacks." | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| // BAD: Disabling certificate validation in Rust | ||
|
|
||
| let _client = reqwest::Client::builder() | ||
| .danger_accept_invalid_certs(true) // disables certificate validation | ||
| .build() | ||
| .unwrap(); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| // GOOD: Certificate validation is enabled (default) | ||
|
|
||
| let _client = reqwest::Client::builder() | ||
| .danger_accept_invalid_certs(false) // certificate validation enabled explicitly | ||
| .build() | ||
| .unwrap(); | ||
|
|
||
| let _client = native_tls::TlsConnector::builder() // certificate validation enabled by default | ||
| .build() | ||
| .unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this because there are some functions with these names that where not covered by the MaD? Or is it just to avoid enumerating all the variants of these methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could enumerate all the variants of these methods that we find in the MRVA-1000, but:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Also, I see now that you had already explained this in other comments — I missed those when I asked the above 😅