Fixed QL for QL findings

Author: fegge

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

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

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

@@ -1,6 +1,8 @@
-import java
-import OperationInstances
-import FlowAnalysis
+private import java
+private import experimental.quantum.Language
+private import AlgorithmValueConsumers
+private import OperationInstances
+private import FlowAnalysis
 
 /**
  * A string literal that represents an elliptic curve name.
@@ -194,34 +196,24 @@ class StatefulSignatureAlgorithmInstance extends SignatureAlgorithmInstance inst
  * A key generation algorithm where the algorithm is implicitly defined by the
  * type.
  */
-abstract class KeyGenerationAlgorithmInstance extends Crypto::KeyOperationAlgorithmInstance,
+abstract class KeyGenerationAlgorithmInstance extends Crypto::AlgorithmInstance,
   KeyGenerationAlgorithmValueConsumer instanceof ClassInstanceExpr
 {
-  override Crypto::ModeOfOperationAlgorithmInstance getModeOfOperationAlgorithm() { none() }
-
-  override Crypto::PaddingAlgorithmInstance getPaddingAlgorithm() { none() }
-
-  override Crypto::ConsumerInputDataFlowNode getKeySizeConsumer() { none() }
-
-  override Crypto::KeyOpAlg::Algorithm getAlgorithmType() {
-    generatorNameToKeySizeAndAlgorithmMapping(this.getRawAlgorithmName(), _, result)
-  }
-
-  override int getKeySizeFixed() {
-    generatorNameToKeySizeAndAlgorithmMapping(this.getRawAlgorithmName(), result, _)
-  }
-
-  override string getRawAlgorithmName() {
-    typeNameToRawAlgorithmNameMapping(super.getConstructedType().getName(), result)
-  }
-
   // Used for data flow from elliptic curve string literals to the algorithm
   // instance.
   DataFlow::Node getParametersInput() { none() }
 
   // Used for data flow from elliptic curve string literals to the algorithm
   // instance.
   DataFlow::Node getEllipticCurveInput() { none() }
+
+  string getRawAlgorithmName() {
+    typeNameToRawAlgorithmNameMapping(super.getConstructedType().getName(), result)
+  }
+
+  int getKeySizeFixed() {
+    generatorNameToKeySizeAndAlgorithmMapping(this.getRawAlgorithmName(), result, _)
+  }
 }
 
 /**
@@ -267,22 +259,6 @@ class GenericEllipticCurveKeyGenerationAlgorithmInstance extends KeyGenerationAl
     super.getConstructedType().getName().matches("EC%")
   }
 
-  override string getRawAlgorithmName() {
-    // TODO: The generator constructs an elliptic curve key pair. The curve used
-    // is determined using data flow. If this fails we would like to report
-    // something useful, so we use "UnknownCurve". However, this should probably
-    // be handled at the node layer.
-    if exists(this.getConsumedEllipticCurve())
-    then result = this.getConsumedEllipticCurve().getRawEllipticCurveName()
-    else result = "UnknownCurve"
-  }
-
-  override Crypto::KeyOpAlg::Algorithm getAlgorithmType() {
-    // TODO: There is currently to algorithm type for elliptic curve key
-    // generation.
-    result = Crypto::KeyOpAlg::TUnknownKeyOperationAlgorithmType()
-  }
-
   override Crypto::AlgorithmValueConsumer getEllipticCurveConsumer() {
     // The elliptic curve is resolved recursively from the parameters passed to
     // the `init()` call.
@@ -308,18 +284,6 @@ class StatefulSignatureKeyGenerationAlgorithmInstance extends KeyGenerationAlgor
     super.getConstructedType() instanceof Generators::KeyGenerator and
     super.getConstructedType().getName().matches(["LMS%", "HSS%"])
   }
-
-  override string getRawAlgorithmName() {
-    typeNameToRawAlgorithmNameMapping(super.getConstructedType().getName(), result)
-  }
-
-  override Crypto::KeyOpAlg::Algorithm getAlgorithmType() {
-    super.getConstructedType().getName().matches("LMS%") and
-    result = Crypto::KeyOpAlg::TSignature(Crypto::KeyOpAlg::LMS())
-    or
-    super.getConstructedType().getName().matches("HSS%") and
-    result = Crypto::KeyOpAlg::TSignature(Crypto::KeyOpAlg::HSS())
-  }
 }
 
 /**

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

@@ -1,5 +1,7 @@
-import java
-import AlgorithmInstances
+private import java
+private import experimental.quantum.Language
+private import AlgorithmValueConsumers
+private import AlgorithmInstances
 
 abstract class EllipticCurveAlgorithmValueConsumer extends Crypto::AlgorithmValueConsumer { }
 

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

@@ -1,7 +1,7 @@
-import java
-import semmle.code.java.dataflow.DataFlow
-import experimental.quantum.Language
-import AlgorithmValueConsumers
+private import java
+private import semmle.code.java.dataflow.DataFlow
+private import experimental.quantum.Language
+private import AlgorithmValueConsumers
 
 /**
  * A signature for the `getInstance()` method calls used in JCA, or direct
@@ -46,24 +46,25 @@ signature class UseCallSig instanceof MethodCall {
  * ```
  */
 module NewToInitToUseFlow<NewCallSig New, InitCallSig Init, UseCallSig Use> {
-  newtype TFlowState =
+  private newtype TFlowState =
     TUninitialized() or
     TInitialized(Init call) or
     TIntermediateUse(Use call)
 
-  abstract class InitFlowState extends TFlowState {
+  abstract private class InitFlowState extends TFlowState {
     string toString() {
       this = TUninitialized() and result = "Uninitialized"
       or
       this = TInitialized(_) and result = "Initialized"
-      // TODO: add intermediate use
+      or
+      this = TIntermediateUse(_) and result = "Intermediate"
     }
   }
 
   // The flow state is uninitialized if the `init` call is not yet made.
-  class UninitializedFlowState extends InitFlowState, TUninitialized { }
+  private class UninitializedFlowState extends InitFlowState, TUninitialized { }
 
-  class InitializedFlowState extends InitFlowState, TInitialized {
+  private class InitializedFlowState extends InitFlowState, TInitialized {
     Init call;
     DataFlow::Node node1;
     DataFlow::Node node2; // The receiver of the `init` call
@@ -82,7 +83,7 @@ module NewToInitToUseFlow<NewCallSig New, InitCallSig Init, UseCallSig Use> {
     DataFlow::Node getSndNode() { result = node2 }
   }
 
-  class IntermediateUseState extends InitFlowState, TIntermediateUse {
+  private class IntermediateUseState extends InitFlowState, TIntermediateUse {
     Use call;
     DataFlow::Node node1; // The receiver of the method call
     DataFlow::Node node2;
@@ -101,19 +102,21 @@ module NewToInitToUseFlow<NewCallSig New, InitCallSig Init, UseCallSig Use> {
     DataFlow::Node getSndNode() { result = node2 }
   }
 
-  module NewToInitToUseConfig implements DataFlow::StateConfigSig {
+  private module NewToInitToUseConfig implements DataFlow::StateConfigSig {
     class FlowState = InitFlowState;
 
     predicate isSource(DataFlow::Node src, FlowState state) {
       state instanceof UninitializedFlowState and
       src.asExpr() instanceof New
       or
+      // Needed to determine the init call from a (final) use.
       src = state.(InitializedFlowState).getSndNode()
       or
+      // Needed to determine all intermediate uses from a (final) use.
       src = state.(IntermediateUseState).getSndNode()
     }
 
-    // TODO: document this, but this is intentional (avoid cross products?)
+    // TODO: Document this, but this is intentional (to avoid cross products).
     predicate isSink(DataFlow::Node sink, FlowState state) { none() }
 
     predicate isSink(DataFlow::Node sink) {
@@ -138,15 +141,15 @@ module NewToInitToUseFlow<NewCallSig New, InitCallSig Init, UseCallSig Use> {
     predicate isBarrier(DataFlow::Node node, FlowState state) {
       exists(Init call | node.asExpr() = call.(MethodCall).getQualifier() |
         // Ensures that the receiver of a call to `init` is tracked as initialized.
-        state instanceof UninitializedFlowState
+        not state instanceof InitializedFlowState
         or
         // Ensures that call tracked by the state is the last call to `init`.
         state.(InitializedFlowState).getInitCall() != call
       )
     }
   }
 
-  module NewToInitToUseFlow = DataFlow::GlobalWithState<NewToInitToUseConfig>;
+  private module NewToInitToUseFlow = DataFlow::GlobalWithState<NewToInitToUseConfig>;
 
   New getNewFromUse(Use use, NewToInitToUseFlow::PathNode src, NewToInitToUseFlow::PathNode sink) {
     src.getNode().asExpr() = result and
@@ -222,7 +225,7 @@ module ParametersToInitFlow<NewCallSig New, InitCallSig Init> {
     predicate isSink(DataFlow::Node sink) { exists(Init init | sink = init.getParametersInput()) }
 
     /**
-     * A flow step for parameters created from other parameters.
+     * Holds 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

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

@@ -1,9 +1,9 @@
-import java
+private import java
+private import experimental.quantum.Language
+private import FlowAnalysis
+private import AlgorithmInstances
 
 module Params {
-  import FlowAnalysis
-  import AlgorithmInstances
-
   /**
    * A model of the `Parameters` class in Bouncy Castle.
    */
@@ -17,15 +17,15 @@ module Params {
 
   class Curve extends Class {
     Curve() {
-      this.getPackage().getName() = "org.bouncycastle.math.ec" and
-      this.getName().matches("ECCurve")
+      this.getPackage().hasName("org.bouncycastle.math.ec") and
+      this.hasName("ECCurve")
     }
   }
 
   class KeyParameters extends Parameters {
     KeyParameters() {
-      this.getPackage().getName() =
-        ["org.bouncycastle.crypto.params", "org.bouncycastle.pqc.crypto.lms"] and
+      this.getPackage()
+          .hasName(["org.bouncycastle.crypto.params", "org.bouncycastle.pqc.crypto.lms"]) and
       this.getName().matches(["%KeyParameter", "%KeyParameters"])
     }
   }
@@ -95,7 +95,7 @@ module Params {
   }
 
   /**
-   * The named elliptic curve passed to `X9ECParameters.getCurve()`.
+   * A named elliptic curve passed to `X9ECParameters.getCurve()`.
    */
   class X9ECParametersInstantiation extends ParametersInstantiation {
     X9ECParametersInstantiation() { this.(Expr).getType().getName() = "X9ECParameters" }
@@ -108,7 +108,7 @@ module Params {
   }
 
   /**
-   * The named elliptic curve passed to `ECNamedCurveTable.getParameterSpec()`.
+   * A named elliptic curve passed to `ECNamedCurveTable.getParameterSpec()`.
    */
   class ECNamedCurveParameterSpecInstantiation extends ParametersInstantiation {
     ECNamedCurveParameterSpecInstantiation() {
@@ -135,6 +135,11 @@ module Params {
     override Expr getNonceArg() { result = this.(ConstructorCall).getArgument(2) }
   }
 
+  /**
+   * A `ParametersWithIV` instantiation.
+   *
+   * This type is used to model data flow from a nonce to a cipher operation.
+   */
   class ParametersWithIvInstantiation extends ParametersInstantiation {
     ParametersWithIvInstantiation() {
       this.(ConstructorCall).getConstructedType().getName() = "ParametersWithIV"
@@ -161,9 +166,6 @@ module Params {
  * Models for the signature algorithms defined by the `org.bouncycastle.crypto.signers` package.
  */
 module Signers {
-  import FlowAnalysis
-  import AlgorithmInstances
-
   /**
    * A model of the `Signer` class in Bouncy Castle.
    *
@@ -207,8 +209,8 @@ module Signers {
   }
 
   /**
-   * This class represents signers with a one shot API (where the entire message
-   * is passed to either `generateSignature()` or `verifySignature`.).
+   * A signer with a one shot API (where the entire message is passed to either
+   * `generateSignature()` or `verifySignature`.).
    */
   class OneShotSigner extends Signer {
     OneShotSigner() { this.getName().matches(["DSASigner", "ECDSA%", "LMS%", "HSS%"]) }
@@ -335,9 +337,6 @@ module Signers {
  * Models for the key generation algorithms defined by the `org.bouncycastle.crypto.generators` package.
  */
 module Generators {
-  import FlowAnalysis
-  import AlgorithmInstances
-
   /**
    * A model of the `KeyGenerator` and `KeyPairGenerator` classes in Bouncy Castle.
    */
@@ -366,6 +365,8 @@ module Generators {
   }
 
   /**
+   * An asymmetric key pair.
+   *
    * This type is used to model data flow from a key pair to the private and
    * public components of the key pair.
    */
@@ -391,13 +392,13 @@ module Generators {
   private class KeyGeneratorNewCall = KeyGenerationAlgorithmInstance;
 
   /**
+   * A call to a key generator `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 KeyGeneratorInitCall extends MethodCall {
-    KeyGenerator gen;
-
-    KeyGeneratorInitCall() { this = gen.getAnInitCall() }
+    KeyGeneratorInitCall() { this = any(KeyGenerator gen).getAnInitCall() }
 
     Crypto::ConsumerInputDataFlowNode getKeySizeConsumer() { none() }
 
@@ -435,7 +436,8 @@ module Generators {
   class KeyGenerationOperationInstance extends Crypto::KeyGenerationOperationInstance instanceof KeyGeneratorUseCall
   {
     override Crypto::AlgorithmValueConsumer getAnAlgorithmValueConsumer() {
-      // The algorithm is implicitly defined by the key generator type
+      // The algorithm is implicitly defined by the key generator type, which is
+      // determined by the constructor call.
       result = KeyGeneratorFlow::getNewFromUse(this, _, _)
     }
 
@@ -529,6 +531,9 @@ module Modes {
     }
   }
 
+  /**
+   * A block cipher engine, like `AESEngine`.
+   */
   class BlockCipher extends Class {
     BlockCipher() {
       this.getPackage().getName() = "org.bouncycastle.crypto.engines" and
@@ -605,9 +610,7 @@ module Modes {
    * decrypt data.
    */
   private class BlockCipherModeUseCall extends MethodCall {
-    BlockCipherMode mode;
-
-    BlockCipherModeUseCall() { this = mode.getAUseCall() }
+    BlockCipherModeUseCall() { this = any(BlockCipherMode mode).getAUseCall() }
 
     predicate isIntermediate() { not this.getCallee().getName() = "doFinal" }