Crypto: Continued refactoring of operation steps and bug fixes.

bdrodes · bdrodes · commit 422352c63208 · 2025-08-26T11:49:26.000-04:00
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/AlgToAVCFlow.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/AlgToAVCFlow.qll
@@ -14,9 +14,13 @@ private import PaddingAlgorithmInstance
  */
 module KnownOpenSslAlgorithmToAlgorithmValueConsumerConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) {
-    source.asExpr() instanceof KnownOpenSslAlgorithmExpr and
+    (
+      source.asExpr() instanceof KnownOpenSslAlgorithmExpr or
+      source.asIndirectExpr() instanceof KnownOpenSslAlgorithmExpr
+    ) and
     // No need to flow direct operations to AVCs
-    not source.asExpr() instanceof OpenSslDirectAlgorithmOperationCall
+    not source.asExpr() instanceof OpenSslDirectAlgorithmOperationCall and
+    not source.asIndirectExpr() instanceof OpenSslDirectAlgorithmOperationCall
   }
 
   predicate isSink(DataFlow::Node sink) {
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/BlockAlgorithmInstance.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/BlockAlgorithmInstance.qll
@@ -53,7 +53,8 @@ class KnownOpenSslBlockModeConstantAlgorithmInstance extends OpenSslAlgorithmIns
       // Sink is an argument to a CipherGetterCall
       sink = getterCall.getInputNode() and
       // Source is `this`
-      src.asExpr() = this and
+      // NOTE: src literals can be ints or strings, so need to consider asExpr and asIndirectExpr
+      this = [src.asExpr(), src.asIndirectExpr()] and
       // This traces to a getter
       KnownOpenSslAlgorithmToAlgorithmValueConsumerFlow::flow(src, sink)
     )
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/CipherAlgorithmInstance.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/CipherAlgorithmInstance.qll
@@ -77,7 +77,8 @@ class KnownOpenSslCipherConstantAlgorithmInstance extends OpenSslAlgorithmInstan
       // Sink is an argument to a CipherGetterCall
       sink = getterCall.getInputNode() and
       // Source is `this`
-      src.asExpr() = this and
+      // NOTE: src literals can be ints or strings, so need to consider asExpr and asIndirectExpr
+      this = [src.asExpr(), src.asIndirectExpr()] and
       // This traces to a getter
       KnownOpenSslAlgorithmToAlgorithmValueConsumerFlow::flow(src, sink)
     )
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/EllipticCurveAlgorithmInstance.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/EllipticCurveAlgorithmInstance.qll
@@ -21,7 +21,8 @@ class KnownOpenSslEllipticCurveConstantAlgorithmInstance extends OpenSslAlgorith
       // Sink is an argument to a CipherGetterCall
       sink = getterCall.getInputNode() and
       // Source is `this`
-      src.asExpr() = this and
+      // NOTE: src literals can be ints or strings, so need to consider asExpr and asIndirectExpr
+      this = [src.asExpr(), src.asIndirectExpr()] and
       // This traces to a getter
       KnownOpenSslAlgorithmToAlgorithmValueConsumerFlow::flow(src, sink)
     )
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/HashAlgorithmInstance.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/HashAlgorithmInstance.qll
@@ -59,7 +59,8 @@ class KnownOpenSslHashConstantAlgorithmInstance extends OpenSslAlgorithmInstance
       // Sink is an argument to a CipherGetterCall
       sink = getterCall.getInputNode() and
       // Source is `this`
-      src.asExpr() = this and
+      // NOTE: src literals can be ints or strings, so need to consider asExpr and asIndirectExpr
+      this = [src.asExpr(), src.asIndirectExpr()] and
       // This traces to a getter
       KnownOpenSslAlgorithmToAlgorithmValueConsumerFlow::flow(src, sink)
     )
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/KeyAgreementAlgorithmInstance.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/KeyAgreementAlgorithmInstance.qll
@@ -37,7 +37,8 @@ class KnownOpenSslKeyAgreementConstantAlgorithmInstance extends OpenSslAlgorithm
       // Sink is an argument to a CipherGetterCall
       sink = getterCall.getInputNode() and
       // Source is `this`
-      src.asExpr() = this and
+      // NOTE: src literals can be ints or strings, so need to consider asExpr and asIndirectExpr
+      this = [src.asExpr(), src.asIndirectExpr()] and
       // This traces to a getter
       KnownOpenSslAlgorithmToAlgorithmValueConsumerFlow::flow(src, sink)
     )
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/KnownAlgorithmConstants.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/KnownAlgorithmConstants.qll
@@ -177,6 +177,10 @@ predicate knownOpenSslAlgorithmOperationCall(Call c, string normalized, string a
     ] and
   normalized = "RSA" and
   algType = "ASYMMETRIC_ENCRYPTION"
+  or
+  c.getTarget().getName() in ["DSA_do_sign", "DSA_do_verify"] and
+  normalized = "DSA" and
+  algType = "SIGNATURE"
 }
 
 /**
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/MACAlgorithmInstance.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/MACAlgorithmInstance.qll
@@ -22,7 +22,8 @@ class KnownOpenSslMacConstantAlgorithmInstance extends OpenSslAlgorithmInstance,
       // Sink is an argument to a CipherGetterCall
       sink = getterCall.getInputNode() and
       // Source is `this`
-      src.asExpr() = this and
+      // NOTE: src literals can be ints or strings, so need to consider asExpr and asIndirectExpr
+      this = [src.asExpr(), src.asIndirectExpr()] and
       // This traces to a getter
       KnownOpenSslAlgorithmToAlgorithmValueConsumerFlow::flow(src, sink)
     )
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/PaddingAlgorithmInstance.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/PaddingAlgorithmInstance.qll
@@ -64,7 +64,8 @@ class KnownOpenSslPaddingConstantAlgorithmInstance extends OpenSslAlgorithmInsta
       // Sink is an argument to a CipherGetterCall
       sink = getterCall.getInputNode() and
       // Source is `this`
-      src.asExpr() = this and
+      // NOTE: src literals can be ints or strings, so need to consider asExpr and asIndirectExpr
+      this = [src.asExpr(), src.asIndirectExpr()] and
       // This traces to a getter
       KnownOpenSslAlgorithmToAlgorithmValueConsumerFlow::flow(src, sink) and
       isPaddingSpecificConsumer = false
@@ -82,7 +83,8 @@ class KnownOpenSslPaddingConstantAlgorithmInstance extends OpenSslAlgorithmInsta
       // Sink is an argument to a CipherGetterCall
       sink = getterCall.getInputNode() and
       // Source is `this`
-      src.asExpr() = this and
+      // NOTE: src literals can be ints or strings, so need to consider asExpr and asIndirectExpr
+      this = [src.asExpr(), src.asIndirectExpr()] and
       // This traces to a padding-specific consumer
       RsaPaddingAlgorithmToPaddingAlgorithmValueConsumerFlow::flow(src, sink)
     ) and
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/SignatureAlgorithmInstance.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/SignatureAlgorithmInstance.qll
@@ -47,7 +47,8 @@ class KnownOpenSslSignatureConstantAlgorithmInstance extends OpenSslAlgorithmIns
       // Sink is an argument to a signature getter call
       sink = getterCall.getInputNode() and
       // Source is `this`
-      src.asExpr() = this and
+      // NOTE: src literals can be ints or strings, so need to consider asExpr and asIndirectExpr
+      this = [src.asExpr(), src.asIndirectExpr()] and
       // This traces to a getter
       KnownOpenSslAlgorithmToAlgorithmValueConsumerFlow::flow(src, sink)
     )
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/CipherAlgorithmValueConsumer.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/CipherAlgorithmValueConsumer.qll
@@ -14,13 +14,15 @@ class EvpCipherAlgorithmValueConsumer extends CipherAlgorithmValueConsumer {
   EvpCipherAlgorithmValueConsumer() {
     resultNode.asIndirectExpr() = this and
     (
-      this.(Call).getTarget().getName() in [
-          "EVP_get_cipherbyname", "EVP_get_cipherbyobj", "EVP_get_cipherbynid"
-        ] and
+      this.(Call).getTarget().getName() in ["EVP_get_cipherbyname", "EVP_get_cipherbyobj"] and
+      valueArgNode.asIndirectExpr() = this.(Call).getArgument(0)
+      or
+      this.(Call).getTarget().getName() = "EVP_get_cipherbynid" and
+      // algorithm is an NID (int), use asExpr()
       valueArgNode.asExpr() = this.(Call).getArgument(0)
       or
       this.(Call).getTarget().getName() in ["EVP_CIPHER_fetch", "EVP_ASYM_CIPHER_fetch"] and
-      valueArgNode.asExpr() = this.(Call).getArgument(1)
+      valueArgNode.asIndirectExpr() = this.(Call).getArgument(1)
     )
   }
 
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/EllipticCurveAlgorithmValueConsumer.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/EllipticCurveAlgorithmValueConsumer.qll
@@ -14,12 +14,17 @@ class EvpEllipticCurveAlgorithmConsumer extends EllipticCurveValueConsumer {
   EvpEllipticCurveAlgorithmConsumer() {
     resultNode.asIndirectExpr() = this.(Call) and // in all cases the result is the return
     (
-      this.(Call).getTarget().getName() in ["EVP_EC_gen", "EC_KEY_new_by_curve_name"] and
+      this.(Call).getTarget().getName() = "EVP_EC_gen" and
+      valueArgNode.asIndirectExpr() = this.(Call).getArgument(0)
+      or
+      this.(Call).getTarget().getName() = "EC_KEY_new_by_curve_name" and
+      // algorithm is an NID (int), use asExpr()
       valueArgNode.asExpr() = this.(Call).getArgument(0)
       or
       this.(Call).getTarget().getName() in [
           "EC_KEY_new_by_curve_name_ex", "EVP_PKEY_CTX_set_ec_paramgen_curve_nid"
         ] and
+      // algorithm is an NID (int), use asExpr
       valueArgNode.asExpr() = this.(Call).getArgument(2)
     )
   }
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/KEMAlgorithmValueConsumer.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/KEMAlgorithmValueConsumer.qll
@@ -14,7 +14,7 @@ class EvpKemAlgorithmValueConsumer extends KemAlgorithmValueConsumer {
     resultNode.asIndirectExpr() = this and
     (
       this.(Call).getTarget().getName() = "EVP_KEM_fetch" and
-      valueArgNode.asExpr() = this.(Call).getArgument(1)
+      valueArgNode.asIndirectExpr() = this.(Call).getArgument(1)
     )
   }
 
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/KeyExchangeAlgorithmValueConsumer.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/KeyExchangeAlgorithmValueConsumer.qll
@@ -14,7 +14,7 @@ class EvpKeyExchangeAlgorithmValueConsumer extends KeyExchangeAlgorithmValueCons
     resultNode.asIndirectExpr() = this and
     (
       this.(Call).getTarget().getName() = "EVP_KEYEXCH_fetch" and
-      valueArgNode.asExpr() = this.(Call).getArgument(1)
+      valueArgNode.asIndirectExpr() = this.(Call).getArgument(1)
     )
   }
 
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/PKeyAlgorithmValueConsumer.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/PKeyAlgorithmValueConsumer.qll
@@ -19,14 +19,16 @@ class EvpPKeyAlgorithmConsumer extends PKeyValueConsumer {
           "EVP_PKEY_CTX_new_id", "EVP_PKEY_new_raw_private_key", "EVP_PKEY_new_raw_public_key",
           "EVP_PKEY_new_mac_key"
         ] and
+      // Algorithm is an int, use asExpr
       valueArgNode.asExpr() = this.(Call).getArgument(0)
       or
       this.(Call).getTarget().getName() in [
           "EVP_PKEY_CTX_new_from_name", "EVP_PKEY_new_raw_private_key_ex",
           "EVP_PKEY_new_raw_public_key_ex", "EVP_PKEY_CTX_ctrl", "EVP_PKEY_CTX_ctrl_uint64",
           "EVP_PKEY_CTX_ctrl_str", "EVP_PKEY_CTX_set_group_name"
         ] and
-      valueArgNode.asExpr() = this.(Call).getArgument(1)
+      // AAlgorithm is a char*, use asIndirectExpr
+      valueArgNode.asIndirectExpr() = this.(Call).getArgument(1)
       or
       // argInd 2 is 'type' which can be RSA, or EC
       // if RSA argInd 3 is the key size, else if EC argInd 3 is the curve name
@@ -38,10 +40,10 @@ class EvpPKeyAlgorithmConsumer extends PKeyValueConsumer {
         // Elliptic curve case
         // If the argInd 3 is a derived type (pointer or array) then assume it is a curve name
         if this.(Call).getArgument(3).getType().getUnderlyingType() instanceof DerivedType
-        then valueArgNode.asExpr() = this.(Call).getArgument(3)
+        then valueArgNode.asIndirectExpr() = this.(Call).getArgument(3)
         else
           // All other cases
-          valueArgNode.asExpr() = this.(Call).getArgument(2)
+          valueArgNode.asIndirectExpr() = this.(Call).getArgument(2)
       )
     )
   }
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/PaddingAlgorithmValueConsumer.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/PaddingAlgorithmValueConsumer.qll
@@ -16,6 +16,7 @@ class Evp_PKey_Ctx_set_rsa_padding_AlgorithmValueConsumer extends PaddingAlgorit
   Evp_PKey_Ctx_set_rsa_padding_AlgorithmValueConsumer() {
     resultNode.asDefiningArgument() = this.(Call).getArgument(0) and
     this.(Call).getTarget().getName() = "EVP_PKEY_CTX_set_rsa_padding" and
+    // algorithm is an int, use asExpr
     valueArgNode.asExpr() = this.(Call).getArgument(1)
   }
 
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/SignatureAlgorithmValueConsumer.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/SignatureAlgorithmValueConsumer.qll
@@ -16,7 +16,7 @@ class EvpSignatureAlgorithmValueConsumer extends SignatureAlgorithmValueConsumer
     (
       // EVP_SIGNATURE
       this.(Call).getTarget().getName() = "EVP_SIGNATURE_fetch" and
-      valueArgNode.asExpr() = this.(Call).getArgument(1)
+      valueArgNode.asIndirectExpr() = this.(Call).getArgument(1)
       // EVP_PKEY_get1_DSA, EVP_PKEY_get1_RSA
       // DSA_SIG_new, DSA_SIG_get0 ?
     )
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/OpenSSL.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/OpenSSL.qll
@@ -4,4 +4,5 @@ module OpenSslModel {
   import Operations.OpenSSLOperations
   import Random
   import GenericSourceCandidateLiteral
+  import ArtifactPassthrough
 }
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/Operations/EVPPKeyCtxInitializer.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/Operations/EVPPKeyCtxInitializer.qll
@@ -31,10 +31,10 @@ class EvpNewKeyCtx extends OperationStep instanceof Call {
   }
 
   override DataFlow::Node getInput(IOType type) {
-    result.asExpr() = keyArg and type = KeyIO()
+    result.asIndirectExpr() = keyArg and type = KeyIO()
     or
     this.getTarget().getName() = "EVP_PKEY_CTX_new_from_pkey" and
-    result.asExpr() = this.getArgument(0) and
+    result.asIndirectExpr() = this.getArgument(0) and
     type = OsslLibContextIO()
   }
 
@@ -203,28 +203,3 @@ 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.asIndirectExpr() = this and type = KeyIO()
-  }
-
-  override DataFlow::Node getInput(IOType type) {
-    // Key being loaded or created from another location
-    result.asIndirectExpr() = 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() }
-}
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/Operations/OpenSSLOperationBase.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/Operations/OpenSSLOperationBase.qll
@@ -224,10 +224,7 @@ abstract class OperationStep extends Call {
    * the operation allows for multiple inputs (like plaintext for cipher update calls before final).
    */
   OperationStep getDominatingInitializersToStep(IOType type) {
-    //exists(IOType sinkInType |
-    //sinkInType = ContextIO() or sinkInType = type |
-    result.flowsToOperationStep(this) and //, sinkInType)
-    //)
+    (result.flowsToOperationStep(this) or result = this) and
     result.setsValue(type) and
     (
       // Do not consider a 'reset' to occur on updates
@@ -432,7 +429,9 @@ private class CtxParamGenCall extends CtxPassThroughCall {
 
   CtxParamGenCall() {
     this.getTarget().getName() = "EVP_PKEY_paramgen" and
-    n1.asExpr() = this.getArgument(0) and
+    //Arg 0 is *ctx
+    n1.asIndirectExpr() = this.getArgument(0) and
+    //Arg 1 is **pkey
     n2.asDefiningArgument() = this.getArgument(1)
   }
 
@@ -464,6 +463,8 @@ module OperationStepFlowConfig implements DataFlow::ConfigSig {
   }
 
   predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
+    node1.(AdditionalFlowInputStep).getOutput() = node2
+    or
     exists(CtxPassThroughCall c | c.getNode1() = node1 and c.getNode2() = node2)
     or
     // Flow only through context and key inputs and outputs
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/Operations/SignatureOperation.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/Operations/SignatureOperation.qll
@@ -465,8 +465,8 @@ class EvpVerify extends SignatureFinalOperation {
  * A call to `RSA_sign` or `RSA_verify`.
  * https://docs.openssl.org/3.0/man3/RSA_sign/
  */
-class RsaSign extends SignatureFinalOperation {
-  RsaSign() { this.getTarget().getName() in ["RSA_sign", "RSA_verify"] }
+class RsaSignorVerify extends SignatureFinalOperation {
+  RsaSignorVerify() { this.getTarget().getName() in ["RSA_sign", "RSA_verify"] }
 
   override DataFlow::Node getInput(IOType type) {
     // Arg 0 is an NID (so asExpr not asIndirectExpr)
@@ -500,6 +500,37 @@ class RsaSign extends SignatureFinalOperation {
   }
 }
 
+/**
+ * A call to `DSA_do_sign` or `DSA_do_verify`
+ */
+class DSADoSignOrVerify extends SignatureFinalOperation {
+  DSADoSignOrVerify() { this.getTarget().getName() in ["DSA_do_sign", "DSA_do_verify"] }
+
+  override DataFlow::Node getInput(IOType type) {
+    result.asIndirectExpr() = this.getArgument(0) and type = PlaintextIO()
+    or
+    result.asExpr() = this.getArgument(1) and type = PlaintextSizeIO()
+    or
+    this.getTarget().getName() = "DSA_do_sign" and
+    result.asIndirectExpr() = this.getArgument(2) and
+    type = KeyIO()
+    or
+    this.getTarget().getName() = "DSA_do_verify" and
+    result.asIndirectExpr() = this.getArgument(2) and
+    type = SignatureIO()
+    or
+    this.getTarget().getName() = "DSA_do_verify" and
+    result.asIndirectExpr() = this.getArgument(3) and
+    type = KeyIO()
+  }
+
+  override DataFlow::Node getOutput(IOType type) {
+    this.getTarget().getName() = "DSA_do_sign" and
+    result.asIndirectExpr() = this and
+    type = SignatureIO()
+  }
+}
+
 /**
  * An instance of a signature operation.
  * This is an OpenSSL specific class that extends the base SignatureOperationInstance.
