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

@@ -424,6 +424,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.
    */
@@ -458,6 +469,8 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
     }
 
     override DataFlowNode getOutputNode() { result = creator.getOutputArtifact() }
+
+    KeyOperationInstance getCreator() { result = creator }
   }
 
   /**
@@ -783,25 +796,14 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
   }
 
   /**
-   * The output artifact from a signature operation, representing a signature
-   * that is either generated or verified.
-   */
-  abstract class SignatureArtifactInstance extends KeyOperationOutputArtifactInstance { }
-
-  /**
-   * A key operation instance representing the generation or verification of a
-   * signature.
+   * A key operation instance representing a signature being generated or verified.
    */
   abstract class SignatureOperationInstance extends KeyOperationInstance {
     /**
-     * Gets the consumer of the signature input for this operation. This is
-     * typically a signature that is being verified against a message.
+     * Gets the consumer of the signature that is being verified in case of a
+     * verification operation.
      */
-    abstract ConsumerInputDataFlowNode getSignatureArtifactConsumer();
-
-    final SignatureArtifactInstance getSignatureOutputArtifact() {
-      result.getOutputNode() = this.getOutputArtifact()
-    }
+    abstract ConsumerInputDataFlowNode getSignatureConsumer();
   }
 
   /**
@@ -1286,6 +1288,7 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
     TNonceInput(NonceArtifactConsumer e) or
     TMessageInput(MessageArtifactConsumer e) or
     TSaltInput(SaltArtifactConsumer e) or
+    TSignatureInput(SignatureArtifactConsumer e) or
     TRandomNumberGeneration(RandomNumberGenerationInstance e) { e.flowsTo(_) } or
     // Key Creation Operation union type (e.g., key generation, key load)
     TKeyCreationOperation(KeyCreationOperationInstance e) or
@@ -1295,7 +1298,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
@@ -1348,14 +1350,14 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
     /**
      * Returns the child of this node with the given edge name.
      *
-     * This predicate is overriden by derived classes to construct the graph of cryptographic operations.
+     * This predicate is overridden by derived classes to construct the graph of cryptographic operations.
      */
     NodeBase getChild(string edgeName) { none() }
 
     /**
      * Defines properties of this node by name and either a value or location or both.
      *
-     * This predicate is overriden by derived classes to construct the graph of cryptographic operations.
+     * This predicate is overridden by derived classes to construct the graph of cryptographic operations.
      */
     predicate properties(string key, string value, Location location) { none() }
 
@@ -1528,6 +1530,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.
    */
@@ -1551,23 +1567,20 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
 
     KeyOperationOutputNode() { this = TKeyOperationOutput(instance) }
 
-    final override string getInternalType() { result = "KeyOperationOutput" }
+    override string getInternalType() { result = "KeyOperationOutput" }
 
     override LocatableElement asElement() { result = instance }
 
     override string getSourceNodeRelationship() { none() }
   }
 
-  class SignatureArtifactNode extends ArtifactNode, TKeyOperationOutput {
-    SignatureArtifactInstance instance;
-
-    SignatureArtifactNode() { this = TKeyOperationOutput(instance) }
-
-    final override string getInternalType() { result = "Signature" }
-
-    override LocatableElement asElement() { result = instance }
+  class SignOperationOutputNode extends KeyOperationOutputNode {
+    SignOperationOutputNode() {
+      this.asElement().(KeyOperationOutputArtifactInstance).getCreator().getKeyOperationSubtype() =
+        TSignMode()
+    }
 
-    override string getSourceNodeRelationship() { none() }
+    override string getInternalType() { result = "SignatureOutput" }
   }
 
   /**
@@ -2153,10 +2166,18 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
 
     override string getInternalType() { result = nodeName }
 
-    SignatureArtifactNode getSignatureArtifact() {
-      result.asElement() = instance.getOutputArtifactInstance()
+    SignatureArtifactNode getASignatureArtifact() {
+      result.asElement() = instance.getSignatureConsumer().getConsumer()
+    }
+
+    override NodeBase getChild(string key) {
+      result = super.getChild(key)
       or
-      result.asElement() = instance.getSignatureArtifactConsumer().getConsumer()
+      // [KNOWN_OR_UNKNOWN]
+      key = "Signature" and
+      if exists(this.getASignatureArtifact())
+      then result = this.getASignatureArtifact()
+      else result = this
     }
   }