Merge pull request github#5751 from aschackmull/java/collection-flow

Java: Convert all collection and array steps from taint flow to value flow.

Author: aschackmull

java/change-notes/2021-06-01-collection-flow.md

@@ -0,0 +1,5 @@
+lgtm,codescanning
+* Data flow now tracks steps through collections and arrays more precisely.
+  That means that collection and array read steps are now matched up with
+  preceding store steps. This results in increased precision for all flow-based
+  queries, in particular most of the security queries.

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

@@ -75,6 +75,7 @@ private import FlowSummary
  * ensuring that they are visible to the taint tracking / data flow library.
  */
 private module Frameworks {
+  private import internal.ContainerFlow
   private import semmle.code.java.frameworks.ApacheHttp
   private import semmle.code.java.frameworks.apache.Lang
   private import semmle.code.java.frameworks.guava.Guava
@@ -482,7 +483,7 @@ module CsvValidation {
       not namespace.regexpMatch("[a-zA-Z0-9_\\.]+") and
       msg = "Dubious namespace \"" + namespace + "\" in " + pred + " model."
       or
-      not type.regexpMatch("[a-zA-Z0-9_\\$]+") and
+      not type.regexpMatch("[a-zA-Z0-9_\\$<>]+") and
       msg = "Dubious type \"" + type + "\" in " + pred + " model."
       or
       not name.regexpMatch("[a-zA-Z0-9_]*") and
@@ -566,7 +567,7 @@ private RefType interpretType(string namespace, string type, boolean subtypes) {
 private string paramsStringPart(Callable c, int i) {
   i = -1 and result = "("
   or
-  exists(int n, string p | c.getParameterType(n).toString() = p |
+  exists(int n, string p | c.getParameterType(n).getErasure().toString() = p |
     i = 2 * n and result = p
     or
     i = 2 * n - 1 and result = "," and n != 0

java/ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll

@@ -4,6 +4,7 @@ private import DataFlowImplCommon
 private import DataFlowDispatch
 private import semmle.code.java.controlflow.Guards
 private import semmle.code.java.dataflow.SSA
+private import ContainerFlow
 private import FlowSummaryImpl as FlowSummaryImpl
 import DataFlowNodes::Private
 
@@ -137,13 +138,15 @@ class MapValueContent extends Content, TMapValueContent {
  * Thus, `node2` references an object with a field `f` that contains the
  * value of `node1`.
  */
-predicate storeStep(Node node1, Content f, PostUpdateNode node2) {
+predicate storeStep(Node node1, Content f, Node node2) {
   exists(FieldAccess fa |
     instanceFieldAssign(node1.asExpr(), fa) and
-    node2.getPreUpdateNode() = getFieldQualifier(fa) and
+    node2.(PostUpdateNode).getPreUpdateNode() = getFieldQualifier(fa) and
     f.(FieldContent).getField() = fa.getField()
   )
   or
+  f instanceof ArrayContent and arrayStoreStep(node1, node2)
+  or
   FlowSummaryImpl::Private::Steps::summaryStoreStep(node1, f, node2)
 }
 
@@ -171,6 +174,10 @@ predicate readStep(Node node1, Content f, Node node2) {
     node2.asExpr() = get
   )
   or
+  f instanceof ArrayContent and arrayReadStep(node1, node2, _)
+  or
+  f instanceof CollectionContent and collectionReadStep(node1, node2)
+  or
   FlowSummaryImpl::Private::Steps::summaryReadStep(node1, f, node2)
 }
 

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

@@ -144,6 +144,8 @@ predicate simpleLocalFlowStep(Node node1, Node node2) {
   or
   node2.asExpr().(AssignExpr).getSource() = node1.asExpr()
   or
+  node2.asExpr().(ArrayCreationExpr).getInit() = node1.asExpr()
+  or
   exists(MethodAccess ma, ValuePreservingMethod m, int argNo |
     ma.getCallee().getSourceDeclaration() = m and m.returnsValue(argNo)
   |

java/ql/src/semmle/code/java/dataflow/internal/DataFlowUtil.qll

@@ -31,6 +31,12 @@ DataFlowType getContentType(Content c) {
   or
   c instanceof ArrayContent and
   result instanceof TypeObject
+  or
+  c instanceof MapKeyContent and
+  result instanceof TypeObject
+  or
+  c instanceof MapValueContent and
+  result instanceof TypeObject
 }
 
 /** Gets the return type of kind `rk` for callable `c`. */

java/ql/src/semmle/code/java/dataflow/internal/FlowSummaryImplSpecific.qll

@@ -60,10 +60,15 @@ private module Cached {
     localAdditionalTaintUpdateStep(src.asExpr(),
       sink.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr())
     or
-    exists(Argument arg |
-      src.asExpr() = arg and
-      arg.isVararg() and
-      sink.(DataFlow::ImplicitVarargsArray).getCall() = arg.getCall()
+    exists(Content f |
+      readStep(src, f, sink) and
+      not sink.getTypeBound() instanceof PrimitiveType and
+      not sink.getTypeBound() instanceof BoxedType and
+      not sink.getTypeBound() instanceof NumberType
+    |
+      f instanceof ArrayContent or
+      f instanceof CollectionContent or
+      f instanceof MapValueContent
     )
     or
     FlowSummaryImpl::Private::Steps::summaryLocalStep(src, sink, false)
@@ -93,6 +98,92 @@ private module Cached {
 
 import Cached
 
+/**
+ * These configurations add a number of configuration-dependent additional taint
+ * steps to all taint configurations. For each sink or additional step provided
+ * by a given configuration the types are inspected to find those implicit
+ * collection or array read steps that might be required at the sink or step
+ * input. The corresponding store steps are then added as additional taint steps
+ * to provide backwards-compatible taint flow to such sinks and steps.
+ *
+ * This is a temporary measure until support is added for such sinks that
+ * require implicit read steps.
+ */
+private module StoreTaintSteps {
+  private import semmle.code.java.dataflow.TaintTracking
+  private import semmle.code.java.dataflow.TaintTracking2
+
+  private class StoreTaintConfig extends TaintTracking::Configuration {
+    StoreTaintConfig() { this instanceof TaintTracking::Configuration or none() }
+
+    override predicate isSource(DataFlow::Node n) { none() }
+
+    override predicate isSink(DataFlow::Node n) { none() }
+
+    private predicate needsTaintStore(RefType container, Type elem, Content f) {
+      exists(DataFlow::Node arg |
+        (isSink(arg) or isAdditionalTaintStep(arg, _)) and
+        (arg.asExpr() instanceof Argument or arg instanceof ArgumentNode) and
+        arg.getType() = container
+        or
+        needsTaintStore(_, container, _)
+      |
+        container.(Array).getComponentType() = elem and
+        f instanceof ArrayContent
+        or
+        container.(CollectionType).getElementType() = elem and
+        f instanceof CollectionContent
+        or
+        container.(MapType).getValueType() = elem and
+        f instanceof MapValueContent
+      )
+    }
+
+    override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
+      exists(Content f, Type elem |
+        storeStep(node1, f, node2) and
+        needsTaintStore(_, elem, f) and
+        not exists(Type srctyp | srctyp = node1.getTypeBound() | not compatibleTypes(srctyp, elem))
+      )
+    }
+  }
+
+  private class StoreTaintConfig2 extends TaintTracking2::Configuration {
+    StoreTaintConfig2() { this instanceof TaintTracking2::Configuration or none() }
+
+    override predicate isSource(DataFlow::Node n) { none() }
+
+    override predicate isSink(DataFlow::Node n) { none() }
+
+    private predicate needsTaintStore(RefType container, Type elem, Content f) {
+      exists(DataFlow::Node arg |
+        (isSink(arg) or isAdditionalTaintStep(arg, _)) and
+        (arg.asExpr() instanceof Argument or arg instanceof ArgumentNode) and
+        arg.getType() = container
+        or
+        needsTaintStore(_, container, _)
+      |
+        container.(Array).getComponentType() = elem and
+        f instanceof ArrayContent
+        or
+        container.(CollectionType).getElementType() = elem and
+        f instanceof CollectionContent
+        or
+        container.(MapType).getValueType() = elem and
+        f instanceof MapValueContent
+      )
+    }
+
+    override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
+      exists(Content f, Type elem |
+        storeStep(node1, f, node2) and
+        needsTaintStore(_, elem, f) and
+        not exists(Type srctyp | srctyp = node1.getTypeBound() | not compatibleTypes(srctyp, elem))
+      )
+    }
+  }
+}
+
 /**
  * Holds if taint can flow in one local step from `src` to `sink` excluding
  * local data flow steps. That is, `src` and `sink` are likely to represent
@@ -103,22 +194,8 @@ private predicate localAdditionalTaintExprStep(Expr src, Expr sink) {
   or
   sink.(AssignAddExpr).getSource() = src and sink.getType() instanceof TypeString
   or
-  sink.(ArrayCreationExpr).getInit() = src
-  or
-  sink.(ArrayInit).getAnInit() = src
-  or
-  sink.(ArrayAccess).getArray() = src
-  or
   sink.(LogicExpr).getAnOperand() = src
   or
-  exists(EnhancedForStmt for, SsaExplicitUpdate v |
-    for.getExpr() = src and
-    v.getDefiningExpr() = for.getVariable() and
-    v.getAFirstUse() = sink
-  )
-  or
-  containerReturnValueStep(src, sink)
-  or
   constructorStep(src, sink)
   or
   qualifierToMethodStep(src, sink)
@@ -141,12 +218,6 @@ private predicate localAdditionalTaintExprStep(Expr src, Expr sink) {
  * This is restricted to cases where the step updates the value of `sink`.
  */
 private predicate localAdditionalTaintUpdateStep(Expr src, Expr sink) {
-  exists(Assignment assign | assign.getSource() = src |
-    sink = assign.getDest().(ArrayAccess).getArray()
-  )
-  or
-  containerUpdateStep(src, sink)
-  or
   qualifierToArgumentStep(src, sink)
   or
   argToArgStep(src, sink)

java/ql/src/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll

@@ -165,14 +165,17 @@ private class ApacheHttpFlowStep extends SummaryModelCsv {
         "org.apache.http.util;EncodingUtils;true;getAsciiString;;;Argument[0];ReturnValue;taint",
         "org.apache.http.util;EncodingUtils;true;getBytes;(String,String);;Argument[0];ReturnValue;taint",
         "org.apache.http.util;EncodingUtils;true;getString;;;Argument[0];ReturnValue;taint",
-        "org.apache.http.util;Args;true;containsNoBlanks;(T,String);;Argument[0];ReturnValue;value",
-        "org.apache.http.util;Args;true;notNull;(T,String);;Argument[0];ReturnValue;value",
-        "org.apache.http.util;Args;true;notEmpty;(T,String);;Argument[0];ReturnValue;value",
-        "org.apache.http.util;Args;true;notBlank;(T,String);;Argument[0];ReturnValue;value",
-        "org.apache.hc.core5.util;Args;true;containsNoBlanks;(T,String);;Argument[0];ReturnValue;value",
-        "org.apache.hc.core5.util;Args;true;notNull;(T,String);;Argument[0];ReturnValue;value",
-        "org.apache.hc.core5.util;Args;true;notEmpty;(T,String);;Argument[0];ReturnValue;value",
-        "org.apache.hc.core5.util;Args;true;notBlank;(T,String);;Argument[0];ReturnValue;value",
+        "org.apache.http.util;Args;true;containsNoBlanks;(CharSequence,String);;Argument[0];ReturnValue;value",
+        "org.apache.http.util;Args;true;notNull;(Object,String);;Argument[0];ReturnValue;value",
+        "org.apache.http.util;Args;true;notEmpty;(CharSequence,String);;Argument[0];ReturnValue;value",
+        "org.apache.http.util;Args;true;notEmpty;(Collection,String);;Argument[0];ReturnValue;value",
+        "org.apache.http.util;Args;true;notBlank;(CharSequence,String);;Argument[0];ReturnValue;value",
+        "org.apache.hc.core5.util;Args;true;containsNoBlanks;(CharSequence,String);;Argument[0];ReturnValue;value",
+        "org.apache.hc.core5.util;Args;true;notNull;(Object,String);;Argument[0];ReturnValue;value",
+        "org.apache.hc.core5.util;Args;true;notEmpty;(Collection,String);;Argument[0];ReturnValue;value",
+        "org.apache.hc.core5.util;Args;true;notEmpty;(CharSequence,String);;Argument[0];ReturnValue;value",
+        "org.apache.hc.core5.util;Args;true;notEmpty;(Object,String);;Argument[0];ReturnValue;value",
+        "org.apache.hc.core5.util;Args;true;notBlank;(CharSequence,String);;Argument[0];ReturnValue;value",
         "org.apache.hc.core5.http.io.entity;HttpEntities;true;create;;;Argument[0];ReturnValue;taint",
         "org.apache.hc.core5.http.io.entity;HttpEntities;true;createGzipped;;;Argument[0];ReturnValue;taint",
         "org.apache.hc.core5.http.io.entity;HttpEntities;true;createUrlEncoded;;;Argument[0];ReturnValue;taint",