Refactor timing attack queries

Author: egregius313

java/ql/src/experimental/Security/CWE/CWE-208/NonConstantTimeCheckOnSignatureQuery.qll

@@ -4,8 +4,6 @@
 
 import semmle.code.java.controlflow.Guards
 import semmle.code.java.dataflow.TaintTracking
-import semmle.code.java.dataflow.TaintTracking2
-import semmle.code.java.dataflow.DataFlow3
 import semmle.code.java.dataflow.FlowSources
 
 /** A method call that produces cryptographic result. */
@@ -51,25 +49,25 @@ private class ProduceSignatureCall extends ProduceCryptoCall {
  * A config that tracks data flow from initializing a cipher for encryption
  * to producing a ciphertext using this cipher.
  */
-private class InitializeEncryptorConfig extends DataFlow3::Configuration {
-  InitializeEncryptorConfig() { this = "InitializeEncryptorConfig" }
-
-  override predicate isSource(DataFlow::Node source) {
+private module InitializeEncryptorConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
     exists(MethodAccess ma |
       ma.getMethod().hasQualifiedName("javax.crypto", "Cipher", "init") and
       ma.getArgument(0).(VarAccess).getVariable().hasName("ENCRYPT_MODE") and
       ma.getQualifier() = source.asExpr()
     )
   }
 
-  override predicate isSink(DataFlow::Node sink) {
+  predicate isSink(DataFlow::Node sink) {
     exists(MethodAccess ma |
       ma.getMethod().hasQualifiedName("javax.crypto", "Cipher", "doFinal") and
       ma.getQualifier() = sink.asExpr()
     )
   }
 }
 
+private module InitializeEncryptorFlow = DataFlow::Global<InitializeEncryptorConfig>;
+
 /** A method call that produces a ciphertext. */
 private class ProduceCiphertextCall extends ProduceCryptoCall {
   ProduceCiphertextCall() {
@@ -90,9 +88,7 @@ private class ProduceCiphertextCall extends ProduceCryptoCall {
         this.getArgument(1) = output
       )
     ) and
-    exists(InitializeEncryptorConfig config |
-      config.hasFlowTo(DataFlow3::exprNode(this.getQualifier()))
-    )
+    InitializeEncryptorFlow::flowToExpr(this.getQualifier())
   }
 
   override string getResultType() { result = "ciphertext" }
@@ -151,16 +147,14 @@ private predicate updateMessageDigestStep(DataFlow2::Node fromNode, DataFlow2::N
  * A config that tracks data flow from remote user input to a cryptographic operation
  * such as cipher, MAC or signature.
  */
-private class UserInputInCryptoOperationConfig extends TaintTracking2::Configuration {
-  UserInputInCryptoOperationConfig() { this = "UserInputInCryptoOperationConfig" }
-
-  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+private module UserInputInCryptoOperationConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
 
-  override predicate isSink(DataFlow::Node sink) {
+  predicate isSink(DataFlow::Node sink) {
     exists(ProduceCryptoCall call | call.getQualifier() = sink.asExpr())
   }
 
-  override predicate isAdditionalTaintStep(DataFlow2::Node fromNode, DataFlow2::Node toNode) {
+  predicate isAdditionalFlowStep(DataFlow2::Node fromNode, DataFlow2::Node toNode) {
     updateCryptoOperationStep(fromNode, toNode)
     or
     createMessageDigestStep(fromNode, toNode)
@@ -169,6 +163,13 @@ private class UserInputInCryptoOperationConfig extends TaintTracking2::Configura
   }
 }
 
+/**
+ * Taint-tracking flow from remote user input to a cryptographic operation
+ * such as cipher, MAC or signature.
+ */
+private module UserInputInCryptoOperationFlow =
+  TaintTracking::Global<UserInputInCryptoOperationConfig>;
+
 /** A source that produces result of cryptographic operation. */
 class CryptoOperationSource extends DataFlow::Node {
   ProduceCryptoCall call;
@@ -177,8 +178,8 @@ class CryptoOperationSource extends DataFlow::Node {
 
   /** Holds if remote user input was used in the cryptographic operation. */
   predicate includesUserInput() {
-    exists(DataFlow2::PathNode sink, UserInputInCryptoOperationConfig config |
-      config.hasFlowPath(_, sink)
+    exists(UserInputInCryptoOperationFlow::PathNode sink |
+      UserInputInCryptoOperationFlow::flowPath(_, sink)
     |
       sink.getNode().asExpr() = call.getQualifier()
     )
@@ -212,12 +213,10 @@ private class NonConstantTimeComparisonCall extends StaticMethodAccess {
  * A config that tracks data flow from remote user input to methods
  * that compare inputs using a non-constant-time algorithm.
  */
-private class UserInputInComparisonConfig extends TaintTracking2::Configuration {
-  UserInputInComparisonConfig() { this = "UserInputInComparisonConfig" }
+private module UserInputInComparisonConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
 
-  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
-
-  override predicate isSink(DataFlow::Node sink) {
+  predicate isSink(DataFlow::Node sink) {
     exists(NonConstantTimeEqualsCall call |
       sink.asExpr() = [call.getAnArgument(), call.getQualifier()]
     )
@@ -226,6 +225,8 @@ private class UserInputInComparisonConfig extends TaintTracking2::Configuration
   }
 }
 
+private module UserInputInComparisonFlow = TaintTracking::Global<UserInputInComparisonConfig>;
+
 /** Holds if `expr` looks like a constant. */
 private predicate looksLikeConstant(Expr expr) {
   expr.isCompileTimeConstant()
@@ -301,21 +302,18 @@ class NonConstantTimeComparisonSink extends DataFlow::Node {
   }
 
   /** Holds if remote user input was used in the comparison. */
-  predicate includesUserInput() {
-    exists(UserInputInComparisonConfig config |
-      config.hasFlowTo(DataFlow2::exprNode(anotherParameter))
-    )
-  }
+  predicate includesUserInput() { UserInputInComparisonFlow::flowToExpr(anotherParameter) }
 }
 
 /**
  * A configuration that tracks data flow from cryptographic operations
  * to methods that compare data using a non-constant-time algorithm.
  */
-class NonConstantTimeCryptoComparisonConfig extends TaintTracking::Configuration {
-  NonConstantTimeCryptoComparisonConfig() { this = "NonConstantTimeCryptoComparisonConfig" }
-
-  override predicate isSource(DataFlow::Node source) { source instanceof CryptoOperationSource }
+module NonConstantTimeCryptoComparisonConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof CryptoOperationSource }
 
-  override predicate isSink(DataFlow::Node sink) { sink instanceof NonConstantTimeComparisonSink }
+  predicate isSink(DataFlow::Node sink) { sink instanceof NonConstantTimeComparisonSink }
 }
+
+module NonConstantTimeCryptoComparisonFlow =
+  TaintTracking::Global<NonConstantTimeCryptoComparisonConfig>;

java/ql/src/experimental/Security/CWE/CWE-208/PossibleTimingAttackAgainstSignature.ql

@@ -15,9 +15,11 @@
 
 import java
 import NonConstantTimeCheckOnSignatureQuery
-import DataFlow::PathGraph
+import NonConstantTimeCryptoComparisonFlow::PathGraph
 
-from DataFlow::PathNode source, DataFlow::PathNode sink, NonConstantTimeCryptoComparisonConfig conf
-where conf.hasFlowPath(source, sink)
+from
+  NonConstantTimeCryptoComparisonFlow::PathNode source,
+  NonConstantTimeCryptoComparisonFlow::PathNode sink
+where NonConstantTimeCryptoComparisonFlow::flowPath(source, sink)
 select sink.getNode(), source, sink, "Possible timing attack against $@ validation.", source,
   source.getNode().(CryptoOperationSource).getCall().getResultType()

java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSignature.ql

@@ -16,11 +16,13 @@
 
 import java
 import NonConstantTimeCheckOnSignatureQuery
-import DataFlow::PathGraph
+import NonConstantTimeCryptoComparisonFlow::PathGraph
 
-from DataFlow::PathNode source, DataFlow::PathNode sink, NonConstantTimeCryptoComparisonConfig conf
+from
+  NonConstantTimeCryptoComparisonFlow::PathNode source,
+  NonConstantTimeCryptoComparisonFlow::PathNode sink
 where
-  conf.hasFlowPath(source, sink) and
+  NonConstantTimeCryptoComparisonFlow::flowPath(source, sink) and
   (
     source.getNode().(CryptoOperationSource).includesUserInput() and
     sink.getNode().(NonConstantTimeComparisonSink).includesUserInput()