Big refactor:

- Move classes and predicates to appropriate libraries - Overhaul the endpoint identification algorithm logic to use taint tracking - Adapt tests
github · atorralba · Jan 20, 2022 · Jun 21, 2021 · Jun 21, 2021 · Jun 22, 2021
commit 4313baf6228885378fa85612ebd61a8581e97a19
@@ -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]

@@ -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 Metho
8000
d {
+  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"
     ]
 }

@@ -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") }
+}
@@ -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"