Merge pull request #6975 from atorralba/atorralba/android-intent-uri-permission-manipulation

Java: CWE-266 - Query to detect Intent URI Permission Manipulation in Android applications

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

@@ -67,6 +67,14 @@ class AndroidActivity extends ExportableAndroidComponent {
   AndroidActivity() { getASuperTypeStar(this).hasQualifiedName("android.app", "Activity") }
 }
 
+/** The method `setResult` of the class `android.app.Activity`. */
+class ActivitySetResultMethod extends Method {
+  ActivitySetResultMethod() {
+    this.getDeclaringType().hasQualifiedName("android.app", "Activity") and
+    this.hasName("setResult")
+  }
+}
+
 /** An Android service. */
 class AndroidService extends ExportableAndroidComponent {
   AndroidService() { getASuperTypeStar(this).hasQualifiedName("android.app", "Service") }

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

@@ -148,6 +148,31 @@ predicate allowIntentExtrasImplicitRead(DataFlow::Node node, DataFlow::Content c
   )
 }
 
+/**
+ * The fields to grant URI permissions of the class `android.content.Intent`:
+ *
+ * - `Intent.FLAG_GRANT_READ_URI_PERMISSION`
+ * - `Intent.FLAG_GRANT_WRITE_URI_PERMISSION`
+ * - `Intent.FLAG_GRANT_PERSISTABLE_URI_PERMISSION`
+ * - `Intent.FLAG_GRANT_PREFIX_URI_PERMISSION`
+ */
+class GrantUriPermissionFlag extends Field {
+  GrantUriPermissionFlag() {
+    this.getDeclaringType() instanceof TypeIntent and
+    this.getName().matches("FLAG_GRANT_%_URI_PERMISSION")
+  }
+}
+
+/** The field `Intent.FLAG_GRANT_READ_URI_PERMISSION`. */
+class GrantReadUriPermissionFlag extends GrantUriPermissionFlag {
+  GrantReadUriPermissionFlag() { this.hasName("FLAG_GRANT_READ_URI_PERMISSION") }
+}
+
+/** The field `Intent.FLAG_GRANT_WRITE_URI_PERMISSION`. */
+class GrantWriteUriPermissionFlag extends GrantUriPermissionFlag {
+  GrantWriteUriPermissionFlag() { this.hasName("FLAG_GRANT_WRITE_URI_PERMISSION") }
+}
+
 private class IntentBundleFlowSteps extends SummaryModelCsv {
   override predicate row(string row) {
     row =

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

@@ -0,0 +1,146 @@
+/**
+ * Provides classes and predicates to reason about Intent URI permission manipulation
+ * vulnerabilities on Android.
+ */
+
+import java
+private import semmle.code.java.controlflow.Guards
+private import semmle.code.java.dataflow.DataFlow
+private import semmle.code.java.dataflow.TaintTracking
+private import semmle.code.java.frameworks.android.Android
+private import semmle.code.java.frameworks.android.Intent
+
+/**
+ * A sink for Intent URI permission manipulation vulnerabilities in Android,
+ * that is, method calls that return an Intent as the result of an Activity.
+ */
+abstract class IntentUriPermissionManipulationSink extends DataFlow::Node { }
+
+/**
+ * A sanitizer that makes sure that an Intent is safe to be returned to another Activity.
+ *
+ * Usually, this is done by setting the Intent's data URI and/or its flags to safe values.
+ */
+abstract class IntentUriPermissionManipulationSanitizer extends DataFlow::Node { }
+
+/**
+ * A guard that makes sure that an Intent is safe to be returned to another Activity.
+ *
+ * Usually, this is done by checking that the Intent's data URI and/or its flags contain
+ * expected values.
+ */
+abstract class IntentUriPermissionManipulationGuard extends DataFlow::BarrierGuard { }
+
+/**
+ * An additional taint step for flows related to Intent URI permission manipulation
+ * vulnerabilities.
+ */
+class IntentUriPermissionManipulationAdditionalTaintStep extends Unit {
+  /**
+   * Holds if the step from `node1` to `node2` should be considered a taint
+   * step for flows related to Intent URI permission manipulation vulnerabilities.
+   */
+  abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
+}
+
+private class DefaultIntentUriPermissionManipulationSink extends IntentUriPermissionManipulationSink {
+  DefaultIntentUriPermissionManipulationSink() {
+    exists(MethodAccess ma | ma.getMethod() instanceof ActivitySetResultMethod |
+      ma.getArgument(1) = this.asExpr()
+    )
+  }
+}
+
+/**
+ * Sanitizer that prevents access to arbitrary content providers by modifying the Intent in one of
+ * the following ways:
+ * * Removing the flags `FLAG_GRANT_READ_URI_PERMISSION` and `FLAG_GRANT_WRITE_URI_PERMISSION`.
+ * * Setting the flags to a combination that doesn't include `FLAG_GRANT_READ_URI_PERMISSION` or
+ * `FLAG_GRANT_WRITE_URI_PERMISSION`.
+ * * Replacing the data URI.
+ */
+private class IntentFlagsOrDataChangedSanitizer extends IntentUriPermissionManipulationSanitizer {
+  IntentFlagsOrDataChangedSanitizer() {
+    exists(MethodAccess ma, Method m |
+      ma.getMethod() = m and
+      m.getDeclaringType() instanceof TypeIntent and
+      this.asExpr() = ma.getQualifier()
+    |
+      m.hasName("removeFlags") and
+      bitwiseLocalTaintStep*(DataFlow::exprNode(any(GrantReadUriPermissionFlag f).getAnAccess()),
+        DataFlow::exprNode(ma.getArgument(0))) and
+      bitwiseLocalTaintStep*(DataFlow::exprNode(any(GrantWriteUriPermissionFlag f).getAnAccess()),
+        DataFlow::exprNode(ma.getArgument(0)))
+      or
+      m.hasName("setFlags") and
+      not bitwiseLocalTaintStep*(DataFlow::exprNode(any(GrantUriPermissionFlag f).getAnAccess()),
+        DataFlow::exprNode(ma.getArgument(0)))
+      or
+      m.hasName("setData")
+    )
+  }
+}
+
+/**
+ * A guard that checks an Intent's flags or data URI to make sure they are trusted.
+ * It matches the following patterns:
+ *
+ * ```java
+ * if (intent.getData().equals("trustedValue")) {}
+ *
+ * if (intent.getFlags() & Intent.FLAG_GRANT_READ_URI_PERMISSION == 0 &&
+ *      intent.getFlags() & Intent.FLAG_GRANT_WRITE_URI_PERMISSION == 0) {}
+ *
+ * if (intent.getFlags() & Intent.FLAG_GRANT_READ_URI_PERMISSION != 0 ||
+ *      intent.getFlags() & Intent.FLAG_GRANT_WRITE_URI_PERMISSION != 0) {}
+ * ```
+ */
+private class IntentFlagsOrDataCheckedGuard extends IntentUriPermissionManipulationGuard {
+  Expr condition;
+
+  IntentFlagsOrDataCheckedGuard() { intentFlagsOrDataChecked(this, _, _) }
+
+  override predicate checks(Expr e, boolean branch) { intentFlagsOrDataChecked(this, e, branch) }
+}
+
+/**
+ * Holds if `g` checks `intent` when the result of an `intent.getFlags` or `intent.getData` call
+ * is equality-tested.
+ */
+private predicate intentFlagsOrDataChecked(Guard g, Expr intent, boolean branch) {
+  exists(MethodAccess ma, Method m, Expr checkedValue |
+    ma.getQualifier() = intent and
+    ma.getMethod() = m and
+    m.getDeclaringType() instanceof TypeIntent and
+    m.hasName(["getFlags", "getData"]) and
+    bitwiseLocalTaintStep*(DataFlow::exprNode(ma), DataFlow::exprNode(checkedValue))
+  |
+    bitwiseCheck(g, branch) and
+    checkedValue = g.(EqualityTest).getAnOperand().(AndBitwiseExpr)
+    or
+    g.(MethodAccess).getMethod() instanceof EqualsMethod and
+    branch = true and
+    checkedValue = [g.(MethodAccess).getArgument(0), g.(MethodAccess).getQualifier()]
+  )
+}
+
+/**
+ * Holds if `g` is a bitwise check. `branch` is `true` when the expected value is `0`
+ * and `false` otherwise.
+ */
+private predicate bitwiseCheck(Guard g, boolean branch) {
+  exists(CompileTimeConstantExpr operand | operand = g.(EqualityTest).getAnOperand() |
+    if operand.getIntValue() = 0
+    then g.(EqualityTest).polarity() = branch
+    else g.(EqualityTest).polarity().booleanNot() = branch
+  )
+}
+
+/**
+ * Holds if taint can flow from `source` to `sink` in one local step,
+ * including bitwise operations.
+ */
+private predicate bitwiseLocalTaintStep(DataFlow::Node source, DataFlow::Node sink) {
+  TaintTracking::localTaintStep(source, sink) or
+  source.asExpr() = sink.asExpr().(BitwiseExpr).(BinaryExpr).getAnOperand()
+}

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

@@ -0,0 +1,34 @@
+/**
+ * Provides taint tracking configurations to be used in queries related to Intent URI permission
+ * manipulation in Android.
+ */
+
+import java
+private import semmle.code.java.dataflow.FlowSources
+private import semmle.code.java.dataflow.DataFlow
+private import IntentUriPermissionManipulation
+
+/**
+ * A taint tracking configuration for user-provided Intents being returned to third party apps.
+ */
+class IntentUriPermissionManipulationConf extends TaintTracking::Configuration {
+  IntentUriPermissionManipulationConf() { this = "UriPermissionManipulationConf" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) {
+    sink instanceof IntentUriPermissionManipulationSink
+  }
+
+  override predicate isSanitizer(DataFlow::Node barrier) {
+    barrier instanceof IntentUriPermissionManipulationSanitizer
+  }
+
+  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+    guard instanceof IntentUriPermissionManipulationGuard
+  }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
+    any(IntentUriPermissionManipulationAdditionalTaintStep c).step(node1, node2)
+  }
+}

java/ql/lib/semmle/code/xml/AndroidManifest.qll

@@ -74,6 +74,13 @@ class AndroidReceiverXmlElement extends AndroidComponentXmlElement {
   AndroidReceiverXmlElement() { this.getName() = "receiver" }
 }
 
+/**
+ * An XML attribute with the `android:` prefix.
+ */
+class AndroidXmlAttribute extends XMLAttribute {
+  AndroidXmlAttribute() { this.getNamespace().getPrefix() = "android" }
+}
+
 /**
  * A `<provider>` element in an Android manifest file.
  */
@@ -91,6 +98,14 @@ class AndroidProviderXmlElement extends AndroidComponentXmlElement {
     this.getAnAttribute().(AndroidPermissionXmlAttribute).isWrite() and
     this.getAnAttribute().(AndroidPermissionXmlAttribute).isRead()
   }
+
+  predicate grantsUriPermissions() {
+    exists(AndroidXmlAttribute attr |
+      this.getAnAttribute() = attr and
+      attr.getName() = "grantUriPermissions" and
+      attr.getValue() = "true"
+    )
+  }
 }
 
 /**

java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.java

@@ -0,0 +1,25 @@
+public class IntentUriPermissionManipulation extends Activity {
+
+    // BAD: the user-provided Intent is returned as-is
+    public void dangerous() {
+        Intent intent = getIntent();
+        intent.putExtra("result", "resultData");
+        setResult(intent);
+    }
+
+    // GOOD: a new Intent is created and returned
+    public void safe() {
+        Intent intent = new Intent();
+        intent.putExtra("result", "resultData");
+        setResult(intent);
+    }
+
+    // GOOD: the user-provided Intent is sanitized before being returned
+    public void sanitized() {
+        Intent intent = getIntent();
+        intent.putExtra("result", "resultData");
+        intent.removeFlags(
+                Intent.FLAG_GRANT_WRITE_URI_PERMISSION | Intent.FLAG_GRANT_READ_URI_PERMISSION);
+        setResult(intent);
+    }
+}

java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.qhelp

@@ -0,0 +1,34 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>When an Android component expects a result from an Activity, <code>startActivityForResult</code> can be used.
+The started Activity can then use <code>setResult</code> to return the appropriate data to the calling component.</p>
+<p>If an Activity obtains the incoming, user-provided Intent and directly returns it via <code>setResult</code>
+without any checks, the application may be unintentionally giving arbitrary access to its content providers, even
+if they are not exported, as long as they are configured with the attribute <code>android:grantUriPermissions="true"</code>. 
+This happens because the attacker adds the appropriate URI permission flags to the provided Intent, which take effect
+once the Intent is reflected back.</p>
+</overview>
+
+<recommendation>
+<p>Avoid returning user-provided or untrusted Intents via <code>setResult</code>. Use a new Intent instead.</p>
+<p>If it is required to use the received Intent, make sure that it does not contain URI permission flags, either
+by checking them with <code>Intent.getFlags</code> or removing them with <code>Intent.removeFlags</code>.</p>
+</recommendation>
+
+<example>
+<p>The following sample contains three examples. In the first example, a user-provided Intent is obtained and 
+  directly returned back with <code>setResult</code>, which is dangerous. In the second example, a new Intent
+  is created to safely return the desired data. The third example shows how the obtained Intent can be sanitized
+  by removing dangerous flags before using it to return data to the calling component.
+</p>
+
+<sample src="IntentUriPermissionManipulation.java" />
+</example>
+
+<references>
+<li>Google Help: <a href="https://support.google.com/faqs/answer/9267555?hl=en">Remediation for Intent Redirection Vulnerability</a>.</li>
+</references>
+</qhelp>

java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.ql

@@ -0,0 +1,24 @@
+/**
+ * @name Intent URI permission manipulation
+ * @description Returning an externally provided Intent via 'setResult' may allow a malicious
+ *              application to access arbitrary content providers of the vulnerable application.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 7.8
+ * @precision high
+ * @id java/android/intent-uri-permission-manipulation
+ * @tags security
+ *       external/cwe/cwe-266
+ *       external/cwe/cwe-926
+ */
+
+import java
+import semmle.code.java.security.IntentUriPermissionManipulationQuery
+import semmle.code.java.dataflow.DataFlow
+import DataFlow::PathGraph
+
+from DataFlow::PathNode source, DataFlow::PathNode sink
+where any(IntentUriPermissionManipulationConf c).hasFlowPath(source, sink)
+select sink.getNode(), source, sink,
+  "This Intent can be set with arbitrary flags from $@, " +
+    "and used to give access to internal content providers.", source.getNode(), "this user input"

java/ql/src/change-notes/2021-10-27-android-intent-uri-permission-manipulation-query.md

@@ -0,0 +1,6 @@
+---
+category: newQuery
+---
+* A new query "Intent URI permission manipulation" (`java/android/intent-uri-permission-manipulation`) has been added.
+This query finds Android components that return unmodified, received Intents to the calling applications, which
+can provide unintended access to internal content providers of the victim application.

java/ql/test/library-tests/frameworks/android/content-provider/Safe.java

@@ -11,7 +11,7 @@ import android.os.CancellationSignal;
 import android.os.ParcelFileDescriptor;
 import android.os.RemoteException;
 
-// This Content Provider isn't exported, so there shouldn't be any flow
+// This content provider isn't exported, so there shouldn't be any flow
 public class Safe extends ContentProvider {
 
 	void sink(Object o) {}

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

@@ -0,0 +1,22 @@
+<manifest xmlns:android="http://schemas.android.com/apk/res/android"
+    package="com.example.app"
+    android:installLocation="auto"
+    android:versionCode="1"
+    android:versionName="0.1" >
+
+    <application
+        android:icon="@drawable/ic_launcher"
+        android:label="@string/app_name"
+        android:theme="@style/AppTheme" >
+        <activity
+            android:name=".MainActivity"
+            android:icon="@drawable/ic_launcher"
+			android:label="@string/app_name">
+            <intent-filter>
+                <action android:name="android.intent.action.MAIN" />
+                <category android:name="android.intent.category.LAUNCHER" />
+            </intent-filter>
+        </activity>
+    </application>
+
+</manifest>

java/ql/test/query-tests/security/CWE-266/IntentUriPermissionManipulationTest.ql

@@ -0,0 +1,11 @@
+import java
+import TestUtilities.InlineFlowTest
+import semmle.code.java.security.IntentUriPermissionManipulationQuery
+
+class IntentUriPermissionManipulationTest extends InlineFlowTest {
+  override DataFlow::Configuration getValueFlowConfig() { none() }
+
+  override TaintTracking::Configuration getTaintFlowConfig() {
+    result instanceof IntentUriPermissionManipulationConf
+  }
+}

java/ql/test/query-tests/security/CWE-266/MainActivity.java

@@ -0,0 +1,85 @@
+package com.example.app;
+
+import android.app.Activity;
+import android.content.Intent;
+import android.net.Uri;
+import android.os.Bundle;
+
+public class MainActivity extends Activity {
+
+    public void onCreate(Bundle savedInstance) {
+        {
+            Intent intent = getIntent();
+            setResult(RESULT_OK, intent); // $ hasTaintFlow
+        }
+        {
+            Intent extraIntent = (Intent) getIntent().getParcelableExtra("extraIntent");
+            setResult(RESULT_OK, extraIntent); // $ hasTaintFlow
+        }
+        {
+            Intent intent = getIntent();
+            intent.setData(Uri.parse("content://safe/uri")); // Sanitizer
+            setResult(RESULT_OK, intent); // Safe
+        }
+        {
+            Intent intent = getIntent();
+            intent.setFlags(0); // Sanitizer
+            setResult(RESULT_OK, intent); // Safe
+        }
+        {
+            Intent intent = getIntent();
+            intent.setFlags( // Not properly sanitized
+                    Intent.FLAG_GRANT_WRITE_URI_PERMISSION | Intent.FLAG_ACTIVITY_CLEAR_TOP);
+            setResult(RESULT_OK, intent); // $ hasTaintFlow
+        }
+        {
+            Intent intent = getIntent();
+            intent.removeFlags( // Sanitizer
+                    Intent.FLAG_GRANT_WRITE_URI_PERMISSION | Intent.FLAG_GRANT_READ_URI_PERMISSION);
+            setResult(RESULT_OK, intent); // Safe
+        }
+        {
+            Intent intent = getIntent();
+            // Combined, the following two calls are a sanitizer
+            intent.removeFlags(Intent.FLAG_GRANT_READ_URI_PERMISSION);
+            intent.removeFlags(Intent.FLAG_GRANT_WRITE_URI_PERMISSION);
+            setResult(RESULT_OK, intent); // $ SPURIOUS: $ hasTaintFlow
+        }
+        {
+            Intent intent = getIntent();
+            intent.removeFlags( // Not properly sanitized
+                    Intent.FLAG_GRANT_WRITE_URI_PERMISSION | Intent.FLAG_ACTIVITY_CLEAR_TOP);
+            setResult(RESULT_OK, intent); // $ hasTaintFlow
+        }
+        {
+            Intent intent = getIntent();
+            // Good check
+            if (intent.getData().equals(Uri.parse("content://safe/uri"))) {
+                setResult(RESULT_OK, intent); // Safe
+            } else {
+                setResult(RESULT_OK, intent); // $ hasTaintFlow
+            }
+        }
+        {
+            Intent intent = getIntent();
+            int flags = intent.getFlags();
+            // Good check
+            if ((flags & Intent.FLAG_GRANT_READ_URI_PERMISSION) == 0
+                    && (flags & Intent.FLAG_GRANT_WRITE_URI_PERMISSION) == 0) {
+                setResult(RESULT_OK, intent); // Safe
+            } else {
+                setResult(RESULT_OK, intent); // $ hasTaintFlow
+            }
+        }
+        {
+            Intent intent = getIntent();
+            int flags = intent.getFlags();
+            // Insufficient check
+            if ((flags & Intent.FLAG_GRANT_READ_URI_PERMISSION) == 0) {
+                setResult(RESULT_OK, intent); // $ MISSING: $ hasTaintFlow
+            } else {
+                setResult(RESULT_OK, intent); // $ hasTaintFlow
+            }
+        }
+    }
+}

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

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