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..65698ac33a30 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.java @@ -0,0 +1,101 @@ +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 + } + }; + } + + { + 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 new file mode 100644 index 000000000000..2e8d08fd68b9 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.qhelp @@ -0,0 +1,46 @@ + + + + +

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-273 +
    • +
  • +How to fix apps containing an unsafe implementation of TrustManager +
    • +
  • +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 +
    • +     +     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..497e87b37ac3 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.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-273/UnsafeCertTrust.expected b/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.expected new file mode 100644 index 000000000000..c0ea40f9bdb0 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.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-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..ff62035fd333 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrustTest.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