diff --git a/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql b/cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql index f1195a43736..1b72331a6cc 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(); -} - -Function getAnInsecureFunction() { - isInsecureEncryption(result.getName()) and +/** + * A function which may relate to an insecure encryption algorithm. + */ +Function getAnInsecureEncryptionFunction() { + ( + isInsecureEncryption(result.getName()) or + isInsecureEncryption(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 function with additional evidence it is related to encryption. + */ +Function getAdditionalEvidenceFunction() { + ( + isEncryptionAdditionalEvidence(result.getName()) or + isEncryptionAdditionalEvidence(result.getAParameter().getName()) + ) and + exists(result.getACallToThisFunction()) } -Macro getAnInsecureMacro() { +/** + * A macro which may relate to an insecure encryption algorithm. + */ +Macro getAnInsecureEncryptionMacro() { isInsecureEncryption(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() - } - - override string description() { result = "macro invocation" } - - override string toString() { result = MacroInvocation.super.toString() } - - override Location getLocation() { result = MacroInvocation.super.getLocation() } +/** + * A macro with additional evidence it is related to encryption. + */ +Macro getAdditionalEvidenceMacro() { + isEncryptionAdditionalEvidence(result.getName()) and + exists(result.getAnInvocation()) } -from InsecureCryptoSpec c +/** + * 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() + ) + ) + } + + string description() { result = "function call" } +} + +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 d2261df6bc8..692b6ebeae9 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 53e9054a507..cf31479a533 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 8d16e5ed98e..ede8f19b761 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 ---