Refactor to use FlowState

Remove the auxiliary DataFlow configuration

java/ql/lib/semmle/code/java/frameworks/android/Intent.qll

@@ -87,11 +87,6 @@ class AndroidBundle extends Class {
   AndroidBundle() { this.getASupertype*().hasQualifiedName("android.os", "BaseBundle") }
 }
 
-/** The class `android.app.PendingIntent`. */
-class PendingIntent extends Class {
-  PendingIntent() { this.hasQualifiedName("android.app", "PendingIntent") }
-}
-
 /** An `Intent` that explicitly sets a destination component. */
 class ExplicitIntent extends Expr {
   ExplicitIntent() {

java/ql/lib/semmle/code/java/frameworks/android/PendingIntent.qll

@@ -0,0 +1,47 @@
+/** Provides classes and predicates related to the class `PendingIntent`. */
+
+import java
+
+/** The class `android.app.PendingIntent`. */
+class PendingIntent extends Class {
+  PendingIntent() { this.hasQualifiedName("android.app", "PendingIntent") }
+}
+
+/** A call to a method that creates a `PendingIntent`. */
+class PendingIntentCreation extends MethodAccess {
+  PendingIntentCreation() {
+    exists(Method m |
+      this.getMethod() = m and
+      m.getDeclaringType() instanceof PendingIntent and
+      m.hasName([
+          "getActivity", "getActivityAsUser", "getActivities", "getActivitiesAsUser",
+          "getBroadcast", "getBroadcastAsUser", "getService", "getForegroundService"
+        ])
+    )
+  }
+
+  /** The `Intent` argument of this call. */
+  Argument getIntentArg() { result = this.getArgument(2) }
+
+  /** The flags argument of this call. */
+  Argument getFlagsArg() { result = this.getArgument(3) }
+}
+
+/** A field of the class `PendingIntent` representing a flag. */
+class PendingIntentFlag extends Field {
+  PendingIntentFlag() {
+    this.getDeclaringType() instanceof PendingIntent and
+    this.isPublic() and
+    this.getName().matches("FLAG_%")
+  }
+}
+
+/** The field `FLAG_IMMUTABLE` of the class `PendingIntent`. */
+class ImmutablePendingIntentFlag extends PendingIntentFlag {
+  ImmutablePendingIntentFlag() { this.hasName("FLAG_IMMUTABLE") }
+}
+
+/** The field `FLAG_MUTABLE` of the class `PendingIntent`. */
+class MutablePendingIntentFlag extends PendingIntentFlag {
+  MutablePendingIntentFlag() { this.hasName("FLAG_MUTABLE") }
+}

java/ql/lib/semmle/code/java/security/ImplicitPendingIntents.qll

@@ -1,23 +1,102 @@
 /** Provides classes and predicates for working with implicit `PendingIntent`s. */
 
 import java
-private import semmle.code.java.dataflow.DataFlow
 private import semmle.code.java.dataflow.ExternalFlow
 private import semmle.code.java.dataflow.FlowSteps
+private import semmle.code.java.dataflow.TaintTracking
+private import semmle.code.java.frameworks.android.Intent
+private import semmle.code.java.frameworks.android.PendingIntent
 
-private class PendingIntentCreationModels extends SinkModelCsv {
-  override predicate row(string row) {
-    row =
-      [
-        "android.app;PendingIntent;false;getActivity;;;Argument[2];pending-intent",
-        "android.app;PendingIntent;false;getActivityAsUser;;;Argument[2];pending-intent",
-        "android.app;PendingIntent;false;getActivities;;;Argument[2];pending-intent",
-        "android.app;PendingIntent;false;getActivitiesAsUser;;;Argument[2];pending-intent",
-        "android.app;PendingIntent;false;getBroadcast;;;Argument[2];pending-intent",
-        "android.app;PendingIntent;false;getBroadcastAsUser;;;Argument[2];pending-intent",
-        "android.app;PendingIntent;false;getService;;;Argument[2];pending-intent",
-        "android.app;PendingIntent;false;getForegroundService;;;Argument[2];pending-intent"
-      ]
+/** A source for an implicit `PendingIntent` flow. */
+abstract class ImplicitPendingIntentSource extends DataFlow::Node {
+  predicate hasState(DataFlow::FlowState state) { state = "" }
+}
+
+/** A sink that sends an implicit and mutable `PendingIntent` to a third party. */
+abstract class ImplicitPendingIntentSink extends DataFlow::Node {
+  predicate hasState(DataFlow::FlowState state) { state = "" }
+}
+
+/**
+ * A unit class for adding additional taint steps.
+ *
+ * Extend this class to add additional taint steps that should apply to flows related to the use
+ * of implicit `PendingIntent`s.
+ */
+class ImplicitPendingIntentAdditionalTaintStep extends Unit {
+  /**
+   * Holds if the step from `node1` to `node2` should be considered a taint
+   * step for flows related to the use of implicit `PendingIntent`s.
+   */
+  predicate step(DataFlow::Node node1, DataFlow::Node node2) { none() }
+
+  /**
+   * Holds if the step from `node1` to `node2` should be considered a taint
+   * step for flows related to the use of implicit `PendingIntent`s. This step is only applicable
+   * in `state1` and updates the flow state to `state2`.
+   */
+  predicate step(
+    DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2,
+    DataFlow::FlowState state2
+  ) {
+    none()
+  }
+}
+
+private class IntentCreationSource extends ImplicitPendingIntentSource {
+  IntentCreationSource() {
+    exists(ClassInstanceExpr cc |
+      cc.getConstructedType() instanceof TypeIntent and this.asExpr() = cc
+    )
+  }
+}
+
+private class SendPendingIntent extends ImplicitPendingIntentSink {
+  SendPendingIntent() {
+    sinkNode(this, "intent-start") and
+    // implicit intents can't be started as services since API 21
+    not exists(MethodAccess ma, Method m |
+      ma.getMethod() = m and
+      m.getDeclaringType().getASupertype*() instanceof TypeContext and
+      m.getName().matches(["start%Service%", "bindService%"]) and
+      this.asExpr() = ma.getArgument(0)
+    )
+    or
+    sinkNode(this, "pending-intent-sent")
+  }
+
+  override predicate hasState(DataFlow::FlowState state) { state = "MutablePendingIntent" }
+}
+
+private class PendingIntentAsFieldAdditionalTaintStep extends ImplicitPendingIntentAdditionalTaintStep {
+  override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
+    exists(Field f |
+      f.getType() instanceof PendingIntent and
+      node1.(DataFlow::PostUpdateNode).getPreUpdateNode() =
+        DataFlow::getFieldQualifier(f.getAnAccess().(FieldWrite)) and
+      node2.asExpr().(FieldRead).getField() = f
+    )
+  }
+}
+
+private class MutablePendingIntentFlowStep extends PendingIntentAsFieldAdditionalTaintStep {
+  override predicate step(
+    DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2,
+    DataFlow::FlowState state2
+  ) {
+    state1 = "" and
+    state2 = "MutablePendingIntent" and
+    exists(PendingIntentCreation pic, Argument flagArg |
+      node1.asExpr() = pic.getIntentArg() and
+      node2.asExpr() = pic and
+      flagArg = pic.getFlagsArg()
+    |
+      // API < 31, PendingIntents are mutable by default
+      not TaintTracking::localExprTaint(any(ImmutablePendingIntentFlag flag).getAnAccess(), flagArg)
+      or
+      // API >= 31, PendingIntents need to explicitly set mutability
+      TaintTracking::localExprTaint(any(MutablePendingIntentFlag flag).getAnAccess(), flagArg)
+    )
   }
 }
 

java/ql/lib/semmle/code/java/security/ImplicitPendingIntentsQuery.qll

@@ -13,112 +13,42 @@ import semmle.code.java.security.ImplicitPendingIntents
 class ImplicitPendingIntentStartConf extends TaintTracking::Configuration {
   ImplicitPendingIntentStartConf() { this = "ImplicitPendingIntentStartConf" }
 
-  override predicate isSource(DataFlow::Node source) {
-    source.asExpr() instanceof ImplicitPendingIntentCreation
+  override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) {
+    source.(ImplicitPendingIntentSource).hasState(state)
   }
 
-  override predicate isSink(DataFlow::Node sink) { sink instanceof SendPendingIntent }
+  override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) {
+    sink.(ImplicitPendingIntentSink).hasState(state)
+  }
 
   override predicate isSanitizer(DataFlow::Node sanitizer) {
     sanitizer instanceof ExplicitIntentSanitizer
   }
 
   override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
-    exists(Field f |
-      f.getType() instanceof PendingIntent and
-      node1.(DataFlow::PostUpdateNode).getPreUpdateNode() =
-        DataFlow::getFieldQualifier(f.getAnAccess().(FieldWrite)) and
-      node2.asExpr().(FieldRead).getField() = f
-    )
+    any(ImplicitPendingIntentAdditionalTaintStep c).step(node1, node2)
+  }
+
+  override predicate isAdditionalFlowStep(
+    DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2,
+    DataFlow::FlowState state2
+  ) {
+    any(ImplicitPendingIntentAdditionalTaintStep c).step(node1, state1, node2, state2)
   }
 
   override predicate allowImplicitRead(DataFlow::Node node, DataFlow::Content c) {
     super.allowImplicitRead(node, c)
     or
-    this.isSink(node) and
+    this.isSink(node, _) and
     allowIntentExtrasImplicitRead(node, c)
     or
     this.isAdditionalTaintStep(node, _) and
     c.(DataFlow::FieldContent).getType() instanceof PendingIntent
-  }
-}
-
-private class ImplicitPendingIntentCreation extends Expr {
-  ImplicitPendingIntentCreation() {
-    exists(Argument arg |
-      this.getType() instanceof PendingIntent and
-      exists(ImplicitPendingIntentConf conf | conf.hasFlowTo(DataFlow::exprNode(arg))) and
-      arg.getCall() = this
-    )
-  }
-}
-
-private class SendPendingIntent extends DataFlow::Node {
-  SendPendingIntent() {
-    sinkNode(this, "intent-start") and
-    // implicit intents can't be started as services since API 21
-    not exists(MethodAccess ma, Method m |
-      ma.getMethod() = m and
-      m.getDeclaringType().getASupertype*() instanceof TypeContext and
-      m.getName().matches(["start%Service%", "bindService%"]) and
-      this.asExpr() = ma.getArgument(0)
-    )
     or
-    sinkNode(this, "pending-intent-sent")
-  }
-}
-
-private class ImplicitPendingIntentConf extends DataFlow2::Configuration {
-  ImplicitPendingIntentConf() { this = "PendingIntentConf" }
-
-  override predicate isSource(DataFlow::Node source) {
-    exists(ClassInstanceExpr cc |
-      cc.getConstructedType() instanceof TypeIntent and source.asExpr() = cc
-    )
-  }
-
-  override predicate isSink(DataFlow::Node sink) { sink instanceof MutablePendingIntentSink }
-
-  override predicate isBarrier(DataFlow::Node barrier) {
-    barrier instanceof ExplicitIntentSanitizer
-  }
-
-  override predicate allowImplicitRead(DataFlow::Node node, DataFlow::Content c) {
-    // Allow implicit reads of Intent arrays for sinks like getStartActivities
-    isSink(node) and
+    // Allow implicit reads of Intent arrays for steps like getActivities
+    // or sinks like startActivities
+    (this.isSink(node, _) or this.isAdditionalFlowStep(node, _, _, _)) and
     node.getType().(Array).getElementType() instanceof TypeIntent and
     c instanceof DataFlow::ArrayContent
   }
 }
-
-private class PendingIntentSink extends DataFlow::Node {
-  PendingIntentSink() { sinkNode(this, "pending-intent") }
-}
-
-private class MutablePendingIntentSink extends PendingIntentSink {
-  MutablePendingIntentSink() {
-    exists(Argument flagArg | flagArg = this.asExpr().(Argument).getCall().getArgument(3) |
-      // API < 31, PendingIntents are mutable by default
-      not TaintTracking::localExprTaint(any(ImmutablePendingIntentFlag flag).getAnAccess(), flagArg)
-      or
-      // API >= 31, PendingIntents need to explicitly set mutability
-      TaintTracking::localExprTaint(any(MutablePendingIntentFlag flag).getAnAccess(), flagArg)
-    )
-  }
-}
-
-private class PendingIntentFlag extends Field {
-  PendingIntentFlag() {
-    this.getDeclaringType() instanceof PendingIntent and
-    this.isPublic() and
-    this.getName().matches("FLAG_%")
-  }
-}
-
-private class ImmutablePendingIntentFlag extends PendingIntentFlag {
-  ImmutablePendingIntentFlag() { this.hasName("FLAG_IMMUTABLE") }
-}
-
-private class MutablePendingIntentFlag extends PendingIntentFlag {
-  MutablePendingIntentFlag() { this.hasName("FLAG_MUTABLE") }
-}

java/ql/test/query-tests/security/CWE-927/ImplicitPendingIntentsTest.java

@@ -98,6 +98,12 @@ public class ImplicitPendingIntentsTest {
             ctx.startActivity(fwdIntent); // $hasImplicitPendingIntent
         }
 
+        {
+            Intent intent = new Intent();
+            // Testing the need of going through a PendingIntent creation (flow state)
+            ctx.startActivity(intent); // Safe
+        }
+
         {
             Intent safeIntent = new Intent(ctx, Activity.class); // Sanitizer
             PendingIntent pi = PendingIntent.getActivity(ctx, 0, safeIntent, 0);