Crypto: Fixed issues in CBOM representations (gaps in the underlying model) and simplified unit tests in terms of the graph complexity to aid visual assessments of model correctness.

Author: bdrodes

cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/KnownAlgorithmConstants.qll

@@ -171,7 +171,10 @@ class KnownOpenSslKeyAgreementAlgorithmExpr extends Expr instanceof KnownOpenSsl
 }
 
 predicate knownOpenSslAlgorithmOperationCall(Call c, string normalized, string algType) {
-  c.getTarget().getName() in ["EVP_RSA_gen", "RSA_generate_key_ex", "RSA_generate_key", "RSA_new"] and
+  c.getTarget().getName() in [
+      "EVP_RSA_gen", "RSA_generate_key_ex", "RSA_generate_key", "RSA_new", "RSA_sign", "RSA_verify",
+      "EVP_PKEY_get1_RSA"
+    ] and
   normalized = "RSA" and
   algType = "ASYMMETRIC_ENCRYPTION"
 }

cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/HashAlgorithmValueConsumer.qll

@@ -87,3 +87,20 @@ class EvpDigestAlgorithmValueConsumer extends HashAlgorithmValueConsumer {
     exists(OpenSslAlgorithmInstance i | i.getAvc() = this and result = i)
   }
 }
+
+class RsaSignOrVerifyHashAlgorithmValueConsumer extends HashAlgorithmValueConsumer {
+  DataFlow::Node valueArgNode;
+
+  RsaSignOrVerifyHashAlgorithmValueConsumer() {
+    this.(Call).getTarget().getName() in ["RSA_sign", "RSA_verify"] and
+    valueArgNode.asExpr() = this.(Call).getArgument(0)
+  }
+
+  override DataFlow::Node getResultNode() { none() }
+
+  override Crypto::ConsumerInputDataFlowNode getInputNode() { result = valueArgNode }
+
+  override Crypto::AlgorithmInstance getAKnownAlgorithmSource() {
+    exists(OpenSslAlgorithmInstance i | i.getAvc() = this and result = i)
+  }
+}

cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/PaddingAlgorithmValueConsumer.qll

@@ -14,7 +14,7 @@ class Evp_PKey_Ctx_set_rsa_padding_AlgorithmValueConsumer extends PaddingAlgorit
   DataFlow::Node resultNode;
 
   Evp_PKey_Ctx_set_rsa_padding_AlgorithmValueConsumer() {
-    resultNode.asExpr() = this and
+    resultNode.asExpr() = this.(Call).getArgument(0) and
     this.(Call).getTarget().getName() = "EVP_PKEY_CTX_set_rsa_padding" and
     valueArgNode.asExpr() = this.(Call).getArgument(1)
   }

cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/SignatureAlgorithmValueConsumer.qll

@@ -18,7 +18,7 @@ class EvpSignatureAlgorithmValueConsumer extends SignatureAlgorithmValueConsumer
       this.(Call).getTarget().getName() = "EVP_SIGNATURE_fetch" and
       valueArgNode.asExpr() = this.(Call).getArgument(1)
       // EVP_PKEY_get1_DSA, EVP_PKEY_get1_RSA
-      // DSA_SIG_new, DSA_SIG_get0, RSA_sign ?
+      // DSA_SIG_new, DSA_SIG_get0 ?
     )
   }
 

cpp/ql/lib/experimental/quantum/OpenSSL/Operations/EVPPKeyCtxInitializer.qll

@@ -1,5 +1,10 @@
 /**
  * Initializers for EVP PKey
+ * These are used to create a Pkey context or set properties on a Pkey context
+ * e.g., key size, hash algorithms, curves, padding schemes, etc.
+ * Meant to capture more general purpose initializers that aren't necessarily
+ * tied to a specific operation. If tied to an operation (i.e., in the docs)
+ * we recommend defining defining all together in the same operation definition qll.
  * including:
  *  https://docs.openssl.org/3.0/man3/EVP_PKEY_CTX_ctrl/
  *  https://docs.openssl.org/3.0/man3/EVP_EncryptInit/#synopsis
@@ -195,3 +200,26 @@ class EvpCtxSetSaltLengthInitializer extends OperationStep {
 
   override OperationStepType getStepType() { result = InitializerStep() }
 }
+
+/**
+ * A call to `EVP_PKEY_get1_RSA` or `EVP_PKEY_get1_DSA`
+ *  - RSA *EVP_PKEY_get1_RSA(EVP_PKEY *pkey);
+ *  - DSA *EVP_PKEY_get1_DSA(EVP_PKEY *pkey);
+ */
+class EvpPkeyGet1RsaOrDsa extends OperationStep {
+  EvpPkeyGet1RsaOrDsa() { this.getTarget().getName() = ["EVP_PKEY_get1_RSA", "EVP_PKEY_get1_DSA"] }
+
+  override DataFlow::Node getOutput(IOType type) { result.asExpr() = this and type = KeyIO() }
+
+  override DataFlow::Node getInput(IOType type) {
+    // Key being loaded or created from another location
+    result.asExpr() = this.getArgument(0) and type = KeyIO()
+  }
+
+  /**
+   * Consider this to be an intialization step. A key is accepted and a different key is produced.
+   * It doesn't create a new context or new key. It isn't quite an initialiation for an operaiton
+   * either.
+   */
+  override OperationStepType getStepType() { result = InitializerStep() }
+}

cpp/ql/lib/experimental/quantum/OpenSSL/Operations/KeyGenOperation.qll

@@ -149,21 +149,16 @@ class EvpNewMacKey extends KeyGenFinalOperationStep {
   EvpNewMacKey() { this.getTarget().getName() = "EVP_PKEY_new_mac_key" }
 
   override DataFlow::Node getInput(IOType type) {
-    result.asExpr() = this.getArgument(0) and type = ContextIO()
-    or
     // the raw key that is configured into the output key
     result.asExpr() = this.getArgument(2) and type = KeyIO()
     or
     result.asExpr() = this.getArgument(3) and type = KeySizeIO()
   }
 
-  override DataFlow::Node getOutput(IOType type) {
-    result.asExpr() = this and type = KeyIO()
-    or
-    result.asExpr() = this.getArgument(0) and type = ContextIO()
-  }
+  override DataFlow::Node getOutput(IOType type) { result.asExpr() = this and type = KeyIO() }
 }
 
+
 /// TODO: https://docs.openssl.org/3.0/man3/EVP_PKEY_new/#synopsis
 /**
  * An `KeyGenerationOperationInstance` for the for all key gen final operation steps.

cpp/ql/lib/experimental/quantum/OpenSSL/Operations/OpenSSLOperationBase.qll

@@ -75,11 +75,13 @@ newtype TIOType =
   PaddingAlgorithmIO() or
   // Plaintext also includes a message for digest, signature, verification, and mac generation
   PlaintextIO() or
+  PlaintextSizeIO() or
   PrimaryAlgorithmIO() or
   RandomSourceIO() or
   SaltLengthIO() or
   SeedIO() or
-  SignatureIO()
+  SignatureIO() or
+  SignatureSizeIO()
 
 private string ioTypeToString(TIOType t) {
   t = CiphertextIO() and result = "CiphertextIO"
@@ -108,6 +110,8 @@ private string ioTypeToString(TIOType t) {
   or
   t = PlaintextIO() and result = "PlaintextIO"
   or
+  t = PlaintextSizeIO() and result = "PlaintextSizeIO"
+  or
   t = PrimaryAlgorithmIO() and result = "PrimaryAlgorithmIO"
   or
   t = RandomSourceIO() and result = "RandomSourceIO"
@@ -117,6 +121,8 @@ private string ioTypeToString(TIOType t) {
   t = SeedIO() and result = "SeedIO"
   or
   t = SignatureIO() and result = "SignatureIO"
+  or
+  t = SignatureSizeIO() and result = "SignatureSizeIO"
 }
 
 class IOType extends TIOType {
@@ -131,8 +137,9 @@ class IOType extends TIOType {
  * The type of step in an `OperationStep`.
  * - `ContextCreationStep`: the creation of a context from an algorithm or key.
  *                       for example `EVP_MD_CTX_create(EVP_sha256())` or `EVP_PKEY_CTX_new(pkey, NULL)`
- * - `InitializerStep`: the initialization of an operation through some sort of shared/accumulated context
- *                       for example `EVP_DigestInit_ex(ctx, EVP_sha256(), NULL)`
+ * - `InitializerStep`: the initialization of an operation or state through some sort of shared/accumulated context
+ *                       for example `EVP_DigestInit_ex(ctx, EVP_sha256(), NULL)`, may also be used for pass through
+ *                       configuration, for example `EVP_PKEY_get1_RSA(key)` where a pkey is input into an RSA key return.
  * - `UpdateStep`: any operation that has and update/final paradigm, the update represents an intermediate step in an operation,
  *                       such as `EVP_DigestUpdate(ctx, data, len)`
  * - `FinalStep`: an ultimate operation step. This may be an explicit 'final' in an update/final paradigm, but not necessarily.
@@ -249,8 +256,9 @@ abstract class OperationStep extends Call {
   /**
    * Gets an AVC for the primary algorithm for this operation.
    * A primary algorithm is an AVC that either:
+   * 0) `this` is an AVC (consider direct algorithm consumers like RSA_sign (algorithm is implicit) or EVP_PKEY_new_mac_key (NID is first arg) )
    * 1) flows to a ctx input directly or
-   * 2) flows to a primary algorithm input directly
+   * 2) flows to a primary algorithm input directly or
    * 3) flows to a key input directly (algorithm held in a key will be considered primary)
    * See `AvcContextCreationStep` for details about resetting scenarios.
    * Gets the first OperationStep an AVC flows to. If a context input,
@@ -259,6 +267,8 @@ abstract class OperationStep extends Call {
    * operation step (dominating operation step, see `getDominatingInitializersToStep`).
    */
   Crypto::AlgorithmValueConsumer getPrimaryAlgorithmValueConsumer() {
+    this instanceof Crypto::AlgorithmValueConsumer and result = this
+    or
     exists(DataFlow::Node src, DataFlow::Node sink, IOType t, OperationStep avcConsumingPred |
       (t = PrimaryAlgorithmIO() or t = ContextIO() or t = KeyIO()) and
       avcConsumingPred.flowsToOperationStep(this) and