Merge pull request #19944 from bdrodes/signature_model_refactor

Crypto: Refactor Model and signatures, fix models, add unit tests

Author: nicolaswill

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

@@ -14,8 +14,8 @@ module CryptoInput implements InputSig<Language::Location> {
     result = node.asExpr() or
     result = node.asParameter() or
     result = node.asVariable() or
-    result = node.asDefiningArgument()
-    // TODO: do we need asIndirectExpr()?
+    result = node.asDefiningArgument() or
+    result = node.asIndirectExpr()
   }
 
   string locationToFileBaseNameAndLineNumberString(Location location) {
@@ -53,7 +53,7 @@ module ArtifactFlowConfig implements DataFlow::ConfigSig {
   }
 }
 
-module ArtifactFlow = DataFlow::Global<ArtifactFlowConfig>;
+module ArtifactFlow = TaintTracking::Global<ArtifactFlowConfig>;
 
 /**
  * An artifact output to node input configuration
@@ -93,7 +93,13 @@ module GenericDataSourceFlow = TaintTracking::Global<GenericDataSourceFlowConfig
 
 private class ConstantDataSource extends Crypto::GenericConstantSourceInstance instanceof OpenSslGenericSourceCandidateLiteral
 {
-  override DataFlow::Node getOutputNode() { result.asExpr() = this }
+  override DataFlow::Node getOutputNode() {
+    // OpenSSL algorithms may be referenced either by string name or by numeric ID:
+    // String names (e.g. "AES-256-CBC") appear in the AST as character pointer
+    // literals. For these we must use `asIndirectExpr`. Numeric IDs (e.g. NID_aes_256_cbc)
+    // appear as integer literals. For these, we must use `asExpr` to get the "value" node.
+    [result.asIndirectExpr(), result.asExpr()] = this
+  }
 
   override predicate flowsTo(Crypto::FlowAwareElement other) {
     // TODO: separate config to avoid blowing up data-flow analysis
@@ -103,28 +109,4 @@ private class ConstantDataSource extends Crypto::GenericConstantSourceInstance i
   override string getAdditionalDescription() { result = this.toString() }
 }
 
-module ArtifactUniversalFlowConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node source) {
-    source = any(Crypto::ArtifactInstance artifact).getOutputNode()
-  }
-
-  predicate isSink(DataFlow::Node sink) {
-    sink = any(Crypto::FlowAwareElement other).getInputNode()
-  }
-
-  predicate isBarrierOut(DataFlow::Node node) {
-    node = any(Crypto::FlowAwareElement element).getInputNode()
-  }
-
-  predicate isBarrierIn(DataFlow::Node node) {
-    node = any(Crypto::FlowAwareElement element).getOutputNode()
-  }
-
-  predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
-    node1.(AdditionalFlowInputStep).getOutput() = node2
-  }
-}
-
-module ArtifactUniversalFlow = DataFlow::Global<ArtifactUniversalFlowConfig>;
-
 import OpenSSL.OpenSSL

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) {
@@ -46,10 +50,12 @@ module KnownOpenSslAlgorithmToAlgorithmValueConsumerConfig implements DataFlow::
 }
 
 module KnownOpenSslAlgorithmToAlgorithmValueConsumerFlow =
-  DataFlow::Global<KnownOpenSslAlgorithmToAlgorithmValueConsumerConfig>;
+  TaintTracking::Global<KnownOpenSslAlgorithmToAlgorithmValueConsumerConfig>;
 
 module RsaPaddingAlgorithmToPaddingAlgorithmValueConsumerConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node source) { source.asExpr() instanceof OpenSslPaddingLiteral }
+  predicate isSource(DataFlow::Node source) {
+    source.asExpr() instanceof OpenSslSpecialPaddingLiteral
+  }
 
   predicate isSink(DataFlow::Node sink) {
     exists(PaddingAlgorithmValueConsumer c | c.getInputNode() = sink)
@@ -61,7 +67,7 @@ module RsaPaddingAlgorithmToPaddingAlgorithmValueConsumerConfig implements DataF
 }
 
 module RsaPaddingAlgorithmToPaddingAlgorithmValueConsumerFlow =
-  DataFlow::Global<RsaPaddingAlgorithmToPaddingAlgorithmValueConsumerConfig>;
+  TaintTracking::Global<RsaPaddingAlgorithmToPaddingAlgorithmValueConsumerConfig>;
 
 class OpenSslAlgorithmAdditionalFlowStep extends AdditionalFlowInputStep {
   OpenSslAlgorithmAdditionalFlowStep() { exists(AlgorithmPassthroughCall c | c.getInNode() = this) }

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)
     )

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

@@ -2,12 +2,10 @@ import cpp
 private import experimental.quantum.Language
 private import KnownAlgorithmConstants
 private import Crypto::KeyOpAlg as KeyOpAlg
-private import OpenSSLAlgorithmInstanceBase
-private import PaddingAlgorithmInstance
-private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumerBase
-private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.DirectAlgorithmValueConsumer
+private import experimental.quantum.OpenSSL.Operations.OpenSSLOperationBase
+private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumers
+private import OpenSSLAlgorithmInstances
 private import AlgToAVCFlow
-private import BlockAlgorithmInstance
 
 /**
  * Given a `KnownOpenSslCipherAlgorithmExpr`, converts this to a cipher family type.
@@ -79,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)
     )
@@ -97,10 +96,13 @@ class KnownOpenSslCipherConstantAlgorithmInstance extends OpenSslAlgorithmInstan
   }
 
   override Crypto::PaddingAlgorithmInstance getPaddingAlgorithm() {
-    //TODO: the padding is either self, or it flows through getter ctx to a set padding call
-    // like EVP_PKEY_CTX_set_rsa_padding
     result = this
-    // TODO or trace through getter ctx to set padding
+    or
+    exists(OperationStep s |
+      this.getAvc().(AvcContextCreationStep).flowsToOperationStep(s) and
+      s.getAlgorithmValueConsumerForInput(PaddingAlgorithmIO()) =
+        result.(OpenSslAlgorithmInstance).getAvc()
+    )
   }
 
   override string getRawAlgorithmName() {

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)
     )

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)
     )

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)
     )

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

@@ -171,9 +171,15 @@ 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"
+    ] and
   normalized = "RSA" and
   algType = "ASYMMETRIC_ENCRYPTION"
+  or
+  c.getTarget().getName() in ["DSA_do_sign", "DSA_do_verify"] and
+  normalized = "DSA" and
+  algType = "SIGNATURE"
 }
 
 /**

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

@@ -2,12 +2,13 @@ import cpp
 private import experimental.quantum.Language
 private import KnownAlgorithmConstants
 private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumers
-private import experimental.quantum.OpenSSL.AlgorithmInstances.OpenSSLAlgorithmInstanceBase
+private import experimental.quantum.OpenSSL.AlgorithmInstances.OpenSSLAlgorithmInstances
 private import experimental.quantum.OpenSSL.Operations.OpenSSLOperations
+private import Crypto::KeyOpAlg as KeyOpAlg
 private import AlgToAVCFlow
 
 class KnownOpenSslMacConstantAlgorithmInstance extends OpenSslAlgorithmInstance,
-  Crypto::MacAlgorithmInstance instanceof KnownOpenSslMacAlgorithmExpr
+  Crypto::KeyOperationAlgorithmInstance instanceof KnownOpenSslMacAlgorithmExpr
 {
   OpenSslAlgorithmValueConsumer getterCall;
 
@@ -21,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)
     )
@@ -33,17 +35,34 @@ class KnownOpenSslMacConstantAlgorithmInstance extends OpenSslAlgorithmInstance,
 
   override OpenSslAlgorithmValueConsumer getAvc() { result = getterCall }
 
-  override string getRawMacAlgorithmName() {
+  override string getRawAlgorithmName() {
     result = this.(Literal).getValue().toString()
     or
     result = this.(Call).getTarget().getName()
   }
 
-  override Crypto::MacType getMacType() {
-    this instanceof KnownOpenSslHMacAlgorithmExpr and result = Crypto::HMAC()
-    or
-    this instanceof KnownOpenSslCMacAlgorithmExpr and result = Crypto::CMAC()
+  override Crypto::KeyOpAlg::AlgorithmType getAlgorithmType() {
+    if this instanceof KnownOpenSslHMacAlgorithmExpr
+    then result = KeyOpAlg::TMac(KeyOpAlg::HMAC())
+    else
+      if this instanceof KnownOpenSslCMacAlgorithmExpr
+      then result = KeyOpAlg::TMac(KeyOpAlg::CMAC())
+      else result = KeyOpAlg::TMac(KeyOpAlg::OtherMacAlgorithmType())
+  }
+
+  override Crypto::ConsumerInputDataFlowNode getKeySizeConsumer() {
+    // TODO: trace to any key size initializer?
+    none()
+  }
+
+  override int getKeySizeFixed() {
+    // TODO: are there known fixed key sizes to consider?
+    none()
   }
+
+  override Crypto::ModeOfOperationAlgorithmInstance getModeOfOperationAlgorithm() { none() }
+
+  override Crypto::PaddingAlgorithmInstance getPaddingAlgorithm() { none() }
 }
 
 class KnownOpenSslHMacConstantAlgorithmInstance extends Crypto::HmacAlgorithmInstance,
@@ -60,9 +79,13 @@ class KnownOpenSslHMacConstantAlgorithmInstance extends Crypto::HmacAlgorithmIns
       // where the current AVC traces to a HashAlgorithmIO consuming operation step.
       // TODO: need to consider getting reset values, tracing down to the first set for now
       exists(OperationStep s, AvcContextCreationStep avc |
-        avc = this.getAvc() and
+        avc = super.getAvc() and
         avc.flowsToOperationStep(s) and
         s.getAlgorithmValueConsumerForInput(HashAlgorithmIO()) = result
       )
   }
+
+  override Crypto::ModeOfOperationAlgorithmInstance getModeOfOperationAlgorithm() { none() }
+
+  override Crypto::PaddingAlgorithmInstance getPaddingAlgorithm() { none() }
 }