Revamp the query to use three configurations to detect password hash without salt

Author: luchua-bc

java/ql/src/experimental/Security/CWE/CWE-759/HashWithoutSalt.ql

@@ -8,14 +8,26 @@
  */
 
 import java
+import semmle.code.java.dataflow.DataFlow3
 import semmle.code.java.dataflow.TaintTracking
+import semmle.code.java.dataflow.TaintTracking2
+import semmle.code.java.dataflow.TaintTracking3
 import DataFlow::PathGraph
 
 /** The Java class `java.security.MessageDigest`. */
 class MessageDigest extends RefType {
   MessageDigest() { this.hasQualifiedName("java.security", "MessageDigest") }
 }
 
+class MDConstructor extends StaticMethodAccess {
+  MDConstructor() {
+    exists(Method m | m = this.getMethod() |
+      m.getDeclaringType() instanceof MessageDigest and
+      m.hasName("getInstance")
+    )
+  }
+}
+
 /** The method `digest()` declared in `java.security.MessageDigest`. */
 class MDDigestMethod extends Method {
   MDDigestMethod() {
@@ -48,24 +60,20 @@ class PasswordVarExpr extends Expr {
 }
 
 /** Taint configuration tracking flow from an expression whose name suggests it holds password data to a method call that generates a hash without a salt. */
-class HashWithoutSaltConfiguration extends TaintTracking::Configuration {
-  HashWithoutSaltConfiguration() { this = "HashWithoutSaltConfiguration" }
+class PasswordHashConfiguration extends TaintTracking3::Configuration {
+  PasswordHashConfiguration() { this = "PasswordHashConfiguration" }
 
-  override predicate isSource(DataFlow::Node source) { source.asExpr() instanceof PasswordVarExpr }
+  override predicate isSource(DataFlow3::Node source) { source.asExpr() instanceof PasswordVarExpr }
 
-  override predicate isSink(DataFlow::Node sink) {
+  override predicate isSink(DataFlow3::Node sink) {
     exists(
-      MethodAccess mua // invoke `md.update(password)` without the call of `md.update(digest)`
+      MethodAccess ma // invoke `md.update(password)` without the call of `md.update(digest)`
     |
-      sink.asExpr() = mua.getArgument(0) and
-      mua.getMethod() instanceof MDUpdateMethod // md.update(password)
-    )
-    or
-    // invoke `md.digest(password)` without another call of `md.update(salt)`
-    exists(MethodAccess mda |
-      sink.asExpr() = mda.getArgument(0) and
-      mda.getMethod() instanceof MDDigestMethod and // md.digest(password)
-      mda.getNumArgument() = 1
+      sink.asExpr() = ma.getArgument(0) and
+      (
+        ma.getMethod() instanceof MDUpdateMethod or // md.update(password)
+        ma.getMethod() instanceof MDDigestMethod // md.digest(password)
+      )
     )
   }
 
@@ -76,48 +84,64 @@ class HashWithoutSaltConfiguration extends TaintTracking::Configuration {
    *  `byte[] messageDigest = md.digest(allBytes);`
    * Or the password is concatenated with a salt as a string.
    */
-  override predicate isSanitizer(DataFlow::Node node) {
+  override predicate isSanitizer(DataFlow3::Node node) {
     exists(MethodAccess ma |
       ma.getMethod().getDeclaringType().hasQualifiedName("java.lang", "System") and
       ma.getMethod().hasName("arraycopy") and
       ma.getArgument(0) = node.asExpr()
     ) // System.arraycopy(password.getBytes(), ...)
     or
     exists(AddExpr e | node.asExpr() = e.getAnOperand()) // password+salt
-    or
-    exists(MethodAccess mua, MethodAccess ma |
-      ma.getArgument(0) = node.asExpr() and // Detect wrapper methods that invoke `md.update(salt)`
-      ma != mua and
+  }
+}
+
+class PasswordDigestConfiguration extends TaintTracking2::Configuration {
+  PasswordDigestConfiguration() { this = "PasswordDigestConfiguration" }
+
+  override predicate isSource(DataFlow2::Node source) {
+    exists(MDConstructor mc | source.asExpr() = mc)
+  }
+
+  override predicate isSink(DataFlow2::Node sink) {
+    exists(MethodAccess ma |
       (
-        ma.getQualifier().getType() instanceof Interface
-        or
-        mua.getQualifier().(VarAccess).getVariable().getAnAccess() = ma.getQualifier()
-        or
-        mua.getAnArgument().(VarAccess).getVariable().getAnAccess() = ma.getQualifier()
-        or
-        mua.getQualifier().(VarAccess).getVariable().getAnAccess() = ma.getAnArgument()
-        or
-        mua.getArgument(0).(VarAccess).getVariable().getAnAccess() = ma.getAnArgument()
+        ma.getMethod() instanceof MDUpdateMethod or
+        ma.getMethod() instanceof MDDigestMethod
       ) and
-      isMDUpdateCall(mua.getMethod())
+      exists(PasswordHashConfiguration cc | cc.hasFlowToExpr(ma.getAnArgument())) and
+      sink.asExpr() = ma.getQualifier()
     )
   }
 }
 
-/** Holds if a method invokes `md.update(salt)`. */
-predicate isMDUpdateCall(Callable caller) {
-  caller instanceof MDUpdateMethod
-  or
-  exists(Callable callee |
-    caller.polyCalls(callee) and
-    (
-      callee instanceof MDUpdateMethod or
-      isMDUpdateCall(callee)
+class HashWithoutSaltConfiguration extends TaintTracking::Configuration {
+  HashWithoutSaltConfiguration() { this = "HashWithoutSaltConfiguration" }
+
+  override predicate isSource(DataFlow::Node source) {
+    exists(PasswordDigestConfiguration pc | pc.hasFlow(source, _))
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(MethodAccess ma |
+      ma.getMethod() instanceof MDDigestMethod and // md.digest(password)
+      sink.asExpr() = ma.getQualifier()
+    )
+  }
+
+  /** Holds if `md.update` or `md.digest` calls integrate something other than the password, perhaps a salt. */
+  override predicate isSanitizer(DataFlow::Node node) {
+    exists(MethodAccess ma |
+      (
+        ma.getMethod() instanceof MDUpdateMethod
+        or
+        ma.getMethod() instanceof MDDigestMethod and ma.getNumArgument() != 0
+      ) and
+      node.asExpr() = ma.getQualifier() and
+      not exists(PasswordHashConfiguration cc | cc.hasFlowToExpr(ma.getAnArgument()))
     )
-  )
+  }
 }
 
-from DataFlow::PathNode source, DataFlow::PathNode sink, HashWithoutSaltConfiguration c
-where c.hasFlowPath(source, sink)
-select sink.getNode(), source, sink, "$@ is hashed without a salt.", source.getNode(),
-  "The password"
+from DataFlow::PathNode source, DataFlow::PathNode sink, HashWithoutSaltConfiguration cc
+where cc.hasFlowPath(source, sink)
+select sink, source, sink, "$@ is hashed without a salt.", source, "The password"

java/ql/src/semmle/code/java/dataflow/TaintTracking3.qll

@@ -0,0 +1,7 @@
+/**
+ * Provides classes for performing local (intra-procedural) and
+ * global (inter-procedural) taint-tracking analyses.
+ */
+module TaintTracking3 {
+  import semmle.code.java.dataflow.internal.tainttracking3.TaintTrackingImpl
+}

java/ql/src/semmle/code/java/dataflow/internal/tainttracking3/TaintTrackingImpl.qll

@@ -0,0 +1,115 @@
+/**
+ * Provides an implementation of global (interprocedural) taint tracking.
+ * This file re-exports the local (intraprocedural) taint-tracking analysis
+ * from `TaintTrackingParameter::Public` and adds a global analysis, mainly
+ * exposed through the `Configuration` class. For some languages, this file
+ * exists in several identical copies, allowing queries to use multiple
+ * `Configuration` classes that depend on each other without introducing
+ * mutual recursion among those configurations.
+ */
+
+import TaintTrackingParameter::Public
+private import TaintTrackingParameter::Private
+
+/**
+ * A configuration of interprocedural taint tracking analysis. This defines
+ * sources, sinks, and any other configurable aspect of the analysis. Each
+ * use of the taint tracking library must define its own unique extension of
+ * this abstract class.
+ *
+ * A taint-tracking configuration is a special data flow configuration
+ * (`DataFlow::Configuration`) that allows for flow through nodes that do not
+ * necessarily preserve values but are still relevant from a taint tracking
+ * perspective. (For example, string concatenation, where one of the operands
+ * is tainted.)
+ *
+ * To create a configuration, extend this class with a subclass whose
+ * characteristic predicate is a unique singleton string. For example, write
+ *
+ * ```ql
+ * class MyAnalysisConfiguration extends TaintTracking::Configuration {
+ *   MyAnalysisConfiguration() { this = "MyAnalysisConfiguration" }
+ *   // Override `isSource` and `isSink`.
+ *   // Optionally override `isSanitizer`.
+ *   // Optionally override `isSanitizerIn`.
+ *   // Optionally override `isSanitizerOut`.
+ *   // Optionally override `isSanitizerGuard`.
+ *   // Optionally override `isAdditionalTaintStep`.
+ * }
+ * ```
+ *
+ * Then, to query whether there is flow between some `source` and `sink`,
+ * write
+ *
+ * ```ql
+ * exists(MyAnalysisConfiguration cfg | cfg.hasFlow(source, sink))
+ * ```
+ *
+ * Multiple configurations can coexist, but it is unsupported to depend on
+ * another `TaintTracking::Configuration` or a `DataFlow::Configuration` in the
+ * overridden predicates that define sources, sinks, or additional steps.
+ * Instead, the dependency should go to a `TaintTracking2::Configuration` or a
+ * `DataFlow2::Configuration`, `DataFlow3::Configuration`, etc.
+ */
+abstract class Configuration extends DataFlow::Configuration {
+  bindingset[this]
+  Configuration() { any() }
+
+  /**
+   * Holds if `source` is a relevant taint source.
+   *
+   * The smaller this predicate is, the faster `hasFlow()` will converge.
+   */
+  // overridden to provide taint-tracking specific qldoc
+  abstract override predicate isSource(DataFlow::Node source);
+
+  /**
+   * Holds if `sink` is a relevant taint sink.
+   *
+   * The smaller this predicate is, the faster `hasFlow()` will converge.
+   */
+  // overridden to provide taint-tracking specific qldoc
+  abstract override predicate isSink(DataFlow::Node sink);
+
+  /** Holds if the node `node` is a taint sanitizer. */
+  predicate isSanitizer(DataFlow::Node node) { none() }
+
+  final override predicate isBarrier(DataFlow::Node node) {
+    isSanitizer(node) or
+    defaultTaintSanitizer(node)
+  }
+
+  /** Holds if taint propagation into `node` is prohibited. */
+  predicate isSanitizerIn(DataFlow::Node node) { none() }
+
+  final override predicate isBarrierIn(DataFlow::Node node) { isSanitizerIn(node) }
+
+  /** Holds if taint propagation out of `node` is prohibited. */
+  predicate isSanitizerOut(DataFlow::Node node) { none() }
+
+  final override predicate isBarrierOut(DataFlow::Node node) { isSanitizerOut(node) }
+
+  /** Holds if taint propagation through nodes guarded by `guard` is prohibited. */
+  predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { none() }
+
+  final override predicate isBarrierGuard(DataFlow::BarrierGuard guard) { isSanitizerGuard(guard) }
+
+  /**
+   * Holds if the additional taint propagation step from `node1` to `node2`
+   * must be taken into account in the analysis.
+   */
+  predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { none() }
+
+  final override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
+    isAdditionalTaintStep(node1, node2) or
+    defaultAdditionalTaintStep(node1, node2)
+  }
+
+  /**
+   * Holds if taint may flow from `source` to `sink` for this configuration.
+   */
+  // overridden to provide taint-tracking specific qldoc
+  override predicate hasFlow(DataFlow::Node source, DataFlow::Node sink) {
+    super.hasFlow(source, sink)
+  }
+}

java/ql/src/semmle/code/java/dataflow/internal/tainttracking3/TaintTrackingParameter.qll

@@ -0,0 +1,5 @@
+import semmle.code.java.dataflow.internal.TaintTrackingUtil as Public
+
+module Private {
+  import semmle.code.java.dataflow.DataFlow3::DataFlow3 as DataFlow
+}