Merge pull request #19552 from bdrodes/ben_refactoring

nicolaswill · web-flow · commit bb4c6a316e3c · 2025-05-21T22:24:12.000+02:00
Crypto: Misc. refactoring and code clean up.
diff --git a/cpp/ql/lib/experimental/quantum/Language.qll b/cpp/ql/lib/experimental/quantum/Language.qll
@@ -94,7 +94,10 @@ private class ConstantDataSource extends Crypto::GenericConstantSourceInstance i
     // where typical algorithms are specified, but EC specifically means set up a
     // default curve container, that will later be specified explicitly (or if not a default)
     // curve is used.
-    this.getValue() != "EC"
+    this.getValue() != "EC" and
+    // Exclude all 0's as algorithms. Currently we know of no algorithm defined as 0, and
+    // the typical case is 0 is assigned to represent null.
+    this.getValue().toInt() != 0
   }
 
   override DataFlow::Node getOutputNode() { result.asExpr() = this }
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/AlgToAVCFlow.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/AlgToAVCFlow.qll
@@ -3,6 +3,7 @@ private import experimental.quantum.Language
 private import semmle.code.cpp.dataflow.new.DataFlow
 private import experimental.quantum.OpenSSL.AlgorithmInstances.KnownAlgorithmConstants
 private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumers
+private import PaddingAlgorithmInstance
 
 /**
  * Traces 'known algorithms' to AVCs, specifically
@@ -19,6 +20,9 @@ module KnownOpenSSLAlgorithmToAlgorithmValueConsumerConfig implements DataFlow::
   predicate isSink(DataFlow::Node sink) {
     exists(OpenSSLAlgorithmValueConsumer c |
       c.getInputNode() = sink and
+      // exclude padding algorithm consumers, since
+      // these consumers take in different constant values
+      // not in the typical "known algorithm" set
       not c instanceof PaddingAlgorithmValueConsumer
     )
   }
@@ -43,9 +47,7 @@ module KnownOpenSSLAlgorithmToAlgorithmValueConsumerFlow =
   DataFlow::Global<KnownOpenSSLAlgorithmToAlgorithmValueConsumerConfig>;
 
 module RSAPaddingAlgorithmToPaddingAlgorithmValueConsumerConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node source) {
-    source.asExpr() instanceof KnownOpenSSLAlgorithmConstant
-  }
+  predicate isSource(DataFlow::Node source) { source.asExpr() instanceof OpenSSLPaddingLiteral }
 
   predicate isSink(DataFlow::Node sink) {
     exists(PaddingAlgorithmValueConsumer c | c.getInputNode() = sink)
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/BlockAlgorithmInstance.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/BlockAlgorithmInstance.qll
@@ -8,7 +8,7 @@ private import AlgToAVCFlow
 
 /**
  * Given a `KnownOpenSSLBlockModeAlgorithmConstant`, converts this to a block family type.
- * Does not bind if there is know mapping (no mapping to 'unknown' or 'other').
+ * Does not bind if there is no mapping (no mapping to 'unknown' or 'other').
  */
 predicate knownOpenSSLConstantToBlockModeFamilyType(
   KnownOpenSSLBlockModeAlgorithmConstant e, Crypto::TBlockCipherModeOfOperationType type
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/CipherAlgorithmInstance.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/CipherAlgorithmInstance.qll
@@ -11,7 +11,7 @@ private import BlockAlgorithmInstance
 
 /**
  * Given a `KnownOpenSSLCipherAlgorithmConstant`, converts this to a cipher family type.
- * Does not bind if there is know mapping (no mapping to 'unknown' or 'other').
+ * Does not bind if there is no mapping (no mapping to 'unknown' or 'other').
  */
 predicate knownOpenSSLConstantToCipherFamilyType(
   KnownOpenSSLCipherAlgorithmConstant e, Crypto::KeyOpAlg::TAlgorithm type
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/KnownAlgorithmConstants.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/KnownAlgorithmConstants.qll
@@ -1,5 +1,4 @@
 import cpp
-private import experimental.quantum.OpenSSL.LibraryDetector
 
 predicate resolveAlgorithmFromExpr(Expr e, string normalizedName, string algType) {
   resolveAlgorithmFromCall(e, normalizedName, algType)
@@ -20,7 +19,7 @@ class KnownOpenSSLCipherAlgorithmConstant extends KnownOpenSSLAlgorithmConstant
 
   KnownOpenSSLCipherAlgorithmConstant() {
     resolveAlgorithmFromExpr(this, _, algType) and
-    algType.toLowerCase().matches("%encryption")
+    algType.matches("%ENCRYPTION")
   }
 
   int getExplicitKeySize() {
@@ -37,7 +36,7 @@ class KnownOpenSSLPaddingAlgorithmConstant extends KnownOpenSSLAlgorithmConstant
 
   KnownOpenSSLPaddingAlgorithmConstant() {
     resolveAlgorithmFromExpr(this, _, algType) and
-    algType.toLowerCase().matches("%padding")
+    algType.matches("%PADDING")
   }
 }
 
@@ -46,7 +45,7 @@ class KnownOpenSSLBlockModeAlgorithmConstant extends KnownOpenSSLAlgorithmConsta
 
   KnownOpenSSLBlockModeAlgorithmConstant() {
     resolveAlgorithmFromExpr(this, _, algType) and
-    algType.toLowerCase().matches("%block_mode")
+    algType.matches("%BLOCK_MODE")
   }
 }
 
@@ -55,7 +54,7 @@ class KnownOpenSSLHashAlgorithmConstant extends KnownOpenSSLAlgorithmConstant {
 
   KnownOpenSSLHashAlgorithmConstant() {
     resolveAlgorithmFromExpr(this, _, algType) and
-    algType.toLowerCase().matches("%hash")
+    algType.matches("%HASH")
   }
 
   int getExplicitDigestLength() {
@@ -71,7 +70,7 @@ class KnownOpenSSLEllipticCurveAlgorithmConstant extends KnownOpenSSLAlgorithmCo
   KnownOpenSSLEllipticCurveAlgorithmConstant() {
     exists(string algType |
       resolveAlgorithmFromExpr(this, _, algType) and
-      algType.toLowerCase().matches("elliptic_curve")
+      algType.matches("ELLIPTIC_CURVE")
     )
   }
 }
@@ -89,7 +88,6 @@ class KnownOpenSSLEllipticCurveAlgorithmConstant extends KnownOpenSSLAlgorithmCo
  *  alias = "dss1" and target = "dsaWithSHA1"
  */
 predicate resolveAlgorithmFromCall(Call c, string normalized, string algType) {
-  isPossibleOpenSSLFunction(c.getTarget()) and
   exists(string name, string parsedTargetName |
     parsedTargetName =
       c.getTarget().getName().replaceAll("EVP_", "").toLowerCase().replaceAll("_", "-") and
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/PaddingAlgorithmInstance.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/PaddingAlgorithmInstance.qll
@@ -6,9 +6,26 @@ private import AlgToAVCFlow
 private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.DirectAlgorithmValueConsumer
 private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumerBase
 
+/**
+ * A class to define padding specific integer values.
+ * from rsa.h in openssl:
+ *     # define RSA_PKCS1_PADDING          1
+ *     # define RSA_NO_PADDING             3
+ *     # define RSA_PKCS1_OAEP_PADDING     4
+ *     # define RSA_X931_PADDING           5
+ *     # define RSA_PKCS1_PSS_PADDING      6
+ *     # define RSA_PKCS1_WITH_TLS_PADDING 7
+ *     # define RSA_PKCS1_NO_IMPLICIT_REJECT_PADDING 8
+ */
+class OpenSSLPaddingLiteral extends Literal {
+  // TODO: we can be more specific about where the literal is in a larger expression
+  // to avoid literals that are clealy not representing an algorithm, e.g., array indices.
+  OpenSSLPaddingLiteral() { this.getValue().toInt() in [0, 1, 3, 4, 5, 6, 7, 8] }
+}
+
 /**
  * Given a `KnownOpenSSLPaddingAlgorithmConstant`, converts this to a padding family type.
- * Does not bind if there is know mapping (no mapping to 'unknown' or 'other').
+ * Does not bind if there is no mapping (no mapping to 'unknown' or 'other').
  */
 predicate knownOpenSSLConstantToPaddingFamilyType(
   KnownOpenSSLPaddingAlgorithmConstant e, Crypto::TPaddingType type
@@ -60,19 +77,8 @@ class KnownOpenSSLPaddingConstantAlgorithmInstance extends OpenSSLAlgorithmInsta
     this instanceof KnownOpenSSLPaddingAlgorithmConstant and
     isPaddingSpecificConsumer = false
     or
-    // Possibility 3:
-    // from rsa.h in openssl:
-    // # define RSA_PKCS1_PADDING          1
-    // # define RSA_NO_PADDING             3
-    // # define RSA_PKCS1_OAEP_PADDING     4
-    // # define RSA_X931_PADDING           5
-    // /* EVP_PKEY_ only */
-    // # define RSA_PKCS1_PSS_PADDING      6
-    // # define RSA_PKCS1_WITH_TLS_PADDING 7
-    // /* internal RSA_ only */
-    // # define RSA_PKCS1_NO_IMPLICIT_REJECT_PADDING 8
-    this instanceof Literal and
-    this.getValue().toInt() in [0, 1, 3, 4, 5, 6, 7, 8] and
+    // Possibility 3: padding-specific literal
+    this instanceof OpenSSLPaddingLiteral and
     exists(DataFlow::Node src, DataFlow::Node sink |
       // Sink is an argument to a CipherGetterCall
       sink = getterCall.(OpenSSLAlgorithmValueConsumer).getInputNode() and
@@ -88,24 +94,24 @@ class KnownOpenSSLPaddingConstantAlgorithmInstance extends OpenSSLAlgorithmInsta
 
   override OpenSSLAlgorithmValueConsumer getAVC() { result = getterCall }
 
+  Crypto::TPaddingType getKnownPaddingType() {
+    this.(Literal).getValue().toInt() in [1, 7, 8] and result = Crypto::PKCS1_v1_5()
+    or
+    this.(Literal).getValue().toInt() = 3 and result = Crypto::NoPadding()
+    or
+    this.(Literal).getValue().toInt() = 4 and result = Crypto::OAEP()
+    or
+    this.(Literal).getValue().toInt() = 5 and result = Crypto::ANSI_X9_23()
+    or
+    this.(Literal).getValue().toInt() = 6 and result = Crypto::PSS()
+  }
+
   override Crypto::TPaddingType getPaddingType() {
     isPaddingSpecificConsumer = true and
     (
-      if this.(Literal).getValue().toInt() in [1, 7, 8]
-      then result = Crypto::PKCS1_v1_5()
-      else
-        if this.(Literal).getValue().toInt() = 3
-        then result = Crypto::NoPadding()
-        else
-          if this.(Literal).getValue().toInt() = 4
-          then result = Crypto::OAEP()
-          else
-            if this.(Literal).getValue().toInt() = 5
-            then result = Crypto::ANSI_X9_23()
-            else
-              if this.(Literal).getValue().toInt() = 6
-              then result = Crypto::PSS()
-              else result = Crypto::OtherPadding()
+      result = this.getKnownPaddingType()
+      or
+      not exists(this.getKnownPaddingType()) and result = Crypto::OtherPadding()
     )
     or
     isPaddingSpecificConsumer = false and
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/CipherAlgorithmValueConsumer.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/CipherAlgorithmValueConsumer.qll
@@ -1,6 +1,5 @@
 import cpp
 private import experimental.quantum.Language
-private import experimental.quantum.OpenSSL.LibraryDetector
 private import experimental.quantum.OpenSSL.AlgorithmInstances.KnownAlgorithmConstants
 private import experimental.quantum.OpenSSL.AlgorithmInstances.OpenSSLAlgorithmInstanceBase
 private import OpenSSLAlgorithmValueConsumerBase
@@ -14,7 +13,6 @@ class EVPCipherAlgorithmValueConsumer extends CipherAlgorithmValueConsumer {
 
   EVPCipherAlgorithmValueConsumer() {
     resultNode.asExpr() = this and
-    isPossibleOpenSSLFunction(this.(Call).getTarget()) and
     (
       this.(Call).getTarget().getName() in [
           "EVP_get_cipherbyname", "EVP_get_cipherbyobj", "EVP_get_cipherbynid"
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/EllipticCurveAlgorithmValueConsumer.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/EllipticCurveAlgorithmValueConsumer.qll
@@ -1,6 +1,5 @@
 import cpp
 private import experimental.quantum.Language
-private import experimental.quantum.OpenSSL.LibraryDetector
 private import experimental.quantum.OpenSSL.AlgorithmInstances.KnownAlgorithmConstants
 private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumerBase
 private import experimental.quantum.OpenSSL.AlgorithmInstances.OpenSSLAlgorithmInstances
@@ -14,7 +13,6 @@ class EVPEllipticCurveAlgorithmConsumer extends EllipticCurveValueConsumer {
 
   EVPEllipticCurveAlgorithmConsumer() {
     resultNode.asExpr() = this.(Call) and // in all cases the result is the return
-    isPossibleOpenSSLFunction(this.(Call).getTarget()) and
     (
       this.(Call).getTarget().getName() in ["EVP_EC_gen", "EC_KEY_new_by_curve_name"] and
       valueArgNode.asExpr() = this.(Call).getArgument(0)
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/OpenSSLAlgorithmValueConsumerBase.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/OpenSSLAlgorithmValueConsumerBase.qll
@@ -1,5 +1,4 @@
 private import experimental.quantum.Language
-private import semmle.code.cpp.dataflow.new.DataFlow
 
 abstract class OpenSSLAlgorithmValueConsumer extends Crypto::AlgorithmValueConsumer instanceof Call {
   /**
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/PKeyAlgorithmValueConsumer.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/PKeyAlgorithmValueConsumer.qll
@@ -1,6 +1,5 @@
 import cpp
 private import experimental.quantum.Language
-private import experimental.quantum.OpenSSL.LibraryDetector
 private import experimental.quantum.OpenSSL.AlgorithmInstances.KnownAlgorithmConstants
 private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumerBase
 private import experimental.quantum.OpenSSL.AlgorithmInstances.OpenSSLAlgorithmInstances
@@ -13,7 +12,6 @@ class EVPPKeyAlgorithmConsumer extends PKeyValueConsumer {
 
   EVPPKeyAlgorithmConsumer() {
     resultNode.asExpr() = this.(Call) and // in all cases the result is the return
-    isPossibleOpenSSLFunction(this.(Call).getTarget()) and
     (
       // NOTE: some of these consumers are themselves key gen operations,
       // in these cases, the operation will be created separately for the same function.
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/PaddingAlgorithmValueConsumer.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmValueConsumers/PaddingAlgorithmValueConsumer.qll
@@ -1,6 +1,5 @@
 import cpp
 private import experimental.quantum.Language
-private import experimental.quantum.OpenSSL.LibraryDetector
 private import experimental.quantum.OpenSSL.AlgorithmInstances.KnownAlgorithmConstants
 private import experimental.quantum.OpenSSL.AlgorithmInstances.OpenSSLAlgorithmInstanceBase
 private import OpenSSLAlgorithmValueConsumerBase
@@ -16,11 +15,8 @@ class EVP_PKEY_CTX_set_rsa_padding_AlgorithmValueConsumer extends PaddingAlgorit
 
   EVP_PKEY_CTX_set_rsa_padding_AlgorithmValueConsumer() {
     resultNode.asExpr() = this and
-    isPossibleOpenSSLFunction(this.(Call).getTarget()) and
-    (
-      this.(Call).getTarget().getName() in ["EVP_PKEY_CTX_set_rsa_padding"] and
-      valueArgNode.asExpr() = this.(Call).getArgument(1)
-    )
+    this.(Call).getTarget().getName() = "EVP_PKEY_CTX_set_rsa_padding" and
+    valueArgNode.asExpr() = this.(Call).getArgument(1)
   }
 
   override DataFlow::Node getResultNode() { result = resultNode }
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/CtxFlow.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/CtxFlow.qll
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/OpenSSL.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/OpenSSL.qll
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/Operations/ECKeyGenOperation.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/Operations/ECKeyGenOperation.qll
diff --git a/cpp/ql/lib/experimental/quantum/OpenSSL/Operations/EVPHashOperation.qll b/cpp/ql/lib/experimental/quantum/OpenSSL/Operations/EVPHashOperation.qll

Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,4 @@`
`1`	`1`	`import cpp`
`2`		`-private import experimental.quantum.OpenSSL.LibraryDetector`
`3`	`2`
`4`	`3`	`predicate resolveAlgorithmFromExpr(Expr e, string normalizedName, string algType) {`
`5`	`4`	`resolveAlgorithmFromCall(e, normalizedName, algType)`
`@@ -20,7 +19,7 @@ class KnownOpenSSLCipherAlgorithmConstant extends KnownOpenSSLAlgorithmConstant`
`20`	`19`
`21`	`20`	`KnownOpenSSLCipherAlgorithmConstant() {`
`22`	`21`	`resolveAlgorithmFromExpr(this, _, algType) and`
`23`		`- algType.toLowerCase().matches("%encryption")`
	`22`	`+ algType.matches("%ENCRYPTION")`
`24`	`23`	`}`
`25`	`24`
`26`	`25`	`int getExplicitKeySize() {`
`@@ -37,7 +36,7 @@ class KnownOpenSSLPaddingAlgorithmConstant extends KnownOpenSSLAlgorithmConstant`
`37`	`36`
`38`	`37`	`KnownOpenSSLPaddingAlgorithmConstant() {`
`39`	`38`	`resolveAlgorithmFromExpr(this, _, algType) and`
`40`		`- algType.toLowerCase().matches("%padding")`
	`39`	`+ algType.matches("%PADDING")`
`41`	`40`	`}`
`42`	`41`	`}`
`43`	`42`
`@@ -46,7 +45,7 @@ class KnownOpenSSLBlockModeAlgorithmConstant extends KnownOpenSSLAlgorithmConsta`
`46`	`45`
`47`	`46`	`KnownOpenSSLBlockModeAlgorithmConstant() {`
`48`	`47`	`resolveAlgorithmFromExpr(this, _, algType) and`
`49`		`- algType.toLowerCase().matches("%block_mode")`
	`48`	`+ algType.matches("%BLOCK_MODE")`
`50`	`49`	`}`
`51`	`50`	`}`
`52`	`51`
`@@ -55,7 +54,7 @@ class KnownOpenSSLHashAlgorithmConstant extends KnownOpenSSLAlgorithmConstant {`
`55`	`54`
`56`	`55`	`KnownOpenSSLHashAlgorithmConstant() {`
`57`	`56`	`resolveAlgorithmFromExpr(this, _, algType) and`
`58`		`- algType.toLowerCase().matches("%hash")`
	`57`	`+ algType.matches("%HASH")`
`59`	`58`	`}`
`60`	`59`
`61`	`60`	`int getExplicitDigestLength() {`
`@@ -71,7 +70,7 @@ class KnownOpenSSLEllipticCurveAlgorithmConstant extends KnownOpenSSLAlgorithmCo`
`71`	`70`	`KnownOpenSSLEllipticCurveAlgorithmConstant() {`
`72`	`71`	`exists(string algType \|`
`73`	`72`	`resolveAlgorithmFromExpr(this, _, algType) and`
`74`		`- algType.toLowerCase().matches("elliptic_curve")`
	`73`	`+ algType.matches("ELLIPTIC_CURVE")`
`75`	`74`	`)`
`76`	`75`	`}`
`77`	`76`	`}`
`@@ -89,7 +88,6 @@ class KnownOpenSSLEllipticCurveAlgorithmConstant extends KnownOpenSSLAlgorithmCo`
`89`	`88`	`* alias = "dss1" and target = "dsaWithSHA1"`
`90`	`89`	`*/`
`91`	`90`	`predicate resolveAlgorithmFromCall(Call c, string normalized, string algType) {`
`92`		`- isPossibleOpenSSLFunction(c.getTarget()) and`
`93`	`91`	`exists(string name, string parsedTargetName \|`
`94`	`92`	`parsedTargetName =`
`95`	`93`	`c.getTarget().getName().replaceAll("EVP_", "").toLowerCase().replaceAll("_", "-") and`
Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,4 @@`
`1`	`1`	`private import experimental.quantum.Language`
`2`		`-private import semmle.code.cpp.dataflow.new.DataFlow`
`3`	`2`
`4`	`3`	`abstract class OpenSSLAlgorithmValueConsumer extends Crypto::AlgorithmValueConsumer instanceof Call {`
`5`	`4`	`/**`