Merge pull request github#5896 from geoffw0/weak_crypto

C++: Improve cpp/weak-cryptographic-algorithm

Author: MathiasVP

cpp/change-notes/2021-05-19-weak-cryptographic-algorithm.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* The "Use of a broken or risky cryptographic algorithm" (`cpp/weak-cryptographic-algorithm`) query has been enhanced to reduce false positive results, and (rarely) find more true positive results.

cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql

@@ -13,39 +13,111 @@
 import cpp
 import semmle.code.cpp.security.Encryption
 
-abstract class InsecureCryptoSpec extends Locatable {
-  abstract string description();
+/**
+ * A function which may relate to an insecure encryption algorithm.
+ */
+Function getAnInsecureEncryptionFunction() {
+  (
+    isInsecureEncryption(result.getName()) or
+    isInsecureEncryption(result.getAParameter().getName()) or
+    isInsecureEncryption(result.getDeclaringType().getName())
+  ) and
+  exists(result.getACallToThisFunction())
 }
 
-Function getAnInsecureFunction() {
-  result.getName().regexpMatch(getInsecureAlgorithmRegex()) and
+/**
+ * A function with additional evidence it is related to encryption.
+ */
+Function getAdditionalEvidenceFunction() {
+  (
+    isEncryptionAdditionalEvidence(result.getName()) or
+    isEncryptionAdditionalEvidence(result.getAParameter().getName())
+  ) and
   exists(result.getACallToThisFunction())
 }
 
-class InsecureFunctionCall extends InsecureCryptoSpec, FunctionCall {
-  InsecureFunctionCall() { this.getTarget() = getAnInsecureFunction() }
-
-  override string description() { result = "function call" }
-
-  override string toString() { result = FunctionCall.super.toString() }
-
-  override Location getLocation() { result = FunctionCall.super.getLocation() }
+/**
+ * A macro which may relate to an insecure encryption algorithm.
+ */
+Macro getAnInsecureEncryptionMacro() {
+  isInsecureEncryption(result.getName()) and
+  exists(result.getAnInvocation())
 }
 
-Macro getAnInsecureMacro() {
-  result.getName().regexpMatch(getInsecureAlgorithmRegex()) and
+/**
+ * A macro with additional evidence it is related to encryption.
+ */
+Macro getAdditionalEvidenceMacro() {
+  isEncryptionAdditionalEvidence(result.getName()) and
   exists(result.getAnInvocation())
 }
 
-class InsecureMacroSpec extends InsecureCryptoSpec, MacroInvocation {
-  InsecureMacroSpec() { this.getMacro() = getAnInsecureMacro() }
+/**
+ * An enum constant which may relate to an insecure encryption algorithm.
+ */
+EnumConstant getAnInsecureEncryptionEnumConst() { isInsecureEncryption(result.getName()) }
+
+/**
+ * An enum constant with additional evidence it is related to encryption.
+ */
+EnumConstant getAdditionalEvidenceEnumConst() { isEncryptionAdditionalEvidence(result.getName()) }
+
+/**
+ * A function call we have a high confidence is related to use of an insecure
+ * encryption algorithm.
+ */
+class InsecureFunctionCall extends FunctionCall {
+  Element blame;
+  string explain;
 
-  override string description() { result = "macro invocation" }
+  InsecureFunctionCall() {
+    // find use of an insecure algorithm name
+    (
+      getTarget() = getAnInsecureEncryptionFunction() and
+      blame = this and
+      explain = "function call"
+      or
+      exists(MacroInvocation mi |
+        (
+          mi.getAnExpandedElement() = this or
+          mi.getAnExpandedElement() = this.getAnArgument()
+        ) and
+        mi.getMacro() = getAnInsecureEncryptionMacro() and
+        blame = mi and
+        explain = "macro invocation"
+      )
+      or
+      exists(EnumConstantAccess ec |
+        ec = this.getAnArgument() and
+        ec.getTarget() = getAnInsecureEncryptionEnumConst() and
+        blame = ec and
+        explain = "enum constant access"
+      )
+    ) and
+    // find additional evidence that this function is related to encryption.
+    (
+      getTarget() = getAdditionalEvidenceFunction()
+      or
+      exists(MacroInvocation mi |
+        (
+          mi.getAnExpandedElement() = this or
+          mi.getAnExpandedElement() = this.getAnArgument()
+        ) and
+        mi.getMacro() = getAdditionalEvidenceMacro()
+      )
+      or
+      exists(EnumConstantAccess ec |
+        ec = this.getAnArgument() and
+        ec.getTarget() = getAdditionalEvidenceEnumConst()
+      )
+    )
+  }
 
-  override string toString() { result = MacroInvocation.super.toString() }
+  Element getBlame() { result = blame }
 
-  override Location getLocation() { result = MacroInvocation.super.getLocation() }
+  string getDescription() { result = explain }
 }
 
-from InsecureCryptoSpec c
-select c, "This " + c.description() + " specifies a broken or weak cryptographic algorithm."
+from InsecureFunctionCall c
+select c.getBlame(),
+  "This " + c.getDescription() + " specifies a broken or weak cryptographic algorithm."

cpp/ql/src/semmle/code/cpp/security/Encryption.qll

@@ -10,10 +10,18 @@ import cpp
 string getAnInsecureAlgorithmName() {
   result =
     [
-      "DES", "RC2", "RC4", "RC5", "ARCFOUR" // ARCFOUR is a variant of RC4
+      "DES", "RC2", "RC4", "RC5", "ARCFOUR", // ARCFOUR is a variant of RC4
+      "3DES", "DES3" // also appears separated, e.g. "TRIPLE-DES", which will be matched as "DES".
     ]
 }
 
+/**
+ * Gets the name of an algorithm that is known to be secure.
+ */
+string getASecureAlgorithmName() {
+  result = ["RSA", "SHA256", "CCM", "GCM", "AES", "Blowfish", "ECIES"]
+}
+
 /**
  * Gets the name of a hash algorithm that is insecure if it is being used for
  * encryption (but it is hard to know when that is happening).
@@ -23,25 +31,40 @@ string getAnInsecureHashAlgorithmName() { result = ["SHA1", "MD5"] }
 /**
  * Gets the regular expression used for matching strings that look like they
  * contain an algorithm that is known to be insecure.
+ *
+ * Consider using `isInsecureEncryption` rather than accessing this regular
+ * expression directly.
  */
 string getInsecureAlgorithmRegex() {
   result =
     // algorithms usually appear in names surrounded by characters that are not
-    // alphabetical characters in the same case. This handles the upper and lower
-    // case cases
-    "(^|.*[^A-Z])(" + strictconcat(getAnInsecureAlgorithmName(), "|") + ")([^A-Z].*|$)" + "|" +
-      // for lowercase, we want to be careful to avoid being confused by camelCase
-      // hence we require two preceding uppercase letters to be sure of a case switch,
-      // or a preceding non-alphabetic character
-      "(^|.*[A-Z]{2}|.*[^a-zA-Z])(" + strictconcat(getAnInsecureAlgorithmName().toLowerCase(), "|") +
-      ")([^a-z].*|$)"
+    // alphabetical characters in the same case or numerical digits. This
+    // handles the upper case:
+    "(^|.*[^A-Z0-9])(" + strictconcat(getAnInsecureAlgorithmName(), "|") + ")([^A-Z0-9].*|$)" + "|" +
+      // for lowercase, we want to be careful to avoid being confused by
+      //camelCase, hence we require two preceding uppercase letters to be
+      // sure of a case switch (or a preceding non-alphabetic, non-numeric
+      // character).
+      "(^|.*[A-Z]{2}|.*[^a-zA-Z0-9])(" +
+      strictconcat(getAnInsecureAlgorithmName().toLowerCase(), "|") + ")([^a-z0-9].*|$)"
 }
 
 /**
- * Gets the name of an algorithm that is known to be secure.
+ * Holds if `name` looks like it might be related to operations with an
+ * insecure encyption algorithm.
  */
-string getASecureAlgorithmName() {
-  result = ["RSA", "SHA256", "CCM", "GCM", "AES", "Blowfish", "ECIES"]
+bindingset[name]
+predicate isInsecureEncryption(string name) { name.regexpMatch(getInsecureAlgorithmRegex()) }
+
+/**
+ * Holds if there is additional evidence that `name` looks like it might be
+ * related to operations with an encyption algorithm, besides the name of a
+ * specific algorithm. This can be used in conjuction with
+ * `isInsecureEncryption` to produce a stronger heuristic.
+ */
+bindingset[name]
+predicate isEncryptionAdditionalEvidence(string name) {
+  name.toUpperCase().matches("%" + ["CRYPT", "CODE", "CODING", "CBC", "KEY", "CIPHER", "MAC"] + "%")
 }
 
 /**
@@ -51,14 +74,15 @@ string getASecureAlgorithmName() {
 string getSecureAlgorithmRegex() {
   result =
     // algorithms usually appear in names surrounded by characters that are not
-    // alphabetical characters in the same case. This handles the upper and lower
-    // case cases
-    "(^|.*[^A-Z])(" + strictconcat(getASecureAlgorithmName(), "|") + ")([^A-Z].*|$)" + "|" +
-      // for lowercase, we want to be careful to avoid being confused by camelCase
-      // hence we require two preceding uppercase letters to be sure of a case
-      // switch, or a preceding non-alphabetic character
-      "(^|.*[A-Z]{2}|.*[^a-zA-Z])(" + strictconcat(getASecureAlgorithmName().toLowerCase(), "|") +
-      ")([^a-z].*|$)"
+    // alphabetical characters in the same case or numerical digits. This
+    // handles the upper case:
+    "(^|.*[^A-Z0-9])(" + strictconcat(getASecureAlgorithmName(), "|") + ")([^A-Z0-9].*|$)" + "|" +
+      // for lowercase, we want to be careful to avoid being confused by
+      //camelCase, hence we require two preceding uppercase letters to be
+      // sure of a case switch (or a preceding non-alphabetic, non-numeric
+      // character).
+      "(^|.*[A-Z]{2}|.*[^a-zA-Z0-9])(" + strictconcat(getASecureAlgorithmName().toLowerCase(), "|") +
+      ")([^a-z0-9].*|$)"
 }
 
 /**

cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected

@@ -0,0 +1,25 @@
+| test2.cpp:49:4:49:24 | call to my_des_implementation | This function call specifies a broken or weak cryptographic algorithm. |
+| test2.cpp:62:33:62:40 | ALGO_DES | This macro invocation specifies a broken or weak cryptographic algorithm. |
+| test2.cpp:124:4:124:24 | call to my_des_implementation | This function call specifies a broken or weak cryptographic algorithm. |
+| test2.cpp:144:27:144:29 | DES | This enum constant access specifies a broken or weak cryptographic algorithm. |
+| test2.cpp:172:28:172:35 | ALGO_DES | This macro invocation specifies a broken or weak cryptographic algorithm. |
+| test2.cpp:175:28:175:34 | USE_DES | This enum constant access specifies a broken or weak cryptographic algorithm. |
+| test2.cpp:182:38:182:45 | ALGO_DES | This macro invocation specifies a broken or weak cryptographic algorithm. |
+| test2.cpp:185:38:185:44 | USE_DES | This enum constant access specifies a broken or weak cryptographic algorithm. |
+| test2.cpp:238:2:238:20 | call to encrypt | This function call specifies a broken or weak cryptographic algorithm. |
+| test2.cpp:245:5:245:11 | call to encrypt | This function call specifies a broken or weak cryptographic algorithm. |
+| test.cpp:38:2:38:31 | ENCRYPT_WITH_DES(data,amount) | This macro invocation specifies a broken or weak cryptographic algorithm. |
+| test.cpp:39:2:39:31 | ENCRYPT_WITH_RC2(data,amount) | This macro invocation specifies a broken or weak cryptographic algorithm. |
+| test.cpp:41:2:41:32 | ENCRYPT_WITH_3DES(data,amount) | This macro invocation specifies a broken or weak cryptographic algorithm. |
+| test.cpp:42:2:42:38 | ENCRYPT_WITH_TRIPLE_DES(data,amount) | This macro invocation specifies a broken or weak cryptographic algorithm. |
+| test.cpp:51:2:51:32 | DES_DO_ENCRYPTION(data,amount) | This macro invocation specifies a broken or weak cryptographic algorithm. |
+| test.cpp:52:2:52:31 | RUN_DES_ENCODING(data,amount) | This macro invocation specifies a broken or weak cryptographic algorithm. |
+| test.cpp:53:2:53:25 | DES_ENCODE(data,amount) | This macro invocation specifies a broken or weak cryptographic algorithm. |
+| test.cpp:54:2:54:26 | DES_SET_KEY(data,amount) | This macro invocation specifies a broken or weak cryptographic algorithm. |
+| test.cpp:88:2:88:11 | call to encryptDES | This function call specifies a broken or weak cryptographic algorithm. |
+| test.cpp:89:2:89:11 | call to encryptRC2 | This function call specifies a broken or weak cryptographic algorithm. |
+| test.cpp:91:2:91:12 | call to encrypt3DES | This function call specifies a broken or weak cryptographic algorithm. |
+| test.cpp:92:2:92:17 | call to encryptTripleDES | This function call specifies a broken or weak cryptographic algorithm. |
+| test.cpp:101:2:101:15 | call to do_des_encrypt | This function call specifies a broken or weak cryptographic algorithm. |
+| test.cpp:102:2:102:12 | call to DES_Set_Key | This function call specifies a broken or weak cryptographic algorithm. |
+| test.cpp:121:2:121:24 | INIT_ENCRYPT_WITH_DES() | This macro invocation specifies a broken or weak cryptographic algorithm. |

cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.qlref

@@ -0,0 +1 @@
+Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql

cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp

@@ -0,0 +1,125 @@
+
+typedef unsigned long size_t;
+
+// --- simple encryption macro invocations ---
+
+void my_implementation1(void *data, size_t amount);
+void my_implementation2(void *data, size_t amount);
+void my_implementation3(void *data, size_t amount);
+void my_implementation4(void *data, size_t amount);
+void my_implementation5(void *data, size_t amount);
+void my_implementation6(const char *str);
+
+#define ENCRYPT_WITH_DES(data, amount) my_implementation1(data, amount)
+#define ENCRYPT_WITH_RC2(data, amount) my_implementation2(data, amount)
+#define ENCRYPT_WITH_AES(data, amount) my_implementation3(data, amount)
+#define ENCRYPT_WITH_3DES(data, amount) my_implementation4(data, amount)
+#define ENCRYPT_WITH_TRIPLE_DES(data, amount) my_implementation4(data, amount)
+#define ENCRYPT_WITH_RC20(data, amount) my_implementation5(data, amount)
+#define ENCRYPT_WITH_DES_REMOVED(data, amount)
+
+#define DESENCRYPT(data, amount) my_implementation1(data, amount)
+#define RC2ENCRYPT(data, amount) my_implementation2(data, amount)
+#define AESENCRYPT(data, amount) my_implementation3(data, amount)
+#define DES3ENCRYPT(data, amount) my_implementation4(data, amount)
+
+#define DES_DO_ENCRYPTION(data, amount) my_implementation1(data, amount)
+#define RUN_DES_ENCODING(data, amount) my_implementation1(data, amount)
+#define DES_ENCODE(data, amount) my_implementation1(data, amount)
+#define DES_SET_KEY(data, amount) my_implementation1(data, amount)
+
+#define DES(str) my_implementation6(str)
+#define DESMOND(str) my_implementation6(str)
+#define ANODES(str) my_implementation6(str)
+#define SORT_ORDER_DES (1)
+
+void test_macros(void *data, size_t amount, const char *str)
+{
+	ENCRYPT_WITH_DES(data, amount); // BAD
+	ENCRYPT_WITH_RC2(data, amount); // BAD
+	ENCRYPT_WITH_AES(data, amount); // GOOD (good algorithm)
+	ENCRYPT_WITH_3DES(data, amount); // BAD
+	ENCRYPT_WITH_TRIPLE_DES(data, amount); // BAD
+	ENCRYPT_WITH_RC20(data, amount); // GOOD (if there ever is an RC20 algorithm, we have no reason to believe it's weak)
+	ENCRYPT_WITH_DES_REMOVED(data, amount); // GOOD (implementation has been deleted)
+
+	DESENCRYPT(data, amount); // BAD [NOT DETECTED]
+	RC2ENCRYPT(data, amount); // BAD [NOT DETECTED]
+	AESENCRYPT(data, amount); // GOOD (good algorithm)
+	DES3ENCRYPT(data, amount); // BAD [NOT DETECTED]
+
+	DES_DO_ENCRYPTION(data, amount); // BAD
+	RUN_DES_ENCODING(data, amount); // BAD
+	DES_ENCODE(data, amount); // BAD
+	DES_SET_KEY(data, amount); // BAD
+
+	DES(str); // GOOD (probably nothing to do with encryption)
+	DESMOND(str); // GOOD (probably nothing to do with encryption)
+	ANODES(str); // GOOD (probably nothing to do with encryption)
+	int ord = SORT_ORDER_DES; // GOOD (probably nothing to do with encryption)
+}
+
+// --- simple encryption function calls ---
+
+void encryptDES(void *data, size_t amount);
+void encryptRC2(void *data, size_t amount);
+void encryptAES(void *data, size_t amount);
+void encrypt3DES(void *data, size_t amount);
+void encryptTripleDES(void *data, size_t amount);
+
+void DESEncrypt(void *data, size_t amount);
+void RC2Encrypt(void *data, size_t amount);
+void AESEncrypt(void *data, size_t amount);
+void DES3Encrypt(void *data, size_t amount);
+
+void DoDESEncryption(void *data, size_t amount);
+void encryptDes(void *data, size_t amount);
+void do_des_encrypt(void *data, size_t amount);
+void DES_Set_Key(const char *key);
+void DESSetKey(const char *key);
+
+int Des();
+void Desmond(const char *str);
+void Anodes(int i);
+void ConDes();
+
+void test_functions(void *data, size_t amount, const char *str)
+{
+	encryptDES(data, amount); // BAD
+	encryptRC2(data, amount); // BAD
+	encryptAES(data, amount); // GOOD (good algorithm)
+	encrypt3DES(data, amount); // BAD
+	encryptTripleDES(data, amount); // BAD
+
+	DESEncrypt(data, amount); // BAD [NOT DETECTED]
+	RC2Encrypt(data, amount); // BAD [NOT DETECTED]
+	AESEncrypt(data, amount); // GOOD (good algorithm)
+	DES3Encrypt(data, amount); // BAD [NOT DETECTED]
+
+	DoDESEncryption(data, amount); // BAD [NOT DETECTED]
+	encryptDes(data, amount); // BAD [NOT DETECTED]
+	do_des_encrypt(data, amount); // BAD
+	DES_Set_Key(str); // BAD
+	DESSetKey(str); // BAD [NOT DETECTED]
+
+	Des(); // GOOD (probably nothing to do with encryption)
+	Desmond(str); // GOOD (probably nothing to do with encryption)
+	Anodes(1); // GOOD (probably nothing to do with encryption)
+	ConDes(); // GOOD (probably nothing to do with encryption)
+}
+
+// --- macros for functions with no arguments ---
+
+void my_implementation7();
+void my_implementation8();
+
+#define INIT_ENCRYPT_WITH_DES() my_implementation7()
+#define INIT_ENCRYPT_WITH_AES() my_implementation8()
+
+void test_macros2()
+{
+	INIT_ENCRYPT_WITH_DES(); // BAD
+	INIT_ENCRYPT_WITH_AES(); // GOOD (good algorithm)
+	
+	// ...
+}