Crypto: Debug missing hashes associated with HMAC. EVP_PKEY_get1_RSA is now just a passthrough, it is not a known implicit operation call. Some final operations generating null outputs are now removed from possible final operartions (typically used to determine buffer lenghth and not actually performing the operation). Misc. false positive/error fixes and code clean up, and added missing models.

Author: bdrodes

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

@@ -172,8 +172,7 @@ 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", "RSA_sign", "RSA_verify",
-      "EVP_PKEY_get1_RSA"
+      "EVP_RSA_gen", "RSA_generate_key_ex", "RSA_generate_key", "RSA_new", "RSA_sign", "RSA_verify"
     ] and
   normalized = "RSA" and
   algType = "ASYMMETRIC_ENCRYPTION"

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

@@ -36,7 +36,11 @@ abstract class EvpCipherInitializer extends OperationStep {
     // non-constants (e.g., variable accesses, which require data-flow to determine the value)
     // A zero (null) value typically indicates use of this operation step to initialize
     // other out parameters in a multi-step initialization.
-    (exists(result.asExpr().getValue()) implies result.asExpr().getValue().toInt() != 0)
+    (
+      exists(result.asIndirectExpr().getValue())
+      implies
+      result.asIndirectExpr().getValue().toInt() != 0
+    )
   }
 
   override DataFlow::Node getOutput(IOType type) {
@@ -58,11 +62,15 @@ abstract class EvpEXInitializer extends EvpCipherInitializer {
       // non-constants (e.g., variable accesses, which require data-flow to determine the value)
       // A zero (null) value typically indicates use of this operation step to initialize
       // other out parameters in a multi-step initialization.
-      result.asExpr() = this.getArgument(3) and type = KeyIO()
+      result.asIndirectExpr() = this.getArgument(3) and type = KeyIO()
       or
-      result.asExpr() = this.getArgument(4) and type = IVorNonceIO()
+      result.asIndirectExpr() = this.getArgument(4) and type = IVorNonceIO()
     ) and
-    (exists(result.asExpr().getValue()) implies result.asExpr().getValue().toInt() != 0)
+    (
+      exists(result.asIndirectExpr().getValue())
+      implies
+      result.asIndirectExpr().getValue().toInt() != 0
+    )
   }
 }
 
@@ -73,9 +81,9 @@ abstract class EvpEX2Initializer extends EvpCipherInitializer {
   override DataFlow::Node getInput(IOType type) {
     result = super.getInput(type)
     or
-    result.asExpr() = this.getArgument(2) and type = KeyIO()
+    result.asIndirectExpr() = this.getArgument(2) and type = KeyIO()
     or
-    result.asExpr() = this.getArgument(3) and type = IVorNonceIO()
+    result.asIndirectExpr() = this.getArgument(3) and type = IVorNonceIO()
   }
 }
 
@@ -110,6 +118,7 @@ class Evp_Cipher_EX2_or_Simple_Init_Call extends EvpEX2Initializer {
     result = super.getInput(type)
     or
     this.getTarget().getName().toLowerCase().matches("%cipherinit%") and
+    // the key op subtype is an int, use asExpr
     result.asExpr() = this.getArgument(4) and
     type = KeyOperationSubtypeIO()
   }
@@ -129,7 +138,7 @@ class EvpPkeyEncryptDecryptInit extends OperationStep {
   override DataFlow::Node getInput(IOType type) {
     result.asIndirectExpr() = this.getArgument(0) and type = ContextIO()
     or
-    result.asExpr() = this.getArgument(1) and type = OsslParamIO()
+    result.asIndirectExpr() = this.getArgument(1) and type = OsslParamIO()
   }
 
   override DataFlow::Node getOutput(IOType type) {
@@ -145,6 +154,7 @@ class EvpCipherInitSKeyCall extends EvpEX2Initializer {
   override DataFlow::Node getInput(IOType type) {
     result = super.getInput(type)
     or
+    // the key op subtype is an int, use asExpr
     result.asExpr() = this.getArgument(5) and
     type = KeyOperationSubtypeIO()
   }
@@ -163,11 +173,11 @@ class EvpCipherUpdateCall extends OperationStep {
   override DataFlow::Node getInput(IOType type) {
     result.asIndirectExpr() = this.getArgument(0) and type = ContextIO()
     or
-    result.asExpr() = this.getArgument(3) and type = PlaintextIO()
+    result.asIndirectExpr() = this.getArgument(3) and type = PlaintextIO()
   }
 
   override DataFlow::Node getOutput(IOType type) {
-    result.asExpr() = this.getArgument(1) and type = CiphertextIO()
+    result.asDefiningArgument() = this.getArgument(1) and type = CiphertextIO()
     or
     result.asDefiningArgument() = this.getArgument(0) and type = ContextIO()
   }
@@ -184,13 +194,13 @@ class EvpCipherCall extends EvpCipherOperationFinalStep {
   override DataFlow::Node getInput(IOType type) {
     super.getInput(type) = result
     or
-    result.asExpr() = this.getArgument(2) and type = PlaintextIO()
+    result.asIndirectExpr() = this.getArgument(2) and type = PlaintextIO()
   }
 
   override DataFlow::Node getOutput(IOType type) {
     super.getOutput(type) = result
     or
-    result.asExpr() = this.getArgument(1) and type = CiphertextIO()
+    result.asDefiningArgument() = this.getArgument(1) and type = CiphertextIO()
   }
 }
 
@@ -234,13 +244,13 @@ class EvpPKeyCipherOperation extends EvpCipherOperationFinalStep {
   override DataFlow::Node getInput(IOType type) {
     super.getInput(type) = result
     or
-    result.asExpr() = this.getArgument(3) and type = PlaintextIO()
+    result.asIndirectExpr() = this.getArgument(3) and type = PlaintextIO()
   }
 
   override DataFlow::Node getOutput(IOType type) {
     super.getOutput(type) = result
     or
-    result.asExpr() = this.getArgument(1) and
+    result.asDefiningArgument() = this.getArgument(1) and
     type = CiphertextIO() and
     this.getStepType() = FinalStep()
     // TODO: could indicate text lengths here, as well

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

@@ -47,7 +47,7 @@ class EvpDigestUpdateCall extends OperationStep instanceof Call {
   override DataFlow::Node getInput(IOType type) {
     result.asIndirectExpr() = this.getArgument(0) and type = ContextIO()
     or
-    result.asExpr() = this.getArgument(1) and type = PlaintextIO()
+    result.asIndirectExpr() = this.getArgument(1) and type = PlaintextIO()
   }
 
   override DataFlow::Node getOutput(IOType type) {
@@ -70,7 +70,7 @@ class EvpQDigestOperation extends FinalDigestOperation instanceof Call {
     or
     result.asIndirectExpr() = this.getArgument(0) and type = ContextIO()
     or
-    result.asExpr() = this.getArgument(3) and type = PlaintextIO()
+    result.asIndirectExpr() = this.getArgument(3) and type = PlaintextIO()
   }
 
   override DataFlow::Node getOutput(IOType type) {
@@ -87,7 +87,7 @@ class EvpDigestOperation extends FinalDigestOperation instanceof Call {
   override DataFlow::Node getInput(IOType type) {
     result.asIndirectExpr() = this.getArgument(4) and type = PrimaryAlgorithmIO()
     or
-    result.asExpr() = this.getArgument(0) and type = PlaintextIO()
+    result.asIndirectExpr() = this.getArgument(0) and type = PlaintextIO()
   }
 
   override DataFlow::Node getOutput(IOType type) {

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

@@ -16,7 +16,9 @@ class ECKeyGen extends OperationStep instanceof Call {
     result.asIndirectExpr() = this.(Call).getArgument(0) and type = ContextIO()
   }
 
-  override DataFlow::Node getOutput(IOType type) { result.asExpr() = this and type = KeyIO() }
+  override DataFlow::Node getOutput(IOType type) {
+    result.asDefiningArgument() = this and type = KeyIO()
+  }
 
   override OperationStepType getStepType() { result = ContextCreationStep() }
 }
@@ -59,7 +61,7 @@ class EvpPKeyQKeyGen extends KeyGenFinalOperationStep instanceof Call {
   override DataFlow::Node getOutput(IOType type) {
     result.asDefiningArgument() = this.getArgument(0) and type = ContextIO()
     or
-    result.asExpr() = this and type = KeyIO()
+    result.asDefiningArgument() = this and type = KeyIO()
   }
 
   override DataFlow::Node getInput(IOType type) {
@@ -68,15 +70,15 @@ class EvpPKeyQKeyGen extends KeyGenFinalOperationStep instanceof Call {
     // When arg 3 is a derived type, it is a curve name, otherwise it is a key size for RSA if provided
     // and arg 2 is the algorithm type
     this.getArgument(3).getType().getUnderlyingType() instanceof DerivedType and
-    result.asExpr() = this.getArgument(3) and
+    result.asIndirectExpr() = this.getArgument(3) and
     type = PrimaryAlgorithmIO()
     or
     not this.getArgument(3).getType().getUnderlyingType() instanceof DerivedType and
-    result.asExpr() = this.getArgument(2) and
+    result.asIndirectExpr() = this.getArgument(2) and
     type = PrimaryAlgorithmIO()
     or
     not this.getArgument(3).getType().getUnderlyingType() instanceof DerivedType and
-    result.asExpr() = this.getArgument(3) and
+    result.asIndirectExpr() = this.getArgument(3) and
     type = KeySizeIO()
   }
 }
@@ -87,7 +89,9 @@ class EvpPKeyQKeyGen extends KeyGenFinalOperationStep instanceof Call {
 class EvpRsaGen extends KeyGenFinalOperationStep instanceof Call {
   EvpRsaGen() { this.getTarget().getName() = "EVP_RSA_gen" }
 
-  override DataFlow::Node getOutput(IOType type) { result.asExpr() = this and type = KeyIO() }
+  override DataFlow::Node getOutput(IOType type) {
+    result.asDefiningArgument() = this and type = KeyIO()
+  }
 
   override DataFlow::Node getInput(IOType type) {
     result.asExpr() = this.getArgument(0) and type = KeySizeIO()
@@ -100,7 +104,9 @@ class EvpRsaGen extends KeyGenFinalOperationStep instanceof Call {
 class RsaGenerateKey extends KeyGenFinalOperationStep instanceof Call {
   RsaGenerateKey() { this.getTarget().getName() = "RSA_generate_key" }
 
-  override DataFlow::Node getOutput(IOType type) { result.asExpr() = this and type = KeyIO() }
+  override DataFlow::Node getOutput(IOType type) {
+    result.asDefiningArgument() = this and type = KeyIO()
+  }
 
   override DataFlow::Node getInput(IOType type) {
     result.asExpr() = this.getArgument(0) and type = KeySizeIO()
@@ -150,12 +156,14 @@ class EvpNewMacKey extends KeyGenFinalOperationStep {
 
   override DataFlow::Node getInput(IOType type) {
     // the raw key that is configured into the output key
-    result.asExpr() = this.getArgument(2) and type = KeyIO()
+    result.asIndirectExpr() = 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() }
+  override DataFlow::Node getOutput(IOType type) {
+    result.asIndirectExpr() = this and type = KeyIO()
+  }
 }
 
 /// TODO: https://docs.openssl.org/3.0/man3/EVP_PKEY_new/#synopsis

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

@@ -198,15 +198,17 @@ abstract class OperationStep extends Call {
    * If `sink` is `this`, then this holds true.
    */
   predicate flowsToOperationStep(OperationStep sink) {
-    OperationStepFlow::flow(this.getAnOutput(), [sink.getAnInput(), sink.getAnOutput()])
+    sink = this or
+    OperationStepCtxFlow::flow(this.getAnOutput(), [sink.getAnInput(), sink.getAnOutput()])
   }
 
   /**
    * Holds if this operation step flows from the given `OperationStep` (`source`).
    * If `source` is `this`, then this holds true.
    */
   predicate flowsFromOperationStep(OperationStep source) {
-    OperationStepFlow::flow(source.getAnOutput(), [this.getAnInput(), this.getAnOutput()])
+    source = this or
+    OperationStepCtxFlow::flow(source.getAnOutput(), [this.getAnInput(), this.getAnOutput()])
   }
 
   /**
@@ -224,22 +226,17 @@ abstract class OperationStep extends Call {
    * the operation allows for multiple inputs (like plaintext for cipher update calls before final).
    */
   OperationStep getDominatingInitializersToStep(IOType type) {
-    (result.flowsToOperationStep(this) or result = this) and
+    result.flowsToOperationStep(this) and
     result.setsValue(type) and
     (
       // Do not consider a 'reset' to occur on updates
       // but only for resets that are part of the same update/finalize
       // progression (e.g., an update for an unrelated finalize is ignored)
-      result.getStepType() = UpdateStep() and
-      not exists(OperationStep op |
-        result.flowsToOperationStep(op) and
-        op.flowsToOperationStep(this) and
-        op != this and
-        op.getStepType() = FinalStep()
-      )
+      result.getStepType() = UpdateStep()
       or
       not exists(OperationStep reset |
         result != reset and
+        result != this and
         reset.setsValue(type) and
         reset.flowsToOperationStep(this) and
         result.flowsToOperationStep(reset)
@@ -443,7 +440,7 @@ private class CtxParamGenCall extends CtxPassThroughCall {
 /**
  * A flow configuration from any non-final `OperationStep` to any other `OperationStep`.
  */
-module OperationStepFlowConfig implements DataFlow::ConfigSig {
+module OperationStepCtxFlowConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) {
     exists(OperationStep s |
       s.getAnOutput() = source or
@@ -469,7 +466,9 @@ module OperationStepFlowConfig implements DataFlow::ConfigSig {
     or
     // Flow only through context and key inputs and outputs
     // keys and context generally hold unifying context that link multiple steps
+    // Flow only out of finalize operations through key outputs, otherwise stop at final operations
     exists(OperationStep s, IOType inType, IOType outType |
+      (s.getStepType() = FinalStep() implies outType = KeyIO()) and
       (
         inType = ContextIO()
         or
@@ -489,7 +488,7 @@ module OperationStepFlowConfig implements DataFlow::ConfigSig {
   }
 }
 
-module OperationStepFlow = DataFlow::Global<OperationStepFlowConfig>;
+module OperationStepCtxFlow = DataFlow::Global<OperationStepCtxFlowConfig>;
 
 /**
  * A flow from AVC to the first `OperationStep` the AVC reaches as an input.