From 109fa4d38e598ef7c8558afcbd0cd58655362067 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Wed, 12 May 2021 13:54:07 +0100
Subject: [PATCH 01/25] C++: Add test cases for BrokenCryptoAlgorithm.ql.

---
 .../CWE-327/BrokenCryptoAlgorithm.expected    |  29 ++
 .../CWE/CWE-327/BrokenCryptoAlgorithm.qlref   |   1 +
 .../query-tests/Security/CWE/CWE-327/test.cpp | 109 ++++++++
 .../Security/CWE/CWE-327/test2.cpp            | 254 ++++++++++++++++++
 4 files changed, 393 insertions(+)
 create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
 create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.qlref
 create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
 create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp

diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
new file mode 100644
index 000000000000..47f6e1daa7ed
--- /dev/null
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
@@ -0,0 +1,29 @@
+| test2.cpp:25:2:25:9 | ALGO_DES | This macro invocation specifies a broken or weak cryptographic algorithm. |
+| test2.cpp:33:7:33:14 | ALGO_DES | This macro invocation specifies a broken or weak cryptographic algorithm. |
+| test2.cpp:47:7:47:14 | ALGO_DES | This macro invocation specifies a broken or weak cryptographic algorithm. |
+| 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:172:28:172:35 | ALGO_DES | This macro invocation 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:192:26:192:33 | ALGO_DES | This macro invocation 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:43:2:43:32 | ENCRYPT_WITH_RC20(data,amount) | This macro invocation specifies a broken or weak cryptographic algorithm. |
+| test.cpp:44:2:44:39 | ENCRYPT_WITH_DES_REMOVED(data,amount) | This macro invocation specifies a broken or weak cryptographic algorithm. |
+| test.cpp:49:2:49:26 | DES3ENCRYPT(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:56:2:56:9 | DES(str) | This macro invocation specifies a broken or weak cryptographic algorithm. |
+| test.cpp:59:12:59:25 | SORT_ORDER_DES | 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:97:2:97:12 | call to DES3Encrypt | 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. |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.qlref b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.qlref
new file mode 100644
index 000000000000..8424dee1a9b6
--- /dev/null
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.qlref
@@ -0,0 +1 @@
+Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
\ No newline at end of file
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
new file mode 100644
index 000000000000..4a03b8a52616
--- /dev/null
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
@@ -0,0 +1,109 @@
+
+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); // GOOD (good enough algorithm) [FALSE POSITIVE]
+	ENCRYPT_WITH_TRIPLE_DES(data, amount); // GOOD (good enough algorithm) [FALSE POSITIVE]
+	ENCRYPT_WITH_RC20(data, amount); // GOOD (if there ever is an RC20 algorithm, we have no reason to believe it's weak) [FALSE POSITIVE]
+	ENCRYPT_WITH_DES_REMOVED(data, amount); // GOOD (implementation has been deleted) [FALSE POSITIVE]
+
+	DESENCRYPT(data, amount); // BAD [NOT DETECTED]
+	RC2ENCRYPT(data, amount); // BAD [NOT DETECTED]
+	AESENCRYPT(data, amount); // GOOD (good algorithm)
+	DES3ENCRYPT(data, amount); // GOOD (good enough algorithm) [FALSE POSITIVE]
+
+	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) [FALSE POSITIVE]
+	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) [FALSE POSITIVE]
+}
+
+// --- 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); // GOOD (good enough algorithm) [FALSE POSITIVE]
+	encryptTripleDES(data, amount); // GOOD (good enough algorithm) [FALSE POSITIVE]
+
+	DESEncrypt(data, amount); // BAD
+	RC2Encrypt(data, amount); // BAD
+	AESEncrypt(data, amount); // GOOD (good algorithm)
+	DES3Encrypt(data, amount); // GOOD (good enough algorithm) [FALSE POSITIVE]
+
+	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)
+}
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp
new file mode 100644
index 000000000000..774d541950f3
--- /dev/null
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp
@@ -0,0 +1,254 @@
+
+typedef unsigned long size_t;
+
+int strcmp(const char *s1, const char *s2);
+void abort(void);
+
+struct keytype
+{
+    char data[16];
+};
+
+void my_des_implementation(char *data, size_t amount, keytype key);
+void my_rc2_implementation(char *data, size_t amount, keytype key);
+void my_aes_implementation(char *data, size_t amount, keytype key);
+void my_3des_implementation(char *data, size_t amount, keytype key);
+
+typedef void (*implementation_fn_ptr)(char *data, size_t amount, keytype key);
+
+// --- more involved C-style example ---
+
+#define ALGO_DES (1)
+#define ALGO_AES (2)
+
+int all_algos[] = {
+	ALGO_DES, // [FALSE POSITIVE]
+	ALGO_AES
+};
+
+void encrypt_good(char *data, size_t amount, keytype key, int algo)
+{
+	switch (algo)
+	{
+	case ALGO_DES: // [FALSE POSITIVE]
+		abort();
+
+	case ALGO_AES:
+		{
+			my_aes_implementation(data, amount, key); // GOOD
+		} break;
+	}
+};
+
+void encrypt_bad(char *data, size_t amount, keytype key, int algo)
+{
+	switch (algo)
+	{
+	case ALGO_DES:
+		{
+			my_des_implementation(data, amount, key); // BAD
+		} break;
+
+	case ALGO_AES:
+		{
+			my_aes_implementation(data, amount, key); // GOOD
+		} break;
+	}
+};
+
+void do_encrypts(char *data, size_t amount, keytype key)
+{
+	encrypt_good(data, amount, key, ALGO_AES); // GOOD
+	encrypt_bad(data, amount, key, ALGO_DES); // BAD
+}
+
+// --- more involved CPP-style example ---
+
+enum algorithm
+{
+	DES,
+	AES
+};
+
+algorithm all_algorithms[] = {
+	DES,
+	AES
+};
+
+class MyGoodEncryptor
+{
+public:
+	MyGoodEncryptor(keytype _key, algorithm _algo) : key(_key), algo(_algo) {};
+
+	void encrypt(char *data, size_t amount);
+
+private:
+	keytype key;
+	algorithm algo;
+};
+
+void MyGoodEncryptor :: encrypt(char *data, size_t amount)
+{
+	switch (algo)
+	{
+	case DES:
+		{
+			throw "DES is not a good choice of encryption algorithm!";
+		} break;
+
+	case AES:
+		{
+			my_aes_implementation(data, amount, key); // GOOD
+		} break;
+	}
+}
+
+class MyBadEncryptor
+{
+public:
+	MyBadEncryptor(keytype _key, algorithm _algo) : key(_key), algo(_algo) {};
+
+	void encrypt(char *data, size_t amount);
+
+private:
+	keytype key;
+	algorithm algo;
+};
+
+void MyBadEncryptor :: encrypt(char *data, size_t amount)
+{
+	switch (algo)
+	{
+	case DES:
+		{
+			my_des_implementation(data, amount, key); // BAD
+		} break;
+
+	case AES:
+		{
+			my_aes_implementation(data, amount, key); // GOOD
+		} break;
+	}
+}
+
+void do_class_encrypts(char *data, size_t amount, keytype key)
+{
+	{
+		MyGoodEncryptor mge(key, AES); // GOOD
+
+		mge.encrypt(data, amount);
+
+	}
+
+	{
+		MyBadEncryptor mbe(key, DES); // BAD [NOT DETECTED]
+
+		mbe.encrypt(data, amount);
+	}
+}
+
+// --- unseen implementation ---
+
+enum use_algorithm
+{
+	USE_DES,
+	USE_AES
+};
+
+void set_encryption_algorithm1(int algorithm);
+void set_encryption_algorithm2(use_algorithm algorithm);
+void set_encryption_algorithm3(const char *algorithm_str);
+
+void encryption_with1(char *data, size_t amount, keytype key, int algorithm);
+void encryption_with2(char *data, size_t amount, keytype key, use_algorithm algorithm);
+void encryption_with3(char *data, size_t amount, keytype key, const char *algorithm_str);
+
+int get_algorithm1();
+use_algorithm get_algorithm2();
+const char *get_algorithm3();
+
+void do_unseen_encrypts(char *data, size_t amount, keytype key)
+{
+	set_encryption_algorithm1(ALGO_DES); // BAD
+	set_encryption_algorithm1(ALGO_AES); // GOOD
+
+	set_encryption_algorithm2(USE_DES); // BAD [NOT DETECTED]
+	set_encryption_algorithm2(USE_AES); // GOOD
+
+	set_encryption_algorithm3("DES"); // BAD [NOT DETECTED]
+	set_encryption_algorithm3("AES"); // GOOD
+	set_encryption_algorithm3("AES-256"); // GOOD
+
+	encryption_with1(data, amount, key, ALGO_DES); // BAD
+	encryption_with1(data, amount, key, ALGO_AES); // GOOD
+
+	encryption_with2(data, amount, key, USE_DES); // BAD [NOT DETECTED]
+	encryption_with2(data, amount, key, USE_AES); // GOOD
+
+	encryption_with3(data, amount, key, "DES"); // BAD [NOT DETECTED]
+	encryption_with3(data, amount, key, "AES"); // GOOD
+	encryption_with3(data, amount, key, "AES-256"); // GOOD
+
+	if (get_algorithm1() == ALGO_DES) // GOOD [FALSE POSITIVE]
+	{
+		throw "DES is not a good choice of encryption algorithm!";
+	}
+	if (get_algorithm2() == USE_DES) // GOOD
+	{
+		throw "DES is not a good choice of encryption algorithm!";
+	}
+	if (strcmp(get_algorithm3(), "DES") == 0) // GOOD
+	{
+		throw "DES is not a good choice of encryption algorithm!";
+	}
+}
+
+// --- classes ---
+
+class desEncrypt
+{
+public:
+	static void encrypt(const char *data);
+};
+
+class aes256Encrypt
+{
+public:
+	static void encrypt(const char *data);
+};
+
+class desCipher
+{
+public:
+	void encrypt(const char *data);
+};
+
+class aesCipher
+{
+public:
+	void encrypt(const char *data);
+};
+
+void do_classes(const char *data)
+{
+	desEncrypt::encrypt(data); // BAD [NOT DETECTED]
+	aes256Encrypt::encrypt(data); // GOOD
+
+	desCipher dc;
+	aesCipher ac;
+	dc.encrypt(data); // BAD [NOT DETECTED]
+	ac.encrypt(data); // GOOD
+}
+
+// --- function pointer ---
+
+void do_fn_ptr(char *data, size_t amount, keytype key)
+{
+	implementation_fn_ptr impl;
+
+	impl = &my_des_implementation; // BAD [NOT DETECTED]
+	impl(data, amount, key);
+
+	impl = &my_aes_implementation; // GOOD
+	impl(data, amount, key);
+}

From b6d5f7c315b71b8ac135a53cf1796e86dd18fc03 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Wed, 12 May 2021 14:45:59 +0100
Subject: [PATCH 02/25] C++: Fix FPs caused by substring regexp.

---
 .../semmle/code/cpp/security/Encryption.qll   | 34 ++++++++++---------
 .../CWE-327/BrokenCryptoAlgorithm.expected    |  5 ---
 .../query-tests/Security/CWE/CWE-327/test.cpp | 10 +++---
 3 files changed, 23 insertions(+), 26 deletions(-)

diff --git a/cpp/ql/src/semmle/code/cpp/security/Encryption.qll b/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
index 606242e833c8..24c69f93758f 100644
--- a/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
+++ b/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
@@ -27,14 +27,15 @@ string getAnInsecureHashAlgorithmName() { result = ["SHA1", "MD5"] }
 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].*|$)"
 }
 
 /**
@@ -51,14 +52,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].*|$)"
 }
 
 /**
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
index 47f6e1daa7ed..f4ecaf8560fc 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
@@ -9,11 +9,8 @@
 | test2.cpp:192:26:192:33 | ALGO_DES | This macro invocation 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:43:2:43:32 | ENCRYPT_WITH_RC20(data,amount) | This macro invocation specifies a broken or weak cryptographic algorithm. |
 | test.cpp:44:2:44:39 | ENCRYPT_WITH_DES_REMOVED(data,amount) | This macro invocation specifies a broken or weak cryptographic algorithm. |
-| test.cpp:49:2:49:26 | DES3ENCRYPT(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. |
@@ -22,8 +19,6 @@
 | test.cpp:59:12:59:25 | SORT_ORDER_DES | 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:97:2:97:12 | call to DES3Encrypt | 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. |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
index 4a03b8a52616..da73b8b35fa9 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
@@ -38,15 +38,15 @@ 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); // GOOD (good enough algorithm) [FALSE POSITIVE]
+	ENCRYPT_WITH_3DES(data, amount); // GOOD (good enough algorithm)
 	ENCRYPT_WITH_TRIPLE_DES(data, amount); // GOOD (good enough algorithm) [FALSE POSITIVE]
-	ENCRYPT_WITH_RC20(data, amount); // GOOD (if there ever is an RC20 algorithm, we have no reason to believe it's weak) [FALSE POSITIVE]
+	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) [FALSE POSITIVE]
 
 	DESENCRYPT(data, amount); // BAD [NOT DETECTED]
 	RC2ENCRYPT(data, amount); // BAD [NOT DETECTED]
 	AESENCRYPT(data, amount); // GOOD (good algorithm)
-	DES3ENCRYPT(data, amount); // GOOD (good enough algorithm) [FALSE POSITIVE]
+	DES3ENCRYPT(data, amount); // GOOD (good enough algorithm)
 
 	DES_DO_ENCRYPTION(data, amount); // BAD
 	RUN_DES_ENCODING(data, amount); // BAD
@@ -88,13 +88,13 @@ 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); // GOOD (good enough algorithm) [FALSE POSITIVE]
+	encrypt3DES(data, amount); // GOOD (good enough algorithm)
 	encryptTripleDES(data, amount); // GOOD (good enough algorithm) [FALSE POSITIVE]
 
 	DESEncrypt(data, amount); // BAD
 	RC2Encrypt(data, amount); // BAD
 	AESEncrypt(data, amount); // GOOD (good algorithm)
-	DES3Encrypt(data, amount); // GOOD (good enough algorithm) [FALSE POSITIVE]
+	DES3Encrypt(data, amount); // GOOD (good enough algorithm)
 
 	DoDESEncryption(data, amount); // BAD [NOT DETECTED]
 	encryptDes(data, amount); // BAD [NOT DETECTED]

From 9404d0676dc6a7ab4b8a3216cb65c07f8e627c19 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Wed, 12 May 2021 14:51:47 +0100
Subject: [PATCH 03/25] C++: Exclude macros that don't generate anything.

---
 .../Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql    | 12 ++++++++++--
 .../CWE/CWE-327/BrokenCryptoAlgorithm.expected       |  1 -
 .../test/query-tests/Security/CWE/CWE-327/test.cpp   |  2 +-
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
index af64a1789c3b..b8f863812169 100644
--- a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
+++ b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
@@ -23,7 +23,10 @@ Function getAnInsecureFunction() {
 }
 
 class InsecureFunctionCall extends InsecureCryptoSpec, FunctionCall {
-  InsecureFunctionCall() { this.getTarget() = getAnInsecureFunction() }
+  InsecureFunctionCall() {
+    // the function name suggests it relates to an insecure crypto algorithm.
+    this.getTarget() = getAnInsecureFunction()
+  }
 
   override string description() { result = "function call" }
 
@@ -38,7 +41,12 @@ Macro getAnInsecureMacro() {
 }
 
 class InsecureMacroSpec extends InsecureCryptoSpec, MacroInvocation {
-  InsecureMacroSpec() { this.getMacro() = getAnInsecureMacro() }
+  InsecureMacroSpec() {
+    // the macro name suggests it relates to an insecure crypto algorithm.
+    this.getMacro() = getAnInsecureMacro() and
+    // the macro invocation generates something.
+    exists(this.getAGeneratedElement())
+  }
 
   override string description() { result = "macro invocation" }
 
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
index f4ecaf8560fc..62fdec370cea 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
@@ -10,7 +10,6 @@
 | 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:42:2:42:38 | ENCRYPT_WITH_TRIPLE_DES(data,amount) | This macro invocation specifies a broken or weak cryptographic algorithm. |
-| test.cpp:44:2:44:39 | ENCRYPT_WITH_DES_REMOVED(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. |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
index da73b8b35fa9..2fbd9ae73d5c 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
@@ -41,7 +41,7 @@ void test_macros(void *data, size_t amount, const char *str)
 	ENCRYPT_WITH_3DES(data, amount); // GOOD (good enough algorithm)
 	ENCRYPT_WITH_TRIPLE_DES(data, amount); // GOOD (good enough algorithm) [FALSE POSITIVE]
 	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) [FALSE POSITIVE]
+	ENCRYPT_WITH_DES_REMOVED(data, amount); // GOOD (implementation has been deleted)
 
 	DESENCRYPT(data, amount); // BAD [NOT DETECTED]
 	RC2ENCRYPT(data, amount); // BAD [NOT DETECTED]

From 52a88af6c1e7f2ffbb51dfc3fe8f499ca95ba189 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Wed, 12 May 2021 14:58:21 +0100
Subject: [PATCH 04/25] C++: Exclude macro invocations in switch case
 expressions.

---
 cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql     | 5 ++++-
 .../Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected      | 3 ---
 cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp       | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
index b8f863812169..296f6bec3f85 100644
--- a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
+++ b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
@@ -45,7 +45,10 @@ class InsecureMacroSpec extends InsecureCryptoSpec, MacroInvocation {
     // the macro name suggests it relates to an insecure crypto algorithm.
     this.getMacro() = getAnInsecureMacro() and
     // the macro invocation generates something.
-    exists(this.getAGeneratedElement())
+    exists(this.getAGeneratedElement().(ControlFlowNode)) and
+    // exclude expressions controlling ifs/switches (as they may not be used).
+    not any(IfStmt c).getCondition().getAChild*() = this.getAGeneratedElement() and
+    not any(SwitchCase c).getExpr().getAChild*() = this.getAGeneratedElement()
   }
 
   override string description() { result = "macro invocation" }
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
index 62fdec370cea..35be30d162cd 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
@@ -1,12 +1,9 @@
 | test2.cpp:25:2:25:9 | ALGO_DES | This macro invocation specifies a broken or weak cryptographic algorithm. |
-| test2.cpp:33:7:33:14 | ALGO_DES | This macro invocation specifies a broken or weak cryptographic algorithm. |
-| test2.cpp:47:7:47:14 | ALGO_DES | This macro invocation specifies a broken or weak cryptographic algorithm. |
 | 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:172:28:172:35 | ALGO_DES | This macro invocation 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:192:26:192:33 | ALGO_DES | This macro invocation 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:42:2:42:38 | ENCRYPT_WITH_TRIPLE_DES(data,amount) | This macro invocation specifies a broken or weak cryptographic algorithm. |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp
index 774d541950f3..80e8691426cb 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp
@@ -30,7 +30,7 @@ void encrypt_good(char *data, size_t amount, keytype key, int algo)
 {
 	switch (algo)
 	{
-	case ALGO_DES: // [FALSE POSITIVE]
+	case ALGO_DES:
 		abort();
 
 	case ALGO_AES:
@@ -189,7 +189,7 @@ void do_unseen_encrypts(char *data, size_t amount, keytype key)
 	encryption_with3(data, amount, key, "AES"); // GOOD
 	encryption_with3(data, amount, key, "AES-256"); // GOOD
 
-	if (get_algorithm1() == ALGO_DES) // GOOD [FALSE POSITIVE]
+	if (get_algorithm1() == ALGO_DES) // GOOD
 	{
 		throw "DES is not a good choice of encryption algorithm!";
 	}

From 0450caa73df21b92914b4633ceba4f613aea8b74 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Wed, 12 May 2021 19:39:30 +0100
Subject: [PATCH 05/25] C++: Exclude array initializers.

---
 cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql      | 4 +++-
 .../Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected       | 1 -
 cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp        | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
index 296f6bec3f85..3015528114db 100644
--- a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
+++ b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
@@ -48,7 +48,9 @@ class InsecureMacroSpec extends InsecureCryptoSpec, MacroInvocation {
     exists(this.getAGeneratedElement().(ControlFlowNode)) and
     // exclude expressions controlling ifs/switches (as they may not be used).
     not any(IfStmt c).getCondition().getAChild*() = this.getAGeneratedElement() and
-    not any(SwitchCase c).getExpr().getAChild*() = this.getAGeneratedElement()
+    not any(SwitchCase c).getExpr().getAChild*() = this.getAGeneratedElement() and
+    // exclude expressions in array initializers (as they may not be used).
+    not any(AggregateLiteral i).getAChild*() = this.getAGeneratedElement() 
   }
 
   override string description() { result = "macro invocation" }
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
index 35be30d162cd..8b17f1a83b70 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
@@ -1,4 +1,3 @@
-| test2.cpp:25:2:25:9 | ALGO_DES | This macro invocation specifies a broken or weak cryptographic algorithm. |
 | 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. |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp
index 80e8691426cb..622db3aff0ed 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp
@@ -22,7 +22,7 @@ typedef void (*implementation_fn_ptr)(char *data, size_t amount, keytype key);
 #define ALGO_AES (2)
 
 int all_algos[] = {
-	ALGO_DES, // [FALSE POSITIVE]
+	ALGO_DES,
 	ALGO_AES
 };
 

From 40cf29b62521ada08c8dbfeffa28027c1f283045 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 13 May 2021 08:38:58 +0100
Subject: [PATCH 06/25] C++: Rearrange the library.

---
 .../Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql  |  4 ++--
 cpp/ql/src/semmle/code/cpp/security/Encryption.qll | 14 +++++++++++---
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
index 3015528114db..f1195a437369 100644
--- a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
+++ b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
@@ -18,7 +18,7 @@ abstract class InsecureCryptoSpec extends Locatable {
 }
 
 Function getAnInsecureFunction() {
-  result.getName().regexpMatch(getInsecureAlgorithmRegex()) and
+  isInsecureEncryption(result.getName()) and
   exists(result.getACallToThisFunction())
 }
 
@@ -36,7 +36,7 @@ class InsecureFunctionCall extends InsecureCryptoSpec, FunctionCall {
 }
 
 Macro getAnInsecureMacro() {
-  result.getName().regexpMatch(getInsecureAlgorithmRegex()) and
+  isInsecureEncryption(result.getName()) and
   exists(result.getAnInvocation())
 }
 
diff --git a/cpp/ql/src/semmle/code/cpp/security/Encryption.qll b/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
index 24c69f93758f..22ca62372fed 100644
--- a/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
+++ b/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
@@ -14,6 +14,13 @@ string getAnInsecureAlgorithmName() {
     ]
 }
 
+/**
+ * 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).
@@ -39,10 +46,11 @@ string getInsecureAlgorithmRegex() {
 }
 
 /**
- * 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())
 }
 
 /**

From 123889a6716c16aee3febcbf820348f0c92c2e2d Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 13 May 2021 09:15:58 +0100
Subject: [PATCH 07/25] C++: Fix 'triple DES' false positives.

---
 cpp/ql/src/semmle/code/cpp/security/Encryption.qll    | 11 +++++++++--
 .../CWE/CWE-327/BrokenCryptoAlgorithm.expected        |  2 --
 cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp |  4 ++--
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/cpp/ql/src/semmle/code/cpp/security/Encryption.qll b/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
index 22ca62372fed..d2261df6bc8e 100644
--- a/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
+++ b/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
@@ -30,6 +30,9 @@ 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 =
@@ -49,8 +52,12 @@ string getInsecureAlgorithmRegex() {
  * Holds if `name` looks like it might be related to operations with an
  * insecure encyption algorithm.
  */
-bindingset[name] predicate isInsecureEncryption(string name) {
-  name.regexpMatch(getInsecureAlgorithmRegex())
+bindingset[name]
+predicate isInsecureEncryption(string name) {
+  name.regexpMatch(getInsecureAlgorithmRegex()) and
+  // Check for evidence that an otherwise matching name may in fact not be
+  // related to insecure encrpytion, e.g. "Triple-DES" is not "DES".
+  not name.toUpperCase().regexpMatch(".*TRIPLE.*")
 }
 
 /**
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
index 8b17f1a83b70..53e9054a5070 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
@@ -5,7 +5,6 @@
 | test2.cpp:182:38:182:45 | ALGO_DES | This macro invocation 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: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. |
@@ -14,6 +13,5 @@
 | test.cpp:59:12:59:25 | SORT_ORDER_DES | 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: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. |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
index 2fbd9ae73d5c..8d16e5ed98e9 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
@@ -39,7 +39,7 @@ void test_macros(void *data, size_t amount, const char *str)
 	ENCRYPT_WITH_RC2(data, amount); // BAD
 	ENCRYPT_WITH_AES(data, amount); // GOOD (good algorithm)
 	ENCRYPT_WITH_3DES(data, amount); // GOOD (good enough algorithm)
-	ENCRYPT_WITH_TRIPLE_DES(data, amount); // GOOD (good enough algorithm) [FALSE POSITIVE]
+	ENCRYPT_WITH_TRIPLE_DES(data, amount); // GOOD (good enough algorithm)
 	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)
 
@@ -89,7 +89,7 @@ void test_functions(void *data, size_t amount, const char *str)
 	encryptRC2(data, amount); // BAD
 	encryptAES(data, amount); // GOOD (good algorithm)
 	encrypt3DES(data, amount); // GOOD (good enough algorithm)
-	encryptTripleDES(data, amount); // GOOD (good enough algorithm) [FALSE POSITIVE]
+	encryptTripleDES(data, amount); // GOOD (good enough algorithm)
 
 	DESEncrypt(data, amount); // BAD
 	RC2Encrypt(data, amount); // BAD

From e4d2c7cfc423ebd263bbc767c4ca829eb002b982 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 13 May 2021 10:04:57 +0100
Subject: [PATCH 08/25] C++: Rewrite so that we look for additional evidence.

---
 .../CWE/CWE-327/BrokenCryptoAlgorithm.ql      | 88 ++++++++++++-------
 .../semmle/code/cpp/security/Encryption.qll   | 11 +++
 .../CWE-327/BrokenCryptoAlgorithm.expected    | 20 ++---
 .../query-tests/Security/CWE/CWE-327/test.cpp |  4 +-
 4 files changed, 76 insertions(+), 47 deletions(-)

diff --git a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
index f1195a437369..1b72331a6cc1 100644
--- a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
+++ b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
@@ -13,52 +13,72 @@
 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())
+  ) and
+  exists(result.getACallToThisFunction())
 }
 
-Function getAnInsecureFunction() {
-  isInsecureEncryption(result.getName()) 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() {
-    // the function name suggests it relates to an insecure crypto algorithm.
-    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() {
-  isInsecureEncryption(result.getName()) 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() {
-    // the macro name suggests it relates to an insecure crypto algorithm.
-    this.getMacro() = getAnInsecureMacro() and
-    // the macro invocation generates something.
-    exists(this.getAGeneratedElement().(ControlFlowNode)) and
-    // exclude expressions controlling ifs/switches (as they may not be used).
-    not any(IfStmt c).getCondition().getAChild*() = this.getAGeneratedElement() and
-    not any(SwitchCase c).getExpr().getAChild*() = this.getAGeneratedElement() and
-    // exclude expressions in array initializers (as they may not be used).
-    not any(AggregateLiteral i).getAChild*() = this.getAGeneratedElement() 
+/**
+ * A function call we have a high confidence is related to use of an insecure
+ * encryption algorithm.
+ */
+class InsecureFunctionCall extends FunctionCall {
+  InsecureFunctionCall() {
+    // find use of an insecure algorithm name
+    (
+      getTarget() = getAnInsecureEncryptionFunction()
+      or
+      exists(MacroInvocation mi |
+        mi.getAGeneratedElement() = this.getAChild*() and
+        mi.getMacro() = getAnInsecureEncryptionMacro()
+      )
+    ) and
+    // find additional evidence that this function is related to encryption.
+    (
+      getTarget() = getAdditionalEvidenceFunction()
+      or
+      exists(MacroInvocation mi |
+        mi.getAGeneratedElement() = this.getAChild*() and
+        mi.getMacro() = getAdditionalEvidenceMacro()
+      )
+    )
   }
 
-  override string description() { result = "macro invocation" }
-
-  override string toString() { result = MacroInvocation.super.toString() }
-
-  override Location getLocation() { result = MacroInvocation.super.getLocation() }
+  string description() { result = "function call" }
 }
 
-from InsecureCryptoSpec c
+from InsecureFunctionCall c
 select c, "This " + c.description() + " specifies a broken or weak cryptographic algorithm."
diff --git a/cpp/ql/src/semmle/code/cpp/security/Encryption.qll b/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
index d2261df6bc8e..692b6ebeae99 100644
--- a/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
+++ b/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
@@ -60,6 +60,17 @@ predicate isInsecureEncryption(string name) {
   not name.toUpperCase().regexpMatch(".*TRIPLE.*")
 }
 
+ /**
+ * 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().regexpMatch(".*(CRYPT|CODE|CODING|CBC|KEY).*")
+}
+
 /**
  * Gets a regular expression for matching strings that look like they
  * contain an algorithm that is known to be secure.
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
index 53e9054a5070..cf31479a533d 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
@@ -1,16 +1,14 @@
 | 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:62:2:62:12 | call to encrypt_bad | This function call 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:172:28:172:35 | ALGO_DES | This macro invocation 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. |
-| 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: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:56:2:56:9 | DES(str) | This macro invocation specifies a broken or weak cryptographic algorithm. |
-| test.cpp:59:12:59:25 | SORT_ORDER_DES | This macro invocation specifies a broken or weak cryptographic algorithm. |
+| test2.cpp:172:2:172:26 | call to set_encryption_algorithm1 | This function call specifies a broken or weak cryptographic algorithm. |
+| test2.cpp:182:2:182:17 | call to encryption_with1 | This function call specifies a broken or weak cryptographic algorithm. |
+| test.cpp:38:2:38:31 | call to my_implementation1 | This function call specifies a broken or weak cryptographic algorithm. |
+| test.cpp:39:2:39:31 | call to my_implementation2 | This function call specifies a broken or weak cryptographic algorithm. |
+| test.cpp:51:2:51:32 | call to my_implementation1 | This function call specifies a broken or weak cryptographic algorithm. |
+| test.cpp:52:2:52:31 | call to my_implementation1 | This function call specifies a broken or weak cryptographic algorithm. |
+| test.cpp:53:2:53:25 | call to my_implementation1 | This function call specifies a broken or weak cryptographic algorithm. |
+| test.cpp:54:2:54:26 | call to my_implementation1 | This function call 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:101:2:101:15 | call to do_des_encrypt | This function call specifies a broken or weak cryptographic algorithm. |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
index 8d16e5ed98e9..ede8f19b7611 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
@@ -53,10 +53,10 @@ void test_macros(void *data, size_t amount, const char *str)
 	DES_ENCODE(data, amount); // BAD
 	DES_SET_KEY(data, amount); // BAD
 
-	DES(str); // GOOD (probably nothing to do with encryption) [FALSE POSITIVE]
+	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) [FALSE POSITIVE]
+	int ord = SORT_ORDER_DES; // GOOD (probably nothing to do with encryption)
 }
 
 // --- simple encryption function calls ---

From 5d1ef49f8feb2fd17ee6adb4dd3ea81c9e9d81e0 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 13 May 2021 15:42:42 +0100
Subject: [PATCH 09/25] C++: Add support for enum constants.

---
 .../CWE/CWE-327/BrokenCryptoAlgorithm.ql      | 24 +++++++++++++++++++
 .../CWE-327/BrokenCryptoAlgorithm.expected    |  3 +++
 .../Security/CWE/CWE-327/test2.cpp            |  6 ++---
 3 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
index 1b72331a6cc1..b7a34ff8f970 100644
--- a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
+++ b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
@@ -51,6 +51,20 @@ Macro getAdditionalEvidenceMacro() {
   exists(result.getAnInvocation())
 }
 
+/**
+ * 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.
@@ -65,6 +79,11 @@ class InsecureFunctionCall extends FunctionCall {
         mi.getAGeneratedElement() = this.getAChild*() and
         mi.getMacro() = getAnInsecureEncryptionMacro()
       )
+      or
+      exists(EnumConstantAccess ec |
+        ec = this.getAChild*() and
+        ec.getTarget() = getAnInsecureEncryptionEnumConst()
+      )
     ) and
     // find additional evidence that this function is related to encryption.
     (
@@ -74,6 +93,11 @@ class InsecureFunctionCall extends FunctionCall {
         mi.getAGeneratedElement() = this.getAChild*() and
         mi.getMacro() = getAdditionalEvidenceMacro()
       )
+      or
+      exists(EnumConstantAccess ec |
+        ec = this.getAChild*() and
+        ec.getTarget() = getAdditionalEvidenceEnumConst()
+      )
     )
   }
 
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
index cf31479a533d..1b85eae2d4cb 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
@@ -1,8 +1,11 @@
 | test2.cpp:49:4:49:24 | call to my_des_implementation | This function call specifies a broken or weak cryptographic algorithm. |
 | test2.cpp:62:2:62:12 | call to encrypt_bad | This function call 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:22:144:30 | call to MyBadEncryptor | This function call specifies a broken or weak cryptographic algorithm. |
 | test2.cpp:172:2:172:26 | call to set_encryption_algorithm1 | This function call specifies a broken or weak cryptographic algorithm. |
+| test2.cpp:175:2:175:26 | call to set_encryption_algorithm2 | This function call specifies a broken or weak cryptographic algorithm. |
 | test2.cpp:182:2:182:17 | call to encryption_with1 | This function call specifies a broken or weak cryptographic algorithm. |
+| test2.cpp:185:2:185:17 | call to encryption_with2 | This function call specifies a broken or weak cryptographic algorithm. |
 | test.cpp:38:2:38:31 | call to my_implementation1 | This function call specifies a broken or weak cryptographic algorithm. |
 | test.cpp:39:2:39:31 | call to my_implementation2 | This function call specifies a broken or weak cryptographic algorithm. |
 | test.cpp:51:2:51:32 | call to my_implementation1 | This function call specifies a broken or weak cryptographic algorithm. |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp
index 622db3aff0ed..def7e0ae8085 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp
@@ -141,7 +141,7 @@ void do_class_encrypts(char *data, size_t amount, keytype key)
 	}
 
 	{
-		MyBadEncryptor mbe(key, DES); // BAD [NOT DETECTED]
+		MyBadEncryptor mbe(key, DES); // BAD
 
 		mbe.encrypt(data, amount);
 	}
@@ -172,7 +172,7 @@ void do_unseen_encrypts(char *data, size_t amount, keytype key)
 	set_encryption_algorithm1(ALGO_DES); // BAD
 	set_encryption_algorithm1(ALGO_AES); // GOOD
 
-	set_encryption_algorithm2(USE_DES); // BAD [NOT DETECTED]
+	set_encryption_algorithm2(USE_DES); // BAD
 	set_encryption_algorithm2(USE_AES); // GOOD
 
 	set_encryption_algorithm3("DES"); // BAD [NOT DETECTED]
@@ -182,7 +182,7 @@ void do_unseen_encrypts(char *data, size_t amount, keytype key)
 	encryption_with1(data, amount, key, ALGO_DES); // BAD
 	encryption_with1(data, amount, key, ALGO_AES); // GOOD
 
-	encryption_with2(data, amount, key, USE_DES); // BAD [NOT DETECTED]
+	encryption_with2(data, amount, key, USE_DES); // BAD
 	encryption_with2(data, amount, key, USE_AES); // GOOD
 
 	encryption_with3(data, amount, key, "DES"); // BAD [NOT DETECTED]

From 2576075b989c05b91b37ef8798052fd944dffb19 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 13 May 2021 15:52:28 +0100
Subject: [PATCH 10/25] C++: Repair result message.

---
 .../CWE/CWE-327/BrokenCryptoAlgorithm.ql      | 25 +++++++++++++++----
 .../CWE-327/BrokenCryptoAlgorithm.expected    | 24 +++++++++---------
 2 files changed, 32 insertions(+), 17 deletions(-)

diff --git a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
index b7a34ff8f970..e96a803d7c07 100644
--- a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
+++ b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
@@ -70,19 +70,28 @@ EnumConstant getAdditionalEvidenceEnumConst() {
  * encryption algorithm.
  */
 class InsecureFunctionCall extends FunctionCall {
+  Element blame;
+  string explain;
+
   InsecureFunctionCall() {
     // find use of an insecure algorithm name
     (
-      getTarget() = getAnInsecureEncryptionFunction()
+      getTarget() = getAnInsecureEncryptionFunction() and
+      blame = this and
+      explain = "function call"
       or
       exists(MacroInvocation mi |
         mi.getAGeneratedElement() = this.getAChild*() and
-        mi.getMacro() = getAnInsecureEncryptionMacro()
+        mi.getMacro() = getAnInsecureEncryptionMacro() and
+        blame = mi and
+        explain = "macro invocation"
       )
       or
       exists(EnumConstantAccess ec |
         ec = this.getAChild*() and
-        ec.getTarget() = getAnInsecureEncryptionEnumConst()
+        ec.getTarget() = getAnInsecureEncryptionEnumConst() and
+        blame = ec and
+        explain = "enum constant access"
       )
     ) and
     // find additional evidence that this function is related to encryption.
@@ -101,8 +110,14 @@ class InsecureFunctionCall extends FunctionCall {
     )
   }
 
-  string description() { result = "function call" }
+  Element getBlame() {
+    result = blame
+  }
+
+  string getDescription() {
+     result = explain
+  }
 }
 
 from InsecureFunctionCall c
-select c, "This " + c.description() + " specifies a broken or weak cryptographic algorithm."
+select c.getBlame(), "This " + c.getDescription() + " specifies a broken or weak cryptographic algorithm."
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
index 1b85eae2d4cb..197472977b45 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
@@ -1,17 +1,17 @@
 | test2.cpp:49:4:49:24 | call to my_des_implementation | This function call specifies a broken or weak cryptographic algorithm. |
-| test2.cpp:62:2:62:12 | call to encrypt_bad | 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:22:144:30 | call to MyBadEncryptor | This function call specifies a broken or weak cryptographic algorithm. |
-| test2.cpp:172:2:172:26 | call to set_encryption_algorithm1 | This function call specifies a broken or weak cryptographic algorithm. |
-| test2.cpp:175:2:175:26 | call to set_encryption_algorithm2 | This function call specifies a broken or weak cryptographic algorithm. |
-| test2.cpp:182:2:182:17 | call to encryption_with1 | This function call specifies a broken or weak cryptographic algorithm. |
-| test2.cpp:185:2:185:17 | call to encryption_with2 | This function call specifies a broken or weak cryptographic algorithm. |
-| test.cpp:38:2:38:31 | call to my_implementation1 | This function call specifies a broken or weak cryptographic algorithm. |
-| test.cpp:39:2:39:31 | call to my_implementation2 | This function call specifies a broken or weak cryptographic algorithm. |
-| test.cpp:51:2:51:32 | call to my_implementation1 | This function call specifies a broken or weak cryptographic algorithm. |
-| test.cpp:52:2:52:31 | call to my_implementation1 | This function call specifies a broken or weak cryptographic algorithm. |
-| test.cpp:53:2:53:25 | call to my_implementation1 | This function call specifies a broken or weak cryptographic algorithm. |
-| test.cpp:54:2:54:26 | call to my_implementation1 | 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. |
+| 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: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:101:2:101:15 | call to do_des_encrypt | This function call specifies a broken or weak cryptographic algorithm. |

From 3a83ff54e6c95dcbe878d61eee2dca06199cb10b Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 13 May 2021 16:02:00 +0100
Subject: [PATCH 11/25] C++: Add support for class methods.

---
 cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql      | 3 ++-
 .../Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected       | 2 ++
 cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp        | 4 ++--
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
index e96a803d7c07..71c835491f4e 100644
--- a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
+++ b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
@@ -19,7 +19,8 @@ import semmle.code.cpp.security.Encryption
 Function getAnInsecureEncryptionFunction() {
   (
     isInsecureEncryption(result.getName()) or
-    isInsecureEncryption(result.getAParameter().getName())
+    isInsecureEncryption(result.getAParameter().getName()) or
+    isInsecureEncryption(result.getDeclaringType().getName())
   ) and
   exists(result.getACallToThisFunction())
 }
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
index 197472977b45..e213a6e92c3a 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
@@ -6,6 +6,8 @@
 | 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:234:2:234:20 | call to encrypt | This function call specifies a broken or weak cryptographic algorithm. |
+| test2.cpp:239:5:239: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:51:2:51:32 | DES_DO_ENCRYPTION(data,amount) | This macro invocation specifies a broken or weak cryptographic algorithm. |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp
index def7e0ae8085..1cf186592420 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp
@@ -231,12 +231,12 @@ class aesCipher
 
 void do_classes(const char *data)
 {
-	desEncrypt::encrypt(data); // BAD [NOT DETECTED]
+	desEncrypt::encrypt(data); // BAD
 	aes256Encrypt::encrypt(data); // GOOD
 
 	desCipher dc;
 	aesCipher ac;
-	dc.encrypt(data); // BAD [NOT DETECTED]
+	dc.encrypt(data); // BAD
 	ac.encrypt(data); // GOOD
 }
 

From a9d57450c89bb93b0882e9c723ebabc5684c0c52 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 13 May 2021 16:19:09 +0100
Subject: [PATCH 12/25] C++: Autoformat.

---
 .../CWE/CWE-327/BrokenCryptoAlgorithm.ql      | 19 ++++++-------------
 .../semmle/code/cpp/security/Encryption.qll   |  2 +-
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
index 71c835491f4e..2f1b7fed6f00 100644
--- a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
+++ b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
@@ -55,16 +55,12 @@ Macro getAdditionalEvidenceMacro() {
 /**
  * An enum constant which may relate to an insecure encryption algorithm.
  */
-EnumConstant getAnInsecureEncryptionEnumConst() {
-  isInsecureEncryption(result.getName())
-}
+EnumConstant getAnInsecureEncryptionEnumConst() { isInsecureEncryption(result.getName()) }
 
 /**
  * An enum constant with additional evidence it is related to encryption.
  */
-EnumConstant getAdditionalEvidenceEnumConst() {
-  isEncryptionAdditionalEvidence(result.getName())
-}
+EnumConstant getAdditionalEvidenceEnumConst() { isEncryptionAdditionalEvidence(result.getName()) }
 
 /**
  * A function call we have a high confidence is related to use of an insecure
@@ -111,14 +107,11 @@ class InsecureFunctionCall extends FunctionCall {
     )
   }
 
-  Element getBlame() {
-    result = blame
-  }
+  Element getBlame() { result = blame }
 
-  string getDescription() {
-     result = explain
-  }
+  string getDescription() { result = explain }
 }
 
 from InsecureFunctionCall c
-select c.getBlame(), "This " + c.getDescription() + " specifies a broken or weak cryptographic algorithm."
+select c.getBlame(),
+  "This " + c.getDescription() + " specifies a broken or weak cryptographic algorithm."
diff --git a/cpp/ql/src/semmle/code/cpp/security/Encryption.qll b/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
index 692b6ebeae99..8c9c4b3beee7 100644
--- a/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
+++ b/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
@@ -60,7 +60,7 @@ predicate isInsecureEncryption(string name) {
   not name.toUpperCase().regexpMatch(".*TRIPLE.*")
 }
 
- /**
+/**
  * 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

From 9cdf8389815e4bb97e3ed3dd18ae3820fadb17c9 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 13 May 2021 16:20:52 +0100
Subject: [PATCH 13/25] C++: Bug fix.

---
 cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
index 2f1b7fed6f00..0d311333fca8 100644
--- a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
+++ b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
@@ -31,7 +31,8 @@ Function getAnInsecureEncryptionFunction() {
 Function getAdditionalEvidenceFunction() {
   (
     isEncryptionAdditionalEvidence(result.getName()) or
-    isEncryptionAdditionalEvidence(result.getAParameter().getName())
+    isEncryptionAdditionalEvidence(result.getAParameter().getName()) or
+    isInsecureEncryption(result.getDeclaringType().getName())
   ) and
   exists(result.getACallToThisFunction())
 }

From 9e75f53798d9093332d40deaeda645dd514ec05d Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Mon, 17 May 2021 15:35:19 +0100
Subject: [PATCH 14/25] C++: Prefer matches to regexpMatch.

---
 cpp/ql/src/semmle/code/cpp/security/Encryption.qll | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cpp/ql/src/semmle/code/cpp/security/Encryption.qll b/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
index 8c9c4b3beee7..361eb5695d2a 100644
--- a/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
+++ b/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
@@ -68,7 +68,7 @@ predicate isInsecureEncryption(string name) {
  */
 bindingset[name]
 predicate isEncryptionAdditionalEvidence(string name) {
-  name.toUpperCase().regexpMatch(".*(CRYPT|CODE|CODING|CBC|KEY).*")
+  name.toUpperCase().matches("%" + ["CRYPT", "CODE", "CODING", "CBC", "KEY"] + "%")
 }
 
 /**

From 57354def9ebaf4b4577e7e901e24dfa8ff3f1767 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Mon, 17 May 2021 15:36:27 +0100
Subject: [PATCH 15/25] C++: Real world diffs suggest that 'Cipher' should be
 an encryption word as well.

---
 cpp/ql/src/semmle/code/cpp/security/Encryption.qll | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cpp/ql/src/semmle/code/cpp/security/Encryption.qll b/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
index 361eb5695d2a..49cdd2d7c63c 100644
--- a/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
+++ b/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
@@ -68,7 +68,7 @@ predicate isInsecureEncryption(string name) {
  */
 bindingset[name]
 predicate isEncryptionAdditionalEvidence(string name) {
-  name.toUpperCase().matches("%" + ["CRYPT", "CODE", "CODING", "CBC", "KEY"] + "%")
+  name.toUpperCase().matches("%" + ["CRYPT", "CODE", "CODING", "CBC", "KEY", "CIPHER"] + "%")
 }
 
 /**

From 930b9fe3e5e1ed1b9b59987b97cd99ff05a6b647 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Mon, 17 May 2021 15:46:41 +0100
Subject: [PATCH 16/25] C++: Add triple-DES to the bad algorithms list.

---
 cpp/ql/src/semmle/code/cpp/security/Encryption.qll   | 10 +++-------
 .../CWE/CWE-327/BrokenCryptoAlgorithm.expected       |  4 ++++
 .../test/query-tests/Security/CWE/CWE-327/test.cpp   | 12 ++++++------
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/cpp/ql/src/semmle/code/cpp/security/Encryption.qll b/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
index 49cdd2d7c63c..68a3d841d01f 100644
--- a/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
+++ b/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
@@ -10,7 +10,8 @@ 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".
     ]
 }
 
@@ -53,12 +54,7 @@ string getInsecureAlgorithmRegex() {
  * insecure encyption algorithm.
  */
 bindingset[name]
-predicate isInsecureEncryption(string name) {
-  name.regexpMatch(getInsecureAlgorithmRegex()) and
-  // Check for evidence that an otherwise matching name may in fact not be
-  // related to insecure encrpytion, e.g. "Triple-DES" is not "DES".
-  not name.toUpperCase().regexpMatch(".*TRIPLE.*")
-}
+predicate isInsecureEncryption(string name) { name.regexpMatch(getInsecureAlgorithmRegex()) }
 
 /**
  * Holds if there is additional evidence that `name` looks like it might be
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
index e213a6e92c3a..79943db5f602 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
@@ -10,11 +10,15 @@
 | test2.cpp:239:5:239: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. |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
index ede8f19b7611..39005f63bd98 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
@@ -38,15 +38,15 @@ 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); // GOOD (good enough algorithm)
-	ENCRYPT_WITH_TRIPLE_DES(data, amount); // GOOD (good enough 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); // GOOD (good enough algorithm)
+	DES3ENCRYPT(data, amount); // BAD [NOT DETECTED]
 
 	DES_DO_ENCRYPTION(data, amount); // BAD
 	RUN_DES_ENCODING(data, amount); // BAD
@@ -88,13 +88,13 @@ 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); // GOOD (good enough algorithm)
-	encryptTripleDES(data, amount); // GOOD (good enough algorithm)
+	encrypt3DES(data, amount); // BAD
+	encryptTripleDES(data, amount); // BAD
 
 	DESEncrypt(data, amount); // BAD
 	RC2Encrypt(data, amount); // BAD
 	AESEncrypt(data, amount); // GOOD (good algorithm)
-	DES3Encrypt(data, amount); // GOOD (good enough algorithm)
+	DES3Encrypt(data, amount); // BAD [NOT DETECTED]
 
 	DoDESEncryption(data, amount); // BAD [NOT DETECTED]
 	encryptDes(data, amount); // BAD [NOT DETECTED]

From 09d00b133e6d5d99472344b03787186d0cdbf011 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Mon, 17 May 2021 15:53:03 +0100
Subject: [PATCH 17/25] C++: Acknowledge another not detected result in tests.

---
 cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
index 39005f63bd98..76ab7ce34238 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
@@ -91,8 +91,8 @@ void test_functions(void *data, size_t amount, const char *str)
 	encrypt3DES(data, amount); // BAD
 	encryptTripleDES(data, amount); // BAD
 
-	DESEncrypt(data, amount); // BAD
-	RC2Encrypt(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]
 

From 3b29920255f01305ee9890a8ad8dcee6cc2b5350 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Mon, 17 May 2021 16:10:39 +0100
Subject: [PATCH 18/25] C++: Replace getAChild with getAnArgument().

---
 cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
index 0d311333fca8..04dda58cbf74 100644
--- a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
+++ b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
@@ -79,14 +79,14 @@ class InsecureFunctionCall extends FunctionCall {
       explain = "function call"
       or
       exists(MacroInvocation mi |
-        mi.getAGeneratedElement() = this.getAChild*() and
+        mi.getAGeneratedElement() = this.getAnArgument() and
         mi.getMacro() = getAnInsecureEncryptionMacro() and
         blame = mi and
         explain = "macro invocation"
       )
       or
       exists(EnumConstantAccess ec |
-        ec = this.getAChild*() and
+        ec = this.getAnArgument() and
         ec.getTarget() = getAnInsecureEncryptionEnumConst() and
         blame = ec and
         explain = "enum constant access"
@@ -97,12 +97,12 @@ class InsecureFunctionCall extends FunctionCall {
       getTarget() = getAdditionalEvidenceFunction()
       or
       exists(MacroInvocation mi |
-        mi.getAGeneratedElement() = this.getAChild*() and
+        mi.getAGeneratedElement() = this.getAnArgument() and
         mi.getMacro() = getAdditionalEvidenceMacro()
       )
       or
       exists(EnumConstantAccess ec |
-        ec = this.getAChild*() and
+        ec = this.getAnArgument() and
         ec.getTarget() = getAdditionalEvidenceEnumConst()
       )
     )

From da83e9142bfb43f2a886e38bf5f7020b13160163 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Tue, 18 May 2021 13:23:41 +0100
Subject: [PATCH 19/25] C++: Replace getAnExpandedElement with
 getAGeneratedElement as it's all we really need.

---
 cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
index 04dda58cbf74..dda70c2cb060 100644
--- a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
+++ b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
@@ -79,7 +79,7 @@ class InsecureFunctionCall extends FunctionCall {
       explain = "function call"
       or
       exists(MacroInvocation mi |
-        mi.getAGeneratedElement() = this.getAnArgument() and
+        mi.getAnExpandedElement() = this.getAnArgument() and
         mi.getMacro() = getAnInsecureEncryptionMacro() and
         blame = mi and
         explain = "macro invocation"
@@ -97,7 +97,7 @@ class InsecureFunctionCall extends FunctionCall {
       getTarget() = getAdditionalEvidenceFunction()
       or
       exists(MacroInvocation mi |
-        mi.getAGeneratedElement() = this.getAnArgument() and
+        mi.getAnExpandedElement() = this.getAnArgument() and
         mi.getMacro() = getAdditionalEvidenceMacro()
       )
       or

From 3d8513c1e0fd547396b18efdfdba83198d3fb537 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Tue, 18 May 2021 13:24:51 +0100
Subject: [PATCH 20/25] C++: Add 'MAC' as additional evidence.

---
 cpp/ql/src/semmle/code/cpp/security/Encryption.qll | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cpp/ql/src/semmle/code/cpp/security/Encryption.qll b/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
index 68a3d841d01f..55ef606483c4 100644
--- a/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
+++ b/cpp/ql/src/semmle/code/cpp/security/Encryption.qll
@@ -64,7 +64,7 @@ predicate isInsecureEncryption(string name) { name.regexpMatch(getInsecureAlgori
  */
 bindingset[name]
 predicate isEncryptionAdditionalEvidence(string name) {
-  name.toUpperCase().matches("%" + ["CRYPT", "CODE", "CODING", "CBC", "KEY", "CIPHER"] + "%")
+  name.toUpperCase().matches("%" + ["CRYPT", "CODE", "CODING", "CBC", "KEY", "CIPHER", "MAC"] + "%")
 }
 
 /**

From 012840e602298c066b08a93e7fd5a2d1beba5e09 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Tue, 18 May 2021 13:57:24 +0100
Subject: [PATCH 21/25] C++: Add more test cases.

---
 .../CWE/CWE-327/BrokenCryptoAlgorithm.expected   |  6 ++++--
 .../query-tests/Security/CWE/CWE-327/test.cpp    | 16 ++++++++++++++++
 .../query-tests/Security/CWE/CWE-327/test2.cpp   |  8 ++++++++
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
index 79943db5f602..886044b9f1aa 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
@@ -6,8 +6,10 @@
 | 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:234:2:234:20 | call to encrypt | This function call specifies a broken or weak cryptographic algorithm. |
-| test2.cpp:239:5:239:11 | call to encrypt | This function call 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:240:2:240:28 | call to doSomethingElse | 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. |
+| test2.cpp:247:5:247:19 | call to doSomethingElse | 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. |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
index 76ab7ce34238..1c7c90f9c88b 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
@@ -107,3 +107,19 @@ void test_functions(void *data, size_t amount, const char *str)
 	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 [NOT DETECTED]
+	INIT_ENCRYPT_WITH_AES(); // GOOD (good algorithm)
+	
+	// ...
+}
\ No newline at end of file
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp
index 1cf186592420..31fc3676bb8d 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp
@@ -209,35 +209,43 @@ class desEncrypt
 {
 public:
 	static void encrypt(const char *data);
+	static void doSomethingElse();
 };
 
 class aes256Encrypt
 {
 public:
 	static void encrypt(const char *data);
+	static void doSomethingElse();
 };
 
 class desCipher
 {
 public:
 	void encrypt(const char *data);
+	void doSomethingElse();
 };
 
 class aesCipher
 {
 public:
 	void encrypt(const char *data);
+	void doSomethingElse();
 };
 
 void do_classes(const char *data)
 {
 	desEncrypt::encrypt(data); // BAD
 	aes256Encrypt::encrypt(data); // GOOD
+	desEncrypt::doSomethingElse(); // GOOD [FALSE POSITIVE]
+	aes256Encrypt::doSomethingElse(); // GOOD
 
 	desCipher dc;
 	aesCipher ac;
 	dc.encrypt(data); // BAD
 	ac.encrypt(data); // GOOD
+	dc.doSomethingElse(); // GOOD [FALSE POSITIVE]
+	ac.doSomethingElse(); // GOOD
 }
 
 // --- function pointer ---

From c7382ee06d55fcf2df0f1a665ddae88a4a94c3fa Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Tue, 18 May 2021 13:40:53 +0100
Subject: [PATCH 22/25] C++: Repair for function call macros.

---
 .../src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql  | 10 ++++++++--
 .../CWE/CWE-327/BrokenCryptoAlgorithm.expected         |  1 +
 cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp  |  2 +-
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
index dda70c2cb060..7abb71c5a6c8 100644
--- a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
+++ b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
@@ -79,7 +79,10 @@ class InsecureFunctionCall extends FunctionCall {
       explain = "function call"
       or
       exists(MacroInvocation mi |
-        mi.getAnExpandedElement() = this.getAnArgument() and
+        (
+          mi.getAnExpandedElement() = this or
+          mi.getAnExpandedElement() = this.getAnArgument()
+        ) and
         mi.getMacro() = getAnInsecureEncryptionMacro() and
         blame = mi and
         explain = "macro invocation"
@@ -97,7 +100,10 @@ class InsecureFunctionCall extends FunctionCall {
       getTarget() = getAdditionalEvidenceFunction()
       or
       exists(MacroInvocation mi |
-        mi.getAnExpandedElement() = this.getAnArgument() and
+        (
+          mi.getAnExpandedElement() = this or
+          mi.getAnExpandedElement() = this.getAnArgument()
+        ) and
         mi.getMacro() = getAdditionalEvidenceMacro()
       )
       or
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
index 886044b9f1aa..e1e177218fd7 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
@@ -24,3 +24,4 @@
 | 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. |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
index 1c7c90f9c88b..8af9868f1eef 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp
@@ -118,7 +118,7 @@ void my_implementation8();
 
 void test_macros2()
 {
-	INIT_ENCRYPT_WITH_DES(); // BAD [NOT DETECTED]
+	INIT_ENCRYPT_WITH_DES(); // BAD
 	INIT_ENCRYPT_WITH_AES(); // GOOD (good algorithm)
 	
 	// ...

From 88dc0861ac0f3f5dbe15df07fa6010650032ecce Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Tue, 18 May 2021 14:18:59 +0100
Subject: [PATCH 23/25] C++: Fix copy-paste error.

---
 cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
index 7abb71c5a6c8..9869aca2d702 100644
--- a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
+++ b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
@@ -32,7 +32,7 @@ Function getAdditionalEvidenceFunction() {
   (
     isEncryptionAdditionalEvidence(result.getName()) or
     isEncryptionAdditionalEvidence(result.getAParameter().getName()) or
-    isInsecureEncryption(result.getDeclaringType().getName())
+    isEncryptionAdditionalEvidence(result.getDeclaringType().getName())
   ) and
   exists(result.getACallToThisFunction())
 }

From cdf261b54b10538062a5cce22390487ce9544cb2 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Tue, 18 May 2021 14:30:48 +0100
Subject: [PATCH 24/25] C++: In fact it's just not good enough to get
 additional evidence from the declaring type.

---
 cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql      | 3 +--
 .../Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected       | 2 --
 cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp        | 4 ++--
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
index 9869aca2d702..7162b000dfc2 100644
--- a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
+++ b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
@@ -31,8 +31,7 @@ Function getAnInsecureEncryptionFunction() {
 Function getAdditionalEvidenceFunction() {
   (
     isEncryptionAdditionalEvidence(result.getName()) or
-    isEncryptionAdditionalEvidence(result.getAParameter().getName()) or
-    isEncryptionAdditionalEvidence(result.getDeclaringType().getName())
+    isEncryptionAdditionalEvidence(result.getAParameter().getName())
   ) and
   exists(result.getACallToThisFunction())
 }
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
index e1e177218fd7..19cdfa5e9c73 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected
@@ -7,9 +7,7 @@
 | 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:240:2:240:28 | call to doSomethingElse | 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. |
-| test2.cpp:247:5:247:19 | call to doSomethingElse | 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. |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp
index 31fc3676bb8d..66d6f283ba30 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp
@@ -237,14 +237,14 @@ void do_classes(const char *data)
 {
 	desEncrypt::encrypt(data); // BAD
 	aes256Encrypt::encrypt(data); // GOOD
-	desEncrypt::doSomethingElse(); // GOOD [FALSE POSITIVE]
+	desEncrypt::doSomethingElse(); // GOOD
 	aes256Encrypt::doSomethingElse(); // GOOD
 
 	desCipher dc;
 	aesCipher ac;
 	dc.encrypt(data); // BAD
 	ac.encrypt(data); // GOOD
-	dc.doSomethingElse(); // GOOD [FALSE POSITIVE]
+	dc.doSomethingElse(); // GOOD
 	ac.doSomethingElse(); // GOOD
 }
 

From e985204a6247445891bef71ea2a5b73929939808 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Wed, 19 May 2021 11:14:23 +0100
Subject: [PATCH 25/25] C++: Add change note.

---
 cpp/change-notes/2021-05-19-weak-cryptographic-algorithm.md | 2 ++
 1 file changed, 2 insertions(+)
 create mode 100644 cpp/change-notes/2021-05-19-weak-cryptographic-algorithm.md

diff --git a/cpp/change-notes/2021-05-19-weak-cryptographic-algorithm.md b/cpp/change-notes/2021-05-19-weak-cryptographic-algorithm.md
new file mode 100644
index 000000000000..5e48ba166b7c
--- /dev/null
+++ b/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.