8000 Java: Remove overlapping code · github/codeql@58b293b · GitHub
[go: up one dir, main page]
Skip to content

Commit 58b293b

Browse files
intrigusintrigus
committed
Java: Remove overlapping code
1 parent 06231dc commit 58b293b

File tree

4 files changed

+5
-159
lines changed

4 files changed

+5
-159
lines changed

‎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 & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/**
22
* @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.
3+
* @description Using a SSLSocket/SSLEngine that 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
@@ -10,49 +10,6 @@
1010
import java
1111
import semmle.code.java.security.Encryption
1212

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

204161
from MethodAccess aa
205162
where
206-
aa instanceof X509TrustAllManagerInit or
207163
aa instanceof SSLEndpointIdentificationNotSet or
208164
aa instanceof RabbitMQEnableHostnameVerificationNotSet
209165
select aa, "Unsafe configuration of trusted certificates"

‎java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrustTest.java

Lines changed: 0 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -18,72 +18,6 @@
1818

1919
public class UnsafeCertTrustTest {
2020

21-
/**
22-
* Test the implementation of trusting all server certs as a variable
23-
*/
24-
public SSLSocketFactory testTrustAllCertManager() {
25-
try {
26-
final SSLContext context = SSLContext.getInstance("TLS");
27-
context.init(null, new TrustManager[] { TRUST_ALL_CERTIFICATES }, null);
28-
final SSLSocketFactory socketFactory = context.getSocketFactory();
29-
return socketFactory;
30-
} catch (final Exception x) {
31-
throw new RuntimeException(x);
32-
}
33-
}
34-
35-
/**
36-
* Test the implementation of trusting all server certs as an anonymous class
37-
*/
38-
public SSLSocketFactory testTrustAllCertManagerOfVariable() {
39-
try {
40-
SSLContext context = SSLContext.getInstance("TLS");
41-
TrustManager[] serverTMs = new TrustManager[] { new X509TrustAllManager() };
42-
context.init(null, serverTMs, null);
43-
44-
final SSLSocketFactory socketFactory = context.getSocketFactory();
45-
return socketFactory;
46-
} catch (final Exception x) {
47-
throw new RuntimeException(x);
48-
}
49-
}
50-
51-
private static final X509TrustManager TRUST_ALL_CERTIFICATES = new X509TrustManager() {
52-
@Override
53-
public void checkClientTrusted(final X509Certificate[] chain, final String authType)
54-
throws CertificateException {
55-
}
56-
57-
@Override
58-
public void checkServerTrusted(final X509Certificate[] chain, final String authType)
59-
throws CertificateException {
60-
// Noncompliant
61-
}
62-
63-
@Override
64-
public X509Certificate[] getAcceptedIssuers() {
65-
return null; // Noncompliant
66-
}
67-
};
68-
69-
private class X509TrustAllManager implements X509TrustManager {
70-
@Override
71-
public void checkClientTrusted(final X509Certificate[] chain, final String authType)
72-
throws CertificateException {
73-
}
74-
75-
@Override
76-
public void checkServerTrusted(final X509Certificate[] chain, final String authType)
77-
throws CertificateException {
78-
// Noncompliant
79-
}
80-
81-
@Override
82-
public X509Certificate[] getAcceptedIssuers() {
83-
return null; // Noncompliant
84-
}
85-
};
86-
8721
/**
8822
* Test the endpoint identification of SSL engine is set to null
8923
*/

0 commit comments

Comments
 (0)
0