Crypto: Improve literal filtering for OpenSSL for algorithms and generic sources

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

@@ -1,6 +1,7 @@
 private import cpp as Language
 import semmle.code.cpp.dataflow.new.TaintTracking
 import codeql.quantum.experimental.Model
+private import OpenSSL.GenericSourceCandidateLiteral
 
 module CryptoInput implements InputSig<Language::Location> {
   class DataFlowNode = DataFlow::Node;
@@ -88,17 +89,8 @@
 
 module GenericDataSourceFlow = TaintTracking::Global<GenericDataSourceFlowConfig>;
 
 private class ConstantDataSource extends Crypto::GenericConstantSourceInstance instanceof Literal {
-  ConstantDataSource() {
-    // TODO: this is an API specific workaround for OpenSSL, as 'EC' is a constant that may be used
-    // 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" 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
-  }
+  ConstantDataSource() { this instanceof OpenSSLGenericSourceCandidateLiteral }
 
   override DataFlow::Node getOutputNode() { result.asExpr() = this }
 

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

@@ -1,4 +1,5 @@
 import cpp
+import experimental.quantum.OpenSSL.GenericSourceCandidateLiteral
 
 predicate resolveAlgorithmFromExpr(Expr e, string normalizedName, string algType) {
   resolveAlgorithmFromCall(e, normalizedName, algType)
@@ -32,30 +33,20 @@
 }
 
 class KnownOpenSSLPaddingAlgorithmConstant extends KnownOpenSSLAlgorithmConstant {
-  string algType;
-
   KnownOpenSSLPaddingAlgorithmConstant() {
-    resolveAlgorithmFromExpr(this, _, algType) and
-    algType.matches("%PADDING")
+    exists(string algType |
+      resolveAlgorithmFromExpr(this, _, algType) and
+      algType.matches("%PADDING")
+    )
   }
 }
 
 class KnownOpenSSLBlockModeAlgorithmConstant extends KnownOpenSSLAlgorithmConstant {
-  string algType;
-
-  KnownOpenSSLBlockModeAlgorithmConstant() {
-    resolveAlgorithmFromExpr(this, _, algType) and
-    algType.matches("%BLOCK_MODE")
-  }
+  KnownOpenSSLBlockModeAlgorithmConstant() { resolveAlgorithmFromExpr(this, _, "BLOCK_MODE") }
 }
 
 class KnownOpenSSLHashAlgorithmConstant extends KnownOpenSSLAlgorithmConstant {
-  string algType;
-
-  KnownOpenSSLHashAlgorithmConstant() {
-    resolveAlgorithmFromExpr(this, _, algType) and
-    algType.matches("%HASH")
-  }
+  KnownOpenSSLHashAlgorithmConstant() { resolveAlgorithmFromExpr(this, _, "HASH") }
 
   int getExplicitDigestLength() {
     exists(string name |
@@ -68,13 +59,14 @@
 
 class KnownOpenSSLEllipticCurveAlgorithmConstant extends KnownOpenSSLAlgorithmConstant {
   KnownOpenSSLEllipticCurveAlgorithmConstant() {
-    exists(string algType |
-      resolveAlgorithmFromExpr(this, _, algType) and
-      algType.matches("ELLIPTIC_CURVE")
-    )
+    resolveAlgorithmFromExpr(this, _, "ELLIPTIC_CURVE")
   }
 }
 
+class KnownOpenSSLSignatureAlgorithmConstant extends KnownOpenSSLAlgorithmConstant {
+  KnownOpenSSLSignatureAlgorithmConstant() { resolveAlgorithmFromExpr(this, _, "SIGNATURE") }
+}
+
 /**
  * Resolves a call to a 'direct algorithm getter', e.g., EVP_MD5()
  * This approach to fetching algorithms was used in OpenSSL 1.0.2.
@@ -101,10 +93,10 @@
  * if `e` resolves to a known algorithm.
  * If this predicate does not hold, then `e` can be interpreted as being of `UNKNOWN` type.
  */
-predicate resolveAlgorithmFromLiteral(Literal e, string normalized, string algType) {
-  exists(int nid |
-    nid = getPossibleNidFromLiteral(e) and knownOpenSSLAlgorithmLiteral(_, nid, normalized, algType)
-  )
+predicate resolveAlgorithmFromLiteral(
+  OpenSSLGenericSourceCandidateLiteral e, string normalized, string algType
+) {
+  knownOpenSSLAlgorithmLiteral(_, e.getValue().toInt(), normalized, algType)
   or
   exists(string name |
     name = resolveAlgorithmAlias(e.getValue()) and
@@ -123,30 +115,6 @@
   )
 }
 
-private int getPossibleNidFromLiteral(Literal e) {
-  result = e.getValue().toInt() and
-  not e instanceof CharLiteral and
-  not e instanceof StringLiteral and
-  // ASSUMPTION, no negative numbers are allowed
-  // RATIONALE: this is a performance improvement to avoid having to trace every number
-  not exists(UnaryMinusExpr u | u.getOperand() = e) and
-  //  OPENSSL has a special macro for getting every line, ignore it
-  not exists(MacroInvocation mi | mi.getExpr() = e and mi.getMacroName() = "OPENSSL_LINE") and
-  // Filter out cases where an int is assigned into a pointer, e.g., char* x = NULL;
-  not exists(Assignment a |
-    a.getRValue() = e and a.getLValue().getType().getUnspecifiedType() instanceof PointerType
-  ) and
-  not exists(Initializer i |
-    i.getExpr() = e and
-    i.getDeclaration().getADeclarationEntry().getUnspecifiedType() instanceof PointerType
-  ) and
-  // Filter out cases where an int is returned into a pointer, e.g., return NULL;
-  not exists(ReturnStmt r |
-    r.getExpr() = e and
-    r.getEnclosingFunction().getType().getUnspecifiedType() instanceof PointerType
-  )
-}
-
 string getAlgorithmAlias(string alias) {
   customAliases(result, alias)
   or
@@ -260,11 +228,6 @@
   alias = "ssl3-sha1" and target = "sha1"
 }
 
-predicate tbd(string normalized, string algType) {
-  knownOpenSSLAlgorithmLiteral(_, _, normalized, algType) and
-  algType = "HASH"
-}
-
 /**
  * Enumeration of all known crypto algorithms for openSSL
  * `name` is all lower case (caller's must ensure they pass in lower case)
@@ -291,8 +254,12 @@
   or
   name = "ed25519" and nid = 1087 and normalized = "ED25519" and algType = "ELLIPTIC_CURVE"
   or
+  name = "ed25519" and nid = 1087 and normalized = "ED25519" and algType = "SIGNATURE"
+  or
   name = "ed448" and nid = 1088 and normalized = "ED448" and algType = "ELLIPTIC_CURVE"
   or
+  name = "ed448" and nid = 1088 and normalized = "ED448" and algType = "SIGNATURE"
+  or
   name = "md2" and nid = 3 and normalized = "MD2" and algType = "HASH"
   or
   name = "sha" and nid = 41 and normalized = "SHA" and algType = "HASH"
@@ -1712,8 +1679,12 @@
   or
   name = "x448" and nid = 1035 and normalized = "X448" and algType = "ELLIPTIC_CURVE"
   or
+  name = "x448" and nid = 1035 and normalized = "X448" and algType = "KEY_EXCHANGE"
+  or
   name = "x25519" and nid = 1034 and normalized = "X25519" and algType = "ELLIPTIC_CURVE"
   or
+  name = "x25519" and nid = 1034 and normalized = "X25519" and algType = "KEY_EXCHANGE"
+  or
   name = "authecdsa" and nid = 1047 and normalized = "ECDSA" and algType = "SIGNATURE"
   or
   name = "authgost01" and nid = 1050 and normalized = "GOST" and algType = "SYMMETRIC_ENCRYPTION"

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

@@ -0,0 +1,122 @@
+import cpp
+private import semmle.code.cpp.models.Models
+private import semmle.code.cpp.models.interfaces.FormattingFunction
+
+private class IntLiteral extends Literal {
+  IntLiteral() {
+    //Heuristics for distinguishing int literals from other literals
+    exists(this.getValue().toInt()) and
+    not this instanceof CharLiteral and
+    not this instanceof StringLiteral
+  }
+}
+
+/**
+ * Holds if a StringLiteral could conceivably be used in some way for cryptography.
+ * Note: this predicate should only consider restrictions with respect to strings only.
+ * General restrictions are in the OpenSSLGenericSourceCandidateLiteral class.
+ */
+private predicate isOpenSSLStringLiteralGenericSourceCandidate(StringLiteral s) {
+  // 'EC' is a constant that may be used 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.
+  s.getValue() != "EC" and
+  // Ignore empty strings
+  s.getValue() != "" and
+  // Filter out strings with "%", to filter out format strings
+  not s.getValue().matches("%\\%%") and
+  // Filter out strings in brackets or braces (commonly seen strings not relevant for crypto)
+  not s.getValue().matches(["[%]", "(%)"]) and
+  // Filter out all strings of length 1, since these are not algorithm names
+  // NOTE/ASSUMPTION: If a user legitimately passes a string of length 1 to some configuration
+  // we will assume this is generally unknown. We may need to reassess this in the future.
+  s.getValue().length() > 1 and
+  // Ignore all strings that are in format string calls outputing to a stream (e.g., stdout)
+  not exists(FormattingFunctionCall f |
+    exists(f.getOutputArgument(true)) and s = f.(Call).getAnArgument()
+  ) and
+  // Ignore all format string calls where there is no known out param (resulting string)
+  // i.e., ignore printf, since it will just output a string and not produce a new string
+  not exists(FormattingFunctionCall f |
+    // Note: using two ways of determining if there is an out param, since I'm not sure
+    // which way is canonical
+    not exists(f.getOutputArgument(false)) and
+    not f.getTarget().hasTaintFlow(_, _) and
+    f.(Call).getAnArgument() = s
+  )
+}
+
+/**
+ * Holds if a StringLiteral could conceivably be used in some way for cryptography.
+ * Note: this predicate should only consider restrictions with respect to integers only.
+ * General restrictions are in the OpenSSLGenericSourceCandidateLiteral class.
+ */
+private predicate isOpenSSLIntLiteralGenericSourceCandidate(IntLiteral l) {
+  // Ignore integer values of 0, commonly referring to NULL only (no known algorithm 0)
+  l.getValue().toInt() != 0 and
+  // ASSUMPTION, no negative numbers are allowed
+  // RATIONALE: this is a performance improvement to avoid having to trace every number
+  not exists(UnaryMinusExpr u | u.getOperand() = l) and
+  //  OPENSSL has a special macro for getting every line, ignore it
+  not exists(MacroInvocation mi | mi.getExpr() = l and mi.getMacroName() = "OPENSSL_LINE") and
+  // Filter out cases where an int is returned into a pointer, e.g., return NULL;
+  not exists(ReturnStmt r |
+    r.getExpr() = l and
+    r.getEnclosingFunction().getType().getUnspecifiedType() instanceof DerivedType
+  ) and
+  // A literal as an array index should not be relevant for crypo
+  not exists(ArrayExpr op | op.getArrayOffset() = l) and
+  // A literal used in a bitwise should not be relevant for crypto
+  not exists(BinaryBitwiseOperation op | op.getAnOperand() = l) and
+  not exists(AssignBitwiseOperation op | op.getAnOperand() = l) and
+  //Filter out cases where an int is assigned or initialized into a pointer, e.g., char* x = NULL;
+  not exists(Assignment a |
+    a.getRValue() = l and
+    a.getLValue().getType().getUnspecifiedType() instanceof DerivedType
+  ) and
+  not exists(Initializer i |
+    i.getExpr() = l and
+    i.getDeclaration().getADeclarationEntry().getUnspecifiedType() instanceof DerivedType
+  ) and
+  // Filter out cases where the literal is used in any kind of arithmetic operation
+  not exists(BinaryArithmeticOperation op | op.getAnOperand() = l) and
+  not exists(UnaryArithmeticOperation op | op.getOperand() = l) and
+  not exists(AssignArithmeticOperation op | op.getAnOperand() = l) and
+  // If a literal has no parent ignore it, this is a workaround for the current inability
+  // to find a literal in an array declaration: int x[100];
+  // NOTE/ASSUMPTION: this value might actually be relevant for finding hard coded sizes
+  // consider size as inferred through the allocation of a buffer.
+  // In these cases, we advise that the source is not generic and must be traced explicitly.
+  exists(l.getParent())
+}
+
+/**
+ * Any literal that may be conceivably be used in some way for cryptography.
+ * The set of all literals is restricted by this class to cases where there is higher
+ * plausibility that the literal could be used as a source of configuration.
+ * Literals are filtered, for example, if they are used in a way no indicative of an algorithm use
+ * such as in an array index, bitwise operation, or logical operation.
+ * Note a case like this:
+ *      if(algVal == "AES")
+ *
+ * "AES" may be a legitimate algorithm literal, but the literal will not be used for an operation directly
+ * since it is in a equality comparison, hence this case would also be filtered.
+ */
+class OpenSSLGenericSourceCandidateLiteral extends Literal {
+  OpenSSLGenericSourceCandidateLiteral() {
+    (
+      isOpenSSLIntLiteralGenericSourceCandidate(this) or
+      isOpenSSLStringLiteralGenericSourceCandidate(this)
+    ) and
+    // ********* General filters beyond what is filtered for strings and ints *********
+    // An algorithm literal in a switch case will not be directly applied to an operation.
+    not exists(SwitchCase sc | sc.getExpr() = this) and
+    // A literal in a logical operation may be an algorithm, but not a candidate
+    // for the purposes of finding applied algorithms
+    not exists(BinaryLogicalOperation op | op.getAnOperand() = this) and
+    not exists(UnaryLogicalOperation op | op.getOperand() = this) and
+    // A literal in a comparison operation may be an algorithm, but not a candidate
+    // for the purposes of finding applied algorithms
+    not exists(ComparisonOperation op | op.getAnOperand() = this)
+  }
+}

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

@@ -3,4 +3,5 @@ module OpenSSLModel {
   import AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumers
   import Operations.OpenSSLOperations
   import Random
+  import GenericSourceCandidateLiteral
 }