diff --git a/java/ql/lib/semmle/code/java/frameworks/android/Android.qll b/java/ql/lib/semmle/code/java/frameworks/android/Android.qll index c019dc11bd8..41ad2d14968 100644 --- a/java/ql/lib/semmle/code/java/frameworks/android/Android.qll +++ b/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") } diff --git a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll index d77e4a5b86d..51f3b577420 100644 --- a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll +++ b/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 = diff --git a/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll b/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll new file mode 100644 index 00000000000..8a51de7efd7 --- /dev/null +++ b/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() +} diff --git a/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulationQuery.qll b/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulationQuery.qll new file mode 100644 index 00000000000..067277a71b7 --- /dev/null +++ b/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) + } +} diff --git a/java/ql/lib/semmle/code/xml/AndroidManifest.qll b/java/ql/lib/semmle/code/xml/AndroidManifest.qll index 234aacca967..81b3c44e62b 100644 --- a/java/ql/lib/semmle/code/xml/AndroidManifest.qll +++ b/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 `` 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" + ) + } } /** diff --git a/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.java b/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.java new file mode 100644 index 00000000000..4d53add78c2 --- /dev/null +++ b/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); + } +} diff --git a/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.qhelp b/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.qhelp new file mode 100644 index 00000000000..5577cf7cdb8 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.qhelp @@ -0,0 +1,34 @@ + + + +

When an Android component expects a result from an Activity, startActivityForResult can be used. +The started Activity can then use setResult to return the appropriate data to the calling component.

+

If an Activity obtains the incoming, user-provided Intent and directly returns it via setResult +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 android:grantUriPermissions="true". +This happens because the attacker adds the appropriate URI permission flags to the provided Intent, which take effect +once the Intent is reflected back.

+ + + +

Avoid returning user-provided or untrusted Intents via setResult. Use a new Intent instead.

+

If it is required to use the received Intent, make sure that it does not contain URI permission flags, either +by checking them with Intent.getFlags or removing them with Intent.removeFlags.

+ + + +

The following sample contains three examples. In the first example, a user-provided Intent is obtained and + directly returned back with setResult, 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. +

+ + + + + +
  • Google Help: Remediation for Intent Redirection Vulnerability.
    • +     +     diff --git a/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.ql b/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.ql new file mode 100644 index 00000000000..b108da2f1de --- /dev/null +++ b/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" diff --git a/java/ql/src/change-notes/2021-10-27-android-intent-uri-permission-manipulation-query.md b/java/ql/src/change-notes/2021-10-27-android-intent-uri-permission-manipulation-query.md new file mode 100644 index 00000000000..fddecd1b953 --- /dev/null +++ b/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. \ No newline at end of file diff --git a/java/ql/test/library-tests/frameworks/android/content-provider/Safe.java b/java/ql/test/library-tests/frameworks/android/content-provider/Safe.java index c61ad642b88..0ef6f8f8e32 100644 --- a/java/ql/test/library-tests/frameworks/android/content-provider/Safe.java +++ b/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) {} diff --git a/java/ql/test/query-tests/security/CWE-266/AndroidManifest.xml b/java/ql/test/query-tests/security/CWE-266/AndroidManifest.xml new file mode 100755 index 00000000000..cff71a75235 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-266/AndroidManifest.xml @@ -0,0 +1,22 @@ + + + + + + + + + + + + diff --git a/java/ql/test/query-tests/security/CWE-266/IntentUriPermissionManipulationTest.expected b/java/ql/test/query-tests/security/CWE-266/IntentUriPermissionManipulationTest.expected new file mode 100644 index 00000000000..e69de29bb2d diff --git a/java/ql/test/query-tests/security/CWE-266/IntentUriPermissionManipulationTest.ql b/java/ql/test/query-tests/security/CWE-266/IntentUriPermissionManipulationTest.ql new file mode 100644 index 00000000000..72b95d874d2 --- /dev/null +++ b/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 + } +} diff --git a/java/ql/test/query-tests/security/CWE-266/MainActivity.java b/java/ql/test/query-tests/security/CWE-266/MainActivity.java new file mode 100644 index 00000000000..4af80cc2b18 --- /dev/null +++ b/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 + } + } + } +} diff --git a/java/ql/test/query-tests/security/CWE-266/options b/java/ql/test/query-tests/security/CWE-266/options new file mode 100644 index 00000000000..dacd3cb21df --- /dev/null +++ b/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