From ec8ffeed072821de635bc662f9bdd0b413418f4a Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 27 Oct 2021 13:12:08 +0200 Subject: [PATCH 01/12] Add Intent URI Permission Manipulation query --- .../dataflow/internal/TaintTrackingUtil.qll | 5 + .../code/java/frameworks/android/Android.qll | 8 ++ .../code/java/frameworks/android/Intent.qll | 25 +++++ .../IntentUriPermissionManipulation.qll | 101 ++++++++++++++++++ .../IntentUriPermissionManipulationQuery.qll | 30 ++++++ .../lib/semmle/code/xml/AndroidManifest.qll | 15 +++ .../IntentUriPermissionManipulation.ql | 24 +++++ .../security/CWE-266/AndroidManifest.xml | 22 ++++ ...tentUriPermissionManipulationTest.expected | 0 .../IntentUriPermissionManipulationTest.ql | 11 ++ .../security/CWE-266/MainActivity.java | 85 +++++++++++++++ .../test/query-tests/security/CWE-266/options | 1 + 12 files changed, 327 insertions(+) create mode 100644 java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll create mode 100644 java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulationQuery.qll create mode 100644 java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.ql create mode 100755 java/ql/test/query-tests/security/CWE-266/AndroidManifest.xml create mode 100644 java/ql/test/query-tests/security/CWE-266/IntentUriPermissionManipulationTest.expected create mode 100644 java/ql/test/query-tests/security/CWE-266/IntentUriPermissionManipulationTest.ql create mode 100644 java/ql/test/query-tests/security/CWE-266/MainActivity.java create mode 100644 java/ql/test/query-tests/security/CWE-266/options diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll b/java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll index b1840e56212..20803508a0d 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll @@ -176,6 +176,8 @@ private predicate localAdditionalTaintExprStep(Expr src, Expr sink) { serializationStep(src, sink) or formatStep(src, sink) + or + bitwiseStep(src, sink) } /** @@ -525,6 +527,9 @@ private class FormatterCallable extends TaintPreservingCallable { } } +/** Holds if taint may flow from the operand of a bitwise expression to its result. */ +private predicate bitwiseStep(Expr src, BitwiseExpr sink) { sink.(BinaryExpr).getAnOperand() = src } + private import StringBuilderVarModule module StringBuilderVarModule { 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..cf273abbbe5 --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll @@ -0,0 +1,101 @@ +/** + * Provides classes and predicates to reason about Intent URI permission manipulation + * vulnerabilities on Android. + */ + +import java +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 { } + +private class DefaultIntentUriPermissionManipulationSink extends IntentUriPermissionManipulationSink { + DefaultIntentUriPermissionManipulationSink() { + exists(MethodAccess ma | ma.getMethod() instanceof ActivitySetResultMethod | + ma.getArgument(1) = this.asExpr() + ) + } +} + +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 + TaintTracking::localExprTaint(any(GrantReadUriPermissionFlag f).getAnAccess(), + ma.getArgument(0)) and + TaintTracking::localExprTaint(any(GrantWriteUriPermissionFlag f).getAnAccess(), + ma.getArgument(0)) + or + ma.getMethod() = m and + m.getDeclaringType() instanceof TypeIntent and + this.asExpr() = ma.getQualifier() and + m.hasName("setFlags") and + not TaintTracking::localExprTaint(any(GrantUriPermissionFlag f).getAnAccess(), + ma.getArgument(0)) + or + m.hasName("setData") + ) + } +} + +private class IntentFlagsOrDataCheckedGuard extends IntentUriPermissionManipulationGuard { + Expr condition; + + IntentFlagsOrDataCheckedGuard() { + this.(MethodAccess).getMethod() instanceof EqualsMethod and + condition = [this.(MethodAccess).getArgument(0), this.(MethodAccess).getQualifier()] + or + condition = this.(EqualityTest).getAnOperand().(AndBitwiseExpr) + } + + override predicate checks(Expr e, boolean branch) { + exists(MethodAccess ma, Method m | + ma.getMethod() = m and + m.getDeclaringType() instanceof TypeIntent and + m.hasName(["getFlags", "getData"]) and + TaintTracking::localExprTaint(ma, condition) and + ma.getQualifier() = e + | + bitwiseCheck(branch) + or + this.(MethodAccess).getMethod() instanceof EqualsMethod and branch = true + ) + } + + /** + * Holds if `this` is a bitwise check. `branch` is `true` when the expected value is `0` + * and `false` otherwise. + */ + private predicate bitwiseCheck(boolean branch) { + exists(CompileTimeConstantExpr operand | operand = this.(EqualityTest).getAnOperand() | + if operand.getIntValue() = 0 + then this.(EqualityTest).polarity() = branch + else this.(EqualityTest).polarity().booleanNot() = branch + ) + } +} 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..ab7ff6540ae --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulationQuery.qll @@ -0,0 +1,30 @@ +/** + * 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 + } +} 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.ql b/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.ql new file mode 100644 index 00000000000..b722e4312be --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.ql @@ -0,0 +1,24 @@ +/** + * @name Intent URI permission manipulation + * @description When an externally provided Intent is returned to an Activity via setResult, + * a malicious application could use this to grant itself permissions to access + * arbitrary Content Providers that are accessible by the vulnerable application. + * @kind path-problem + * @problem.severity error + * @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/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 From e1d30ebc09dde1c09389921e4b1959653b045742 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 27 Oct 2021 14:00:10 +0200 Subject: [PATCH 02/12] Added severity Removed duplicated code --- .../code/java/security/IntentUriPermissionManipulation.qll | 3 --- .../Security/CWE/CWE-266/IntentUriPermissionManipulation.ql | 1 + 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll b/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll index cf273abbbe5..34194e0d074 100644 --- a/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll +++ b/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll @@ -51,9 +51,6 @@ private class IntentFlagsOrDataChangedSanitizer extends IntentUriPermissionManip TaintTracking::localExprTaint(any(GrantWriteUriPermissionFlag f).getAnAccess(), ma.getArgument(0)) or - ma.getMethod() = m and - m.getDeclaringType() instanceof TypeIntent and - this.asExpr() = ma.getQualifier() and m.hasName("setFlags") and not TaintTracking::localExprTaint(any(GrantUriPermissionFlag f).getAnAccess(), ma.getArgument(0)) diff --git a/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.ql b/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.ql index b722e4312be..fceb4569505 100644 --- a/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.ql +++ b/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.ql @@ -5,6 +5,7 @@ * arbitrary Content Providers that are accessible by the vulnerable application. * @kind path-problem * @problem.severity error + * @security-severity 7.8 * @precision high * @id java/android/intent-uri-permission-manipulation * @tags security From 6152c8a989d8a4294698b911b177324259a3e1dc Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 4 Nov 2021 11:06:35 +0100 Subject: [PATCH 03/12] Add change note --- ...0-27-android-intent-uri-permission-manipulation-query.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 java/ql/src/change-notes/2021-10-27-android-intent-uri-permission-manipulation-query.md 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..9644ce80453 --- /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 returning 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 From 3405db31b86eb333d283a482e5fccba864d9309d Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 11 Nov 2021 09:54:14 +0100 Subject: [PATCH 04/12] Add qhelp --- .../IntentUriPermissionManipulation.java | 25 ++++++++++++++ .../IntentUriPermissionManipulation.qhelp | 34 +++++++++++++++++++ 2 files changed, 59 insertions(+) create mode 100644 java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.java create mode 100644 java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.qhelp 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..75b4d13bf48 --- /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.
    • +     +     From ab560234e3247111144950c25abcfb9c656fbb3b Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 22 Nov 2021 15:27:11 +0100 Subject: [PATCH 05/12] Update java/change-notes/2021-10-27-android-intent-uri-permission-manipulation-query.md Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com> --- ...21-10-27-android-intent-uri-permission-manipulation-query.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 9644ce80453..0da4c4189f6 100644 --- 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 @@ -2,5 +2,5 @@ category: newQuery --- * A new query "Intent URI permission manipulation" (`java/android/intent-uri-permission-manipulation`) has been added. -This query finds Android components returning unmodified, received Intents to the calling applications, which +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 From 596cfd399e8a80e35a3474513b663c8f2b5f46e8 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 22 Nov 2021 15:51:15 +0100 Subject: [PATCH 06/12] Improve description --- .../Security/CWE/CWE-266/IntentUriPermissionManipulation.ql | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.ql b/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.ql index fceb4569505..3cde1287e4a 100644 --- a/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.ql +++ b/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.ql @@ -1,8 +1,7 @@ /** * @name Intent URI permission manipulation - * @description When an externally provided Intent is returned to an Activity via setResult, - * a malicious application could use this to grant itself permissions to access - * arbitrary Content Providers that are accessible by the vulnerable application. + * @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 From 8767d2db230d1bb000d43de1fe6e85225dabb6d9 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Tue, 23 Nov 2021 11:11:23 +0100 Subject: [PATCH 07/12] Don't capitalize the term content provider Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com> --- .../CWE/CWE-266/IntentUriPermissionManipulation.qhelp | 2 +- .../Security/CWE/CWE-266/IntentUriPermissionManipulation.ql | 6 +++--- ...0-27-android-intent-uri-permission-manipulation-query.md | 2 +- .../frameworks/android/content-provider/Safe.java | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.qhelp b/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.qhelp index 75b4d13bf48..5577cf7cdb8 100644 --- a/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.qhelp +++ b/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.qhelp @@ -6,7 +6,7 @@

    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 +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.

    diff --git a/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.ql b/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.ql index 3cde1287e4a..b108da2f1de 100644 --- a/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.ql +++ b/java/ql/src/Security/CWE/CWE-266/IntentUriPermissionManipulation.ql @@ -1,7 +1,7 @@ /** * @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. + * @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 @@ -21,4 +21,4 @@ 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" + "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 index 0da4c4189f6..fddecd1b953 100644 --- 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 @@ -3,4 +3,4 @@ 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 +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) {} From 58a0bcd70f8279296348ae137652e750c8969b44 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 17 Jan 2022 12:34:37 +0100 Subject: [PATCH 08/12] Apply suggestions from code review Co-authored-by: Chris Smowton --- .../code/java/security/IntentUriPermissionManipulation.qll | 2 ++ 1 file changed, 2 insertions(+) diff --git a/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll b/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll index 34194e0d074..a2e58ed7ecb 100644 --- a/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll +++ b/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll @@ -72,6 +72,8 @@ private class IntentFlagsOrDataCheckedGuard extends IntentUriPermissionManipulat override predicate checks(Expr e, boolean branch) { exists(MethodAccess ma, Method m | + // This checks `intent` when the result of an `intent.getFlags` or `intent.getData` call flows to `condition` + // (i.e., that result is equality-tested) ma.getMethod() = m and m.getDeclaringType() instanceof TypeIntent and m.hasName(["getFlags", "getData"]) and From 62c21918b2921b444a19c47965a3522f40200993 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 17 Jan 2022 17:39:55 +0100 Subject: [PATCH 09/12] Add QLDoc to guard and sanitizer --- .../IntentUriPermissionManipulation.qll | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll b/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll index a2e58ed7ecb..31c9610e3c0 100644 --- a/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll +++ b/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll @@ -38,6 +38,14 @@ private class DefaultIntentUriPermissionManipulationSink extends IntentUriPermis } } +/** + * 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 | @@ -60,6 +68,20 @@ private class IntentFlagsOrDataChangedSanitizer extends IntentUriPermissionManip } } +/** + * 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; From 4e9849e19d0c8536b51bdd70ad14c0d96ce99dde Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 19 Jan 2022 18:03:00 +0100 Subject: [PATCH 10/12] Refactor IntentFlagsOrDataCheckedGuard to avoid footgun --- .../IntentUriPermissionManipulation.qll | 69 ++++++++++--------- 1 file changed, 36 insertions(+), 33 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll b/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll index 31c9610e3c0..f7c857832a1 100644 --- a/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll +++ b/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll @@ -4,6 +4,7 @@ */ 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 @@ -85,38 +86,40 @@ private class IntentFlagsOrDataChangedSanitizer extends IntentUriPermissionManip private class IntentFlagsOrDataCheckedGuard extends IntentUriPermissionManipulationGuard { Expr condition; - IntentFlagsOrDataCheckedGuard() { - this.(MethodAccess).getMethod() instanceof EqualsMethod and - condition = [this.(MethodAccess).getArgument(0), this.(MethodAccess).getQualifier()] - or - condition = this.(EqualityTest).getAnOperand().(AndBitwiseExpr) - } + IntentFlagsOrDataCheckedGuard() { intentFlagsOrDataChecked(this, _, _) } - override predicate checks(Expr e, boolean branch) { - exists(MethodAccess ma, Method m | - // This checks `intent` when the result of an `intent.getFlags` or `intent.getData` call flows to `condition` - // (i.e., that result is equality-tested) - ma.getMethod() = m and - m.getDeclaringType() instanceof TypeIntent and - m.hasName(["getFlags", "getData"]) and - TaintTracking::localExprTaint(ma, condition) and - ma.getQualifier() = e - | - bitwiseCheck(branch) - or - this.(MethodAccess).getMethod() instanceof EqualsMethod and branch = true - ) - } - - /** - * Holds if `this` is a bitwise check. `branch` is `true` when the expected value is `0` - * and `false` otherwise. - */ - private predicate bitwiseCheck(boolean branch) { - exists(CompileTimeConstantExpr operand | operand = this.(EqualityTest).getAnOperand() | - if operand.getIntValue() = 0 - then this.(EqualityTest).polarity() = branch - else this.(EqualityTest).polarity().booleanNot() = branch - ) - } + 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 + TaintTracking::localExprTaint(ma, 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 + ) } From 265f8a3b19213622443065098573adfcc8f1d4a3 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 20 Jan 2022 13:18:56 +0100 Subject: [PATCH 11/12] Make bitwise taintsteps specific for this query --- .../dataflow/internal/TaintTrackingUtil.qll | 5 --- .../IntentUriPermissionManipulation.qll | 32 +++++++++++++++---- .../IntentUriPermissionManipulationQuery.qll | 4 +++ 3 files changed, 29 insertions(+), 12 deletions(-) diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll b/java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll index 20803508a0d..b1840e56212 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll @@ -176,8 +176,6 @@ private predicate localAdditionalTaintExprStep(Expr src, Expr sink) { serializationStep(src, sink) or formatStep(src, sink) - or - bitwiseStep(src, sink) } /** @@ -527,9 +525,6 @@ private class FormatterCallable extends TaintPreservingCallable { } } -/** Holds if taint may flow from the operand of a bitwise expression to its result. */ -private predicate bitwiseStep(Expr src, BitwiseExpr sink) { sink.(BinaryExpr).getAnOperand() = src } - private import StringBuilderVarModule module StringBuilderVarModule { diff --git a/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll b/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll index f7c857832a1..027a915b78b 100644 --- a/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll +++ b/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll @@ -31,6 +31,18 @@ abstract class IntentUriPermissionManipulationSanitizer extends DataFlow::Node { */ 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 | @@ -55,14 +67,11 @@ private class IntentFlagsOrDataChangedSanitizer extends IntentUriPermissionManip this.asExpr() = ma.getQualifier() | m.hasName("removeFlags") and - TaintTracking::localExprTaint(any(GrantReadUriPermissionFlag f).getAnAccess(), - ma.getArgument(0)) and - TaintTracking::localExprTaint(any(GrantWriteUriPermissionFlag f).getAnAccess(), - ma.getArgument(0)) + bitwiseLocalTaintStep*(any(GrantReadUriPermissionFlag f).getAnAccess(), ma.getArgument(0)) and + bitwiseLocalTaintStep*(any(GrantWriteUriPermissionFlag f).getAnAccess(), ma.getArgument(0)) or m.hasName("setFlags") and - not TaintTracking::localExprTaint(any(GrantUriPermissionFlag f).getAnAccess(), - ma.getArgument(0)) + not bitwiseLocalTaintStep*(any(GrantUriPermissionFlag f).getAnAccess(), ma.getArgument(0)) or m.hasName("setData") ) @@ -101,7 +110,7 @@ private predicate intentFlagsOrDataChecked(Guard g, Expr intent, boolean branch) ma.getMethod() = m and m.getDeclaringType() instanceof TypeIntent and m.hasName(["getFlags", "getData"]) and - TaintTracking::localExprTaint(ma, checkedValue) + bitwiseLocalTaintStep*(ma, checkedValue) | bitwiseCheck(g, branch) and checkedValue = g.(EqualityTest).getAnOperand().(AndBitwiseExpr) @@ -123,3 +132,12 @@ private predicate bitwiseCheck(Guard g, boolean 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(Expr source, Expr sink) { + TaintTracking::localTaintStep(DataFlow::exprNode(source), DataFlow::exprNode(sink)) or + source = sink.(BinaryExpr).getAnOperand() +} diff --git a/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulationQuery.qll b/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulationQuery.qll index ab7ff6540ae..067277a71b7 100644 --- a/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulationQuery.qll +++ b/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulationQuery.qll @@ -27,4 +27,8 @@ class IntentUriPermissionManipulationConf extends TaintTracking::Configuration { override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { guard instanceof IntentUriPermissionManipulationGuard } + + override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { + any(IntentUriPermissionManipulationAdditionalTaintStep c).step(node1, node2) + } } From 3957ebe880138671098e1c3d65d3c9fa5ad4b0c5 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 20 Jan 2022 13:34:32 +0100 Subject: [PATCH 12/12] Fix bitwiseLocalTaintStep --- .../IntentUriPermissionManipulation.qll | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll b/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll index 027a915b78b..8a51de7efd7 100644 --- a/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll +++ b/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll @@ -67,11 +67,14 @@ private class IntentFlagsOrDataChangedSanitizer extends IntentUriPermissionManip this.asExpr() = ma.getQualifier() | m.hasName("removeFlags") and - bitwiseLocalTaintStep*(any(GrantReadUriPermissionFlag f).getAnAccess(), ma.getArgument(0)) and - bitwiseLocalTaintStep*(any(GrantWriteUriPermissionFlag f).getAnAccess(), ma.getArgument(0)) + 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*(any(GrantUriPermissionFlag f).getAnAccess(), ma.getArgument(0)) + not bitwiseLocalTaintStep*(DataFlow::exprNode(any(GrantUriPermissionFlag f).getAnAccess()), + DataFlow::exprNode(ma.getArgument(0))) or m.hasName("setData") ) @@ -110,7 +113,7 @@ private predicate intentFlagsOrDataChecked(Guard g, Expr intent, boolean branch) ma.getMethod() = m and m.getDeclaringType() instanceof TypeIntent and m.hasName(["getFlags", "getData"]) and - bitwiseLocalTaintStep*(ma, checkedValue) + bitwiseLocalTaintStep*(DataFlow::exprNode(ma), DataFlow::exprNode(checkedValue)) | bitwiseCheck(g, branch) and checkedValue = g.(EqualityTest).getAnOperand().(AndBitwiseExpr) @@ -137,7 +140,7 @@ private predicate bitwiseCheck(Guard g, boolean branch) { * Holds if taint can flow from `source` to `sink` in one local step, * including bitwise operations. */ -private predicate bitwiseLocalTaintStep(Expr source, Expr sink) { - TaintTracking::localTaintStep(DataFlow::exprNode(source), DataFlow::exprNode(sink)) or - source = sink.(BinaryExpr).getAnOperand() +private predicate bitwiseLocalTaintStep(DataFlow::Node source, DataFlow::Node sink) { + TaintTracking::localTaintStep(source, sink) or + source.asExpr() = sink.asExpr().(BitwiseExpr).(BinaryExpr).getAnOperand() }