Crypto: Overhaul/refactor of EVPInitialzers. Update cipher operation to disallow null key and IV on initializers (typically do not represent an actual key or IV).

Author: bdrodes

cpp/ql/lib/experimental/quantum/OpenSSL/AvcFlow.qll

@@ -0,0 +1,19 @@
+import semmle.code.cpp.dataflow.new.DataFlow
+import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumers
+
+/**
+ * Flows from algorithm values to operations, specific to OpenSSL
+ */
+module AvcToCallArgConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    exists(OpenSSLAlgorithmValueConsumer c | c.getResultNode() = source)
+  }
+
+  /**
+   * Trace to any call accepting the algorithm.
+   * NOTE: users must restrict this set to the operations they are interested in.
+   */
+  predicate isSink(DataFlow::Node sink) { exists(Call c | c.getAnArgument() = sink.asExpr()) }
+}
+
+module AvcToCallArgFlow = DataFlow::Global<AvcToCallArgConfig>;

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

@@ -3,60 +3,31 @@ private import experimental.quantum.OpenSSL.CtxFlow
 private import OpenSSLOperationBase
 private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumers
 
-module EncValToInitEncArgConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node source) { source.asExpr().getValue().toInt() in [0, 1] }
-
-  predicate isSink(DataFlow::Node sink) {
-    exists(EVP_Cipher_Initializer initCall | sink.asExpr() = initCall.getOperationSubtypeArg())
-  }
-}
-
-module EncValToInitEncArgFlow = DataFlow::Global<EncValToInitEncArgConfig>;
-
-int getEncConfigValue(Expr e) {
-  exists(EVP_Cipher_Initializer initCall | e = initCall.getOperationSubtypeArg()) and
-  exists(DataFlow::Node a, DataFlow::Node b |
-    EncValToInitEncArgFlow::flow(a, b) and b.asExpr() = e and result = a.asExpr().getValue().toInt()
-  )
-}
-
-bindingset[i]
-Crypto::KeyOperationSubtype intToCipherOperationSubtype(int i) {
-  if i = 0
-  then result instanceof Crypto::TEncryptMode
-  else
-    if i = 1
-    then result instanceof Crypto::TDecryptMode
-    else result instanceof Crypto::TUnknownKeyOperationMode
-}
-
 // TODO: need to add key consumer
 abstract class EVP_Cipher_Initializer extends EvpKeyOperationSubtypeInitializer,
-  EvpAlgorithmInitializer, EvpKeyInitializer, EvpIVInitializer
+  EvpPrimaryAlgorithmInitializer, EvpKeyInitializer, EvpIVInitializer
 {
-  override CtxPointerSource getContextArg() { result = this.(Call).getArgument(0) }
+  override CtxPointerSource getContext() { result = this.(Call).getArgument(0) }
 
   override Expr getAlgorithmArg() { result = this.(Call).getArgument(1) }
-
-  abstract Expr getOperationSubtypeArg();
-
-  override Crypto::KeyOperationSubtype getKeyOperationSubtype() {
-    if this.(Call).getTarget().getName().toLowerCase().matches("%encrypt%")
-    then result instanceof Crypto::TEncryptMode
-    else
-      if this.(Call).getTarget().getName().toLowerCase().matches("%decrypt%")
-      then result instanceof Crypto::TDecryptMode
-      else
-        if exists(getEncConfigValue(this.getOperationSubtypeArg()))
-        then result = intToCipherOperationSubtype(getEncConfigValue(this.getOperationSubtypeArg()))
-        else result instanceof Crypto::TUnknownKeyOperationMode
-  }
 }
 
 abstract class EVP_EX_Initializer extends EVP_Cipher_Initializer {
-  override Expr getKeyArg() { result = this.(Call).getArgument(3) }
+  override Expr getKeyArg() {
+    // Null key indicates the key is not actually set
+    // This pattern can occur during a multi-step initialization
+    // TODO/Note: not flowing 0 to the sink, assuming a direct use of NULL for now
+    result = this.(Call).getArgument(3) and
+    (exists(result.getValue()) implies result.getValue().toInt() != 0)
+  }
 
-  override Expr getIVArg() { result = this.(Call).getArgument(4) }
+  override Expr getIVArg() {
+    // Null IV indicates the IV is not actually set
+    // This occurs given that setting the IV sometimes requires first setting the IV size.
+    // TODO/Note: not flowing 0 to the sink, assuming a direct use of NULL for now
+    result = this.(Call).getArgument(4) and
+    (exists(result.getValue()) implies result.getValue().toInt() != 0)
+  }
 }
 
 abstract class EVP_EX2_Initializer extends EVP_Cipher_Initializer {
@@ -65,19 +36,26 @@ abstract class EVP_EX2_Initializer extends EVP_Cipher_Initializer {
   override Expr getIVArg() { result = this.(Call).getArgument(3) }
 }
 
-class EVP_Cipher_EX_Init_Call extends EVP_EX_Initializer {
-  EVP_Cipher_EX_Init_Call() {
+class EvpCipherEXInitCall extends EVP_EX_Initializer {
+  EvpCipherEXInitCall() {
     this.(Call).getTarget().getName() in [
         "EVP_EncryptInit_ex", "EVP_DecryptInit_ex", "EVP_CipherInit_ex"
       ]
   }
 
-  override Expr getOperationSubtypeArg() {
+  override Expr getKeyOperationSubtypeArg() {
+    // NOTE: for EncryptInit and DecryptInit there is no subtype arg
+    // the subtype is determined automatically by the initializer based on the operation name
     this.(Call).getTarget().getName().toLowerCase().matches("%cipherinit%") and
     result = this.(Call).getArgument(5)
   }
 }
 
+//   if this.(Call).getTarget().getName().toLowerCase().matches("%encrypt%")
+//   then result instanceof Crypto::TEncryptMode
+//   else
+//     if this.(Call).getTarget().getName().toLowerCase().matches("%decrypt%")
+//     then result instanceof Crypto::TDecryptMode
 class EVP_Cipher_EX2_or_Simple_Init_Call extends EVP_EX2_Initializer {
   EVP_Cipher_EX2_or_Simple_Init_Call() {
     this.(Call).getTarget().getName() in [
@@ -86,7 +64,7 @@ class EVP_Cipher_EX2_or_Simple_Init_Call extends EVP_EX2_Initializer {
       ]
   }
 
-  override Expr getOperationSubtypeArg() {
+  override Expr getKeyOperationSubtypeArg() {
     this.(Call).getTarget().getName().toLowerCase().matches("%cipherinit%") and
     result = this.(Call).getArgument(4)
   }
@@ -95,7 +73,7 @@ class EVP_Cipher_EX2_or_Simple_Init_Call extends EVP_EX2_Initializer {
 class EVP_CipherInit_SKEY_Call extends EVP_EX2_Initializer {
   EVP_CipherInit_SKEY_Call() { this.(Call).getTarget().getName() in ["EVP_CipherInit_SKEY"] }
 
-  override Expr getOperationSubtypeArg() { result = this.(Call).getArgument(5) }
+  override Expr getKeyOperationSubtypeArg() { result = this.(Call).getArgument(5) }
 }
 
 class EVP_Cipher_Update_Call extends EvpUpdate {
@@ -105,7 +83,7 @@ class EVP_Cipher_Update_Call extends EvpUpdate {
       ]
   }
 
-  override CtxPointerSource getContextArg() { result = this.(Call).getArgument(0) }
+  override CtxPointerSource getContext() { result = this.(Call).getArgument(0) }
 
   override Expr getInputArg() { result = this.(Call).getArgument(3) }
 
@@ -154,10 +132,10 @@ class EVP_Cipher_Call extends EvpOperation, EVP_Cipher_Operation {
   override Expr getInputArg() { result = this.(Call).getArgument(2) }
 
   override Expr getAlgorithmArg() {
-    result = this.getInitCall().(EvpAlgorithmInitializer).getAlgorithmArg()
+    result = this.getInitCall().(EvpPrimaryAlgorithmInitializer).getAlgorithmArg()
   }
 
-  override CtxPointerSource getContextArg() { result = this.(Call).getArgument(0) }
+  override CtxPointerSource getContext() { result = this.(Call).getArgument(0) }
 }
 
 class EVP_Cipher_Final_Call extends EVPFinal, EVP_Cipher_Operation {
@@ -178,10 +156,10 @@ class EVP_Cipher_Final_Call extends EVPFinal, EVP_Cipher_Operation {
   }
 
   override Expr getAlgorithmArg() {
-    result = this.getInitCall().(EvpAlgorithmInitializer).getAlgorithmArg()
+    result = this.getInitCall().(EvpPrimaryAlgorithmInitializer).getAlgorithmArg()
   }
 
-  override CtxPointerSource getContextArg() { result = this.(Call).getArgument(0) }
+  override CtxPointerSource getContext() { result = this.(Call).getArgument(0) }
 }
 
 /**
@@ -195,9 +173,9 @@ class Evp_PKey_Cipher_Operation extends EVP_Cipher_Operation {
 
   override Expr getInputArg() { result = this.(Call).getArgument(3) }
 
-  override CtxPointerSource getContextArg() { result = this.(Call).getArgument(0) }
+  override CtxPointerSource getContext() { result = this.(Call).getArgument(0) }
 
   override Expr getAlgorithmArg() {
-    result = this.getInitCall().(EvpAlgorithmInitializer).getAlgorithmArg()
+    result = this.getInitCall().(EvpPrimaryAlgorithmInitializer).getAlgorithmArg()
   }
 }

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

@@ -7,9 +7,7 @@ private import experimental.quantum.OpenSSL.CtxFlow
 private import OpenSSLOperationBase
 private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumers
 
-abstract class EVP_Hash_Initializer extends EvpAlgorithmInitializer { }
-
-class EVP_DigestInit_Variant_Calls extends EVP_Hash_Initializer {
+class EVP_DigestInit_Variant_Calls extends EvpPrimaryAlgorithmInitializer {
   EVP_DigestInit_Variant_Calls() {
     this.(Call).getTarget().getName() in [
         "EVP_DigestInit", "EVP_DigestInit_ex", "EVP_DigestInit_ex2"
@@ -18,15 +16,15 @@ class EVP_DigestInit_Variant_Calls extends EVP_Hash_Initializer {
 
   override Expr getAlgorithmArg() { result = this.(Call).getArgument(1) }
 
-  override CtxPointerSource getContextArg() { result = this.(Call).getArgument(0) }
+  override CtxPointerSource getContext() { result = this.(Call).getArgument(0) }
 }
 
 class EVP_Digest_Update_Call extends EvpUpdate {
   EVP_Digest_Update_Call() { this.(Call).getTarget().getName() = "EVP_DigestUpdate" }
 
   override Expr getInputArg() { result = this.(Call).getArgument(1) }
 
-  override CtxPointerSource getContextArg() { result = this.(Call).getArgument(0) }
+  override CtxPointerSource getContext() { result = this.(Call).getArgument(0) }
 }
 
 //https://docs.openssl.org/3.0/man3/EVP_DigestInit/#synopsis
@@ -35,7 +33,7 @@ class EVP_Q_Digest_Operation extends EvpOperation, Crypto::HashOperationInstance
 
   override Expr getAlgorithmArg() { result = this.(Call).getArgument(1) }
 
-  override EVP_Hash_Initializer getInitCall() {
+  override EvpInitializer getInitCall() {
     // This variant of digest does not use an init
     // and even if it were used, the init would be ignored/undefined
     none()
@@ -53,18 +51,18 @@ class EVP_Q_Digest_Operation extends EvpOperation, Crypto::HashOperationInstance
     result = EvpOperation.super.getInputConsumer()
   }
 
-  override CtxPointerSource getContextArg() { result = this.(Call).getArgument(0) }
+  override CtxPointerSource getContext() { result = this.(Call).getArgument(0) }
 }
 
 class EVP_Digest_Operation extends EvpOperation, Crypto::HashOperationInstance {
   EVP_Digest_Operation() { this.(Call).getTarget().getName() = "EVP_Digest" }
 
   // There is no context argument for this function
-  override CtxPointerSource getContextArg() { none() }
+  override CtxPointerSource getContext() { none() }
 
   override Expr getAlgorithmArg() { result = this.(Call).getArgument(4) }
 
-  override EVP_Hash_Initializer getInitCall() {
+  override EvpPrimaryAlgorithmInitializer getInitCall() {
     // This variant of digest does not use an init
     // and even if it were used, the init would be ignored/undefined
     none()
@@ -90,7 +88,7 @@ class EVP_Digest_Final_Call extends EVPFinal, Crypto::HashOperationInstance {
       ]
   }
 
-  override CtxPointerSource getContextArg() { result = this.(Call).getArgument(0) }
+  override CtxPointerSource getContext() { result = this.(Call).getArgument(0) }
 
   override Expr getOutputArg() { result = this.(Call).getArgument(1) }
 
@@ -103,6 +101,6 @@ class EVP_Digest_Final_Call extends EVPFinal, Crypto::HashOperationInstance {
   }
 
   override Expr getAlgorithmArg() {
-    result = this.getInitCall().(EvpAlgorithmInitializer).getAlgorithmArg()
+    result = this.getInitCall().(EvpPrimaryAlgorithmInitializer).getAlgorithmArg()
   }
 }

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

@@ -4,7 +4,7 @@ private import OpenSSLOperationBase
 private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumers
 private import semmle.code.cpp.dataflow.new.DataFlow
 
-class EVPKeyGenInitialize extends EvpAlgorithmInitializer {
+class EVPKeyGenInitialize extends EvpPrimaryAlgorithmInitializer {
   EVPKeyGenInitialize() {
     this.(Call).getTarget().getName() in [
         "EVP_PKEY_keygen_init",
@@ -14,10 +14,13 @@ class EVPKeyGenInitialize extends EvpAlgorithmInitializer {
 
   /**
    * The algorithm is encoded through the context argument.
+   * The context may be directly created from an algorithm consumer,
+   * or from a new operation off of a prior key. Either way,
+   * we will treat this argument as the algorithm argument.
    */
-  override Expr getAlgorithmArg() { result = this.getContextArg() }
+  override Expr getAlgorithmArg() { result = this.getContext() }
 
-  override CtxPointerSource getContextArg() { result = this.(Call).getArgument(0) }
+  override CtxPointerSource getContext() { result = this.(Call).getArgument(0) }
 }
 
 class EVPKeyGenOperation extends EVPFinal, Crypto::KeyGenerationOperationInstance {
@@ -31,13 +34,13 @@ class EVPKeyGenOperation extends EVPFinal, Crypto::KeyGenerationOperationInstanc
     keyResultNode.asDefiningArgument() = this.(Call).getArgument(1)
   }
 
-  override CtxPointerSource getContextArg() { result = this.(Call).getArgument(0) }
+  override CtxPointerSource getContext() { result = this.(Call).getArgument(0) }
 
   override Expr getAlgorithmArg() {
     this.(Call).getTarget().getName() = "EVP_PKEY_Q_keygen" and
     result = this.(Call).getArgument(0)
     or
-    result = this.getInitCall().(EvpAlgorithmInitializer).getAlgorithmArg()
+    result = this.getInitCall().(EvpPrimaryAlgorithmInitializer).getAlgorithmArg()
   }
 
   override Crypto::KeyArtifactType getOutputKeyType() { result = Crypto::TAsymmetricKeyType() }