Merge pull request #100 from atorralba/atorralba/java/weak-hashing-suggestion

Java: Generalize MaybeBrokenCryptoAlgorithmQuery.qll

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,17 +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, string value |
-      n.asExpr() = mc and value = mc.getPropertyValue()
-    |
-      // 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 value.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,24 +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
-  exists(string value | value = algo.asExpr().(PropertiesGetPropertyMethodCall).getPropertyValue() |
-    result = value and not value.regexpMatch(getSecureAlgorithmRegex())
-  )
-}
-
 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()