Added signature input nodes to signature verify operation nodes

Author: fegge

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

@@ -236,8 +236,6 @@ module Signers {
     SignatureOperationInstance() { not this.isIntermediate() }
 
     override Crypto::AlgorithmValueConsumer getAnAlgorithmValueConsumer() {
-      result = this.getParameters().getAnAlgorithmValueConsumer()
-      or
       result = SignerFlow::getNewFromUse(this, _, _)
     }
 
@@ -266,7 +264,7 @@ module Signers {
       result.asExpr() = super.getMessageInput()
     }
 
-    override Crypto::ConsumerInputDataFlowNode getSignatureArtifactConsumer() {
+    override Crypto::ConsumerInputDataFlowNode getSignatureConsumer() {
       result.asExpr() = super.getSignatureInput()
     }
 
@@ -280,16 +278,6 @@ module Signers {
     SignerUseCall getAnUpdateCall() {
       result = SignerFlow::getAnIntermediateUseFromFinalUse(this, _, _)
     }
-
-    Crypto::KeyArtifactOutputInstance getKey() { result.flowsTo(this.getInitCall().getKeyArg()) }
-
-    Generators::KeyGenerationOperationInstance getKeyGenerationOperationInstance() {
-      result.getKeyArtifactOutputInstance() = this.getKey()
-    }
-
-    Params::ParametersInstantiation getParameters() {
-      result = this.getKeyGenerationOperationInstance().getParameters()
-    }
   }
 }
 

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

@@ -150,7 +150,6 @@ abstract class KeyGenerationAlgorithmInstance extends Crypto::KeyOperationAlgori
 
   override Crypto::PaddingAlgorithmInstance getPaddingAlgorithm() { none() }
 
-  // TODO: Model flow from the parameter specification to the key generator.
   override Crypto::ConsumerInputDataFlowNode getKeySizeConsumer() { none() }
 
   override Crypto::KeyOpAlg::Algorithm getAlgorithmType() {

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

@@ -247,10 +247,42 @@ class KeyAdditionalFlowSteps extends MethodCall {
   DataFlow::Node getOutputNode() { result.asExpr() = this }
 }
 
+/**
+ * Model data flow from an ECDSA signature to the scalars r and s passed to
+ * `verifySignature()`. The ECDSA signature is represented as a `BigInteger`
+ * array, where the first element is the scalar r and the second element is the
+ * scalar s.
+ */
+class ECDSASignatureAdditionalFlowSteps extends ArrayAccess {
+  ECDSASignatureAdditionalFlowSteps() {
+    this.getArray().getType().getName() = "BigInteger[]" and
+    // It is reasonable to assume that the indices are integer literals
+    this.getIndexExpr().(IntegerLiteral).getValue().toInt() = [0, 1]
+  }
+
+  /**
+   * The input node is the ECDSA signature represented as a `BigInteger` array.
+   */
+  DataFlow::Node getInputNode() {
+    // TODO: This should be the array node `this.getArray()`
+    result.asExpr() = this.getArray()
+  }
+
+  /**
+   * The output node is the `BigInteger` element accessed by this array access.
+   */
+  DataFlow::Node getOutputNode() { result.asExpr() = this }
+}
+
 predicate additionalFlowSteps(DataFlow::Node node1, DataFlow::Node node2) {
-  exists(KeyAdditionalFlowSteps call |
-    node1 = call.getInputNode() and
-    node2 = call.getOutputNode()
+  exists(KeyAdditionalFlowSteps fs |
+    node1 = fs.getInputNode() and
+    node2 = fs.getOutputNode()
+  )
+  or
+  exists(ECDSASignatureAdditionalFlowSteps fs |
+    node1 = fs.getInputNode() and
+    node2 = fs.getOutputNode()
   )
 }
 

shared/quantum/codeql/quantum/experimental/Model.qll

@@ -435,6 +435,17 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
     final override ConsumerInputDataFlowNode getInputNode() { result = inputNode }
   }
 
+  final private class SignatureArtifactConsumer extends ArtifactConsumerAndInstance {
+    ConsumerInputDataFlowNode inputNode;
+
+    SignatureArtifactConsumer() {
+      exists(SignatureOperationInstance op | inputNode = op.getSignatureConsumer()) and
+      this = Input::dfn_to_element(inputNode)
+    }
+
+    final override ConsumerInputDataFlowNode getInputNode() { result = inputNode }
+  }
+
   /**
    * An artifact that is produced by an operation, representing a concrete artifact instance rather than a synthetic consumer artifact.
    */
@@ -471,6 +482,8 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
     override DataFlowNode getOutputNode() { result = creator.getOutputArtifact() }
 
     KeyOperationInstance getCreator() { result = creator }
+
+    KeyOperationInstance getCreator() { result = creator }
   }
 
   /**
@@ -800,6 +813,8 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
    */
   abstract class SignatureOperationInstance extends KeyOperationInstance {
     /**
+     * Gets the consumer of the signature that is being verified in case of a
+     * verification operation.
      * Gets the consumer of the signature that is being verified in case of a
      * verification operation.
      */
@@ -1300,7 +1315,6 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
     TKeyOperation(KeyOperationInstance e) or
     TKeyOperationAlgorithm(KeyOperationAlgorithmInstanceOrValueConsumer e) or
     TKeyOperationOutput(KeyOperationOutputArtifactInstance e) or
-    TSignature(SignatureOperationInstance e) or
     // Non-Standalone Algorithms (e.g., Mode, Padding)
     // These algorithms are always tied to a key operation algorithm
     TModeOfOperationAlgorithm(ModeOfOperationAlgorithmInstance e) or
@@ -1547,6 +1561,20 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
     override LocatableElement asElement() { result = instance }
   }
 
+  /**
+   * A signature input. This may represent a signature, or a signature component
+   * such as the scalar values r and s in ECDSA.
+   */
+  final class SignatureArtifactNode extends ArtifactNode, TSignatureInput {
+    SignatureArtifactConsumer instance;
+
+    SignatureArtifactNode() { this = TSignatureInput(instance) }
+
+    final override string getInternalType() { result = "SignatureInput" }
+
+    override LocatableElement asElement() { result = instance }
+  }
+
   /**
    * A salt input.
    */