From e0f4c73aedf5ae47e9d935477ac6eaacbdd5f22b Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 21 Jun 2021 10:44:32 +0200 Subject: [PATCH 01/26] Move from experimental --- .../{experimental => }/Security/CWE/CWE-273/UnsafeCertTrust.java | 0 .../Security/CWE/CWE-273/UnsafeCertTrust.qhelp | 0 .../{experimental => }/Security/CWE/CWE-273/UnsafeCertTrust.ql | 0 .../query-tests/security/CWE-273/UnsafeCertTrust.qlref | 1 - .../query-tests/security/CWE-273/UnsafeCertTrust.expected | 0 java/ql/test/query-tests/security/CWE-273/UnsafeCertTrust.qlref | 1 + .../query-tests/security/CWE-273/UnsafeCertTrustTest.java | 0 7 files changed, 1 insertion(+), 1 deletion(-) rename java/ql/src/{experimental => }/Security/CWE/CWE-273/UnsafeCertTrust.java (100%) rename java/ql/src/{experimental => }/Security/CWE/CWE-273/UnsafeCertTrust.qhelp (100%) rename java/ql/src/{experimental => }/Security/CWE/CWE-273/UnsafeCertTrust.ql (100%) delete mode 100644 java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.qlref rename java/ql/test/{experimental => }/query-tests/security/CWE-273/UnsafeCertTrust.expected (100%) create mode 100644 java/ql/test/query-tests/security/CWE-273/UnsafeCertTrust.qlref rename java/ql/test/{experimental => }/query-tests/security/CWE-273/UnsafeCertTrustTest.java (100%) diff --git a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.java b/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.java similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.java rename to java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.java diff --git a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.qhelp b/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.qhelp rename to java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp diff --git a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql b/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql rename to java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql 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/UnsafeCertTrust.expected b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrust.expected similarity index 100% rename from java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.expected rename to java/ql/test/query-tests/security/CWE-273/UnsafeCertTrust.expected diff --git a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrust.qlref b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrust.qlref new file mode 100644 index 000000000000..e63adf79e5e3 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrust.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-273/UnsafeCertTrust.ql diff --git a/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrustTest.java b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java similarity index 100% rename from java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrustTest.java rename to java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java From 4313baf6228885378fa85612ebd61a8581e97a19 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 21 Jun 2021 17:36:50 +0200 Subject: [PATCH 02/26] Big refactor: - Move classes and predicates to appropriate libraries - Overhaul the endpoint identification algorithm logic to use taint tracking - Adapt tests --- .../code/java/frameworks/Networking.qll | 20 ++ .../semmle/code/java/security/Encryption.qll | 31 ++- .../code/java/security/UnsafeCertTrust.qll | 103 ++++++++ .../Security/CWE/CWE-273/UnsafeCertTrust.ql | 161 ++----------- .../security/CWE-273/UnsafeCertTrustTest.java | 124 ++++++++-- .../test/query-tests/security/CWE-273/options | 1 + .../rabbitmq/client/ConnectionFactory.java | 223 ++++++++++++++++++ 7 files changed, 494 insertions(+), 169 deletions(-) create mode 100644 java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll create mode 100644 java/ql/test/query-tests/security/CWE-273/options create mode 100644 java/ql/test/stubs/amqp-client-5.12.0/com/rabbitmq/client/ConnectionFactory.java diff --git a/java/ql/lib/semmle/code/java/frameworks/Networking.qll b/java/ql/lib/semmle/code/java/frameworks/Networking.qll index 6e5ad743d44d..7be77e26d648 100644 --- a/java/ql/lib/semmle/code/java/frameworks/Networking.qll +++ b/java/ql/lib/semmle/code/java/frameworks/Networking.qll @@ -14,6 +14,11 @@ class TypeSocket extends RefType { TypeSocket() { this.hasQualifiedName("java.net", "Socket") } } +/** The type `javax.net.SocketFactory` */ +class TypeSocketFactory extends RefType { + TypeSocketFactory() { this.hasQualifiedName("javax.net", "SocketFactory") } +} + /** The type `java.net.URL`. */ class TypeUrl extends RefType { TypeUrl() { this.hasQualifiedName("java.net", "URL") } @@ -143,6 +148,21 @@ class UrlOpenConnectionMethod extends Method { } } +/** The method `javax.net.SocketFactory::createSocket`. */ +class CreateSocketMethod extends Method { + CreateSocketMethod() { + this.hasName("createSocket") and + this.getDeclaringType() instanceof TypeSocketFactory + } +} + +class SocketConnectMethod extends Method { + SocketConnectMethod() { + this.hasName("connect") and + this.getDeclaringType() instanceof TypeSocket + } +} + /** * A string matching private host names of IPv4 and IPv6, which only matches the host portion therefore checking for port is not necessary. * Several examples are localhost, reserved IPv4 IP addresses including 127.0.0.1, 10.x.x.x, 172.16.x,x, 192.168.x,x, and reserved IPv6 addresses including [0:0:0:0:0:0:0:1] and [::1] diff --git a/java/ql/lib/semmle/code/java/security/Encryption.qll b/java/ql/lib/semmle/code/java/security/Encryption.qll index 8f4cef23ee6c..3ae5fdf3d9d2 100644 --- a/java/ql/lib/semmle/code/java/security/Encryption.qll +++ b/java/ql/lib/semmle/code/java/security/Encryption.qll @@ -34,6 +34,19 @@ class SSLSession extends RefType { SSLSession() { this.hasQualifiedName("javax.net.ssl", "SSLSession") } } +class SSLEngine extends RefType { + SSLEngine() { this.hasQualifiedName("javax.net.ssl", "SSLEngine") } +} + +class SSLSocket extends RefType { + SSLSocket() { this.hasQualifiedName("javax.net.ssl", "SSLSocket") } +} + +/** The `javax.net.ssl.SSLParameters` class. */ +class SSLParameters extends RefType { + SSLParameters() { this.hasQualifiedName("javax.net.ssl", "SSLParameters") } +} + class HostnameVerifier extends RefType { HostnameVerifier() { this.hasQualifiedName("javax.net.ssl", "HostnameVerifier") } } @@ -79,6 +92,14 @@ class GetSocketFactory extends Method { } } +/** The `createSSLEngine` method of the class `javax.net.ssl.SSLContext` */ +class CreateSslEngineMethod extends Method { + CreateSslEngineMethod() { + this.hasName("createSSLEngine") and + this.getDeclaringType() instanceof SSLContext + } +} + class SetConnectionFactoryMethod extends Method { SetConnectionFactoryMethod() { this.hasName("setSSLSocketFactory") and @@ -101,6 +122,14 @@ class SetDefaultHostnameVerifierMethod extends Method { } } +/** The `getSession` method of the class `javax.net.ssl.SSLSession`.select */ +class GetSslSessionMethod extends Method { + GetSslSessionMethod() { + hasName("getSession") and + getDeclaringType().getASupertype*() instanceof SSLSession + } +} + bindingset[algorithmString] private string algorithmRegex(string algorithmString) { // Algorithms usually appear in names surrounded by characters that are not @@ -168,7 +197,7 @@ string getInsecureAlgorithmRegex() { string getASecureAlgorithmName() { result = [ - "RSA", "SHA256", "SHA512", "CCM", "GCM", "AES(?![^a-zA-Z](ECB|CBC/PKCS[57]Padding))", + "RSA", "SHA256", "SHA512", "CCM", "GCM", "AES([^a-zA-Z](?!ECB|CBC/PKCS[57]Padding)).*", "Blowfish", "ECIES" ] } diff --git a/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll b/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll new file mode 100644 index 000000000000..4dfa16e20781 --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll @@ -0,0 +1,103 @@ +/** Provides classes and predicates to reason about unsafe certificate trust vulnerablities. */ + +import java +private import semmle.code.java.frameworks.Networking +private import semmle.code.java.security.Encryption +private import semmle.code.java.dataflow.DataFlow +private import semmle.code.java.dataflow.DataFlow2 + +/** + * The creation of an object that prepares an SSL connection. + */ +class SslConnectionInit extends DataFlow::Node { + SslConnectionInit() { + this.asExpr().(MethodAccess).getMethod() instanceof CreateSslEngineMethod or + this.asExpr().(MethodAccess).getMethod() instanceof CreateSocketMethod + } +} + +/** + * A call to a method that establishes an SSL connection. + */ +class SslConnectionCreation extends DataFlow::Node { + SslConnectionCreation() { + exists(MethodAccess ma, Method m | + m instanceof GetSslSessionMethod or + m instanceof SocketConnectMethod + | + ma.getMethod() = m and + this.asExpr() = ma.getQualifier() + ) + or + // calls to SocketFactory.createSocket with parameters immediately create the connection + exists(MethodAccess ma, Method m | + ma.getMethod() = m and + m instanceof CreateSocket and + m.getNumberOfParameters() > 0 + | + this.asExpr() = ma + ) + } +} + +/** + * An SSL object that was assigned a safe `SSLParameters` object an can be considered safe. + */ +class SslConnectionWithSafeSslParameters extends Expr { + SslConnectionWithSafeSslParameters() { + exists(SafeSslParametersFlowConfig config, DataFlow::Node safe | + config.hasFlowTo(safe) and this = safe.asExpr().(Argument).getCall().getQualifier() + ) + } +} + +private class SafeSslParametersFlowConfig extends DataFlow2::Configuration { + SafeSslParametersFlowConfig() { this = "SafeSslParametersFlowConfig" } + + override predicate isSource(DataFlow::Node source) { + exists(MethodAccess ma | + ma instanceof SafeSetEndpointIdentificationAlgorithm and + ma.getQualifier() = source.asExpr() + ) + } + + override predicate isSink(DataFlow::Node sink) { + exists(MethodAccess ma, RefType t | t instanceof SSLSocket or t instanceof SSLEngine | + ma.getMethod().hasName("setSSLParameters") and + ma.getMethod().getDeclaringType().getASupertype*() = t and + ma.getArgument(0) = sink.asExpr() + ) + } +} + +private class SafeSetEndpointIdentificationAlgorithm extends MethodAccess { + SafeSetEndpointIdentificationAlgorithm() { + this.getMethod().hasName("setEndpointIdentificationAlgorithm") and + this.getMethod().getDeclaringType() instanceof SSLParameters and + not this.getArgument(0) instanceof NullLiteral and + not this.getArgument(0).(CompileTimeConstantExpr).getStringValue().length() = 0 + } +} + +/** + * A call to the method `useSslProtocol` on an instance of `com.rabbitmq.client.ConnectionFactory` + * that doesn't have `enableHostnameVerification` set. + */ +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() + ) + ) + } +} + +private class RabbitMQConnectionFactory extends RefType { + RabbitMQConnectionFactory() { this.hasQualifiedName("com.rabbitmq.client", "ConnectionFactory") } +} diff --git a/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql b/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql index a86505606ca0..b576e24d3c20 100644 --- a/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql +++ b/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql @@ -12,158 +12,25 @@ */ import java -import semmle.code.java.security.Encryption +import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.security.UnsafeCertTrust -class SSLEngine extends RefType { - SSLEngine() { this.hasQualifiedName("javax.net.ssl", "SSLEngine") } -} - -class Socket extends RefType { - Socket() { this.hasQualifiedName("java.net", "Socket") } -} +class SslEndpointIdentificationFlowConfig extends TaintTracking::Configuration { + SslEndpointIdentificationFlowConfig() { this = "SslEndpointIdentificationFlowConfig" } -class SocketFactory extends RefType { - SocketFactory() { this.hasQualifiedName("javax.net", "SocketFactory") } -} + override predicate isSource(DataFlow::Node source) { source instanceof SslConnectionInit } -class SSLSocket extends RefType { - SSLSocket() { this.hasQualifiedName("javax.net.ssl", "SSLSocket") } -} + override predicate isSink(DataFlow::Node sink) { sink instanceof SslConnectionCreation } -/** - * 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() - ) - ) + override predicate isSanitizer(DataFlow::Node sanitizer) { + sanitizer.asExpr() instanceof SslConnectionWithSafeSslParameters } } -from MethodAccess aa +from Expr unsafeConfig where - aa instanceof SSLEndpointIdentificationNotSet or - aa instanceof RabbitMQEnableHostnameVerificationNotSet -select aa, "Unsafe configuration of trusted certificates" + unsafeConfig instanceof RabbitMQEnableHostnameVerificationNotSet or + exists(SslEndpointIdentificationFlowConfig config | + config.hasFlowTo(DataFlow::exprNode(unsafeConfig)) + ) +select unsafeConfig, "Unsafe configuration of trusted certificates" diff --git a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java index cb8b472eb8fd..aae253530451 100644 --- a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java +++ b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java @@ -1,20 +1,12 @@ -import javax.net.ssl.HostnameVerifier; -import javax.net.ssl.HttpsURLConnection; -import javax.net.ssl.SSLSession; -import javax.net.ssl.SSLSocket; +import java.net.InetSocketAddress; +import java.net.Socket; +import javax.net.SocketFactory; import javax.net.ssl.SSLContext; import javax.net.ssl.SSLEngine; import javax.net.ssl.SSLParameters; +import javax.net.ssl.SSLSocket; 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; +import com.rabbitmq.client.ConnectionFactory; public class UnsafeCertTrustTest { @@ -27,6 +19,7 @@ public void testSSLEngineEndpointIdSetNull() throws java.security.NoSuchAlgorith SSLParameters sslParameters = sslEngine.getSSLParameters(); sslParameters.setEndpointIdentificationAlgorithm(null); sslEngine.setSSLParameters(sslParameters); + sslEngine.getSession(); } /** @@ -35,15 +28,95 @@ public void testSSLEngineEndpointIdSetNull() throws java.security.NoSuchAlgorith public void testSSLEngineEndpointIdNotSet() throws java.security.NoSuchAlgorithmException { SSLContext sslContext = SSLContext.getInstance("TLS"); SSLEngine sslEngine = sslContext.createSSLEngine(); + sslEngine.getSession(); + } + + /** + * Test the endpoint identification of SSL engine is set to HTTPS + */ + public void testSSLEngineEndpointIdSafe() throws java.security.NoSuchAlgorithmException { + SSLContext sslContext = SSLContext.getInstance("TLS"); + SSLEngine sslEngine = sslContext.createSSLEngine(); + SSLParameters sslParameters = sslEngine.getSSLParameters(); + sslParameters.setEndpointIdentificationAlgorithm("HTTPS"); + sslEngine.setSSLParameters(sslParameters); + sslEngine.getSession(); + } + + /** + * Test the endpoint identification of SSL socket is not set + */ + public void testSSLSocketImmediatelyConnects() + throws java.security.NoSuchAlgorithmException, java.io.IOException { + SSLContext sslContext = SSLContext.getInstance("TLS"); + final SSLSocketFactory socketFactory = sslContext.getSocketFactory(); + SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443); + } + + /** + * Test the endpoint identification of SSL socket is not set + */ + public void testSSLSocketEndpointIdNotSet() + throws java.security.NoSuchAlgorithmException, java.io.IOException { + SSLContext sslContext = SSLContext.getInstance("TLS"); + final SSLSocketFactory socketFactory = sslContext.getSocketFactory(); + SSLSocket socket = (SSLSocket) socketFactory.createSocket(); + socket.connect(new InetSocketAddress("www.example.com", 443)); + } + + /** + * Test the endpoint identification of SSL socket is set to null + */ + public void testSSLSocketEndpointIdSetNull() + throws java.security.NoSuchAlgorithmException, java.io.IOException { + SSLContext sslContext = SSLContext.getInstance("TLS"); + final SSLSocketFactory socketFactory = sslContext.getSocketFactory(); + SSLSocket socket = (SSLSocket) socketFactory.createSocket(); + SSLParameters sslParameters = socket.getSSLParameters(); + sslParameters.setEndpointIdentificationAlgorithm(null); + socket.setSSLParameters(sslParameters); + socket.connect(new InetSocketAddress("www.example.com", 443)); + } + + /** + * Test the endpoint identification of SSL socket is set to empty + */ + public void testSSLSocketEndpointIdSetEmpty() + throws java.security.NoSuchAlgorithmException, java.io.IOException { + SSLContext sslContext = SSLContext.getInstance("TLS"); + final SSLSocketFactory socketFactory = sslContext.getSocketFactory(); + SSLSocket socket = (SSLSocket) socketFactory.createSocket(); + SSLParameters sslParameters = socket.getSSLParameters(); + sslParameters.setEndpointIdentificationAlgorithm(""); + socket.setSSLParameters(sslParameters); + socket.connect(new InetSocketAddress("www.example.com", 443)); } /** * Test the endpoint identification of SSL socket is not set */ - public void testSSLSocketEndpointIdNotSet() throws java.security.NoSuchAlgorithmException, java.io.IOException { + public void testSSLSocketEndpointIdAfterConnecting() + throws java.security.NoSuchAlgorithmException, java.io.IOException { SSLContext sslContext = SSLContext.getInstance("TLS"); final SSLSocketFactory socketFactory = sslContext.getSocketFactory(); SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443); + SSLParameters sslParameters = socket.getSSLParameters(); + sslParameters.setEndpointIdentificationAlgorithm("HTTPS"); + socket.setSSLParameters(sslParameters); + } + + /** + * Test the endpoint identification of SSL socket is not set + */ + public void testSSLSocketEndpointIdSafe() + throws java.security.NoSuchAlgorithmException, java.io.IOException { + SSLContext sslContext = SSLContext.getInstance("TLS"); + final SSLSocketFactory socketFactory = sslContext.getSocketFactory(); + SSLSocket socket = (SSLSocket) socketFactory.createSocket(); + SSLParameters sslParameters = socket.getSSLParameters(); + sslParameters.setEndpointIdentificationAlgorithm("HTTPS"); + socket.setSSLParameters(sslParameters); + socket.connect(new InetSocketAddress("www.example.com", 443)); } /** @@ -54,11 +127,20 @@ public void testSocketEndpointIdNotSet() throws java.io.IOException { 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(); - // } + /** + * Test the enableHostnameVerification of RabbitMQConnectionFactory is not set + */ + public void testRabbitMQFactoryEnableHostnameVerificationNotSet() throws Exception { + ConnectionFactory connectionFactory = new ConnectionFactory(); + connectionFactory.useSslProtocol(); + } + + /** + * Test the enableHostnameVerification of RabbitMQConnectionFactory is not set + */ + public void testRabbitMQFactorySafe() throws Exception { + ConnectionFactory connectionFactory = new ConnectionFactory(); + connectionFactory.useSslProtocol(); + connectionFactory.enableHostnameVerification(); + } } diff --git a/java/ql/test/query-tests/security/CWE-273/options b/java/ql/test/query-tests/security/CWE-273/options new file mode 100644 index 000000000000..a97d4b5eeeee --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-273/options @@ -0,0 +1 @@ +//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/amqp-client-5.12.0 \ No newline at end of file diff --git a/java/ql/test/stubs/amqp-client-5.12.0/com/rabbitmq/client/ConnectionFactory.java b/java/ql/test/stubs/amqp-client-5.12.0/com/rabbitmq/client/ConnectionFactory.java new file mode 100644 index 000000000000..f02c88893aa6 --- /dev/null +++ b/java/ql/test/stubs/amqp-client-5.12.0/com/rabbitmq/client/ConnectionFactory.java @@ -0,0 +1,223 @@ +// Copyright (c) 2007-2020 VMware, Inc. or its affiliates. All rights reserved. +// +// This software, the RabbitMQ Java client library, is triple-licensed under the +// Mozilla Public License 2.0 ("MPL"), the GNU General Public License version 2 +// ("GPL") and the Apache License version 2 ("ASL"). For the MPL, please see +// LICENSE-MPL-RabbitMQ. For the GPL, please see LICENSE-GPL2. For the ASL, +// please see LICENSE-APACHE2. +// +// This software is distributed on an "AS IS" basis, WITHOUT WARRANTY OF ANY KIND, +// either express or implied. See the LICENSE file for specific language governing +// rights and limitations of this software. +// +// If you have any questions regarding licensing, please contact us at +// info@rabbitmq.com. + +package com.rabbitmq.client; + +import javax.net.ssl.SSLContext; +import javax.net.ssl.TrustManager; +import java.io.IOException; +import java.net.URI; +import java.net.URISyntaxException; +import java.security.KeyManagementException; +import java.security.NoSuchAlgorithmException; +import java.util.*; +import java.util.concurrent.*; +import java.util.function.Predicate; +import static java.util.concurrent.TimeUnit.MINUTES; + +public class ConnectionFactory implements Cloneable { + public static final int DEFAULT_CHANNEL_RPC_TIMEOUT = (int) MINUTES.toMillis(10); + + public String getHost() { + return null; + } + + public void setHost(String host) {} + + public static int portOrDefault(int port, boolean ssl) { + return 0; + } + + public int getPort() { + return 0; + } + + public void setPort(int port) {} + + public String getUsername() { + return null; + } + + public void setUsername(String username) {} + + public String getPassword() { + return null; + } + + public void setPassword(String password) {} + + public String getVirtualHost() { + return null; + } + + public void setVirtualHost(String virtualHost) {} + + public void setUri(URI uri) + throws URISyntaxException, NoSuchAlgorithmException, KeyManagementException {} + + public void setUri(String uriString) + throws URISyntaxException, NoSuchAlgorithmException, KeyManagementException {} + + public int getRequestedChannelMax() { + return 0; + } + + public void setRequestedChannelMax(int requestedChannelMax) {} + + public int getRequestedFrameMax() { + return 0; + } + + public void setRequestedFrameMax(int requestedFrameMax) {} + + public int getRequestedHeartbeat() { + return 0; + } + + public void setConnectionTimeout(int timeout) {} + + public int getConnectionTimeout() { + return 0; + } + + public int getHandshakeTimeout() { + return 0; + } + + public void setHandshakeTimeout(int timeout) {} + + public void setShutdownTimeout(int shutdownTimeout) {} + + public int getShutdownTimeout() { + return 0; + } + + public void setRequestedHeartbeat(int requestedHeartbeat) {} + + public Map getClientProperties() { + return null; + } + + public void setClientProperties(Map clientProperties) {} + + public void setSharedExecutor(ExecutorService executor) {} + + public void setShutdownExecutor(ExecutorService executor) {} + + public void setHeartbeatExecutor(ScheduledExecutorService executor) {} + + public ThreadFactory getThreadFactory() { + return null; + } + + public void setThreadFactory(ThreadFactory threadFactory) {} + + public boolean isSSL() { + return false; + } + + public void useSslProtocol() throws NoSuchAlgorithmException, KeyManagementException {} + + public void useSslProtocol(String protocol) + throws NoSuchAlgorithmException, KeyManagementException {} + + public void useSslProtocol(String protocol, TrustManager trustManager) + throws NoSuchAlgorithmException, KeyManagementException {} + + public void useSslProtocol(SSLContext context) {} + + public void enableHostnameVerification() {} + + public static String computeDefaultTlsProtocol(String[] supportedProtocols) { + return null; + } + + public boolean isAutomaticRecoveryEnabled() { + return false; + } + + public void setAutomaticRecoveryEnabled(boolean automaticRecovery) {} + + public boolean isTopologyRecoveryEnabled() { + return false; + } + + public void setTopologyRecoveryEnabled(boolean topologyRecovery) {} + + public ExecutorService getTopologyRecoveryExecutor() { + return null; + } + + public void setTopologyRecoveryExecutor(final ExecutorService topologyRecoveryExecutor) {} + + public ConnectionFactory load(String propertyFileLocation) throws IOException { + return null; + } + + public ConnectionFactory load(String propertyFileLocation, String prefix) throws IOException { + return null; + } + + public ConnectionFactory load(Properties properties) { + return null; + } + + public ConnectionFactory load(Properties properties, String prefix) { + return null; + } + + public ConnectionFactory load(Map properties) { + return null; + } + + public ConnectionFactory load(Map properties, String prefix) { + return null; + } + + public long getNetworkRecoveryInterval() { + return 0; + } + + public void setNetworkRecoveryInterval(int networkRecoveryInterval) {} + + public void setNetworkRecoveryInterval(long networkRecoveryInterval) {} + + public void useNio() {} + + public void useBlockingIo() {} + + public void setChannelRpcTimeout(int channelRpcTimeout) {} + + public int getChannelRpcTimeout() { + return 0; + } + + public void setChannelShouldCheckRpcResponseType(boolean channelShouldCheckRpcResponseType) {} + + public boolean isChannelShouldCheckRpcResponseType() { + return false; + } + + public void setWorkPoolTimeout(int workPoolTimeout) {} + + public int getWorkPoolTimeout() { + return 0; + } + + public static int ensureUnsignedShort(int value) { + return 0; + } + +} From 02d0fa91889d5e34a451363dd2b790bc33aba5d6 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Tue, 22 Jun 2021 09:31:47 +0200 Subject: [PATCH 03/26] Minor changes in QLDocs and a sanitizer's type --- .../lib/semmle/code/java/security/UnsafeCertTrust.qll | 10 +++++++--- java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll b/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll index 4dfa16e20781..89232a1e0a92 100644 --- a/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll +++ b/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll @@ -8,6 +8,7 @@ private import semmle.code.java.dataflow.DataFlow2 /** * The creation of an object that prepares an SSL connection. + * This is a source for `SslEndpointIdentificationFlowConfig`. */ class SslConnectionInit extends DataFlow::Node { SslConnectionInit() { @@ -18,6 +19,7 @@ class SslConnectionInit extends DataFlow::Node { /** * A call to a method that establishes an SSL connection. + * This is a sink for `SslEndpointIdentificationFlowConfig`. */ class SslConnectionCreation extends DataFlow::Node { SslConnectionCreation() { @@ -41,12 +43,14 @@ class SslConnectionCreation extends DataFlow::Node { } /** - * An SSL object that was assigned a safe `SSLParameters` object an can be considered safe. + * An SSL object that was assigned a safe `SSLParameters` object and can be considered safe. + * This is a sanitizer for `SslEndpointIdentificationFlowConfig`. */ -class SslConnectionWithSafeSslParameters extends Expr { +class SslConnectionWithSafeSslParameters extends DataFlow::Node { SslConnectionWithSafeSslParameters() { exists(SafeSslParametersFlowConfig config, DataFlow::Node safe | - config.hasFlowTo(safe) and this = safe.asExpr().(Argument).getCall().getQualifier() + config.hasFlowTo(safe) and + this = DataFlow::exprNode(safe.asExpr().(Argument).getCall().getQualifier()) ) } } diff --git a/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql b/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql index b576e24d3c20..93de3cca8725 100644 --- a/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql +++ b/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql @@ -23,7 +23,7 @@ class SslEndpointIdentificationFlowConfig extends TaintTracking::Configuration { override predicate isSink(DataFlow::Node sink) { sink instanceof SslConnectionCreation } override predicate isSanitizer(DataFlow::Node sanitizer) { - sanitizer.asExpr() instanceof SslConnectionWithSafeSslParameters + sanitizer instanceof SslConnectionWithSafeSslParameters } } From e43fff2d3026c10778969e6c68e83610af8f08fd Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Tue, 22 Jun 2021 10:08:26 +0200 Subject: [PATCH 04/26] Use InlineExpectationsTest --- .../code/java/security/UnsafeCertTrust.qll | 21 ++++++++-- .../Security/CWE/CWE-273/UnsafeCertTrust.ql | 8 ++-- .../security/CWE-273/UnsafeCertTrust.expected | 3 -- .../security/CWE-273/UnsafeCertTrust.qlref | 1 - .../CWE-273/UnsafeCertTrustTest.expected | 0 .../security/CWE-273/UnsafeCertTrustTest.java | 36 +++++++++--------- .../security/CWE-273/UnsafeCertTrustTest.ql | 38 +++++++++++++++++++ 7 files changed, 78 insertions(+), 29 deletions(-) delete mode 100644 java/ql/test/query-tests/security/CWE-273/UnsafeCertTrust.expected delete mode 100644 java/ql/test/query-tests/security/CWE-273/UnsafeCertTrust.qlref create mode 100644 java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.expected create mode 100644 java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.ql diff --git a/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll b/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll index 89232a1e0a92..986472fa58be 100644 --- a/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll +++ b/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll @@ -34,8 +34,9 @@ class SslConnectionCreation extends DataFlow::Node { // calls to SocketFactory.createSocket with parameters immediately create the connection exists(MethodAccess ma, Method m | ma.getMethod() = m and - m instanceof CreateSocket and - m.getNumberOfParameters() > 0 + m instanceof CreateSocketMethod and + m.getNumberOfParameters() > 0 and + isSslSocket(ma) | this.asExpr() = ma ) @@ -55,6 +56,20 @@ class SslConnectionWithSafeSslParameters extends DataFlow::Node { } } +/** + * Holds if the return value of `createSocket` is cast to `SSLSocket` + * or the qualifier of `createSocket` is an instance of `SSLSocketFactory`. + */ +private predicate isSslSocket(MethodAccess createSocket) { + exists(Variable ssl, CastExpr ce | + ce.getExpr() = createSocket and + ce.getControlFlowNode().getASuccessor().(VariableAssign).getDestVar() = ssl and + ssl.getType() instanceof SSLSocket + ) + or + createSocket.getQualifier().getType().(RefType).getASupertype*() instanceof SSLSocketFactory +} + private class SafeSslParametersFlowConfig extends DataFlow2::Configuration { SafeSslParametersFlowConfig() { this = "SafeSslParametersFlowConfig" } @@ -85,7 +100,7 @@ private class SafeSetEndpointIdentificationAlgorithm extends MethodAccess { /** * A call to the method `useSslProtocol` on an instance of `com.rabbitmq.client.ConnectionFactory` - * that doesn't have `enableHostnameVerification` set. + * that doesn't set `enableHostnameVerification`. */ class RabbitMQEnableHostnameVerificationNotSet extends MethodAccess { RabbitMQEnableHostnameVerificationNotSet() { diff --git a/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql b/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql index 93de3cca8725..8ab77389d36d 100644 --- a/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql +++ b/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql @@ -27,10 +27,10 @@ class SslEndpointIdentificationFlowConfig extends TaintTracking::Configuration { } } -from Expr unsafeConfig +from Expr unsafeTrust where - unsafeConfig instanceof RabbitMQEnableHostnameVerificationNotSet or + unsafeTrust instanceof RabbitMQEnableHostnameVerificationNotSet or exists(SslEndpointIdentificationFlowConfig config | - config.hasFlowTo(DataFlow::exprNode(unsafeConfig)) + config.hasFlowTo(DataFlow::exprNode(unsafeTrust)) ) -select unsafeConfig, "Unsafe configuration of trusted certificates" +select unsafeTrust, "Unsafe configuration of trusted certificates" diff --git a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrust.expected b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrust.expected deleted file mode 100644 index f26706a56d2c..000000000000 --- a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrust.expected +++ /dev/null @@ -1,3 +0,0 @@ -| UnsafeCertTrustTest.java:26:25:26:52 | createSSLEngine(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:37:25:37:52 | createSSLEngine(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:46:34:46:83 | createSocket(...) | Unsafe configuration of trusted certificates | diff --git a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrust.qlref b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrust.qlref deleted file mode 100644 index e63adf79e5e3..000000000000 --- a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrust.qlref +++ /dev/null @@ -1 +0,0 @@ -Security/CWE/CWE-273/UnsafeCertTrust.ql diff --git a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.expected b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.expected new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java index aae253530451..6b65cda88682 100644 --- a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java +++ b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java @@ -19,7 +19,7 @@ public void testSSLEngineEndpointIdSetNull() throws java.security.NoSuchAlgorith SSLParameters sslParameters = sslEngine.getSSLParameters(); sslParameters.setEndpointIdentificationAlgorithm(null); sslEngine.setSSLParameters(sslParameters); - sslEngine.getSession(); + sslEngine.getSession(); // $hasUnsafeCertTrust } /** @@ -28,7 +28,7 @@ public void testSSLEngineEndpointIdSetNull() throws java.security.NoSuchAlgorith public void testSSLEngineEndpointIdNotSet() throws java.security.NoSuchAlgorithmException { SSLContext sslContext = SSLContext.getInstance("TLS"); SSLEngine sslEngine = sslContext.createSSLEngine(); - sslEngine.getSession(); + sslEngine.getSession(); // $hasUnsafeCertTrust } /** @@ -40,7 +40,7 @@ public void testSSLEngineEndpointIdSafe() throws java.security.NoSuchAlgorithmEx SSLParameters sslParameters = sslEngine.getSSLParameters(); sslParameters.setEndpointIdentificationAlgorithm("HTTPS"); sslEngine.setSSLParameters(sslParameters); - sslEngine.getSession(); + sslEngine.getSession(); // Safe } /** @@ -49,8 +49,8 @@ public void testSSLEngineEndpointIdSafe() throws java.security.NoSuchAlgorithmEx public void testSSLSocketImmediatelyConnects() throws java.security.NoSuchAlgorithmException, java.io.IOException { SSLContext sslContext = SSLContext.getInstance("TLS"); - final SSLSocketFactory socketFactory = sslContext.getSocketFactory(); - SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443); + SocketFactory socketFactory = sslContext.getSocketFactory(); + SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443); // $hasUnsafeCertTrust } /** @@ -59,9 +59,9 @@ public void testSSLSocketImmediatelyConnects() public void testSSLSocketEndpointIdNotSet() throws java.security.NoSuchAlgorithmException, java.io.IOException { SSLContext sslContext = SSLContext.getInstance("TLS"); - final SSLSocketFactory socketFactory = sslContext.getSocketFactory(); + SSLSocketFactory socketFactory = sslContext.getSocketFactory(); SSLSocket socket = (SSLSocket) socketFactory.createSocket(); - socket.connect(new InetSocketAddress("www.example.com", 443)); + socket.connect(new InetSocketAddress("www.example.com", 443)); // $hasUnsafeCertTrust } /** @@ -70,12 +70,12 @@ public void testSSLSocketEndpointIdNotSet() public void testSSLSocketEndpointIdSetNull() throws java.security.NoSuchAlgorithmException, java.io.IOException { SSLContext sslContext = SSLContext.getInstance("TLS"); - final SSLSocketFactory socketFactory = sslContext.getSocketFactory(); + SSLSocketFactory socketFactory = sslContext.getSocketFactory(); SSLSocket socket = (SSLSocket) socketFactory.createSocket(); SSLParameters sslParameters = socket.getSSLParameters(); sslParameters.setEndpointIdentificationAlgorithm(null); socket.setSSLParameters(sslParameters); - socket.connect(new InetSocketAddress("www.example.com", 443)); + socket.connect(new InetSocketAddress("www.example.com", 443)); // $hasUnsafeCertTrust } /** @@ -84,12 +84,12 @@ public void testSSLSocketEndpointIdSetNull() public void testSSLSocketEndpointIdSetEmpty() throws java.security.NoSuchAlgorithmException, java.io.IOException { SSLContext sslContext = SSLContext.getInstance("TLS"); - final SSLSocketFactory socketFactory = sslContext.getSocketFactory(); + SSLSocketFactory socketFactory = sslContext.getSocketFactory(); SSLSocket socket = (SSLSocket) socketFactory.createSocket(); SSLParameters sslParameters = socket.getSSLParameters(); sslParameters.setEndpointIdentificationAlgorithm(""); socket.setSSLParameters(sslParameters); - socket.connect(new InetSocketAddress("www.example.com", 443)); + socket.connect(new InetSocketAddress("www.example.com", 443)); // $hasUnsafeCertTrust } /** @@ -98,8 +98,8 @@ public void testSSLSocketEndpointIdSetEmpty() public void testSSLSocketEndpointIdAfterConnecting() throws java.security.NoSuchAlgorithmException, java.io.IOException { SSLContext sslContext = SSLContext.getInstance("TLS"); - final SSLSocketFactory socketFactory = sslContext.getSocketFactory(); - SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443); + SSLSocketFactory socketFactory = sslContext.getSocketFactory(); + SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443); // $hasUnsafeCertTrust SSLParameters sslParameters = socket.getSSLParameters(); sslParameters.setEndpointIdentificationAlgorithm("HTTPS"); socket.setSSLParameters(sslParameters); @@ -111,12 +111,12 @@ public void testSSLSocketEndpointIdAfterConnecting() public void testSSLSocketEndpointIdSafe() throws java.security.NoSuchAlgorithmException, java.io.IOException { SSLContext sslContext = SSLContext.getInstance("TLS"); - final SSLSocketFactory socketFactory = sslContext.getSocketFactory(); + SSLSocketFactory socketFactory = sslContext.getSocketFactory(); SSLSocket socket = (SSLSocket) socketFactory.createSocket(); SSLParameters sslParameters = socket.getSSLParameters(); sslParameters.setEndpointIdentificationAlgorithm("HTTPS"); socket.setSSLParameters(sslParameters); - socket.connect(new InetSocketAddress("www.example.com", 443)); + socket.connect(new InetSocketAddress("www.example.com", 443)); // Safe } /** @@ -124,7 +124,7 @@ public void testSSLSocketEndpointIdSafe() */ public void testSocketEndpointIdNotSet() throws java.io.IOException { SocketFactory socketFactory = SocketFactory.getDefault(); - Socket socket = socketFactory.createSocket("www.example.com", 80); + Socket socket = socketFactory.createSocket("www.example.com", 80); // Safe } /** @@ -132,7 +132,7 @@ public void testSocketEndpointIdNotSet() throws java.io.IOException { */ public void testRabbitMQFactoryEnableHostnameVerificationNotSet() throws Exception { ConnectionFactory connectionFactory = new ConnectionFactory(); - connectionFactory.useSslProtocol(); + connectionFactory.useSslProtocol(); // $hasUnsafeCertTrust } /** @@ -140,7 +140,7 @@ public void testRabbitMQFactoryEnableHostnameVerificationNotSet() throws Excepti */ public void testRabbitMQFactorySafe() throws Exception { ConnectionFactory connectionFactory = new ConnectionFactory(); - connectionFactory.useSslProtocol(); + connectionFactory.useSslProtocol(); // Safe connectionFactory.enableHostnameVerification(); } } diff --git a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.ql b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.ql new file mode 100644 index 000000000000..7abcbd0fd354 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.ql @@ -0,0 +1,38 @@ +import java +import semmle.code.java.dataflow.FlowSources +import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.security.UnsafeCertTrust +import TestUtilities.InlineExpectationsTest + +class Conf extends TaintTracking::Configuration { + Conf() { this = "qltest:cwe:unsafe-cert-trust" } + + override predicate isSource(DataFlow::Node source) { source instanceof SslConnectionInit } + + override predicate isSink(DataFlow::Node sink) { sink instanceof SslConnectionCreation } + + override predicate isSanitizer(DataFlow::Node sanitizer) { + sanitizer instanceof SslConnectionWithSafeSslParameters + } +} + +class UnsafeCertTrustTest extends InlineExpectationsTest { + UnsafeCertTrustTest() { this = "HasUnsafeCertTrustTest" } + + override string getARelevantTag() { result = "hasUnsafeCertTrust" } + + override predicate hasActualResult(Location location, string element, string tag, string value) { + tag = "hasUnsafeCertTrust" and + exists(Expr unsafeTrust | + unsafeTrust instanceof X509TrustAllManagerInit + or + unsafeTrust instanceof RabbitMQEnableHostnameVerificationNotSet + or + exists(Conf config | config.hasFlowTo(DataFlow::exprNode(unsafeTrust))) + | + unsafeTrust.getLocation() = location and + element = unsafeTrust.toString() and + value = "" + ) + } +} From 5d4cd70f8cdc9877abb0cac828f2b16a74179e62 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Tue, 22 Jun 2021 17:53:15 +0200 Subject: [PATCH 05/26] Adjusted sources and sanitizer of UnsafeCertTrust taint tracking config --- .../semmle/code/java/security/Encryption.qll | 24 ++ .../code/java/security/UnsafeCertTrust.qll | 31 +- .../Security/CWE/CWE-273/UnsafeCertTrust.ql | 2 +- .../semmle/code/java/security/Encryption.qll | 373 ++++++++++++++++++ .../security/CWE-273/UnsafeCertTrustTest.java | 56 ++- .../security/CWE-273/UnsafeCertTrustTest.ql | 2 +- 6 files changed, 463 insertions(+), 25 deletions(-) create mode 100644 java/ql/src/semmle/code/java/security/Encryption.qll diff --git a/java/ql/lib/semmle/code/java/security/Encryption.qll b/java/ql/lib/semmle/code/java/security/Encryption.qll index 3ae5fdf3d9d2..57c91d9cd55b 100644 --- a/java/ql/lib/semmle/code/java/security/Encryption.qll +++ b/java/ql/lib/semmle/code/java/security/Encryption.qll @@ -122,6 +122,30 @@ class SetDefaultHostnameVerifierMethod extends Method { } } +/** The `beginHandshake` method of the class `javax.net.ssl.SSLEngine`. */ +class BeginHandshakeMethod extends Method { + BeginHandshakeMethod() { + this.hasName("beginHandshake") and + this.getDeclaringType().getASupertype*() instanceof SSLEngine + } +} + +/** The `wrap` method of the class `javax.net.ssl.SSLEngine`. */ +class SslWrapMethod extends Method { + SslWrapMethod() { + this.hasName("wrap") and + this.getDeclaringType().getASupertype*() instanceof SSLEngine + } +} + +/** The `unwrap` method of the class `javax.net.ssl.SSLEngine`. */ +class SslUnwrapMethod extends Method { + SslUnwrapMethod() { + this.hasName("unwrap") and + this.getDeclaringType().getASupertype*() instanceof SSLEngine + } +} + /** The `getSession` method of the class `javax.net.ssl.SSLSession`.select */ class GetSslSessionMethod extends Method { GetSslSessionMethod() { diff --git a/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll b/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll index 986472fa58be..64efc5130af9 100644 --- a/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll +++ b/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll @@ -8,6 +8,7 @@ private import semmle.code.java.dataflow.DataFlow2 /** * The creation of an object that prepares an SSL connection. + * * This is a source for `SslEndpointIdentificationFlowConfig`. */ class SslConnectionInit extends DataFlow::Node { @@ -19,12 +20,15 @@ class SslConnectionInit extends DataFlow::Node { /** * A call to a method that establishes an SSL connection. + * * This is a sink for `SslEndpointIdentificationFlowConfig`. */ class SslConnectionCreation extends DataFlow::Node { SslConnectionCreation() { exists(MethodAccess ma, Method m | - m instanceof GetSslSessionMethod or + m instanceof BeginHandshakeMethod or + m instanceof SslWrapMethod or + m instanceof SslUnwrapMethod or m instanceof SocketConnectMethod | ma.getMethod() = m and @@ -44,10 +48,16 @@ class SslConnectionCreation extends DataFlow::Node { } /** - * An SSL object that was assigned a safe `SSLParameters` object and can be considered safe. + * An SSL object that correctly verifies hostnames, or doesn't need to (because e.g. it's a server). + * * This is a sanitizer for `SslEndpointIdentificationFlowConfig`. */ -class SslConnectionWithSafeSslParameters extends DataFlow::Node { +abstract class SslUnsafeCertTrustSanitizer extends DataFlow::Node { } + +/** + * An SSL object that was assigned a safe `SSLParameters` object and can be considered safe. + */ +private class SslConnectionWithSafeSslParameters extends SslUnsafeCertTrustSanitizer { SslConnectionWithSafeSslParameters() { exists(SafeSslParametersFlowConfig config, DataFlow::Node safe | config.hasFlowTo(safe) and @@ -56,6 +66,21 @@ class SslConnectionWithSafeSslParameters extends DataFlow::Node { } } +/** + * An `SSLEngine` set in server mode. + */ +private class SslEngineServerMode extends SslUnsafeCertTrustSanitizer { + SslEngineServerMode() { + exists(MethodAccess ma, Method m | + m.hasName("setUseClientMode") and + m.getDeclaringType().getASupertype*() instanceof SSLEngine and + ma.getMethod() = m and + ma.getArgument(0).(CompileTimeConstantExpr).getBooleanValue() = false and + this = DataFlow::exprNode(ma.getQualifier()) + ) + } +} + /** * Holds if the return value of `createSocket` is cast to `SSLSocket` * or the qualifier of `createSocket` is an instance of `SSLSocketFactory`. diff --git a/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql b/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql index 8ab77389d36d..2893329a4c33 100644 --- a/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql +++ b/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql @@ -23,7 +23,7 @@ class SslEndpointIdentificationFlowConfig extends TaintTracking::Configuration { override predicate isSink(DataFlow::Node sink) { sink instanceof SslConnectionCreation } override predicate isSanitizer(DataFlow::Node sanitizer) { - sanitizer instanceof SslConnectionWithSafeSslParameters + sanitizer instanceof SslUnsafeCertTrustSanitizer } } diff --git a/java/ql/src/semmle/code/java/security/Encryption.qll b/java/ql/src/semmle/code/java/security/Encryption.qll new file mode 100644 index 000000000000..05720974dea5 --- /dev/null +++ b/java/ql/src/semmle/code/java/security/Encryption.qll @@ -0,0 +1,373 @@ +/** + * Provides predicates and classes relating to encryption in Java. + */ + +import java + +class SSLClass extends RefType { + SSLClass() { + exists(Class c | this.getASupertype*() = c | + c.hasQualifiedName("javax.net.ssl", _) or + c.hasQualifiedName("javax.rmi.ssl", _) + ) + } +} + +class X509TrustManager extends RefType { + X509TrustManager() { this.hasQualifiedName("javax.net.ssl", "X509TrustManager") } +} + +class HttpsURLConnection extends RefType { + HttpsURLConnection() { hasQualifiedName("javax.net.ssl", "HttpsURLConnection") } +} + +class SSLSocketFactory extends RefType { + SSLSocketFactory() { this.hasQualifiedName("javax.net.ssl", "SSLSocketFactory") } +} + +class SSLContext extends RefType { + SSLContext() { hasQualifiedName("javax.net.ssl", "SSLContext") } +} + +/** The `javax.net.ssl.SSLSession` class. */ +class SSLSession extends RefType { + SSLSession() { hasQualifiedName("javax.net.ssl", "SSLSession") } +} + +class SSLEngine extends RefType { + SSLEngine() { this.hasQualifiedName("javax.net.ssl", "SSLEngine") } +} + +class SSLSocket extends RefType { + SSLSocket() { this.hasQualifiedName("javax.net.ssl", "SSLSocket") } +} + +/** The `javax.net.ssl.SSLParameters` class. */ +class SSLParameters extends RefType { + SSLParameters() { this.hasQualifiedName("javax.net.ssl", "SSLParameters") } +} + +class HostnameVerifier extends RefType { + HostnameVerifier() { hasQualifiedName("javax.net.ssl", "HostnameVerifier") } +} + +/** The Java class `javax.crypto.KeyGenerator`. */ +class KeyGenerator extends RefType { + KeyGenerator() { this.hasQualifiedName("javax.crypto", "KeyGenerator") } +} + +/** The Java class `java.security.KeyPairGenerator`. */ +class KeyPairGenerator extends RefType { + KeyPairGenerator() { this.hasQualifiedName("java.security", "KeyPairGenerator") } +} + +/** The `verify` method of the class `javax.net.ssl.HostnameVerifier`. */ +class HostnameVerifierVerify extends Method { + HostnameVerifierVerify() { + hasName("verify") and + getDeclaringType().getASupertype*() instanceof HostnameVerifier and + getParameterType(0) instanceof TypeString and + getParameterType(1) instanceof SSLSession + } +} + +class TrustManagerCheckMethod extends Method { + TrustManagerCheckMethod() { + (this.hasName("checkClientTrusted") or this.hasName("checkServerTrusted")) and + this.getDeclaringType().getASupertype*() instanceof X509TrustManager + } +} + +class CreateSocket extends Method { + CreateSocket() { + hasName("createSocket") and + getDeclaringType() instanceof SSLSocketFactory + } +} + +class GetSocketFactory extends Method { + GetSocketFactory() { + hasName("getSocketFactory") and + getDeclaringType() instanceof SSLContext + } +} + +/** The `createSSLEngine` method of the class `javax.net.ssl.SSLContext` */ +class CreateSslEngineMethod extends Method { + CreateSslEngineMethod() { + this.hasName("createSSLEngine") and + this.getDeclaringType() instanceof SSLContext + } +} + +class SetConnectionFactoryMethod extends Method { + SetConnectionFactoryMethod() { + hasName("setSSLSocketFactory") and + getDeclaringType().getASupertype*() instanceof HttpsURLConnection + } +} + +class SetHostnameVerifierMethod extends Method { + SetHostnameVerifierMethod() { + hasName("setHostnameVerifier") and + getDeclaringType().getASupertype*() instanceof HttpsURLConnection + } +} + +/** The `setDefaultHostnameVerifier` method of the class `javax.net.ssl.HttpsURLConnection`. */ +class SetDefaultHostnameVerifierMethod extends Method { + SetDefaultHostnameVerifierMethod() { + hasName("setDefaultHostnameVerifier") and + getDeclaringType().getASupertype*() instanceof HttpsURLConnection + } +} + +/** The `beginHandshake` method of the class `javax.net.ssl.SSLEngine`. */ +class BeginHandshakeMethod extends Method { + BeginHandshakeMethod() { + hasName("beginHandshake") and + getDeclaringType().getASupertype*() instanceof SSLEngine + } +} + +/** The `wrap` method of the class `javax.net.ssl.SSLEngine`. */ +class SslWrapMethod extends Method { + SslWrapMethod() { + hasName("wrap") and + getDeclaringType().getASupertype*() instanceof SSLEngine + } +} + +/** The `unwrap` method of the class `javax.net.ssl.SSLEngine`. */ +class SslUnwrapMethod extends Method { + SslUnwrapMethod() { + hasName("unwrap") and + getDeclaringType().getASupertype*() instanceof SSLEngine + } +} + +bindingset[algorithmString] +private string algorithmRegex(string algorithmString) { + // Algorithms usually appear in names surrounded by characters that are not + // alphabetical characters in the same case. This handles the upper and lower + // case cases. + result = + "((^|.*[^A-Z])(" + algorithmString + ")([^A-Z].*|$))" + + // or... + "|" + + // For lowercase, we want to be careful to avoid being confused by camelCase + // hence we require two preceding uppercase letters to be sure of a case switch, + // or a preceding non-alphabetic character + "((^|.*[A-Z]{2}|.*[^a-zA-Z])(" + algorithmString.toLowerCase() + ")([^a-z].*|$))" +} + +/** + * Gets the name of an algorithm that is known to be insecure. + */ +string getAnInsecureAlgorithmName() { + result = + [ + "DES", "RC2", "RC4", "RC5", + // ARCFOUR is a variant of RC4 + "ARCFOUR", + // Encryption mode ECB like AES/ECB/NoPadding is vulnerable to replay and other attacks + "ECB", + // CBC mode of operation with PKCS#5 or PKCS#7 padding is vulnerable to padding oracle attacks + "AES/CBC/PKCS[57]Padding" + ] +} + +/** + * Gets the name of a hash algorithm that is insecure if it is being used for + * encryption. + */ +string getAnInsecureHashAlgorithmName() { + result = "SHA1" or + result = "MD5" +} + +private string rankedInsecureAlgorithm(int i) { + // In this case we know these are being used for encryption, so we want to match + // weak hash algorithms too. + result = + rank[i](string s | s = getAnInsecureAlgorithmName() or s = getAnInsecureHashAlgorithmName()) +} + +private string insecureAlgorithmString(int i) { + i = 1 and result = rankedInsecureAlgorithm(i) + or + result = rankedInsecureAlgorithm(i) + "|" + insecureAlgorithmString(i - 1) +} + +/** + * Gets the regular expression used for matching strings that look like they + * contain an algorithm that is known to be insecure. + */ +string getInsecureAlgorithmRegex() { + result = algorithmRegex(insecureAlgorithmString(max(int i | exists(rankedInsecureAlgorithm(i))))) +} + +/** + * Gets the name of an algorithm that is known to be secure. + */ +string getASecureAlgorithmName() { + result = + [ + "RSA", "SHA256", "SHA512", "CCM", "GCM", "AES([^a-zA-Z](?!ECB|CBC/PKCS[57]Padding)).*", + "Blowfish", "ECIES" + ] +} + +private string rankedSecureAlgorithm(int i) { result = rank[i](getASecureAlgorithmName()) } + +private string secureAlgorithmString(int i) { + i = 1 and result = rankedSecureAlgorithm(i) + or + result = rankedSecureAlgorithm(i) + "|" + secureAlgorithmString(i - 1) +} + +/** + * Gets a regular expression for matching strings that look like they + * contain an algorithm that is known to be secure. + */ +string getSecureAlgorithmRegex() { + result = algorithmRegex(secureAlgorithmString(max(int i | exists(rankedSecureAlgorithm(i))))) +} + +/** + * DEPRECATED: Terminology has been updated. Use `getAnInsecureAlgorithmName()` + * instead. + */ +deprecated string algorithmBlacklist() { result = getAnInsecureAlgorithmName() } + +/** + * DEPRECATED: Terminology has been updated. Use + * `getAnInsecureHashAlgorithmName()` instead. + */ +deprecated string hashAlgorithmBlacklist() { result = getAnInsecureHashAlgorithmName() } + +/** + * DEPRECATED: Terminology has been updated. Use `getInsecureAlgorithmRegex()` instead. + */ +deprecated string algorithmBlacklistRegex() { result = getInsecureAlgorithmRegex() } + +/** + * DEPRECATED: Terminology has been updated. Use `getASecureAlgorithmName()` + * instead. + */ +deprecated string algorithmWhitelist() { result = getASecureAlgorithmName() } + +/** + * DEPRECATED: Terminology has been updated. Use `getSecureAlgorithmRegex()` instead. + */ +deprecated string algorithmWhitelistRegex() { result = getSecureAlgorithmRegex() } + +/** + * Any use of a cryptographic element that specifies an encryption + * algorithm. For example, methods returning ciphers, decryption methods, + * constructors of cipher classes, etc. + */ +abstract class CryptoAlgoSpec extends Top { + CryptoAlgoSpec() { this instanceof Call } + + abstract Expr getAlgoSpec(); +} + +abstract class JavaxCryptoAlgoSpec extends CryptoAlgoSpec { } + +class JavaxCryptoCipher extends JavaxCryptoAlgoSpec { + JavaxCryptoCipher() { + exists(Method m | m.getAReference() = this | + m.getDeclaringType().getQualifiedName() = "javax.crypto.Cipher" and + m.getName() = "getInstance" + ) + } + + override Expr getAlgoSpec() { result = this.(MethodAccess).getArgument(0) } +} + +class JavaxCryptoSecretKey extends JavaxCryptoAlgoSpec { + JavaxCryptoSecretKey() { + exists(Constructor c | c.getAReference() = this | + c.getDeclaringType().getQualifiedName() = "javax.crypto.spec.SecretKeySpec" + ) + } + + override Expr getAlgoSpec() { + exists(ConstructorCall c | c = this | + if c.getNumArgument() = 2 then result = c.getArgument(1) else result = c.getArgument(3) + ) + } +} + +class JavaxCryptoKeyGenerator extends JavaxCryptoAlgoSpec { + JavaxCryptoKeyGenerator() { + exists(Method m | m.getAReference() = this | + m.getDeclaringType() instanceof KeyGenerator and + m.getName() = "getInstance" + ) + } + + override Expr getAlgoSpec() { result = this.(MethodAccess).getArgument(0) } +} + +class JavaxCryptoKeyAgreement extends JavaxCryptoAlgoSpec { + JavaxCryptoKeyAgreement() { + exists(Method m | m.getAReference() = this | + m.getDeclaringType().getQualifiedName() = "javax.crypto.KeyAgreement" and + m.getName() = "getInstance" + ) + } + + override Expr getAlgoSpec() { result = this.(MethodAccess).getArgument(0) } +} + +class JavaxCryptoKeyFactory extends JavaxCryptoAlgoSpec { + JavaxCryptoKeyFactory() { + exists(Method m | m.getAReference() = this | + m.getDeclaringType().getQualifiedName() = "javax.crypto.SecretKeyFactory" and + m.getName() = "getInstance" + ) + } + + override Expr getAlgoSpec() { result = this.(MethodAccess).getArgument(0) } +} + +abstract class JavaSecurityAlgoSpec extends CryptoAlgoSpec { } + +class JavaSecurityMessageDigest extends JavaSecurityAlgoSpec { + JavaSecurityMessageDigest() { + exists(Constructor c | c.getAReference() = this | + c.getDeclaringType().hasQualifiedName("java.security", "MessageDigest") + ) + or + exists(Method m | m.getAReference() = this | + m.getDeclaringType().hasQualifiedName("java.security", "MessageDigest") and + m.getName() = "getInstance" + ) + } + + override Expr getAlgoSpec() { result = this.(Call).getArgument(0) } +} + +class JavaSecuritySignature extends JavaSecurityAlgoSpec { + JavaSecuritySignature() { + exists(Constructor c | c.getAReference() = this | + c.getDeclaringType().getQualifiedName() = "java.security.Signature" + ) + } + + override Expr getAlgoSpec() { result = this.(ConstructorCall).getArgument(0) } +} + +/** A method call to the Java class `java.security.KeyPairGenerator`. */ +class JavaSecurityKeyPairGenerator extends JavaxCryptoAlgoSpec { + JavaSecurityKeyPairGenerator() { + exists(Method m | m.getAReference() = this | + m.getDeclaringType() instanceof KeyPairGenerator and + m.getName() = "getInstance" + ) + } + + override Expr getAlgoSpec() { result = this.(MethodAccess).getArgument(0) } +} diff --git a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java index 6b65cda88682..181ce1d9a4f3 100644 --- a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java +++ b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java @@ -1,5 +1,6 @@ import java.net.InetSocketAddress; import java.net.Socket; +import java.nio.ByteBuffer; import javax.net.SocketFactory; import javax.net.ssl.SSLContext; import javax.net.ssl.SSLEngine; @@ -13,41 +14,61 @@ public class UnsafeCertTrustTest { /** * Test the endpoint identification of SSL engine is set to null */ - public void testSSLEngineEndpointIdSetNull() throws java.security.NoSuchAlgorithmException { + public void testSSLEngineEndpointIdSetNull() throws Exception { SSLContext sslContext = SSLContext.getInstance("TLS"); SSLEngine sslEngine = sslContext.createSSLEngine(); SSLParameters sslParameters = sslEngine.getSSLParameters(); sslParameters.setEndpointIdentificationAlgorithm(null); sslEngine.setSSLParameters(sslParameters); - sslEngine.getSession(); // $hasUnsafeCertTrust + sslEngine.beginHandshake(); // $hasUnsafeCertTrust + sslEngine.wrap(new ByteBuffer[] {}, null); // $hasUnsafeCertTrust + sslEngine.unwrap(null, null, 0, 0); // $hasUnsafeCertTrust } /** - * Test the endpoint identification of SSL engine is not set + * Test the endpoint identification of SSL engine is set to null */ - public void testSSLEngineEndpointIdNotSet() throws java.security.NoSuchAlgorithmException { + public void testSSLEngineEndpointIdSetEmpty() throws Exception { SSLContext sslContext = SSLContext.getInstance("TLS"); SSLEngine sslEngine = sslContext.createSSLEngine(); - sslEngine.getSession(); // $hasUnsafeCertTrust + SSLParameters sslParameters = sslEngine.getSSLParameters(); + sslParameters.setEndpointIdentificationAlgorithm(""); + sslEngine.setSSLParameters(sslParameters); + sslEngine.beginHandshake(); // $hasUnsafeCertTrust + sslEngine.wrap(new ByteBuffer[] {}, null); // $hasUnsafeCertTrust + sslEngine.unwrap(null, null, 0, 0); // $hasUnsafeCertTrust } /** * Test the endpoint identification of SSL engine is set to HTTPS */ - public void testSSLEngineEndpointIdSafe() throws java.security.NoSuchAlgorithmException { + public void testSSLEngineEndpointIdSafe() throws Exception { SSLContext sslContext = SSLContext.getInstance("TLS"); SSLEngine sslEngine = sslContext.createSSLEngine(); SSLParameters sslParameters = sslEngine.getSSLParameters(); sslParameters.setEndpointIdentificationAlgorithm("HTTPS"); sslEngine.setSSLParameters(sslParameters); - sslEngine.getSession(); // Safe + sslEngine.beginHandshake(); // Safe + sslEngine.wrap(new ByteBuffer[] {}, null); // Safe + sslEngine.unwrap(null, null, 0, 0); // Safe + } + + /** + * Test the endpoint identification of SSL engine is set to HTTPS + */ + public void testSSLEngineInServerMode() throws Exception { + SSLContext sslContext = SSLContext.getInstance("TLS"); + SSLEngine sslEngine = sslContext.createSSLEngine(); + sslEngine.setUseClientMode(false); + sslEngine.beginHandshake(); // Safe + sslEngine.wrap(new ByteBuffer[] {}, null); // Safe + sslEngine.unwrap(null, null, 0, 0); // Safe } /** * Test the endpoint identification of SSL socket is not set */ - public void testSSLSocketImmediatelyConnects() - throws java.security.NoSuchAlgorithmException, java.io.IOException { + public void testSSLSocketImmediatelyConnects() throws Exception { SSLContext sslContext = SSLContext.getInstance("TLS"); SocketFactory socketFactory = sslContext.getSocketFactory(); SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443); // $hasUnsafeCertTrust @@ -56,8 +77,7 @@ public void testSSLSocketImmediatelyConnects() /** * Test the endpoint identification of SSL socket is not set */ - public void testSSLSocketEndpointIdNotSet() - throws java.security.NoSuchAlgorithmException, java.io.IOException { + public void testSSLSocketEndpointIdNotSet() throws Exception { SSLContext sslContext = SSLContext.getInstance("TLS"); SSLSocketFactory socketFactory = sslContext.getSocketFactory(); SSLSocket socket = (SSLSocket) socketFactory.createSocket(); @@ -67,8 +87,7 @@ public void testSSLSocketEndpointIdNotSet() /** * Test the endpoint identification of SSL socket is set to null */ - public void testSSLSocketEndpointIdSetNull() - throws java.security.NoSuchAlgorithmException, java.io.IOException { + public void testSSLSocketEndpointIdSetNull() throws Exception { SSLContext sslContext = SSLContext.getInstance("TLS"); SSLSocketFactory socketFactory = sslContext.getSocketFactory(); SSLSocket socket = (SSLSocket) socketFactory.createSocket(); @@ -81,8 +100,7 @@ public void testSSLSocketEndpointIdSetNull() /** * Test the endpoint identification of SSL socket is set to empty */ - public void testSSLSocketEndpointIdSetEmpty() - throws java.security.NoSuchAlgorithmException, java.io.IOException { + public void testSSLSocketEndpointIdSetEmpty() throws Exception { SSLContext sslContext = SSLContext.getInstance("TLS"); SSLSocketFactory socketFactory = sslContext.getSocketFactory(); SSLSocket socket = (SSLSocket) socketFactory.createSocket(); @@ -95,8 +113,7 @@ public void testSSLSocketEndpointIdSetEmpty() /** * Test the endpoint identification of SSL socket is not set */ - public void testSSLSocketEndpointIdAfterConnecting() - throws java.security.NoSuchAlgorithmException, java.io.IOException { + public void testSSLSocketEndpointIdAfterConnecting() throws Exception { SSLContext sslContext = SSLContext.getInstance("TLS"); SSLSocketFactory socketFactory = sslContext.getSocketFactory(); SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443); // $hasUnsafeCertTrust @@ -108,8 +125,7 @@ public void testSSLSocketEndpointIdAfterConnecting() /** * Test the endpoint identification of SSL socket is not set */ - public void testSSLSocketEndpointIdSafe() - throws java.security.NoSuchAlgorithmException, java.io.IOException { + public void testSSLSocketEndpointIdSafe() throws Exception { SSLContext sslContext = SSLContext.getInstance("TLS"); SSLSocketFactory socketFactory = sslContext.getSocketFactory(); SSLSocket socket = (SSLSocket) socketFactory.createSocket(); @@ -122,7 +138,7 @@ public void testSSLSocketEndpointIdSafe() /** * Test the endpoint identification of regular socket is not set */ - public void testSocketEndpointIdNotSet() throws java.io.IOException { + public void testSocketEndpointIdNotSet() throws Exception { SocketFactory socketFactory = SocketFactory.getDefault(); Socket socket = socketFactory.createSocket("www.example.com", 80); // Safe } diff --git a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.ql b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.ql index 7abcbd0fd354..66a76175c65d 100644 --- a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.ql +++ b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.ql @@ -12,7 +12,7 @@ class Conf extends TaintTracking::Configuration { override predicate isSink(DataFlow::Node sink) { sink instanceof SslConnectionCreation } override predicate isSanitizer(DataFlow::Node sanitizer) { - sanitizer instanceof SslConnectionWithSafeSslParameters + sanitizer instanceof SslUnsafeCertTrustSanitizer } } From e842acf9e0da32247eb8cb5c3497f634af9207f1 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 23 Jun 2021 13:01:48 +0200 Subject: [PATCH 06/26] Improve qhelp --- .../src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp b/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp index ae8d76b1bb1f..c317b9478de6 100644 --- a/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp +++ b/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp @@ -4,26 +4,23 @@ -

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

-

Unsafe implementation of the interface X509TrustManager and SSLSocket/SSLEngine ignores all SSL certificate validation errors when establishing an HTTPS connection, thereby making the app vulnerable to man-in-the-middle attacks.

-

This query checks whether setEndpointIdentificationAlgorithm is missing. The query also covers a special implementation com.rabbitmq.client.ConnectionFactory.

+

Java offers two mechanisms for SSL authentication - trust manager and hostname verifier (the later is checked by the java/insecure-hostname-verifier query). Trust manager validates the peer's certificate chain while hostname verification establishes that the hostname in the URL matches the hostname in the server's identification.

+

When SSLSocket or SSLEngine are created without a secure setEndpointIdentificationAlgorithm, hostname verification is disabled by default.

+

This query checks whether a trust manager is set to trust all certificates or setEndpointIdentificationAlgorithm is missing, thereby making the application vulnerable to man-in-the-middle attacks. The query also covers insecure configurations of com.rabbitmq.client.ConnectionFactory.

 -

Validate SSL certificate in SSL authentication.

+

Validate SSL certificates in SSL authentication.

The following two examples show two ways of configuring SSLSocket/SSLEngine. In the 'BAD' case, -setEndpointIdentificationAlgorithm is not called, thus no hostname verification takes place. In the 'GOOD' case, setEndpointIdentificationAlgorithm is called.

+setEndpointIdentificationAlgorithm is not called, thus no hostname verification takes place. In the 'GOOD' case, setEndpointIdentificationAlgorithm is called.
  • -CWE-273 -
    • -
  • Testing Endpoint Identify Verification (MSTG-NETWORK-3)
  • From 4508945f85371bde2e31aee70d5fe741112cf1ae Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 23 Jun 2021 13:02:57 +0200 Subject: [PATCH 07/26] Fix assumption regarding when an SSLSocket does the TLS handhsake --- .../code/java/frameworks/Networking.qll | 10 +++- .../code/java/security/UnsafeCertTrust.qll | 22 +++---- .../security/CWE-273/UnsafeCertTrustTest.java | 57 +++---------------- 3 files changed, 26 insertions(+), 63 deletions(-) diff --git a/java/ql/lib/semmle/code/java/frameworks/Networking.qll b/java/ql/lib/semmle/code/java/frameworks/Networking.qll index 7be77e26d648..c45b70364345 100644 --- a/java/ql/lib/semmle/code/java/frameworks/Networking.qll +++ b/java/ql/lib/semmle/code/java/frameworks/Networking.qll @@ -47,6 +47,14 @@ class SocketGetInputStreamMethod extends Method { } } +class SocketGetOutputStreamMethod extends Method { + SocketGetOutputStreamMethod() { + this.getDeclaringType() instanceof TypeSocket and + this.hasName("getOutputStream") and + this.hasNoParameters() + } +} + /** A method or constructor call that returns a new `URI`. */ class UriCreation extends Call { UriCreation() { @@ -152,7 +160,7 @@ class UrlOpenConnectionMethod extends Method { class CreateSocketMethod extends Method { CreateSocketMethod() { this.hasName("createSocket") and - this.getDeclaringType() instanceof TypeSocketFactory + this.getDeclaringType().getASupertype*() instanceof TypeSocketFactory } } diff --git a/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll b/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll index 64efc5130af9..bed35a1efa10 100644 --- a/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll +++ b/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll @@ -13,8 +13,14 @@ private import semmle.code.java.dataflow.DataFlow2 */ class SslConnectionInit extends DataFlow::Node { SslConnectionInit() { - this.asExpr().(MethodAccess).getMethod() instanceof CreateSslEngineMethod or - this.asExpr().(MethodAccess).getMethod() instanceof CreateSocketMethod + exists(MethodAccess ma, Method m | + this.asExpr() = ma and + ma.getMethod() = m + | + m instanceof CreateSslEngineMethod + or + m instanceof CreateSocketMethod and isSslSocket(ma) + ) } } @@ -29,21 +35,11 @@ class SslConnectionCreation extends DataFlow::Node { m instanceof BeginHandshakeMethod or m instanceof SslWrapMethod or m instanceof SslUnwrapMethod or - m instanceof SocketConnectMethod + m instanceof SocketGetOutputStreamMethod | ma.getMethod() = m and this.asExpr() = ma.getQualifier() ) - or - // calls to SocketFactory.createSocket with parameters immediately create the connection - exists(MethodAccess ma, Method m | - ma.getMethod() = m and - m instanceof CreateSocketMethod and - m.getNumberOfParameters() > 0 and - isSslSocket(ma) - | - this.asExpr() = ma - ) } } diff --git a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java index 181ce1d9a4f3..df9da3527d35 100644 --- a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java +++ b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java @@ -1,4 +1,3 @@ -import java.net.InetSocketAddress; import java.net.Socket; import java.nio.ByteBuffer; import javax.net.SocketFactory; @@ -25,9 +24,6 @@ public void testSSLEngineEndpointIdSetNull() throws Exception { sslEngine.unwrap(null, null, 0, 0); // $hasUnsafeCertTrust } - /** - * Test the endpoint identification of SSL engine is set to null - */ public void testSSLEngineEndpointIdSetEmpty() throws Exception { SSLContext sslContext = SSLContext.getInstance("TLS"); SSLEngine sslEngine = sslContext.createSSLEngine(); @@ -39,9 +35,6 @@ public void testSSLEngineEndpointIdSetEmpty() throws Exception { sslEngine.unwrap(null, null, 0, 0); // $hasUnsafeCertTrust } - /** - * Test the endpoint identification of SSL engine is set to HTTPS - */ public void testSSLEngineEndpointIdSafe() throws Exception { SSLContext sslContext = SSLContext.getInstance("TLS"); SSLEngine sslEngine = sslContext.createSSLEngine(); @@ -53,9 +46,6 @@ public void testSSLEngineEndpointIdSafe() throws Exception { sslEngine.unwrap(null, null, 0, 0); // Safe } - /** - * Test the endpoint identification of SSL engine is set to HTTPS - */ public void testSSLEngineInServerMode() throws Exception { SSLContext sslContext = SSLContext.getInstance("TLS"); SSLEngine sslEngine = sslContext.createSSLEngine(); @@ -65,28 +55,13 @@ public void testSSLEngineInServerMode() throws Exception { sslEngine.unwrap(null, null, 0, 0); // Safe } - /** - * Test the endpoint identification of SSL socket is not set - */ - public void testSSLSocketImmediatelyConnects() throws Exception { - SSLContext sslContext = SSLContext.getInstance("TLS"); - SocketFactory socketFactory = sslContext.getSocketFactory(); - SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443); // $hasUnsafeCertTrust - } - - /** - * Test the endpoint identification of SSL socket is not set - */ public void testSSLSocketEndpointIdNotSet() throws Exception { SSLContext sslContext = SSLContext.getInstance("TLS"); SSLSocketFactory socketFactory = sslContext.getSocketFactory(); SSLSocket socket = (SSLSocket) socketFactory.createSocket(); - socket.connect(new InetSocketAddress("www.example.com", 443)); // $hasUnsafeCertTrust + socket.getOutputStream(); // $hasUnsafeCertTrust } - /** - * Test the endpoint identification of SSL socket is set to null - */ public void testSSLSocketEndpointIdSetNull() throws Exception { SSLContext sslContext = SSLContext.getInstance("TLS"); SSLSocketFactory socketFactory = sslContext.getSocketFactory(); @@ -94,12 +69,9 @@ public void testSSLSocketEndpointIdSetNull() throws Exception { SSLParameters sslParameters = socket.getSSLParameters(); sslParameters.setEndpointIdentificationAlgorithm(null); socket.setSSLParameters(sslParameters); - socket.connect(new InetSocketAddress("www.example.com", 443)); // $hasUnsafeCertTrust + socket.getOutputStream(); // $hasUnsafeCertTrust } - /** - * Test the endpoint identification of SSL socket is set to empty - */ public void testSSLSocketEndpointIdSetEmpty() throws Exception { SSLContext sslContext = SSLContext.getInstance("TLS"); SSLSocketFactory socketFactory = sslContext.getSocketFactory(); @@ -107,24 +79,19 @@ public void testSSLSocketEndpointIdSetEmpty() throws Exception { SSLParameters sslParameters = socket.getSSLParameters(); sslParameters.setEndpointIdentificationAlgorithm(""); socket.setSSLParameters(sslParameters); - socket.connect(new InetSocketAddress("www.example.com", 443)); // $hasUnsafeCertTrust + socket.getOutputStream(); // $hasUnsafeCertTrust } - /** - * Test the endpoint identification of SSL socket is not set - */ public void testSSLSocketEndpointIdAfterConnecting() throws Exception { SSLContext sslContext = SSLContext.getInstance("TLS"); SSLSocketFactory socketFactory = sslContext.getSocketFactory(); - SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443); // $hasUnsafeCertTrust + SSLSocket socket = (SSLSocket) socketFactory.createSocket(); + socket.getOutputStream(); // $hasUnsafeCertTrust SSLParameters sslParameters = socket.getSSLParameters(); sslParameters.setEndpointIdentificationAlgorithm("HTTPS"); socket.setSSLParameters(sslParameters); } - /** - * Test the endpoint identification of SSL socket is not set - */ public void testSSLSocketEndpointIdSafe() throws Exception { SSLContext sslContext = SSLContext.getInstance("TLS"); SSLSocketFactory socketFactory = sslContext.getSocketFactory(); @@ -132,28 +99,20 @@ public void testSSLSocketEndpointIdSafe() throws Exception { SSLParameters sslParameters = socket.getSSLParameters(); sslParameters.setEndpointIdentificationAlgorithm("HTTPS"); socket.setSSLParameters(sslParameters); - socket.connect(new InetSocketAddress("www.example.com", 443)); // Safe + socket.getOutputStream(); // Safe } - /** - * Test the endpoint identification of regular socket is not set - */ public void testSocketEndpointIdNotSet() throws Exception { SocketFactory socketFactory = SocketFactory.getDefault(); - Socket socket = socketFactory.createSocket("www.example.com", 80); // Safe + Socket socket = socketFactory.createSocket("www.example.com", 80); + socket.getOutputStream(); // Safe } - /** - * Test the enableHostnameVerification of RabbitMQConnectionFactory is not set - */ public void testRabbitMQFactoryEnableHostnameVerificationNotSet() throws Exception { ConnectionFactory connectionFactory = new ConnectionFactory(); connectionFactory.useSslProtocol(); // $hasUnsafeCertTrust } - /** - * Test the enableHostnameVerification of RabbitMQConnectionFactory is not set - */ public void testRabbitMQFactorySafe() throws Exception { ConnectionFactory connectionFactory = new ConnectionFactory(); connectionFactory.useSslProtocol(); // Safe From 64518bf91a32cb98bebe19ef9a049497400e9147 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 23 Jun 2021 15:43:48 +0200 Subject: [PATCH 08/26] Handle a specific pass-by-reference flow issue --- .../semmle/code/java/security/UnsafeCertTrust.qll | 2 +- .../security/CWE-273/UnsafeCertTrustTest.java | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll b/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll index bed35a1efa10..2e3e26669331 100644 --- a/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll +++ b/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll @@ -97,7 +97,7 @@ private class SafeSslParametersFlowConfig extends DataFlow2::Configuration { override predicate isSource(DataFlow::Node source) { exists(MethodAccess ma | ma instanceof SafeSetEndpointIdentificationAlgorithm and - ma.getQualifier() = source.asExpr() + DataFlow::getInstanceArgument(ma) = source.(DataFlow::PostUpdateNode).getPreUpdateNode() ) } diff --git a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java index df9da3527d35..6ac808b96231 100644 --- a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java +++ b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java @@ -102,6 +102,20 @@ public void testSSLSocketEndpointIdSafe() throws Exception { socket.getOutputStream(); // Safe } + public void testSSLSocketEndpointIdSafeWithModificationByReference() throws Exception { + SSLContext sslContext = SSLContext.getInstance("TLS"); + SSLSocketFactory socketFactory = sslContext.getSocketFactory(); + SSLSocket socket = (SSLSocket) socketFactory.createSocket(); + SSLParameters sslParameters = socket.getSSLParameters(); + onSetSSLParameters(sslParameters); + socket.setSSLParameters(sslParameters); + socket.getOutputStream(); // Safe + } + + private void onSetSSLParameters(SSLParameters sslParameters) { + sslParameters.setEndpointIdentificationAlgorithm("HTTPS"); + } + public void testSocketEndpointIdNotSet() throws Exception { SocketFactory socketFactory = SocketFactory.getDefault(); Socket socket = socketFactory.createSocket("www.example.com", 80); From 19d1a780cabff7b9a0d2b8463f84ed7b42ebf679 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 23 Jun 2021 15:46:13 +0200 Subject: [PATCH 09/26] Generalize sanitizer using local flow --- .../semmle/code/java/security/UnsafeCertTrust.qll | 7 ++++--- .../security/CWE-273/UnsafeCertTrustTest.java | 12 ++++++++++++ 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll b/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll index 2e3e26669331..82e65c2fc874 100644 --- a/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll +++ b/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll @@ -55,9 +55,10 @@ abstract class SslUnsafeCertTrustSanitizer extends DataFlow::Node { } */ private class SslConnectionWithSafeSslParameters extends SslUnsafeCertTrustSanitizer { SslConnectionWithSafeSslParameters() { - exists(SafeSslParametersFlowConfig config, DataFlow::Node safe | + exists(SafeSslParametersFlowConfig config, DataFlow::Node safe, DataFlow::Node sanitizer | config.hasFlowTo(safe) and - this = DataFlow::exprNode(safe.asExpr().(Argument).getCall().getQualifier()) + sanitizer = DataFlow::exprNode(safe.asExpr().(Argument).getCall().getQualifier()) and + DataFlow::localFlow(sanitizer, this) ) } } @@ -72,7 +73,7 @@ private class SslEngineServerMode extends SslUnsafeCertTrustSanitizer { m.getDeclaringType().getASupertype*() instanceof SSLEngine and ma.getMethod() = m and ma.getArgument(0).(CompileTimeConstantExpr).getBooleanValue() = false and - this = DataFlow::exprNode(ma.getQualifier()) + this.asExpr() = ma.getQualifier() ) } } diff --git a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java index 6ac808b96231..84c5ff9048f3 100644 --- a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java +++ b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java @@ -116,6 +116,18 @@ private void onSetSSLParameters(SSLParameters sslParameters) { sslParameters.setEndpointIdentificationAlgorithm("HTTPS"); } + public void testSSLSocketEndpointIdSafeWithConditionalSanitizer(boolean safe) throws Exception { + SSLContext sslContext = SSLContext.getInstance("TLS"); + SSLSocketFactory socketFactory = sslContext.getSocketFactory(); + SSLSocket socket = (SSLSocket) socketFactory.createSocket(); + if (safe) { + SSLParameters sslParameters = socket.getSSLParameters(); + onSetSSLParameters(sslParameters); + socket.setSSLParameters(sslParameters); + } + socket.getOutputStream(); // Safe + } + public void testSocketEndpointIdNotSet() throws Exception { SocketFactory socketFactory = SocketFactory.getDefault(); Socket socket = socketFactory.createSocket("www.example.com", 80); From 9e93aecf756f028336ba034bc1e17185882448bd Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 28 Jun 2021 12:16:41 +0200 Subject: [PATCH 10/26] Add spurious test case --- .../security/CWE-273/UnsafeCertTrustTest.java | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java index 84c5ff9048f3..195b9142a4d7 100644 --- a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java +++ b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java @@ -122,12 +122,23 @@ public void testSSLSocketEndpointIdSafeWithConditionalSanitizer(boolean safe) th SSLSocket socket = (SSLSocket) socketFactory.createSocket(); if (safe) { SSLParameters sslParameters = socket.getSSLParameters(); - onSetSSLParameters(sslParameters); + sslParameters.setEndpointIdentificationAlgorithm("HTTPS"); socket.setSSLParameters(sslParameters); } socket.getOutputStream(); // Safe } + public void testSSLSocketEndpointIdSafeWithSanitizerInCast(boolean safe) throws Exception { + SSLContext sslContext = SSLContext.getInstance("TLS"); + SSLSocketFactory socketFactory = sslContext.getSocketFactory(); + Socket socket = socketFactory.createSocket(); + SSLSocket sslSocket = (SSLSocket) socket; + SSLParameters sslParameters = sslSocket.getSSLParameters(); + sslParameters.setEndpointIdentificationAlgorithm("HTTPS"); + sslSocket.setSSLParameters(sslParameters); + socket.getOutputStream(); // $ SPURIOUS: hasUnsafeCertTrust + } + public void testSocketEndpointIdNotSet() throws Exception { SocketFactory socketFactory = SocketFactory.getDefault(); Socket socket = socketFactory.createSocket("www.example.com", 80); From 5997b874de9321b5c9f5d270322136191b99d6a0 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 28 Jun 2021 13:03:35 +0200 Subject: [PATCH 11/26] Add change note --- .../ql/src/change-notes/2021-06-28-unsafe-cert-trust-query.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 java/ql/src/change-notes/2021-06-28-unsafe-cert-trust-query.md diff --git a/java/ql/src/change-notes/2021-06-28-unsafe-cert-trust-query.md b/java/ql/src/change-notes/2021-06-28-unsafe-cert-trust-query.md new file mode 100644 index 000000000000..b9944f3a12b7 --- /dev/null +++ b/java/ql/src/change-notes/2021-06-28-unsafe-cert-trust-query.md @@ -0,0 +1,4 @@ +--- +category: newQuery +--- +* The query "Unsafe certificate trust" (`java/unsafe-cert-trust`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @luchua-bc](https://github.com/github/codeql/pull/3550) \ No newline at end of file From c24520cb75d1e797a8e227476cdde528d8c16a49 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 28 Jun 2021 15:29:37 +0200 Subject: [PATCH 12/26] Adjust qhelp after rebase --- java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp b/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp index c317b9478de6..3c76d429dbe9 100644 --- a/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp +++ b/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp @@ -6,7 +6,7 @@

    Java offers two mechanisms for SSL authentication - trust manager and hostname verifier (the later is checked by the java/insecure-hostname-verifier query). Trust manager validates the peer's certificate chain while hostname verification establishes that the hostname in the URL matches the hostname in the server's identification.

    When SSLSocket or SSLEngine are created without a secure setEndpointIdentificationAlgorithm, hostname verification is disabled by default.

    -

    This query checks whether a trust manager is set to trust all certificates or setEndpointIdentificationAlgorithm is missing, thereby making the application vulnerable to man-in-the-middle attacks. The query also covers insecure configurations of com.rabbitmq.client.ConnectionFactory.

    +

    This query checks whether setEndpointIdentificationAlgorithm is missing, thereby making the application vulnerable to man-in-the-middle attacks. The query also covers insecure configurations of com.rabbitmq.client.ConnectionFactory.

     From 68fe3dd9f4120598b5d42d8d89550087f49f38b7 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 28 Jun 2021 15:40:18 +0200 Subject: [PATCH 13/26] Fix conflicts in experimental query --- .../src/experimental/Security/CWE/CWE-327/SslLib.qll | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-327/SslLib.qll b/java/ql/src/experimental/Security/CWE/CWE-327/SslLib.qll index bfa2530b07e7..7ca794220fbf 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-327/SslLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-327/SslLib.qll @@ -94,18 +94,6 @@ class UnsafeTlsVersion extends StringLiteral { } } -class SSLParameters extends RefType { - SSLParameters() { hasQualifiedName("javax.net.ssl", "SSLParameters") } -} - -class SSLSocket extends RefType { - SSLSocket() { hasQualifiedName("javax.net.ssl", "SSLSocket") } -} - class SSLServerSocket extends RefType { SSLServerSocket() { hasQualifiedName("javax.net.ssl", "SSLServerSocket") } } - -class SSLEngine extends RefType { - SSLEngine() { hasQualifiedName("javax.net.ssl", "SSLEngine") } -} From 698fd64f7f9e72ad1cf7345435d2896966175fb1 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 28 Jun 2021 16:01:03 +0200 Subject: [PATCH 14/26] Adjust test after rebase --- .../ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.ql | 2 -- 1 file changed, 2 deletions(-) diff --git a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.ql b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.ql index 66a76175c65d..605e546d3d12 100644 --- a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.ql +++ b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.ql @@ -24,8 +24,6 @@ class UnsafeCertTrustTest extends InlineExpectationsTest { override predicate hasActualResult(Location location, string element, string tag, string value) { tag = "hasUnsafeCertTrust" and exists(Expr unsafeTrust | - unsafeTrust instanceof X509TrustAllManagerInit - or unsafeTrust instanceof RabbitMQEnableHostnameVerificationNotSet or exists(Conf config | config.hasFlowTo(DataFlow::exprNode(unsafeTrust))) From e9712f04a435831e418f1e966a2d0f10e97dcb62 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Tue, 29 Jun 2021 14:30:45 +0200 Subject: [PATCH 15/26] Add missing QLDoc --- java/ql/src/semmle/code/java/security/Encryption.qll | 2 ++ 1 file changed, 2 insertions(+) diff --git a/java/ql/src/semmle/code/java/security/Encryption.qll b/java/ql/src/semmle/code/java/security/Encryption.qll index 05720974dea5..e8fab54fc73f 100644 --- a/java/ql/src/semmle/code/java/security/Encryption.qll +++ b/java/ql/src/semmle/code/java/security/Encryption.qll @@ -34,10 +34,12 @@ class SSLSession extends RefType { SSLSession() { hasQualifiedName("javax.net.ssl", "SSLSession") } } +/** The `javax.net.ssl.SSLEngine` class. */ class SSLEngine extends RefType { SSLEngine() { this.hasQualifiedName("javax.net.ssl", "SSLEngine") } } +/** The `javax.net.ssl.SSLSocket` class. */ class SSLSocket extends RefType { SSLSocket() { this.hasQualifiedName("javax.net.ssl", "SSLSocket") } } From 999acb00213b2b039a86035b6346d32c669403a8 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 1 Jul 2021 10:23:40 +0200 Subject: [PATCH 16/26] Improve qhelp references --- .../ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp b/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp index 3c76d429dbe9..6dc4f14e0444 100644 --- a/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp +++ b/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp @@ -23,6 +23,17 @@
  • Testing Endpoint Identify Verification (MSTG-NETWORK-3)
    • +
  • + SSLParameters.setEndpointIdentificationAlgorithm documentation +
    • +
  • + RabbitMQ: + ConnectionFactory.enableHostnameVerification documentation +
    • +
  • + RabbitMQ: + Using TLS in the Java Client +
  • CVE-2018-17187: Apache Qpid Proton-J transport issue with hostname verification
    • From 4d207101e274bdd0c62b82800be9623f01f4ee15 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 1 Jul 2021 10:30:42 +0200 Subject: [PATCH 17/26] Fix QLDoc --- java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll b/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll index 82e65c2fc874..2d7b8bf0c661 100644 --- a/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll +++ b/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll @@ -44,7 +44,7 @@ class SslConnectionCreation extends DataFlow::Node { } /** - * An SSL object that correctly verifies hostnames, or doesn't need to (because e.g. it's a server). + * An SSL object that correctly verifies hostnames, or doesn't need to (for instance, because it's a server). * * This is a sanitizer for `SslEndpointIdentificationFlowConfig`. */ From d9e98ceaccb028df29e759edf49f6e23958e46b9 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 1 Jul 2021 11:47:49 +0200 Subject: [PATCH 18/26] Consider setSslContextFactory and fix tests --- .../code/java/security/UnsafeCertTrust.qll | 6 +- .../security/CWE-273/UnsafeCertTrustTest.java | 38 +- .../rabbitmq/client/ConnectionFactory.java | 9 + .../rabbitmq/client/SslContextFactory.java | 23 + .../rabbitmq/client/ConnectionFactory.java | 410 ++++++++++++++++++ 5 files changed, 478 insertions(+), 8 deletions(-) create mode 100644 java/ql/test/stubs/amqp-client-5.12.0/com/rabbitmq/client/SslContextFactory.java create mode 100644 java/ql/test/stubs/http-client-3.10.0/com/rabbitmq/client/ConnectionFactory.java diff --git a/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll b/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll index 2d7b8bf0c661..64dd8dfe766b 100644 --- a/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll +++ b/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll @@ -121,12 +121,12 @@ private class SafeSetEndpointIdentificationAlgorithm extends MethodAccess { } /** - * A call to the method `useSslProtocol` on an instance of `com.rabbitmq.client.ConnectionFactory` - * that doesn't set `enableHostnameVerification`. + * A call to a method that enables SSL (`useSslProtocol` or `setSslContextFactory`) + * on an instance of `com.rabbitmq.client.ConnectionFactory` that doesn't set `enableHostnameVerification`. */ class RabbitMQEnableHostnameVerificationNotSet extends MethodAccess { RabbitMQEnableHostnameVerificationNotSet() { - this.getMethod().hasName("useSslProtocol") and + this.getMethod().hasName(["useSslProtocol", "setSslContextFactory"]) and this.getMethod().getDeclaringType() instanceof RabbitMQConnectionFactory and exists(Variable v | v.getType() instanceof RabbitMQConnectionFactory and diff --git a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java index 195b9142a4d7..5375c7c329ea 100644 --- a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java +++ b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.java @@ -1,5 +1,6 @@ import java.net.Socket; import java.nio.ByteBuffer; +import java.security.NoSuchAlgorithmException; import javax.net.SocketFactory; import javax.net.ssl.SSLContext; import javax.net.ssl.SSLEngine; @@ -7,6 +8,7 @@ import javax.net.ssl.SSLSocket; import javax.net.ssl.SSLSocketFactory; import com.rabbitmq.client.ConnectionFactory; +import com.rabbitmq.client.SslContextFactory; public class UnsafeCertTrustTest { @@ -146,13 +148,39 @@ public void testSocketEndpointIdNotSet() throws Exception { } public void testRabbitMQFactoryEnableHostnameVerificationNotSet() throws Exception { - ConnectionFactory connectionFactory = new ConnectionFactory(); - connectionFactory.useSslProtocol(); // $hasUnsafeCertTrust + { + ConnectionFactory connectionFactory = new ConnectionFactory(); + connectionFactory.useSslProtocol(SSLContext.getDefault()); // $hasUnsafeCertTrust + } + { + ConnectionFactory connectionFactory = new ConnectionFactory(); + connectionFactory.setSslContextFactory(new TestSslContextFactory()); // $hasUnsafeCertTrust + } } public void testRabbitMQFactorySafe() throws Exception { - ConnectionFactory connectionFactory = new ConnectionFactory(); - connectionFactory.useSslProtocol(); // Safe - connectionFactory.enableHostnameVerification(); + { + ConnectionFactory connectionFactory = new ConnectionFactory(); + connectionFactory.useSslProtocol(SSLContext.getDefault()); // Safe + connectionFactory.enableHostnameVerification(); + } + { + ConnectionFactory connectionFactory = new ConnectionFactory(); + connectionFactory.setSslContextFactory(new TestSslContextFactory()); // Safe + connectionFactory.enableHostnameVerification(); + } + } + + static class TestSslContextFactory implements SslContextFactory { + + @Override + public SSLContext create(String name) { + try { + return SSLContext.getDefault(); + } catch (NoSuchAlgorithmException e) { + return null; + } + } + } } diff --git a/java/ql/test/stubs/amqp-client-5.12.0/com/rabbitmq/client/ConnectionFactory.java b/java/ql/test/stubs/amqp-client-5.12.0/com/rabbitmq/client/ConnectionFactory.java index f02c88893aa6..44dd5df1a266 100644 --- a/java/ql/test/stubs/amqp-client-5.12.0/com/rabbitmq/client/ConnectionFactory.java +++ b/java/ql/test/stubs/amqp-client-5.12.0/com/rabbitmq/client/ConnectionFactory.java @@ -15,6 +15,7 @@ package com.rabbitmq.client; +import javax.net.SocketFactory; import javax.net.ssl.SSLContext; import javax.net.ssl.TrustManager; import java.io.IOException; @@ -112,6 +113,12 @@ public Map getClientProperties() { public void setClientProperties(Map clientProperties) {} + public SocketFactory getSocketFactory() { + return null; + } + + public void setSocketFactory(SocketFactory factory) {} + public void setSharedExecutor(ExecutorService executor) {} public void setShutdownExecutor(ExecutorService executor) {} @@ -204,6 +211,8 @@ public int getChannelRpcTimeout() { return 0; } + public void setSslContextFactory(SslContextFactory sslContextFactory) {} + public void setChannelShouldCheckRpcResponseType(boolean channelShouldCheckRpcResponseType) {} public boolean isChannelShouldCheckRpcResponseType() { diff --git a/java/ql/test/stubs/amqp-client-5.12.0/com/rabbitmq/client/SslContextFactory.java b/java/ql/test/stubs/amqp-client-5.12.0/com/rabbitmq/client/SslContextFactory.java new file mode 100644 index 000000000000..ae17f15437dc --- /dev/null +++ b/java/ql/test/stubs/amqp-client-5.12.0/com/rabbitmq/client/SslContextFactory.java @@ -0,0 +1,23 @@ +// Copyright (c) 2017-2020 VMware, Inc. or its affiliates. All rights reserved. +// +// This software, the RabbitMQ Java client library, is triple-licensed under the +// Mozilla Public License 2.0 ("MPL"), the GNU General Public License version 2 +// ("GPL") and the Apache License version 2 ("ASL"). For the MPL, please see +// LICENSE-MPL-RabbitMQ. For the GPL, please see LICENSE-GPL2. For the ASL, +// please see LICENSE-APACHE2. +// +// This software is distributed on an "AS IS" basis, WITHOUT WARRANTY OF ANY KIND, +// either express or implied. See the LICENSE file for specific language governing +// rights and limitations of this software. +// +// If you have any questions regarding licensing, please contact us at +// info@rabbitmq.com. + +package com.rabbitmq.client; + +import javax.net.ssl.SSLContext; + +public interface SslContextFactory { + SSLContext create(String name); + +} diff --git a/java/ql/test/stubs/http-client-3.10.0/com/rabbitmq/client/ConnectionFactory.java b/java/ql/test/stubs/http-client-3.10.0/com/rabbitmq/client/ConnectionFactory.java new file mode 100644 index 000000000000..90698f2edfa2 --- /dev/null +++ b/java/ql/test/stubs/http-client-3.10.0/com/rabbitmq/client/ConnectionFactory.java @@ -0,0 +1,410 @@ +// Copyright (c) 2007-2020 VMware, Inc. or its affiliates. All rights reserved. +// +// This software, the RabbitMQ Java client library, is triple-licensed under the +// Mozilla Public License 2.0 ("MPL"), the GNU General Public License version 2 +// ("GPL") and the Apache License version 2 ("ASL"). For the MPL, please see +// LICENSE-MPL-RabbitMQ. For the GPL, please see LICENSE-GPL2. For the ASL, +// please see LICENSE-APACHE2. +// +// This software is distributed on an "AS IS" basis, WITHOUT WARRANTY OF ANY KIND, +// either express or implied. See the LICENSE file for specific language governing +// rights and limitations of this software. +// +// If you have any questions regarding licensing, please contact us at +// info@rabbitmq.com. + +package com.rabbitmq.client; +import com.rabbitmq.client.impl.*; +import com.rabbitmq.client.impl.nio.NioParams; +import com.rabbitmq.client.impl.recovery.RetryHandler; +import com.rabbitmq.client.impl.recovery.TopologyRecoveryFilter; +import javax.net.SocketFactory; +import javax.net.ssl.SSLContext; +import javax.net.ssl.TrustManager; +import java.io.IOException; +import java.net.URI; +import java.net.URISyntaxException; +import java.security.KeyManagementException; +import java.security.NoSuchAlgorithmException; +import java.util.*; +import java.util.concurrent.*; +import java.util.function.Predicate; +import static java.util.concurrent.TimeUnit.MINUTES; + +public class ConnectionFactory implements Cloneable { + public static final int DEFAULT_CHANNEL_RPC_TIMEOUT = (int) MINUTES.toMillis(10); + + public String getHost() { + return null; + } + + public void setHost(String host) { + } + + public static int portOrDefault(int port, boolean ssl) { + return 0; + } + + public int getPort() { + return 0; + } + + public void setPort(int port) { + } + + public String getUsername() { + return null; + } + + public void setUsername(String username) { + } + + public String getPassword() { + return null; + } + + public void setPassword(String password) { + } + + public void setCredentialsProvider(CredentialsProvider credentialsProvider) { + } + + public String getVirtualHost() { + return null; + } + + public void setVirtualHost(String virtualHost) { + } + + public void setUri(URI uri) + throws URISyntaxException, NoSuchAlgorithmException, KeyManagementException + { + } + + public void setUri(String uriString) + throws URISyntaxException, NoSuchAlgorithmException, KeyManagementException + { + } + + public int getRequestedChannelMax() { + return 0; + } + + public void setRequestedChannelMax(int requestedChannelMax) { + } + + public int getRequestedFrameMax() { + return 0; + } + + public void setRequestedFrameMax(int requestedFrameMax) { + } + + public int getRequestedHeartbeat() { + return 0; + } + + public void setConnectionTimeout(int timeout) { + } + + public int getConnectionTimeout() { + return 0; + } + + public int getHandshakeTimeout() { + return 0; + } + + public void setHandshakeTimeout(int timeout) { + } + + public void setShutdownTimeout(int shutdownTimeout) { + } + + public int getShutdownTimeout() { + return 0; + } + + public void setRequestedHeartbeat(int requestedHeartbeat) { + } + + public Map getClientProperties() { + return null; + } + + public void setClientProperties(Map clientProperties) { + } + + public SaslConfig getSaslConfig() { + return null; + } + + public void setSaslConfig(SaslConfig saslConfig) { + } + + public SocketFactory getSocketFactory() { + return null; + } + + public void setSocketFactory(SocketFactory factory) { + } + + public SocketConfigurator getSocketConfigurator() { + return null; + } + + public void setSocketConfigurator(SocketConfigurator socketConfigurator) { + } + + public void setSharedExecutor(ExecutorService executor) { + } + + public void setShutdownExecutor(ExecutorService executor) { + } + + public void setHeartbeatExecutor(ScheduledExecutorService executor) { + } + + public ThreadFactory getThreadFactory() { + return null; + } + + public void setThreadFactory(ThreadFactory threadFactory) { + } + + public ExceptionHandler getExceptionHandler() { + return null; + } + + public void setExceptionHandler(ExceptionHandler exceptionHandler) { + } + + public boolean isSSL(){ + return false; + } + + public void useSslProtocol() + throws NoSuchAlgorithmException, KeyManagementException + { + } + + public void useSslProtocol(String protocol) + throws NoSuchAlgorithmException, KeyManagementException + { + } + + public void useSslProtocol(String protocol, TrustManager trustManager) + throws NoSuchAlgorithmException, KeyManagementException + { + } + + public void useSslProtocol(SSLContext context) { + } + + public void enableHostnameVerification() { + } + + public static String computeDefaultTlsProtocol(String[] supportedProtocols) { + return null; + } + + public boolean isAutomaticRecoveryEnabled() { + return false; + } + + public void setAutomaticRecoveryEnabled(boolean automaticRecovery) { + } + + public boolean isTopologyRecoveryEnabled() { + return false; + } + + public void setTopologyRecoveryEnabled(boolean topologyRecovery) { + } + + public ExecutorService getTopologyRecoveryExecutor() { + return null; + } + + public void setTopologyRecoveryExecutor(final ExecutorService topologyRecoveryExecutor) { + } + + public void setMetricsCollector(MetricsCollector metricsCollector) { + } + + public MetricsCollector getMetricsCollector() { + return null; + } + + public void setCredentialsRefreshService(CredentialsRefreshService credentialsRefreshService) { + } + + public Connection newConnection(Address[] addrs) throws IOException, TimeoutException { + return null; + } + + public Connection newConnection(AddressResolver addressResolver) throws IOException, TimeoutException { + return null; + } + + public Connection newConnection(Address[] addrs, String clientProvidedName) throws IOException, TimeoutException { + return null; + } + + public Connection newConnection(List
    addrs) throws IOException, TimeoutException { + return null; + } + + public Connection newConnection(List
    addrs, String clientProvidedName) throws IOException, TimeoutException { + return null; + } + + public Connection newConnection(ExecutorService executor, Address[] addrs) throws IOException, TimeoutException { + return null; + } + + public Connection newConnection(ExecutorService executor, Address[] addrs, String clientProvidedName) throws IOException, TimeoutException { + return null; + } + + public Connection newConnection(ExecutorService executor, List
    addrs) throws IOException, TimeoutException { + return null; + } + + public Connection newConnection(ExecutorService executor, AddressResolver addressResolver) throws IOException, TimeoutException { + return null; + } + + public Connection newConnection(ExecutorService executor, List
    addrs, String clientProvidedName) + throws IOException, TimeoutException { + return null; + } + + public Connection newConnection(ExecutorService executor, AddressResolver addressResolver, String clientProvidedName) + throws IOException, TimeoutException { + return null; + } + + public ConnectionParams params(ExecutorService consumerWorkServiceExecutor) { + return null; + } + + public Connection newConnection() throws IOException, TimeoutException { + return null; + } + + public Connection newConnection(String connectionName) throws IOException, TimeoutException { + return null; + } + + public Connection newConnection(ExecutorService executor) throws IOException, TimeoutException { + return null; + } + + public Connection newConnection(ExecutorService executor, String connectionName) throws IOException, TimeoutException { + return null; + } + + @Override public ConnectionFactory clone(){ + @Override public ConnectionFactory clone(){ + return null; + } + + public ConnectionFactory load(String propertyFileLocation) throws IOException { + return null; + } + + public ConnectionFactory load(String propertyFileLocation, String prefix) throws IOException { + return null; + } + + public ConnectionFactory load(Properties properties) { + return null; + } + + public ConnectionFactory load(Properties properties, String prefix) { + return null; + } + + public ConnectionFactory load(Map properties) { + return null; + } + + public ConnectionFactory load(Map properties, String prefix) { + return null; + } + + public long getNetworkRecoveryInterval() { + return 0; + } + + public void setNetworkRecoveryInterval(int networkRecoveryInterval) { + } + + public void setNetworkRecoveryInterval(long networkRecoveryInterval) { + } + + public RecoveryDelayHandler getRecoveryDelayHandler() { + return null; + } + + public void setRecoveryDelayHandler(final RecoveryDelayHandler recoveryDelayHandler) { + } + + public void setNioParams(NioParams nioParams) { + } + + public NioParams getNioParams() { + return null; + } + + public void useNio() { + } + + public void useBlockingIo() { + } + + public void setChannelRpcTimeout(int channelRpcTimeout) { + } + + public int getChannelRpcTimeout() { + return 0; + } + + public void setSslContextFactory(SslContextFactory sslContextFactory) { + } + + public void setChannelShouldCheckRpcResponseType(boolean channelShouldCheckRpcResponseType) { + } + + public boolean isChannelShouldCheckRpcResponseType() { + return false; + } + + public void setWorkPoolTimeout(int workPoolTimeout) { + } + + public int getWorkPoolTimeout() { + return 0; + } + + public void setErrorOnWriteListener(ErrorOnWriteListener errorOnWriteListener) { + } + + public void setTopologyRecoveryFilter(TopologyRecoveryFilter topologyRecoveryFilter) { + } + + public void setConnectionRecoveryTriggeringCondition(Predicate connectionRecoveryTriggeringCondition) { + } + + public void setTopologyRecoveryRetryHandler(RetryHandler topologyRecoveryRetryHandler) { + } + + public void setTrafficListener(TrafficListener trafficListener) { + } + + public static int ensureUnsignedShort(int value) { + return 0; + } + +} From 1e2a956a30afb263feb699b0f6729f79b18b7890 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 1 Jul 2021 15:11:54 +0200 Subject: [PATCH 19/26] Remove unused stub --- .../rabbitmq/client/ConnectionFactory.java | 410 ------------------ 1 file changed, 410 deletions(-) delete mode 100644 java/ql/test/stubs/http-client-3.10.0/com/rabbitmq/client/ConnectionFactory.java diff --git a/java/ql/test/stubs/http-client-3.10.0/com/rabbitmq/client/ConnectionFactory.java b/java/ql/test/stubs/http-client-3.10.0/com/rabbitmq/client/ConnectionFactory.java deleted file mode 100644 index 90698f2edfa2..000000000000 --- a/java/ql/test/stubs/http-client-3.10.0/com/rabbitmq/client/ConnectionFactory.java +++ /dev/null @@ -1,410 +0,0 @@ -// Copyright (c) 2007-2020 VMware, Inc. or its affiliates. All rights reserved. -// -// This software, the RabbitMQ Java client library, is triple-licensed under the -// Mozilla Public License 2.0 ("MPL"), the GNU General Public License version 2 -// ("GPL") and the Apache License version 2 ("ASL"). For the MPL, please see -// LICENSE-MPL-RabbitMQ. For the GPL, please see LICENSE-GPL2. For the ASL, -// please see LICENSE-APACHE2. -// -// This software is distributed on an "AS IS" basis, WITHOUT WARRANTY OF ANY KIND, -// either express or implied. See the LICENSE file for specific language governing -// rights and limitations of this software. -// -// If you have any questions regarding licensing, please contact us at -// info@rabbitmq.com. - -package com.rabbitmq.client; -import com.rabbitmq.client.impl.*; -import com.rabbitmq.client.impl.nio.NioParams; -import com.rabbitmq.client.impl.recovery.RetryHandler; -import com.rabbitmq.client.impl.recovery.TopologyRecoveryFilter; -import javax.net.SocketFactory; -import javax.net.ssl.SSLContext; -import javax.net.ssl.TrustManager; -import java.io.IOException; -import java.net.URI; -import java.net.URISyntaxException; -import java.security.KeyManagementException; -import java.security.NoSuchAlgorithmException; -import java.util.*; -import java.util.concurrent.*; -import java.util.function.Predicate; -import static java.util.concurrent.TimeUnit.MINUTES; - -public class ConnectionFactory implements Cloneable { - public static final int DEFAULT_CHANNEL_RPC_TIMEOUT = (int) MINUTES.toMillis(10); - - public String getHost() { - return null; - } - - public void setHost(String host) { - } - - public static int portOrDefault(int port, boolean ssl) { - return 0; - } - - public int getPort() { - return 0; - } - - public void setPort(int port) { - } - - public String getUsername() { - return null; - } - - public void setUsername(String username) { - } - - public String getPassword() { - return null; - } - - public void setPassword(String password) { - } - - public void setCredentialsProvider(CredentialsProvider credentialsProvider) { - } - - public String getVirtualHost() { - return null; - } - - public void setVirtualHost(String virtualHost) { - } - - public void setUri(URI uri) - throws URISyntaxException, NoSuchAlgorithmException, KeyManagementException - { - } - - public void setUri(String uriString) - throws URISyntaxException, NoSuchAlgorithmException, KeyManagementException - { - } - - public int getRequestedChannelMax() { - return 0; - } - - public void setRequestedChannelMax(int requestedChannelMax) { - } - - public int getRequestedFrameMax() { - return 0; - } - - public void setRequestedFrameMax(int requestedFrameMax) { - } - - public int getRequestedHeartbeat() { - return 0; - } - - public void setConnectionTimeout(int timeout) { - } - - public int getConnectionTimeout() { - return 0; - } - - public int getHandshakeTimeout() { - return 0; - } - - public void setHandshakeTimeout(int timeout) { - } - - public void setShutdownTimeout(int shutdownTimeout) { - } - - public int getShutdownTimeout() { - return 0; - } - - public void setRequestedHeartbeat(int requestedHeartbeat) { - } - - public Map getClientProperties() { - return null; - } - - public void setClientProperties(Map clientProperties) { - } - - public SaslConfig getSaslConfig() { - return null; - } - - public void setSaslConfig(SaslConfig saslConfig) { - } - - public SocketFactory getSocketFactory() { - return null; - } - - public void setSocketFactory(SocketFactory factory) { - } - - public SocketConfigurator getSocketConfigurator() { - return null; - } - - public void setSocketConfigurator(SocketConfigurator socketConfigurator) { - } - - public void setSharedExecutor(ExecutorService executor) { - } - - public void setShutdownExecutor(ExecutorService executor) { - } - - public void setHeartbeatExecutor(ScheduledExecutorService executor) { - } - - public ThreadFactory getThreadFactory() { - return null; - } - - public void setThreadFactory(ThreadFactory threadFactory) { - } - - public ExceptionHandler getExceptionHandler() { - return null; - } - - public void setExceptionHandler(ExceptionHandler exceptionHandler) { - } - - public boolean isSSL(){ - return false; - } - - public void useSslProtocol() - throws NoSuchAlgorithmException, KeyManagementException - { - } - - public void useSslProtocol(String protocol) - throws NoSuchAlgorithmException, KeyManagementException - { - } - - public void useSslProtocol(String protocol, TrustManager trustManager) - throws NoSuchAlgorithmException, KeyManagementException - { - } - - public void useSslProtocol(SSLContext context) { - } - - public void enableHostnameVerification() { - } - - public static String computeDefaultTlsProtocol(String[] supportedProtocols) { - return null; - } - - public boolean isAutomaticRecoveryEnabled() { - return false; - } - - public void setAutomaticRecoveryEnabled(boolean automaticRecovery) { - } - - public boolean isTopologyRecoveryEnabled() { - return false; - } - - public void setTopologyRecoveryEnabled(boolean topologyRecovery) { - } - - public ExecutorService getTopologyRecoveryExecutor() { - return null; - } - - public void setTopologyRecoveryExecutor(final ExecutorService topologyRecoveryExecutor) { - } - - public void setMetricsCollector(MetricsCollector metricsCollector) { - } - - public MetricsCollector getMetricsCollector() { - return null; - } - - public void setCredentialsRefreshService(CredentialsRefreshService credentialsRefreshService) { - } - - public Connection newConnection(Address[] addrs) throws IOException, TimeoutException { - return null; - } - - public Connection newConnection(AddressResolver addressResolver) throws IOException, TimeoutException { - return null; - } - - public Connection newConnection(Address[] addrs, String clientProvidedName) throws IOException, TimeoutException { - return null; - } - - public Connection newConnection(List
    addrs) throws IOException, TimeoutException { - return null; - } - - public Connection newConnection(List
    addrs, String clientProvidedName) throws IOException, TimeoutException { - return null; - } - - public Connection newConnection(ExecutorService executor, Address[] addrs) throws IOException, TimeoutException { - return null; - } - - public Connection newConnection(ExecutorService executor, Address[] addrs, String clientProvidedName) throws IOException, TimeoutException { - return null; - } - - public Connection newConnection(ExecutorService executor, List
    addrs) throws IOException, TimeoutException { - return null; - } - - public Connection newConnection(ExecutorService executor, AddressResolver addressResolver) throws IOException, TimeoutException { - return null; - } - - public Connection newConnection(ExecutorService executor, List
    addrs, String clientProvidedName) - throws IOException, TimeoutException { - return null; - } - - public Connection newConnection(ExecutorService executor, AddressResolver addressResolver, String clientProvidedName) - throws IOException, TimeoutException { - return null; - } - - public ConnectionParams params(ExecutorService consumerWorkServiceExecutor) { - return null; - } - - public Connection newConnection() throws IOException, TimeoutException { - return null; - } - - public Connection newConnection(String connectionName) throws IOException, TimeoutException { - return null; - } - - public Connection newConnection(ExecutorService executor) throws IOException, TimeoutException { - return null; - } - - public Connection newConnection(ExecutorService executor, String connectionName) throws IOException, TimeoutException { - return null; - } - - @Override public ConnectionFactory clone(){ - @Override public ConnectionFactory clone(){ - return null; - } - - public ConnectionFactory load(String propertyFileLocation) throws IOException { - return null; - } - - public ConnectionFactory load(String propertyFileLocation, String prefix) throws IOException { - return null; - } - - public ConnectionFactory load(Properties properties) { - return null; - } - - public ConnectionFactory load(Properties properties, String prefix) { - return null; - } - - public ConnectionFactory load(Map properties) { - return null; - } - - public ConnectionFactory load(Map properties, String prefix) { - return null; - } - - public long getNetworkRecoveryInterval() { - return 0; - } - - public void setNetworkRecoveryInterval(int networkRecoveryInterval) { - } - - public void setNetworkRecoveryInterval(long networkRecoveryInterval) { - } - - public RecoveryDelayHandler getRecoveryDelayHandler() { - return null; - } - - public void setRecoveryDelayHandler(final RecoveryDelayHandler recoveryDelayHandler) { - } - - public void setNioParams(NioParams nioParams) { - } - - public NioParams getNioParams() { - return null; - } - - public void useNio() { - } - - public void useBlockingIo() { - } - - public void setChannelRpcTimeout(int channelRpcTimeout) { - } - - public int getChannelRpcTimeout() { - return 0; - } - - public void setSslContextFactory(SslContextFactory sslContextFactory) { - } - - public void setChannelShouldCheckRpcResponseType(boolean channelShouldCheckRpcResponseType) { - } - - public boolean isChannelShouldCheckRpcResponseType() { - return false; - } - - public void setWorkPoolTimeout(int workPoolTimeout) { - } - - public int getWorkPoolTimeout() { - return 0; - } - - public void setErrorOnWriteListener(ErrorOnWriteListener errorOnWriteListener) { - } - - public void setTopologyRecoveryFilter(TopologyRecoveryFilter topologyRecoveryFilter) { - } - - public void setConnectionRecoveryTriggeringCondition(Predicate connectionRecoveryTriggeringCondition) { - } - - public void setTopologyRecoveryRetryHandler(RetryHandler topologyRecoveryRetryHandler) { - } - - public void setTrafficListener(TrafficListener trafficListener) { - } - - public static int ensureUnsignedShort(int value) { - return 0; - } - -} From 000a5447298416ddd5e3d843e1a4f123d9077ca2 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 21 Jul 2021 11:29:49 +0200 Subject: [PATCH 20/26] Decouple UnsafeCertTrust.qll to reuse the taint tracking configuration --- .../code/java/security/UnsafeCertTrust.qll | 42 ------------- .../Security/CWE/CWE-273/UnsafeCertTrust.ql | 15 +---- .../2021-06-28-unsafe-cert-trust-query.md | 2 +- .../java/security/UnsafeCertTrustQuery.qll | 62 +++++++++++++++++++ .../security/CWE-273/UnsafeCertTrustTest.ql | 20 ++---- 5 files changed, 68 insertions(+), 73 deletions(-) create mode 100644 java/ql/src/semmle/code/java/security/UnsafeCertTrustQuery.qll diff --git a/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll b/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll index 64dd8dfe766b..55c278a173c9 100644 --- a/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll +++ b/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll @@ -4,7 +4,6 @@ import java private import semmle.code.java.frameworks.Networking private import semmle.code.java.security.Encryption private import semmle.code.java.dataflow.DataFlow -private import semmle.code.java.dataflow.DataFlow2 /** * The creation of an object that prepares an SSL connection. @@ -50,19 +49,6 @@ class SslConnectionCreation extends DataFlow::Node { */ abstract class SslUnsafeCertTrustSanitizer extends DataFlow::Node { } -/** - * An SSL object that was assigned a safe `SSLParameters` object and can be considered safe. - */ -private class SslConnectionWithSafeSslParameters extends SslUnsafeCertTrustSanitizer { - SslConnectionWithSafeSslParameters() { - exists(SafeSslParametersFlowConfig config, DataFlow::Node safe, DataFlow::Node sanitizer | - config.hasFlowTo(safe) and - sanitizer = DataFlow::exprNode(safe.asExpr().(Argument).getCall().getQualifier()) and - DataFlow::localFlow(sanitizer, this) - ) - } -} - /** * An `SSLEngine` set in server mode. */ @@ -92,34 +78,6 @@ private predicate isSslSocket(MethodAccess createSocket) { createSocket.getQualifier().getType().(RefType).getASupertype*() instanceof SSLSocketFactory } -private class SafeSslParametersFlowConfig extends DataFlow2::Configuration { - SafeSslParametersFlowConfig() { this = "SafeSslParametersFlowConfig" } - - override predicate isSource(DataFlow::Node source) { - exists(MethodAccess ma | - ma instanceof SafeSetEndpointIdentificationAlgorithm and - DataFlow::getInstanceArgument(ma) = source.(DataFlow::PostUpdateNode).getPreUpdateNode() - ) - } - - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma, RefType t | t instanceof SSLSocket or t instanceof SSLEngine | - ma.getMethod().hasName("setSSLParameters") and - ma.getMethod().getDeclaringType().getASupertype*() = t and - ma.getArgument(0) = sink.asExpr() - ) - } -} - -private class SafeSetEndpointIdentificationAlgorithm extends MethodAccess { - SafeSetEndpointIdentificationAlgorithm() { - this.getMethod().hasName("setEndpointIdentificationAlgorithm") and - this.getMethod().getDeclaringType() instanceof SSLParameters and - not this.getArgument(0) instanceof NullLiteral and - not this.getArgument(0).(CompileTimeConstantExpr).getStringValue().length() = 0 - } -} - /** * A call to a method that enables SSL (`useSslProtocol` or `setSslContextFactory`) * on an instance of `com.rabbitmq.client.ConnectionFactory` that doesn't set `enableHostnameVerification`. diff --git a/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql b/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql index 2893329a4c33..12f20b40c93b 100644 --- a/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql +++ b/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql @@ -12,20 +12,7 @@ */ import java -import semmle.code.java.dataflow.TaintTracking -import semmle.code.java.security.UnsafeCertTrust - -class SslEndpointIdentificationFlowConfig extends TaintTracking::Configuration { - SslEndpointIdentificationFlowConfig() { this = "SslEndpointIdentificationFlowConfig" } - - override predicate isSource(DataFlow::Node source) { source instanceof SslConnectionInit } - - override predicate isSink(DataFlow::Node sink) { sink instanceof SslConnectionCreation } - - override predicate isSanitizer(DataFlow::Node sanitizer) { - sanitizer instanceof SslUnsafeCertTrustSanitizer - } -} +import semmle.code.java.security.UnsafeCertTrustQuery from Expr unsafeTrust where diff --git a/java/ql/src/change-notes/2021-06-28-unsafe-cert-trust-query.md b/java/ql/src/change-notes/2021-06-28-unsafe-cert-trust-query.md index b9944f3a12b7..bde0c9d02496 100644 --- a/java/ql/src/change-notes/2021-06-28-unsafe-cert-trust-query.md +++ b/java/ql/src/change-notes/2021-06-28-unsafe-cert-trust-query.md @@ -1,4 +1,4 @@ --- category: newQuery --- -* The query "Unsafe certificate trust" (`java/unsafe-cert-trust`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @luchua-bc](https://github.com/github/codeql/pull/3550) \ No newline at end of file +* The query "Unsafe certificate trust" (`java/unsafe-cert-trust`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @luchua-bc](https://github.com/github/codeql/pull/3550). diff --git a/java/ql/src/semmle/code/java/security/UnsafeCertTrustQuery.qll b/java/ql/src/semmle/code/java/security/UnsafeCertTrustQuery.qll new file mode 100644 index 000000000000..61f890a1d47b --- /dev/null +++ b/java/ql/src/semmle/code/java/security/UnsafeCertTrustQuery.qll @@ -0,0 +1,62 @@ +/** Provides taint tracking configurations to be used by unsafe certificate trust queries. */ + +import java +import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.security.UnsafeCertTrust +import semmle.code.java.security.Encryption + +class SslEndpointIdentificationFlowConfig extends TaintTracking::Configuration { + SslEndpointIdentificationFlowConfig() { this = "SslEndpointIdentificationFlowConfig" } + + override predicate isSource(DataFlow::Node source) { source instanceof SslConnectionInit } + + override predicate isSink(DataFlow::Node sink) { sink instanceof SslConnectionCreation } + + override predicate isSanitizer(DataFlow::Node sanitizer) { + sanitizer instanceof SslUnsafeCertTrustSanitizer + } +} + +/** + * An SSL object that was assigned a safe `SSLParameters` object and can be considered safe. + */ +private class SslConnectionWithSafeSslParameters extends SslUnsafeCertTrustSanitizer { + SslConnectionWithSafeSslParameters() { + exists(SafeSslParametersFlowConfig config, DataFlow::Node safe, DataFlow::Node sanitizer | + config.hasFlowTo(safe) and + sanitizer = DataFlow::exprNode(safe.asExpr().(Argument).getCall().getQualifier()) and + DataFlow::localFlow(sanitizer, this) + ) + } +} + +private class SafeSslParametersFlowConfig extends DataFlow2::Configuration { + SafeSslParametersFlowConfig() { this = "SafeSslParametersFlowConfig" } + + override predicate isSource(DataFlow::Node source) { + exists(MethodAccess ma | + ma instanceof SafeSetEndpointIdentificationAlgorithm and + DataFlow::getInstanceArgument(ma) = source.(DataFlow::PostUpdateNode).getPreUpdateNode() + ) + } + + override predicate isSink(DataFlow::Node sink) { + exists(MethodAccess ma, RefType t | t instanceof SSLSocket or t instanceof SSLEngine | + ma.getMethod().hasName("setSSLParameters") and + ma.getMethod().getDeclaringType().getASupertype*() = t and + ma.getArgument(0) = sink.asExpr() + ) + } +} + +/** + * A call to `SSLParameters.setEndpointIdentificationAlgorithm` with a non-null and non-zero parameter. + */ +private class SafeSetEndpointIdentificationAlgorithm extends MethodAccess { + SafeSetEndpointIdentificationAlgorithm() { + this.getMethod().hasName("setEndpointIdentificationAlgorithm") and + this.getMethod().getDeclaringType() instanceof SSLParameters and + not this.getArgument(0) instanceof NullLiteral and + not this.getArgument(0).(CompileTimeConstantExpr).getStringValue().length() = 0 + } +} diff --git a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.ql b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.ql index 605e546d3d12..344faa7f86b6 100644 --- a/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.ql +++ b/java/ql/test/query-tests/security/CWE-273/UnsafeCertTrustTest.ql @@ -1,21 +1,7 @@ import java -import semmle.code.java.dataflow.FlowSources -import semmle.code.java.dataflow.TaintTracking -import semmle.code.java.security.UnsafeCertTrust +import semmle.code.java.security.UnsafeCertTrustQuery import TestUtilities.InlineExpectationsTest -class Conf extends TaintTracking::Configuration { - Conf() { this = "qltest:cwe:unsafe-cert-trust" } - - override predicate isSource(DataFlow::Node source) { source instanceof SslConnectionInit } - - override predicate isSink(DataFlow::Node sink) { sink instanceof SslConnectionCreation } - - override predicate isSanitizer(DataFlow::Node sanitizer) { - sanitizer instanceof SslUnsafeCertTrustSanitizer - } -} - class UnsafeCertTrustTest extends InlineExpectationsTest { UnsafeCertTrustTest() { this = "HasUnsafeCertTrustTest" } @@ -26,7 +12,9 @@ class UnsafeCertTrustTest extends InlineExpectationsTest { exists(Expr unsafeTrust | unsafeTrust instanceof RabbitMQEnableHostnameVerificationNotSet or - exists(Conf config | config.hasFlowTo(DataFlow::exprNode(unsafeTrust))) + exists(SslEndpointIdentificationFlowConfig config | + config.hasFlowTo(DataFlow::exprNode(unsafeTrust)) + ) | unsafeTrust.getLocation() = location and element = unsafeTrust.toString() and From c16181dd2f9c944b4fe26908e2cffc1cb5ed49f9 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 21 Jul 2021 11:40:02 +0200 Subject: [PATCH 21/26] QLDocs --- java/ql/src/semmle/code/java/security/UnsafeCertTrustQuery.qll | 3 +++ 1 file changed, 3 insertions(+) diff --git a/java/ql/src/semmle/code/java/security/UnsafeCertTrustQuery.qll b/java/ql/src/semmle/code/java/security/UnsafeCertTrustQuery.qll index 61f890a1d47b..954af24dbbb1 100644 --- a/java/ql/src/semmle/code/java/security/UnsafeCertTrustQuery.qll +++ b/java/ql/src/semmle/code/java/security/UnsafeCertTrustQuery.qll @@ -5,6 +5,9 @@ import semmle.code.java.dataflow.TaintTracking import semmle.code.java.security.UnsafeCertTrust import semmle.code.java.security.Encryption +/** + * A taint flow configuration for SSL connections created without a proper certificate trust configuration. + */ class SslEndpointIdentificationFlowConfig extends TaintTracking::Configuration { SslEndpointIdentificationFlowConfig() { this = "SslEndpointIdentificationFlowConfig" } From 9ffc5ab1839decc10361f8d35e4fab9debe290dc Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 26 Jul 2021 16:58:45 +0200 Subject: [PATCH 22/26] Update java/ql/src/semmle/code/java/security/UnsafeCertTrustQuery.qll Co-authored-by: Marcono1234 --- java/ql/src/semmle/code/java/security/UnsafeCertTrustQuery.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/semmle/code/java/security/UnsafeCertTrustQuery.qll b/java/ql/src/semmle/code/java/security/UnsafeCertTrustQuery.qll index 954af24dbbb1..05bd9825a58f 100644 --- a/java/ql/src/semmle/code/java/security/UnsafeCertTrustQuery.qll +++ b/java/ql/src/semmle/code/java/security/UnsafeCertTrustQuery.qll @@ -53,7 +53,7 @@ private class SafeSslParametersFlowConfig extends DataFlow2::Configuration { } /** - * A call to `SSLParameters.setEndpointIdentificationAlgorithm` with a non-null and non-zero parameter. + * A call to `SSLParameters.setEndpointIdentificationAlgorithm` with a non-null and non-empty parameter. */ private class SafeSetEndpointIdentificationAlgorithm extends MethodAccess { SafeSetEndpointIdentificationAlgorithm() { From 03020582afb8c59c352d5591db9cd14697c227ac Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 29 Jul 2021 17:16:09 +0200 Subject: [PATCH 23/26] Apply suggestions from code review Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com> --- .../Security/CWE/CWE-273/UnsafeCertTrust.qhelp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp b/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp index 6dc4f14e0444..8545b3ffbb6d 100644 --- a/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp +++ b/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp @@ -4,7 +4,7 @@ -

    Java offers two mechanisms for SSL authentication - trust manager and hostname verifier (the later is checked by the java/insecure-hostname-verifier query). Trust manager validates the peer's certificate chain while hostname verification establishes that the hostname in the URL matches the hostname in the server's identification.

    +

    Java offers two mechanisms for SSL authentication - trust manager and hostname verifier (the later is checked by the java/insecure-hostname-verifier query). The trust manager validates the peer's certificate chain while hostname verification establishes that the hostname in the URL matches the hostname in the server's identification.

    When SSLSocket or SSLEngine are created without a secure setEndpointIdentificationAlgorithm, hostname verification is disabled by default.

    This query checks whether setEndpointIdentificationAlgorithm is missing, thereby making the application vulnerable to man-in-the-middle attacks. The query also covers insecure configurations of com.rabbitmq.client.ConnectionFactory.

     @@ -21,30 +21,30 @@
  • -Testing Endpoint Identify Verification (MSTG-NETWORK-3) +Testing Endpoint Identify Verification (MSTG-NETWORK-3).
  • - SSLParameters.setEndpointIdentificationAlgorithm documentation + SSLParameters.setEndpointIdentificationAlgorithm documentation.
  • RabbitMQ: - ConnectionFactory.enableHostnameVerification documentation + ConnectionFactory.enableHostnameVerification documentation.
  • RabbitMQ: - Using TLS in the Java Client + Using TLS in the Java Client.
  • -CVE-2018-17187: Apache Qpid Proton-J transport issue with hostname verification +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-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-11087: Pivotal Spring AMQP vulnerability due to lack of hostname validation.
  • -CVE-2018-11775: TLS hostname verification issue when using the Apache ActiveMQ Client +CVE-2018-11775: TLS hostname verification issue when using the Apache ActiveMQ Client.
    • From 101ad777e3430f0be2bc983d4564fffdf50b730d Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Fri, 12 Nov 2021 10:50:07 +0100 Subject: [PATCH 24/26] Move things around after rebase --- .../code/java/frameworks/Networking.qll | 2 + .../semmle/code/java/security/Encryption.qll | 4 +- .../java/security/UnsafeCertTrustQuery.qll | 0 .../semmle/code/java/security/Encryption.qll | 375 ------------------ 4 files changed, 5 insertions(+), 376 deletions(-) rename java/ql/{src => lib}/semmle/code/java/security/UnsafeCertTrustQuery.qll (100%) delete mode 100644 java/ql/src/semmle/code/java/security/Encryption.qll diff --git a/java/ql/lib/semmle/code/java/frameworks/Networking.qll b/java/ql/lib/semmle/code/java/frameworks/Networking.qll index c45b70364345..daa1f4ca8f96 100644 --- a/java/ql/lib/semmle/code/java/frameworks/Networking.qll +++ b/java/ql/lib/semmle/code/java/frameworks/Networking.qll @@ -47,6 +47,7 @@ class SocketGetInputStreamMethod extends Method { } } +/** The method `java.net.Socket::getOutputStream`. */ class SocketGetOutputStreamMethod extends Method { SocketGetOutputStreamMethod() { this.getDeclaringType() instanceof TypeSocket and @@ -164,6 +165,7 @@ class CreateSocketMethod extends Method { } } +/** The method `javax.net.Socket::connect`. */ class SocketConnectMethod extends Method { SocketConnectMethod() { this.hasName("connect") and diff --git a/java/ql/lib/semmle/code/java/security/Encryption.qll b/java/ql/lib/semmle/code/java/security/Encryption.qll index 57c91d9cd55b..513c4c0099ca 100644 --- a/java/ql/lib/semmle/code/java/security/Encryption.qll +++ b/java/ql/lib/semmle/code/java/security/Encryption.qll @@ -34,10 +34,12 @@ class SSLSession extends RefType { SSLSession() { this.hasQualifiedName("javax.net.ssl", "SSLSession") } } +/** The `javax.net.ssl.SSLEngine` class. */ class SSLEngine extends RefType { SSLEngine() { this.hasQualifiedName("javax.net.ssl", "SSLEngine") } } +/** The `javax.net.ssl.SSLSocket` class. */ class SSLSocket extends RefType { SSLSocket() { this.hasQualifiedName("javax.net.ssl", "SSLSocket") } } @@ -221,7 +223,7 @@ string getInsecureAlgorithmRegex() { string getASecureAlgorithmName() { result = [ - "RSA", "SHA256", "SHA512", "CCM", "GCM", "AES([^a-zA-Z](?!ECB|CBC/PKCS[57]Padding)).*", + "RSA", "SHA256", "SHA512", "CCM", "GCM", "AES(?![^a-zA-Z](ECB|CBC/PKCS[57]Padding))", "Blowfish", "ECIES" ] } diff --git a/java/ql/src/semmle/code/java/security/UnsafeCertTrustQuery.qll b/java/ql/lib/semmle/code/java/security/UnsafeCertTrustQuery.qll similarity index 100% rename from java/ql/src/semmle/code/java/security/UnsafeCertTrustQuery.qll rename to java/ql/lib/semmle/code/java/security/UnsafeCertTrustQuery.qll diff --git a/java/ql/src/semmle/code/java/security/Encryption.qll b/java/ql/src/semmle/code/java/security/Encryption.qll deleted file mode 100644 index e8fab54fc73f..000000000000 --- a/java/ql/src/semmle/code/java/security/Encryption.qll +++ /dev/null @@ -1,375 +0,0 @@ -/** - * Provides predicates and classes relating to encryption in Java. - */ - -import java - -class SSLClass extends RefType { - SSLClass() { - exists(Class c | this.getASupertype*() = c | - c.hasQualifiedName("javax.net.ssl", _) or - c.hasQualifiedName("javax.rmi.ssl", _) - ) - } -} - -class X509TrustManager extends RefType { - X509TrustManager() { this.hasQualifiedName("javax.net.ssl", "X509TrustManager") } -} - -class HttpsURLConnection extends RefType { - HttpsURLConnection() { hasQualifiedName("javax.net.ssl", "HttpsURLConnection") } -} - -class SSLSocketFactory extends RefType { - SSLSocketFactory() { this.hasQualifiedName("javax.net.ssl", "SSLSocketFactory") } -} - -class SSLContext extends RefType { - SSLContext() { hasQualifiedName("javax.net.ssl", "SSLContext") } -} - -/** The `javax.net.ssl.SSLSession` class. */ -class SSLSession extends RefType { - SSLSession() { hasQualifiedName("javax.net.ssl", "SSLSession") } -} - -/** The `javax.net.ssl.SSLEngine` class. */ -class SSLEngine extends RefType { - SSLEngine() { this.hasQualifiedName("javax.net.ssl", "SSLEngine") } -} - -/** The `javax.net.ssl.SSLSocket` class. */ -class SSLSocket extends RefType { - SSLSocket() { this.hasQualifiedName("javax.net.ssl", "SSLSocket") } -} - -/** The `javax.net.ssl.SSLParameters` class. */ -class SSLParameters extends RefType { - SSLParameters() { this.hasQualifiedName("javax.net.ssl", "SSLParameters") } -} - -class HostnameVerifier extends RefType { - HostnameVerifier() { hasQualifiedName("javax.net.ssl", "HostnameVerifier") } -} - -/** The Java class `javax.crypto.KeyGenerator`. */ -class KeyGenerator extends RefType { - KeyGenerator() { this.hasQualifiedName("javax.crypto", "KeyGenerator") } -} - -/** The Java class `java.security.KeyPairGenerator`. */ -class KeyPairGenerator extends RefType { - KeyPairGenerator() { this.hasQualifiedName("java.security", "KeyPairGenerator") } -} - -/** The `verify` method of the class `javax.net.ssl.HostnameVerifier`. */ -class HostnameVerifierVerify extends Method { - HostnameVerifierVerify() { - hasName("verify") and - getDeclaringType().getASupertype*() instanceof HostnameVerifier and - getParameterType(0) instanceof TypeString and - getParameterType(1) instanceof SSLSession - } -} - -class TrustManagerCheckMethod extends Method { - TrustManagerCheckMethod() { - (this.hasName("checkClientTrusted") or this.hasName("checkServerTrusted")) and - this.getDeclaringType().getASupertype*() instanceof X509TrustManager - } -} - -class CreateSocket extends Method { - CreateSocket() { - hasName("createSocket") and - getDeclaringType() instanceof SSLSocketFactory - } -} - -class GetSocketFactory extends Method { - GetSocketFactory() { - hasName("getSocketFactory") and - getDeclaringType() instanceof SSLContext - } -} - -/** The `createSSLEngine` method of the class `javax.net.ssl.SSLContext` */ -class CreateSslEngineMethod extends Method { - CreateSslEngineMethod() { - this.hasName("createSSLEngine") and - this.getDeclaringType() instanceof SSLContext - } -} - -class SetConnectionFactoryMethod extends Method { - SetConnectionFactoryMethod() { - hasName("setSSLSocketFactory") and - getDeclaringType().getASupertype*() instanceof HttpsURLConnection - } -} - -class SetHostnameVerifierMethod extends Method { - SetHostnameVerifierMethod() { - hasName("setHostnameVerifier") and - getDeclaringType().getASupertype*() instanceof HttpsURLConnection - } -} - -/** The `setDefaultHostnameVerifier` method of the class `javax.net.ssl.HttpsURLConnection`. */ -class SetDefaultHostnameVerifierMethod extends Method { - SetDefaultHostnameVerifierMethod() { - hasName("setDefaultHostnameVerifier") and - getDeclaringType().getASupertype*() instanceof HttpsURLConnection - } -} - -/** The `beginHandshake` method of the class `javax.net.ssl.SSLEngine`. */ -class BeginHandshakeMethod extends Method { - BeginHandshakeMethod() { - hasName("beginHandshake") and - getDeclaringType().getASupertype*() instanceof SSLEngine - } -} - -/** The `wrap` method of the class `javax.net.ssl.SSLEngine`. */ -class SslWrapMethod extends Method { - SslWrapMethod() { - hasName("wrap") and - getDeclaringType().getASupertype*() instanceof SSLEngine - } -} - -/** The `unwrap` method of the class `javax.net.ssl.SSLEngine`. */ -class SslUnwrapMethod extends Method { - SslUnwrapMethod() { - hasName("unwrap") and - getDeclaringType().getASupertype*() instanceof SSLEngine - } -} - -bindingset[algorithmString] -private string algorithmRegex(string algorithmString) { - // Algorithms usually appear in names surrounded by characters that are not - // alphabetical characters in the same case. This handles the upper and lower - // case cases. - result = - "((^|.*[^A-Z])(" + algorithmString + ")([^A-Z].*|$))" + - // or... - "|" + - // For lowercase, we want to be careful to avoid being confused by camelCase - // hence we require two preceding uppercase letters to be sure of a case switch, - // or a preceding non-alphabetic character - "((^|.*[A-Z]{2}|.*[^a-zA-Z])(" + algorithmString.toLowerCase() + ")([^a-z].*|$))" -} - -/** - * Gets the name of an algorithm that is known to be insecure. - */ -string getAnInsecureAlgorithmName() { - result = - [ - "DES", "RC2", "RC4", "RC5", - // ARCFOUR is a variant of RC4 - "ARCFOUR", - // Encryption mode ECB like AES/ECB/NoPadding is vulnerable to replay and other attacks - "ECB", - // CBC mode of operation with PKCS#5 or PKCS#7 padding is vulnerable to padding oracle attacks - "AES/CBC/PKCS[57]Padding" - ] -} - -/** - * Gets the name of a hash algorithm that is insecure if it is being used for - * encryption. - */ -string getAnInsecureHashAlgorithmName() { - result = "SHA1" or - result = "MD5" -} - -private string rankedInsecureAlgorithm(int i) { - // In this case we know these are being used for encryption, so we want to match - // weak hash algorithms too. - result = - rank[i](string s | s = getAnInsecureAlgorithmName() or s = getAnInsecureHashAlgorithmName()) -} - -private string insecureAlgorithmString(int i) { - i = 1 and result = rankedInsecureAlgorithm(i) - or - result = rankedInsecureAlgorithm(i) + "|" + insecureAlgorithmString(i - 1) -} - -/** - * Gets the regular expression used for matching strings that look like they - * contain an algorithm that is known to be insecure. - */ -string getInsecureAlgorithmRegex() { - result = algorithmRegex(insecureAlgorithmString(max(int i | exists(rankedInsecureAlgorithm(i))))) -} - -/** - * Gets the name of an algorithm that is known to be secure. - */ -string getASecureAlgorithmName() { - result = - [ - "RSA", "SHA256", "SHA512", "CCM", "GCM", "AES([^a-zA-Z](?!ECB|CBC/PKCS[57]Padding)).*", - "Blowfish", "ECIES" - ] -} - -private string rankedSecureAlgorithm(int i) { result = rank[i](getASecureAlgorithmName()) } - -private string secureAlgorithmString(int i) { - i = 1 and result = rankedSecureAlgorithm(i) - or - result = rankedSecureAlgorithm(i) + "|" + secureAlgorithmString(i - 1) -} - -/** - * Gets a regular expression for matching strings that look like they - * contain an algorithm that is known to be secure. - */ -string getSecureAlgorithmRegex() { - result = algorithmRegex(secureAlgorithmString(max(int i | exists(rankedSecureAlgorithm(i))))) -} - -/** - * DEPRECATED: Terminology has been updated. Use `getAnInsecureAlgorithmName()` - * instead. - */ -deprecated string algorithmBlacklist() { result = getAnInsecureAlgorithmName() } - -/** - * DEPRECATED: Terminology has been updated. Use - * `getAnInsecureHashAlgorithmName()` instead. - */ -deprecated string hashAlgorithmBlacklist() { result = getAnInsecureHashAlgorithmName() } - -/** - * DEPRECATED: Terminology has been updated. Use `getInsecureAlgorithmRegex()` instead. - */ -deprecated string algorithmBlacklistRegex() { result = getInsecureAlgorithmRegex() } - -/** - * DEPRECATED: Terminology has been updated. Use `getASecureAlgorithmName()` - * instead. - */ -deprecated string algorithmWhitelist() { result = getASecureAlgorithmName() } - -/** - * DEPRECATED: Terminology has been updated. Use `getSecureAlgorithmRegex()` instead. - */ -deprecated string algorithmWhitelistRegex() { result = getSecureAlgorithmRegex() } - -/** - * Any use of a cryptographic element that specifies an encryption - * algorithm. For example, methods returning ciphers, decryption methods, - * constructors of cipher classes, etc. - */ -abstract class CryptoAlgoSpec extends Top { - CryptoAlgoSpec() { this instanceof Call } - - abstract Expr getAlgoSpec(); -} - -abstract class JavaxCryptoAlgoSpec extends CryptoAlgoSpec { } - -class JavaxCryptoCipher extends JavaxCryptoAlgoSpec { - JavaxCryptoCipher() { - exists(Method m | m.getAReference() = this | - m.getDeclaringType().getQualifiedName() = "javax.crypto.Cipher" and - m.getName() = "getInstance" - ) - } - - override Expr getAlgoSpec() { result = this.(MethodAccess).getArgument(0) } -} - -class JavaxCryptoSecretKey extends JavaxCryptoAlgoSpec { - JavaxCryptoSecretKey() { - exists(Constructor c | c.getAReference() = this | - c.getDeclaringType().getQualifiedName() = "javax.crypto.spec.SecretKeySpec" - ) - } - - override Expr getAlgoSpec() { - exists(ConstructorCall c | c = this | - if c.getNumArgument() = 2 then result = c.getArgument(1) else result = c.getArgument(3) - ) - } -} - -class JavaxCryptoKeyGenerator extends JavaxCryptoAlgoSpec { - JavaxCryptoKeyGenerator() { - exists(Method m | m.getAReference() = this | - m.getDeclaringType() instanceof KeyGenerator and - m.getName() = "getInstance" - ) - } - - override Expr getAlgoSpec() { result = this.(MethodAccess).getArgument(0) } -} - -class JavaxCryptoKeyAgreement extends JavaxCryptoAlgoSpec { - JavaxCryptoKeyAgreement() { - exists(Method m | m.getAReference() = this | - m.getDeclaringType().getQualifiedName() = "javax.crypto.KeyAgreement" and - m.getName() = "getInstance" - ) - } - - override Expr getAlgoSpec() { result = this.(MethodAccess).getArgument(0) } -} - -class JavaxCryptoKeyFactory extends JavaxCryptoAlgoSpec { - JavaxCryptoKeyFactory() { - exists(Method m | m.getAReference() = this | - m.getDeclaringType().getQualifiedName() = "javax.crypto.SecretKeyFactory" and - m.getName() = "getInstance" - ) - } - - override Expr getAlgoSpec() { result = this.(MethodAccess).getArgument(0) } -} - -abstract class JavaSecurityAlgoSpec extends CryptoAlgoSpec { } - -class JavaSecurityMessageDigest extends JavaSecurityAlgoSpec { - JavaSecurityMessageDigest() { - exists(Constructor c | c.getAReference() = this | - c.getDeclaringType().hasQualifiedName("java.security", "MessageDigest") - ) - or - exists(Method m | m.getAReference() = this | - m.getDeclaringType().hasQualifiedName("java.security", "MessageDigest") and - m.getName() = "getInstance" - ) - } - - override Expr getAlgoSpec() { result = this.(Call).getArgument(0) } -} - -class JavaSecuritySignature extends JavaSecurityAlgoSpec { - JavaSecuritySignature() { - exists(Constructor c | c.getAReference() = this | - c.getDeclaringType().getQualifiedName() = "java.security.Signature" - ) - } - - override Expr getAlgoSpec() { result = this.(ConstructorCall).getArgument(0) } -} - -/** A method call to the Java class `java.security.KeyPairGenerator`. */ -class JavaSecurityKeyPairGenerator extends JavaxCryptoAlgoSpec { - JavaSecurityKeyPairGenerator() { - exists(Method m | m.getAReference() = this | - m.getDeclaringType() instanceof KeyPairGenerator and - m.getName() = "getInstance" - ) - } - - override Expr getAlgoSpec() { result = this.(MethodAccess).getArgument(0) } -} From e442e50e6becbc970cd34fcc6a4d640ca29f222d Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 19 Jan 2022 16:41:31 +0100 Subject: [PATCH 25/26] Apply suggestions from code review Co-authored-by: Anders Schack-Mulligen --- java/ql/lib/semmle/code/java/security/UnsafeCertTrustQuery.qll | 2 +- java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/UnsafeCertTrustQuery.qll b/java/ql/lib/semmle/code/java/security/UnsafeCertTrustQuery.qll index 05bd9825a58f..767b86d4e412 100644 --- a/java/ql/lib/semmle/code/java/security/UnsafeCertTrustQuery.qll +++ b/java/ql/lib/semmle/code/java/security/UnsafeCertTrustQuery.qll @@ -60,6 +60,6 @@ private class SafeSetEndpointIdentificationAlgorithm extends MethodAccess { this.getMethod().hasName("setEndpointIdentificationAlgorithm") and this.getMethod().getDeclaringType() instanceof SSLParameters and not this.getArgument(0) instanceof NullLiteral and - not this.getArgument(0).(CompileTimeConstantExpr).getStringValue().length() = 0 + not this.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "" } } diff --git a/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql b/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql index 12f20b40c93b..13e1375d164a 100644 --- a/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql +++ b/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql @@ -20,4 +20,4 @@ where exists(SslEndpointIdentificationFlowConfig config | config.hasFlowTo(DataFlow::exprNode(unsafeTrust)) ) -select unsafeTrust, "Unsafe configuration of trusted certificates" +select unsafeTrust, "Unsafe configuration of trusted certificates." From 695e77a21996fa77e8900093ad272506e9b6f911 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 19 Jan 2022 16:59:19 +0100 Subject: [PATCH 26/26] Simplify isSslSocket predicate --- java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll b/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll index 55c278a173c9..c94864d86859 100644 --- a/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll +++ b/java/ql/lib/semmle/code/java/security/UnsafeCertTrust.qll @@ -69,11 +69,7 @@ private class SslEngineServerMode extends SslUnsafeCertTrustSanitizer { * or the qualifier of `createSocket` is an instance of `SSLSocketFactory`. */ private predicate isSslSocket(MethodAccess createSocket) { - exists(Variable ssl, CastExpr ce | - ce.getExpr() = createSocket and - ce.getControlFlowNode().getASuccessor().(VariableAssign).getDestVar() = ssl and - ssl.getType() instanceof SSLSocket - ) + createSocket = any(CastExpr ce | ce.getType() instanceof SSLSocket).getExpr() or createSocket.getQualifier().getType().(RefType).getASupertype*() instanceof SSLSocketFactory }