Skip to content

Commit def4a23

Browse files
smowtonsmowton
authored
Merge pull request #4879 from intrigus-lgtm/java/improve-trustmanager
Java: Add/improve insecure trustmanager query
2 parents e624fb4 + 5aa711a commit def4a23

File tree

13 files changed

+856
-222
lines changed

13 files changed

+856
-222
lines changed

‎java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql

Lines changed: 17 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import semmle.code.java.controlflow.Guards
1515
import semmle.code.java.dataflow.DataFlow
1616
import semmle.code.java.dataflow.FlowSources
1717
import semmle.code.java.security.Encryption
18+
import semmle.code.java.security.SecurityFlag
1819
import DataFlow::PathGraph
1920
private import semmle.code.java.dataflow.ExternalFlow
2021

@@ -86,71 +87,30 @@ private class HostnameVerifierSink extends DataFlow::Node {
8687
HostnameVerifierSink() { sinkNode(this, "set-hostname-verifier") }
8788
}
8889

89-
bindingset[result]
90-
private string getAFlagName() {
91-
result
92-
.regexpMatch("(?i).*(secure|disable|selfCert|selfSign|validat|verif|trust|ignore|nocertificatecheck).*")
93-
}
94-
9590
/**
96-
* A flag has to either be of type `String`, `boolean` or `Boolean`.
91+
* Flags suggesting a deliberately unsafe `HostnameVerifier` usage.
9792
*/
98-
private class FlagType extends Type {
99-
FlagType() {
100-
this instanceof TypeString
101-
or
102-
this instanceof BooleanType
93+
private class UnsafeHostnameVerificationFlag extends FlagKind {
94+
UnsafeHostnameVerificationFlag() { this = "UnsafeHostnameVerificationFlag" }
95+
96+
bindingset[result]
97+
override string getAFlagName() {
98+
result
99+
.regexpMatch("(?i).*(secure|disable|selfCert|selfSign|validat|verif|trust|ignore|nocertificatecheck).*") and
100+
result != "equalsIgnoreCase"
103101
}
104102
}
105103

106-
private predicate isEqualsIgnoreCaseMethodAccess(MethodAccess ma) {
107-
ma.getMethod().hasName("equalsIgnoreCase") and
108-
ma.getMethod().getDeclaringType() instanceof TypeString
109-
}
110-
111-
/** Holds if `source` should is considered a flag. */
112-
private predicate isFlag(DataFlow::Node source) {
113-
exists(VarAccess v | v.getVariable().getName() = getAFlagName() |
114-
source.asExpr() = v and v.getType() instanceof FlagType
115-
)
116-
or
117-
exists(StringLiteral s | s.getRepresentedString() = getAFlagName() | source.asExpr() = s)
118-
or
119-
exists(MethodAccess ma | ma.getMethod().getName() = getAFlagName() |
120-
source.asExpr() = ma and
121-
ma.getType() instanceof FlagType and
122-
not isEqualsIgnoreCaseMethodAccess(ma)
123-
)
124-
}
125-
126-
/** Holds if there is flow from `node1` to `node2` either due to local flow or due to custom flow steps. */
127-
private predicate flagFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
128-
DataFlow::localFlowStep(node1, node2)
129-
or
130-
exists(MethodAccess ma | ma.getMethod() = any(EnvReadMethod m) |
131-
ma = node2.asExpr() and ma.getAnArgument() = node1.asExpr()
132-
)
133-
or
134-
exists(MethodAccess ma |
135-
ma.getMethod().hasName("parseBoolean") and
136-
ma.getMethod().getDeclaringType().hasQualifiedName("java.lang", "Boolean")
137-
|
138-
ma = node2.asExpr() and ma.getAnArgument() = node1.asExpr()
139-
)
104+
/** Gets a guard that represents a (likely) flag controlling an unsafe `HostnameVerifier` use. */
105+
private Guard getAnUnsafeHostnameVerifierFlagGuard() {
106+
result = any(UnsafeHostnameVerificationFlag flag).getAFlag().asExpr()
140107
}
141108

142-
/** Gets a guard that depends on a flag. */
143-
private Guard getAGuard() {
144-
exists(DataFlow::Node source, DataFlow::Node sink |
145-
isFlag(source) and
146-
flagFlowStep*(source, sink) and
147-
sink.asExpr() = result
148-
)
149-
}
150-
151-
/** Holds if `node` is guarded by a flag that suggests an intentionally insecure feature. */
109+
/** Holds if `node` is guarded by a flag that suggests an intentionally insecure use. */
152110
private predicate isNodeGuardedByFlag(DataFlow::Node node) {
153-
exists(Guard g | g.controls(node.asExpr().getBasicBlock(), _) | g = getAGuard())
111+
exists(Guard g | g.controls(node.asExpr().getBasicBlock(), _) |
112+
g = getASecurityFeatureFlagGuard() or g = getAnUnsafeHostnameVerifierFlagGuard()
113+
)
154114
}
155115

156116
from

‎java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.java

Lines changed: 0 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -1,45 +1,5 @@
11
public static void main(String[] args) {
22

3-
{
4-
X509TrustManager trustAllCertManager = new X509TrustManager() {
5-
@Override
6-
public void checkClientTrusted(final X509Certificate[] chain, final String authType)
7-
throws CertificateException {
8-
}
9-
10-
@Override
11-
public void checkServerTrusted(final X509Certificate[] chain, final String authType)
12-
throws CertificateException {
13-
// BAD: trust any server cert
14-
}
15-
16-
@Override
17-
public X509Certificate[] getAcceptedIssuers() {
18-
return null; //BAD: doesn't check cert issuer
19-
}
20-
};
21-
}
22-
23-
{
24-
X509TrustManager trustCertManager = new X509TrustManager() {
25-
@Override
26-
public void checkClientTrusted(final X509Certificate[] chain, final String authType)
27-
throws CertificateException {
28-
}
29-
30-
@Override
31-
public void checkServerTrusted(final X509Certificate[] chain, final String authType)
32-
throws CertificateException {
33-
pkixTrustManager.checkServerTrusted(chain, authType); //GOOD: validate the server cert
34-
}
35-
36-
@Override
37-
public X509Certificate[] getAcceptedIssuers() {
38-
return new X509Certificate[0]; //GOOD: Validate the cert issuer
39-
}
40-
};
41-
}
42-
433
{
444
SSLContext sslContext = SSLContext.getInstance("TLS");
455
SSLEngine sslEngine = sslContext.createSSLEngine();

‎java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.qhelp

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,19 +4,18 @@
44
<qhelp>
55

66
<overview>
7-
<p>Java offers two mechanisms for SSL authentication - trust manager and hostname verifier (checked by the <code>java/insecure-hostname-verifier</code> query). Trust manager validates the peer's certificate chain while hostname verification establishes that the hostname in the URL matches the hostname in the server's identification.</p>
8-
<p>And when SSLSocket or SSLEngine is created without a valid parameter of setEndpointIdentificationAlgorithm, hostname verification is disabled by default.</p>
7+
<p>When SSLSocket or SSLEngine is created without a valid parameter of setEndpointIdentificationAlgorithm, hostname verification is disabled by default.</p>
98
<p>Unsafe implementation of the interface X509TrustManager and SSLSocket/SSLEngine ignores all SSL certificate validation errors when establishing an HTTPS connection, thereby making the app vulnerable to man-in-the-middle attacks.</p>
10-
<p>This query checks whether trust manager is set to trust all certificates or setEndpointIdentificationAlgorithm is missing. The query also covers a special implementation com.rabbitmq.client.ConnectionFactory.</p>
9+
<p>This query checks whether setEndpointIdentificationAlgorithm is missing. The query also covers a special implementation com.rabbitmq.client.ConnectionFactory.</p>
1110
</overview>
1211

1312
<recommendation>
1413
<p>Validate SSL certificate in SSL authentication.</p>
1514
</recommendation>
1615

1716
<example>
18-
<p>The following two examples show two ways of configuring X509 trust cert manager. In the 'BAD' case,
19-
no validation is performed thus any certificate is trusted. In the 'GOOD' case, the proper validation is performed.</p>
17+
<p>The following two examples show two ways of configuring SSLSocket/SSLEngine. In the 'BAD' case,
18+
setEndpointIdentificationAlgorithm is not called, thus no hostname verification takes place. In the 'GOOD' case, setEndpointIdentificationAlgorithm is called.</p>
2019
<sample src="UnsafeCertTrust.java" />
2120
</example>
2221

@@ -25,9 +24,6 @@ no validation is performed thus any certificate is trusted. In the 'GOOD' case,
2524
<a href="https://cwe.mitre.org/data/definitions/273.html">CWE-273</a>
2625
</li>
2726
<li>
28-
<a href="https://support.google.com/faqs/answer/6346016?hl=en">How to fix apps containing an unsafe implementation of TrustManager</a>
29-
</li>
30-
<li>
3127
<a href="https://github.com/OWASP/owasp-mstg/blob/master/Document/0x05g-Testing-Network-Communication.md">Testing Endpoint Identify Verification (MSTG-NETWORK-3)</a>
3228
</li>
3329
<li>

‎java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql

Lines changed: 1 addition & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
/**
22
* @name Unsafe certificate trust
3-
* @description Unsafe implementation of the interface X509TrustManager and
4-
* SSLSocket/SSLEngine ignores all SSL certificate validation
3+
* @description SSLSocket/SSLEngine ignores all SSL certificate validation
54
* errors when establishing an HTTPS connection, thereby making
65
* the app vulnerable to man-in-the-middle attacks.
76
* @kind problem
@@ -15,49 +14,6 @@
1514
import java
1615
import semmle.code.java.security.Encryption
1716

18-
/**
19-
* X509TrustManager class that blindly trusts all certificates in server SSL authentication
20-
*/
21-
class X509TrustAllManager extends RefType {
22-
X509TrustAllManager() {
23-
this.getASupertype*() instanceof X509TrustManager and
24-
exists(Method m1 |
25-
m1.getDeclaringType() = this and
26-
m1.hasName("checkServerTrusted") and
27-
m1.getBody().getNumStmt() = 0
28-
) and
29-
exists(Method m2, ReturnStmt rt2 |
30-
m2.getDeclaringType() = this and
31-
m2.hasName("getAcceptedIssuers") and
32-
rt2.getEnclosingCallable() = m2 and
33-
rt2.getResult() instanceof NullLiteral
34-
)
35-
}
36-
}
37-
38-
/**
39-
* The init method of SSLContext with the trust all manager, which is sslContext.init(..., serverTMs, ...)
40-
*/
41-
class X509TrustAllManagerInit extends MethodAccess {
42-
X509TrustAllManagerInit() {
43-
this.getMethod().hasName("init") and
44-
this.getMethod().getDeclaringType() instanceof SSLContext and //init method of SSLContext
45-
(
46-
exists(ArrayInit ai |
47-
this.getArgument(1).(ArrayCreationExpr).getInit() = ai and
48-
ai.getInit(0).(VarAccess).getVariable().getInitializer().getType().(Class).getASupertype*()
49-
instanceof X509TrustAllManager //Scenario of context.init(null, new TrustManager[] { TRUST_ALL_CERTIFICATES }, null);
50-
)
51-
or
52-
exists(Variable v, ArrayInit ai |
53-
this.getArgument(1).(VarAccess).getVariable() = v and
54-
ai.getParent() = v.getAnAssignedValue() and
55-
ai.getInit(0).getType().(Class).getASupertype*() instanceof X509TrustAllManager //Scenario of context.init(null, serverTMs, null);
56-
)
57-
)
58-
}
59-
}
60-
6117
class SSLEngine extends RefType {
6218
SSLEngine() { this.hasQualifiedName("javax.net.ssl", "SSLEngine") }
6319
}
@@ -208,7 +164,6 @@ class RabbitMQEnableHostnameVerificationNotSet extends MethodAccess {
208164

209165
from MethodAccess aa
210166
where
211-
aa instanceof X509TrustAllManagerInit or
212167
aa instanceof SSLEndpointIdentificationNotSet or
213168
aa instanceof RabbitMQEnableHostnameVerificationNotSet
214169
select aa, "Unsafe configuration of trusted certificates"

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

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
public static void main(String[] args) throws Exception {
2+
{
3+
class InsecureTrustManager implements X509TrustManager {
4+
@Override
5+
public X509Certificate[] getAcceptedIssuers() {
6+
return null;
7+
}
8+
9+
@Override
10+
public void checkServerTrusted(X509Certificate[] chain, String authType) throws CertificateException {
11+
// BAD: Does not verify the certificate chain, allowing any certificate.
12+
}
13+
14+
@Override
15+
public void checkClientTrusted(X509Certificate[] chain, String authType) throws CertificateException {
16+
17+
}
18+
}
19+
SSLContext context = SSLContext.getInstance("TLS");
20+
TrustManager[] trustManager = new TrustManager[] { new InsecureTrustManager() };
21+
context.init(null, trustManager, null);
22+
}
23+
{
24+
SSLContext context = SSLContext.getInstance("TLS");
25+
File certificateFile = new File("path/to/self-signed-certificate");
26+
// Create a `KeyStore` with default type
27+
KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType());
28+
// `keyStore` is initially empty
29+
keyStore.load(null, null);
30+
X509Certificate generatedCertificate;
31+
try (InputStream cert = new FileInputStream(certificateFile)) {
32+
generatedCertificate = (X509Certificate) CertificateFactory.getInstance("X509")
33+
.generateCertificate(cert);
34+
}
35+
// Add the self-signed certificate to the key store
36+
keyStore.setCertificateEntry(certificateFile.getName(), generatedCertificate);
37+
// Get default `TrustManagerFactory`
38+
TrustManagerFactory tmf = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
39+
// Use it with our key store that trusts our self-signed certificate
40+
tmf.init(keyStore);
41+
TrustManager[] trustManagers = tmf.getTrustManagers();
42+
context.init(null, trustManagers, null);
43+
// GOOD, we are not using a custom `TrustManager` but instead have
44+
// added the self-signed certificate we want to trust to the key
45+
// store. Note, the `trustManagers` will **only** trust this one
46+
// certificate.
47+
48+
URL url = new URL("https://self-signed.badssl.com/");
49+
HttpsURLConnection conn = (HttpsURLConnection) url.openConnection();
50+
conn.setSSLSocketFactory(context.getSocketFactory());
51+
}
52+
}

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

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<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.
9+
</p>
10+
11+
<p>
12+
An attack might look like this:
13+
</p>
14+
15+
<ol>
16+
<li>The vulnerable program connects to <code>https://example.com</code>.</li>
17+
<li>The attacker intercepts this connection and presents a valid, self-signed certificate for <code>https://example.com</code>.</li>
18+
<li>The vulnerable program calls the <code>checkServerTrusted</code> method to check whether it should trust the certificate.</li>
19+
<li>The <code>checkServerTrusted</code> method of your <code>TrustManager</code> does not throw a <code>CertificateException</code>.</li>
20+
<li>The vulnerable program accepts the certificate and proceeds with the connection since your <code>TrustManager</code> implicitly trusted it by not throwing an exception.</li>
21+
<li>The attacker can now read the data your program sends to <code>https://example.com</code> and/or alter its replies while the program thinks the connection is secure.</li>
22+
</ol>
23+
</overview>
24+
25+
<recommendation>
26+
<p>
27+
Do not use a custom <code>TrustManager</code> that trusts any certificate.
28+
If you have to use a self-signed certificate, don't trust every certificate, but instead only trust this specific certificate.
29+
See below for an example of how to do this.
30+
</p>
31+
32+
</recommendation>
33+
34+
<example>
35+
<p>
36+
In the first (bad) example, the <code>TrustManager</code> never throws a <code>CertificateException</code> and therefore implicitly trusts any certificate.
37+
This allows an attacker to perform a machine-in-the-middle attack.
38+
In the second (good) example, the self-signed certificate that should be trusted
39+
is loaded into a <code>KeyStore</code>. This explicitly defines the certificate as trusted and there is no need to create a custom <code>TrustManager</code>.
40+
</p>
41+
<sample src="InsecureTrustManager.java" />
42+
</example>
43+
44+
<references>
45+
<li>Android Develoers:<a href="https://developer.android.com/training/articles/security-ssl">Security with HTTPS and SSL</a>.</li>
46+
</references>
47+
</qhelp>

0 commit comments

Comments
 (0)