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..8a4bb065eb5c --- /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 and its `@precision` increased to `high`. diff --git a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql index aef114bcc4e4..e6c7b186ce2c 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 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() ) } 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..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 @@ -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 | 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 66d6f283ba30..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 @@ -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 container +{ +public: + container() { + obj = new C(); // GOOD + } + + ~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] +} + +// --- 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 +}