diff --git a/java/ql/lib/semmle/code/java/security/Encryption.qll b/java/ql/lib/semmle/code/java/security/Encryption.qll index 6fc7f6b7d16a..1b28f5cbba61 100644 --- a/java/ql/lib/semmle/code/java/security/Encryption.qll +++ b/java/ql/lib/semmle/code/java/security/Encryption.qll @@ -223,10 +223,7 @@ string getAnInsecureHashAlgorithmName() { } private string rankedInsecureAlgorithm(int i) { - // In this case we know these are being used for encryption, so we want to match - // weak hash algorithms too. - result = - rank[i](string s | s = getAnInsecureAlgorithmName() or s = getAnInsecureHashAlgorithmName()) + result = rank[i](string s | s = getAnInsecureAlgorithmName()) } private string insecureAlgorithmString(int i) { diff --git a/java/ql/lib/semmle/code/java/security/MaybeBrokenCryptoAlgorithmQuery.qll b/java/ql/lib/semmle/code/java/security/MaybeBrokenCryptoAlgorithmQuery.qll index 1533b61dd5e4..060a30f87e6a 100644 --- a/java/ql/lib/semmle/code/java/security/MaybeBrokenCryptoAlgorithmQuery.qll +++ b/java/ql/lib/semmle/code/java/security/MaybeBrokenCryptoAlgorithmQuery.qll @@ -30,7 +30,11 @@ class InsecureAlgoLiteral extends InsecureAlgorithm, ShortStringLiteral { s.length() > 1 and not s.regexpMatch(getSecureAlgorithmRegex()) and // Exclude results covered by another query. - not s.regexpMatch(getInsecureAlgorithmRegex()) + not s.regexpMatch(getInsecureAlgorithmRegex()) and + // Exclude results covered by `InsecureAlgoProperty`. + // This removes duplicates when a string literal is a default value for the property, + // such as "MD5" in the following: `props.getProperty("hashAlg2", "MD5")`. + not exists(InsecureAlgoProperty insecAlgoProp | this = insecAlgoProp.getAnArgument()) ) } diff --git a/java/ql/src/change-notes/2024-10-29-weak-crypto-hash.md b/java/ql/src/change-notes/2024-10-29-weak-crypto-hash.md new file mode 100644 index 000000000000..b4ac88bcdc6a --- /dev/null +++ b/java/ql/src/change-notes/2024-10-29-weak-crypto-hash.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The `java/weak-cryptographic-algorithm` query has been updated to no longer report uses of hash functions such as `MD5` and `SHA1` even if they are known to be weak. These hash algorithms are used very often in non-sensitive contexts, making the query too imprecise in practice. The `java/potentially-weak-cryptographic-algorithm` query has been updated to report these uses instead. 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 612e1c730544..94719b477391 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,14 +1,8 @@ #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 | -| WeakHashing.java:21:30:21:92 | getInstance(...) | WeakHashing.java:21:86:21:90 | "MD5" : String | WeakHashing.java:21:56:21:91 | getProperty(...) | Cryptographic algorithm $@ is weak and should not be used. | WeakHashing.java:21:86:21:90 | "MD5" | MD5 | edges -| WeakHashing.java:21:86:21:90 | "MD5" : String | WeakHashing.java:21:56:21:91 | getProperty(...) | provenance | MaD:1 | -models -| 1 | Summary: java.util; Properties; true; getProperty; (String,String); ; Argument[1]; ReturnValue; value; manual | nodes | Test.java:19:45:19:49 | "DES" | semmle.label | "DES" | | Test.java:42:33:42:37 | "RC2" | semmle.label | "RC2" | -| WeakHashing.java:21:56:21:91 | getProperty(...) | semmle.label | getProperty(...) | -| WeakHashing.java:21:86:21:90 | "MD5" : String | semmle.label | "MD5" : String | subpaths