Multiple bug fixes in BouncyCastle signature model

Author: fegge

java/ql/lib/experimental/quantum/BouncyCastle.qll

@@ -1,17 +1,13 @@
-
 import java
 
 /**
  * Models for the signature algorithms defined by the `org.bouncycastle.crypto.signers` package.
- * 
  */
 module Signers {
   import Language
   import BouncyCastle.FlowAnalysis
   import BouncyCastle.AlgorithmInstances
 
-  abstract class SignatureAlgorithmValueConsumer extends Crypto::AlgorithmValueConsumer { }
-
   /**
    * A model of the `Signer` class in Bouncy Castle.
    */
@@ -21,70 +17,49 @@ module Signers {
       this.getName().matches("%Signer")
     }
 
-    MethodCall getAnInitCall() {
-      result = this.getAMethodCall("init")
-    }
+    MethodCall getAnInitCall() { result = this.getAMethodCall("init") }
 
     MethodCall getAUseCall() {
       result = this.getAMethodCall(["update", "generateSignature", "verifySignature"])
     }
-    
+
     MethodCall getAMethodCall(string name) {
       result.getCallee().hasQualifiedName("org.bouncycastle.crypto.signers", this.getName(), name)
     }
   }
 
   /**
    * BouncyCastle algorithms are instantiated by calling the constructor of the
-   * corresponding class. The algorithm is implicitly defined by the constructor
-   * call.
+   * corresponding class.
    */
-  class NewCall extends SignatureAlgorithmValueConsumer instanceof ClassInstanceExpr {
-    NewCall() {
-      this.getConstructedType() instanceof Signer
-    }
-
-    override Crypto::AlgorithmInstance getAKnownAlgorithmSource() {
-      result = getSignatureAlgorithmInstanceFromType(super.getConstructedType())
-    }
-  
-    // TODO: Since the algorithm is implicitly defined by the constructor, should
-    // the input node be `this`?
-    override Crypto::ConsumerInputDataFlowNode getInputNode() { 
-      result.asExpr() = this
-    }
-  }
+  class NewCall = SignatureAlgorithmInstance;
 
   /**
    * The type is instantiated by a constructor call and initialized by a call to
    * `init()` which takes two arguments. The first argument is a flag indicating
    * whether the operation is signing data or verifying a signature, and the
-   * second is the key to use. 
+   * second is the key to use.
    */
   class InitCall extends MethodCall {
-    InitCall() {
-      this = any(Signer signer).getAnInitCall() 
-    }
+    InitCall() { this = any(Signer signer).getAnInitCall() }
 
     Expr getForSigningArg() { result = this.getArgument(0) }
 
     Expr getKeyArg() { result = this.getArgument(1) }
 
-    Crypto::KeyOperationAlgorithmInstance getAlgorithm() {
-      result = getSignatureAlgorithmInstanceFromType(this.getReceiverType())
-    }
-
+    // TODO: Support dataflow for the operation sub-type.
     Crypto::KeyOperationSubtype getKeyOperationSubtype() {
-      (
+      if this.isOperationSubTypeKnown()
+      then
         this.getForSigningArg().(BooleanLiteral).getBooleanValue() = true and
         result = Crypto::TSignMode()
-      ) or (
+        or
         this.getForSigningArg().(BooleanLiteral).getBooleanValue() = false and
         result = Crypto::TVerifyMode()
-      ) or (
-        result = Crypto::TUnknownKeyOperationMode()
-      )
+      else result = Crypto::TUnknownKeyOperationMode()
     }
+
+    predicate isOperationSubTypeKnown() { this.getForSigningArg() instanceof BooleanLiteral }
   }
 
   /**
@@ -93,19 +68,13 @@ module Signers {
    * verify the signature, respectively.
    */
   class UseCall extends MethodCall {
-    UseCall() {
-      this = any(Signer signer).getAUseCall()
-    }
+    UseCall() { this = any(Signer signer).getAUseCall() }
 
-    predicate isIntermediate() {
-      this.getCallee().getName() = "update"
-    }
+    predicate isIntermediate() { this.getCallee().getName() = "update" }
 
     Expr getInput() { result = this.getArgument(0) }
 
-    Expr getOutput() {
-      result = this
-    }
+    Expr getOutput() { result = this }
   }
 
   /**
@@ -118,10 +87,12 @@ module Signers {
    * or `verifySignature()` on a `Signer` instance.
    */
   class SignatureOperationInstance extends Crypto::KeyOperationInstance instanceof UseCall {
-    
+    SignatureOperationInstance() { not this.isIntermediate() }
+
     override Crypto::AlgorithmValueConsumer getAnAlgorithmValueConsumer() {
-      result = FlowAnalysis::getInstantiationFromInit(this.getInitCall(), _, _)
+      result = FlowAnalysis::getInstantiationFromUse(this, _, _)
     }
+
     override Crypto::KeyOperationSubtype getKeyOperationSubtype() {
       if FlowAnalysis::hasInit(this)
       then result = this.getInitCall().getKeyOperationSubtype()
@@ -144,17 +115,12 @@ module Signers {
       result.asExpr() = super.getOutput()
     }
 
-    InitCall getInitCall() {
-      result = FlowAnalysis::getInitFromUse(this, _, _)
-    }
+    InitCall getInitCall() { result = FlowAnalysis::getInitFromUse(this, _, _) }
 
     UseCall getAnUpdateCall() {
-      (
-        super.isIntermediate() and result = this
-      ) or (
-        result = FlowAnalysis::getAnIntermediateUseFromFinalUse(this, _, _)
-      )
+      super.isIntermediate() and result = this
+      or
+      result = FlowAnalysis::getAnIntermediateUseFromFinalUse(this, _, _)
     }
   }
 }
-

java/ql/lib/experimental/quantum/BouncyCastle/AlgorithmInstances.qll

@@ -1,10 +1,3 @@
 import java
 import experimental.quantum.Language
 import AlgorithmInstances.SignatureAlgorithmInstances
-
-SignatureAlgorithmInstance getSignatureAlgorithmInstanceFromType(Type t) {
-  t.getName() = "Ed25519Signer" and result instanceof Ed25519AlgorithmInstance
-  or
-  t.getName() = "Ed448Signer" and result instanceof Ed448AlgorithmInstance
-}
-

java/ql/lib/experimental/quantum/BouncyCastle/AlgorithmInstances/SignatureAlgorithmInstances.qll

@@ -1,44 +1,51 @@
 import java
 import experimental.quantum.Language
 
-abstract class SignatureAlgorithmInstance extends Crypto::KeyOperationAlgorithmInstance {
+/**
+ * Signature algorithms are implicitly defined by the constructor.
+ */
+private abstract class SignatureAlgorithmValueConsumer extends Crypto::AlgorithmValueConsumer { 
+  override Crypto::AlgorithmInstance getAKnownAlgorithmSource() {
+    result = this
+  }
+
+  override Crypto::ConsumerInputDataFlowNode getInputNode() { 
+    none()
+  }
+}
 
-  // TODO: Could potentially be used to model signature modes like Ed25519ph and Ed25519ctx.
+class SignatureAlgorithmInstance extends Crypto::KeyOperationAlgorithmInstance, SignatureAlgorithmValueConsumer instanceof ClassInstanceExpr {
+  SignatureAlgorithmInstance() {
+    super.getConstructedType().getPackage().getName() = "org.bouncycastle.crypto.signers" and
+    super.getConstructedType().getName().matches("%Signer")
+
+  }
+
+   // TODO: Could potentially be used to model signature modes like Ed25519ph and Ed25519ctx.
   override Crypto::ModeOfOperationAlgorithmInstance getModeOfOperationAlgorithm() { none() }
 
   override Crypto::PaddingAlgorithmInstance getPaddingAlgorithm() { none() }
 
   override Crypto::ConsumerInputDataFlowNode getKeySizeConsumer() { none() }
-}
 
-class Ed25519AlgorithmInstance extends SignatureAlgorithmInstance instanceof ClassInstanceExpr {
-  Ed25519AlgorithmInstance() {
-    this.getConstructedType().hasQualifiedName("org.bouncycastle.crypto.signers", "Ed25519Signer")
+  override Crypto::KeyOpAlg::Algorithm getAlgorithmType() {
+    nameToTypeAndKeySizeMapping(this.getRawAlgorithmName(), _, result)
   }
 
-  override string getRawAlgorithmName() { result = "Ed25519" }
-
-  override Crypto::KeyOpAlg::Algorithm getAlgorithmType() { 
-    result = Crypto::KeyOpAlg::TSignature(Crypto::KeyOpAlg::Ed25519()) 
-  }
-
-  // TODO: May be redundant.
-  override string getKeySizeFixed() { result = "256" }
-}
-
-class Ed448AlgorithmInstance extends SignatureAlgorithmInstance instanceof ClassInstanceExpr {
-  Ed448AlgorithmInstance() {
-    this.getConstructedType().hasQualifiedName("org.bouncycastle.crypto.signers", "Ed448Signer")
+  override string getRawAlgorithmName() { 
+    result = super.getConstructedType().getName().splitAt("Signer", 0)
   }
 
-  override string getRawAlgorithmName() { result = "Ed448" }
-
-  override Crypto::KeyOpAlg::Algorithm getAlgorithmType() { 
-    result = Crypto::KeyOpAlg::TSignature(Crypto::KeyOpAlg::Ed448()) 
+  override string getKeySizeFixed() { 
+    nameToTypeAndKeySizeMapping(this.getRawAlgorithmName(), result, _)
   }
+}
 
-  // TODO: May be redundant.
-  override string getKeySizeFixed() { result = "448" }
+predicate nameToTypeAndKeySizeMapping(string name, string keySize, Crypto::KeyOpAlg::Algorithm algorithm) {
+  name = "Ed25519" and keySize = "256" and algorithm = Crypto::KeyOpAlg::TSignature(Crypto::KeyOpAlg::Ed25519())
+  or
+  name = "Ed448" and keySize = "448" and algorithm = Crypto::KeyOpAlg::TSignature(Crypto::KeyOpAlg::Ed448())
 }
 
 
+