Merge pull request #1503 from aschackmull/java/object-tostring-dispatch

Java: Restrict Object.toString() dispatch based on a more closed-world assumption.

Author: yh-semmle

change-notes/1.22/analysis-java.md

@@ -8,3 +8,8 @@
 
 ## Changes to QL libraries
 
+* The virtual dispatch library has been updated to give more precise dispatch
+  targets for `Object.toString()` calls. This affects all security queries and
+  removes false positives that arose from paths through impossible `toString()`
+  calls.
+

java/ql/src/Likely Bugs/Serialization/NonSerializableField.ql

@@ -34,6 +34,7 @@ predicate serializableType(RefType t) {
   // Collection interfaces are not serializable, but their implementations are
   // likely to be.
   collectionOrMapType(t) and
+  not t instanceof RawType and
   forall(RefType param | param = t.(ParameterizedType).getATypeArgument() | serializableType(param))
   or
   exists(BoundedType bt | bt = t | serializableType(bt.getUpperBoundType()))

java/ql/src/semmle/code/java/Collections.qll

@@ -10,12 +10,11 @@ import java
  */
 predicate instantiates(RefType t, GenericType g, int i, RefType arg) {
   t = g.getAParameterizedType() and
+  arg = t.(ParameterizedType).getTypeArgument(i)
+  or
+  t = g.getRawType() and
   exists(g.getTypeParameter(i)) and
-  (
-    arg = t.(ParameterizedType).getTypeArgument(i)
-    or
-    t instanceof RawType and arg instanceof TypeObject
-  )
+  arg instanceof TypeObject
 }
 
 /**
@@ -32,42 +31,18 @@ predicate instantiates(RefType t, GenericType g, int i, RefType arg) {
  *   with the `0`-th type parameter being `Integer` and the `1`-th type parameter being `V`.
  */
 predicate indirectlyInstantiates(RefType t, GenericType g, int i, RefType arg) {
-  exists(RefType tsrc | tsrc = t.getSourceDeclaration() |
-    // base case: `t` directly instantiates `g`
-    tsrc = g and instantiates(t, g, i, arg)
-    or
-    // inductive step
-    exists(RefType sup, RefType suparg |
-      // follow `extends`/`implements`
-      (extendsReftype(tsrc, sup) or implInterface(tsrc, sup)) and
-      // check whether the subtype instantiates `g`
-      indirectlyInstantiates(sup, g, i, suparg)
-    |
-      // if `t` is itself an instantiation of `tsrc` and `sup` instantiates
-      // `g` to one of the type parameters of `tsrc`, we return the corresponding
-      // instantiation in `t`
-      exists(int j | suparg = tsrc.(GenericType).getTypeParameter(j) |
-        instantiates(t, tsrc, j, arg)
-      )
-      or
-      // otherwise, we directly return `suparg`
-      not (
-        t = tsrc.(GenericType).getAParameterizedType() and
-        suparg = tsrc.(GenericType).getATypeParameter()
-      ) and
-      arg = suparg
-    )
+  instantiates(t, g, i, arg)
+  or
+  exists(RefType sup |
+    t.extendsOrImplements(sup) and
+    indirectlyInstantiates(sup, g, i, arg)
   )
 }
 
 /** A reference type that extends a parameterization of `java.util.Collection`. */
 class CollectionType extends RefType {
   CollectionType() {
-    exists(ParameterizedInterface coll |
-      coll.getSourceDeclaration().hasQualifiedName("java.util", "Collection")
-    |
-      this.hasSupertype*(coll)
-    )
+    this.getSourceDeclaration().getASourceSupertype*().hasQualifiedName("java.util", "Collection")
   }
 
   /** Gets the type of elements stored in this collection. */

java/ql/src/semmle/code/java/Maps.qll

@@ -4,11 +4,7 @@ import Collections
 /** A reference type that extends a parameterization of `java.util.Map`. */
 class MapType extends RefType {
   MapType() {
-    exists(ParameterizedInterface coll |
-      coll.getSourceDeclaration().hasQualifiedName("java.util", "Map")
-    |
-      this.hasSupertype*(coll)
-    )
+    this.getSourceDeclaration().getASourceSupertype*().hasQualifiedName("java.util", "Map")
   }
 
   /** Gets the type of keys stored in this map. */

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

@@ -15,6 +15,7 @@ private import semmle.code.java.frameworks.android.Intent
 private import semmle.code.java.frameworks.Guice
 private import semmle.code.java.frameworks.Protobuf
 private import semmle.code.java.Maps
+private import semmle.code.java.dataflow.internal.ContainerFlow
 
 module TaintTracking {
   /**
@@ -218,6 +219,8 @@ module TaintTracking {
       v.getAFirstUse() = sink
     )
     or
+    containerStep(src, sink)
+    or
     constructorStep(src, sink)
     or
     qualifierToMethodStep(src, sink)
@@ -353,10 +356,6 @@ module TaintTracking {
 
   /** Methods that passes tainted data from qualifier to argument. */
   private predicate taintPreservingQualifierToArgument(Method m, int arg) {
-    m instanceof CollectionMethod and
-    m.hasName("toArray") and
-    arg = 1
-    or
     m.getDeclaringType().hasQualifiedName("java.io", "ByteArrayOutputStream") and
     m.hasName("writeTo") and
     arg = 0
@@ -427,50 +426,6 @@ module TaintTracking {
     or
     m instanceof IntentGetExtraMethod
     or
-    m
-        .getDeclaringType()
-        .getSourceDeclaration()
-        .getASourceSupertype*()
-        .hasQualifiedName("java.util", "Map<>$Entry") and
-    m.hasName("getValue")
-    or
-    m
-        .getDeclaringType()
-        .getSourceDeclaration()
-        .getASourceSupertype*()
-        .hasQualifiedName("java.lang", "Iterable") and
-    m.hasName("iterator")
-    or
-    m
-        .getDeclaringType()
-        .getSourceDeclaration()
-        .getASourceSupertype*()
-        .hasQualifiedName("java.util", "Iterator") and
-    m.hasName("next")
-    or
-    m.getDeclaringType().getSourceDeclaration().hasQualifiedName("java.util", "Enumeration") and
-    m.hasName("nextElement")
-    or
-    m.(MapMethod).hasName("entrySet")
-    or
-    m.(MapMethod).hasName("get")
-    or
-    m.(MapMethod).hasName("remove")
-    or
-    m.(MapMethod).hasName("values")
-    or
-    m.(CollectionMethod).hasName("toArray")
-    or
-    m.(CollectionMethod).hasName("get")
-    or
-    m.(CollectionMethod).hasName("remove") and m.getParameterType(0).(PrimitiveType).hasName("int")
-    or
-    m.(CollectionMethod).hasName("subList")
-    or
-    m.(CollectionMethod).hasName("firstElement")
-    or
-    m.(CollectionMethod).hasName("lastElement")
-    or
     m.getDeclaringType().hasQualifiedName("java.nio", "ByteBuffer") and
     m.hasName("get")
     or
@@ -656,18 +611,6 @@ module TaintTracking {
     method.getDeclaringType().hasQualifiedName("java.io", "ByteArrayOutputStream") and
     method.hasName("write") and
     arg = 0
-    or
-    method.(MapMethod).hasName("put") and arg = 1
-    or
-    method.(MapMethod).hasName("putAll") and arg = 0
-    or
-    method.(CollectionMethod).hasName("add") and arg = method.getNumberOfParameters() - 1
-    or
-    method.(CollectionMethod).hasName("addAll") and arg = method.getNumberOfParameters() - 1
-    or
-    method.(CollectionMethod).hasName("addElement") and arg = 0
-    or
-    method.(CollectionMethod).hasName("set") and arg = 1
   }
 
   /** A comparison or equality test with a constant. */

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

@@ -298,9 +298,139 @@ private module SsaImpl {
       )
     }
   }
+
+  private module AdjacentUsesImpl {
+    /**
+     * Holds if `rankix` is the rank the index `i` at which there is an SSA definition or explicit use of
+     * `v` in the basic block `b`.
+     */
+    private predicate defUseRank(BaseSsaSourceVariable v, BasicBlock b, int rankix, int i) {
+      i = rank[rankix](int j | any(TrackedSsaDef def).definesAt(v, b, j) or variableUse(v, _, b, j))
+    }
+
+    /** Gets the maximum rank index for the given variable and basic block. */
+    private int lastRank(BaseSsaSourceVariable v, BasicBlock b) {
+      result = max(int rankix | defUseRank(v, b, rankix, _))
+    }
+
+    /** Holds if `v` is defined or used in `b`. */
+    private predicate varOccursInBlock(BaseSsaSourceVariable v, BasicBlock b) {
+      defUseRank(v, b, _, _)
+    }
+
+    /** Holds if `v` occurs in `b` or one of `b`'s transitive successors. */
+    private predicate blockPrecedesVar(BaseSsaSourceVariable v, BasicBlock b) {
+      varOccursInBlock(v, b.getABBSuccessor*())
+    }
+
+    /**
+     * Holds if `b2` is a transitive successor of `b1` and `v` occurs in `b1` and
+     * in `b2` or one of its transitive successors but not in any block on the path
+     * between `b1` and `b2`.
+     */
+    private predicate varBlockReaches(BaseSsaSourceVariable v, BasicBlock b1, BasicBlock b2) {
+      varOccursInBlock(v, b1) and b2 = b1.getABBSuccessor()
+      or
+      exists(BasicBlock mid |
+        varBlockReaches(v, b1, mid) and
+        b2 = mid.getABBSuccessor() and
+        not varOccursInBlock(v, mid) and
+        blockPrecedesVar(v, b2)
+      )
+    }
+
+    /**
+     * Holds if `b2` is a transitive successor of `b1` and `v` occurs in `b1` and
+     * `b2` but not in any block on the path between `b1` and `b2`.
+     */
+    private predicate varBlockStep(BaseSsaSourceVariable v, BasicBlock b1, BasicBlock b2) {
+      varBlockReaches(v, b1, b2) and
+      varOccursInBlock(v, b2)
+    }
+
+    /**
+     * Holds if `v` occurs at index `i1` in `b1` and at index `i2` in `b2` and
+     * there is a path between them without any occurrence of `v`.
+     */
+    predicate adjacentVarRefs(BaseSsaSourceVariable v, BasicBlock b1, int i1, BasicBlock b2, int i2) {
+      exists(int rankix |
+        b1 = b2 and
+        defUseRank(v, b1, rankix, i1) and
+        defUseRank(v, b2, rankix + 1, i2)
+      )
+      or
+      defUseRank(v, b1, lastRank(v, b1), i1) and
+      varBlockStep(v, b1, b2) and
+      defUseRank(v, b2, 1, i2)
+    }
+  }
+  private import AdjacentUsesImpl
+
+  /**
+   * Holds if the value defined at `def` can reach `use` without passing through
+   * any other uses, but possibly through phi nodes.
+   */
+  cached
+  predicate firstUse(TrackedSsaDef def, RValue use) {
+    exists(BaseSsaSourceVariable v, BasicBlock b1, int i1, BasicBlock b2, int i2 |
+      adjacentVarRefs(v, b1, i1, b2, i2) and
+      def.definesAt(v, b1, i1) and
+      variableUse(v, use, b2, i2)
+    )
+    or
+    exists(
+      BaseSsaSourceVariable v, TrackedSsaDef redef, BasicBlock b1, int i1, BasicBlock b2, int i2
+    |
+      redef instanceof BaseSsaPhiNode
+    |
+      adjacentVarRefs(v, b1, i1, b2, i2) and
+      def.definesAt(v, b1, i1) and
+      redef.definesAt(v, b2, i2) and
+      firstUse(redef, use)
+    )
+  }
+
+  cached
+  module SsaPublic {
+    /**
+     * Holds if `use1` and `use2` form an adjacent use-use-pair of the same SSA
+     * variable, that is, the value read in `use1` can reach `use2` without passing
+     * through any other use or any SSA definition of the variable.
+     */
+    cached
+    predicate baseSsaAdjacentUseUseSameVar(RValue use1, RValue use2) {
+      exists(BaseSsaSourceVariable v, BasicBlock b1, int i1, BasicBlock b2, int i2 |
+        adjacentVarRefs(v, b1, i1, b2, i2) and
+        variableUse(v, use1, b1, i1) and
+        variableUse(v, use2, b2, i2)
+      )
+    }
+
+    /**
+     * Holds if `use1` and `use2` form an adjacent use-use-pair of the same
+     * `SsaSourceVariable`, that is, the value read in `use1` can reach `use2`
+     * without passing through any other use or any SSA definition of the variable
+     * except for phi nodes.
+     */
+    cached
+    predicate baseSsaAdjacentUseUse(RValue use1, RValue use2) {
+      baseSsaAdjacentUseUseSameVar(use1, use2)
+      or
+      exists(
+        BaseSsaSourceVariable v, TrackedSsaDef def, BasicBlock b1, int i1, BasicBlock b2, int i2
+      |
+        adjacentVarRefs(v, b1, i1, b2, i2) and
+        variableUse(v, use1, b1, i1) and
+        def.definesAt(v, b2, i2) and
+        firstUse(def, use2) and
+        def instanceof BaseSsaPhiNode
+      )
+    }
+  }
 }
 private import SsaImpl
 private import SsaDefReaches
+import SsaPublic
 
 private newtype TBaseSsaVariable =
   TSsaPhiNode(BaseSsaSourceVariable v, BasicBlock b) { phiNode(v, b) } or
@@ -354,6 +484,16 @@ class BaseSsaVariable extends TBaseSsaVariable {
   /** Gets an access of this SSA variable. */
   RValue getAUse() { ssaDefReachesUse(_, this, result) }
 
+  /**
+   * Gets an access of the SSA source variable underlying this SSA variable
+   * that can be reached from this SSA variable without passing through any
+   * other uses, but potentially through phi nodes.
+   *
+   * Subsequent uses can be found by following the steps defined by
+   * `baseSsaAdjacentUseUse`.
+   */
+  RValue getAFirstUse() { firstUse(this, result) }
+
   /** Holds if this SSA variable is live at the end of `b`. */
   predicate isLiveAtEndOfBlock(BasicBlock b) { ssaDefReachesEndOfBlock(_, this, b) }