From 2a0a7adcf4b33f8df3e5646c6501eb16f4846ab3 Mon Sep 17 00:00:00 2001 From: intrigus Date: Wed, 21 Oct 2020 22:06:30 +0200 Subject: [PATCH 1/5] Separate hostname verification and trustmanger. This seperates security issues with missing hostname verification (CWE 297) and issues with trustmanagers that accept any certificate as valid (CWE 295). (This is an incomplete separation for now) --- .../CWE/CWE-295/UnsafeCertificateTrust.java | 41 +++ .../CWE/CWE-295/UnsafeCertificateTrust.qhelp | 31 +++ .../UnsafeCertificateTrust.ql} | 0 .../UnsafeHostnameVerification.java} | 40 --- .../UnsafeHostnameVerification.qhelp} | 4 +- .../CWE/CWE-297/UnsafeHostnameVerification.ql | 246 ++++++++++++++++++ .../CWE-295/UnsafeCertificateTrust.expected | 7 + .../CWE-295/UnsafeCertificateTrust.java | 162 ++++++++++++ .../CWE-295/UnsafeCertificateTrust.qlref | 1 + .../UnsafeHostnameVerification.expected | 7 + .../CWE-297/UnsafeHostnameVerification.java | 162 ++++++++++++ .../CWE-297/UnsafeHostnameVerification.qlref | 1 + 12 files changed, 660 insertions(+), 42 deletions(-) create mode 100644 java/ql/src/experimental/Security/CWE/CWE-295/UnsafeCertificateTrust.java create mode 100644 java/ql/src/experimental/Security/CWE/CWE-295/UnsafeCertificateTrust.qhelp rename java/ql/src/experimental/Security/CWE/{CWE-273/UnsafeCertTrust.ql => CWE-295/UnsafeCertificateTrust.ql} (100%) rename java/ql/src/experimental/Security/CWE/{CWE-273/UnsafeCertTrust.java => CWE-297/UnsafeHostnameVerification.java} (67%) rename java/ql/src/experimental/Security/CWE/{CWE-273/UnsafeCertTrust.qhelp => CWE-297/UnsafeHostnameVerification.qhelp} (91%) create mode 100644 java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.ql create mode 100644 java/ql/test/experimental/query-tests/security/CWE-295/UnsafeCertificateTrust.expected create mode 100644 java/ql/test/experimental/query-tests/security/CWE-295/UnsafeCertificateTrust.java create mode 100644 java/ql/test/experimental/query-tests/security/CWE-295/UnsafeCertificateTrust.qlref create mode 100644 java/ql/test/experimental/query-tests/security/CWE-297/UnsafeHostnameVerification.expected create mode 100644 java/ql/test/experimental/query-tests/security/CWE-297/UnsafeHostnameVerification.java create mode 100644 java/ql/test/experimental/query-tests/security/CWE-297/UnsafeHostnameVerification.qlref diff --git a/java/ql/src/experimental/Security/CWE/CWE-295/UnsafeCertificateTrust.java b/java/ql/src/experimental/Security/CWE/CWE-295/UnsafeCertificateTrust.java new file mode 100644 index 000000000000..4d1a1b89aa9c --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-295/UnsafeCertificateTrust.java @@ -0,0 +1,41 @@ +public static void main(String[] args) { + { + X509TrustManager trustAllCertManager = new X509TrustManager() { + @Override + public void checkClientTrusted(final X509Certificate[] chain, final String authType) + throws CertificateException { + } + + @Override + public void checkServerTrusted(final X509Certificate[] chain, final String authType) + throws CertificateException { + // BAD: trust any server cert + } + + @Override + public X509Certificate[] getAcceptedIssuers() { + return null; //BAD: doesn't check cert issuer + } + }; + } + + { + X509TrustManager trustCertManager = new X509TrustManager() { + @Override + public void checkClientTrusted(final X509Certificate[] chain, final String authType) + throws CertificateException { + } + + @Override + public void checkServerTrusted(final X509Certificate[] chain, final String authType) + throws CertificateException { + pkixTrustManager.checkServerTrusted(chain, authType); //GOOD: validate the server cert + } + + @Override + public X509Certificate[] getAcceptedIssuers() { + return new X509Certificate[0]; //GOOD: Validate the cert issuer + } + }; + } +} \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-295/UnsafeCertificateTrust.qhelp b/java/ql/src/experimental/Security/CWE/CWE-295/UnsafeCertificateTrust.qhelp new file mode 100644 index 000000000000..d526c024b67f --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-295/UnsafeCertificateTrust.qhelp @@ -0,0 +1,31 @@ + + + + +

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.

+

And when SSLSocket or SSLEngine is created without a valid parameter of setEndpointIdentificationAlgorithm, hostname verification is disabled by default.

+

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.

+

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.

+ + + +

Validate SSL certificate in SSL authentication.

+ + + +

The following two examples show two ways of configuring X509 trust cert manager and hostname verifier. In the 'BAD' case, +no validation is performed thus any certificate is trusted. In the 'GOOD' case, the proper validation is performed.

+ + + + +
  • +CWE-295 +
    • +
  • +How to fix apps containing an unsafe implementation of TrustManager +
    • +     +     diff --git a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql b/java/ql/src/experimental/Security/CWE/CWE-295/UnsafeCertificateTrust.ql similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql rename to java/ql/src/experimental/Security/CWE/CWE-295/UnsafeCertificateTrust.ql diff --git a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.java b/java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.java similarity index 67% rename from java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.java rename to java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.java index 65698ac33a30..b5999a9fb866 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.java +++ b/java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.java @@ -26,46 +26,6 @@ public boolean verify(String hostname, SSLSession session) { HttpsURLConnection.setDefaultHostnameVerifier(verifier); } - { - X509TrustManager trustAllCertManager = new X509TrustManager() { - @Override - public void checkClientTrusted(final X509Certificate[] chain, final String authType) - throws CertificateException { - } - - @Override - public void checkServerTrusted(final X509Certificate[] chain, final String authType) - throws CertificateException { - // BAD: trust any server cert - } - - @Override - public X509Certificate[] getAcceptedIssuers() { - return null; //BAD: doesn't check cert issuer - } - }; - } - - { - X509TrustManager trustCertManager = new X509TrustManager() { - @Override - public void checkClientTrusted(final X509Certificate[] chain, final String authType) - throws CertificateException { - } - - @Override - public void checkServerTrusted(final X509Certificate[] chain, final String authType) - throws CertificateException { - pkixTrustManager.checkServerTrusted(chain, authType); //GOOD: validate the server cert - } - - @Override - public X509Certificate[] getAcceptedIssuers() { - return new X509Certificate[0]; //GOOD: Validate the cert issuer - } - }; - } - { SSLContext sslContext = SSLContext.getInstance("TLS"); SSLEngine sslEngine = sslContext.createSSLEngine(); diff --git a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.qhelp b/java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp similarity index 91% rename from java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.qhelp rename to java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp index 2e8d08fd68b9..07c9e3122c7b 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp @@ -22,10 +22,10 @@ no validation is performed thus any certificate is trusted. In the 'GOOD' case,
  • -CWE-273 +CWE-297
  • -How to fix apps containing an unsafe implementation of TrustManager +How to resolve Insecure HostnameVerifier
  • Testing Endpoint Identify Verification (MSTG-NETWORK-3) diff --git a/java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.ql b/java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.ql new file mode 100644 index 000000000000..497e87b37ac3 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.ql @@ -0,0 +1,246 @@ +/** + * @name Unsafe certificate trust and improper hostname verification + * @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. + * @kind problem + * @id java/unsafe-cert-trust + * @tags security + * external/cwe-273 + */ + +import java +import semmle.code.java.security.Encryption + +/** + * X509TrustManager class that blindly trusts all certificates in server SSL authentication + */ +class X509TrustAllManager extends RefType { + X509TrustAllManager() { + this.getASupertype*() instanceof X509TrustManager and + exists(Method m1 | + m1.getDeclaringType() = this and + m1.hasName("checkServerTrusted") and + m1.getBody().getNumStmt() = 0 + ) and + exists(Method m2, ReturnStmt rt2 | + m2.getDeclaringType() = this and + m2.hasName("getAcceptedIssuers") and + rt2.getEnclosingCallable() = m2 and + rt2.getResult() instanceof NullLiteral + ) + } +} + +/** + * The init method of SSLContext with the trust all manager, which is sslContext.init(..., serverTMs, ...) + */ +class X509TrustAllManagerInit extends MethodAccess { + X509TrustAllManagerInit() { + this.getMethod().hasName("init") and + this.getMethod().getDeclaringType() instanceof SSLContext and //init method of SSLContext + ( + exists(ArrayInit ai | + this.getArgument(1).(ArrayCreationExpr).getInit() = ai and + ai.getInit(0).(VarAccess).getVariable().getInitializer().getType().(Class).getASupertype*() + instanceof X509TrustAllManager //Scenario of context.init(null, new TrustManager[] { TRUST_ALL_CERTIFICATES }, null); + ) + or + exists(Variable v, ArrayInit ai | + this.getArgument(1).(VarAccess).getVariable() = v and + ai.getParent() = v.getAnAssignedValue() and + ai.getInit(0).getType().(Class).getASupertype*() instanceof X509TrustAllManager //Scenario of context.init(null, serverTMs, null); + ) + ) + } +} + +/** + * HostnameVerifier class that allows a certificate whose CN (Common Name) does not match the host name in the URL + */ +class TrustAllHostnameVerifier extends RefType { + TrustAllHostnameVerifier() { + this.getASupertype*() instanceof HostnameVerifier and + exists(Method m, ReturnStmt rt | + m.getDeclaringType() = this and + m.hasName("verify") and + rt.getEnclosingCallable() = m and + rt.getResult().(BooleanLiteral).getBooleanValue() = true + ) + } +} + +/** + * The setDefaultHostnameVerifier method of HttpsURLConnection with the trust all configuration + */ +class TrustAllHostnameVerify extends MethodAccess { + TrustAllHostnameVerify() { + this.getMethod().hasName("setDefaultHostnameVerifier") and + this.getMethod().getDeclaringType() instanceof HttpsURLConnection and //httpsURLConnection.setDefaultHostnameVerifier method + ( + exists(NestedClass nc | + nc.getASupertype*() instanceof TrustAllHostnameVerifier and + this.getArgument(0).getType() = nc //Scenario of HttpsURLConnection.setDefaultHostnameVerifier(new HostnameVerifier() {...}); + ) + or + exists(Variable v | + this.getArgument(0).(VarAccess).getVariable() = v and + v.getInitializer().getType() instanceof TrustAllHostnameVerifier //Scenario of HttpsURLConnection.setDefaultHostnameVerifier(verifier); + ) + ) + } +} + +class SSLEngine extends RefType { + SSLEngine() { this.hasQualifiedName("javax.net.ssl", "SSLEngine") } +} + +class Socket extends RefType { + Socket() { this.hasQualifiedName("java.net", "Socket") } +} + +class SocketFactory extends RefType { + SocketFactory() { this.hasQualifiedName("javax.net", "SocketFactory") } +} + +class SSLSocket extends RefType { + SSLSocket() { this.hasQualifiedName("javax.net.ssl", "SSLSocket") } +} + +/** + * has setEndpointIdentificationAlgorithm set correctly + */ +predicate setEndpointIdentificationAlgorithm(MethodAccess createSSL) { + exists( + Variable sslo, MethodAccess ma, Variable sslparams //setSSLParameters with valid setEndpointIdentificationAlgorithm set + | + createSSL = sslo.getAnAssignedValue() and + ma.getQualifier() = sslo.getAnAccess() and + ma.getMethod().hasName("setSSLParameters") and + ma.getArgument(0) = sslparams.getAnAccess() and + exists(MethodAccess setepa | + setepa.getQualifier() = sslparams.getAnAccess() and + setepa.getMethod().hasName("setEndpointIdentificationAlgorithm") and + not setepa.getArgument(0) instanceof NullLiteral + ) + ) +} + +/** + * has setEndpointIdentificationAlgorithm set correctly + */ +predicate hasEndpointIdentificationAlgorithm(Variable ssl) { + exists( + MethodAccess ma, Variable sslparams //setSSLParameters with valid setEndpointIdentificationAlgorithm set + | + ma.getQualifier() = ssl.getAnAccess() and + ma.getMethod().hasName("setSSLParameters") and + ma.getArgument(0) = sslparams.getAnAccess() and + exists(MethodAccess setepa | + setepa.getQualifier() = sslparams.getAnAccess() and + setepa.getMethod().hasName("setEndpointIdentificationAlgorithm") and + not setepa.getArgument(0) instanceof NullLiteral + ) + ) +} + +/** + * Cast of Socket to SSLSocket + */ +predicate sslCast(MethodAccess createSSL) { + exists(Variable ssl, CastExpr ce | + ce.getExpr() = createSSL and + ce.getControlFlowNode().getASuccessor().(VariableAssign).getDestVar() = ssl and + ssl.getType() instanceof SSLSocket //With a type cast `SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443)` + ) +} + +/** + * SSL object is created in a separate method call or in the same method + */ +predicate hasFlowPath(MethodAccess createSSL, Variable ssl) { + ( + createSSL = ssl.getAnAssignedValue() + or + exists(CastExpr ce | + ce.getExpr() = createSSL and + ce.getControlFlowNode().getASuccessor().(VariableAssign).getDestVar() = ssl //With a type cast like SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443); + ) + ) + or + exists(MethodAccess tranm | + createSSL.getEnclosingCallable() = tranm.getMethod() and + tranm.getControlFlowNode().getASuccessor().(VariableAssign).getDestVar() = ssl and + not setEndpointIdentificationAlgorithm(createSSL) //Check the scenario of invocation before used in the current method + ) +} + +/** + * Not have the SSLParameter set + */ +predicate hasNoEndpointIdentificationSet(MethodAccess createSSL, Variable ssl) { + //No setSSLParameters set + hasFlowPath(createSSL, ssl) and + not exists(MethodAccess ma | + ma.getQualifier() = ssl.getAnAccess() and + ma.getMethod().hasName("setSSLParameters") + ) + or + //No endpointIdentificationAlgorithm set with setSSLParameters + hasFlowPath(createSSL, ssl) and + not setEndpointIdentificationAlgorithm(createSSL) +} + +/** + * The setEndpointIdentificationAlgorithm method of SSLParameters with the ssl engine or socket + */ +class SSLEndpointIdentificationNotSet extends MethodAccess { + SSLEndpointIdentificationNotSet() { + ( + this.getMethod().hasName("createSSLEngine") and + this.getMethod().getDeclaringType() instanceof SSLContext //createEngine method of SSLContext + or + this.getMethod().hasName("createSocket") and + this.getMethod().getDeclaringType() instanceof SocketFactory and + this.getMethod().getReturnType() instanceof Socket and + sslCast(this) //createSocket method of SocketFactory + ) and + exists(Variable ssl | + hasNoEndpointIdentificationSet(this, ssl) and //Not set in itself + not exists(VariableAssign ar, Variable newSsl | + ar.getSource() = this.getCaller().getAReference() and + ar.getDestVar() = newSsl and + hasEndpointIdentificationAlgorithm(newSsl) //Not set in its caller either + ) + ) and + not exists(MethodAccess ma | ma.getMethod() instanceof HostnameVerifierVerify) //Reduce false positives since this method access set default hostname verifier + } +} + +class RabbitMQConnectionFactory extends RefType { + RabbitMQConnectionFactory() { this.hasQualifiedName("com.rabbitmq.client", "ConnectionFactory") } +} + +/** + * The com.rabbitmq.client.ConnectionFactory useSslProtocol method access without enableHostnameVerification + */ +class RabbitMQEnableHostnameVerificationNotSet extends MethodAccess { + RabbitMQEnableHostnameVerificationNotSet() { + this.getMethod().hasName("useSslProtocol") and + this.getMethod().getDeclaringType() instanceof RabbitMQConnectionFactory and + exists(Variable v | + v.getType() instanceof RabbitMQConnectionFactory and + this.getQualifier() = v.getAnAccess() and + not exists(MethodAccess ma | + ma.getMethod().hasName("enableHostnameVerification") and + ma.getQualifier() = v.getAnAccess() + ) + ) + } +} + +from MethodAccess aa +where + aa instanceof TrustAllHostnameVerify or + aa instanceof X509TrustAllManagerInit or + aa instanceof SSLEndpointIdentificationNotSet or + aa instanceof RabbitMQEnableHostnameVerificationNotSet +select aa, "Unsafe configuration of trusted certificates" diff --git a/java/ql/test/experimental/query-tests/security/CWE-295/UnsafeCertificateTrust.expected b/java/ql/test/experimental/query-tests/security/CWE-295/UnsafeCertificateTrust.expected new file mode 100644 index 000000000000..c0ea40f9bdb0 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-295/UnsafeCertificateTrust.expected @@ -0,0 +1,7 @@ +| UnsafeCertTrustTest.java:27:4:27:74 | init(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:42:4:42:38 | init(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:55:3:60:4 | setDefaultHostnameVerifier(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:73:3:73:57 | setDefaultHostnameVerifier(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:124:25:124:52 | createSSLEngine(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:135:25:135:52 | createSSLEngine(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:144:34:144:83 | createSocket(...) | Unsafe configuration of trusted certificates | diff --git a/java/ql/test/experimental/query-tests/security/CWE-295/UnsafeCertificateTrust.java b/java/ql/test/experimental/query-tests/security/CWE-295/UnsafeCertificateTrust.java new file mode 100644 index 000000000000..ff62035fd333 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-295/UnsafeCertificateTrust.java @@ -0,0 +1,162 @@ +import javax.net.ssl.HostnameVerifier; +import javax.net.ssl.HttpsURLConnection; +import javax.net.ssl.SSLSession; +import javax.net.ssl.SSLSocket; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLEngine; +import javax.net.ssl.SSLParameters; +import javax.net.ssl.SSLSocketFactory; +import javax.net.ssl.TrustManager; +import javax.net.ssl.X509TrustManager; + +import java.net.Socket; +import javax.net.SocketFactory; +import java.security.cert.CertificateException; +import java.security.cert.X509Certificate; + +//import com.rabbitmq.client.ConnectionFactory; + +public class UnsafeCertTrustTest { + + /** + * Test the implementation of trusting all server certs as a variable + */ + public SSLSocketFactory testTrustAllCertManager() { + try { + final SSLContext context = SSLContext.getInstance("TLS"); + context.init(null, new TrustManager[] { TRUST_ALL_CERTIFICATES }, null); + final SSLSocketFactory socketFactory = context.getSocketFactory(); + return socketFactory; + } catch (final Exception x) { + throw new RuntimeException(x); + } + } + + /** + * Test the implementation of trusting all server certs as an anonymous class + */ + public SSLSocketFactory testTrustAllCertManagerOfVariable() { + try { + SSLContext context = SSLContext.getInstance("TLS"); + TrustManager[] serverTMs = new TrustManager[] { new X509TrustAllManager() }; + context.init(null, serverTMs, null); + + final SSLSocketFactory socketFactory = context.getSocketFactory(); + return socketFactory; + } catch (final Exception x) { + throw new RuntimeException(x); + } + } + + /** + * Test the implementation of trusting all hostnames as an anonymous class + */ + public void testTrustAllHostnameOfAnonymousClass() { + HttpsURLConnection.setDefaultHostnameVerifier(new HostnameVerifier() { + @Override + public boolean verify(String hostname, SSLSession session) { + return true; // Noncompliant + } + }); + } + + /** + * Test the implementation of trusting all hostnames as a variable + */ + public void testTrustAllHostnameOfVariable() { + HostnameVerifier verifier = new HostnameVerifier() { + @Override + public boolean verify(String hostname, SSLSession session) { + return true; // Noncompliant + } + }; + HttpsURLConnection.setDefaultHostnameVerifier(verifier); + } + + private static final X509TrustManager TRUST_ALL_CERTIFICATES = new X509TrustManager() { + @Override + public void checkClientTrusted(final X509Certificate[] chain, final String authType) + throws CertificateException { + } + + @Override + public void checkServerTrusted(final X509Certificate[] chain, final String authType) + throws CertificateException { + // Noncompliant + } + + @Override + public X509Certificate[] getAcceptedIssuers() { + return null; // Noncompliant + } + }; + + private class X509TrustAllManager implements X509TrustManager { + @Override + public void checkClientTrusted(final X509Certificate[] chain, final String authType) + throws CertificateException { + } + + @Override + public void checkServerTrusted(final X509Certificate[] chain, final String authType) + throws CertificateException { + // Noncompliant + } + + @Override + public X509Certificate[] getAcceptedIssuers() { + return null; // Noncompliant + } + }; + + public static final HostnameVerifier ALLOW_ALL_HOSTNAME_VERIFIER = new HostnameVerifier() { + @Override + public boolean verify(String hostname, SSLSession session) { + return true; // Noncompliant + } + }; + + /** + * Test the endpoint identification of SSL engine is set to null + */ + public void testSSLEngineEndpointIdSetNull() { + SSLContext sslContext = SSLContext.getInstance("TLS"); + SSLEngine sslEngine = sslContext.createSSLEngine(); + SSLParameters sslParameters = sslEngine.getSSLParameters(); + sslParameters.setEndpointIdentificationAlgorithm(null); + sslEngine.setSSLParameters(sslParameters); + } + + /** + * Test the endpoint identification of SSL engine is not set + */ + public void testSSLEngineEndpointIdNotSet() { + SSLContext sslContext = SSLContext.getInstance("TLS"); + SSLEngine sslEngine = sslContext.createSSLEngine(); + } + + /** + * Test the endpoint identification of SSL socket is not set + */ + public void testSSLSocketEndpointIdNotSet() { + SSLContext sslContext = SSLContext.getInstance("TLS"); + final SSLSocketFactory socketFactory = sslContext.getSocketFactory(); + SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443); + } + + /** + * Test the endpoint identification of regular socket is not set + */ + public void testSocketEndpointIdNotSet() { + SocketFactory socketFactory = SocketFactory.getDefault(); + Socket socket = socketFactory.createSocket("www.example.com", 80); + } + + // /** + // * Test the enableHostnameVerification of RabbitMQConnectionFactory is not set + // */ + // public void testEnableHostnameVerificationOfRabbitMQFactoryNotSet() { + // ConnectionFactory connectionFactory = new ConnectionFactory(); + // connectionFactory.useSslProtocol(); + // } +} \ No newline at end of file diff --git a/java/ql/test/experimental/query-tests/security/CWE-295/UnsafeCertificateTrust.qlref b/java/ql/test/experimental/query-tests/security/CWE-295/UnsafeCertificateTrust.qlref new file mode 100644 index 000000000000..bad00eae8b33 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-295/UnsafeCertificateTrust.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-295/UnsafeCertificateTrust.ql diff --git a/java/ql/test/experimental/query-tests/security/CWE-297/UnsafeHostnameVerification.expected b/java/ql/test/experimental/query-tests/security/CWE-297/UnsafeHostnameVerification.expected new file mode 100644 index 000000000000..c0ea40f9bdb0 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-297/UnsafeHostnameVerification.expected @@ -0,0 +1,7 @@ +| UnsafeCertTrustTest.java:27:4:27:74 | init(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:42:4:42:38 | init(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:55:3:60:4 | setDefaultHostnameVerifier(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:73:3:73:57 | setDefaultHostnameVerifier(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:124:25:124:52 | createSSLEngine(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:135:25:135:52 | createSSLEngine(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:144:34:144:83 | createSocket(...) | Unsafe configuration of trusted certificates | diff --git a/java/ql/test/experimental/query-tests/security/CWE-297/UnsafeHostnameVerification.java b/java/ql/test/experimental/query-tests/security/CWE-297/UnsafeHostnameVerification.java new file mode 100644 index 000000000000..ff62035fd333 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-297/UnsafeHostnameVerification.java @@ -0,0 +1,162 @@ +import javax.net.ssl.HostnameVerifier; +import javax.net.ssl.HttpsURLConnection; +import javax.net.ssl.SSLSession; +import javax.net.ssl.SSLSocket; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLEngine; +import javax.net.ssl.SSLParameters; +import javax.net.ssl.SSLSocketFactory; +import javax.net.ssl.TrustManager; +import javax.net.ssl.X509TrustManager; + +import java.net.Socket; +import javax.net.SocketFactory; +import java.security.cert.CertificateException; +import java.security.cert.X509Certificate; + +//import com.rabbitmq.client.ConnectionFactory; + +public class UnsafeCertTrustTest { + + /** + * Test the implementation of trusting all server certs as a variable + */ + public SSLSocketFactory testTrustAllCertManager() { + try { + final SSLContext context = SSLContext.getInstance("TLS"); + context.init(null, new TrustManager[] { TRUST_ALL_CERTIFICATES }, null); + final SSLSocketFactory socketFactory = context.getSocketFactory(); + return socketFactory; + } catch (final Exception x) { + throw new RuntimeException(x); + } + } + + /** + * Test the implementation of trusting all server certs as an anonymous class + */ + public SSLSocketFactory testTrustAllCertManagerOfVariable() { + try { + SSLContext context = SSLContext.getInstance("TLS"); + TrustManager[] serverTMs = new TrustManager[] { new X509TrustAllManager() }; + context.init(null, serverTMs, null); + + final SSLSocketFactory socketFactory = context.getSocketFactory(); + return socketFactory; + } catch (final Exception x) { + throw new RuntimeException(x); + } + } + + /** + * Test the implementation of trusting all hostnames as an anonymous class + */ + public void testTrustAllHostnameOfAnonymousClass() { + HttpsURLConnection.setDefaultHostnameVerifier(new HostnameVerifier() { + @Override + public boolean verify(String hostname, SSLSession session) { + return true; // Noncompliant + } + }); + } + + /** + * Test the implementation of trusting all hostnames as a variable + */ + public void testTrustAllHostnameOfVariable() { + HostnameVerifier verifier = new HostnameVerifier() { + @Override + public boolean verify(String hostname, SSLSession session) { + return true; // Noncompliant + } + }; + HttpsURLConnection.setDefaultHostnameVerifier(verifier); + } + + private static final X509TrustManager TRUST_ALL_CERTIFICATES = new X509TrustManager() { + @Override + public void checkClientTrusted(final X509Certificate[] chain, final String authType) + throws CertificateException { + } + + @Override + public void checkServerTrusted(final X509Certificate[] chain, final String authType) + throws CertificateException { + // Noncompliant + } + + @Override + public X509Certificate[] getAcceptedIssuers() { + return null; // Noncompliant + } + }; + + private class X509TrustAllManager implements X509TrustManager { + @Override + public void checkClientTrusted(final X509Certificate[] chain, final String authType) + throws CertificateException { + } + + @Override + public void checkServerTrusted(final X509Certificate[] chain, final String authType) + throws CertificateException { + // Noncompliant + } + + @Override + public X509Certificate[] getAcceptedIssuers() { + return null; // Noncompliant + } + }; + + public static final HostnameVerifier ALLOW_ALL_HOSTNAME_VERIFIER = new HostnameVerifier() { + @Override + public boolean verify(String hostname, SSLSession session) { + return true; // Noncompliant + } + }; + + /** + * Test the endpoint identification of SSL engine is set to null + */ + public void testSSLEngineEndpointIdSetNull() { + SSLContext sslContext = SSLContext.getInstance("TLS"); + SSLEngine sslEngine = sslContext.createSSLEngine(); + SSLParameters sslParameters = sslEngine.getSSLParameters(); + sslParameters.setEndpointIdentificationAlgorithm(null); + sslEngine.setSSLParameters(sslParameters); + } + + /** + * Test the endpoint identification of SSL engine is not set + */ + public void testSSLEngineEndpointIdNotSet() { + SSLContext sslContext = SSLContext.getInstance("TLS"); + SSLEngine sslEngine = sslContext.createSSLEngine(); + } + + /** + * Test the endpoint identification of SSL socket is not set + */ + public void testSSLSocketEndpointIdNotSet() { + SSLContext sslContext = SSLContext.getInstance("TLS"); + final SSLSocketFactory socketFactory = sslContext.getSocketFactory(); + SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443); + } + + /** + * Test the endpoint identification of regular socket is not set + */ + public void testSocketEndpointIdNotSet() { + SocketFactory socketFactory = SocketFactory.getDefault(); + Socket socket = socketFactory.createSocket("www.example.com", 80); + } + + // /** + // * Test the enableHostnameVerification of RabbitMQConnectionFactory is not set + // */ + // public void testEnableHostnameVerificationOfRabbitMQFactoryNotSet() { + // ConnectionFactory connectionFactory = new ConnectionFactory(); + // connectionFactory.useSslProtocol(); + // } +} \ No newline at end of file diff --git a/java/ql/test/experimental/query-tests/security/CWE-297/UnsafeHostnameVerification.qlref b/java/ql/test/experimental/query-tests/security/CWE-297/UnsafeHostnameVerification.qlref new file mode 100644 index 000000000000..c0c80eedc6d3 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-297/UnsafeHostnameVerification.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.ql From 104accfdbee102ac98edb73c0f98781f2a93d359 Mon Sep 17 00:00:00 2001 From: intrigus Date: Wed, 21 Oct 2020 22:56:05 +0200 Subject: [PATCH 2/5] Accept test changes and more separation. --- .../CWE/CWE-295/UnsafeCertificateTrust.ql | 202 +----------------- .../CWE/CWE-297/UnsafeHostnameVerification.ql | 44 ---- .../security/CWE-273/UnsafeCertTrust.expected | 7 - .../security/CWE-273/UnsafeCertTrust.qlref | 1 - .../security/CWE-273/UnsafeCertTrustTest.java | 162 -------------- .../CWE-295/UnsafeCertificateTrust.expected | 9 +- .../CWE-295/UnsafeCertificateTrust.java | 77 +------ .../UnsafeHostnameVerification.expected | 12 +- .../CWE-297/UnsafeHostnameVerification.java | 68 +----- 9 files changed, 16 insertions(+), 566 deletions(-) delete mode 100644 java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.expected delete mode 100644 java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.qlref delete mode 100644 java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrustTest.java diff --git a/java/ql/src/experimental/Security/CWE/CWE-295/UnsafeCertificateTrust.ql b/java/ql/src/experimental/Security/CWE/CWE-295/UnsafeCertificateTrust.ql index 497e87b37ac3..6ac985b3dd10 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-295/UnsafeCertificateTrust.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-295/UnsafeCertificateTrust.ql @@ -1,17 +1,17 @@ /** - * @name Unsafe certificate trust and improper hostname verification - * @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. + * @name Unsafe certificate trust + * @description Unsafe implementation of a `X509TrustManager`, thereby making the app vulnerable to man-in-the-middle attacks. * @kind problem * @id java/unsafe-cert-trust * @tags security - * external/cwe-273 + * external/cwe-295 */ import java import semmle.code.java.security.Encryption /** - * X509TrustManager class that blindly trusts all certificates in server SSL authentication + * A `X509TrustManager` class that blindly trusts all certificates in server SSL authentication. */ class X509TrustAllManager extends RefType { X509TrustAllManager() { @@ -53,194 +53,6 @@ class X509TrustAllManagerInit extends MethodAccess { } } -/** - * HostnameVerifier class that allows a certificate whose CN (Common Name) does not match the host name in the URL - */ -class TrustAllHostnameVerifier extends RefType { - TrustAllHostnameVerifier() { - this.getASupertype*() instanceof HostnameVerifier and - exists(Method m, ReturnStmt rt | - m.getDeclaringType() = this and - m.hasName("verify") and - rt.getEnclosingCallable() = m and - rt.getResult().(BooleanLiteral).getBooleanValue() = true - ) - } -} - -/** - * The setDefaultHostnameVerifier method of HttpsURLConnection with the trust all configuration - */ -class TrustAllHostnameVerify extends MethodAccess { - TrustAllHostnameVerify() { - this.getMethod().hasName("setDefaultHostnameVerifier") and - this.getMethod().getDeclaringType() instanceof HttpsURLConnection and //httpsURLConnection.setDefaultHostnameVerifier method - ( - exists(NestedClass nc | - nc.getASupertype*() instanceof TrustAllHostnameVerifier and - this.getArgument(0).getType() = nc //Scenario of HttpsURLConnection.setDefaultHostnameVerifier(new HostnameVerifier() {...}); - ) - or - exists(Variable v | - this.getArgument(0).(VarAccess).getVariable() = v and - v.getInitializer().getType() instanceof TrustAllHostnameVerifier //Scenario of HttpsURLConnection.setDefaultHostnameVerifier(verifier); - ) - ) - } -} - -class SSLEngine extends RefType { - SSLEngine() { this.hasQualifiedName("javax.net.ssl", "SSLEngine") } -} - -class Socket extends RefType { - Socket() { this.hasQualifiedName("java.net", "Socket") } -} - -class SocketFactory extends RefType { - SocketFactory() { this.hasQualifiedName("javax.net", "SocketFactory") } -} - -class SSLSocket extends RefType { - SSLSocket() { this.hasQualifiedName("javax.net.ssl", "SSLSocket") } -} - -/** - * has setEndpointIdentificationAlgorithm set correctly - */ -predicate setEndpointIdentificationAlgorithm(MethodAccess createSSL) { - exists( - Variable sslo, MethodAccess ma, Variable sslparams //setSSLParameters with valid setEndpointIdentificationAlgorithm set - | - createSSL = sslo.getAnAssignedValue() and - ma.getQualifier() = sslo.getAnAccess() and - ma.getMethod().hasName("setSSLParameters") and - ma.getArgument(0) = sslparams.getAnAccess() and - exists(MethodAccess setepa | - setepa.getQualifier() = sslparams.getAnAccess() and - setepa.getMethod().hasName("setEndpointIdentificationAlgorithm") and - not setepa.getArgument(0) instanceof NullLiteral - ) - ) -} - -/** - * has setEndpointIdentificationAlgorithm set correctly - */ -predicate hasEndpointIdentificationAlgorithm(Variable ssl) { - exists( - MethodAccess ma, Variable sslparams //setSSLParameters with valid setEndpointIdentificationAlgorithm set - | - ma.getQualifier() = ssl.getAnAccess() and - ma.getMethod().hasName("setSSLParameters") and - ma.getArgument(0) = sslparams.getAnAccess() and - exists(MethodAccess setepa | - setepa.getQualifier() = sslparams.getAnAccess() and - setepa.getMethod().hasName("setEndpointIdentificationAlgorithm") and - not setepa.getArgument(0) instanceof NullLiteral - ) - ) -} - -/** - * Cast of Socket to SSLSocket - */ -predicate sslCast(MethodAccess createSSL) { - exists(Variable ssl, CastExpr ce | - ce.getExpr() = createSSL and - ce.getControlFlowNode().getASuccessor().(VariableAssign).getDestVar() = ssl and - ssl.getType() instanceof SSLSocket //With a type cast `SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443)` - ) -} - -/** - * SSL object is created in a separate method call or in the same method - */ -predicate hasFlowPath(MethodAccess createSSL, Variable ssl) { - ( - createSSL = ssl.getAnAssignedValue() - or - exists(CastExpr ce | - ce.getExpr() = createSSL and - ce.getControlFlowNode().getASuccessor().(VariableAssign).getDestVar() = ssl //With a type cast like SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443); - ) - ) - or - exists(MethodAccess tranm | - createSSL.getEnclosingCallable() = tranm.getMethod() and - tranm.getControlFlowNode().getASuccessor().(VariableAssign).getDestVar() = ssl and - not setEndpointIdentificationAlgorithm(createSSL) //Check the scenario of invocation before used in the current method - ) -} - -/** - * Not have the SSLParameter set - */ -predicate hasNoEndpointIdentificationSet(MethodAccess createSSL, Variable ssl) { - //No setSSLParameters set - hasFlowPath(createSSL, ssl) and - not exists(MethodAccess ma | - ma.getQualifier() = ssl.getAnAccess() and - ma.getMethod().hasName("setSSLParameters") - ) - or - //No endpointIdentificationAlgorithm set with setSSLParameters - hasFlowPath(createSSL, ssl) and - not setEndpointIdentificationAlgorithm(createSSL) -} - -/** - * The setEndpointIdentificationAlgorithm method of SSLParameters with the ssl engine or socket - */ -class SSLEndpointIdentificationNotSet extends MethodAccess { - SSLEndpointIdentificationNotSet() { - ( - this.getMethod().hasName("createSSLEngine") and - this.getMethod().getDeclaringType() instanceof SSLContext //createEngine method of SSLContext - or - this.getMethod().hasName("createSocket") and - this.getMethod().getDeclaringType() instanceof SocketFactory and - this.getMethod().getReturnType() instanceof Socket and - sslCast(this) //createSocket method of SocketFactory - ) and - exists(Variable ssl | - hasNoEndpointIdentificationSet(this, ssl) and //Not set in itself - not exists(VariableAssign ar, Variable newSsl | - ar.getSource() = this.getCaller().getAReference() and - ar.getDestVar() = newSsl and - hasEndpointIdentificationAlgorithm(newSsl) //Not set in its caller either - ) - ) and - not exists(MethodAccess ma | ma.getMethod() instanceof HostnameVerifierVerify) //Reduce false positives since this method access set default hostname verifier - } -} - -class RabbitMQConnectionFactory extends RefType { - RabbitMQConnectionFactory() { this.hasQualifiedName("com.rabbitmq.client", "ConnectionFactory") } -} - -/** - * The com.rabbitmq.client.ConnectionFactory useSslProtocol method access without enableHostnameVerification - */ -class RabbitMQEnableHostnameVerificationNotSet extends MethodAccess { - RabbitMQEnableHostnameVerificationNotSet() { - this.getMethod().hasName("useSslProtocol") and - this.getMethod().getDeclaringType() instanceof RabbitMQConnectionFactory and - exists(Variable v | - v.getType() instanceof RabbitMQConnectionFactory and - this.getQualifier() = v.getAnAccess() and - not exists(MethodAccess ma | - ma.getMethod().hasName("enableHostnameVerification") and - ma.getQualifier() = v.getAnAccess() - ) - ) - } -} - -from MethodAccess aa -where - aa instanceof TrustAllHostnameVerify or - aa instanceof X509TrustAllManagerInit or - aa instanceof SSLEndpointIdentificationNotSet or - aa instanceof RabbitMQEnableHostnameVerificationNotSet -select aa, "Unsafe configuration of trusted certificates" +from MethodAccess ma +where ma instanceof X509TrustAllManagerInit +select ma, "Unsafe configuration of trusted certificates" diff --git a/java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.ql b/java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.ql index 497e87b37ac3..0a4f02445de2 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.ql @@ -10,49 +10,6 @@ import java import semmle.code.java.security.Encryption -/** - * X509TrustManager class that blindly trusts all certificates in server SSL authentication - */ -class X509TrustAllManager extends RefType { - X509TrustAllManager() { - this.getASupertype*() instanceof X509TrustManager and - exists(Method m1 | - m1.getDeclaringType() = this and - m1.hasName("checkServerTrusted") and - m1.getBody().getNumStmt() = 0 - ) and - exists(Method m2, ReturnStmt rt2 | - m2.getDeclaringType() = this and - m2.hasName("getAcceptedIssuers") and - rt2.getEnclosingCallable() = m2 and - rt2.getResult() instanceof NullLiteral - ) - } -} - -/** - * The init method of SSLContext with the trust all manager, which is sslContext.init(..., serverTMs, ...) - */ -class X509TrustAllManagerInit extends MethodAccess { - X509TrustAllManagerInit() { - this.getMethod().hasName("init") and - this.getMethod().getDeclaringType() instanceof SSLContext and //init method of SSLContext - ( - exists(ArrayInit ai | - this.getArgument(1).(ArrayCreationExpr).getInit() = ai and - ai.getInit(0).(VarAccess).getVariable().getInitializer().getType().(Class).getASupertype*() - instanceof X509TrustAllManager //Scenario of context.init(null, new TrustManager[] { TRUST_ALL_CERTIFICATES }, null); - ) - or - exists(Variable v, ArrayInit ai | - this.getArgument(1).(VarAccess).getVariable() = v and - ai.getParent() = v.getAnAssignedValue() and - ai.getInit(0).getType().(Class).getASupertype*() instanceof X509TrustAllManager //Scenario of context.init(null, serverTMs, null); - ) - ) - } -} - /** * HostnameVerifier class that allows a certificate whose CN (Common Name) does not match the host name in the URL */ @@ -240,7 +197,6 @@ class RabbitMQEnableHostnameVerificationNotSet extends MethodAccess { from MethodAccess aa where aa instanceof TrustAllHostnameVerify or - aa instanceof X509TrustAllManagerInit or aa instanceof SSLEndpointIdentificationNotSet or aa instanceof RabbitMQEnableHostnameVerificationNotSet select aa, "Unsafe configuration of trusted certificates" diff --git a/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.expected b/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.expected deleted file mode 100644 index c0ea40f9bdb0..000000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.expected +++ /dev/null @@ -1,7 +0,0 @@ -| UnsafeCertTrustTest.java:27:4:27:74 | init(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:42:4:42:38 | init(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:55:3:60:4 | setDefaultHostnameVerifier(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:73:3:73:57 | setDefaultHostnameVerifier(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:124:25:124:52 | createSSLEngine(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:135:25:135:52 | createSSLEngine(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:144:34:144:83 | createSocket(...) | Unsafe configuration of trusted certificates | diff --git a/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.qlref b/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.qlref deleted file mode 100644 index f054d6037876..000000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.qlref +++ /dev/null @@ -1 +0,0 @@ -experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql diff --git a/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrustTest.java b/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrustTest.java deleted file mode 100644 index ff62035fd333..000000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrustTest.java +++ /dev/null @@ -1,162 +0,0 @@ -import javax.net.ssl.HostnameVerifier; -import javax.net.ssl.HttpsURLConnection; -import javax.net.ssl.SSLSession; -import javax.net.ssl.SSLSocket; -import javax.net.ssl.SSLContext; -import javax.net.ssl.SSLEngine; -import javax.net.ssl.SSLParameters; -import javax.net.ssl.SSLSocketFactory; -import javax.net.ssl.TrustManager; -import javax.net.ssl.X509TrustManager; - -import java.net.Socket; -import javax.net.SocketFactory; -import java.security.cert.CertificateException; -import java.security.cert.X509Certificate; - -//import com.rabbitmq.client.ConnectionFactory; - -public class UnsafeCertTrustTest { - - /** - * Test the implementation of trusting all server certs as a variable - */ - public SSLSocketFactory testTrustAllCertManager() { - try { - final SSLContext context = SSLContext.getInstance("TLS"); - context.init(null, new TrustManager[] { TRUST_ALL_CERTIFICATES }, null); - final SSLSocketFactory socketFactory = context.getSocketFactory(); - return socketFactory; - } catch (final Exception x) { - throw new RuntimeException(x); - } - } - - /** - * Test the implementation of trusting all server certs as an anonymous class - */ - public SSLSocketFactory testTrustAllCertManagerOfVariable() { - try { - SSLContext context = SSLContext.getInstance("TLS"); - TrustManager[] serverTMs = new TrustManager[] { new X509TrustAllManager() }; - context.init(null, serverTMs, null); - - final SSLSocketFactory socketFactory = context.getSocketFactory(); - return socketFactory; - } catch (final Exception x) { - throw new RuntimeException(x); - } - } - - /** - * Test the implementation of trusting all hostnames as an anonymous class - */ - public void testTrustAllHostnameOfAnonymousClass() { - HttpsURLConnection.setDefaultHostnameVerifier(new HostnameVerifier() { - @Override - public boolean verify(String hostname, SSLSession session) { - return true; // Noncompliant - } - }); - } - - /** - * Test the implementation of trusting all hostnames as a variable - */ - public void testTrustAllHostnameOfVariable() { - HostnameVerifier verifier = new HostnameVerifier() { - @Override - public boolean verify(String hostname, SSLSession session) { - return true; // Noncompliant - } - }; - HttpsURLConnection.setDefaultHostnameVerifier(verifier); - } - - private static final X509TrustManager TRUST_ALL_CERTIFICATES = new X509TrustManager() { - @Override - public void checkClientTrusted(final X509Certificate[] chain, final String authType) - throws CertificateException { - } - - @Override - public void checkServerTrusted(final X509Certificate[] chain, final String authType) - throws CertificateException { - // Noncompliant - } - - @Override - public X509Certificate[] getAcceptedIssuers() { - return null; // Noncompliant - } - }; - - private class X509TrustAllManager implements X509TrustManager { - @Override - public void checkClientTrusted(final X509Certificate[] chain, final String authType) - throws CertificateException { - } - - @Override - public void checkServerTrusted(final X509Certificate[] chain, final String authType) - throws CertificateException { - // Noncompliant - } - - @Override - public X509Certificate[] getAcceptedIssuers() { - return null; // Noncompliant - } - }; - - public static final HostnameVerifier ALLOW_ALL_HOSTNAME_VERIFIER = new HostnameVerifier() { - @Override - public boolean verify(String hostname, SSLSession session) { - return true; // Noncompliant - } - }; - - /** - * Test the endpoint identification of SSL engine is set to null - */ - public void testSSLEngineEndpointIdSetNull() { - SSLContext sslContext = SSLContext.getInstance("TLS"); - SSLEngine sslEngine = sslContext.createSSLEngine(); - SSLParameters sslParameters = sslEngine.getSSLParameters(); - sslParameters.setEndpointIdentificationAlgorithm(null); - sslEngine.setSSLParameters(sslParameters); - } - - /** - * Test the endpoint identification of SSL engine is not set - */ - public void testSSLEngineEndpointIdNotSet() { - SSLContext sslContext = SSLContext.getInstance("TLS"); - SSLEngine sslEngine = sslContext.createSSLEngine(); - } - - /** - * Test the endpoint identification of SSL socket is not set - */ - public void testSSLSocketEndpointIdNotSet() { - SSLContext sslContext = SSLContext.getInstance("TLS"); - final SSLSocketFactory socketFactory = sslContext.getSocketFactory(); - SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443); - } - - /** - * Test the endpoint identification of regular socket is not set - */ - public void testSocketEndpointIdNotSet() { - SocketFactory socketFactory = SocketFactory.getDefault(); - Socket socket = socketFactory.createSocket("www.example.com", 80); - } - - // /** - // * Test the enableHostnameVerification of RabbitMQConnectionFactory is not set - // */ - // public void testEnableHostnameVerificationOfRabbitMQFactoryNotSet() { - // ConnectionFactory connectionFactory = new ConnectionFactory(); - // connectionFactory.useSslProtocol(); - // } -} \ No newline at end of file diff --git a/java/ql/test/experimental/query-tests/security/CWE-295/UnsafeCertificateTrust.expected b/java/ql/test/experimental/query-tests/security/CWE-295/UnsafeCertificateTrust.expected index c0ea40f9bdb0..f0ca58390ffb 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-295/UnsafeCertificateTrust.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-295/UnsafeCertificateTrust.expected @@ -1,7 +1,2 @@ -| UnsafeCertTrustTest.java:27:4:27:74 | init(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:42:4:42:38 | init(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:55:3:60:4 | setDefaultHostnameVerifier(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:73:3:73:57 | setDefaultHostnameVerifier(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:124:25:124:52 | createSSLEngine(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:135:25:135:52 | createSSLEngine(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:144:34:144:83 | createSocket(...) | Unsafe configuration of trusted certificates | +| UnsafeCertificateTrust.java:27:4:27:74 | init(...) | Unsafe configuration of trusted certificates | +| UnsafeCertificateTrust.java:42:4:42:38 | init(...) | Unsafe configuration of trusted certificates | diff --git a/java/ql/test/experimental/query-tests/security/CWE-295/UnsafeCertificateTrust.java b/java/ql/test/experimental/query-tests/security/CWE-295/UnsafeCertificateTrust.java index ff62035fd333..e8bcc0f8dd5a 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-295/UnsafeCertificateTrust.java +++ b/java/ql/test/experimental/query-tests/security/CWE-295/UnsafeCertificateTrust.java @@ -16,7 +16,7 @@ //import com.rabbitmq.client.ConnectionFactory; -public class UnsafeCertTrustTest { +public class UnsafeCertificateTrust { /** * Test the implementation of trusting all server certs as a variable @@ -48,31 +48,6 @@ public SSLSocketFactory testTrustAllCertManagerOfVariable() { } } - /** - * Test the implementation of trusting all hostnames as an anonymous class - */ - public void testTrustAllHostnameOfAnonymousClass() { - HttpsURLConnection.setDefaultHostnameVerifier(new HostnameVerifier() { - @Override - public boolean verify(String hostname, SSLSession session) { - return true; // Noncompliant - } - }); - } - - /** - * Test the implementation of trusting all hostnames as a variable - */ - public void testTrustAllHostnameOfVariable() { - HostnameVerifier verifier = new HostnameVerifier() { - @Override - public boolean verify(String hostname, SSLSession session) { - return true; // Noncompliant - } - }; - HttpsURLConnection.setDefaultHostnameVerifier(verifier); - } - private static final X509TrustManager TRUST_ALL_CERTIFICATES = new X509TrustManager() { @Override public void checkClientTrusted(final X509Certificate[] chain, final String authType) @@ -109,54 +84,4 @@ public X509Certificate[] getAcceptedIssuers() { } }; - public static final HostnameVerifier ALLOW_ALL_HOSTNAME_VERIFIER = new HostnameVerifier() { - @Override - public boolean verify(String hostname, SSLSession session) { - return true; // Noncompliant - } - }; - - /** - * Test the endpoint identification of SSL engine is set to null - */ - public void testSSLEngineEndpointIdSetNull() { - SSLContext sslContext = SSLContext.getInstance("TLS"); - SSLEngine sslEngine = sslContext.createSSLEngine(); - SSLParameters sslParameters = sslEngine.getSSLParameters(); - sslParameters.setEndpointIdentificationAlgorithm(null); - sslEngine.setSSLParameters(sslParameters); - } - - /** - * Test the endpoint identification of SSL engine is not set - */ - public void testSSLEngineEndpointIdNotSet() { - SSLContext sslContext = SSLContext.getInstance("TLS"); - SSLEngine sslEngine = sslContext.createSSLEngine(); - } - - /** - * Test the endpoint identification of SSL socket is not set - */ - public void testSSLSocketEndpointIdNotSet() { - SSLContext sslContext = SSLContext.getInstance("TLS"); - final SSLSocketFactory socketFactory = sslContext.getSocketFactory(); - SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443); - } - - /** - * Test the endpoint identification of regular socket is not set - */ - public void testSocketEndpointIdNotSet() { - SocketFactory socketFactory = SocketFactory.getDefault(); - Socket socket = socketFactory.createSocket("www.example.com", 80); - } - - // /** - // * Test the enableHostnameVerification of RabbitMQConnectionFactory is not set - // */ - // public void testEnableHostnameVerificationOfRabbitMQFactoryNotSet() { - // ConnectionFactory connectionFactory = new ConnectionFactory(); - // connectionFactory.useSslProtocol(); - // } } \ No newline at end of file diff --git a/java/ql/test/experimental/query-tests/security/CWE-297/UnsafeHostnameVerification.expected b/java/ql/test/experimental/query-tests/security/CWE-297/UnsafeHostnameVerification.expected index c0ea40f9bdb0..20bb5a58b8b3 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-297/UnsafeHostnameVerification.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-297/UnsafeHostnameVerification.expected @@ -1,7 +1,5 @@ -| UnsafeCertTrustTest.java:27:4:27:74 | init(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:42:4:42:38 | init(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:55:3:60:4 | setDefaultHostnameVerifier(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:73:3:73:57 | setDefaultHostnameVerifier(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:124:25:124:52 | createSSLEngine(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:135:25:135:52 | createSSLEngine(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:144:34:144:83 | createSocket(...) | Unsafe configuration of trusted certificates | +| UnsafeHostnameVerification.java:25:3:30:4 | setDefaultHostnameVerifier(...) | Unsafe configuration of trusted certificates | +| UnsafeHostnameVerification.java:43:3:43:57 | setDefaultHostnameVerifier(...) | Unsafe configuration of trusted certificates | +| UnsafeHostnameVerification.java:58:25:58:52 | createSSLEngine(...) | Unsafe configuration of trusted certificates | +| UnsafeHostnameVerification.java:69:25:69:52 | createSSLEngine(...) | Unsafe configuration of trusted certificates | +| UnsafeHostnameVerification.java:78:34:78:83 | createSocket(...) | Unsafe configuration of trusted certificates | diff --git a/java/ql/test/experimental/query-tests/security/CWE-297/UnsafeHostnameVerification.java b/java/ql/test/experimental/query-tests/security/CWE-297/UnsafeHostnameVerification.java index ff62035fd333..5e3913a8a046 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-297/UnsafeHostnameVerification.java +++ b/java/ql/test/experimental/query-tests/security/CWE-297/UnsafeHostnameVerification.java @@ -16,37 +16,7 @@ //import com.rabbitmq.client.ConnectionFactory; -public class UnsafeCertTrustTest { - - /** - * Test the implementation of trusting all server certs as a variable - */ - public SSLSocketFactory testTrustAllCertManager() { - try { - final SSLContext context = SSLContext.getInstance("TLS"); - context.init(null, new TrustManager[] { TRUST_ALL_CERTIFICATES }, null); - final SSLSocketFactory socketFactory = context.getSocketFactory(); - return socketFactory; - } catch (final Exception x) { - throw new RuntimeException(x); - } - } - - /** - * Test the implementation of trusting all server certs as an anonymous class - */ - public SSLSocketFactory testTrustAllCertManagerOfVariable() { - try { - SSLContext context = SSLContext.getInstance("TLS"); - TrustManager[] serverTMs = new TrustManager[] { new X509TrustAllManager() }; - context.init(null, serverTMs, null); - - final SSLSocketFactory socketFactory = context.getSocketFactory(); - return socketFactory; - } catch (final Exception x) { - throw new RuntimeException(x); - } - } +public class UnsafeHostnameVerification { /** * Test the implementation of trusting all hostnames as an anonymous class @@ -73,42 +43,6 @@ public boolean verify(String hostname, SSLSession session) { HttpsURLConnection.setDefaultHostnameVerifier(verifier); } - private static final X509TrustManager TRUST_ALL_CERTIFICATES = new X509TrustManager() { - @Override - public void checkClientTrusted(final X509Certificate[] chain, final String authType) - throws CertificateException { - } - - @Override - public void checkServerTrusted(final X509Certificate[] chain, final String authType) - throws CertificateException { - // Noncompliant - } - - @Override - public X509Certificate[] getAcceptedIssuers() { - return null; // Noncompliant - } - }; - - private class X509TrustAllManager implements X509TrustManager { - @Override - public void checkClientTrusted(final X509Certificate[] chain, final String authType) - throws CertificateException { - } - - @Override - public void checkServerTrusted(final X509Certificate[] chain, final String authType) - throws CertificateException { - // Noncompliant - } - - @Override - public X509Certificate[] getAcceptedIssuers() { - return null; // Noncompliant - } - }; - public static final HostnameVerifier ALLOW_ALL_HOSTNAME_VERIFIER = new HostnameVerifier() { @Override public boolean verify(String hostname, SSLSession session) { From 4210ba8e89b457edc5edbe645e858a0a49d649cf Mon Sep 17 00:00:00 2001 From: intrigus Date: Wed, 21 Oct 2020 23:33:09 +0200 Subject: [PATCH 3/5] Switch to dataflow for trust-all verifier. --- .../CWE/CWE-297/UnsafeHostnameVerification.ql | 75 +++++++++++-------- .../semmle/code/java/security/Encryption.qll | 7 ++ .../CWE-297/UnsafeHostnameVerification.java | 7 ++ 3 files changed, 58 insertions(+), 31 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.ql b/java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.ql index 0a4f02445de2..8fa444b5caed 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.ql @@ -9,39 +9,30 @@ import java import semmle.code.java.security.Encryption +import semmle.code.java.dataflow.DataFlow + +/** A return statement that returns `true`. */ +private class TrueReturnStmt extends ReturnStmt { + TrueReturnStmt() { getResult().(CompileTimeConstantExpr).getBooleanValue() = true } +} /** - * HostnameVerifier class that allows a certificate whose CN (Common Name) does not match the host name in the URL + * Holds if `m` always returns `true` ignoring any exceptional flow. */ -class TrustAllHostnameVerifier extends RefType { - TrustAllHostnameVerifier() { - this.getASupertype*() instanceof HostnameVerifier and - exists(Method m, ReturnStmt rt | - m.getDeclaringType() = this and - m.hasName("verify") and - rt.getEnclosingCallable() = m and - rt.getResult().(BooleanLiteral).getBooleanValue() = true - ) - } +private predicate alwaysReturnsTrue(HostnameVerifierVerify m) { + forex(ReturnStmt rs | rs.getEnclosingCallable() = m | rs instanceof TrueReturnStmt) } /** - * The setDefaultHostnameVerifier method of HttpsURLConnection with the trust all configuration + * A class that overrides the `javax.net.ssl.HostnameVerifier.verify` method and **always** returns `true`, thus + * accepting any certificate despite a hostname mismatch. */ -class TrustAllHostnameVerify extends MethodAccess { - TrustAllHostnameVerify() { - this.getMethod().hasName("setDefaultHostnameVerifier") and - this.getMethod().getDeclaringType() instanceof HttpsURLConnection and //httpsURLConnection.setDefaultHostnameVerifier method - ( - exists(NestedClass nc | - nc.getASupertype*() instanceof TrustAllHostnameVerifier and - this.getArgument(0).getType() = nc //Scenario of HttpsURLConnection.setDefaultHostnameVerifier(new HostnameVerifier() {...}); - ) - or - exists(Variable v | - this.getArgument(0).(VarAccess).getVariable() = v and - v.getInitializer().getType() instanceof TrustAllHostnameVerifier //Scenario of HttpsURLConnection.setDefaultHostnameVerifier(verifier); - ) +class TrustAllHostnameVerifier extends RefType { + TrustAllHostnameVerifier() { + this.getASupertype*() instanceof HostnameVerifier and + exists(HostnameVerifierVerify m | + m.getDeclaringType() = this and + alwaysReturnsTrue(m) ) } } @@ -62,6 +53,26 @@ class SSLSocket extends RefType { SSLSocket() { this.hasQualifiedName("javax.net.ssl", "SSLSocket") } } +/** + * A configuration to model the flow of a `TrustAllHostnameVerifier` to a `set(Default)HostnameVerifier` call. + */ +class TrustAllHostnameVerifierConfiguration extends DataFlow::Configuration { + TrustAllHostnameVerifierConfiguration() { this = "TrustAllHostnameVerifierConfiguration" } + + override predicate isSource(DataFlow::Node source) { + source.asExpr().(ClassInstanceExpr).getConstructedType() instanceof TrustAllHostnameVerifier + } + + override predicate isSink(DataFlow::Node sink) { + exists(MethodAccess ma, Method m | + (m instanceof SetDefaultHostnameVerifierMethod or m instanceof SetHostnameVerifierMethod) and + ma.getMethod() = m + | + ma.getArgument(0) = sink.asExpr() + ) + } +} + /** * has setEndpointIdentificationAlgorithm set correctly */ @@ -194,9 +205,11 @@ class RabbitMQEnableHostnameVerificationNotSet extends MethodAccess { } } -from MethodAccess aa +//from MethodAccess aa +from DataFlow::Node source, DataFlow::Node sink, TrustAllHostnameVerifierConfiguration cfg where - aa instanceof TrustAllHostnameVerify or - aa instanceof SSLEndpointIdentificationNotSet or - aa instanceof RabbitMQEnableHostnameVerificationNotSet -select aa, "Unsafe configuration of trusted certificates" +cfg.hasFlow(source, sink) + //aa instanceof TrustAllHostnameVerify or + //aa instanceof SSLEndpointIdentificationNotSet or + //aa instanceof RabbitMQEnableHostnameVerificationNotSet +select sink, "Unsafe configuration of trusted certificates" diff --git a/java/ql/src/semmle/code/java/security/Encryption.qll b/java/ql/src/semmle/code/java/security/Encryption.qll index ea7a33151f8f..961153cc5752 100644 --- a/java/ql/src/semmle/code/java/security/Encryption.qll +++ b/java/ql/src/semmle/code/java/security/Encryption.qll @@ -67,6 +67,13 @@ class SetConnectionFactoryMethod extends Method { } } +class SetDefaultHostnameVerifierMethod extends Method { + SetDefaultHostnameVerifierMethod() { + hasName("setDefaultHostnameVerifier") and + getDeclaringType().getASupertype*() instanceof HttpsURLConnection + } +} + class SetHostnameVerifierMethod extends Method { SetHostnameVerifierMethod() { hasName("setHostnameVerifier") and diff --git a/java/ql/test/experimental/query-tests/security/CWE-297/UnsafeHostnameVerification.java b/java/ql/test/experimental/query-tests/security/CWE-297/UnsafeHostnameVerification.java index 5e3913a8a046..17fab6d1199d 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-297/UnsafeHostnameVerification.java +++ b/java/ql/test/experimental/query-tests/security/CWE-297/UnsafeHostnameVerification.java @@ -30,6 +30,13 @@ public boolean verify(String hostname, SSLSession session) { }); } + /** + * Test the implementation of trusting all hostnames as a lambda. + */ + public void testTrustAllHostnameLambda() { + HttpsURLConnection.setDefaultHostnameVerifier((name, s) -> true); + } + /** * Test the implementation of trusting all hostnames as a variable */ From 2a0067e5b46d151b263c1354c4cb2af0ceaadd78 Mon Sep 17 00:00:00 2001 From: intrigus Date: Wed, 21 Oct 2020 23:35:45 +0200 Subject: [PATCH 4/5] Further separation. Separate trust-all hostname verification and cases that miss enabling SSL endpoint identification. --- .../MissingSslEndpointIdentification.ql | 165 ++++++++++++++++++ .../CWE/CWE-297/UnsafeHostnameVerification.ql | 139 +-------------- 2 files changed, 166 insertions(+), 138 deletions(-) create mode 100644 java/ql/src/experimental/Security/CWE/CWE-297/MissingSslEndpointIdentification.ql diff --git a/java/ql/src/experimental/Security/CWE/CWE-297/MissingSslEndpointIdentification.ql b/java/ql/src/experimental/Security/CWE/CWE-297/MissingSslEndpointIdentification.ql new file mode 100644 index 000000000000..1984319ac220 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-297/MissingSslEndpointIdentification.ql @@ -0,0 +1,165 @@ +/** + * @name Unsafe certificate trust and improper hostname verification + * @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. + * @kind problem + * @id java/unsafe-cert-trust + * @tags security + * external/cwe-273 + */ + +import java +import semmle.code.java.security.Encryption + +class SSLEngine extends RefType { + SSLEngine() { this.hasQualifiedName("javax.net.ssl", "SSLEngine") } +} + +class Socket extends RefType { + Socket() { this.hasQualifiedName("java.net", "Socket") } +} + +class SocketFactory extends RefType { + SocketFactory() { this.hasQualifiedName("javax.net", "SocketFactory") } +} + +class SSLSocket extends RefType { + SSLSocket() { this.hasQualifiedName("javax.net.ssl", "SSLSocket") } +} + +/** + * has setEndpointIdentificationAlgorithm set correctly + */ +predicate setEndpointIdentificationAlgorithm(MethodAccess createSSL) { + exists( + Variable sslo, MethodAccess ma, Variable sslparams //setSSLParameters with valid setEndpointIdentificationAlgorithm set + | + createSSL = sslo.getAnAssignedValue() and + ma.getQualifier() = sslo.getAnAccess() and + ma.getMethod().hasName("setSSLParameters") and + ma.getArgument(0) = sslparams.getAnAccess() and + exists(MethodAccess setepa | + setepa.getQualifier() = sslparams.getAnAccess() and + setepa.getMethod().hasName("setEndpointIdentificationAlgorithm") and + not setepa.getArgument(0) instanceof NullLiteral + ) + ) +} + +/** + * has setEndpointIdentificationAlgorithm set correctly + */ +predicate hasEndpointIdentificationAlgorithm(Variable ssl) { + exists( + MethodAccess ma, Variable sslparams //setSSLParameters with valid setEndpointIdentificationAlgorithm set + | + ma.getQualifier() = ssl.getAnAccess() and + ma.getMethod().hasName("setSSLParameters") and + ma.getArgument(0) = sslparams.getAnAccess() and + exists(MethodAccess setepa | + setepa.getQualifier() = sslparams.getAnAccess() and + setepa.getMethod().hasName("setEndpointIdentificationAlgorithm") and + not setepa.getArgument(0) instanceof NullLiteral + ) + ) +} + +/** + * Cast of Socket to SSLSocket + */ +predicate sslCast(MethodAccess createSSL) { + exists(Variable ssl, CastExpr ce | + ce.getExpr() = createSSL and + ce.getControlFlowNode().getASuccessor().(VariableAssign).getDestVar() = ssl and + ssl.getType() instanceof SSLSocket //With a type cast `SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443)` + ) +} + +/** + * SSL object is created in a separate method call or in the same method + */ +predicate hasFlowPath(MethodAccess createSSL, Variable ssl) { + ( + createSSL = ssl.getAnAssignedValue() + or + exists(CastExpr ce | + ce.getExpr() = createSSL and + ce.getControlFlowNode().getASuccessor().(VariableAssign).getDestVar() = ssl //With a type cast like SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443); + ) + ) + or + exists(MethodAccess tranm | + createSSL.getEnclosingCallable() = tranm.getMethod() and + tranm.getControlFlowNode().getASuccessor().(VariableAssign).getDestVar() = ssl and + not setEndpointIdentificationAlgorithm(createSSL) //Check the scenario of invocation before used in the current method + ) +} + +/** + * Not have the SSLParameter set + */ +predicate hasNoEndpointIdentificationSet(MethodAccess createSSL, Variable ssl) { + //No setSSLParameters set + hasFlowPath(createSSL, ssl) and + not exists(MethodAccess ma | + ma.getQualifier() = ssl.getAnAccess() and + ma.getMethod().hasName("setSSLParameters") + ) + or + //No endpointIdentificationAlgorithm set with setSSLParameters + hasFlowPath(createSSL, ssl) and + not setEndpointIdentificationAlgorithm(createSSL) +} + +/** + * The setEndpointIdentificationAlgorithm method of SSLParameters with the ssl engine or socket + */ +class SSLEndpointIdentificationNotSet extends MethodAccess { + SSLEndpointIdentificationNotSet() { + ( + this.getMethod().hasName("createSSLEngine") and + this.getMethod().getDeclaringType() instanceof SSLContext //createEngine method of SSLContext + or + this.getMethod().hasName("createSocket") and + this.getMethod().getDeclaringType() instanceof SocketFactory and + this.getMethod().getReturnType() instanceof Socket and + sslCast(this) //createSocket method of SocketFactory + ) and + exists(Variable ssl | + hasNoEndpointIdentificationSet(this, ssl) and //Not set in itself + not exists(VariableAssign ar, Variable newSsl | + ar.getSource() = this.getCaller().getAReference() and + ar.getDestVar() = newSsl and + hasEndpointIdentificationAlgorithm(newSsl) //Not set in its caller either + ) + ) and + not exists(MethodAccess ma | ma.getMethod() instanceof HostnameVerifierVerify) //Reduce false positives since this method access set default hostname verifier + } +} + +class RabbitMQConnectionFactory extends RefType { + RabbitMQConnectionFactory() { this.hasQualifiedName("com.rabbitmq.client", "ConnectionFactory") } +} + +/** + * The com.rabbitmq.client.ConnectionFactory useSslProtocol method access without enableHostnameVerification + */ +class RabbitMQEnableHostnameVerificationNotSet extends MethodAccess { + RabbitMQEnableHostnameVerificationNotSet() { + this.getMethod().hasName("useSslProtocol") and + this.getMethod().getDeclaringType() instanceof RabbitMQConnectionFactory and + exists(Variable v | + v.getType() instanceof RabbitMQConnectionFactory and + this.getQualifier() = v.getAnAccess() and + not exists(MethodAccess ma | + ma.getMethod().hasName("enableHostnameVerification") and + ma.getQualifier() = v.getAnAccess() + ) + ) + } +} + +from MethodAccess aa +where + aa instanceof SSLEndpointIdentificationNotSet or + aa instanceof RabbitMQEnableHostnameVerificationNotSet +select aa, "Unsafe configuration of trusted certificates" diff --git a/java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.ql b/java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.ql index 8fa444b5caed..07980f24d1be 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.ql @@ -73,143 +73,6 @@ class TrustAllHostnameVerifierConfiguration extends DataFlow::Configuration { } } -/** - * has setEndpointIdentificationAlgorithm set correctly - */ -predicate setEndpointIdentificationAlgorithm(MethodAccess createSSL) { - exists( - Variable sslo, MethodAccess ma, Variable sslparams //setSSLParameters with valid setEndpointIdentificationAlgorithm set - | - createSSL = sslo.getAnAssignedValue() and - ma.getQualifier() = sslo.getAnAccess() and - ma.getMethod().hasName("setSSLParameters") and - ma.getArgument(0) = sslparams.getAnAccess() and - exists(MethodAccess setepa | - setepa.getQualifier() = sslparams.getAnAccess() and - setepa.getMethod().hasName("setEndpointIdentificationAlgorithm") and - not setepa.getArgument(0) instanceof NullLiteral - ) - ) -} - -/** - * has setEndpointIdentificationAlgorithm set correctly - */ -predicate hasEndpointIdentificationAlgorithm(Variable ssl) { - exists( - MethodAccess ma, Variable sslparams //setSSLParameters with valid setEndpointIdentificationAlgorithm set - | - ma.getQualifier() = ssl.getAnAccess() and - ma.getMethod().hasName("setSSLParameters") and - ma.getArgument(0) = sslparams.getAnAccess() and - exists(MethodAccess setepa | - setepa.getQualifier() = sslparams.getAnAccess() and - setepa.getMethod().hasName("setEndpointIdentificationAlgorithm") and - not setepa.getArgument(0) instanceof NullLiteral - ) - ) -} - -/** - * Cast of Socket to SSLSocket - */ -predicate sslCast(MethodAccess createSSL) { - exists(Variable ssl, CastExpr ce | - ce.getExpr() = createSSL and - ce.getControlFlowNode().getASuccessor().(VariableAssign).getDestVar() = ssl and - ssl.getType() instanceof SSLSocket //With a type cast `SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443)` - ) -} - -/** - * SSL object is created in a separate method call or in the same method - */ -predicate hasFlowPath(MethodAccess createSSL, Variable ssl) { - ( - createSSL = ssl.getAnAssignedValue() - or - exists(CastExpr ce | - ce.getExpr() = createSSL and - ce.getControlFlowNode().getASuccessor().(VariableAssign).getDestVar() = ssl //With a type cast like SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443); - ) - ) - or - exists(MethodAccess tranm | - createSSL.getEnclosingCallable() = tranm.getMethod() and - tranm.getControlFlowNode().getASuccessor().(VariableAssign).getDestVar() = ssl and - not setEndpointIdentificationAlgorithm(createSSL) //Check the scenario of invocation before used in the current method - ) -} - -/** - * Not have the SSLParameter set - */ -predicate hasNoEndpointIdentificationSet(MethodAccess createSSL, Variable ssl) { - //No setSSLParameters set - hasFlowPath(createSSL, ssl) and - not exists(MethodAccess ma | - ma.getQualifier() = ssl.getAnAccess() and - ma.getMethod().hasName("setSSLParameters") - ) - or - //No endpointIdentificationAlgorithm set with setSSLParameters - hasFlowPath(createSSL, ssl) and - not setEndpointIdentificationAlgorithm(createSSL) -} - -/** - * The setEndpointIdentificationAlgorithm method of SSLParameters with the ssl engine or socket - */ -class SSLEndpointIdentificationNotSet extends MethodAccess { - SSLEndpointIdentificationNotSet() { - ( - this.getMethod().hasName("createSSLEngine") and - this.getMethod().getDeclaringType() instanceof SSLContext //createEngine method of SSLContext - or - this.getMethod().hasName("createSocket") and - this.getMethod().getDeclaringType() instanceof SocketFactory and - this.getMethod().getReturnType() instanceof Socket and - sslCast(this) //createSocket method of SocketFactory - ) and - exists(Variable ssl | - hasNoEndpointIdentificationSet(this, ssl) and //Not set in itself - not exists(VariableAssign ar, Variable newSsl | - ar.getSource() = this.getCaller().getAReference() and - ar.getDestVar() = newSsl and - hasEndpointIdentificationAlgorithm(newSsl) //Not set in its caller either - ) - ) and - not exists(MethodAccess ma | ma.getMethod() instanceof HostnameVerifierVerify) //Reduce false positives since this method access set default hostname verifier - } -} - -class RabbitMQConnectionFactory extends RefType { - RabbitMQConnectionFactory() { this.hasQualifiedName("com.rabbitmq.client", "ConnectionFactory") } -} - -/** - * The com.rabbitmq.client.ConnectionFactory useSslProtocol method access without enableHostnameVerification - */ -class RabbitMQEnableHostnameVerificationNotSet extends MethodAccess { - RabbitMQEnableHostnameVerificationNotSet() { - this.getMethod().hasName("useSslProtocol") and - this.getMethod().getDeclaringType() instanceof RabbitMQConnectionFactory and - exists(Variable v | - v.getType() instanceof RabbitMQConnectionFactory and - this.getQualifier() = v.getAnAccess() and - not exists(MethodAccess ma | - ma.getMethod().hasName("enableHostnameVerification") and - ma.getQualifier() = v.getAnAccess() - ) - ) - } -} - -//from MethodAccess aa from DataFlow::Node source, DataFlow::Node sink, TrustAllHostnameVerifierConfiguration cfg -where -cfg.hasFlow(source, sink) - //aa instanceof TrustAllHostnameVerify or - //aa instanceof SSLEndpointIdentificationNotSet or - //aa instanceof RabbitMQEnableHostnameVerificationNotSet +where cfg.hasFlow(source, sink) select sink, "Unsafe configuration of trusted certificates" From e140fb580866e6bbef6312af4b5ff226f4f38dd8 Mon Sep 17 00:00:00 2001 From: intrigus Date: Wed, 2 Dec 2020 20:49:17 +0100 Subject: [PATCH 5/5] Inline `TrueStmt` class --- .../Security/CWE/CWE-297/UnsafeHostnameVerification.ql | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.ql b/java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.ql index 07980f24d1be..b4f5cdbe7cd0 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.ql @@ -11,16 +11,13 @@ import java import semmle.code.java.security.Encryption import semmle.code.java.dataflow.DataFlow -/** A return statement that returns `true`. */ -private class TrueReturnStmt extends ReturnStmt { - TrueReturnStmt() { getResult().(CompileTimeConstantExpr).getBooleanValue() = true } -} - /** * Holds if `m` always returns `true` ignoring any exceptional flow. */ private predicate alwaysReturnsTrue(HostnameVerifierVerify m) { - forex(ReturnStmt rs | rs.getEnclosingCallable() = m | rs instanceof TrueReturnStmt) + forex(ReturnStmt rs | rs.getEnclosingCallable() = m | + rs.getResult().(CompileTimeConstantExpr).getBooleanValue() = true + ) } /**