Merge pull request github#14297 from aschackmull/java/additional-steps-and-nodes

Java: Add support for additional nodes, read steps, and store steps for QL models and model ThreadLocal.initialValue

Author: aschackmull

java/ql/lib/semmle/code/java/dataflow/FlowSteps.qll

@@ -23,6 +23,7 @@ private module Frameworks {
   private import semmle.code.java.frameworks.InputStream
   private import semmle.code.java.frameworks.Properties
   private import semmle.code.java.frameworks.Protobuf
+  private import semmle.code.java.frameworks.ThreadLocal
   private import semmle.code.java.frameworks.ratpack.RatpackExec
   private import semmle.code.java.frameworks.stapler.Stapler
 }
@@ -57,6 +58,22 @@ abstract class FluentMethod extends ValuePreservingMethod {
   override predicate returnsValue(int arg) { arg = -1 }
 }
 
+/**
+ * A unit class for adding additional data flow nodes.
+ *
+ * Extend this class to add additional data flow nodes for use in globally
+ * applicable additional steps.
+ */
+class AdditionalDataFlowNode extends Unit {
+  /**
+   * Holds if an additional node is needed in relation to `e`. The pair `(e,id)`
+   * must uniquely identify the node.
+   * The added node can be selected for use in a predicate by the corresponding
+   * `DataFlow::AdditionalNode.nodeAt(Expr e, string id)` predicate.
+   */
+  abstract predicate nodeAt(Expr e, string id);
+}
+
 /**
  * A unit class for adding additional taint steps.
  *
@@ -85,6 +102,36 @@ class AdditionalValueStep extends Unit {
   abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
 }
 
+/**
+ * A unit class for adding additional store steps.
+ *
+ * Extend this class to add additional store steps that should apply to all
+ * data flow configurations. A store step must be local, so non-local steps are
+ * ignored.
+ */
+class AdditionalStoreStep extends Unit {
+  /**
+   * Holds if the step from `node1` to `node2` is a store step of `c` and should
+   * apply to all data flow configurations.
+   */
+  abstract predicate step(DataFlow::Node node1, DataFlow::Content c, DataFlow::Node node2);
+}
+
+/**
+ * A unit class for adding additional read steps.
+ *
+ * Extend this class to add additional read steps that should apply to all
+ * data flow configurations. A read step must be local, so non-local steps are
+ * ignored.
+ */
+class AdditionalReadStep extends Unit {
+  /**
+   * Holds if the step from `node1` to `node2` is a read step of `c` and should
+   * apply to all data flow configurations.
+   */
+  abstract predicate step(DataFlow::Node node1, DataFlow::Content c, DataFlow::Node node2);
+}
+
 /**
  * A method or constructor that preserves taint.
  *

java/ql/lib/semmle/code/java/dataflow/internal/DataFlowNodes.qll

@@ -3,6 +3,7 @@ private import semmle.code.java.dataflow.InstanceAccess
 private import semmle.code.java.dataflow.ExternalFlow
 private import semmle.code.java.dataflow.FlowSummary
 private import semmle.code.java.dataflow.TypeFlow
+private import semmle.code.java.dataflow.FlowSteps
 private import DataFlowPrivate
 private import DataFlowUtil
 private import FlowSummaryImpl as FlowSummaryImpl
@@ -56,7 +57,8 @@ private module Cached {
     } or
     TFlowSummaryNode(FlowSummaryImpl::Private::SummaryNode sn) or
     TFieldValueNode(Field f) or
-    TCaptureNode(CaptureFlow::SynthesizedCaptureNode cn)
+    TCaptureNode(CaptureFlow::SynthesizedCaptureNode cn) or
+    TAdditionalNode(Expr e, string id) { any(AdditionalDataFlowNode adfn).nodeAt(e, id) }
 
   cached
   newtype TContent =
@@ -133,6 +135,8 @@ module Public {
       result = this.(CaptureNode).getTypeImpl()
       or
       result = this.(FieldValueNode).getField().getType()
+      or
+      result instanceof TypeObject and this instanceof AdditionalNode
     }
 
     /** Gets the callable in which this node occurs. */
@@ -335,6 +339,21 @@ module Public {
     /** Holds if this is an access to an object's own instance. */
     predicate isOwnInstanceAccess() { this.getInstanceAccess().isOwnInstanceAccess() }
   }
+
+  /** A node introduced by an extension of `AdditionalDataFlowNode`. */
+  class AdditionalNode extends Node, TAdditionalNode {
+    Expr e_;
+    string id_;
+
+    AdditionalNode() { this = TAdditionalNode(e_, id_) }
+
+    override string toString() { result = e_.toString() + " (" + id_ + ")" }
+
+    override Location getLocation() { result = e_.getLocation() }
+
+    /** Holds if this node was introduced by `AdditionalDataFlowNode.nodeAt(e, id)`. */
+    predicate nodeAt(Expr e, string id) { e = e_ and id = id_ }
+  }
 }
 
 private import Public
@@ -378,7 +397,8 @@ module Private {
     result = nodeGetEnclosingCallable(n.(ImplicitPostUpdateNode).getPreUpdateNode()) or
     result.asSummarizedCallable() = n.(FlowSummaryNode).getSummarizedCallable() or
     result.asCallable() = n.(CaptureNode).getSynthesizedCaptureNode().getEnclosingCallable() or
-    result.asFieldScope() = n.(FieldValueNode).getField()
+    result.asFieldScope() = n.(FieldValueNode).getField() or
+    result.asCallable() = any(Expr e | n.(AdditionalNode).nodeAt(e, _)).getEnclosingCallable()
   }
 
   /** Holds if `p` is a `ParameterNode` of `c` with position `pos`. */

java/ql/lib/semmle/code/java/dataflow/internal/DataFlowPrivate.qll

@@ -228,6 +228,10 @@ predicate storeStep(Node node1, ContentSet f, Node node2) {
     node2.(FlowSummaryNode).getSummaryNode())
   or
   captureStoreStep(node1, f, node2)
+  or
+  any(AdditionalStoreStep a).step(node1, f, node2) and
+  pragma[only_bind_out](node1.getEnclosingCallable()) =
+    pragma[only_bind_out](node2.getEnclosingCallable())
 }
 
 /**
@@ -262,6 +266,10 @@ predicate readStep(Node node1, ContentSet f, Node node2) {
     node2.(FlowSummaryNode).getSummaryNode())
   or
   captureReadStep(node1, f, node2)
+  or
+  any(AdditionalReadStep a).step(node1, f, node2) and
+  pragma[only_bind_out](node1.getEnclosingCallable()) =
+    pragma[only_bind_out](node2.getEnclosingCallable())
 }
 
 /**

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

@@ -0,0 +1,42 @@
+/** Definitions related to `java.lang.ThreadLocal`. */
+
+import java
+private import semmle.code.java.dataflow.DataFlow
+private import semmle.code.java.dataflow.FlowSteps
+
+/**
+ * Holds if `cie` construct a `ThreadLocal` object with an overridden
+ * `initialValue` method with a return value of `init`, such that `init` is the
+ * initial value of the `ThreadLocal` object.
+ */
+private predicate threadLocalInitialValue(ClassInstanceExpr cie, Method initialValue, Expr init) {
+  exists(RefType t, ReturnStmt ret |
+    cie.getConstructedType().getSourceDeclaration() = t and
+    t.getASourceSupertype+().hasQualifiedName("java.lang", "ThreadLocal") and
+    ret.getResult() = init and
+    ret.getEnclosingCallable() = initialValue and
+    initialValue.hasName("initialValue") and
+    initialValue.getDeclaringType() = t
+  )
+}
+
+private class ThreadLocalInitialValueStore extends AdditionalStoreStep {
+  override predicate step(DataFlow::Node node1, DataFlow::Content c, DataFlow::Node node2) {
+    exists(Method initialValue, Expr init |
+      threadLocalInitialValue(_, initialValue, init) and
+      node1.asExpr() = init and
+      node2.(DataFlow::InstanceParameterNode).getCallable() = initialValue and
+      c.(DataFlow::SyntheticFieldContent).getField() = "java.lang.ThreadLocal.value"
+    )
+  }
+}
+
+private class ThreadLocalInitialValueStep extends AdditionalValueStep {
+  override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
+    exists(ClassInstanceExpr cie, Method initialValue |
+      threadLocalInitialValue(cie, initialValue, _) and
+      node1.(DataFlow::InstanceParameterNode).getCallable() = initialValue and
+      node2.asExpr() = cie
+    )
+  }
+}

java/ql/test/query-tests/security/CWE-611/DocumentBuilderTests.java

@@ -129,7 +129,7 @@ protected DocumentBuilder initialValue() {
 
   public void disableExternalEntities2(Socket sock) throws Exception {
     DocumentBuilder builder = XML_DOCUMENT_BUILDER.get();
-    builder.parse(sock.getInputStream()); // $ SPURIOUS: hasTaintFlow
+    builder.parse(sock.getInputStream()); // safe
   }
 
 }