Merge pull request #18161 from owen-mc/java/weak-crypto-algo-more-informative

Java: Make `java/weak-cryptographic-algorithm` give a reason why the algo is insecure
2025-12-17 01:03:14 +01:00 · 2025-01-13 23:43:04 +00:00
parent 599411b440 0728b3bd60
commit 883301938b
9 changed files with 67 additions and 41 deletions
--- 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 =
+    "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 with PKCS#5 or PKCS#7 padding is vulnerable to padding oracle attacks. Consider using GCM instead."
 }

 /**
@@ -222,22 +235,18 @@ string getAnInsecureHashAlgorithmName() {
  result = "MD5"
 }

-private string rankedInsecureAlgorithm(int i) {
-  result = rank[i](string s | s = getAnInsecureAlgorithmName())
-}
-
-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. */
+bindingset[input]
+string getInsecureAlgorithmReason(string input) {
+  exists(string name | insecureAlgorithm(name, result) | input.regexpMatch(algorithmRegex(name)))
 }

 /**
--- 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()
--- 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
@@ -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.
--- 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");

--- 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" |
--- 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" | 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. |
--- 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 = "<no reason>"
+select s, reason
--- 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" |
--- 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" |