Skip to content

Commit c09b669

Browse files
atorralbaatorralba
authored
Merge pull request #6171 from atorralba/atorralba/promote-unsafe-certificate-trust
Java: Promote Unsafe certificate trust query from experimental
2 parents f154530 + 695e77a commit c09b669

File tree

20 files changed

+791
-291
lines changed

20 files changed

+791
-291
lines changed

‎java/ql/lib/semmle/code/java/frameworks/Networking.qll

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,11 @@ class TypeSocket extends RefType {
1414
TypeSocket() { this.hasQualifiedName("java.net", "Socket") }
1515
}
1616

17+
/** The type `javax.net.SocketFactory` */
18+
class TypeSocketFactory extends RefType {
19+
TypeSocketFactory() { this.hasQualifiedName("javax.net", "SocketFactory") }
20+
}
21+
1722
/** The type `java.net.URL`. */
1823
class TypeUrl extends RefType {
1924
TypeUrl() { this.hasQualifiedName("java.net", "URL") }
@@ -42,6 +47,15 @@ class SocketGetInputStreamMethod extends Method {
4247
}
4348
}
4449

50+
/** The method `java.net.Socket::getOutputStream`. */
51+
class SocketGetOutputStreamMethod extends Method {
52+
SocketGetOutputStreamMethod() {
53+
this.getDeclaringType() instanceof TypeSocket and
54+
this.hasName("getOutputStream") and
55+
this.hasNoParameters()
56+
}
57+
}
58+
4559
/** A method or constructor call that returns a new `URI`. */
4660
class UriCreation extends Call {
4761
UriCreation() {
@@ -143,6 +157,22 @@ class UrlOpenConnectionMethod extends Method {
143157
}
144158
}
145159

160+
/** The method `javax.net.SocketFactory::createSocket`. */
161+
class CreateSocketMethod extends Method {
162+
CreateSocketMethod() {
163+
this.hasName("createSocket") and
164+
this.getDeclaringType().getASupertype*() instanceof TypeSocketFactory
165+
}
166+
}
167+
168+
/** The method `javax.net.Socket::connect`. */
169+
class SocketConnectMethod extends Method {
170+
SocketConnectMethod() {
171+
this.hasName("connect") and
172+
this.getDeclaringType() instanceof TypeSocket
173+
}
174+
}
175+
146176
/**
147177
* A string matching private host names of IPv4 and IPv6, which only matches the host portion therefore checking for port is not necessary.
148178
* Several examples are localhost, reserved IPv4 IP addresses including 127.0.0.1, 10.x.x.x, 172.16.x,x, 192.168.x,x, and reserved IPv6 addresses including [0:0:0:0:0:0:0:1] and [::1]

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

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,21 @@ class SSLSession extends RefType {
3434
SSLSession() { this.hasQualifiedName("javax.net.ssl", "SSLSession") }
3535
}
3636

37+
/** The `javax.net.ssl.SSLEngine` class. */
38+
class SSLEngine extends RefType {
39+
SSLEngine() { this.hasQualifiedName("javax.net.ssl", "SSLEngine") }
40+
}
41+
42+
/** The `javax.net.ssl.SSLSocket` class. */
43+
class SSLSocket extends RefType {
44+
SSLSocket() { this.hasQualifiedName("javax.net.ssl", "SSLSocket") }
45+
}
46+
47+
/** The `javax.net.ssl.SSLParameters` class. */
48+
class SSLParameters extends RefType {
49+
SSLParameters() { this.hasQualifiedName("javax.net.ssl", "SSLParameters") }
50+
}
51+
3752
class HostnameVerifier extends RefType {
3853
HostnameVerifier() { this.hasQualifiedName("javax.net.ssl", "HostnameVerifier") }
3954
}
@@ -79,6 +94,14 @@ class GetSocketFactory extends Method {
7994
}
8095
}
8196

97+
/** The `createSSLEngine` method of the class `javax.net.ssl.SSLContext` */
98+
class CreateSslEngineMethod extends Method {
99+
CreateSslEngineMethod() {
100+
this.hasName("createSSLEngine") and
101+
this.getDeclaringType() instanceof SSLContext
102+
}
103+
}
104+
82105
class SetConnectionFactoryMethod extends Method {
83106
SetConnectionFactoryMethod() {
84107
this.hasName("setSSLSocketFactory") and
@@ -101,6 +124,38 @@ class SetDefaultHostnameVerifierMethod extends Method {
101124
}
102125
}
103126

127+
/** The `beginHandshake` method of the class `javax.net.ssl.SSLEngine`. */
128+
class BeginHandshakeMethod extends Method {
129+
BeginHandshakeMethod() {
130+
this.hasName("beginHandshake") and
131+
this.getDeclaringType().getASupertype*() instanceof SSLEngine
132+
}
133+
}
134+
135+
/** The `wrap` method of the class `javax.net.ssl.SSLEngine`. */
136+
class SslWrapMethod extends Method {
137+
SslWrapMethod() {
138+
this.hasName("wrap") and
139+
this.getDeclaringType().getASupertype*() instanceof SSLEngine
140+
}
141+
}
142+
143+
/** The `unwrap` method of the class `javax.net.ssl.SSLEngine`. */
144+
class SslUnwrapMethod extends Method {
145+
SslUnwrapMethod() {
146+
this.hasName("unwrap") and
147+
this.getDeclaringType().getASupertype*() instanceof SSLEngine
148+
}
149+
}
150+
151+
/** The `getSession` method of the class `javax.net.ssl.SSLSession`.select */
152+
class GetSslSessionMethod extends Method {
153+
GetSslSessionMethod() {
154+
hasName("getSession") and
155+
getDeclaringType().getASupertype*() instanceof SSLSession
156+
}
157+
}
158+
104159
bindingset[algorithmString]
105160
private string algorithmRegex(string algorithmString) {
106161
// Algorithms usually appear in names surrounded by characters that are not

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

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
/** Provides classes and predicates to reason about unsafe certificate trust vulnerablities. */
2+
3+
import java
4+
private import semmle.code.java.frameworks.Networking
5+
private import semmle.code.java.security.Encryption
6+
private import semmle.code.java.dataflow.DataFlow
7+
8+
/**
9+
* The creation of an object that prepares an SSL connection.
10+
*
11+
* This is a source for `SslEndpointIdentificationFlowConfig`.
12+
*/
13+
class SslConnectionInit extends DataFlow::Node {
14+
SslConnectionInit() {
15+
exists(MethodAccess ma, Method m |
16+
this.asExpr() = ma and
17+
ma.getMethod() = m
18+
|
19+
m instanceof CreateSslEngineMethod
20+
or
21+
m instanceof CreateSocketMethod and isSslSocket(ma)
22+
)
23+
}
24+
}
25+
26+
/**
27+
* A call to a method that establishes an SSL connection.
28+
*
29+
* This is a sink for `SslEndpointIdentificationFlowConfig`.
30+
*/
31+
class SslConnectionCreation extends DataFlow::Node {
32+
SslConnectionCreation() {
33+
exists(MethodAccess ma, Method m |
34+
m instanceof BeginHandshakeMethod or
35+
m instanceof SslWrapMethod or
36+
m instanceof SslUnwrapMethod or
37+
m instanceof SocketGetOutputStreamMethod
38+
|
39+
ma.getMethod() = m and
40+
this.asExpr() = ma.getQualifier()
41+
)
42+
}
43+
}
44+
45+
/**
46+
* An SSL object that correctly verifies hostnames, or doesn't need to (for instance, because it's a server).
47+
*
48+
* This is a sanitizer for `SslEndpointIdentificationFlowConfig`.
49+
*/
50+
abstract class SslUnsafeCertTrustSanitizer extends DataFlow::Node { }
51+
52+
/**
53+
* An `SSLEngine` set in server mode.
54+
*/
55+
private class SslEngineServerMode extends SslUnsafeCertTrustSanitizer {
56+
SslEngineServerMode() {
57+
exists(MethodAccess ma, Method m |
58+
m.hasName("setUseClientMode") and
59+
m.getDeclaringType().getASupertype*() instanceof SSLEngine and
60+
ma.getMethod() = m and
61+
ma.getArgument(0).(CompileTimeConstantExpr).getBooleanValue() = false and
62+
this.asExpr() = ma.getQualifier()
63+
)
64+
}
65+
}
66+
67+
/**
68+
* Holds if the return value of `createSocket` is cast to `SSLSocket`
69+
* or the qualifier of `createSocket` is an instance of `SSLSocketFactory`.
70+
*/
71+
private predicate isSslSocket(MethodAccess createSocket) {
72+
createSocket = any(CastExpr ce | ce.getType() instanceof SSLSocket).getExpr()
73+
or
74+
createSocket.getQualifier().getType().(RefType).getASupertype*() instanceof SSLSocketFactory
75+
}
76+
77+
/**
78+
* A call to a method that enables SSL (`useSslProtocol` or `setSslContextFactory`)
79+
* on an instance of `com.rabbitmq.client.ConnectionFactory` that doesn't set `enableHostnameVerification`.
80+
*/
81+
class RabbitMQEnableHostnameVerificationNotSet extends MethodAccess {
82+
RabbitMQEnableHostnameVerificationNotSet() {
83+
this.getMethod().hasName(["useSslProtocol", "setSslContextFactory"]) and
84+
this.getMethod().getDeclaringType() instanceof RabbitMQConnectionFactory and
85+
exists(Variable v |
86+
v.getType() instanceof RabbitMQConnectionFactory and
87+
this.getQualifier() = v.getAnAccess() and
88+
not exists(MethodAccess ma |
89+
ma.getMethod().hasName("enableHostnameVerification") and
90+
ma.getQualifier() = v.getAnAccess()
91+
)
92+
)
93+
}
94+
}
95+
96+
private class RabbitMQConnectionFactory extends RefType {
97+
RabbitMQConnectionFactory() { this.hasQualifiedName("com.rabbitmq.client", "ConnectionFactory") }
98+
}

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

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
/** Provides taint tracking configurations to be used by unsafe certificate trust queries. */
2+
3+
import java
4+
import semmle.code.java.dataflow.TaintTracking
5+
import semmle.code.java.security.UnsafeCertTrust
6+
import semmle.code.java.security.Encryption
7+
8+
/**
9+
* A taint flow configuration for SSL connections created without a proper certificate trust configuration.
10+
*/
11+
class SslEndpointIdentificationFlowConfig extends TaintTracking::Configuration {
12+
SslEndpointIdentificationFlowConfig() { this = "SslEndpointIdentificationFlowConfig" }
13+
14+
override predicate isSource(DataFlow::Node source) { source instanceof SslConnectionInit }
15+
16+
override predicate isSink(DataFlow::Node sink) { sink instanceof SslConnectionCreation }
17+
18+
override predicate isSanitizer(DataFlow::Node sanitizer) {
19+
sanitizer instanceof SslUnsafeCertTrustSanitizer
20+
}
21+
}
22+
23+
/**
24+
* An SSL object that was assigned a safe `SSLParameters` object and can be considered safe.
25+
*/
26+
private class SslConnectionWithSafeSslParameters extends SslUnsafeCertTrustSanitizer {
27+
SslConnectionWithSafeSslParameters() {
28+
exists(SafeSslParametersFlowConfig config, DataFlow::Node safe, DataFlow::Node sanitizer |
29+
config.hasFlowTo(safe) and
30+
sanitizer = DataFlow::exprNode(safe.asExpr().(Argument).getCall().getQualifier()) and
31+
DataFlow::localFlow(sanitizer, this)
32+
)
33+
}
34+
}
35+
36+
private class SafeSslParametersFlowConfig extends DataFlow2::Configuration {
37+
SafeSslParametersFlowConfig() { this = "SafeSslParametersFlowConfig" }
38+
39+
override predicate isSource(DataFlow::Node source) {
40+
exists(MethodAccess ma |
41+
ma instanceof SafeSetEndpointIdentificationAlgorithm and
42+
DataFlow::getInstanceArgument(ma) = source.(DataFlow::PostUpdateNode).getPreUpdateNode()
43+
)
44+
}
45+
46+
override predicate isSink(DataFlow::Node sink) {
47+
exists(MethodAccess ma, RefType t | t instanceof SSLSocket or t instanceof SSLEngine |
48+
ma.getMethod().hasName("setSSLParameters") and
49+
ma.getMethod().getDeclaringType().getASupertype*() = t and
50+
ma.getArgument(0) = sink.asExpr()
51+
)
52+
}
53+
}
54+
55+
/**
56+
* A call to `SSLParameters.setEndpointIdentificationAlgorithm` with a non-null and non-empty parameter.
57+
*/
58+
private class SafeSetEndpointIdentificationAlgorithm extends MethodAccess {
59+
SafeSetEndpointIdentificationAlgorithm() {
60+
this.getMethod().hasName("setEndpointIdentificationAlgorithm") and
61+
this.getMethod().getDeclaringType() instanceof SSLParameters and
62+
not this.getArgument(0) instanceof NullLiteral and
63+
not this.getArgument(0).(CompileTimeConstantExpr).getStringValue() = ""
64+
}
65+
}

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

File renamed without changes.

‎java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.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+
6+
<overview>
7+
<p>Java offers two mechanisms for SSL authentication - trust manager and hostname verifier (the later is checked by the <code>java/insecure-hostname-verifier</code> query). The 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>When <code>SSLSocket</code> or <code>SSLEngine</code> are created without a secure <code>setEndpointIdentificationAlgorithm</code>, hostname verification is disabled by default.</p>
9+
<p>This query checks whether <code>setEndpointIdentificationAlgorithm</code> is missing, thereby making the application vulnerable to man-in-the-middle attacks. The query also covers insecure configurations of <code>com.rabbitmq.client.ConnectionFactory</code>.</p>
10+
</overview>
11+
12+
<recommendation>
13+
<p>Validate SSL certificates in SSL authentication.</p>
14+
</recommendation>
15+
16+
<example>
17+
<p>The following two examples show two ways of configuring SSLSocket/SSLEngine. In the 'BAD' case,
18+
<code>setEndpointIdentificationAlgorithm</code> is not called, thus no hostname verification takes place. In the 'GOOD' case, <code>setEndpointIdentificationAlgorithm</code> is called.</p>
19+
<sample src="UnsafeCertTrust.java" />
20+
</example>
21+
22+
<references>
23+
<li>
24+
<a href="https://github.com/OWASP/owasp-mstg/blob/master/Document/0x05g-Testing-Network-Communication.md">Testing Endpoint Identify Verification (MSTG-NETWORK-3)</a>.
25+
</li>
26+
<li>
27+
<a href="https://docs.oracle.com/en/java/javase/11/docs/api/java.base/javax/net/ssl/SSLParameters.html#setEndpointIdentificationAlgorithm(java.lang.String)">SSLParameters.setEndpointIdentificationAlgorithm documentation</a>.
28+
</li>
29+
<li>
30+
RabbitMQ:
31+
<a href="https://rabbitmq.github.io/rabbitmq-java-client/api/current/com/rabbitmq/client/ConnectionFactory.html#enableHostnameVerification()">ConnectionFactory.enableHostnameVerification documentation</a>.
32+
</li>
33+
<li>
34+
RabbitMQ:
35+
<a href="https://www.rabbitmq.com/ssl.html#java-client">Using TLS in the Java Client</a>.
36+
</li>
37+
<li>
38+
<a href="https://github.com/advisories/GHSA-xvch-r4wf-h8w9">CVE-2018-17187: Apache Qpid Proton-J transport issue with hostname verification</a>.
39+
</li>
40+
<li>
41+
<a href="https://github.com/advisories/GHSA-46j3-r4pj-4835">CVE-2018-8034: Apache Tomcat - host name verification when using TLS with the WebSocket client</a>.
42+
</li>
43+
<li>
44+
<a href="https://github.com/advisories/GHSA-w4g2-9hj6-5472">CVE-2018-11087: Pivotal Spring AMQP vulnerability due to lack of hostname validation</a>.
45+
</li>
46+
<li>
47+
<a href="https://github.com/advisories/GHSA-m9w8-v359-9ffr">CVE-2018-11775: TLS hostname verification issue when using the Apache ActiveMQ Client</a>.
48+
</li>
49+
</references>
50+
</qhelp>

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/**
2+
* @name Unsafe certificate trust
3+
* @description SSLSocket/SSLEngine ignores all SSL certificate validation
4+
* errors when establishing an HTTPS connection, thereby making
5+
* the app vulnerable to man-in-the-middle attacks.
6+
* @kind problem
7+
* @problem.severity warning
8+
* @precision medium
9+
* @id java/unsafe-cert-trust
10+
* @tags security
11+
* external/cwe/cwe-273
12+
*/
13+
14+
import java
15+
import semmle.code.java.security.UnsafeCertTrustQuery
16+
17+
from Expr unsafeTrust
18+
where
19+
unsafeTrust instanceof RabbitMQEnableHostnameVerificationNotSet or
20+
exists(SslEndpointIdentificationFlowConfig config |
21+
config.hasFlowTo(DataFlow::exprNode(unsafeTrust))
22+
)
23+
select unsafeTrust, "Unsafe configuration of trusted certificates."

‎java/ql/src/change-notes/2021-06-28-unsafe-cert-trust-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 "Unsafe certificate trust" (`java/unsafe-cert-trust`) 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 @luchua-bc](https://github.com/github/codeql/pull/3550).

0 commit comments

Comments
 (0)