Merge pull request #14040 from egregius313/egregius313/weak-hashing-properties

Java: Add support for algorithm names specified in `.properties` files to `java/potentially-weak-cryptographic-algorithm`

Author: egregius313

java/ql/lib/semmle/code/java/frameworks/Properties.qll

@@ -2,6 +2,8 @@
 
 import semmle.code.java.Type
 private import semmle.code.java.dataflow.FlowSteps
+private import semmle.code.configfiles.ConfigFiles
+private import semmle.code.java.dataflow.RangeUtils
 
 /**
  * The `java.util.Properties` class.
@@ -43,3 +45,22 @@ class PropertiesStoreMethod extends Method {
     (this.getName().matches("store%") or this.getName() = "save")
   }
 }
+
+/**
+ * A call to the `getProperty` method of the class `java.util.Properties`.
+ */
+class PropertiesGetPropertyMethodCall extends MethodCall {
+  PropertiesGetPropertyMethodCall() { this.getMethod() instanceof PropertiesGetPropertyMethod }
+
+  private ConfigPair getPair() {
+    this.getArgument(0).(ConstantStringExpr).getStringValue() = result.getNameElement().getName()
+  }
+
+  /**
+   * Get the potential string values that can be associated with the given property name.
+   */
+  string getPropertyValue() {
+    result = this.getPair().getValueElement().getValue() or
+    result = this.getArgument(1).(ConstantStringExpr).getStringValue()
+  }
+}

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

@@ -3,9 +3,12 @@
  */
 
 import java
+private import semmle.code.configfiles.ConfigFiles
 private import semmle.code.java.security.Encryption
 private import semmle.code.java.dataflow.TaintTracking
+private import semmle.code.java.dataflow.RangeUtils
 private import semmle.code.java.dispatch.VirtualDispatch
+private import semmle.code.java.frameworks.Properties
 
 private class ShortStringLiteral extends StringLiteral {
   ShortStringLiteral() { this.getValue().length() < 100 }
@@ -38,7 +41,15 @@ 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 }
+  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 isSink(DataFlow::Node n) { exists(CryptoAlgoSpec c | n.asExpr() = c.getAlgoSpec()) }
 

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

@@ -13,16 +13,27 @@
 
 import java
 import semmle.code.java.security.Encryption
+import semmle.code.java.dataflow.DataFlow
+import semmle.code.java.frameworks.Properties
 import semmle.code.java.security.MaybeBrokenCryptoAlgorithmQuery
 import InsecureCryptoFlow::PathGraph
 
-from
-  InsecureCryptoFlow::PathNode source, InsecureCryptoFlow::PathNode sink, CryptoAlgoSpec c,
-  InsecureAlgoLiteral s
+/**
+ * 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
-  source.getNode().asExpr() = s and
   InsecureCryptoFlow::flowPath(source, sink)
 select c, source, sink,
-  "Cryptographic algorithm $@ may not be secure, consider using a different algorithm.", s,
-  s.getValue()
+  "Cryptographic algorithm $@ may not be secure, consider using a different algorithm.", source,
+  getStringValue(source.getNode())

java/ql/src/change-notes/2023-12-15-weak-cryptographic-algorithm-from-properties-file.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Modified the `java/potentially-weak-cryptographic-algorithm` query to include the use of weak cryptographic algorithms from configuration values specified in properties files.

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

@@ -1,6 +1,8 @@
 edges
 nodes
 | Test.java:34:48:34:52 | "foo" | semmle.label | "foo" |
+| WeakHashing.java:15:55:15:83 | 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 |

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

@@ -0,0 +1,20 @@
+package test.cwe327.semmle.tests;
+
+import java.util.Properties;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+
+public class WeakHashing {
+    void hashing() throws NoSuchAlgorithmException, IOException {
+        java.util.Properties props = new java.util.Properties();
+        props.load(new FileInputStream("example.properties"));
+
+        // BAD: Using a weak hashing algorithm
+        MessageDigest bad = MessageDigest.getInstance(props.getProperty("hashAlg1"));
+
+        // GOOD: Using a strong hashing algorithm
+        MessageDigest ok = MessageDigest.getInstance(props.getProperty("hashAlg2"));
+    }
+}

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

@@ -0,0 +1,2 @@
+hashAlg1=MD5
+hashAlg2=SHA-256