Merge pull request #8669 from joefarebrother/intent-verification

Java: Add query for Improper Verification of Intent by Broadcast Receiver (CWE-925)

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

@@ -0,0 +1,79 @@
+/** Definitions for the improper intent verification query. */
+
+import java
+import semmle.code.java.dataflow.DataFlow
+import semmle.code.xml.AndroidManifest
+import semmle.code.java.frameworks.android.Intent
+
+/** An `onReceive` method of a `BroadcastReceiver` */
+private class OnReceiveMethod extends Method {
+  OnReceiveMethod() { this.getASourceOverriddenMethod*() instanceof AndroidReceiveIntentMethod }
+
+  /** Gets the parameter of this method that holds the received `Intent`. */
+  Parameter getIntentParameter() { result = this.getParameter(1) }
+}
+
+/** A configuration to detect whether the `action` of an `Intent` is checked. */
+private class VerifiedIntentConfig extends DataFlow::Configuration {
+  VerifiedIntentConfig() { this = "VerifiedIntentConfig" }
+
+  override predicate isSource(DataFlow::Node src) {
+    src.asParameter() = any(OnReceiveMethod orm).getIntentParameter()
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(MethodAccess ma |
+      ma.getMethod().hasQualifiedName("android.content", "Intent", "getAction") and
+      sink.asExpr() = ma.getQualifier()
+    )
+  }
+}
+
+/** An `onReceive` method that doesn't verify the action of the intent it receives. */
+private class UnverifiedOnReceiveMethod extends OnReceiveMethod {
+  UnverifiedOnReceiveMethod() {
+    not any(VerifiedIntentConfig c).hasFlow(DataFlow::parameterNode(this.getIntentParameter()), _)
+  }
+}
+
+/** Gets the name of an intent action that can only be sent by the system. */
+string getASystemActionName() {
+  result =
+    [
+      "AIRPLANE_MODE", "AIRPLANE_MODE_CHANGED", "APPLICATION_LOCALE_CHANGED",
+      "APPLICATION_RESTRICTIONS_CHANGED", "BATTERY_CHANGED", "BATTERY_LOW", "BATTERY_OKAY",
+      "BOOT_COMPLETED", "CONFIGURATION_CHANGED", "DEVICE_STORAGE_LOW", "DEVICE_STORAGE_OK",
+      "DREAMING_STARTED", "DREAMING_STOPPED", "EXTERNAL_APPLICATIONS_AVAILABLE",
+      "EXTERNAL_APPLICATIONS_UNAVAILABLE", "LOCALE_CHANGED", "LOCKED_BOOT_COMPLETED",
+      "MY_PACKAGE_REPLACED", "MY_PACKAGE_SUSPENDED", "MY_PACKAGE_UNSUSPENDED", "NEW_OUTGOING_CALL",
+      "PACKAGES_SUSPENDED", "PACKAGES_UNSUSPENDED", "PACKAGE_ADDED", "PACKAGE_CHANGED",
+      "PACKAGE_DATA_CLEARED", "PACKAGE_FIRST_LAUNCH", "PACKAGE_FULLY_REMOVED", "PACKAGE_INSTALL",
+      "PACKAGE_NEEDS_VERIFICATION", "PACKAGE_REMOVED", "PACKAGE_REPLACED", "PACKAGE_RESTARTED",
+      "PACKAGE_VERIFIED", "POWER_CONNECTED", "POWER_DISCONNECTED", "REBOOT", "SCREEN_OFF",
+      "SCREEN_ON", "SHUTDOWN", "TIMEZONE_CHANGED", "TIME_TICK", "UID_REMOVED", "USER_PRESENT"
+    ]
+}
+
+/** An expression or XML attribute that contains the name of a system intent action. */
+class SystemActionName extends AndroidActionXmlElement {
+  string name;
+
+  SystemActionName() {
+    name = getASystemActionName() and
+    this.getActionName() = "android.intent.action." + name
+  }
+
+  /** Gets the name of the system intent that this expression or attribute represents. */
+  string getSystemActionName() { result = name }
+}
+
+/** Holds if the XML element `rec` declares a receiver `orm` to receive the system action named `sa` that doesn't verify intents it receives. */
+predicate unverifiedSystemReceiver(
+  AndroidReceiverXmlElement rec, UnverifiedOnReceiveMethod orm, SystemActionName sa
+) {
+  exists(Class ormty |
+    ormty = orm.getDeclaringType() and
+    rec.getComponentName() = ["." + ormty.getName(), ormty.getQualifiedName()] and
+    rec.getAnIntentFilterElement().getAnActionElement() = sa
+  )
+}

java/ql/src/Security/CWE/CWE-925/AndroidManifest.xml

@@ -0,0 +1,9 @@
+<manifest xmlns:android="http://schemas.android.com/apk/res/android" package="test">
+    <application>
+        <receiver android:name=".BootReceiverXml">
+            <intent-filter>
+                <action android:name="android.intent.action.BOOT_COMPLETED" />
+            </intent-filter>
+        </receiver>
+    </application>
+</manifest>

java/ql/src/Security/CWE/CWE-925/Bad.java

@@ -0,0 +1,7 @@
+public class ShutdownReceiver extends BroadcastReceiver {
+    @Override
+    public void onReceive(final Context context, final Intent intent) {
+        mainActivity.saveLocalData();
+        mainActivity.stopActivity();
+    }
+}

java/ql/src/Security/CWE/CWE-925/Good.java

@@ -0,0 +1,10 @@
+public class ShutdownReceiver extends BroadcastReceiver {
+    @Override
+    public void onReceive(final Context context, final Intent intent) {
+        if (!intent.getAction().equals(Intent.ACTION_SHUTDOWN)) {
+            return;
+        }
+        mainActivity.saveLocalData();
+        mainActivity.stopActivity();
+    }
+}

java/ql/src/Security/CWE/CWE-925/ImproperIntentVerification.qhelp

@@ -0,0 +1,40 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+
+<qhelp>
+
+<overview>
+<p>
+When an Android application uses a <code>BroadcastReceiver</code> to receive intents, 
+it is also able to receive explicit intents that are sent directly to it, regardless of its filter.
+
+Certain intent actions are only able to be sent by the operating system, not third-party applications. 
+However, a <code>BroadcastReceiver</code> that is registered to receive system intents is still able to receive 
+intents from a third-party application, so it should check that the intent received has the expected action.
+Otherwise, a third-party application could impersonate the system this way to cause unintended behavior, such as a denial of service.
+</p>
+</overview>
+
+<example>
+  <p>In the following code, the <code>ShutdownReceiver</code> initiates a shutdown procedure upon receiving an intent, 
+  without checking that the received action is indeed <code>ACTION_SHUTDOWN</code>. This allows third-party applications to 
+  send explicit intents to this receiver to cause a denial of service.</p>
+<sample src="Bad.java" />
+<sample src="AndroidManifest.xml" />
+</example>
+
+<recommendation>
+<p>
+In the <code>onReceive</code> method of a <code>BroadcastReciever</code>, the action of the received Intent should be checked. The following code demonstrates this.
+</p>
+<sample src="Good.java" />
+</recommendation>
+
+
+
+<references>
+
+</references>
+
+</qhelp>

java/ql/src/Security/CWE/CWE-925/ImproperIntentVerification.ql

@@ -0,0 +1,19 @@
+/**
+ * @name Improper verification of intent by broadcast receiver
+ * @description A broadcast receiver that does not verify intents it receives may be susceptible to unintended behavior by third party applications sending it explicit intents.
+ * @kind problem
+ * @problem.severity warning
+ * @security-severity 8.2
+ * @precision high
+ * @id java/improper-intent-verification
+ * @tags security
+ *       external/cwe/cwe-925
+ */
+
+import java
+import semmle.code.java.security.ImproperIntentVerificationQuery
+
+from AndroidReceiverXmlElement reg, Method orm, SystemActionName sa
+where unverifiedSystemReceiver(reg, orm, sa)
+select orm, "This reciever doesn't verify intents it receives, and is registered $@ to receive $@.",
+  reg, "here", sa, "the system action " + sa.getName()

java/ql/src/change-notes/2022-04-27-intent-verification.md

@@ -0,0 +1,6 @@
+---
+category: newQuery
+---
+* A new query "Improper verification of intent by broadcast receiver" (`java/improper-intent-verification`) has been added. 
+This query finds instances of Android `BroadcastReceiver`s that don't verify the action string of received intents when registered
+to receive system intents.

java/ql/test/query-tests/security/CWE-925/AndroidManifest.xml

@@ -0,0 +1,9 @@
+<manifest xmlns:android="http://schemas.android.com/apk/res/android" package="test">
+    <application>
+        <receiver android:name=".BootReceiverXml">
+            <intent-filter>
+                <action android:name="android.intent.action.BOOT_COMPLETED" />
+            </intent-filter>
+        </receiver>
+    </application>
+</manifest>

java/ql/test/query-tests/security/CWE-925/BootReceiverXml.java

@@ -0,0 +1,13 @@
+package test;
+import android.content.Intent;
+import android.content.Context;
+import android.content.BroadcastReceiver;
+
+class BootReceiverXml extends BroadcastReceiver {
+    void doStuff(Intent intent) {}
+
+    @Override 
+    public void onReceive(Context ctx, Intent intent) { // $hasResult
+        doStuff(intent);
+    }
+}

java/ql/test/query-tests/security/CWE-925/ImproperIntentVerification.ql

@@ -0,0 +1,18 @@
+import java
+import semmle.code.java.security.ImproperIntentVerificationQuery
+import TestUtilities.InlineExpectationsTest
+
+class HasFlowTest extends InlineExpectationsTest {
+  HasFlowTest() { this = "HasFlowTest" }
+
+  override string getARelevantTag() { result = "hasResult" }
+
+  override predicate hasActualResult(Location location, string element, string tag, string value) {
+    tag = "hasResult" and
+    exists(Method orm | unverifiedSystemReceiver(_, orm, _) |
+      orm.getLocation() = location and
+      element = orm.toString() and
+      value = ""
+    )
+  }
+}

java/ql/test/query-tests/security/CWE-925/options

@@ -0,0 +1 @@
+// semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/google-android-9.0.0