Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
f6b7aea
Rust: Add prototype query.
geoffw0 Nov 11, 2025
f8ef48b
Rust: Add query test.
geoffw0 Nov 11, 2025
209f394
Rust: Fix the alert message.
geoffw0 Nov 11, 2025
c77eef3
Rust: Convert the query to a path-problem with global data flow.
geoffw0 Nov 11, 2025
bb78fdf
Rust: Add qhelp and examples (translated from Go, by Copilot).
geoffw0 Nov 12, 2025
87d66c6
Rust: Clean up the .qhelp a little.
geoffw0 Nov 12, 2025
dcae0ef
Rust: I prefer the original certificates reference from the Go .qhelp.
geoffw0 Nov 12, 2025
49063ac
Rust: Cut down the example for readability.
geoffw0 Nov 12, 2025
7a62642
Rust: Change note.
geoffw0 Nov 12, 2025
0675a29
Rust: Minor corrections.
geoffw0 Nov 12, 2025
42aca4a
Apply suggestions from code review
geoffw0 Nov 13, 2025
15fa99a
Rust: Clarify some confusing text in the .qhelp.
geoffw0 Nov 13, 2025
12cbb64
Rust: Add query to suite .expected lists.
geoffw0 Nov 13, 2025
e43000f
Rust: Correct ordering in query suite .expected lists.
geoffw0 Nov 13, 2025
2da0814
Rust: Add test case involving taint.
geoffw0 Nov 20, 2025
8145264
Rust: Add threat model sources as additional sources for the query.
geoffw0 Nov 20, 2025
aca7877
Rust: Add some missing path / file metadata models.
geoffw0 Nov 21, 2025
89a9c46
Rust: Second change note.
geoffw0 Nov 21, 2025
785754e
Rust: Switch the query to taint flow, since some taint summaries are …
geoffw0 Nov 21, 2025
ace7a77
Rust: Switch to MaD models.
geoffw0 Nov 21, 2025
3ad014b
Rust: Additional sinks found in MRVA-1000.
geoffw0 Nov 21, 2025
e01c871
Rust: Accept changes to the dataflow/sources/file test.
geoffw0 Nov 21, 2025
8061505
Merge remote-tracking branch 'upstream/main' into cert-checks
geoffw0 Nov 21, 2025
2ce4c47
Rust: More sinks from the MRVA-1000.
geoffw0 Nov 21, 2025
eb674d0
Rust: Reinstate the original function names model but call it a heuri…
geoffw0 Nov 21, 2025
ff8032a
Rust: Fix after merge.
geoffw0 Nov 21, 2025
0ea28b4
Rust: Test .expected changes.
geoffw0 Nov 21, 2025
993154e
Rust: Avoid duplicating sinks.
geoffw0 Nov 21, 2025
b62968f
Rust: Spelling.
geoffw0 Nov 22, 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
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
Copy link
Contributor

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?

Copy link
Contributor Author

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 are quite a lot and it would be somewhat messy to enumerate.
  • also judging by the rare variants we found there, its likely there are even rarer variants that we would miss with a strictly models-as-data approach. There's no way to find them all.

Copy link
Contributor

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 😅

fc.getArg(0) = this.asExpr().getExpr()
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 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 Builder.a().b().sink(true), we need summaries for a() and b() to understand that sink(true) is being called on a Builder. With the QL-defined sinks that only match the function names, these models are not required - and given the very specific names of these functions, matching with just names is likely to be very accurate.

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 not sure I follow this. Since the sink here uses getStaticTarget it only works when we can resolve a target. Wouldn't we then also be able to find the MaD in the same cases as MaD works by finding the target and checking its canonical name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I think you're right, maybe something slightly different is going on. I will investigate...

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... 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,39 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>

<overview>
<p>
The <code>danger_accept_invalid_certs</code> and <code>danger_accept_invalid_hostnames</code> options on TLS connectors and HTTP clients control whether certificate and hostname verification are performed. If set to <code>true</code>, the client will accept any certificate or any host name, 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 are 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 If an application disables TLS certificate checking, it may be 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"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to also have tainted user input as a sink? In such cases a user could make the value true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. It was a bit tricky to test as we didn't have any (direct) bool taint sources until now, but we've ended up with a bunch of new models and a second change note coming from that. I'll do a second DCA run as well - I think the effect on results for this query is going to be quite marginal, but best to check...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(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();
1 change: 1 addition & 0 deletions rust/ql/src/queries/summary/Stats.qll
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ private import codeql.rust.security.AccessInvalidPointerExtensions
private import codeql.rust.security.CleartextLoggingExtensions
private import codeql.rust.security.CleartextStorageDatabaseExtensions
private import codeql.rust.security.CleartextTransmissionExtensions
private import codeql.rust.security.DisabledCertificateCheckExtensions
private import codeql.rust.security.HardcodedCryptographicValueExtensions
private import codeql.rust.security.InsecureCookieExtensions
private import codeql.rust.security.LogInjectionExtensions
Expand Down
Loading