Skip to content

Commit ec8ffee

Browse files
atorralbaatorralba
committed
Add Intent URI Permission Manipulation query
1 parent c09b669 commit ec8ffee

File tree

12 files changed

+327
-0
lines changed

12 files changed

+327
-0
lines changed

‎java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,8 @@ private predicate localAdditionalTaintExprStep(Expr src, Expr sink) {
176176
serializationStep(src, sink)
177177
or
178178
formatStep(src, sink)
179+
or
180+
bitwiseStep(src, sink)
179181
}
180182

181183
/**
@@ -525,6 +527,9 @@ private class FormatterCallable extends TaintPreservingCallable {
525527
}
526528
}
527529

530+
/** Holds if taint may flow from the operand of a bitwise expression to its result. */
531+
private predicate bitwiseStep(Expr src, BitwiseExpr sink) { sink.(BinaryExpr).getAnOperand() = src }
532+
528533
private import StringBuilderVarModule
529534

530535
module StringBuilderVarModule {

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,14 @@ class AndroidActivity extends ExportableAndroidComponent {
6767
AndroidActivity() { getASuperTypeStar(this).hasQualifiedName("android.app", "Activity") }
6868
}
6969

70+
/** The method `setResult` of the class `android.app.Activity`. */
71+
class ActivitySetResultMethod extends Method {
72+
ActivitySetResultMethod() {
73+
this.getDeclaringType().hasQualifiedName("android.app", "Activity") and
74+
this.hasName("setResult")
75+
}
76+
}
77+
7078
/** An Android service. */
7179
class AndroidService extends ExportableAndroidComponent {
7280
AndroidService() { getASuperTypeStar(this).hasQualifiedName("android.app", "Service") }

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

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,31 @@ predicate allowIntentExtrasImplicitRead(DataFlow::Node node, DataFlow::Content c
148148
)
149149
}
150150

151+
/**
152+
* The fields to grant URI permissions of the class `android.content.Intent`:
153+
*
154+
* - `Intent.FLAG_GRANT_READ_URI_PERMISSION`
155+
* - `Intent.FLAG_GRANT_WRITE_URI_PERMISSION`
156+
* - `Intent.FLAG_GRANT_PERSISTABLE_URI_PERMISSION`
157+
* - `Intent.FLAG_GRANT_PREFIX_URI_PERMISSION`
158+
*/
159+
class GrantUriPermissionFlag extends Field {
160+
GrantUriPermissionFlag() {
161+
this.getDeclaringType() instanceof TypeIntent and
162+
this.getName().matches("FLAG_GRANT_%_URI_PERMISSION")
163+
}
164+
}
165+
166+
/** The field `Intent.FLAG_GRANT_READ_URI_PERMISSION`. */
167+
class GrantReadUriPermissionFlag extends GrantUriPermissionFlag {
168+
GrantReadUriPermissionFlag() { this.hasName("FLAG_GRANT_READ_URI_PERMISSION") }
169+
}
170+
171+
/** The field `Intent.FLAG_GRANT_WRITE_URI_PERMISSION`. */
172+
class GrantWriteUriPermissionFlag extends GrantUriPermissionFlag {
173+
GrantWriteUriPermissionFlag() { this.hasName("FLAG_GRANT_WRITE_URI_PERMISSION") }
174+
}
175+
151176
private class IntentBundleFlowSteps extends SummaryModelCsv {
152177
override predicate row(string row) {
153178
row =

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

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
/**
2+
* Provides classes and predicates to reason about Intent URI permission manipulation
3+
* vulnerabilities on Android.
4+
*/
5+
6+
import java
7+
private import semmle.code.java.dataflow.DataFlow
8+
private import semmle.code.java.dataflow.TaintTracking
9+
private import semmle.code.java.frameworks.android.Android
10+
private import semmle.code.java.frameworks.android.Intent
11+
12+
/**
13+
* A sink for Intent URI permission manipulation vulnerabilities in Android,
14+
* that is, method calls that return an Intent as the result of an Activity.
15+
*/
16+
abstract class IntentUriPermissionManipulationSink extends DataFlow::Node { }
17+
18+
/**
19+
* A sanitizer that makes sure that an Intent is safe to be returned to another Activity.
20+
*
21+
* Usually, this is done by setting the Intent's data URI and/or its flags to safe values.
22+
*/
23+
abstract class IntentUriPermissionManipulationSanitizer extends DataFlow::Node { }
24+
25+
/**
26+
* A guard that makes sure that an Intent is safe to be returned to another Activity.
27+
*
28+
* Usually, this is done by checking that the Intent's data URI and/or its flags contain
29+
* expected values.
30+
*/
31+
abstract class IntentUriPermissionManipulationGuard extends DataFlow::BarrierGuard { }
32+
33+
private class DefaultIntentUriPermissionManipulationSink extends IntentUriPermissionManipulationSink {
34+
DefaultIntentUriPermissionManipulationSink() {
35+
exists(MethodAccess ma | ma.getMethod() instanceof ActivitySetResultMethod |
36+
ma.getArgument(1) = this.asExpr()
37+
)
38+
}
39+
}
40+
41+
private class IntentFlagsOrDataChangedSanitizer extends IntentUriPermissionManipulationSanitizer {
42+
IntentFlagsOrDataChangedSanitizer() {
43+
exists(MethodAccess ma, Method m |
44+
ma.getMethod() = m and
45+
m.getDeclaringType() instanceof TypeIntent and
46+
this.asExpr() = ma.getQualifier()
47+
|
48+
m.hasName("removeFlags") and
49+
TaintTracking::localExprTaint(any(GrantReadUriPermissionFlag f).getAnAccess(),
50+
ma.getArgument(0)) and
51+
TaintTracking::localExprTaint(any(GrantWriteUriPermissionFlag f).getAnAccess(),
52+
ma.getArgument(0))
53+
or
54+
ma.getMethod() = m and
55+
m.getDeclaringType() instanceof TypeIntent and
56+
this.asExpr() = ma.getQualifier() and
57+
m.hasName("setFlags") and
58+
not TaintTracking::localExprTaint(any(GrantUriPermissionFlag f).getAnAccess(),
59+
ma.getArgument(0))
60+
or
61+
m.hasName("setData")
62+
)
63+
}
64+
}
65+
66+
private class IntentFlagsOrDataCheckedGuard extends IntentUriPermissionManipulationGuard {
67+
Expr condition;
68+
69+
IntentFlagsOrDataCheckedGuard() {
70+
this.(MethodAccess).getMethod() instanceof EqualsMethod and
71+
condition = [this.(MethodAccess).getArgument(0), this.(MethodAccess).getQualifier()]
72+
or
73+
condition = this.(EqualityTest).getAnOperand().(AndBitwiseExpr)
74+
}
75+
76+
override predicate checks(Expr e, boolean branch) {
77+
exists(MethodAccess ma, Method m |
78+
ma.getMethod() = m and
79+
m.getDeclaringType() instanceof TypeIntent and
80+
m.hasName(["getFlags", "getData"]) and
81+
TaintTracking::localExprTaint(ma, condition) and
82+
ma.getQualifier() = e
83+
|
84+
bitwiseCheck(branch)
85+
or
86+
this.(MethodAccess).getMethod() instanceof EqualsMethod and branch = true
87+
)
88+
}
89+
90+
/**
91+
* Holds if `this` is a bitwise check. `branch` is `true` when the expected value is `0`
92+
* and `false` otherwise.
93+
*/
94+
private predicate bitwiseCheck(boolean branch) {
95+
exists(CompileTimeConstantExpr operand | operand = this.(EqualityTest).getAnOperand() |
96+
if operand.getIntValue() = 0
97+
then this.(EqualityTest).polarity() = branch
98+
else this.(EqualityTest).polarity().booleanNot() = branch
99+
)
100+
}
101+
}

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
/**
2+
* Provides taint tracking configurations to be used in queries related to Intent URI permission
3+
* manipulation in Android.
4+
*/
5+
6+
import java
7+
private import semmle.code.java.dataflow.FlowSources
8+
private import semmle.code.java.dataflow.DataFlow
9+
private import IntentUriPermissionManipulation
10+
11+
/**
12+
* A taint tracking configuration for user-provided Intents being returned to third party apps.
13+
*/
14+
class IntentUriPermissionManipulationConf extends TaintTracking::Configuration {
15+
IntentUriPermissionManipulationConf() { this = "UriPermissionManipulationConf" }
16+
17+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
18+
19+
override predicate isSink(DataFlow::Node sink) {
20+
sink instanceof IntentUriPermissionManipulationSink
21+
}
22+
23+
override predicate isSanitizer(DataFlow::Node barrier) {
24+
barrier instanceof IntentUriPermissionManipulationSanitizer
25+
}
26+
27+
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
28+
guard instanceof IntentUriPermissionManipulationGuard
29+
}
30+
}

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,13 @@ class AndroidReceiverXmlElement extends AndroidComponentXmlElement {
7474
AndroidReceiverXmlElement() { this.getName() = "receiver" }
7575
}
7676

77+
/**
78+
* An XML attribute with the `android:` prefix.
79+
*/
80+
class AndroidXmlAttribute extends XMLAttribute {
81+
AndroidXmlAttribute() { this.getNamespace().getPrefix() = "android" }
82+
}
83+
7784
/**
7885
* A `<provider>` element in an Android manifest file.
7986
*/
@@ -91,6 +98,14 @@ class AndroidProviderXmlElement extends AndroidComponentXmlElement {
9198
this.getAnAttribute().(AndroidPermissionXmlAttribute).isWrite() and
9299
this.getAnAttribute().(AndroidPermissionXmlAttribute).isRead()
93100
}
101+
102+
predicate grantsUriPermissions() {
103+
exists(AndroidXmlAttribute attr |
104+
this.getAnAttribute() = attr and
105+
attr.getName() = "grantUriPermissions" and
106+
attr.getValue() = "true"
107+
)
108+
}
94109
}
95110

96111
/**

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
/**
2+
* @name Intent URI permission manipulation
3+
* @description When an externally provided Intent is returned to an Activity via setResult,
4+
* a malicious application could use this to grant itself permissions to access
5+
* arbitrary Content Providers that are accessible by the vulnerable application.
6+
* @kind path-problem
7+
* @problem.severity error
8+
* @precision high
9+
* @id java/android/intent-uri-permission-manipulation
10+
* @tags security
11+
* external/cwe/cwe-266
12+
* external/cwe/cwe-926
13+
*/
14+
15+
import java
16+
import semmle.code.java.security.IntentUriPermissionManipulationQuery
17+
import semmle.code.java.dataflow.DataFlow
18+
import DataFlow::PathGraph
19+
20+
from DataFlow::PathNode source, DataFlow::PathNode sink
21+
where any(IntentUriPermissionManipulationConf c).hasFlowPath(source, sink)
22+
select sink.getNode(), source, sink,
23+
"This Intent can be set with arbitrary flags from $@, " +
24+
"and used to give access to internal Content Providers.", source.getNode(), "this user input"

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
2+
package="com.example.app"
3+
android:installLocation="auto"
4+
android:versionCode="1"
5+
android:versionName="0.1" >
6+
7+
<application
8+
android:icon="@drawable/ic_launcher"
9+
android:label="@string/app_name"
10+
android:theme="@style/AppTheme" >
11+
<activity
12+
android:name=".MainActivity"
13+
android:icon="@drawable/ic_launcher"
14+
android:label="@string/app_name">
15+
<intent-filter>
16+
<action android:name="android.intent.action.MAIN" />
17+
<category android:name="android.intent.category.LAUNCHER" />
18+
</intent-filter>
19+
</activity>
20+
</application>
21+
22+
</manifest>

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

Whitespace-only changes.

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
import java
2+
import TestUtilities.InlineFlowTest
3+
import semmle.code.java.security.IntentUriPermissionManipulationQuery
4+
5+
class IntentUriPermissionManipulationTest extends InlineFlowTest {
6+
override DataFlow::Configuration getValueFlowConfig() { none() }
7+
8+
override TaintTracking::Configuration getTaintFlowConfig() {
9+
result instanceof IntentUriPermissionManipulationConf
10+
}
11+
}

0 commit comments

Comments
 (0)