Merge pull request github#6099 from geoffw0/weak-crypto3

Further improvements to cpp/weak-cryptographic-algorithm

Author: MathiasVP

cpp/change-notes/2021-06-21-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 further improved to reduce false positives and its `@precision` increased to `high`.

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

@@ -5,7 +5,7 @@
  * @kind problem
  * @problem.severity error
  * @security-severity 7.5
- * @precision medium
+ * @precision high
  * @id cpp/weak-cryptographic-algorithm
  * @tags security
  *       external/cwe/cwe-327
@@ -70,9 +70,12 @@ EnumConstant getAdditionalEvidenceEnumConst() { isEncryptionAdditionalEvidence(r
 predicate getInsecureEncryptionEvidence(FunctionCall fc, Element blame, string description) {
   // find use of an insecure algorithm name
   (
-    fc.getTarget() = getAnInsecureEncryptionFunction() and
-    blame = fc and
-    description = "call to " + fc.getTarget().getName()
+    exists(FunctionCall fc2 |
+      fc.getAChild*() = fc2 and
+      fc2.getTarget() = getAnInsecureEncryptionFunction() and
+      blame = fc2 and
+      description = "call to " + fc.getTarget().getName()
+    )
     or
     exists(MacroInvocation mi |
       (
@@ -93,7 +96,10 @@ predicate getInsecureEncryptionEvidence(FunctionCall fc, Element blame, string d
   ) and
   // find additional evidence that this function is related to encryption.
   (
-    fc.getTarget() = getAnAdditionalEvidenceFunction()
+    exists(FunctionCall fc2 |
+      fc.getAChild*() = fc2 and
+      fc2.getTarget() = getAnAdditionalEvidenceFunction()
+    )
     or
     exists(MacroInvocation mi |
       (
@@ -107,6 +113,27 @@ predicate getInsecureEncryptionEvidence(FunctionCall fc, Element blame, string d
       ec = fc.getAnArgument() and
       ec.getTarget() = getAdditionalEvidenceEnumConst()
     )
+  ) and
+  // exclude calls from templates as this is rarely the right place to flag an
+  // issue
+  not fc.isFromTemplateInstantiation(_) and
+  (
+    // the function should have an input that looks like a non-constant buffer
+    exists(Expr e |
+      fc.getAnArgument() = e and
+      (
+        e.getUnspecifiedType() instanceof PointerType or
+        e.getUnspecifiedType() instanceof ReferenceType or
+        e.getUnspecifiedType() instanceof ArrayType
+      ) and
+      not e.getType().isDeeplyConstBelow() and
+      not e.isConstant()
+    )
+    or
+    // or be a non-const member function of an object
+    fc.getTarget() instanceof MemberFunction and
+    not fc.getTarget() instanceof ConstMemberFunction and
+    not fc.getTarget().isStatic()
   )
 }
 

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

@@ -1,13 +1,16 @@
 | test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:49:4:49:24 | call to my_des_implementation | call to my_des_implementation |
-| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:62:33:62:40 | ALGO_DES | invocation of macro ALGO_DES |
-| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:124:4:124:24 | call to my_des_implementation | call to my_des_implementation |
-| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:144:27:144:29 | DES | access of enum constant DES |
-| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:172:28:172:35 | ALGO_DES | invocation of macro ALGO_DES |
-| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:175:28:175:34 | USE_DES | access of enum constant USE_DES |
-| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:182:38:182:45 | ALGO_DES | invocation of macro ALGO_DES |
-| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:185:38:185:44 | USE_DES | access of enum constant USE_DES |
-| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:238:2:238:20 | call to encrypt | call to encrypt |
-| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:245:5:245:11 | call to encrypt | call to encrypt |
+| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:64:33:64:40 | ALGO_DES | invocation of macro ALGO_DES |
+| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:66:31:66:38 | ALGO_DES | invocation of macro ALGO_DES |
+| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:128:4:128:24 | call to my_des_implementation | call to my_des_implementation |
+| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:148:27:148:29 | DES | access of enum constant DES |
+| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:186:38:186:45 | ALGO_DES | invocation of macro ALGO_DES |
+| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:189:38:189:44 | USE_DES | access of enum constant USE_DES |
+| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:242:2:242:20 | call to encrypt | call to encrypt |
+| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:249:5:249:11 | call to encrypt | call to encrypt |
+| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:304:20:304:37 | call to desEncryptor | call to desEncryptor |
+| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:308:5:308:19 | call to doDesEncryption | call to doDesEncryption |
+| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:309:9:309:23 | call to doDesEncryption | call to doDesEncryption |
+| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:403:26:403:45 | call to getEncryptionNameDES | call to doEncryption |
 | test.cpp:38:2:38:31 | ENCRYPT_WITH_DES(data,amount) | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test.cpp:38:2:38:31 | ENCRYPT_WITH_DES(data,amount) | invocation of macro ENCRYPT_WITH_DES |
 | test.cpp:38:2:38:31 | ENCRYPT_WITH_DES(data,amount) | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test.cpp:39:2:39:31 | ENCRYPT_WITH_RC2(data,amount) | invocation of macro ENCRYPT_WITH_RC2 |
 | test.cpp:38:2:38:31 | ENCRYPT_WITH_DES(data,amount) | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test.cpp:41:2:41:32 | ENCRYPT_WITH_3DES(data,amount) | invocation of macro ENCRYPT_WITH_3DES |
@@ -21,5 +24,3 @@
 | test.cpp:38:2:38:31 | ENCRYPT_WITH_DES(data,amount) | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test.cpp:91:2:91:12 | call to encrypt3DES | call to encrypt3DES |
 | test.cpp:38:2:38:31 | ENCRYPT_WITH_DES(data,amount) | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test.cpp:92:2:92:17 | call to encryptTripleDES | call to encryptTripleDES |
 | test.cpp:38:2:38:31 | ENCRYPT_WITH_DES(data,amount) | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test.cpp:101:2:101:15 | call to do_des_encrypt | call to do_des_encrypt |
-| test.cpp:38:2:38:31 | ENCRYPT_WITH_DES(data,amount) | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test.cpp:102:2:102:12 | call to DES_Set_Key | call to DES_Set_Key |
-| test.cpp:38:2:38:31 | ENCRYPT_WITH_DES(data,amount) | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test.cpp:121:2:121:24 | INIT_ENCRYPT_WITH_DES() | invocation of macro INIT_ENCRYPT_WITH_DES |

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

@@ -99,7 +99,7 @@ void test_functions(void *data, size_t amount, const char *str)
 	DoDESEncryption(data, amount); // BAD [NOT DETECTED]
 	encryptDes(data, amount); // BAD [NOT DETECTED]
 	do_des_encrypt(data, amount); // BAD
-	DES_Set_Key(str); // BAD
+	DES_Set_Key(str); // BAD [NOT DETECTED]
 	DESSetKey(str); // BAD [NOT DETECTED]
 
 	Des(); // GOOD (probably nothing to do with encryption)
@@ -118,7 +118,7 @@ void my_implementation8();
 
 void test_macros2()
 {
-	INIT_ENCRYPT_WITH_DES(); // BAD
+	INIT_ENCRYPT_WITH_DES(); // BAD [NOT DETECTED]
 	INIT_ENCRYPT_WITH_AES(); // GOOD (good algorithm)
 
 	// ...

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

@@ -58,8 +58,12 @@ void encrypt_bad(char *data, size_t amount, keytype key, int algo)
 
 void do_encrypts(char *data, size_t amount, keytype key)
 {
+	char data2[128];
+
 	encrypt_good(data, amount, key, ALGO_AES); // GOOD
 	encrypt_bad(data, amount, key, ALGO_DES); // BAD
+	encrypt_good(data2, 128, key, ALGO_AES); // GOOD
+	encrypt_bad(data2, 128, key, ALGO_DES); // BAD
 }
 
 // --- more involved CPP-style example ---
@@ -169,10 +173,10 @@ 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_DES); // BAD [NOT DETECTED]
 	set_encryption_algorithm1(ALGO_AES); // GOOD
 
-	set_encryption_algorithm2(USE_DES); // BAD
+	set_encryption_algorithm2(USE_DES); // BAD [NOT DETECTED]
 	set_encryption_algorithm2(USE_AES); // GOOD
 
 	set_encryption_algorithm3("DES"); // BAD [NOT DETECTED]
@@ -208,32 +212,32 @@ void do_unseen_encrypts(char *data, size_t amount, keytype key)
 class desEncrypt
 {
 public:
-	static void encrypt(const char *data);
+	static void encrypt(char *data);
 	static void doSomethingElse();
 };
 
 class aes256Encrypt
 {
 public:
-	static void encrypt(const char *data);
+	static void encrypt(char *data);
 	static void doSomethingElse();
 };
 
 class desCipher
 {
 public:
-	void encrypt(const char *data);
+	void encrypt(char *data);
 	void doSomethingElse();
 };
 
 class aesCipher
 {
 public:
-	void encrypt(const char *data);
+	void encrypt(char *data);
 	void doSomethingElse();
 };
 
-void do_classes(const char *data)
+void do_classes(char *data)
 {
 	desEncrypt::encrypt(data); // BAD
 	aes256Encrypt::encrypt(data); // GOOD
@@ -260,3 +264,142 @@ void do_fn_ptr(char *data, size_t amount, keytype key)
 	impl = &my_aes_implementation; // GOOD
 	impl(data, amount, key);
 }
+
+// --- template classes ---
+
+class desEncryptor
+{
+public:
+	desEncryptor();
+
+	void doDesEncryption(char *data);
+};
+
+template <class C>
+class container
+{
+public:
+	container() {
+		obj = new C(); // GOOD
+	}
+
+	~container() {
+		delete obj;
+	}
+
+	C *obj;
+};
+
+template <class C>
+class templateDesEncryptor
+{
+public:
+	templateDesEncryptor();
+
+	void doDesEncryption(C &data);
+};
+
+void do_template_classes(char *data)
+{
+	desEncryptor *p = new desEncryptor(); // BAD
+	container<desEncryptor> c; // BAD [NOT DETECTED]
+	templateDesEncryptor<char *> t; // BAD [NOT DETECTED]
+
+	p->doDesEncryption(data); // BAD
+	c.obj->doDesEncryption(data); // BAD
+	t.doDesEncryption(data); // BAD [NOT DETECTED]
+}
+
+// --- assert ---
+
+int assertFunc(const char *file, int line);
+#define assert(_cond) ((_cond) || assertFunc(__FILE__, __LINE__))
+
+struct algorithmInfo;
+
+const algorithmInfo *getEncryptionAlgorithmInfo(int algo);
+
+void test_assert(int algo, algorithmInfo *algoInfo)
+{
+	assert(algo != ALGO_DES); // GOOD
+	assert(algoInfo != getEncryptionAlgorithmInfo(ALGO_DES)); // GOOD
+
+	// ...
+}
+
+// --- string comparisons ---
+
+int strcmp(const char *s1, const char *s2);
+void abort(void);
+
+#define ENCRYPTION_DES_NAME "DES"
+#define ENCRYPTION_AES_NAME "AES"
+
+void test_string_comparisons1(const char *algo_name)
+{
+	if (strcmp(algo_name, ENCRYPTION_DES_NAME) == 0) // GOOD
+	{
+		abort();
+	}
+	if (strcmp(algo_name, ENCRYPTION_AES_NAME) == 0) // GOOD
+	{
+		// ...
+	}
+}
+
+const char *getEncryptionNameDES()
+{
+	return "DES";
+}
+
+const char *getEncryptionNameAES()
+{
+	return "AES";
+}
+
+void test_string_comparisons2(const char *algo_name)
+{
+	if (strcmp(algo_name, getEncryptionNameDES()) == 0) // GOOD
+	{
+		abort();
+	}
+	if (strcmp(algo_name, getEncryptionNameAES()) == 0) // GOOD
+	{
+		// ...
+	}
+}
+
+const char *getEncryptionName(int algo)
+{
+	switch (algo)
+	{
+	case ALGO_DES:
+		return getEncryptionNameDES(); // GOOD
+	case ALGO_AES:
+		return getEncryptionNameAES(); // GOOD
+	default:
+		abort();
+	}
+}
+
+void test_string_comparisons3(const char *algo_name)
+{
+	if (strcmp(algo_name, getEncryptionName(ALGO_DES)) == 0) // GOOD
+	{
+		abort();
+	}
+	if (strcmp(algo_name, getEncryptionName(ALGO_AES)) == 0) // GOOD
+	{
+		// ...
+	}
+}
+
+// --- function call in a function call ---
+
+void doEncryption(char *data, size_t len, const char *algorithmName);
+
+void test_fn_in_fn(char *data, size_t len)
+{
+	doEncryption(data, len, getEncryptionNameDES()); // BAD
+	doEncryption(data, len, getEncryptionNameAES()); // GOOD
+}