From 6d1ba3f8997a633af8fe93fe3c6e53e94966b5ef Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Sun, 24 May 2020 16:43:15 +0000 Subject: [PATCH 1/5] Java: CWE-273 Unsafe certificate trust --- .../Security/CWE/CWE-273/UnsafeCertTrust.java | 68 +++++++++++ .../CWE/CWE-273/UnsafeCertTrust.qhelp | 33 ++++++ .../Security/CWE/CWE-273/UnsafeCertTrust.ql | 95 +++++++++++++++ .../security/CWE-273/UnsafeCertTrust.expected | 4 + .../security/CWE-273/UnsafeCertTrust.qlref | 1 + .../security/CWE-273/UnsafeCertTrustTest.java | 110 ++++++++++++++++++ 6 files changed, 311 insertions(+) create mode 100644 java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.java create mode 100644 java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.qhelp create mode 100644 java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql create mode 100644 java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.expected create mode 100644 java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.qlref create mode 100644 java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrustTest.java diff --git a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.java b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.java new file mode 100644 index 000000000000..75c0269bde8c --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.java @@ -0,0 +1,68 @@ +public static void main(String[] args) { + { + HostnameVerifier verifier = new HostnameVerifier() { + @Override + public boolean verify(String hostname, SSLSession session) { + try { //GOOD: verify the certificate + Certificate[] certs = session.getPeerCertificates(); + X509Certificate x509 = (X509Certificate) certs[0]; + check(new String[]{host}, x509); + return true; + } catch (SSLException e) { + return false; + } + } + }; + HttpsURLConnection.setDefaultHostnameVerifier(verifier); + } + + { + HostnameVerifier verifier = new HostnameVerifier() { + @Override + public boolean verify(String hostname, SSLSession session) { + return true; // BAD: accept even if the hostname doesn't match + } + }; + 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 + } + }; + } +} \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.qhelp b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.qhelp new file mode 100644 index 000000000000..713034200462 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.qhelp @@ -0,0 +1,33 @@ + + + + +

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.

+

Unsafe implementation of the interface X509TrustManager and HostnameVerifier 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 or the hostname verifier is turned off.

+ + + +

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-273 +
    • +
  • +How to fix apps containing an unsafe implementation of TrustManager +
    • +
  • +Testing Endpoint Identify Verification (MSTG-NETWORK-3) +
    • +     +     \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql new file mode 100644 index 000000000000..380b1b79d953 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql @@ -0,0 +1,95 @@ +/** + * @id java/unsafe-cert-trust + * @name Unsafe implementation of trusting any certificate in SSL configuration + * @description Unsafe implementation of the interface X509TrustManager and HostnameVerifier ignores all SSL certificate validation errors when establishing an HTTPS connection, thereby making the app vulnerable to man-in-the-middle attacks. + * @kind problem + * @tags security + * external/cwe-273 + */ + +import java +import semmle.code.java.security.Encryption +import semmle.code.java.dataflow.DataFlow +import DataFlow + +/** + * 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 | + 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.getAnAccess().getVariable().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.getAnAccess().getVariable() and + v.getInitializer().getType() instanceof TrustAllHostnameVerifier //Scenario of HttpsURLConnection.setDefaultHostnameVerifier(verifier); + ) + ) + } +} + +from MethodAccess aa +where aa instanceof TrustAllHostnameVerify or aa instanceof X509TrustAllManagerInit +select aa, "Unsafe configuration of trusted certificates" \ No newline at end of file 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 new file mode 100644 index 000000000000..8a592b94654b --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.expected @@ -0,0 +1,4 @@ +| UnsafeCertTrustTest.java:19:4:19:74 | init(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:34:4:34:38 | init(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:47:3:52:4 | setDefaultHostnameVerifier(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:65:3:65:57 | setDefaultHostnameVerifier(...) | 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 new file mode 100644 index 000000000000..f054d6037876 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.qlref @@ -0,0 +1 @@ +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 new file mode 100644 index 000000000000..459d2e3e5f58 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrustTest.java @@ -0,0 +1,110 @@ +import javax.net.ssl.HostnameVerifier; +import javax.net.ssl.HttpsURLConnection; +import javax.net.ssl.SSLSession; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLSocketFactory; +import javax.net.ssl.TrustManager; +import javax.net.ssl.X509TrustManager; +import java.security.cert.CertificateException; +import java.security.cert.X509Certificate; + +public class UnsafeCertTrustTest { + + /** + * Test the implementation of trusting all server certs as a variable + */ + public SSLSocketFactory testTrustAllCertManager() { + try { + final SSLContext context = SSLContext.getInstance("SSL"); + 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("SSL"); + 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 + } + }; +} From 104f1c3197d6399880a6a63e8398765690a34a23 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Thu, 28 May 2020 03:34:29 +0000 Subject: [PATCH 2/5] Add validation query for SSL Engine/Socket and com.rabbitmq.client.ConnectionFactory --- .../Security/CWE/CWE-273/UnsafeCertTrust.java | 33 ++++ .../CWE/CWE-273/UnsafeCertTrust.qhelp | 19 ++- .../Security/CWE/CWE-273/UnsafeCertTrust.ql | 145 +++++++++++++++++- .../security/CWE-273/UnsafeCertTrust.expected | 11 +- .../security/CWE-273/UnsafeCertTrustTest.java | 49 +++++- 5 files changed, 241 insertions(+), 16 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.java b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.java index 75c0269bde8c..65698ac33a30 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.java +++ b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.java @@ -65,4 +65,37 @@ public X509Certificate[] getAcceptedIssuers() { } }; } + + { + SSLContext sslContext = SSLContext.getInstance("TLS"); + SSLEngine sslEngine = sslContext.createSSLEngine(); + SSLParameters sslParameters = sslEngine.getSSLParameters(); + sslParameters.setEndpointIdentificationAlgorithm("HTTPS"); //GOOD: Set a valid endpointIdentificationAlgorithm for SSL engine to trigger hostname verification + sslEngine.setSSLParameters(sslParameters); + } + + { + SSLContext sslContext = SSLContext.getInstance("TLS"); + SSLEngine sslEngine = sslContext.createSSLEngine(); //BAD: No endpointIdentificationAlgorithm set + } + + { + SSLContext sslContext = SSLContext.getInstance("TLS"); + final SSLSocketFactory socketFactory = sslContext.getSocketFactory(); + SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443); + SSLParameters sslParameters = sslEngine.getSSLParameters(); + sslParameters.setEndpointIdentificationAlgorithm("HTTPS"); //GOOD: Set a valid endpointIdentificationAlgorithm for SSL socket to trigger hostname verification + socket.setSSLParameters(sslParameters); + } + + { + com.rabbitmq.client.ConnectionFactory connectionFactory = new com.rabbitmq.client.ConnectionFactory(); + connectionFactory.useSslProtocol(); + connectionFactory.enableHostnameVerification(); //GOOD: Enable hostname verification for rabbitmq ConnectionFactory + } + + { + com.rabbitmq.client.ConnectionFactory connectionFactory = new com.rabbitmq.client.ConnectionFactory(); + connectionFactory.useSslProtocol(); //BAD: Hostname verification for rabbitmq ConnectionFactory is not enabled + } } \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.qhelp b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.qhelp index 713034200462..2e8d08fd68b9 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.qhelp @@ -5,8 +5,9 @@

    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.

    -

    Unsafe implementation of the interface X509TrustManager and HostnameVerifier 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 or the hostname verifier is turned off.

    +

    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.

     @@ -29,5 +30,17 @@ no validation is performed thus any certificate is trusted. In the 'GOOD' case,
  • Testing Endpoint Identify Verification (MSTG-NETWORK-3)
    • +
  • +CVE-2018-17187: Apache Qpid Proton-J transport issue with hostname verification +
    • +
  • +CVE-2018-8034: Apache Tomcat - host name verification when using TLS with the WebSocket client +
    • +
  • +CVE-2018-11087: Pivotal Spring AMQP vulnerability due to lack of hostname validation +
    • +
  • +CVE-2018-11775: TLS hostname verification issue when using the Apache ActiveMQ Client +
    • - \ No newline at end of file + diff --git a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql index 380b1b79d953..7df3a3548261 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql @@ -1,7 +1,7 @@ /** * @id java/unsafe-cert-trust - * @name Unsafe implementation of trusting any certificate in SSL configuration - * @description Unsafe implementation of the interface X509TrustManager and HostnameVerifier ignores all SSL certificate validation errors when establishing an HTTPS connection, thereby making the app vulnerable to man-in-the-middle attacks. + * @name Unsafe implementation of trusting any certificate or missing hostname verification in SSL configuration + * @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 * @tags security * external/cwe-273 @@ -9,8 +9,6 @@ import java import semmle.code.java.security.Encryption -import semmle.code.java.dataflow.DataFlow -import DataFlow /** * X509TrustManager class that blindly trusts all certificates in server SSL authentication @@ -79,7 +77,7 @@ class TrustAllHostnameVerify extends MethodAccess { ( exists(NestedClass nc | nc.getASupertype*() instanceof TrustAllHostnameVerifier and - this.getArgument(0).getType() = nc //Scenario of HttpsURLConnection.setDefaultHostnameVerifier(new HostnameVerifier() {...}); + this.getArgument(0).getType() = nc //Scenario of HttpsURLConnection.setDefaultHostnameVerifier(new HostnameVerifier() {...}); ) or exists(Variable v | @@ -90,6 +88,141 @@ class TrustAllHostnameVerify extends MethodAccess { } } +class SSLEngine extends RefType { + SSLEngine() { this.hasQualifiedName("javax.net.ssl", "SSLEngine") } +} + +class Socket extends RefType { + Socket() { this.hasQualifiedName("java.net", "Socket") } +} + +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).(VarAccess) = sslparams.getAnAccess() and + exists(MethodAccess setepa | + setepa.getQualifier().(VarAccess) = 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).(VarAccess) = sslparams.getAnAccess() and + exists(MethodAccess setepa | + setepa.getQualifier().(VarAccess) = sslparams.getAnAccess() and + setepa.getMethod().hasName("setEndpointIdentificationAlgorithm") and + not setepa.getArgument(0) instanceof NullLiteral + ) + ) +} + +/** + * 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().(MethodAccess) = 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().(Method) = 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().getReturnType() instanceof Socket //createSocket method of SSLSocketFactory + ) 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(VarAccess va | + va.getVariable().getType() instanceof RabbitMQConnectionFactory and + this.getQualifier() = va.getVariable().getAnAccess() and + not exists(MethodAccess ma | + ma.getMethod().hasName("enableHostnameVerification") and + ma.getQualifier() = va.getVariable().getAnAccess() + ) + ) + } +} + from MethodAccess aa -where aa instanceof TrustAllHostnameVerify or aa instanceof X509TrustAllManagerInit +where + aa instanceof TrustAllHostnameVerify or + aa instanceof X509TrustAllManagerInit or + aa instanceof SSLEndpointIdentificationNotSet or + aa instanceof RabbitMQEnableHostnameVerificationNotSet select aa, "Unsafe configuration of trusted certificates" \ No newline at end of file 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 index 8a592b94654b..7bf8454bd263 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.expected @@ -1,4 +1,7 @@ -| UnsafeCertTrustTest.java:19:4:19:74 | init(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:34:4:34:38 | init(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:47:3:52:4 | setDefaultHostnameVerifier(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:65:3:65:57 | setDefaultHostnameVerifier(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:26:4:26:74 | init(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:41:4:41:38 | init(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:54:3:59:4 | setDefaultHostnameVerifier(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:72:3:72:57 | setDefaultHostnameVerifier(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:123:25:123:52 | createSSLEngine(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:134:25:134:52 | createSSLEngine(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:143:34:143:83 | createSocket(...) | Unsafe configuration of trusted certificates | 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 index 459d2e3e5f58..920e9e1a9039 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrustTest.java +++ b/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrustTest.java @@ -1,13 +1,20 @@ 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 java.security.cert.CertificateException; import java.security.cert.X509Certificate; +//import com.rabbitmq.client.ConnectionFactory; + public class UnsafeCertTrustTest { /** @@ -15,7 +22,7 @@ public class UnsafeCertTrustTest { */ public SSLSocketFactory testTrustAllCertManager() { try { - final SSLContext context = SSLContext.getInstance("SSL"); + final SSLContext context = SSLContext.getInstance("TLS"); context.init(null, new TrustManager[] { TRUST_ALL_CERTIFICATES }, null); final SSLSocketFactory socketFactory = context.getSocketFactory(); return socketFactory; @@ -29,7 +36,7 @@ public SSLSocketFactory testTrustAllCertManager() { */ public SSLSocketFactory testTrustAllCertManagerOfVariable() { try { - SSLContext context = SSLContext.getInstance("SSL"); + SSLContext context = SSLContext.getInstance("TLS"); TrustManager[] serverTMs = new TrustManager[] { new X509TrustAllManager() }; context.init(null, serverTMs, null); @@ -107,4 +114,40 @@ 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 enableHostnameVerification of RabbitMQConnectionFactory is not set + // */ + // public void testEnableHostnameVerificationOfRabbitMQFactoryNotSet() { + // ConnectionFactory connectionFactory = new ConnectionFactory(); + // connectionFactory.useSslProtocol(); + // } +} \ No newline at end of file From deabfe6e5cba6755ee6132c3ec7985a17c2cc63c Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Tue, 23 Jun 2020 12:24:03 +0000 Subject: [PATCH 3/5] Adjust id tag and fix ending line error --- .../src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql index 7df3a3548261..ce066d70ca69 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql @@ -1,8 +1,8 @@ /** - * @id java/unsafe-cert-trust * @name Unsafe implementation of trusting any certificate or missing hostname verification in SSL configuration * @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 */ @@ -225,4 +225,5 @@ where aa instanceof X509TrustAllManagerInit or aa instanceof SSLEndpointIdentificationNotSet or aa instanceof RabbitMQEnableHostnameVerificationNotSet -select aa, "Unsafe configuration of trusted certificates" \ No newline at end of file +select aa, "Unsafe configuration of trusted certificates" + From f8c494716fa0800b0b4d95e83222a68ae79e1ee0 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Tue, 23 Jun 2020 12:48:07 +0000 Subject: [PATCH 4/5] Fix ending line error --- java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql | 1 - 1 file changed, 1 deletion(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql index ce066d70ca69..6e2757be3062 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql @@ -226,4 +226,3 @@ where aa instanceof SSLEndpointIdentificationNotSet or aa instanceof RabbitMQEnableHostnameVerificationNotSet select aa, "Unsafe configuration of trusted certificates" - From 0779aab28fe1baa0db8a70862e932a7a05e2a670 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Wed, 24 Jun 2020 15:02:16 +0000 Subject: [PATCH 5/5] Clean up the QL code --- .../Security/CWE/CWE-273/UnsafeCertTrust.ql | 46 +++++++++++++------ .../security/CWE-273/UnsafeCertTrust.expected | 14 +++--- .../security/CWE-273/UnsafeCertTrustTest.java | 19 ++++++-- 3 files changed, 53 insertions(+), 26 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql index 6e2757be3062..497e87b37ac3 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql @@ -1,5 +1,5 @@ /** - * @name Unsafe implementation of trusting any certificate or missing hostname verification in SSL configuration + * @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 @@ -39,13 +39,14 @@ class X509TrustAllManagerInit extends MethodAccess { 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.getAnAccess().getVariable().getAnAssignedValue() and + ai.getParent() = v.getAnAssignedValue() and ai.getInit(0).getType().(Class).getASupertype*() instanceof X509TrustAllManager //Scenario of context.init(null, serverTMs, null); ) ) @@ -81,7 +82,7 @@ class TrustAllHostnameVerify extends MethodAccess { ) or exists(Variable v | - this.getArgument(0).(VarAccess).getVariable() = v.getAnAccess().getVariable() and + this.getArgument(0).(VarAccess).getVariable() = v and v.getInitializer().getType() instanceof TrustAllHostnameVerifier //Scenario of HttpsURLConnection.setDefaultHostnameVerifier(verifier); ) ) @@ -96,6 +97,10 @@ 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") } } @@ -110,9 +115,9 @@ predicate setEndpointIdentificationAlgorithm(MethodAccess createSSL) { createSSL = sslo.getAnAssignedValue() and ma.getQualifier() = sslo.getAnAccess() and ma.getMethod().hasName("setSSLParameters") and - ma.getArgument(0).(VarAccess) = sslparams.getAnAccess() and + ma.getArgument(0) = sslparams.getAnAccess() and exists(MethodAccess setepa | - setepa.getQualifier().(VarAccess) = sslparams.getAnAccess() and + setepa.getQualifier() = sslparams.getAnAccess() and setepa.getMethod().hasName("setEndpointIdentificationAlgorithm") and not setepa.getArgument(0) instanceof NullLiteral ) @@ -128,15 +133,26 @@ predicate hasEndpointIdentificationAlgorithm(Variable ssl) { | ma.getQualifier() = ssl.getAnAccess() and ma.getMethod().hasName("setSSLParameters") and - ma.getArgument(0).(VarAccess) = sslparams.getAnAccess() and + ma.getArgument(0) = sslparams.getAnAccess() and exists(MethodAccess setepa | - setepa.getQualifier().(VarAccess) = sslparams.getAnAccess() and + 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 */ @@ -145,13 +161,13 @@ predicate hasFlowPath(MethodAccess createSSL, Variable ssl) { createSSL = ssl.getAnAssignedValue() or exists(CastExpr ce | - ce.getExpr().(MethodAccess) = createSSL and + 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().(Method) = tranm.getMethod() and + 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 ) @@ -183,7 +199,9 @@ class SSLEndpointIdentificationNotSet extends MethodAccess { this.getMethod().getDeclaringType() instanceof SSLContext //createEngine method of SSLContext or this.getMethod().hasName("createSocket") and - this.getMethod().getReturnType() instanceof Socket //createSocket method of SSLSocketFactory + 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 @@ -208,12 +226,12 @@ class RabbitMQEnableHostnameVerificationNotSet extends MethodAccess { RabbitMQEnableHostnameVerificationNotSet() { this.getMethod().hasName("useSslProtocol") and this.getMethod().getDeclaringType() instanceof RabbitMQConnectionFactory and - exists(VarAccess va | - va.getVariable().getType() instanceof RabbitMQConnectionFactory and - this.getQualifier() = va.getVariable().getAnAccess() 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() = va.getVariable().getAnAccess() + ma.getQualifier() = v.getAnAccess() ) ) } 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 index 7bf8454bd263..c0ea40f9bdb0 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.expected @@ -1,7 +1,7 @@ -| UnsafeCertTrustTest.java:26:4:26:74 | init(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:41:4:41:38 | init(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:54:3:59:4 | setDefaultHostnameVerifier(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:72:3:72:57 | setDefaultHostnameVerifier(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:123:25:123:52 | createSSLEngine(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:134:25:134:52 | createSSLEngine(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:143:34:143:83 | createSocket(...) | Unsafe configuration of trusted certificates | +| 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/UnsafeCertTrustTest.java b/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrustTest.java index 920e9e1a9039..ff62035fd333 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrustTest.java +++ b/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrustTest.java @@ -10,6 +10,7 @@ import javax.net.ssl.X509TrustManager; import java.net.Socket; +import javax.net.SocketFactory; import java.security.cert.CertificateException; import java.security.cert.X509Certificate; @@ -126,7 +127,7 @@ public void testSSLEngineEndpointIdSetNull() { sslEngine.setSSLParameters(sslParameters); } - /** + /** * Test the endpoint identification of SSL engine is not set */ public void testSSLEngineEndpointIdNotSet() { @@ -143,11 +144,19 @@ public void testSSLSocketEndpointIdNotSet() { 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 - // */ + // * Test the enableHostnameVerification of RabbitMQConnectionFactory is not set + // */ // public void testEnableHostnameVerificationOfRabbitMQFactoryNotSet() { - // ConnectionFactory connectionFactory = new ConnectionFactory(); - // connectionFactory.useSslProtocol(); + // ConnectionFactory connectionFactory = new ConnectionFactory(); + // connectionFactory.useSslProtocol(); // } } \ No newline at end of file