Skip to content

Commit b59fd40

Browse files
atorralbaatorralba
authored
Merge pull request github#7136 from atorralba/atorralba/promote-insecure-trustmanager
Java: Promote Insecure TrustManager from experimental
2 parents b2dc02b + 967308f commit b59fd40

File tree

11 files changed

+166
-289
lines changed

11 files changed

+166
-289
lines changed

‎java/ql/src/experimental/Security/CWE/CWE-295/InsecureTrustManager.ql renamed to ‎java/ql/lib/semmle/code/java/security/InsecureTrustManager.qll

Lines changed: 40 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,52 @@
1+
/** Provides classes and predicates to reason about insecure `TrustManager`s. */
2+
3+
import java
4+
private import semmle.code.java.controlflow.Guards
5+
private import semmle.code.java.security.Encryption
6+
private import semmle.code.java.security.SecurityFlag
7+
8+
/** The creation of an insecure `TrustManager`. */
9+
abstract class InsecureTrustManagerSource extends DataFlow::Node { }
10+
11+
private class DefaultInsecureTrustManagerSource extends InsecureTrustManagerSource {
12+
DefaultInsecureTrustManagerSource() {
13+
this.asExpr().(ClassInstanceExpr).getConstructedType() instanceof InsecureX509TrustManager
14+
}
15+
}
16+
117
/**
2-
* @name `TrustManager` that accepts all certificates
3-
* @description Trusting all certificates allows an attacker to perform a machine-in-the-middle attack.
4-
* @kind path-problem
5-
* @problem.severity error
6-
* @precision high
7-
* @id java/insecure-trustmanager
8-
* @tags security
9-
* external/cwe/cwe-295
18+
* The use of a `TrustManager` in an SSL context.
19+
* Intentionally insecure connections are not considered sinks.
1020
*/
21+
abstract class InsecureTrustManagerSink extends DataFlow::Node {
22+
InsecureTrustManagerSink() { not isGuardedByInsecureFlag(this) }
23+
}
1124

12-
import java
13-
import semmle.code.java.controlflow.Guards
14-
import semmle.code.java.dataflow.DataFlow
15-
import semmle.code.java.dataflow.FlowSources
16-
import semmle.code.java.security.Encryption
17-
import semmle.code.java.security.SecurityFlag
18-
import DataFlow::PathGraph
25+
private class DefaultInsecureTrustManagerSink extends InsecureTrustManagerSink {
26+
DefaultInsecureTrustManagerSink() {
27+
exists(MethodAccess ma, Method m |
28+
m.hasName("init") and
29+
m.getDeclaringType() instanceof SSLContext and
30+
ma.getMethod() = m
31+
|
32+
ma.getArgument(1) = this.asExpr()
33+
)
34+
}
35+
}
36+
37+
/** Holds if `node` is guarded by a flag that suggests an intentionally insecure use. */
38+
private predicate isGuardedByInsecureFlag(DataFlow::Node node) {
39+
exists(Guard g | g.controls(node.asExpr().getBasicBlock(), _) |
40+
g = getASecurityFeatureFlagGuard() or g = getAnInsecureTrustManagerFlagGuard()
41+
)
42+
}
1943

2044
/**
2145
* An insecure `X509TrustManager`.
2246
* An `X509TrustManager` is considered insecure if it never throws a `CertificateException`
2347
* and therefore implicitly trusts any certificate as valid.
2448
*/
25-
class InsecureX509TrustManager extends RefType {
49+
private class InsecureX509TrustManager extends RefType {
2650
InsecureX509TrustManager() {
2751
this.getASupertype*() instanceof X509TrustManager and
2852
exists(Method m |
@@ -59,27 +83,6 @@ private predicate mayThrowCertificateException(Method m) {
5983
)
6084
}
6185

62-
/**
63-
* A configuration to model the flow of an `InsecureX509TrustManager` to an `SSLContext.init` call.
64-
*/
65-
class InsecureTrustManagerConfiguration extends TaintTracking::Configuration {
66-
InsecureTrustManagerConfiguration() { this = "InsecureTrustManagerConfiguration" }
67-
68-
override predicate isSource(DataFlow::Node source) {
69-
source.asExpr().(ClassInstanceExpr).getConstructedType() instanceof InsecureX509TrustManager
70-
}
71-
72-
override predicate isSink(DataFlow::Node sink) {
73-
exists(MethodAccess ma, Method m |
74-
m.hasName("init") and
75-
m.getDeclaringType() instanceof SSLContext and
76-
ma.getMethod() = m
77-
|
78-
ma.getArgument(1) = sink.asExpr()
79-
)
80-
}
81-
}
82-
8386
/**
8487
* Flags suggesting a deliberately insecure `TrustManager` usage.
8588
*/
@@ -98,20 +101,3 @@ private class InsecureTrustManagerFlag extends FlagKind {
98101
private Guard getAnInsecureTrustManagerFlagGuard() {
99102
result = any(InsecureTrustManagerFlag flag).getAFlag().asExpr()
100103
}
101-
102-
/** Holds if `node` is guarded by a flag that suggests an intentionally insecure use. */
103-
private predicate isNodeGuardedByFlag(DataFlow::Node node) {
104-
exists(Guard g | g.controls(node.asExpr().getBasicBlock(), _) |
105-
g = getASecurityFeatureFlagGuard() or g = getAnInsecureTrustManagerFlagGuard()
106-
)
107-
}
108-
109-
from
110-
DataFlow::PathNode source, DataFlow::PathNode sink, InsecureTrustManagerConfiguration cfg,
111-
RefType trustManager
112-
where
113-
cfg.hasFlowPath(source, sink) and
114-
not isNodeGuardedByFlag(sink.getNode()) and
115-
trustManager = source.getNode().asExpr().(ClassInstanceExpr).getConstructedType()
116-
select sink, source, sink, "$@ that is defined $@ and trusts any certificate, is used here.",
117-
source, "This trustmanager", trustManager, "here"

‎java/ql/lib/semmle/code/java/security/InsecureTrustManagerQuery.qll

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
/** Provides taint tracking configurations to be used in Trust Manager queries. */
2+
3+
import java
4+
import semmle.code.java.dataflow.FlowSources
5+
import semmle.code.java.security.InsecureTrustManager
6+
7+
/**
8+
* A configuration to model the flow of an insecure `TrustManager`
9+
* to the initialization of an SSL context.
10+
*/
11+
class InsecureTrustManagerConfiguration extends DataFlow::Configuration {
12+
InsecureTrustManagerConfiguration() { this = "InsecureTrustManagerConfiguration" }
13+
14+
override predicate isSource(DataFlow::Node source) {
15+
source instanceof InsecureTrustManagerSource
16+
}
17+
18+
override predicate isSink(DataFlow::Node sink) { sink instanceof InsecureTrustManagerSink }
19+
20+
override predicate allowImplicitRead(DataFlow::Node node, DataFlow::Content c) {
21+
(this.isSink(node) or this.isAdditionalFlowStep(node, _)) and
22+
node.getType() instanceof Array and
23+
c instanceof DataFlow::ArrayContent
24+
}
25+
}

‎java/ql/src/experimental/Security/CWE/CWE-295/InsecureTrustManager.java renamed to ‎java/ql/src/Security/CWE/CWE-295/InsecureTrustManager.java

File renamed without changes.

‎java/ql/src/experimental/Security/CWE/CWE-295/InsecureTrustManager.qhelp renamed to ‎java/ql/src/Security/CWE/CWE-295/InsecureTrustManager.qhelp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,8 @@
44
<qhelp>
55
<overview>
66
<p>
7-
If the <code>checkServerTrusted</code> method of a <code>TrustManager</code> never throws a <code>CertificateException</code> it trusts every certificate.
8-
This allows an attacker to perform a machine-in-the-middle attack against the application therefore breaking any security Transport Layer Security (TLS) gives.
7+
If the <code>checkServerTrusted</code> method of a <code>TrustManager</code> never throws a <code>CertificateException</code>, it trusts every certificate.
8+
This allows an attacker to perform a machine-in-the-middle attack against the application, therefore breaking any security Transport Layer Security (TLS) gives.
99
</p>
1010

1111
<p>
@@ -42,6 +42,6 @@ is loaded into a <code>KeyStore</code>. This explicitly defines the certificate
4242
</example>
4343

4444
<references>
45-
<li>Android Develoers:<a href="https://developer.android.com/training/articles/security-ssl">Security with HTTPS and SSL</a>.</li>
45+
<li>Android Developers: <a href="https://developer.android.com/training/articles/security-ssl">Security with HTTPS and SSL</a>.</li>
4646
</references>
4747
</qhelp>

‎java/ql/src/Security/CWE/CWE-295/InsecureTrustManager.ql

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
/**
2+
* @name `TrustManager` that accepts all certificates
3+
* @description Trusting all certificates allows an attacker to perform a machine-in-the-middle attack.
4+
* @kind path-problem
5+
* @problem.severity error
6+
* @security-severity 7.5
7+
* @precision high
8+
* @id java/insecure-trustmanager
9+
* @tags security
10+
* external/cwe/cwe-295
11+
*/
12+
13+
import java
14+
import semmle.code.java.dataflow.DataFlow
15+
import semmle.code.java.security.InsecureTrustManagerQuery
16+
import DataFlow::PathGraph
17+
18+
from DataFlow::PathNode source, DataFlow::PathNode sink
19+
where any(InsecureTrustManagerConfiguration cfg).hasFlowPath(source, sink)
20+
select sink, source, sink, "This $@, which is defined $@ and trusts any certificate, is used here.",
21+
source, "TrustManager", source.getNode().asExpr().(ClassInstanceExpr).getConstructedType(), "here"

‎java/ql/src/change-notes/2021-11-15-insecure-trustamanger-query.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: newQuery
3+
---
4+
* The query "`TrustManager` that accepts all certificates" (`java/insecure-trustmanager`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @intrigus-lgtm](https://github.com/github/codeql/pull/4879).

0 commit comments

Comments
 (0)