Move weak hashing into MaybeBrokenCryptoAlgorithm

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 }
@@ -34,11 +37,38 @@ private predicate objectToString(MethodCall ma) {
   )
 }
 
+private class GetPropertyMethodCall extends MethodCall {
+  GetPropertyMethodCall() { this.getMethod() instanceof PropertiesGetPropertyMethod }
+
+  private ConfigPair getPair() {
+    this.getArgument(0).(ConstantStringExpr).getStringValue() = result.getNameElement().getName()
+  }
+
+  string getPropertyValue() {
+    result = this.getPair().getValueElement().getValue() or
+    result = this.getArgument(1).(ConstantStringExpr).getStringValue()
+  }
+}
+
+string insecureAlgorithmName(DataFlow::Node algo) {
+  result = algo.asExpr().(StringLiteral).getValue()
+  or
+  result = algo.asExpr().(GetPropertyMethodCall).getPropertyValue()
+}
+
 /**
  * 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(GetPropertyMethodCall 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/lib/semmle/code/java/security/WeakHashingAlgorithmPropertyQuery.qll

@@ -1,54 +0,0 @@
-/** Provides classes and predicates to reason about property files and weak hashing algorithms. */
-
-import java
-private import semmle.code.configfiles.ConfigFiles
-private import semmle.code.java.dataflow.DataFlow
-private import semmle.code.java.dataflow.TaintTracking
-private import semmle.code.java.security.Encryption
-private import semmle.code.java.frameworks.Properties
-private import semmle.code.java.dataflow.RangeUtils
-
-private class GetPropertyMethodCall extends MethodCall {
-  GetPropertyMethodCall() { this.getMethod() instanceof PropertiesGetPropertyMethod }
-
-  private ConfigPair getPair() {
-    this.getArgument(0).(ConstantStringExpr).getStringValue() = result.getNameElement().getName()
-  }
-
-  string getPropertyValue() {
-    result = this.getPair().getValueElement().getValue() or
-    result = this.getArgument(1).(ConstantStringExpr).getStringValue()
-  }
-}
-
-/**
- * Get the name of the weak cryptographic algorithm represented by `node`.
- */
-string getWeakHashingAlgorithmName(DataFlow::Node node) {
-  exists(MethodCall mc, ConfigPair pair |
-    node.asExpr() = mc and mc.getMethod() instanceof PropertiesGetPropertyMethod
-  |
-    mc.getArgument(0).(ConstantStringExpr).getStringValue() = pair.getNameElement().getName() and
-    pair.getValueElement().getValue() = result and
-    not pair.getValueElement().getValue().regexpMatch(getSecureAlgorithmRegex())
-  )
-}
-
-/**
- * Dataflow configuration from a configuration pair in a properties file to the use of a cryptographic algorithm.
- */
-module InsecureAlgorithmPropertyConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node n) {
-    exists(GetPropertyMethodCall mc, string algo | n.asExpr() = mc |
-      algo = mc.getPropertyValue() and
-      not algo.regexpMatch(getSecureAlgorithmRegex())
-    )
-  }
-
-  predicate isSink(DataFlow::Node n) { n.asExpr() = any(CryptoAlgoSpec c).getAlgoSpec() }
-}
-
-/**
- * Dataflow from a configuration pair in a properties file to the use of a cryptographic algorithm.
- */
-module InsecureAlgorithmPropertyFlow = TaintTracking::Global<InsecureAlgorithmPropertyConfig>;

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

@@ -16,13 +16,10 @@ import semmle.code.java.security.Encryption
 import semmle.code.java.security.MaybeBrokenCryptoAlgorithmQuery
 import InsecureCryptoFlow::PathGraph
 
-from
-  InsecureCryptoFlow::PathNode source, InsecureCryptoFlow::PathNode sink, CryptoAlgoSpec c,
-  InsecureAlgoLiteral s
+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,
+  insecureAlgorithmName(source.getNode())

java/ql/src/Security/CWE/CWE-328/WeakHashingProperty.java

@@ -1,9 +0,0 @@
-import java.io.FileInputStream;
-import java.util.Properties;
-import java.security.MessageDigest;
-
-Properties props = Properties.load(new FileInputStream("settings.properties"));
-
-// BAD: the `hashAlgorithm` variable in `settings.properties` is `MD5` which is
-// a weak hashing algorithm.
-MessageDigest.getInstance(props.getProperty("hashAlgorithm"));

java/ql/src/Security/CWE/CWE-328/WeakHashingProperty.qhelp

@@ -1,32 +0,0 @@
-<!DOCTYPE qhelp PUBLIC
-  "-//Semmle//qhelp//EN"
-  "qhelp.dtd">
-<qhelp>
-  <overview>
-    <p>
-      Using a weak hashing algorithm can result in attackers being able to
-      determine the original input to a hash function or create a second input
-      which will produce the same hash.
-    </p>
-  </overview>
-
-  <recommendation>
-    <p>Ensure you are using a strong, modern hashing algorithm, such as SHA-256.</p>
-  </recommendation>
-
-  <example>
-    <p>In the following (BAD) example, the <code>MD5</code> hashing algorithm is used, specified in a <code>.properties</code> file.</p>
-
-    <sample src="settings.properties"/>
-
-    <sample src="WeakHashingProperty.java"/>
-
-  </example>
-
-  <references>
-    <li>NIST, FIPS 140 Annex a: <a href="http://csrc.nist.gov/publications/fips/fips140-2/fips1402annexa.pdf">
-Approved Security Functions</a>.</li>
-    <li>NIST, SP 800-131A: <a href="http://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-131Ar1.pdf">
-Transitions: Recommendation for Transitioning the Use of Cryptographic Algorithms and Key Lengths</a>.</li>
-  </references>
-</qhelp>

java/ql/src/Security/CWE/CWE-328/WeakHashingProperty.ql

@@ -1,20 +0,0 @@
-/**
- * @name Weak hashing algorithm specified in properties file
- * @description Using weak cryptographic algorithms can allow an attacker to compromise security.
- * @id java/weak-hashing-property
- * @kind path-problem
- * @problem.severity error
- * @security-severity 7.5
- * @precision medium
- * @tags security
- *      external/cwe/cwe-328
- */
-
-import java
-import semmle.code.java.security.WeakHashingAlgorithmPropertyQuery
-import InsecureAlgorithmPropertyFlow::PathGraph
-
-from InsecureAlgorithmPropertyFlow::PathNode source, InsecureAlgorithmPropertyFlow::PathNode sink
-where InsecureAlgorithmPropertyFlow::flowPath(source, sink)
-select sink.getNode(), sink, source, "The $@ hashing algorithm is insecure.", source,
-  getWeakHashingAlgorithmName(source.getNode())

java/ql/src/Security/CWE/CWE-328/settings.properties

@@ -1 +0,0 @@
-hashAlgorithm=MD5