Skip to content

Commit 29935e1

Browse files
aschackmullaschackmull
authored
Merge pull request #4771 from intrigus-lgtm/split-cwe-295
Java: Add unsafe hostname verification query and remove existing overlapping query
2 parents 1c8547c + 2931e1f commit 29935e1

File tree

14 files changed

+403
-110
lines changed

14 files changed

+403
-110
lines changed

‎java/change-notes/2021-01-12-unsafe-hostname-verification.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
lgtm,codescanning
2+
* A new query "Unsafe hostname verification"
3+
(`java/unsafe-hostname-verification`) has been added. This query finds unsafe
4+
`HostnameVerifier`s that allow man-in-the-middle attacks.

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
public static void main(String[] args) {
2+
3+
{
4+
HostnameVerifier verifier = new HostnameVerifier() {
5+
@Override
6+
public boolean verify(String hostname, SSLSession session) {
7+
return true; // BAD: accept even if the hostname doesn't match
8+
}
9+
};
10+
HttpsURLConnection.setDefaultHostnameVerifier(verifier);
11+
}
12+
13+
{
14+
HostnameVerifier verifier = new HostnameVerifier() {
15+
@Override
16+
public boolean verify(String hostname, SSLSession session) {
17+
try { // GOOD: verify the certificate
18+
Certificate[] certs = session.getPeerCertificates();
19+
X509Certificate x509 = (X509Certificate) certs[0];
20+
check(new String[]{host}, x509);
21+
return true;
22+
} catch (SSLException e) {
23+
return false;
24+
}
25+
}
26+
};
27+
HttpsURLConnection.setDefaultHostnameVerifier(verifier);
28+
}
29+
30+
}

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

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
If a <code>HostnameVerifier</code> always returns <code>true</code> it will not verify the hostname at all.
8+
This stops Transport Layer Security (TLS) providing any security and allows an attacker to perform a man-in-the-middle attack against the application.
9+
</p>
10+
11+
<p>
12+
An attack might look like this:
13+
</p>
14+
15+
<ol>
16+
<li>The program connects to <code>https://example.com</code>.</li>
17+
<li>The attacker intercepts this connection and presents an apparently-valid certificate of their choosing.</li>
18+
<li>The <code>TrustManager</code> of the program verifies that the certificate has been issued by a trusted certificate authority.</li>
19+
<li>The Java HTTPS library checks whether the certificate has been issued for the host <code>example.com</code>. This check fails because the certificate has been issued for a domain controlled by the attacker, for example: <code>malicious.domain</code>.</li>
20+
<li>The HTTPS library wants to reject the certificate because the hostname does not match. Before doing this it checks whether a <code>HostnameVerifier</code> exists.</li>
21+
<li>Your <code>HostnameVerifier</code> is called which returns <code>true</code> for any certificate so also for this one.</li>
22+
<li>The program proceeds with the connection since your <code>HostnameVerifier</code> accepted it.</li>
23+
<li>The attacker can now read the data your program sends to <code>https://example.com</code>
24+
and/or alter its replies while the program thinks the connection is secure.</li>
25+
</ol>
26+
27+
</overview>
28+
29+
<recommendation>
30+
<p>
31+
Do not use an open <code>HostnameVerifier</code>.
32+
If you have a configuration problem with TLS/HTTPS, you should always solve the configuration problem instead of using an open verifier.
33+
</p>
34+
35+
</recommendation>
36+
37+
<example>
38+
<p>
39+
In the first (bad) example, the <code>HostnameVerifier</code> always returns <code>true</code>.
40+
This allows an attacker to perform a man-in-the-middle attack, because any certificate is accepted despite an incorrect hostname.
41+
In the second (good) example, the <code>HostnameVerifier</code> only returns <code>true</code> when the certificate has been correctly checked.
42+
</p>
43+
<sample src="UnsafeHostnameVerification.java" />
44+
</example>
45+
46+
<references>
47+
<li>Android developers: <a href="https://developer.android.com/training/articles/security-ssl">Security with HTTPS and SSL</a>.</li>
48+
<li>Terse systems blog: <a href="https://tersesystems.com/blog/2014/03/23/fixing-hostname-verification/">Fixing Hostname Verification</a>.</li>
49+
</references>
50+
</qhelp>

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

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,163 @@
1+
/**
2+
* @name Unsafe hostname verification
3+
* @description Marking a certificate as valid for a host without checking the certificate hostname 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/unsafe-hostname-verification
8+
* @tags security
9+
* external/cwe/cwe-297
10+
*/
11+
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 DataFlow::PathGraph
18+
19+
/**
20+
* Holds if `m` always returns `true` ignoring any exceptional flow.
21+
*/
22+
private predicate alwaysReturnsTrue(HostnameVerifierVerify m) {
23+
forex(ReturnStmt rs | rs.getEnclosingCallable() = m |
24+
rs.getResult().(CompileTimeConstantExpr).getBooleanValue() = true
25+
)
26+
}
27+
28+
/**
29+
* A class that overrides the `javax.net.ssl.HostnameVerifier.verify` method and **always** returns `true` (though it could also exit due to an uncaught exception), thus
30+
* accepting any certificate despite a hostname mismatch.
31+
*/
32+
class TrustAllHostnameVerifier extends RefType {
33+
TrustAllHostnameVerifier() {
34+
this.getASupertype*() instanceof HostnameVerifier and
35+
exists(HostnameVerifierVerify m |
36+
m.getDeclaringType() = this and
37+
alwaysReturnsTrue(m)
38+
)
39+
}
40+
}
41+
42+
/**
43+
* A configuration to model the flow of a `TrustAllHostnameVerifier` to a `set(Default)HostnameVerifier` call.
44+
*/
45+
class TrustAllHostnameVerifierConfiguration extends DataFlow::Configuration {
46+
TrustAllHostnameVerifierConfiguration() { this = "TrustAllHostnameVerifierConfiguration" }
47+
48+
override predicate isSource(DataFlow::Node source) {
49+
source.asExpr().(ClassInstanceExpr).getConstructedType() instanceof TrustAllHostnameVerifier
50+
}
51+
52+
override predicate isSink(DataFlow::Node sink) {
53+
exists(MethodAccess ma, Method m |
54+
(m instanceof SetDefaultHostnameVerifierMethod or m instanceof SetHostnameVerifierMethod) and
55+
ma.getMethod() = m
56+
|
57+
ma.getArgument(0) = sink.asExpr()
58+
)
59+
}
60+
61+
override predicate isBarrier(DataFlow::Node barrier) {
62+
// ignore nodes that are in functions that intentionally disable hostname verification
63+
barrier
64+
.getEnclosingCallable()
65+
.getName()
66+
/*
67+
* Regex: (_)* :
68+
* some methods have underscores.
69+
* Regex: (no|ignore|disable)(strictssl|ssl|verify|verification|hostname)
70+
* noStrictSSL ignoreSsl
71+
* Regex: (set)?(accept|trust|ignore|allow)(all|every|any)
72+
* acceptAll trustAll ignoreAll setTrustAnyHttps
73+
* Regex: (use|do|enable)insecure
74+
* useInsecureSSL
75+
* Regex: (set|do|use)?no.*(check|validation|verify|verification)
76+
* setNoCertificateCheck
77+
* Regex: disable
78+
* disableChecks
79+
*/
80+
81+
.regexpMatch("^(?i)(_)*((no|ignore|disable)(strictssl|ssl|verify|verification|hostname)" +
82+
"|(set)?(accept|trust|ignore|allow)(all|every|any)" +
83+
"|(use|do|enable)insecure|(set|do|use)?no.*(check|validation|verify|verification)|disable).*$")
84+
}
85+
}
86+
87+
bindingset[result]
88+
private string getAFlagName() {
89+
result
90+
.regexpMatch("(?i).*(secure|disable|selfCert|selfSign|validat|verif|trust|ignore|nocertificatecheck).*")
91+
}
92+
93+
/**
94+
* A flag has to either be of type `String`, `boolean` or `Boolean`.
95+
*/
96+
private class FlagType extends Type {
97+
FlagType() {
98+
this instanceof TypeString
99+
or
100+
this instanceof BooleanType
101+
}
102+
}
103+
104+
private predicate isEqualsIgnoreCaseMethodAccess(MethodAccess ma) {
105+
ma.getMethod().hasName("equalsIgnoreCase") and
106+
ma.getMethod().getDeclaringType() instanceof TypeString
107+
}
108+
109+
/** Holds if `source` should is considered a flag. */
110+
private predicate isFlag(DataFlow::Node source) {
111+
exists(VarAccess v | v.getVariable().getName() = getAFlagName() |
112+
source.asExpr() = v and v.getType() instanceof FlagType
113+
)
114+
or
115+
exists(StringLiteral s | s.getRepresentedString() = getAFlagName() | source.asExpr() = s)
116+
or
117+
exists(MethodAccess ma | ma.getMethod().getName() = getAFlagName() |
118+
source.asExpr() = ma and
119+
ma.getType() instanceof FlagType and
120+
not isEqualsIgnoreCaseMethodAccess(ma)
121+
)
122+
}
123+
124+
/** Holds if there is flow from `node1` to `node2` either due to local flow or due to custom flow steps. */
125+
private predicate flagFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
126+
DataFlow::localFlowStep(node1, node2)
127+
or
128+
exists(MethodAccess ma | ma.getMethod() = any(EnvReadMethod m) |
129+
ma = node2.asExpr() and ma.getAnArgument() = node1.asExpr()
130+
)
131+
or
132+
exists(MethodAccess ma |
133+
ma.getMethod().hasName("parseBoolean") and
134+
ma.getMethod().getDeclaringType().hasQualifiedName("java.lang", "Boolean")
135+
|
136+
ma = node2.asExpr() and ma.getAnArgument() = node1.asExpr()
137+
)
138+
}
139+
140+
/** Gets a guard that depends on a flag. */
141+
private Guard getAGuard() {
142+
exists(DataFlow::Node source, DataFlow::Node sink |
143+
isFlag(source) and
144+
flagFlowStep*(source, sink) and
145+
sink.asExpr() = result
146+
)
147+
}
148+
149+
/** Holds if `node` is guarded by a flag that suggests an intentionally insecure feature. */
150+
private predicate isNodeGuardedByFlag(DataFlow::Node node) {
151+
exists(Guard g | g.controls(node.asExpr().getBasicBlock(), _) | g = getAGuard())
152+
}
153+
154+
from
155+
DataFlow::PathNode source, DataFlow::PathNode sink, TrustAllHostnameVerifierConfiguration cfg,
156+
RefType verifier
157+
where
158+
cfg.hasFlowPath(source, sink) and
159+
not isNodeGuardedByFlag(sink.getNode()) and
160+
verifier = source.getNode().asExpr().(ClassInstanceExpr).getConstructedType()
161+
select sink, source, sink,
162+
"$@ that is defined $@ and accepts any certificate as valid, is used here.", source,
163+
"This hostname verifier", verifier, "here"

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

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,30 +1,4 @@
11
public static void main(String[] args) {
2-
{
3-
HostnameVerifier verifier = new HostnameVerifier() {
4-
@Override
5-
public boolean verify(String hostname, SSLSession session) {
6-
try { //GOOD: verify the certificate
7-
Certificate[] certs = session.getPeerCertificates();
8-
X509Certificate x509 = (X509Certificate) certs[0];
9-
check(new String[]{host}, x509);
10-
return true;
11-
} catch (SSLException e) {
12-
return false;
13-
}
14-
}
15-
};
16-
HttpsURLConnection.setDefaultHostnameVerifier(verifier);
17-
}
18-
19-
{
20-
HostnameVerifier verifier = new HostnameVerifier() {
21-
@Override
22-
public boolean verify(String hostname, SSLSession session) {
23-
return true; // BAD: accept even if the hostname doesn't match
24-
}
25-
};
26-
HttpsURLConnection.setDefaultHostnameVerifier(verifier);
27-
}
282

293
{
304
X509TrustManager trustAllCertManager = new X509TrustManager() {

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

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

66
<overview>
7-
<p>Java offers two mechanisms for SSL authentication - trust manager and hostname verifier. 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>
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>
88
<p>And when SSLSocket or SSLEngine is created without a valid parameter of setEndpointIdentificationAlgorithm, hostname verification is disabled by default.</p>
9-
<p>Unsafe implementation of the interface X509TrustManager, HostnameVerifier, 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, the hostname verifier is turned off, or setEndpointIdentificationAlgorithm is missing. The query also covers a special implementation com.rabbitmq.client.ConnectionFactory.</p>
9+
<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>
1111
</overview>
1212

1313
<recommendation>
1414
<p>Validate SSL certificate in SSL authentication.</p>
1515
</recommendation>
1616

1717
<example>
18-
<p>The following two examples show two ways of configuring X509 trust cert manager and hostname verifier. In the 'BAD' case,
18+
<p>The following two examples show two ways of configuring X509 trust cert manager. In the 'BAD' case,
1919
no validation is performed thus any certificate is trusted. In the 'GOOD' case, the proper validation is performed.</p>
2020
<sample src="UnsafeCertTrust.java" />
2121
</example>

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

Lines changed: 2 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/**
2-
* @name Unsafe certificate trust and improper hostname verification
3-
* @description Unsafe implementation of the interface X509TrustManager, HostnameVerifier, 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.
2+
* @name Unsafe certificate trust
3+
* @description 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.
44
* @kind problem
55
* @id java/unsafe-cert-trust
66
* @tags security
@@ -53,42 +53,6 @@ class X509TrustAllManagerInit extends MethodAccess {
5353
}
5454
}
5555

56-
/**
57-
* HostnameVerifier class that allows a certificate whose CN (Common Name) does not match the host name in the URL
58-
*/
59-
class TrustAllHostnameVerifier extends RefType {
60-
TrustAllHostnameVerifier() {
61-
this.getASupertype*() instanceof HostnameVerifier and
62-
exists(Method m, ReturnStmt rt |
63-
m.getDeclaringType() = this and
64-
m.hasName("verify") and
65-
rt.getEnclosingCallable() = m and
66-
rt.getResult().(BooleanLiteral).getBooleanValue() = true
67-
)
68-
}
69-
}
70-
71-
/**
72-
* The setDefaultHostnameVerifier method of HttpsURLConnection with the trust all configuration
73-
*/
74-
class TrustAllHostnameVerify extends MethodAccess {
75-
TrustAllHostnameVerify() {
76-
this.getMethod().hasName("setDefaultHostnameVerifier") and
77-
this.getMethod().getDeclaringType() instanceof HttpsURLConnection and //httpsURLConnection.setDefaultHostnameVerifier method
78-
(
79-
exists(NestedClass nc |
80-
nc.getASupertype*() instanceof TrustAllHostnameVerifier and
81-
this.getArgument(0).getType() = nc //Scenario of HttpsURLConnection.setDefaultHostnameVerifier(new HostnameVerifier() {...});
82-
)
83-
or
84-
exists(Variable v |
85-
this.getArgument(0).(VarAccess).getVariable() = v and
86-
v.getInitializer().getType() instanceof TrustAllHostnameVerifier //Scenario of HttpsURLConnection.setDefaultHostnameVerifier(verifier);
87-
)
88-
)
89-
}
90-
}
91-
9256
class SSLEngine extends RefType {
9357
SSLEngine() { this.hasQualifiedName("javax.net.ssl", "SSLEngine") }
9458
}
@@ -239,7 +203,6 @@ class RabbitMQEnableHostnameVerificationNotSet extends MethodAccess {
239203

240204
from MethodAccess aa
241205
where
242-
aa instanceof TrustAllHostnameVerify or
243206
aa instanceof X509TrustAllManagerInit or
244207
aa instanceof SSLEndpointIdentificationNotSet or
245208
aa instanceof RabbitMQEnableHostnameVerificationNotSet

‎java/ql/src/semmle/code/java/dataflow/FlowSources.qll

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,7 @@ class EnvInput extends LocalUserInput {
226226
)
227227
or
228228
// Results from various specific methods.
229-
this.asExpr().(MethodAccess).getMethod() instanceof EnvTaintedMethod
229+
this.asExpr().(MethodAccess).getMethod() instanceof EnvReadMethod
230230
or
231231
// Access to `System.in`.
232232
exists(Field f | this.asExpr() = f.getAnAccess() | f instanceof SystemIn)
@@ -292,8 +292,9 @@ private class SpringWebRequestGetMethod extends Method {
292292
}
293293
}
294294

295-
private class EnvTaintedMethod extends Method {
296-
EnvTaintedMethod() {
295+
/** A method that reads from the environment, such as `System.getProperty` or `System.getenv`. */
296+
class EnvReadMethod extends Method {
297+
EnvReadMethod() {
297298
this instanceof MethodSystemGetenv or
298299
this instanceof PropertiesGetPropertyMethod or
299300
this instanceof MethodSystemGetProperty

0 commit comments

Comments
 (0)