Skip to content

Commit 9f616e7

Browse files
atorralbaatorralba
committed
Refactor to use FlowState
Remove the auxiliary DataFlow configuration
1 parent df95317 commit 9f616e7

File tree

5 files changed

+163
-106
lines changed

5 files changed

+163
-106
lines changed

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

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -87,11 +87,6 @@ class AndroidBundle extends Class {
8787
AndroidBundle() { this.getASupertype*().hasQualifiedName("android.os", "BaseBundle") }
8888
}
8989

90-
/** The class `android.app.PendingIntent`. */
91-
class PendingIntent extends Class {
92-
PendingIntent() { this.hasQualifiedName("android.app", "PendingIntent") }
93-
}
94-
9590
/** An `Intent` that explicitly sets a destination component. */
9691
class ExplicitIntent extends Expr {
9792
ExplicitIntent() {

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

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
/** Provides classes and predicates related to the class `PendingIntent`. */
2+
3+
import java
4+
5+
/** The class `android.app.PendingIntent`. */
6+
class PendingIntent extends Class {
7+
PendingIntent() { this.hasQualifiedName("android.app", "PendingIntent") }
8+
}
9+
10+
/** A call to a method that creates a `PendingIntent`. */
11+
class PendingIntentCreation extends MethodAccess {
12+
PendingIntentCreation() {
13+
exists(Method m |
14+
this.getMethod() = m and
15+
m.getDeclaringType() instanceof PendingIntent and
16+
m.hasName([
17+
"getActivity", "getActivityAsUser", "getActivities", "getActivitiesAsUser",
18+
"getBroadcast", "getBroadcastAsUser", "getService", "getForegroundService"
19+
])
20+
)
21+
}
22+
23+
/** The `Intent` argument of this call. */
24+
Argument getIntentArg() { result = this.getArgument(2) }
25+
26+
/** The flags argument of this call. */
27+
Argument getFlagsArg() { result = this.getArgument(3) }
28+
}
29+
30+
/** A field of the class `PendingIntent` representing a flag. */
31+
class PendingIntentFlag extends Field {
32+
PendingIntentFlag() {
33+
this.getDeclaringType() instanceof PendingIntent and
34+
this.isPublic() and
35+
this.getName().matches("FLAG_%")
36+
}
37+
}
38+
39+
/** The field `FLAG_IMMUTABLE` of the class `PendingIntent`. */
40+
class ImmutablePendingIntentFlag extends PendingIntentFlag {
41+
ImmutablePendingIntentFlag() { this.hasName("FLAG_IMMUTABLE") }
42+
}
43+
44+
/** The field `FLAG_MUTABLE` of the class `PendingIntent`. */
45+
class MutablePendingIntentFlag extends PendingIntentFlag {
46+
MutablePendingIntentFlag() { this.hasName("FLAG_MUTABLE") }
47+
}

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

Lines changed: 93 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,102 @@
11
/** Provides classes and predicates for working with implicit `PendingIntent`s. */
22

33
import java
4-
private import semmle.code.java.dataflow.DataFlow
54
private import semmle.code.java.dataflow.ExternalFlow
65
private import semmle.code.java.dataflow.FlowSteps
6+
private import semmle.code.java.dataflow.TaintTracking
7+
private import semmle.code.java.frameworks.android.Intent
8+
private import semmle.code.java.frameworks.android.PendingIntent
79

8-
private class PendingIntentCreationModels extends SinkModelCsv {
9-
override predicate row(string row) {
10-
row =
11-
[
12-
"android.app;PendingIntent;false;getActivity;;;Argument[2];pending-intent",
13-
"android.app;PendingIntent;false;getActivityAsUser;;;Argument[2];pending-intent",
14-
"android.app;PendingIntent;false;getActivities;;;Argument[2];pending-intent",
15-
"android.app;PendingIntent;false;getActivitiesAsUser;;;Argument[2];pending-intent",
16-
"android.app;PendingIntent;false;getBroadcast;;;Argument[2];pending-intent",
17-
"android.app;PendingIntent;false;getBroadcastAsUser;;;Argument[2];pending-intent",
18-
"android.app;PendingIntent;false;getService;;;Argument[2];pending-intent",
19-
"android.app;PendingIntent;false;getForegroundService;;;Argument[2];pending-intent"
20-
]
10+
/** A source for an implicit `PendingIntent` flow. */
11+
abstract class ImplicitPendingIntentSource extends DataFlow::Node {
12+
predicate hasState(DataFlow::FlowState state) { state = "" }
13+
}
14+
15+
/** A sink that sends an implicit and mutable `PendingIntent` to a third party. */
16+
abstract class ImplicitPendingIntentSink extends DataFlow::Node {
17+
predicate hasState(DataFlow::FlowState state) { state = "" }
18+
}
19+
20+
/**
21+
* A unit class for adding additional taint steps.
22+
*
23+
* Extend this class to add additional taint steps that should apply to flows related to the use
24+
* of implicit `PendingIntent`s.
25+
*/
26+
class ImplicitPendingIntentAdditionalTaintStep extends Unit {
27+
/**
28+
* Holds if the step from `node1` to `node2` should be considered a taint
29+
* step for flows related to the use of implicit `PendingIntent`s.
30+
*/
31+
predicate step(DataFlow::Node node1, DataFlow::Node node2) { none() }
32+
33+
/**
34+
* Holds if the step from `node1` to `node2` should be considered a taint
35+
* step for flows related to the use of implicit `PendingIntent`s. This step is only applicable
36+
* in `state1` and updates the flow state to `state2`.
37+
*/
38+
predicate step(
39+
DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2,
40+
DataFlow::FlowState state2
41+
) {
42+
none()
43+
}
44+
}
45+
46+
private class IntentCreationSource extends ImplicitPendingIntentSource {
47+
IntentCreationSource() {
48+
exists(ClassInstanceExpr cc |
49+
cc.getConstructedType() instanceof TypeIntent and this.asExpr() = cc
50+
)
51+
}
52+
}
53+
54+
private class SendPendingIntent extends ImplicitPendingIntentSink {
55+
SendPendingIntent() {
56+
sinkNode(this, "intent-start") and
57+
// implicit intents can't be started as services since API 21
58+
not exists(MethodAccess ma, Method m |
59+
ma.getMethod() = m and
60+
m.getDeclaringType().getASupertype*() instanceof TypeContext and
61+
m.getName().matches(["start%Service%", "bindService%"]) and
62+
this.asExpr() = ma.getArgument(0)
63+
)
64+
or
65+
sinkNode(this, "pending-intent-sent")
66+
}
67+
68+
override predicate hasState(DataFlow::FlowState state) { state = "MutablePendingIntent" }
69+
}
70+
71+
private class PendingIntentAsFieldAdditionalTaintStep extends ImplicitPendingIntentAdditionalTaintStep {
72+
override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
73+
exists(Field f |
74+
f.getType() instanceof PendingIntent and
75+
node1.(DataFlow::PostUpdateNode).getPreUpdateNode() =
76+
DataFlow::getFieldQualifier(f.getAnAccess().(FieldWrite)) and
77+
node2.asExpr().(FieldRead).getField() = f
78+
)
79+
}
80+
}
81+
82+
private class MutablePendingIntentFlowStep extends PendingIntentAsFieldAdditionalTaintStep {
83+
override predicate step(
84+
DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2,
85+
DataFlow::FlowState state2
86+
) {
87+
state1 = "" and
88+
state2 = "MutablePendingIntent" and
89+
exists(PendingIntentCreation pic, Argument flagArg |
90+
node1.asExpr() = pic.getIntentArg() and
91+
node2.asExpr() = pic and
92+
flagArg = pic.getFlagsArg()
93+
|
94+
// API < 31, PendingIntents are mutable by default
95+
not TaintTracking::localExprTaint(any(ImmutablePendingIntentFlag flag).getAnAccess(), flagArg)
96+
or
97+
// API >= 31, PendingIntents need to explicitly set mutability
98+
TaintTracking::localExprTaint(any(MutablePendingIntentFlag flag).getAnAccess(), flagArg)
99+
)
21100
}
22101
}
23102

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

Lines changed: 17 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -13,112 +13,42 @@ import semmle.code.java.security.ImplicitPendingIntents
1313
class ImplicitPendingIntentStartConf extends TaintTracking::Configuration {
1414
ImplicitPendingIntentStartConf() { this = "ImplicitPendingIntentStartConf" }
1515

16-
override predicate isSource(DataFlow::Node source) {
17-
source.asExpr() instanceof ImplicitPendingIntentCreation
16+
override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) {
17+
source.(ImplicitPendingIntentSource).hasState(state)
1818
}
1919

20-
override predicate isSink(DataFlow::Node sink) { sink instanceof SendPendingIntent }
20+
override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) {
21+
sink.(ImplicitPendingIntentSink).hasState(state)
22+
}
2123

2224
override predicate isSanitizer(DataFlow::Node sanitizer) {
2325
sanitizer instanceof ExplicitIntentSanitizer
2426
}
2527

2628
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
27-
exists(Field f |
28-
f.getType() instanceof PendingIntent and
29-
node1.(DataFlow::PostUpdateNode).getPreUpdateNode() =
30-
DataFlow::getFieldQualifier(f.getAnAccess().(FieldWrite)) and
31-
node2.asExpr().(FieldRead).getField() = f
32-
)
29+
any(ImplicitPendingIntentAdditionalTaintStep c).step(node1, node2)
30+
}
31+
32+
override predicate isAdditionalFlowStep(
33+
DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2,
34+
DataFlow::FlowState state2
35+
) {
36+
any(ImplicitPendingIntentAdditionalTaintStep c).step(node1, state1, node2, state2)
3337
}
3438

3539
override predicate allowImplicitRead(DataFlow::Node node, DataFlow::Content c) {
3640
super.allowImplicitRead(node, c)
3741
or
38-
this.isSink(node) and
42+
this.isSink(node, _) and
3943
allowIntentExtrasImplicitRead(node, c)
4044
or
4145
this.isAdditionalTaintStep(node, _) and
4246
c.(DataFlow::FieldContent).getType() instanceof PendingIntent
43-
}
44-
}
45-
46-
private class ImplicitPendingIntentCreation extends Expr {
47-
ImplicitPendingIntentCreation() {
48-
exists(Argument arg |
49-
this.getType() instanceof PendingIntent and
50-
exists(ImplicitPendingIntentConf conf | conf.hasFlowTo(DataFlow::exprNode(arg))) and
51-
arg.getCall() = this
52-
)
53-
}
54-
}
55-
56-
private class SendPendingIntent extends DataFlow::Node {
57-
SendPendingIntent() {
58-
sinkNode(this, "intent-start") and
59-
// implicit intents can't be started as services since API 21
60-
not exists(MethodAccess ma, Method m |
61-
ma.getMethod() = m and
62-
m.getDeclaringType().getASupertype*() instanceof TypeContext and
63-
m.getName().matches(["start%Service%", "bindService%"]) and
64-
this.asExpr() = ma.getArgument(0)
65-
)
6647
or
67-
sinkNode(this, "pending-intent-sent")
68-
}
69-
}
70-
71-
private class ImplicitPendingIntentConf extends DataFlow2::Configuration {
72-
ImplicitPendingIntentConf() { this = "PendingIntentConf" }
73-
74-
override predicate isSource(DataFlow::Node source) {
75-
exists(ClassInstanceExpr cc |
76-
cc.getConstructedType() instanceof TypeIntent and source.asExpr() = cc
77-
)
78-
}
79-
80-
override predicate isSink(DataFlow::Node sink) { sink instanceof MutablePendingIntentSink }
81-
82-
override predicate isBarrier(DataFlow::Node barrier) {
83-
barrier instanceof ExplicitIntentSanitizer
84-
}
85-
86-
override predicate allowImplicitRead(DataFlow::Node node, DataFlow::Content c) {
87-
// Allow implicit reads of Intent arrays for sinks like getStartActivities
88-
isSink(node) and
48+
// Allow implicit reads of Intent arrays for steps like getActivities
49+
// or sinks like startActivities
50+
(this.isSink(node, _) or this.isAdditionalFlowStep(node, _, _, _)) and
8951
node.getType().(Array).getElementType() instanceof TypeIntent and
9052
c instanceof DataFlow::ArrayContent
9153
}
9254
}
93-
94-
private class PendingIntentSink extends DataFlow::Node {
95-
PendingIntentSink() { sinkNode(this, "pending-intent") }
96-
}
97-
98-
private class MutablePendingIntentSink extends PendingIntentSink {
99-
MutablePendingIntentSink() {
100-
exists(Argument flagArg | flagArg = this.asExpr().(Argument).getCall().getArgument(3) |
101-
// API < 31, PendingIntents are mutable by default
102-
not TaintTracking::localExprTaint(any(ImmutablePendingIntentFlag flag).getAnAccess(), flagArg)
103-
or
104-
// API >= 31, PendingIntents need to explicitly set mutability
105-
TaintTracking::localExprTaint(any(MutablePendingIntentFlag flag).getAnAccess(), flagArg)
106-
)
107-
}
108-
}
109-
110-
private class PendingIntentFlag extends Field {
111-
PendingIntentFlag() {
112-
this.getDeclaringType() instanceof PendingIntent and
113-
this.isPublic() and
114-
this.getName().matches("FLAG_%")
115-
}
116-
}
117-
118-
private class ImmutablePendingIntentFlag extends PendingIntentFlag {
119-
ImmutablePendingIntentFlag() { this.hasName("FLAG_IMMUTABLE") }
120-
}
121-
122-
private class MutablePendingIntentFlag extends PendingIntentFlag {
123-
MutablePendingIntentFlag() { this.hasName("FLAG_MUTABLE") }
124-
}

‎java/ql/test/query-tests/security/CWE-927/ImplicitPendingIntentsTest.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,12 @@ public static void testPendingIntentAsAnExtra(Context ctx)
9898
ctx.startActivity(fwdIntent); // $hasImplicitPendingIntent
9999
}
100100

101+
{
102+
Intent intent = new Intent();
103+
// Testing the need of going through a PendingIntent creation (flow state)
104+
ctx.startActivity(intent); // Safe
105+
}
106+
101107
{
102108
Intent safeIntent = new Intent(ctx, Activity.class); // Sanitizer
103109
PendingIntent pi = PendingIntent.getActivity(ctx, 0, safeIntent, 0);

0 commit comments

Comments
 (0)