Add AsymmetricAlgorithmNode, refactor and address feedback

Author: nicolaswill

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

@@ -42,19 +42,15 @@ module CryptoInput implements InputSig<Language::Location> {
   }
 }
 
-/**
- * Instantiate the model
- */
+// Instantiate the `CryptographyBase` module
 module Crypto = CryptographyBase<Language::Location, CryptoInput>;
 
-/**
- * Definitions of various generic data sources
- */
+// Definitions of various generic sources
 final class DefaultFlowSource = SourceNode;
 
 final class DefaultRemoteFlowSource = RemoteFlowSource;
 
-class GenericUnreferencedParameterSource extends Crypto::GenericUnreferencedParameterSource {
+private class GenericUnreferencedParameterSource extends Crypto::GenericUnreferencedParameterSource {
   GenericUnreferencedParameterSource() {
     exists(Parameter p | this = p and not exists(p.getAnArgument()))
   }
@@ -68,7 +64,7 @@ class GenericUnreferencedParameterSource extends Crypto::GenericUnreferencedPara
   override string getAdditionalDescription() { result = this.toString() }
 }
 
-class GenericLocalDataSource extends Crypto::GenericLocalDataSource {
+private class GenericLocalDataSource extends Crypto::GenericLocalDataSource {
   GenericLocalDataSource() {
     any(DefaultFlowSource src | not src instanceof DefaultRemoteFlowSource).asExpr() = this
   }
@@ -82,7 +78,7 @@ class GenericLocalDataSource extends Crypto::GenericLocalDataSource {
   override string getAdditionalDescription() { result = this.toString() }
 }
 
-class GenericRemoteDataSource extends Crypto::GenericRemoteDataSource {
+private class GenericRemoteDataSource extends Crypto::GenericRemoteDataSource {
   GenericRemoteDataSource() { any(DefaultRemoteFlowSource src).asExpr() = this }
 
   override DataFlow::Node getOutputNode() { result.asExpr() = this }
@@ -94,7 +90,7 @@ class GenericRemoteDataSource extends Crypto::GenericRemoteDataSource {
   override string getAdditionalDescription() { result = this.toString() }
 }
 
-class ConstantDataSource extends Crypto::GenericConstantSourceInstance instanceof Literal {
+private class ConstantDataSource extends Crypto::GenericConstantSourceInstance instanceof Literal {
   ConstantDataSource() {
     // TODO: this is an API specific workaround for JCA, as 'EC' is a constant that may be used
     // where typical algorithms are specified, but EC specifically means set up a
@@ -114,14 +110,14 @@ class ConstantDataSource extends Crypto::GenericConstantSourceInstance instanceo
 }
 
 /**
- * Random number generation, where each instance is modelled as the expression
+ * An instance of random number generation, modelled as the expression
  * tied to an output node (i.e., the result of the source of randomness)
  */
 abstract class RandomnessInstance extends Crypto::RandomNumberGenerationInstance {
   override DataFlow::Node getOutputNode() { result.asExpr() = this }
 }
 
-class SecureRandomnessInstance extends RandomnessInstance {
+private class SecureRandomnessInstance extends RandomnessInstance {
   RandomDataSource source;
 
   SecureRandomnessInstance() {
@@ -132,7 +128,7 @@ class SecureRandomnessInstance extends RandomnessInstance {
   override string getGeneratorName() { result = source.getSourceOfRandomness().getQualifiedName() }
 }
 
-class InsecureRandomnessInstance extends RandomnessInstance {
+private class InsecureRandomnessInstance extends RandomnessInstance {
   RandomDataSource source;
 
   InsecureRandomnessInstance() {
@@ -143,16 +139,18 @@ class InsecureRandomnessInstance extends RandomnessInstance {
 }
 
 /**
- * Artifact output to node input configuration
+ * An additional flow step in generic data-flow configurations.
+ * Where a step is an edge between nodes `n1` and `n2`,
+ * `this` = `n1` and `getOutput()` = `n2`.
+ *
+ * FOR INTERNAL MODELING USE ONLY.
  */
 abstract class AdditionalFlowInputStep extends DataFlow::Node {
   abstract DataFlow::Node getOutput();
 
   final DataFlow::Node getInput() { result = this }
 }
 
-module ArtifactFlow = DataFlow::Global<ArtifactFlowConfig>;
-
 /**
  * Generic data source to node input configuration
  */
@@ -214,5 +212,7 @@ module ArtifactFlowConfig implements DataFlow::ConfigSig {
 
 module GenericDataSourceFlow = TaintTracking::Global<GenericDataSourceFlowConfig>;
 
+module ArtifactFlow = DataFlow::Global<ArtifactFlowConfig>;
+
 // Import library-specific modeling
 import JCA

java/ql/src/experimental/quantum/InventorySlices/KnownAsymmetricAlgorithm.ql

@@ -10,6 +10,5 @@
 import java
 import experimental.quantum.Language
 
-from Crypto::AlgorithmNode a
-where Crypto::isKnownAsymmetricAlgorithm(a)
+from Crypto::AsymmetricAlgorithmNode a
 select a, a.getAlgorithmName()

java/ql/src/experimental/quantum/InventorySlices/KnownAsymmetricOperationAlgorithm.ql

@@ -10,6 +10,6 @@
 import java
 import experimental.quantum.Language
 
-from Crypto::OperationNode op, Crypto::AlgorithmNode a
-where a = op.getAKnownAlgorithm() and Crypto::isKnownAsymmetricAlgorithm(a)
+from Crypto::OperationNode op, Crypto::AsymmetricAlgorithmNode a
+where a = op.getAKnownAlgorithm()
 select op, a.getAlgorithmName()

java/ql/src/experimental/quantum/InventorySlices/LikelyCryptoAPIFunction.ql

@@ -1,6 +1,6 @@
 /**
  * @name Likely crypto API function
- * @description Detects functions that take in crypto configuration parameters but calls are not detected in source.
+ * @description Outputs functions that take in crypto configuration parameters but calls are not detected in source.
  * @id java/quantum/slices/likely-crypto-api-function
  * @kind problem
  * @severity info

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

@@ -3,7 +3,6 @@
  */
 
 import codeql.util.Location
-import codeql.util.Either
 
 signature module InputSig<LocationSig Location> {
   class LocatableElement {
@@ -36,10 +35,20 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
 
   final class DataFlowNode = Input::DataFlowNode;
 
+  /**
+   * A `ConsumerInputDataFlowNode` is a `DataFlowNode` that represents a consumer of data.
+   *
+   * This class is equivalent to `DataFlowNode` but facilitates binding to a `ConsumerElement`.
+   */
   class ConsumerInputDataFlowNode extends DataFlowNode {
     ConsumerElement getConsumer() { result.getInputNode() = this }
   }
 
+  /**
+   * An `ArtifactOutputDataFlowNode` is a `DataFlowNode` that represents the source of a created artifact.
+   *
+   * This class is equivalent to `DataFlowNode` but facilitates binding to an `OutputArtifactInstance`.
+   */
   class ArtifactOutputDataFlowNode extends DataFlowNode {
     OutputArtifactInstance getArtifact() { result.getOutputNode() = this }
   }
@@ -51,19 +60,17 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
   bindingset[root]
   private string getPropertyAsGraphString(NodeBase node, string key, Location root) {
     result =
-      strictconcat(any(string value, Location location, string parsed |
-            node.properties(key, value, location) and
-            (
-              if location = root or location instanceof UnknownLocation
-              then parsed = value
-              else
-                parsed =
-                  "(" + value + "," + Input::locationToFileBaseNameAndLineNumberString(location) +
-                    ")"
-            )
-          |
-            parsed
-          ), ","
+      strictconcat(string value, Location location, string parsed |
+        node.properties(key, value, location) and
+        (
+          if location = root or location instanceof UnknownLocation
+          then parsed = value
+          else
+            parsed =
+              "(" + value + "," + Input::locationToFileBaseNameAndLineNumberString(location) + ")"
+        )
+      |
+        parsed, ","
       )
   }
 
@@ -154,7 +161,7 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
    * CROSS PRODUCT WARNING: Modeling any *other* element that is a `FlowAwareElement` to the same
    * instance in the database will result in every `FlowAwareElement` sharing the output flow.
    */
-  abstract class KnownElement extends LocatableElement {
+  abstract private class KnownElement extends LocatableElement {
     final ConsumerElement getAConsumer() { result.getAKnownSource() = this }
   }
 
@@ -297,6 +304,23 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
     final override ConsumerInputDataFlowNode getInputNode() { result = input }
   }
 
+  /**
+   * An `AlgorithmValueConsumer` (_AVC_) is an element that consumes a value specifying an algorithm.
+   *
+   * Example 1:
+   * `arg0` of `set_algorithm` (`x`) is the AVC for the `ctx.encrypt()` operation.
+   * ```cpp
+   * x = "RSA";
+   * ctx.set_algorithm(x);
+   * ctx.encrypt();
+   * ```
+   *
+   * Example 2:
+   * `encrypt_with_rsa` is concurrently an an operation, an AVC, and an algorithm.
+   * ```cpp
+   * `encrypt_with_rsa();`
+   * ```
+   */
   abstract class AlgorithmValueConsumer extends ConsumerElement {
     /**
      * DO NOT USE.
@@ -324,8 +348,8 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
    * to the artifact it receives, thereby becoming the definitive contextual source for that artifact.
    *
    * Architectural Implications:
-   *   * By directly coupling a consumer with the node that receives an artifact,
-   *     the data flow is fully transparent with the consumer itself serving only as a transparent node.
+   *   * By directly coupling a consumer with the node that receives an artifact, no modeling considerations have to be made
+   *     to provide an interface for identifying the source via the consumer data-flow mechanisms.
    *   * An artifact's properties (such as being a nonce) are not necessarily inherent; they are determined by the context in which the artifact is consumed.
    *     The consumer node is therefore essential in defining these properties for inputs.
    *   * This approach reduces ambiguity by avoiding separate notions of "artifact source" and "consumer", as the node itself encapsulates both roles.
@@ -347,7 +371,7 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
    * A `NonceArtifactConsumer` is always the `NonceArtifactInstance` itself, since data only becomes (i.e., is determined to be)
    * a `NonceArtifactInstance` when it is consumed in a context that expects a nonce (e.g., an argument expecting nonce data).
    *
-   *  In this case, the artifact (nonce) is fully defined by the context in which it is consumed, and the consumer embodies
+   * In this case, the artifact (nonce) is fully defined by the context in which it is consumed, and the consumer embodies
    * that identity without the need for additional differentiation. Without the context a consumer provides, that data could
    * otherwise be any other type of artifact or even simply random data.
    *
@@ -604,7 +628,6 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
       type = TSymmetricCipher(SEED()) and size = 128
     }
 
-    bindingset[type]
     predicate symmetric_cipher_to_name_and_structure(
       TSymmetricCipherType type, string name, CipherStructureType s
     ) {
@@ -651,7 +674,6 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
       s = UnknownCipherStructureType()
     }
 
-    bindingset[type]
     predicate type_to_name(Algorithm type, string name) {
       // Symmetric cipher algorithm
       symmetric_cipher_to_name_and_structure(type.(SymmetricCipherAlgorithm).getType(), name, _)
@@ -1551,6 +1573,20 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
     string toString() { result = super.getAlgorithmName() }
   }
 
+  /**
+   * The subset of algorithm nodes that are known asymmetric algorithm.
+   *
+   * Note: This is not an independent top-level node type.
+   */
+  class AsymmetricAlgorithmNode extends TKeyCreationCandidateAlgorithm instanceof AlgorithmNode {
+    AsymmetricAlgorithmNode() {
+      this instanceof EllipticCurveNode or
+      this.(KeyOperationAlgorithmNode).isAsymmetric()
+    }
+
+    string toString() { result = super.getAlgorithmName() }
+  }
+
   /**
    * A cryptographic key, such as a symmetric key or asymmetric key pair.
    */
@@ -1709,7 +1745,6 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
 
     TMACType getMACType() { result = instance.asAlg().getMACType() }
 
-    bindingset[type]
     final private predicate macToNameMapping(TMACType type, string name) {
       type instanceof THMAC and
       name = "HMAC"
@@ -2102,7 +2137,6 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
      */
     TBlockCipherModeOfOperationType getModeType() { result = instance.getModeType() }
 
-    bindingset[type]
     final private predicate modeToNameMapping(TBlockCipherModeOfOperationType type, string name) {
       type = ECB() and name = "ECB"
       or
@@ -2148,7 +2182,6 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
 
     TPaddingType getPaddingType() { result = instance.getPaddingType() }
 
-    bindingset[type]
     final private predicate paddingToNameMapping(TPaddingType type, string name) {
       type = ANSI_X9_23() and name = "ANSI_X9_23"
       or
@@ -2454,18 +2487,18 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
     // ALL BRAINPOOL CURVES
     keySize in [160, 192, 224, 256, 320, 384, 512] and
     (
-      curveName = "BRAINPOOLP" + keySize.toString() + "R1"
+      curveName = "BRAINPOOLP" + keySize + "R1"
       or
-      curveName = "BRAINPOOLP" + keySize.toString() + "T1"
+      curveName = "BRAINPOOLP" + keySize + "T1"
     )
   }
 
   private predicate isSecCurve(string curveName, int keySize) {
     // ALL SEC CURVES
     keySize in [112, 113, 128, 131, 160, 163, 192, 193, 224, 233, 239, 256, 283, 384, 409, 521, 571] and
     exists(string suff | suff in ["R1", "R2", "K1"] |
-      curveName = "SECT" + keySize.toString() + suff or
-      curveName = "SECP" + keySize.toString() + suff
+      curveName = "SECT" + keySize + suff or
+      curveName = "SECP" + keySize + suff
     )
   }
 
@@ -2475,22 +2508,20 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
     exists(string pre, string suff |
       pre in ["PNB", "ONB", "TNB"] and suff in ["V1", "V2", "V3", "V4", "V5", "W1", "R1"]
     |
-      curveName = "C2" + pre + keySize.toString() + suff
+      curveName = "C2" + pre + keySize + suff
     )
   }
 
   private predicate isPrimeCurve(string curveName, int keySize) {
     // ALL PRIME CURVES
     keySize in [192, 239, 256] and
-    exists(string suff | suff in ["V1", "V2", "V3"] |
-      curveName = "PRIME" + keySize.toString() + suff
-    )
+    exists(string suff | suff in ["V1", "V2", "V3"] | curveName = "PRIME" + keySize + suff)
   }
 
   private predicate isNumsCurve(string curveName, int keySize) {
     // ALL NUMS CURVES
     keySize in [256, 384, 512] and
-    exists(string suff | suff = "T1" | curveName = "NUMSP" + keySize.toString() + suff)
+    exists(string suff | suff = "T1" | curveName = "NUMSP" + keySize + suff)
   }
 
   /**
@@ -2587,10 +2618,4 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
       location = this.getLocation()
     }
   }
-
-  predicate isKnownAsymmetricAlgorithm(AlgorithmNode node) {
-    node instanceof EllipticCurveNode
-    or
-    node instanceof KeyOperationAlgorithmNode and node.(KeyOperationAlgorithmNode).isAsymmetric()
-  }
 }