From e6409e159fcf3cfaa187e9105636fdef48f2bb8b Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Fri, 29 Nov 2024 11:18:36 +0000 Subject: [PATCH 1/8] Give reason why crypto algorithm is insecure --- .../semmle/code/java/security/Encryption.qll | 45 +++++++++++++------ .../CWE/CWE-327/BrokenCryptoAlgorithm.ql | 5 ++- .../test/library-tests/Encryption/Test.java | 4 ++ .../Encryption/cryptoalgospec.expected | 4 +- .../Encryption/insecure.expected | 18 +++++--- .../test/library-tests/Encryption/insecure.ql | 10 +++-- .../library-tests/Encryption/secure.expected | 4 +- .../tests/BrokenCryptoAlgorithm.expected | 4 +- 8 files changed, 63 insertions(+), 31 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/Encryption.qll b/java/ql/lib/semmle/code/java/security/Encryption.qll index 7c9c5ec60dbf..ec1acd9abe2b 100644 --- a/java/ql/lib/semmle/code/java/security/Encryption.qll +++ b/java/ql/lib/semmle/code/java/security/Encryption.qll @@ -198,19 +198,32 @@ private string algorithmRegex(string algorithmString) { } /** - * Gets the name of an algorithm that is known to be insecure. + * Holds if `name` is the name of an algorithm that is known to be insecure and + * `reason` explains why it is insecure. */ -string getAnInsecureAlgorithmName() { - result = - [ - "DES", "RC2", "RC4", "RC5", - // ARCFOUR is a variant of RC4 - "ARCFOUR", - // Encryption mode ECB like AES/ECB/NoPadding is vulnerable to replay and other attacks - "ECB", - // CBC mode of operation with PKCS#5 or PKCS#7 padding is vulnerable to padding oracle attacks - "AES/CBC/PKCS[57]Padding" - ] +predicate insecureAlgorithm(string name, string reason) { + name = "DES" and + reason = + "It has a short key length of 56 bits, making it vulnerable to brute-force attacks. Consider using AES instead." + or + name = "RC2" and + reason = "It is vulnerable to related-key attacks. Consider using AES instead." + or + // ARCFOUR is a variant of RC4 + name = ["RC4", "ARCFOUR"] and + reason = + "It has multiple vulnerabilities, including biases in its output and susceptibility to several attacks. Consider using AES instead." + or + name = "RC5" and + reason = "It is vulnerable to differential and related-key attacks. Consider using AES instead." + or + name = "ECB" and + reason = + "Encryption mode ECB like AES/ECB/NoPadding is vulnerable to replay and other attacks. Consider using AES instead." + or + name = "AES/CBC/PKCS[57]Padding" and + reason = + "CBC mode of operation with PKCS#5 or PKCS#7 padding is vulnerable to padding oracle attacks. Consider using AES instead." } /** @@ -223,7 +236,7 @@ string getAnInsecureHashAlgorithmName() { } private string rankedInsecureAlgorithm(int i) { - result = rank[i](string s | s = getAnInsecureAlgorithmName()) + result = rank[i](string name | insecureAlgorithm(name, _)) } private string insecureAlgorithmString(int i) { @@ -240,6 +253,12 @@ string getInsecureAlgorithmRegex() { result = algorithmRegex(insecureAlgorithmString(max(int i | exists(rankedInsecureAlgorithm(i))))) } +/** Gets the reason why `input` is an insecure algorithm, if any. */ +bindingset[input] +string getInsecureAlgorithmReason(string input) { + exists(string name | insecureAlgorithm(name, result) | input.regexpMatch(algorithmRegex(name))) +} + /** * Gets the name of an algorithm that is known to be secure. */ diff --git a/java/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql b/java/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql index a848419aaa3c..a2afa3cb568f 100644 --- a/java/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql +++ b/java/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql @@ -18,10 +18,11 @@ import InsecureCryptoFlow::PathGraph from InsecureCryptoFlow::PathNode source, InsecureCryptoFlow::PathNode sink, CryptoAlgoSpec spec, - BrokenAlgoLiteral algo + BrokenAlgoLiteral algo, string reason where sink.getNode().asExpr() = spec.getAlgoSpec() and source.getNode().asExpr() = algo and + reason = getInsecureAlgorithmReason(algo.getValue()) and InsecureCryptoFlow::flowPath(source, sink) -select spec, source, sink, "Cryptographic algorithm $@ is weak and should not be used.", algo, +select spec, source, sink, "Cryptographic algorithm $@ is insecure. " + reason, algo, algo.getValue() diff --git a/java/ql/test/library-tests/Encryption/Test.java b/java/ql/test/library-tests/Encryption/Test.java index 613476292ef9..91bc293c9640 100644 --- a/java/ql/test/library-tests/Encryption/Test.java +++ b/java/ql/test/library-tests/Encryption/Test.java @@ -11,6 +11,10 @@ class Test { "des_function", "function_using_des", "EncryptWithDES", + "RC2", + "RC4", + "ARCFOUR", + "RC5", "AES/ECB/NoPadding", "AES/CBC/PKCS5Padding"); diff --git a/java/ql/test/library-tests/Encryption/cryptoalgospec.expected b/java/ql/test/library-tests/Encryption/cryptoalgospec.expected index f066e6a0b0ba..ab6bb4081bbd 100644 --- a/java/ql/test/library-tests/Encryption/cryptoalgospec.expected +++ b/java/ql/test/library-tests/Encryption/cryptoalgospec.expected @@ -1,2 +1,2 @@ -| Test.java:37:4:37:17 | super(...) | Test.java:37:10:37:15 | "some" | -| Test.java:41:3:41:38 | getInstance(...) | Test.java:41:29:41:37 | "another" | +| Test.java:41:4:41:17 | super(...) | Test.java:41:10:41:15 | "some" | +| Test.java:45:3:45:38 | getInstance(...) | Test.java:45:29:45:37 | "another" | diff --git a/java/ql/test/library-tests/Encryption/insecure.expected b/java/ql/test/library-tests/Encryption/insecure.expected index 1bc1dc71e4d3..f561d6a48752 100644 --- a/java/ql/test/library-tests/Encryption/insecure.expected +++ b/java/ql/test/library-tests/Encryption/insecure.expected @@ -1,7 +1,11 @@ -| Test.java:9:4:9:8 | "DES" | -| Test.java:10:4:10:8 | "des" | -| Test.java:11:4:11:17 | "des_function" | -| Test.java:12:4:12:23 | "function_using_des" | -| Test.java:13:4:13:19 | "EncryptWithDES" | -| Test.java:14:4:14:22 | "AES/ECB/NoPadding" | -| Test.java:15:4:15:25 | "AES/CBC/PKCS5Padding" | +| Test.java:9:4:9:8 | "DES" | It has a short key length of 56 bits, making it vulnerable to brute-force attacks. Consider using AES instead. | +| Test.java:10:4:10:8 | "des" | It has a short key length of 56 bits, making it vulnerable to brute-force attacks. Consider using AES instead. | +| Test.java:11:4:11:17 | "des_function" | It has a short key length of 56 bits, making it vulnerable to brute-force attacks. Consider using AES instead. | +| Test.java:12:4:12:23 | "function_using_des" | It has a short key length of 56 bits, making it vulnerable to brute-force attacks. Consider using AES instead. | +| Test.java:13:4:13:19 | "EncryptWithDES" | It has a short key length of 56 bits, making it vulnerable to brute-force attacks. Consider using AES instead. | +| Test.java:14:4:14:8 | "RC2" | It is vulnerable to related-key attacks. Consider using AES instead. | +| Test.java:15:4:15:8 | "RC4" | It has multiple vulnerabilities, including biases in its output and susceptibility to several attacks. Consider using AES instead. | +| Test.java:16:4:16:12 | "ARCFOUR" | It has multiple vulnerabilities, including biases in its output and susceptibility to several attacks. Consider using AES instead. | +| Test.java:17:4:17:8 | "RC5" | It is vulnerable to differential and related-key attacks. Consider using AES instead. | +| Test.java:18:4:18:22 | "AES/ECB/NoPadding" | Encryption mode ECB like AES/ECB/NoPadding is vulnerable to replay and other attacks. Consider using AES instead. | +| Test.java:19:4:19:25 | "AES/CBC/PKCS5Padding" | CBC mode of operation with PKCS#5 or PKCS#7 padding is vulnerable to padding oracle attacks. Consider using AES instead. | diff --git a/java/ql/test/library-tests/Encryption/insecure.ql b/java/ql/test/library-tests/Encryption/insecure.ql index 4e8d9a84a883..21472f6dd9ef 100644 --- a/java/ql/test/library-tests/Encryption/insecure.ql +++ b/java/ql/test/library-tests/Encryption/insecure.ql @@ -1,6 +1,10 @@ import default import semmle.code.java.security.Encryption -from StringLiteral s -where s.getValue().regexpMatch(getInsecureAlgorithmRegex()) -select s +from StringLiteral s, string reason +where + s.getValue().regexpMatch(getInsecureAlgorithmRegex()) and + if exists(getInsecureAlgorithmReason(s.getValue())) + then reason = getInsecureAlgorithmReason(s.getValue()) + else reason = "" +select s, reason diff --git a/java/ql/test/library-tests/Encryption/secure.expected b/java/ql/test/library-tests/Encryption/secure.expected index a305f4ae7788..18291dd062a8 100644 --- a/java/ql/test/library-tests/Encryption/secure.expected +++ b/java/ql/test/library-tests/Encryption/secure.expected @@ -1,2 +1,2 @@ -| Test.java:18:4:18:8 | "AES" | -| Test.java:19:4:19:17 | "AES_function" | +| Test.java:22:4:22:8 | "AES" | +| Test.java:23:4:23:17 | "AES_function" | diff --git a/java/ql/test/query-tests/security/CWE-327/semmle/tests/BrokenCryptoAlgorithm.expected b/java/ql/test/query-tests/security/CWE-327/semmle/tests/BrokenCryptoAlgorithm.expected index 94719b477391..e54d006737cb 100644 --- a/java/ql/test/query-tests/security/CWE-327/semmle/tests/BrokenCryptoAlgorithm.expected +++ b/java/ql/test/query-tests/security/CWE-327/semmle/tests/BrokenCryptoAlgorithm.expected @@ -1,6 +1,6 @@ #select -| Test.java:19:20:19:50 | getInstance(...) | Test.java:19:45:19:49 | "DES" | Test.java:19:45:19:49 | "DES" | Cryptographic algorithm $@ is weak and should not be used. | Test.java:19:45:19:49 | "DES" | DES | -| Test.java:42:14:42:38 | getInstance(...) | Test.java:42:33:42:37 | "RC2" | Test.java:42:33:42:37 | "RC2" | Cryptographic algorithm $@ is weak and should not be used. | Test.java:42:33:42:37 | "RC2" | RC2 | +| Test.java:19:20:19:50 | getInstance(...) | Test.java:19:45:19:49 | "DES" | Test.java:19:45:19:49 | "DES" | Cryptographic algorithm $@ is insecure. It has a short key length of 56 bits, making it vulnerable to brute-force attacks. Consider using AES instead. | Test.java:19:45:19:49 | "DES" | DES | +| Test.java:42:14:42:38 | getInstance(...) | Test.java:42:33:42:37 | "RC2" | Test.java:42:33:42:37 | "RC2" | Cryptographic algorithm $@ is insecure. It is vulnerable to related-key attacks. Consider using AES instead. | Test.java:42:33:42:37 | "RC2" | RC2 | edges nodes | Test.java:19:45:19:49 | "DES" | semmle.label | "DES" | From 09240e46f2f663ce04dd9a560028c3d461cffb02 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Fri, 29 Nov 2024 11:50:57 +0000 Subject: [PATCH 2/8] Refactor: use `concat` instead of hand-written version This changes the order of the algorithms in the regex, but I don't think that makes any difference. --- java/ql/lib/semmle/code/java/security/Encryption.qll | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/Encryption.qll b/java/ql/lib/semmle/code/java/security/Encryption.qll index ec1acd9abe2b..5150262ed125 100644 --- a/java/ql/lib/semmle/code/java/security/Encryption.qll +++ b/java/ql/lib/semmle/code/java/security/Encryption.qll @@ -235,22 +235,12 @@ string getAnInsecureHashAlgorithmName() { result = "MD5" } -private string rankedInsecureAlgorithm(int i) { - result = rank[i](string name | insecureAlgorithm(name, _)) -} - -private string insecureAlgorithmString(int i) { - i = 1 and result = rankedInsecureAlgorithm(i) - or - result = rankedInsecureAlgorithm(i) + "|" + insecureAlgorithmString(i - 1) -} - /** * Gets the regular expression used for matching strings that look like they * contain an algorithm that is known to be insecure. */ string getInsecureAlgorithmRegex() { - result = algorithmRegex(insecureAlgorithmString(max(int i | exists(rankedInsecureAlgorithm(i))))) + result = algorithmRegex(concat(string name | insecureAlgorithm(name, _) | name, "|")) } /** Gets the reason why `input` is an insecure algorithm, if any. */ From 95d26d96d20214d6a30d0fe75caebcdc02d64eb2 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Fri, 29 Nov 2024 11:21:48 +0000 Subject: [PATCH 3/8] Add change note --- .../2024-11-29-java-weak-crypto-algorithm-explanation.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 java/ql/src/change-notes/2024-11-29-java-weak-crypto-algorithm-explanation.md diff --git a/java/ql/src/change-notes/2024-11-29-java-weak-crypto-algorithm-explanation.md b/java/ql/src/change-notes/2024-11-29-java-weak-crypto-algorithm-explanation.md new file mode 100644 index 000000000000..c26f0816d143 --- /dev/null +++ b/java/ql/src/change-notes/2024-11-29-java-weak-crypto-algorithm-explanation.md @@ -0,0 +1,4 @@ +--- +category: fix +--- +* The query "Use of a broken or risky cryptographic algorithm" (`java/weak-cryptographic-algorithm`) now gives the reason why the cryptographic algorithm is considered weak. From 5c99c8cc372a3e9a2d80d3d46df066cf663f44c7 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Fri, 29 Nov 2024 14:05:07 +0000 Subject: [PATCH 4/8] Improve suggestion for ECB --- java/ql/lib/semmle/code/java/security/Encryption.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/lib/semmle/code/java/security/Encryption.qll b/java/ql/lib/semmle/code/java/security/Encryption.qll index 5150262ed125..6d32e16b4794 100644 --- a/java/ql/lib/semmle/code/java/security/Encryption.qll +++ b/java/ql/lib/semmle/code/java/security/Encryption.qll @@ -219,7 +219,7 @@ predicate insecureAlgorithm(string name, string reason) { or name = "ECB" and reason = - "Encryption mode ECB like AES/ECB/NoPadding is vulnerable to replay and other attacks. Consider using AES instead." + "Encryption mode ECB like AES/ECB/NoPadding is vulnerable to replay and other attacks. Use a different encryption mode." or name = "AES/CBC/PKCS[57]Padding" and reason = From 95116eec51c9753fde58ee6037e1400201e2e022 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 4 Dec 2024 00:42:23 +0000 Subject: [PATCH 5/8] Update recommendations --- java/ql/lib/semmle/code/java/security/Encryption.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/Encryption.qll b/java/ql/lib/semmle/code/java/security/Encryption.qll index 6d32e16b4794..ba374f5d7af3 100644 --- a/java/ql/lib/semmle/code/java/security/Encryption.qll +++ b/java/ql/lib/semmle/code/java/security/Encryption.qll @@ -219,11 +219,11 @@ predicate insecureAlgorithm(string name, string reason) { or name = "ECB" and reason = - "Encryption mode ECB like AES/ECB/NoPadding is vulnerable to replay and other attacks. Use a different encryption mode." + "Encryption mode ECB like AES/ECB/NoPadding is vulnerable to replay and other attacks. Consider using a different encryption mode, like CBC or GCM, instead." or name = "AES/CBC/PKCS[57]Padding" and reason = - "CBC mode of operation with PKCS#5 or PKCS#7 padding is vulnerable to padding oracle attacks. Consider using AES instead." + "CBC mode of operation with PKCS#5 or PKCS#7 padding is vulnerable to padding oracle attacks. Consider using GCM encryption mode instead." } /** From 5351f5b69d60f847fa63eaf228045fb7390c7fa2 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com> Date: Wed, 4 Dec 2024 10:31:14 +0000 Subject: [PATCH 6/8] Update wording of alert (accepting review suggestion) Co-authored-by: Chris Smowton --- java/ql/lib/semmle/code/java/security/Encryption.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/lib/semmle/code/java/security/Encryption.qll b/java/ql/lib/semmle/code/java/security/Encryption.qll index ba374f5d7af3..995bf634493e 100644 --- a/java/ql/lib/semmle/code/java/security/Encryption.qll +++ b/java/ql/lib/semmle/code/java/security/Encryption.qll @@ -219,7 +219,7 @@ predicate insecureAlgorithm(string name, string reason) { or name = "ECB" and reason = - "Encryption mode ECB like AES/ECB/NoPadding is vulnerable to replay and other attacks. Consider using a different encryption mode, like CBC or GCM, instead." + "Encryption mode ECB, as in AES/ECB/NoPadding for example, is vulnerable to replay and other attacks. Consider using a different encryption mode, like CBC or GCM, instead." or name = "AES/CBC/PKCS[57]Padding" and reason = From 5959a736ac969829caaf7493dc77512585a6f4a8 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 7 Jan 2025 16:55:10 +0000 Subject: [PATCH 7/8] Only recommend GCM, and tighten wording --- java/ql/lib/semmle/code/java/security/Encryption.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/Encryption.qll b/java/ql/lib/semmle/code/java/security/Encryption.qll index 995bf634493e..ee8c1f5fbedc 100644 --- a/java/ql/lib/semmle/code/java/security/Encryption.qll +++ b/java/ql/lib/semmle/code/java/security/Encryption.qll @@ -219,11 +219,11 @@ predicate insecureAlgorithm(string name, string reason) { or name = "ECB" and reason = - "Encryption mode ECB, as in AES/ECB/NoPadding for example, is vulnerable to replay and other attacks. Consider using a different encryption mode, like CBC or GCM, instead." + "ECB mode, as in AES/ECB/NoPadding for example, is vulnerable to replay and other attacks. Consider using GCM instead." or name = "AES/CBC/PKCS[57]Padding" and reason = - "CBC mode of operation with PKCS#5 or PKCS#7 padding is vulnerable to padding oracle attacks. Consider using GCM encryption mode instead." + "CBC mode with PKCS#5 or PKCS#7 padding is vulnerable to padding oracle attacks. Consider using GCM instead." } /** From 0728b3bd6019bc573c9e8b2460e680d4afb3af1e Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Fri, 10 Jan 2025 10:37:05 +0000 Subject: [PATCH 8/8] Update test expectation --- java/ql/test/library-tests/Encryption/insecure.expected | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/test/library-tests/Encryption/insecure.expected b/java/ql/test/library-tests/Encryption/insecure.expected index f561d6a48752..eda453984376 100644 --- a/java/ql/test/library-tests/Encryption/insecure.expected +++ b/java/ql/test/library-tests/Encryption/insecure.expected @@ -7,5 +7,5 @@ | Test.java:15:4:15:8 | "RC4" | It has multiple vulnerabilities, including biases in its output and susceptibility to several attacks. Consider using AES instead. | | Test.java:16:4:16:12 | "ARCFOUR" | It has multiple vulnerabilities, including biases in its output and susceptibility to several attacks. Consider using AES instead. | | Test.java:17:4:17:8 | "RC5" | It is vulnerable to differential and related-key attacks. Consider using AES instead. | -| Test.java:18:4:18:22 | "AES/ECB/NoPadding" | Encryption mode ECB like AES/ECB/NoPadding is vulnerable to replay and other attacks. Consider using AES instead. | -| Test.java:19:4:19:25 | "AES/CBC/PKCS5Padding" | CBC mode of operation with PKCS#5 or PKCS#7 padding is vulnerable to padding oracle attacks. Consider using AES instead. | +| Test.java:18:4:18:22 | "AES/ECB/NoPadding" | ECB mode, as in AES/ECB/NoPadding for example, is vulnerable to replay and other attacks. Consider using GCM instead. | +| Test.java:19:4:19:25 | "AES/CBC/PKCS5Padding" | CBC mode with PKCS#5 or PKCS#7 padding is vulnerable to padding oracle attacks. Consider using GCM instead. |