Merge pull request github#7681 from atorralba/atorralba/improve-android-implicit-intents-query

Java: Improvements to the Android query Use of implicit PendingIntents

Author: atorralba

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

@@ -5,6 +5,36 @@ private import semmle.code.java.dataflow.DataFlow
 private import semmle.code.java.dataflow.FlowSteps
 private import semmle.code.java.dataflow.ExternalFlow
 
+/** The class `androidx.slice.SliceProvider`. */
+class SliceProvider extends Class {
+  SliceProvider() { this.hasQualifiedName("androidx.slice", "SliceProvider") }
+}
+
+/**
+ * An additional value step for modeling the lifecycle of a `SliceProvider`.
+ * It connects the `PostUpdateNode` of any update done to the provider object in
+ * `onCreateSliceProvider` to the instance parameter of `onBindSlice`.
+ */
+private class SliceProviderLifecycleStep extends AdditionalValueStep {
+  override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
+    exists(Method onCreate, Method onBind, RefType declaringClass |
+      declaringClass.getASupertype*() instanceof SliceProvider and
+      onCreate.getDeclaringType() = declaringClass and
+      onCreate.hasName("onCreateSliceProvider") and
+      onBind.getDeclaringType() = declaringClass and
+      onBind.hasName("onBindSlice")
+    |
+      node1
+          .(DataFlow::PostUpdateNode)
+          .getPreUpdateNode()
+          .(DataFlow::InstanceAccessNode)
+          .isOwnInstanceAccess() and
+      node1.getEnclosingCallable() = onCreate and
+      node2.(DataFlow::InstanceParameterNode).getEnclosingCallable() = onBind
+    )
+  }
+}
+
 private class SliceActionsInheritTaint extends DataFlow::SyntheticFieldContent,
   TaintInheritingContent {
   SliceActionsInheritTaint() { this.getField().matches("androidx.slice.Slice.action") }

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

@@ -68,21 +68,7 @@ private class SendPendingIntent extends ImplicitPendingIntentSink {
   override predicate hasState(DataFlow::FlowState state) { state = "MutablePendingIntent" }
 }
 
-/**
- * Propagates taint from any tainted object to reads from its `PendingIntent`-typed fields.
- */
-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 {
+private class MutablePendingIntentFlowStep extends ImplicitPendingIntentAdditionalTaintStep {
   override predicate step(
     DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2,
     DataFlow::FlowState state2

java/ql/src/Security/CWE/CWE-927/ImplicitPendingIntents.qhelp

@@ -5,42 +5,42 @@
 <qhelp>
 
 <overview>
-<p>A <code>PendingIntent</code> describes an action in the form of an Intent that is intended to be given and executed 
-  at a later time by another application. The Intent wrapped by a <code>PendingIntent</code> is executed on behalf of 
-  the application that created it, and with its same privileges.</p>
-<p>If a <code>PendingIntent</code> is configured to be mutable, the fields of its internal Intent can be changed by the
-  receiving application if they were not previously set. This means that a mutable <code>PendingIntent</code> that has 
-  not defined a destination component (that is, an implicit <code>PendingIntent</code>) can be altered to execute an 
+<p>A <code>PendingIntent</code> is used to wrap an <code>Intent</code> that will be supplied and executed by another
+  application. When the <code>Intent</code> is executed, it behaves as if it were run directly by the supplying
+  application, using the privileges of that application.</p>
+<p>If a <code>PendingIntent</code> is configured to be mutable, the fields of its internal <code>Intent</code> can be changed by the
+  receiving application if they were not previously set. This means that a mutable <code>PendingIntent</code> that has
+  not defined a destination component (that is, an implicit <code>PendingIntent</code>) can be altered to execute an
   arbitrary action with the privileges of the application that created it.</p>
-<p>If an implicit <code>PendingIntent</code> is obtainable by a malicious application by any of the following means:</p>
+<p>A malicious application can access an implicit <code>PendingIntent</code> as follows:</p>
 <ul>
-  <li>It is wrapped and sent as an extra of another implicit Intent</li>
-  <li>It is sent as the action of a Slide</li>
-  <li>It is sent as the action of a Notification</li>
+  <li>It is wrapped and sent as an extra of another implicit <code>Intent</code>.</li>
+  <li>It is sent as the action of a <code>Slide</code>.</li>
+  <li>It is sent as the action of a <code>Notification</code>.</li>
 </ul>
 <p></p>
-<p>the attacker could modify the underlying Intent and execute an arbitrary action with elevated privileges. 
-  This could give the malicious application access to private components of the victim application, 
-  or the ability to perform actions without having the necessary permissions.</p>
+<p>On gaining access, the attacker can modify the underlying <code>Intent</code> and execute an arbitrary action
+  with elevated privileges. This could give the malicious application access to private components of the victim
+  application, or the ability to perform actions without having the necessary permissions.</p>
 </overview>
 
 <recommendation>
-<p>Avoid creating implicit <code>PendingIntent</code>s. This means that the underlying Intent should always have an 
+<p>Avoid creating implicit <code>PendingIntent</code>s. This means that the underlying <code>Intent</code> should always have an 
   explicit destination component.</p>
-<p>Also, when adding the <code>PendingIntent</code> as an extra of another Intent, make sure that said Intent also has
+<p>When you add the <code>PendingIntent</code> as an extra of another <code>Intent</code>, make sure that this second <code>Intent</code> also has
 an explicit destination component, so that it is not delivered to untrusted applications.</p>
-<p>It is also recommended to create the <code>PendingIntent</code> using the flag <code>FLAG_IMMUTABLE</code> whenever 
-  possible, to prevent the destination component from modifying empty fields of the underlying Intent.</p>
+<p>Create the <code>PendingIntent</code> using the flag <code>FLAG_IMMUTABLE</code> whenever possible,
+  to prevent the destination component from modifying empty fields of the underlying <code>Intent</code>.</p>
 </recommendation>
 
 <example>
-  <p>In the following examples, a <code>PendingIntent</code> is created and wrapped as an extra of another Intent.
+  <p>In the following examples, a <code>PendingIntent</code> is created and wrapped as an extra of another <code>Intent</code>.
   </p>
-  <p>In the first example, both the <code>PendingIntent</code> and the Intent it is wrapped in are implicit,
-    reproducing the vulnerability.</p>
+  <p>In the first example, both the <code>PendingIntent</code> and the <code>Intent</code> it is wrapped in are implicit,
+    making them vulnerable to attack.</p>
   <p>In the second example, the issue is avoided by adding explicit destination components to the 
-    <code>PendingIntent</code> and the wrapping Intent.</p>
-  <p>The third example uses the <code>FLAG_IMMUTABLE</code> flag to prevent the underlying Intent from being modified
+    <code>PendingIntent</code> and the wrapping <code>Intent</code>.</p>
+  <p>The third example uses the <code>FLAG_IMMUTABLE</code> flag to prevent the underlying <code>Intent</code> from being modified
     by the destination component.</p>
 <sample src="ImplicitPendingIntents.java" />
 </example>

java/ql/src/Security/CWE/CWE-927/ImplicitPendingIntents.ql

@@ -1,8 +1,8 @@
 /**
  * @name Use of implicit PendingIntents
- * @description Implicit and mutable PendingIntents being sent to an unspecified third party
- *              component may provide access to internal components of the application or cause
- *              other unintended effects.
+ * @description Sending an implicit and mutable 'PendingIntent' to an unspecified third party
+ *              component may provide an attacker with access to internal components of the
+ *              application or cause other unintended effects.
  * @kind path-problem
  * @problem.severity error
  * @security-severity 8.2

java/ql/src/change-notes/2021-12-21-android-implicit-pendingintents.md

@@ -2,6 +2,6 @@
 category: newQuery
 ---
 * A new query "Use of implicit PendingIntents" (`java/android/pending-intents`) has been added.
-This query finds implicit and mutable `PendingIntents` being sent to an unspecified third party component,
-which can provide access to internal components of the application or cause other unintended
-effects.
+This query finds implicit and mutable `PendingIntents` sent to an unspecified third party
+component, which may provide an attacker with access to internal components of the application
+or cause other unintended effects.