Merge pull request #17869 from jcogs33/jcogs33/improve-weak-crypto

Java: Improve weak crypto query
github · Nov 24, 2024 · f004569 · f004569
2 parents 7baaa23 + 459d168
commit f004569
Show file tree

Hide file tree

Showing 4 changed files with 10 additions and 11 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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