Fixed QL for QL findings

Author: fegge

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

@@ -1,4 +1,2 @@
 import java
-import BouncyCastle.AlgorithmInstances
-import BouncyCastle.AlgorithmValueConsumers
 import BouncyCastle.OperationInstances

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

@@ -1,6 +1,4 @@
 import java
-import experimental.quantum.Language
-import AlgorithmValueConsumers
 import OperationInstances
 import FlowAnalysis
 
@@ -31,8 +29,8 @@ class EllipticCurveStringLiteralInstance extends Crypto::EllipticCurveInstance i
 }
 
 /**
- * Represents an elliptic curve algorithm where the elliptic curve is implicitly
- * defined by the underlying type.
+ * An elliptic curve algorithm where the elliptic curve is implicitly defined by
+ * the underlying type.
  */
 abstract class KnownEllipticCurveInstance extends Crypto::EllipticCurveInstance,
   Crypto::EllipticCurveConsumingAlgorithmInstance, Crypto::AlgorithmValueConsumer instanceof ClassInstanceExpr
@@ -51,7 +49,7 @@ abstract class KnownEllipticCurveInstance extends Crypto::EllipticCurveInstance,
 }
 
 /**
- * Signature algorithms where the algorithm is implicitly defined by the type.
+ * A signature algorithm where the algorithm is implicitly defined by the type.
  */
 abstract class SignatureAlgorithmInstance extends Crypto::KeyOperationAlgorithmInstance,
   SignatureAlgorithmValueConsumer instanceof ClassInstanceExpr
@@ -100,10 +98,10 @@ abstract class KnownEllipticCurveSignatureAlgorithmInstance extends KnownEllipti
 }
 
 /**
- * DSA and DSADigest signers.
+ * A DSA or DSADigest signer.
  */
-class DSASignatureAlgorithmInstance extends SignatureAlgorithmInstance instanceof ClassInstanceExpr {
-  DSASignatureAlgorithmInstance() {
+class DsaSignatureAlgorithmInstance extends SignatureAlgorithmInstance instanceof ClassInstanceExpr {
+  DsaSignatureAlgorithmInstance() {
     super.getConstructedType() instanceof Signers::Signer and
     super.getConstructedType().getName().matches("DSA%")
   }
@@ -114,7 +112,7 @@ class DSASignatureAlgorithmInstance extends SignatureAlgorithmInstance instanceo
 }
 
 /**
- * Ed25519, Ed25519ph, and Ed25519ctx signers.
+ * An Ed25519, Ed25519ph, or Ed25519ctx signer.
  */
 class Ed25519SignatureAlgorithmInstance extends KnownEllipticCurveSignatureAlgorithmInstance instanceof ClassInstanceExpr
 {
@@ -131,7 +129,7 @@ class Ed25519SignatureAlgorithmInstance extends KnownEllipticCurveSignatureAlgor
 }
 
 /**
- * Ed448 and Ed448ph signers.
+ * An Ed448 or Ed448ph signer.
  */
 class Ed448SignatureAlgorithmInstance extends KnownEllipticCurveSignatureAlgorithmInstance instanceof ClassInstanceExpr
 {
@@ -148,7 +146,7 @@ class Ed448SignatureAlgorithmInstance extends KnownEllipticCurveSignatureAlgorit
 }
 
 /**
- * ECDSA signers.
+ * An ECDSA signer.
  *
  * ECDSA curve parameters can be set in at least five ways:
  * - By using the `ECDomainParameters` class, which is passed to the constructor of the signer.
@@ -157,9 +155,9 @@ class Ed448SignatureAlgorithmInstance extends KnownEllipticCurveSignatureAlgorit
  * - By using the `ECNamedCurveSpec` class, which is passed to the constructor of the signer.
  * - By using the `ECParameterSpec` class, which is passed to the constructor of the signer.
  */
-class ECDSASignatureAlgorithmInstance extends SignatureAlgorithmInstance instanceof ClassInstanceExpr
+class EcdsaSignatureAlgorithmInstance extends SignatureAlgorithmInstance instanceof ClassInstanceExpr
 {
-  ECDSASignatureAlgorithmInstance() {
+  EcdsaSignatureAlgorithmInstance() {
     super.getConstructedType() instanceof Signers::OneShotSigner and
     super.getConstructedType().getName().matches("ECDSA%")
   }
@@ -176,7 +174,7 @@ class ECDSASignatureAlgorithmInstance extends SignatureAlgorithmInstance instanc
 }
 
 /**
- * An LMS or HSS stateful hash-based signer.
+ * An LMS or HSS stateful, hash-based signer.
  */
 class StatefulSignatureAlgorithmInstance extends SignatureAlgorithmInstance instanceof ClassInstanceExpr
 {
@@ -199,7 +197,7 @@ class StatefulSignatureAlgorithmInstance extends SignatureAlgorithmInstance inst
 }
 
 /**
- * Key generation algorithms where the algorithm is implicitly defined by the
+ * A key generation algorithm where the algorithm is implicitly defined by the
  * type.
  */
 abstract class KeyGenerationAlgorithmInstance extends Crypto::KeyOperationAlgorithmInstance,
@@ -237,9 +235,8 @@ abstract class KeyGenerationAlgorithmInstance extends Crypto::KeyOperationAlgori
 }
 
 /**
- * Represents an elliptic curve key generation algorithm where both the key
- * generation algorithm and elliptic curve are implicitly defined by the
- * underlying type.
+ * An elliptic curve key generation algorithm where both the key generation
+ * algorithm and elliptic curve are implicitly defined by the underlying type.
  */
 abstract class KnownEllipticCurveKeyGenerationAlgorithmInstance extends KnownEllipticCurveInstance,
   KeyGenerationAlgorithmInstance
@@ -270,7 +267,7 @@ class Ed448KeyGenerationAlgorithmInstance extends KnownEllipticCurveKeyGeneratio
 }
 
 /**
- * Represents a generic `ECKeyPairGenerator` instance.
+ * A generic `ECKeyPairGenerator` instance.
  */
 class GenericEllipticCurveKeyGenerationAlgorithmInstance extends KeyGenerationAlgorithmInstance,
   Crypto::EllipticCurveConsumingAlgorithmInstance instanceof ClassInstanceExpr
@@ -312,8 +309,8 @@ class GenericEllipticCurveKeyGenerationAlgorithmInstance extends KeyGenerationAl
 }
 
 /**
- * Represents LMS or HSS key generation instances. The algorithm is implicitly
- * defined by the type.
+ * An LMS or HSS key generation instances. The algorithm is implicitly defined
+ * by the type.
  */
 class StatefulSignatureKeyGenerationAlgorithmInstance extends KeyGenerationAlgorithmInstance instanceof ClassInstanceExpr
 {

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

@@ -1,5 +1,4 @@
 import java
-import experimental.quantum.Language
 import AlgorithmInstances
 
 abstract class HashAlgorithmValueConsumer extends Crypto::AlgorithmValueConsumer { }
@@ -9,9 +8,9 @@ abstract class CipherAlgorithmValueConsumer extends Crypto::AlgorithmValueConsum
 abstract class EllipticCurveAlgorithmValueConsumer extends Crypto::AlgorithmValueConsumer { }
 
 class EllipticCurveStringLiteralArg extends EllipticCurveAlgorithmValueConsumer instanceof Expr {
-  Params::ParametersInstantiation params;
-
-  EllipticCurveStringLiteralArg() { this = params.getAlgorithmArg() }
+  EllipticCurveStringLiteralArg() {
+    this = any(Params::ParametersInstantiation params).getAlgorithmArg()
+  }
 
   override Crypto::ConsumerInputDataFlowNode getInputNode() { result.asExpr() = this }
 
@@ -21,7 +20,8 @@ class EllipticCurveStringLiteralArg extends EllipticCurveAlgorithmValueConsumer
 }
 
 /**
- * Signature algorithms are implicitly defined by the constructor.
+ * The AVC for a signature algorithm where the algorithm is implicitly defined
+ * by the constructor.
  */
 abstract class SignatureAlgorithmValueConsumer extends Crypto::AlgorithmValueConsumer {
   override Crypto::AlgorithmInstance getAKnownAlgorithmSource() { result = this }
@@ -30,7 +30,8 @@ abstract class SignatureAlgorithmValueConsumer extends Crypto::AlgorithmValueCon
 }
 
 /**
- * Key generation algorithms are implicitly defined by the constructor.
+ * The AVC for a key generation algorithm where the algorithm is implicitly
+ * defined by the constructor.
  */
 abstract class KeyGenerationAlgorithmValueConsumer extends Crypto::AlgorithmValueConsumer {
   override Crypto::AlgorithmInstance getAKnownAlgorithmSource() { result = this }

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

@@ -1,7 +1,6 @@
 import java
 import semmle.code.java.dataflow.DataFlow
 import experimental.quantum.Language
-import AlgorithmInstances
 import AlgorithmValueConsumers
 
 /**
@@ -223,7 +222,7 @@ module ParametersToInitFlowAnalysis<NewCallSig New, InitCallSig Init> {
     predicate isSink(DataFlow::Node sink) { exists(Init init | sink = init.getParametersInput()) }
 
     /**
-     * Pass-through for parameters created from other parameters.
+     * A flow step for parameters created from other parameters.
      *
      * As an example, below we want to track the flow from the `X9ECParameters`
      * constructor call to the `keyPairGenerator.init()` call to be able to
@@ -269,6 +268,9 @@ module ParametersToInitFlowAnalysis<NewCallSig New, InitCallSig Init> {
 
   private module ParametersToInitFlow = DataFlow::Global<ParametersToInitConfig>;
 
+  /**
+   * Gets a parameter instantiation from a call to `init()`.
+   */
   New getParametersFromInit(
     Init init, ParametersToInitFlow::PathNode src, ParametersToInitFlow::PathNode sink
   ) {
@@ -279,8 +281,8 @@ module ParametersToInitFlowAnalysis<NewCallSig New, InitCallSig Init> {
 }
 
 /**
- * Model data flow from a key pair to the public and private components of the
- * key pair.
+ * A model for data flow from a key pair to the public and private components of
+ * the key pair.
  */
 class KeyAdditionalFlowSteps extends MethodCall {
   KeyAdditionalFlowSteps() {
@@ -299,30 +301,27 @@ class KeyAdditionalFlowSteps extends MethodCall {
 }
 
 /**
- * Model data flow from an ECDSA signature to the scalars r and s passed to
- * `verifySignature()`. The ECDSA signature is represented as a `BigInteger`
+ * A model for 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() {
+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()`
+    // The ECDSA signature is represented as a `BigInteger` array.
     result.asExpr() = this.getArray()
   }
 
-  /**
-   * The output node is the `BigInteger` element accessed by this array access.
-   */
-  DataFlow::Node getOutputNode() { result.asExpr() = this }
+  DataFlow::Node getOutputNode() {
+    // r or s is the `BigInteger` element accessed by this array access.
+    result.asExpr() = this
+  }
 }
 
 predicate additionalFlowSteps(DataFlow::Node node1, DataFlow::Node node2) {
@@ -331,12 +330,15 @@ predicate additionalFlowSteps(DataFlow::Node node1, DataFlow::Node node2) {
     node2 = fs.getOutputNode()
   )
   or
-  exists(ECDSASignatureAdditionalFlowSteps fs |
+  exists(EcdsaSignatureAdditionalFlowSteps fs |
     node1 = fs.getInputNode() and
     node2 = fs.getOutputNode()
   )
 }
 
+/**
+ * An additional flow step for a cryptographic artifact.
+ */
 class ArtifactAdditionalFlowStep extends AdditionalFlowInputStep {
   DataFlow::Node output;
 
@@ -347,7 +349,7 @@ class ArtifactAdditionalFlowStep extends AdditionalFlowInputStep {
 
 module EllipticCurveStringLiteralToConsumer {
   /**
-   * Flow from a known elliptic curve name to an elliptic curve algorithm consumer.
+   * A flow from a known elliptic curve name to an elliptic curve algorithm consumer.
    */
   module EllipticCurveStringLiteralToAlgorithmValueConsumerConfig implements DataFlow::ConfigSig {
     // NOTE: We do not reference EllipticCurveStringLiteralInstance directly

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

@@ -7,15 +7,15 @@ module Params {
   /**
    * A model of the `Parameters` class in Bouncy Castle.
    */
-  class Parameters extends RefType {
+  class Parameters extends Class {
     Parameters() {
       // Matches `org.bouncycastle.crypto.params`, `org.bouncycastle.asn1.x9`, etc.
       this.getPackage().getName().matches("org.bouncycastle.%") and
       this.getName().matches(["%Parameters", "%ParameterSpec"])
     }
   }
 
-  class Curve extends RefType {
+  class Curve extends Class {
     Curve() {
       this.getPackage().getName() = "org.bouncycastle.math.ec" and
       this.getName().matches("ECCurve")
@@ -126,7 +126,7 @@ module Signers {
    * This class represents a BouncyCastle signer with a streaming API. For signers
    * with a one-shot API, see `OneShotSigner` below.
    */
-  class Signer extends RefType {
+  class Signer extends Class {
     Signer() {
       this.getPackage().getName() =
         ["org.bouncycastle.crypto.signers", "org.bouncycastle.pqc.crypto.lms"] and
@@ -167,7 +167,7 @@ module Signers {
    * is passed to either `generateSignature()` or `verifySignature`.).
    */
   class OneShotSigner extends Signer {
-    OneShotSigner() { this.getName().matches(["ECDSA%", "LMS%", "HSS%"]) }
+    OneShotSigner() { this.getName().matches(["DSASigner", "ECDSA%", "LMS%", "HSS%"]) }
 
     override Expr getMessageArg(MethodCall call) {
       // For ECDSA and LMS, the message is passed directly to `generateSignature()`.
@@ -300,7 +300,7 @@ module Generators {
   /**
    * A model of the `KeyGenerator` and `KeyPairGenerator` classes in Bouncy Castle.
    */
-  class KeyGenerator extends RefType {
+  class KeyGenerator extends Class {
     Crypto::KeyArtifactType type;
 
     KeyGenerator() {
@@ -422,4 +422,73 @@ module Generators {
   }
 }
 
-module Modes { }
+module Modes {
+  import FlowAnalysis
+
+  class BlockCipherMode extends Class {
+    BlockCipherMode() {
+      this.getPackage().getName() = "org.bouncycastle.crypto.modes" and
+      this.getName().matches("%BlockCipher")
+    }
+
+    MethodCall getAnInitCall() { result = this.getAMethodCall("init") }
+
+    MethodCall getAUseCall() {
+      result =
+        this.getAMethodCall([
+            "processBlock", "processBlocks", "returnByte", "processBytes", "doFinal"
+          ])
+    }
+
+    MethodCall getAMethodCall(string name) {
+      result.getCallee().hasQualifiedName(this.getPackage().getName(), this.getName(), name)
+    }
+  }
+
+  /**
+   * A block cipher mode instantiation.
+   *
+   * BouncyCastle algorithms are instantiated by calling the constructor of the
+   * corresponding class, which also represents the algorithm instance.
+   */
+  private class BlockCipherModeNewCall extends ClassInstanceExpr {
+    BlockCipherModeNewCall() { this.getConstructedType() instanceof BlockCipherMode }
+
+    DataFlow::Node getParametersInput() { none() }
+
+    DataFlow::Node getEllipticCurveInput() { none() }
+  }
+
+  /**
+   * A call to a block cipher mode `init()` method.
+   *
+   * The type is instantiated by a constructor call and initialized by a call to
+   * `init()` which takes a single `KeyGenerationParameters` argument.
+   */
+  private class BlockCipherModeInitCall extends MethodCall {
+    BlockCipherMode mode;
+
+    BlockCipherModeInitCall() { this = mode.getAnInitCall() }
+
+    Crypto::ConsumerInputDataFlowNode getKeySizeConsumer() { none() }
+
+    // The `CipherParameters` argument used to configure the cipher.
+    DataFlow::Node getParametersInput() { result.asExpr() = this.getArgument(1) }
+  }
+
+  /**
+   * A `processX()`, `returnByte()` or `doFinal()` method call to encrypt or
+   * decrypt data.
+   */
+  private class BlockCipherModeUseCall extends MethodCall {
+    BlockCipherMode mode;
+
+    BlockCipherModeUseCall() { this = mode.getAUseCall() }
+
+    predicate isIntermediate() { not this.getCallee().getName() = "doFinal" }
+  }
+
+  module BlockCipherModeFlow =
+    NewToInitToUseFlowAnalysis<BlockCipherModeNewCall, BlockCipherModeInitCall,
+      BlockCipherModeUseCall>;
+}