From dca397dfb13c56a2a460eede37586bbd4b0aea1d Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Wed, 16 Jun 2021 14:17:42 +0100 Subject: [PATCH 01/14] C++: Add a test case with a template class. --- .../CWE-327/BrokenCryptoAlgorithm.expected | 4 ++ .../Security/CWE/CWE-327/test2.cpp | 45 +++++++++++++++++++ 2 files changed, 49 insertions(+) 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 d27d5c4bbca7..791575bc145d 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 @@ -8,6 +8,10 @@ | 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:279:9:279:15 | 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:300:20:300: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:304:5:304: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:305:9:305:23 | call to doDesEncryption | call to doDesEncryption | | 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 | 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 66d6f283ba30..3264465c87c4 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 @@ -260,3 +260,48 @@ 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 container +{ +public: + container() { + obj = new C(); // GOOD [FALSE POSITIVE] + } + + ~container() { + delete obj; + } + + C *obj; +}; + +template +class templateDesEncryptor +{ +public: + templateDesEncryptor(); + + void doDesEncryption(C &data); +}; + +void do_template_classes(char *data) +{ + desEncryptor *p = new desEncryptor(); // BAD + container c; // BAD [NOT DETECTED] + templateDesEncryptor t; // BAD [NOT DETECTED] + + p->doDesEncryption(data); // BAD + c.obj->doDesEncryption(data); // BAD + t.doDesEncryption(data); // BAD [NOT DETECTED] +} From 2e236dd2a905a722b2b7a50ae728e51b6833448a Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Wed, 16 Jun 2021 14:48:14 +0100 Subject: [PATCH 02/14] C++: Add a test case involving a harmless assert. --- .../CWE/CWE-327/BrokenCryptoAlgorithm.expected | 1 + .../query-tests/Security/CWE/CWE-327/test2.cpp | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+) 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 791575bc145d..6876187516b2 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 @@ -12,6 +12,7 @@ | 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:300:20:300: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:304:5:304: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:305:9:305: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:321:2:321:57 | ALGO_DES | invocation of macro ALGO_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: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 | 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 3264465c87c4..62da2c967d24 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 @@ -305,3 +305,21 @@ void do_template_classes(char *data) 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 [FALSE POSITIVE] + + // ... +} + From 7632c9edb5a741031ca6f358b916b23749b26a3d Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Wed, 16 Jun 2021 15:35:17 +0100 Subject: [PATCH 03/14] C++: Add test cases involving strings and comparisons. --- .../CWE-327/BrokenCryptoAlgorithm.expected | 4 ++ .../Security/CWE/CWE-327/test2.cpp | 66 +++++++++++++++++++ 2 files changed, 70 insertions(+) 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 6876187516b2..7ca665c22e5a 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 @@ -13,6 +13,10 @@ | 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:5:304: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:305:9:305: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:321:2:321:57 | 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:336:24:336:42 | ENCRYPTION_DES_NAME | invocation of macro ENCRYPTION_DES_NAME | +| 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:358:24:358:43 | call to getEncryptionNameDES | call to getEncryptionNameDES | +| 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:373:10:373:29 | call to getEncryptionNameDES | call to getEncryptionNameDES | +| 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:383:42:383:49 | ALGO_DES | invocation of macro ALGO_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: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 | 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 62da2c967d24..8c21a2db4f7f 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 @@ -323,3 +323,69 @@ void test_assert(int algo, algorithmInfo *algoInfo) // ... } +// --- 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 [FALSE POSITIVE] + { + 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 [FALSE POSITIVE] + { + abort(); + } + if (strcmp(algo_name, getEncryptionNameAES()) == 0) // GOOD + { + // ... + } +} + +const char *getEncryptionName(int algo) +{ + switch (algo) + { + case ALGO_DES: + return getEncryptionNameDES(); // GOOD [FALSE POSITIVE] + 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 [FALSE POSITIVE] + { + abort(); + } + if (strcmp(algo_name, getEncryptionName(ALGO_AES)) == 0) // GOOD + { + // ... + } +} From d590952aaa78f5c123a603e11b7f9949b481f480 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 17 Jun 2021 09:24:47 +0100 Subject: [PATCH 04/14] C++: Add a test case involving nested function calls. --- .../CWE/CWE-327/BrokenCryptoAlgorithm.expected | 1 + cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp | 10 ++++++++++ 2 files changed, 11 insertions(+) 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 7ca665c22e5a..e54c958c68ef 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 @@ -17,6 +17,7 @@ | 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:358:24:358:43 | call to getEncryptionNameDES | call to getEncryptionNameDES | | 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:373:10:373:29 | call to getEncryptionNameDES | call to getEncryptionNameDES | | 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:383:42:383:49 | 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:399:26:399:45 | call to getEncryptionNameDES | call to getEncryptionNameDES | | 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 | 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 8c21a2db4f7f..ac9a7d7e47b4 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 @@ -389,3 +389,13 @@ void test_string_comparisons3(const char *algo_name) // ... } } + +// --- 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 +} From 23db21cd90e8fc663fb3b7e5cc8897b2729b8ccb Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 17 Jun 2021 11:24:37 +0100 Subject: [PATCH 05/14] C++: Test spacing. --- .../CWE-327/BrokenCryptoAlgorithm.expected | 38 +++++++++---------- .../Security/CWE/CWE-327/test2.cpp | 4 ++ 2 files changed, 23 insertions(+), 19 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 e54c958c68ef..cd1977176987 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,23 +1,23 @@ | 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:279:9:279:15 | 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:300:20:300: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:304:5:304: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:305:9:305: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:321:2:321:57 | 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:336:24:336:42 | ENCRYPTION_DES_NAME | invocation of macro ENCRYPTION_DES_NAME | -| 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:358:24:358:43 | call to getEncryptionNameDES | call to getEncryptionNameDES | -| 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:373:10:373:29 | call to getEncryptionNameDES | call to getEncryptionNameDES | -| 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:383:42:383:49 | 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:399:26:399:45 | call to getEncryptionNameDES | call to getEncryptionNameDES | +| 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: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:176:28:176: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:179:28:179: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: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:283:9:283:15 | 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: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:325:2:325:57 | 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:340:24:340:42 | ENCRYPTION_DES_NAME | invocation of macro ENCRYPTION_DES_NAME | +| 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:362:24:362:43 | call to getEncryptionNameDES | call to getEncryptionNameDES | +| 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:377:10:377:29 | call to getEncryptionNameDES | call to getEncryptionNameDES | +| 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:387:42:387:49 | 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:403:26:403:45 | call to getEncryptionNameDES | call to getEncryptionNameDES | | 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 | 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 ac9a7d7e47b4..46aa901710aa 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 @@ -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) { + + encrypt_good(data, amount, key, ALGO_AES); // GOOD encrypt_bad(data, amount, key, ALGO_DES); // BAD + + } // --- more involved CPP-style example --- From 3641cdcc1fccac40bb4764b97878fb91348887b2 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 17 Jun 2021 11:25:39 +0100 Subject: [PATCH 06/14] C++: Add a test case involving an array. --- .../Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected | 1 + cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp | 6 +++--- 2 files changed, 4 insertions(+), 3 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 cd1977176987..3058add2819f 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,5 +1,6 @@ | 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: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:176:28:176:35 | ALGO_DES | invocation of macro ALGO_DES | 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 46aa901710aa..bdf94d594e5b 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 @@ -58,12 +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 --- From 8efdf359dc70228816e1e2cd11afbe0cf86d9ef6 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 17 Jun 2021 12:20:33 +0100 Subject: [PATCH 07/14] C++: Fix some incorrect uses of 'const' in the tests. --- cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) 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 bdf94d594e5b..283308ab7613 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 @@ -212,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 From a481e5c29229360cd581c4c6a4448d1dffb6b695 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Wed, 16 Jun 2021 15:59:38 +0100 Subject: [PATCH 08/14] C++: Exclude template code. --- cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql | 5 ++++- .../Security/CWE/CWE-327/BrokenCryptoAlgorithm.expected | 1 - cpp/ql/test/query-tests/Security/CWE/CWE-327/test2.cpp | 2 +- 3 files changed, 5 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 848adfd7adcc..92d8f272b4be 100644 --- a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql +++ b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql @@ -107,7 +107,10 @@ 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(_) } /** 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 3058add2819f..ef765fe469b7 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,7 +9,6 @@ | 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:283:9:283:15 | 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: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 | 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 283308ab7613..35e38d3b7302 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 @@ -280,7 +280,7 @@ class container { public: container() { - obj = new C(); // GOOD [FALSE POSITIVE] + obj = new C(); // GOOD } ~container() { From e5147c2a1f7c71c7db735722536e4545501241da Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Wed, 16 Jun 2021 17:35:00 +0100 Subject: [PATCH 09/14] C++: Exclude functions that don't involve buffers. --- .../CWE/CWE-327/BrokenCryptoAlgorithm.ql | 20 ++++++++++++++++++- .../CWE-327/BrokenCryptoAlgorithm.expected | 10 ---------- .../query-tests/Security/CWE/CWE-327/test.cpp | 4 ++-- .../Security/CWE/CWE-327/test2.cpp | 16 +++++++-------- 4 files changed, 29 insertions(+), 21 deletions(-) diff --git a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql index 92d8f272b4be..feb6bb28273e 100644 --- a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql +++ b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql @@ -110,7 +110,25 @@ predicate getInsecureEncryptionEvidence(FunctionCall fc, Element blame, string d ) and // exclude calls from templates as this is rarely the right place to flag an // issue - not fc.isFromTemplateInstantiation(_) + 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() + ) } /** 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 ef765fe469b7..c225ba56aa7e 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 @@ -3,8 +3,6 @@ | 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:176:28:176: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:179:28:179: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: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 | @@ -12,12 +10,6 @@ | 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:325:2:325:57 | 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:340:24:340:42 | ENCRYPTION_DES_NAME | invocation of macro ENCRYPTION_DES_NAME | -| 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:362:24:362:43 | call to getEncryptionNameDES | call to getEncryptionNameDES | -| 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:377:10:377:29 | call to getEncryptionNameDES | call to getEncryptionNameDES | -| 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:387:42:387:49 | 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:403:26:403:45 | call to getEncryptionNameDES | call to getEncryptionNameDES | | 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 | @@ -31,5 +23,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 | 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 8af9868f1eef..91af0f7eede2 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 @@ -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) // ... 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 35e38d3b7302..dbd2b7b1aa43 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 @@ -173,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] @@ -322,7 +322,7 @@ const algorithmInfo *getEncryptionAlgorithmInfo(int algo); void test_assert(int algo, algorithmInfo *algoInfo) { assert(algo != ALGO_DES); // GOOD - assert(algoInfo != getEncryptionAlgorithmInfo(ALGO_DES)); // GOOD [FALSE POSITIVE] + assert(algoInfo != getEncryptionAlgorithmInfo(ALGO_DES)); // GOOD // ... } @@ -337,7 +337,7 @@ void abort(void); void test_string_comparisons1(const char *algo_name) { - if (strcmp(algo_name, ENCRYPTION_DES_NAME) == 0) // GOOD [FALSE POSITIVE] + if (strcmp(algo_name, ENCRYPTION_DES_NAME) == 0) // GOOD { abort(); } @@ -359,7 +359,7 @@ const char *getEncryptionNameAES() void test_string_comparisons2(const char *algo_name) { - if (strcmp(algo_name, getEncryptionNameDES()) == 0) // GOOD [FALSE POSITIVE] + if (strcmp(algo_name, getEncryptionNameDES()) == 0) // GOOD { abort(); } @@ -374,7 +374,7 @@ const char *getEncryptionName(int algo) switch (algo) { case ALGO_DES: - return getEncryptionNameDES(); // GOOD [FALSE POSITIVE] + return getEncryptionNameDES(); // GOOD case ALGO_AES: return getEncryptionNameAES(); // GOOD default: @@ -384,7 +384,7 @@ const char *getEncryptionName(int algo) void test_string_comparisons3(const char *algo_name) { - if (strcmp(algo_name, getEncryptionName(ALGO_DES)) == 0) // GOOD [FALSE POSITIVE] + if (strcmp(algo_name, getEncryptionName(ALGO_DES)) == 0) // GOOD { abort(); } @@ -400,6 +400,6 @@ 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, getEncryptionNameDES()); // BAD [NOT DETECTED] doEncryption(data, len, getEncryptionNameAES()); // GOOD } From b5c71fd1d7966c8abf0cd660b69f259decb611a6 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 17 Jun 2021 09:33:28 +0100 Subject: [PATCH 10/14] C++: Repair funcion call in a function call. --- .../Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql | 14 ++++++++++---- .../CWE/CWE-327/BrokenCryptoAlgorithm.expected | 1 + .../query-tests/Security/CWE/CWE-327/test2.cpp | 2 +- 3 files changed, 12 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 feb6bb28273e..b69bb849e40b 100644 --- a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql +++ b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql @@ -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 | ( 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 c225ba56aa7e..4232a08ec0f8 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,6 +10,7 @@ | 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 | 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 dbd2b7b1aa43..95fc532c842d 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 @@ -400,6 +400,6 @@ 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 [NOT DETECTED] + doEncryption(data, len, getEncryptionNameDES()); // BAD doEncryption(data, len, getEncryptionNameAES()); // GOOD } From b4cbe6dce88f4e366f5e0522dd117837679cc7a2 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Wed, 16 Jun 2021 18:05:27 +0100 Subject: [PATCH 11/14] C++: Increase query precision to high. --- 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 b69bb849e40b..38dd23d1cfce 100644 --- a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql +++ b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql @@ -5,7 +5,7 @@ * @kind problem * @problem.severity error * @security-severity 5.2 - * @precision medium + * @precision high * @id cpp/weak-cryptographic-algorithm * @tags security * external/cwe/cwe-327 From 90e2a2d222272694b1d76c8dba0952da753015b5 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Mon, 21 Jun 2021 11:30:12 +0100 Subject: [PATCH 12/14] C++: Change note. --- cpp/change-notes/2021-06-21-weak-cryptographic-algorithm.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 cpp/change-notes/2021-06-21-weak-cryptographic-algorithm.md diff --git a/cpp/change-notes/2021-06-21-weak-cryptographic-algorithm.md b/cpp/change-notes/2021-06-21-weak-cryptographic-algorithm.md new file mode 100644 index 000000000000..d1700ba167f1 --- /dev/null +++ b/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. From 6f808c9e4ccf2d1f6cc82319f0cf501ad3e2195f Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Mon, 21 Jun 2021 12:32:48 +0100 Subject: [PATCH 13/14] C++: Update change note. --- cpp/change-notes/2021-06-21-weak-cryptographic-algorithm.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/change-notes/2021-06-21-weak-cryptographic-algorithm.md b/cpp/change-notes/2021-06-21-weak-cryptographic-algorithm.md index d1700ba167f1..fb73403fb0d6 100644 --- a/cpp/change-notes/2021-06-21-weak-cryptographic-algorithm.md +++ b/cpp/change-notes/2021-06-21-weak-cryptographic-algorithm.md @@ -1,2 +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. +* The "Use of a broken or risky cryptographic algorithm" (`cpp/weak-cryptographic-algorithm`) query has been further improved to reduce false positives and it's `@precision` increased to `high`. From 05ed4ed739eaa5391ff207f43dae21c46fc2c238 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Mon, 21 Jun 2021 14:22:56 +0100 Subject: [PATCH 14/14] Update cpp/change-notes/2021-06-21-weak-cryptographic-algorithm.md Co-authored-by: Mathias Vorreiter Pedersen --- cpp/change-notes/2021-06-21-weak-cryptographic-algorithm.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/change-notes/2021-06-21-weak-cryptographic-algorithm.md b/cpp/change-notes/2021-06-21-weak-cryptographic-algorithm.md index fb73403fb0d6..8a4bb065eb5c 100644 --- a/cpp/change-notes/2021-06-21-weak-cryptographic-algorithm.md +++ b/cpp/change-notes/2021-06-21-weak-cryptographic-algorithm.md @@ -1,2 +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 it's `@precision` increased to `high`. +* 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`.