Crypto: CtxFlow now uses an interface for additional steps. Add CTX step to handle paramgen. Remove redundant test. Overhaul of EVP update/initializer/final mechanics. Misc. updates for new API and refactoring EVPKeyGenOperation. Clean up of keygen_operaitons.ql.

Author: bdrodes

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

@@ -13,7 +13,9 @@ module CryptoInput implements InputSig<Language::Location> {
   LocatableElement dfn_to_element(DataFlow::Node node) {
     result = node.asExpr() or
     result = node.asParameter() or
-    result = node.asVariable()
+    result = node.asVariable() or
+    result = node.asDefiningArgument()
+    // TODO: do we need asIndirectExpr()?
   }
 
   string locationToFileBaseNameAndLineNumberString(Location location) {

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

@@ -23,7 +23,8 @@ class EVPPKeyAlgorithmConsumer extends PKeyValueConsumer {
       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_set_group_name"
+          "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)
       or

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

@@ -28,7 +28,7 @@ import semmle.code.cpp.dataflow.new.DataFlow
  * - EVP_MD_CTX
  * - EVP_PKEY_CTX
  */
-private class CtxType extends Type {
+class CtxType extends Type {
   CtxType() {
     // It is possible for users to use the underlying type of the CTX variables
     // these have a name matching 'evp_%ctx_%st
@@ -47,7 +47,7 @@ private class CtxType extends Type {
 /**
  * A pointer to a CtxType
  */
-private class CtxPointerExpr extends Expr {
+class CtxPointerExpr extends Expr {
   CtxPointerExpr() {
     this.getType() instanceof CtxType and
     this.getType() instanceof PointerType
@@ -57,7 +57,7 @@ private class CtxPointerExpr extends Expr {
 /**
  * A call argument of type CtxPointerExpr.
  */
-private class CtxPointerArgument extends CtxPointerExpr {
+class CtxPointerArgument extends CtxPointerExpr {
   CtxPointerArgument() { exists(Call c | c.getAnArgument() = this) }
 
   Call getCall() { result.getAnArgument() = this }
@@ -83,32 +83,103 @@ private class CtxClearCall extends Call {
   }
 }
 
+abstract private class CtxPassThroughCall extends Call {
+  abstract DataFlow::Node getNode1();
+
+  abstract DataFlow::Node getNode2();
+}
+
 /**
  * A call whose target contains 'copy' and has an argument of type
  * CtxPointerArgument.
  */
-private class CtxCopyOutArgCall extends Call {
+private class CtxCopyOutArgCall extends CtxPassThroughCall {
+  DataFlow::Node n1;
+  DataFlow::Node n2;
+
   CtxCopyOutArgCall() {
     this.getTarget().getName().toLowerCase().matches("%copy%") and
-    this.getAnArgument() instanceof CtxPointerArgument
+    n1.asExpr() = this.getAnArgument() and
+    n1.getType() instanceof CtxType and
+    n2.asDefiningArgument() = this.getAnArgument() and
+    n2.getType() instanceof CtxType and
+    n1.asDefiningArgument() != n2.asExpr()
   }
+
+  override DataFlow::Node getNode1() { result = n1 }
+
+  override DataFlow::Node getNode2() { result = n2 }
 }
 
 /**
  * A call whose target contains 'dup' and has an argument of type
  * CtxPointerArgument.
  */
-private class CtxCopyReturnCall extends Call, CtxPointerExpr {
+private class CtxCopyReturnCall extends CtxPassThroughCall, CtxPointerExpr {
+  DataFlow::Node n1;
+
   CtxCopyReturnCall() {
     this.getTarget().getName().toLowerCase().matches("%dup%") and
-    this.getAnArgument() instanceof CtxPointerArgument
+    n1.asExpr() = this.getAnArgument() and
+    n1.getType() instanceof CtxType
+  }
+
+  override DataFlow::Node getNode1() { result = n1 }
+
+  override DataFlow::Node getNode2() { result.asExpr() = this }
+}
+
+/**
+ * A call to `EVP_PKEY_paramgen` acts as a kind of pass through.
+ * It's output pkey is eventually used in a new operation generating
+ * a fresh context pointer (e.g., `EVP_PKEY_CTX_new`).
+ * It is easier to model this as a pass through
+ * than to model the flow from the paramgen to the new key generation.
+ */
+private class CtxParamGenCall extends CtxPassThroughCall {
+  DataFlow::Node n1;
+  DataFlow::Node n2;
+
+  CtxParamGenCall() {
+    this.getTarget().getName() = "EVP_PKEY_paramgen" and
+    n1.asExpr() = this.getArgument(0) and
+    (
+      n2.asExpr() = this.getArgument(1)
+      or
+      n2.asDefiningArgument() = this.getArgument(1)
+    )
   }
+
+  override DataFlow::Node getNode1() { result = n1 }
+
+  override DataFlow::Node getNode2() { result = n2 }
+}
+
+/**
+ * If the current node gets is an argument to a function
+ * that returns a pointer type, immediately flow through.
+ * NOTE: this passthrough is required if we allow
+ * intermediate steps to go into variables that are not a CTX type.
+ * See for example `CtxParamGenCall`.
+ */
+private class CallArgToCtxRet extends CtxPassThroughCall, CtxPointerExpr {
+  DataFlow::Node n1;
+  DataFlow::Node n2;
+
+  CallArgToCtxRet() {
+    this.getAnArgument() = n1.asExpr() and
+    n2.asExpr() = this
+  }
+
+  override DataFlow::Node getNode1() { result = n1 }
+
+  override DataFlow::Node getNode2() { result = n2 }
 }
 
 /**
  * A source Ctx of interest is any argument or return of type CtxPointerExpr.
  */
-private class CtxPointerSource extends CtxPointerExpr {
+class CtxPointerSource extends CtxPointerExpr {
   CtxPointerSource() {
     this instanceof CtxPointerReturn or
     this instanceof CtxPointerArgument
@@ -122,43 +193,31 @@ private class CtxPointerSource extends CtxPointerExpr {
 }
 
 /**
- * Flow from any CtxPointerSource to any CtxPointerArgument.
+ * Flow from any CtxPointerSource to other CtxPointerSource.
  */
-module OpenSSLCtxSourceToArgumentFlowConfig implements DataFlow::ConfigSig {
+module OpenSSLCtxSourceToSourceFlowConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) { exists(CtxPointerSource s | s.asNode() = source) }
 
-  predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof CtxPointerArgument }
+  predicate isSink(DataFlow::Node sink) { exists(CtxPointerSource s | s.asNode() = sink) }
 
   predicate isBarrier(DataFlow::Node node) {
     exists(CtxClearCall c | c.getAnArgument() = node.asExpr())
   }
 
   predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
-    exists(CtxCopyOutArgCall c |
-      c.getAnArgument() = node1.asExpr() and
-      c.getAnArgument() = node2.asExpr() and
-      node1.asExpr() != node2.asExpr() and
-      node2.asExpr().getType() instanceof CtxType
-    )
-    or
-    exists(CtxCopyReturnCall c |
-      c.getAnArgument() = node1.asExpr() and
-      c = node2.asExpr() and
-      node1.asExpr() != node2.asExpr() and
-      node2.asExpr().getType() instanceof CtxType
-    )
+    exists(CtxPassThroughCall c | c.getNode1() = node1 and c.getNode2() = node2)
   }
 }
 
-module OpenSSLCtxSourceToArgumentFlow = DataFlow::Global<OpenSSLCtxSourceToArgumentFlowConfig>;
+module OpenSSLCtxSourceToArgumentFlow = DataFlow::Global<OpenSSLCtxSourceToSourceFlowConfig>;
 
 /**
  * Holds if there is a context flow from the source to the sink.
  */
-predicate ctxArgOrRetFlowsToCtxArg(CtxPointerSource source, CtxPointerArgument sink) {
+predicate ctxSrcToSrcFlow(CtxPointerSource source, CtxPointerSource sink) {
   exists(DataFlow::Node a, DataFlow::Node b |
     OpenSSLCtxSourceToArgumentFlow::flow(a, b) and
     a = source.asNode() and
-    b.asExpr() = sink
+    b = sink.asNode()
   )
 }

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

@@ -1,5 +1,5 @@
 private import experimental.quantum.Language
-private import experimental.quantum.OpenSSL.CtxFlow as CTXFlow
+private import experimental.quantum.OpenSSL.CtxFlow
 private import OpenSSLOperationBase
 private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumers
 
@@ -31,7 +31,11 @@ Crypto::KeyOperationSubtype intToCipherOperationSubtype(int i) {
 }
 
 // TODO: need to add key consumer
-abstract class EVP_Cipher_Initializer extends EVPInitialize {
+abstract class EVP_Cipher_Initializer extends EvpKeyOperationSubtypeInitializer,
+  EvpAlgorithmInitializer, EvpKeyInitializer, EvpIVInitializer
+{
+  override CtxPointerSource getContextArg() { result = this.(Call).getArgument(0) }
+
   override Expr getAlgorithmArg() { result = this.(Call).getArgument(1) }
 
   abstract Expr getOperationSubtypeArg();
@@ -94,13 +98,15 @@ class EVP_CipherInit_SKEY_Call extends EVP_EX2_Initializer {
   override Expr getOperationSubtypeArg() { result = this.(Call).getArgument(5) }
 }
 
-class EVP_Cipher_Update_Call extends EVPUpdate {
+class EVP_Cipher_Update_Call extends EvpUpdate {
   EVP_Cipher_Update_Call() {
     this.(Call).getTarget().getName() in [
         "EVP_EncryptUpdate", "EVP_DecryptUpdate", "EVP_CipherUpdate"
       ]
   }
 
+  override CtxPointerSource getContextArg() { result = this.(Call).getArgument(0) }
+
   override Expr getInputArg() { result = this.(Call).getArgument(3) }
 
   override Expr getOutputArg() { result = this.(Call).getArgument(1) }
@@ -110,7 +116,7 @@ class EVP_Cipher_Update_Call extends EVPUpdate {
  * see: https://docs.openssl.org/master/man3/EVP_EncryptInit/#synopsis
  * Base configuration for all EVP cipher operations.
  */
-abstract class EVP_Cipher_Operation extends EVPOperation, Crypto::KeyOperationInstance {
+abstract class EVP_Cipher_Operation extends EvpOperation, Crypto::KeyOperationInstance {
   override Expr getOutputArg() { result = this.(Call).getArgument(1) }
 
   override Crypto::KeyOperationSubtype getKeyOperationSubtype() {
@@ -120,34 +126,38 @@ abstract class EVP_Cipher_Operation extends EVPOperation, Crypto::KeyOperationIn
     result instanceof Crypto::TDecryptMode and
     this.(Call).getTarget().getName().toLowerCase().matches("%decrypt%")
     or
-    result = this.getInitCall().getKeyOperationSubtype() and
+    result = this.getInitCall().(EvpKeyOperationSubtypeInitializer).getKeyOperationSubtype() and
     this.(Call).getTarget().getName().toLowerCase().matches("%cipher%")
   }
 
   override Crypto::ConsumerInputDataFlowNode getNonceConsumer() {
-    this.getInitCall().getIVArg() = result.asExpr()
+    this.getInitCall().(EvpIVInitializer).getIVArg() = result.asExpr()
   }
 
   override Crypto::ConsumerInputDataFlowNode getKeyConsumer() {
-    this.getInitCall().getKeyArg() = result.asExpr()
+    this.getInitCall().(EvpKeyInitializer).getKeyArg() = result.asExpr()
     // todo: or track to the EVP_PKEY_CTX_new
   }
 
   override Crypto::ArtifactOutputDataFlowNode getOutputArtifact() {
-    result = EVPOperation.super.getOutputArtifact()
+    result = EvpOperation.super.getOutputArtifact()
   }
 
   override Crypto::ConsumerInputDataFlowNode getInputConsumer() {
-    result = EVPOperation.super.getInputConsumer()
+    result = EvpOperation.super.getInputConsumer()
   }
 }
 
-class EVP_Cipher_Call extends EVPOperation, EVP_Cipher_Operation {
+class EVP_Cipher_Call extends EvpOperation, EVP_Cipher_Operation {
   EVP_Cipher_Call() { this.(Call).getTarget().getName() = "EVP_Cipher" }
 
   override Expr getInputArg() { result = this.(Call).getArgument(2) }
 
-  override Expr getAlgorithmArg() { result = this.getInitCall().getAlgorithmArg() }
+  override Expr getAlgorithmArg() {
+    result = this.getInitCall().(EvpAlgorithmInitializer).getAlgorithmArg()
+  }
+
+  override CtxPointerSource getContextArg() { result = this.(Call).getArgument(0) }
 }
 
 class EVP_Cipher_Final_Call extends EVPFinal, EVP_Cipher_Operation {
@@ -167,5 +177,9 @@ class EVP_Cipher_Final_Call extends EVPFinal, EVP_Cipher_Operation {
     result = EVP_Cipher_Operation.super.getOutputArg()
   }
 
-  override Expr getAlgorithmArg() { result = this.getInitCall().getAlgorithmArg() }
+  override Expr getAlgorithmArg() {
+    result = this.getInitCall().(EvpAlgorithmInitializer).getAlgorithmArg()
+  }
+
+  override CtxPointerSource getContextArg() { result = this.(Call).getArgument(0) }
 }