Merge pull request github#15183 from egregius313/egregius313/java/fix-weak-hashing-adddition

Java: Fix minor error in `java/potentially-weak-cryptographic-algorithm`

Author: egregius313

java/ql/lib/semmle/code/java/security/MaybeBrokenCryptoAlgorithmQuery.qll

@@ -10,23 +10,47 @@ private import semmle.code.java.dataflow.RangeUtils
 private import semmle.code.java.dispatch.VirtualDispatch
 private import semmle.code.java.frameworks.Properties
 
+/** A reference to an insecure cryptographic algorithm. */
+abstract class InsecureAlgorithm extends Expr {
+  /** Gets the string representation of this insecure cryptographic algorithm. */
+  abstract string getStringValue();
+}
+
 private class ShortStringLiteral extends StringLiteral {
   ShortStringLiteral() { this.getValue().length() < 100 }
 }
 
 /**
  * A string literal that may refer to an insecure cryptographic algorithm.
  */
-class InsecureAlgoLiteral extends ShortStringLiteral {
+class InsecureAlgoLiteral extends InsecureAlgorithm, ShortStringLiteral {
   InsecureAlgoLiteral() {
-    // Algorithm identifiers should be at least two characters.
-    this.getValue().length() > 1 and
     exists(string s | s = this.getValue() |
+      // Algorithm identifiers should be at least two characters.
+      s.length() > 1 and
       not s.regexpMatch(getSecureAlgorithmRegex()) and
       // Exclude results covered by another query.
       not s.regexpMatch(getInsecureAlgorithmRegex())
     )
   }
+
+  override string getStringValue() { result = this.getValue() }
+}
+
+/**
+ * A property access that may refer to an insecure cryptographic algorithm.
+ */
+class InsecureAlgoProperty extends InsecureAlgorithm, PropertiesGetPropertyMethodCall {
+  string value;
+
+  InsecureAlgoProperty() {
+    value = this.getPropertyValue() and
+    // Since properties pairs are not included in the java/weak-cryptographic-algorithm,
+    // the check for values from properties files can be less strict than `InsecureAlgoLiteral`.
+    not value.regexpMatch(getSecureAlgorithmRegex())
+  }
+
+  override string getStringValue() { result = value }
 }
 
 private predicate objectToString(MethodCall ma) {
@@ -41,15 +65,7 @@ private predicate objectToString(MethodCall ma) {
  * A taint-tracking configuration to reason about the use of potentially insecure cryptographic algorithms.
  */
 module InsecureCryptoConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node n) {
-    n.asExpr() instanceof InsecureAlgoLiteral
-    or
-    exists(PropertiesGetPropertyMethodCall mc | n.asExpr() = mc |
-      // Since properties pairs are not included in the java/weak-crypto-algorithm,
-      // The check for values from properties files can be less strict than `InsecureAlgoLiteral`.
-      not mc.getPropertyValue().regexpMatch(getSecureAlgorithmRegex())
-    )
-  }
+  predicate isSource(DataFlow::Node n) { n.asExpr() instanceof InsecureAlgorithm }
 
   predicate isSink(DataFlow::Node n) { exists(CryptoAlgoSpec c | n.asExpr() = c.getAlgoSpec()) }
 

java/ql/src/Security/CWE/CWE-327/MaybeBrokenCryptoAlgorithm.ql

@@ -18,22 +18,10 @@ import semmle.code.java.frameworks.Properties
 import semmle.code.java.security.MaybeBrokenCryptoAlgorithmQuery
 import InsecureCryptoFlow::PathGraph
 
-/**
- * Get the string value represented by the given expression.
- *
- * If the value is a string literal, get the literal value.
- * If the value is a call to `java.util.Properties::getProperty`, get the potential values of the property.
- */
-string getStringValue(DataFlow::Node algo) {
-  result = algo.asExpr().(StringLiteral).getValue()
-  or
-  result = algo.asExpr().(PropertiesGetPropertyMethodCall).getPropertyValue()
-}
-
 from InsecureCryptoFlow::PathNode source, InsecureCryptoFlow::PathNode sink, CryptoAlgoSpec c
 where
   sink.getNode().asExpr() = c.getAlgoSpec() and
   InsecureCryptoFlow::flowPath(source, sink)
 select c, source, sink,
   "Cryptographic algorithm $@ may not be secure, consider using a different algorithm.", source,
-  getStringValue(source.getNode())
+  source.getNode().asExpr().(InsecureAlgorithm).getStringValue()

java/ql/test/query-tests/security/CWE-327/semmle/tests/BrokenCryptoAlgorithm.expected

@@ -1,8 +1,12 @@
 edges
+| WeakHashing.java:21:86:21:90 | "MD5" : String | WeakHashing.java:21:56:21:91 | getProperty(...) |
 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
 #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 |

java/ql/test/query-tests/security/CWE-327/semmle/tests/MaybeBrokenCryptoAlgorithm.expected

@@ -2,7 +2,11 @@ edges
 nodes
 | Test.java:34:48:34:52 | "foo" | semmle.label | "foo" |
 | WeakHashing.java:15:55:15:83 | getProperty(...) | semmle.label | getProperty(...) |
+| WeakHashing.java:18:56:18:95 | getProperty(...) | semmle.label | getProperty(...) |
+| WeakHashing.java:21:56:21:91 | getProperty(...) | semmle.label | getProperty(...) |
 subpaths
 #select
 | Test.java:34:21:34:53 | new SecretKeySpec(...) | Test.java:34:48:34:52 | "foo" | Test.java:34:48:34:52 | "foo" | Cryptographic algorithm $@ may not be secure, consider using a different algorithm. | Test.java:34:48:34:52 | "foo" | foo |
 | WeakHashing.java:15:29:15:84 | getInstance(...) | WeakHashing.java:15:55:15:83 | getProperty(...) | WeakHashing.java:15:55:15:83 | getProperty(...) | Cryptographic algorithm $@ may not be secure, consider using a different algorithm. | WeakHashing.java:15:55:15:83 | getProperty(...) | MD5 |
+| WeakHashing.java:18:30:18:96 | getInstance(...) | WeakHashing.java:18:56:18:95 | getProperty(...) | WeakHashing.java:18:56:18:95 | getProperty(...) | Cryptographic algorithm $@ may not be secure, consider using a different algorithm. | WeakHashing.java:18:56:18:95 | getProperty(...) | MD5 |
+| WeakHashing.java:21:30:21:92 | getInstance(...) | WeakHashing.java:21:56:21:91 | getProperty(...) | WeakHashing.java:21:56:21:91 | getProperty(...) | Cryptographic algorithm $@ may not be secure, consider using a different algorithm. | WeakHashing.java:21:56:21:91 | getProperty(...) | MD5 |

java/ql/test/query-tests/security/CWE-327/semmle/tests/WeakHashing.java

@@ -14,7 +14,16 @@ void hashing() throws NoSuchAlgorithmException, IOException {
         // BAD: Using a weak hashing algorithm
         MessageDigest bad = MessageDigest.getInstance(props.getProperty("hashAlg1"));
 
+        // BAD: Using a weak hashing algorithm even with a secure default
+        MessageDigest bad2 = MessageDigest.getInstance(props.getProperty("hashAlg1", "SHA-256"));
+
+        // BAD: Using a strong hashing algorithm but with a weak default
+        MessageDigest bad3 = MessageDigest.getInstance(props.getProperty("hashAlg2", "MD5"));
+        
         // GOOD: Using a strong hashing algorithm
         MessageDigest ok = MessageDigest.getInstance(props.getProperty("hashAlg2"));
+
+        // OK: Property does not exist and default is secure
+        MessageDigest ok2 = MessageDigest.getInstance(props.getProperty("hashAlg3", "SHA-256"));
     }
 }