Skip to content

Commit 6cf3898

Browse files
Jami Cogswellatorralba
Jami Cogswell
authored and
atorralba
committed
add experimental global flow config, and clean-up some code
1 parent 9947b32 commit 6cf3898

File tree

4 files changed

+132
-99
lines changed

4 files changed

+132
-99
lines changed

‎java/ql/lib/semmle/code/java/Expr.qll

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1327,6 +1327,10 @@ class ClassInstanceExpr extends Expr, ConstructorCall, @classinstancexpr {
13271327
arg.getType() = type and
13281328
result = arg
13291329
)
1330+
// ! e.g. use above in below code in `StartActivityIntentStep` in Intent.qll
1331+
// argType.getName().matches("Class<%>") and
1332+
// newIntent.getArgumentByType(argType).getType().(ParameterizedType).getATypeArgument() =
1333+
// getIntent.getReceiverType() and
13301334
}
13311335

13321336
/**

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

Lines changed: 59 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,9 @@ private import semmle.code.java.frameworks.android.Intent
55
//private import semmle.code.java.frameworks.android.AsyncTask
66
private import semmle.code.java.frameworks.android.Android
77
private import semmle.code.java.dataflow.DataFlow
8+
//private import semmle.code.java.dataflow.DataFlow2
89
private import semmle.code.java.dataflow.FlowSteps
10+
//private import semmle.code.java.dataflow.FlowSources
911
//private import semmle.code.java.dataflow.ExternalFlow
1012
//private import semmle.code.java.dataflow.TaintTracking
1113
private import semmle.code.xml.AndroidManifest
@@ -37,31 +39,71 @@ private class DeepLinkIntentStep extends AdditionalValueStep {
3739
) and
3840
exists(AndroidComponent andComp |
3941
andComp.getAndroidComponentXmlElement().(AndroidActivityXmlElement).hasDeepLink() and
40-
n1.asExpr().getFile() = andComp.getFile() // ! ugly, see if better way to do this
42+
n1.asExpr().getFile() = andComp.getFile() // ! see if better way to do this
4143
)
4244
}
4345
}
4446

45-
// ! experimentation with global flow issue - REMOVE
46-
/**
47-
* A value-preserving step from the Intent variable
48-
* the `Intent` Parameter in the `startActivity`.
49-
*/
50-
class IntentVariableToStartActivityStep extends AdditionalValueStep {
51-
override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
52-
exists(MethodAccess startActivity, Variable intentTypeTest |
47+
// // ! experimentation with global flow issue - REMOVE
48+
// /**
49+
// * A value-preserving step from the Intent variable
50+
// * the `Intent` Parameter in the `startActivity`.
51+
// */
52+
// class IntentVariableToStartActivityStep extends AdditionalValueStep {
53+
// override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
54+
// exists(
55+
// MethodAccess startActivity, Variable intentTypeTest, DataFlow2::Node source,
56+
// DataFlow2::Node sink //ClassInstanceExpr intentTypeTest |
57+
// |
58+
// (
59+
// startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) or
60+
// startActivity.getMethod().overrides*(any(ActivityStartActivityMethod m))
61+
// ) and
62+
// intentTypeTest.getType() instanceof TypeIntent and // Variable
63+
// //intentTypeTest.getConstructedType() instanceof TypeIntent and // ClassInstanceExpr
64+
// startActivity.getFile().getBaseName() = "MainActivity.java" and // ! REMOVE - for testing only
65+
// //exists(StartComponentConfiguration cfg | cfg.hasFlow(source, sink)) and // GLOBAL FLOW ATTEMPT
66+
// DataFlow::localExprFlow(intentTypeTest.getInitializer(), startActivity.getArgument(0)) and // Variable - gives 5 results - misses the 1st ProfileActivity result since no variable with that one
67+
// //DataFlow::localExprFlow(intentTypeTest, startActivity.getArgument(0)) and // ClassInstanceExpr
68+
// n1.asExpr() = intentTypeTest.getInitializer() and // Variable
69+
// //n1.asExpr() = intentTypeTest and // ClassInstanceExpr
70+
// n2.asExpr() = startActivity.getArgument(0) // ! switch to getStartActivityIntentArg(startActivity)
71+
// )
72+
// }
73+
// }
74+
// ! rename?
75+
// ! below works as intended when run by itself (see latest query in AndroidDeeplinks_RemoteSources.ql),
76+
// ! but not when combined with existing flow steps (non-monotonic recursion)
77+
// ! need to figure out how to combine, or wrap all in global flow?
78+
class StartComponentConfiguration extends DataFlow::Configuration {
79+
StartComponentConfiguration() { this = "StartComponentConfiguration" }
80+
81+
// Override `isSource` and `isSink`.
82+
override predicate isSource(DataFlow::Node source) {
83+
exists(ClassInstanceExpr classInstanceExpr |
84+
classInstanceExpr.getConstructedType() instanceof TypeIntent and
85+
source.asExpr() = classInstanceExpr
86+
)
87+
}
88+
89+
override predicate isSink(DataFlow::Node sink) {
90+
exists(MethodAccess startActivity |
91+
// ! need to handle for all components, not just Activity
5392
(
54-
// ! is there a better way to do this?
5593
startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) or
5694
startActivity.getMethod().overrides*(any(ActivityStartActivityMethod m))
5795
) and
58-
intentTypeTest.getType() instanceof TypeIntent and
59-
//startActivity.getFile().getBaseName() = "MainActivity.java" and // ! REMOVE
60-
DataFlow::localExprFlow(intentTypeTest.getInitializer(), startActivity.getArgument(0)) and
61-
n1.asExpr() = intentTypeTest.getInitializer() and
62-
n2.asExpr() = startActivity.getArgument(0) // ! switch to getStartActivityIntentArg(startActivity)
96+
sink.asExpr() = startActivity.getArgument(0)
6397
)
6498
}
99+
// Optionally override `isBarrier`.
100+
// Optionally override `isAdditionalFlowStep`.
101+
// Then, to query whether there is flow between some `source` and `sink`,
102+
// write
103+
//
104+
// ```ql
105+
// exists(MyAnalysisConfiguration cfg | cfg.hasFlow(source, sink))
106+
// ```
65107
}
66108

67109
/* ********************* INTENT METHODS, E.G. parseUri, getData, getExtras, etc. ********************* */
@@ -95,61 +137,8 @@ class AndroidGetDataMethod extends Method {
95137
*/
96138
class AndroidParseUriMethod extends Method {
97139
AndroidParseUriMethod() {
98-
(this.hasName("parseUri") or this.hasName("getIntent")) and // ! Note to self: getIntent for older versions before deprecation to parseUri
140+
// ! Note to self: getIntent for older versions before deprecation to parseUri
141+
(this.hasName("parseUri") or this.hasName("getIntent")) and
99142
this.getDeclaringType() instanceof TypeIntent
100143
}
101144
}
102-
// /**
103-
// * A taint step from the Intent argument of a `startActivity` call to
104-
// * a `Intent.parseUri` call in the Activity the Intent pointed to in its constructor.
105-
// */
106-
// private class StartActivityParseUriStep extends AdditionalTaintStep {
107-
// override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
108-
// exists(MethodAccess startActivity, MethodAccess parseUri, ClassInstanceExpr newIntent |
109-
// startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) and
110-
// parseUri.getMethod().overrides*(any(AndroidParseUriMethod m)) and
111-
// newIntent.getConstructedType() instanceof TypeIntent and
112-
// DataFlow::localExprFlow(newIntent, startActivity.getArgument(0)) and
113-
// newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() =
114-
// parseUri.getReceiverType() and
115-
// n1.asExpr() = startActivity.getArgument(0) and
116-
// n2.asExpr() = parseUri
117-
// )
118-
// }
119-
// }
120-
// /**
121-
// * A taint step from the Intent argument of a `startActivity` call to
122-
// * a `Intent.get%Extra%` call in the Activity the Intent pointed to in its constructor.
123-
// */
124-
// private class StartActivityGetDataStep extends AdditionalTaintStep {
125-
// override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
126-
// exists(MethodAccess startActivity, MethodAccess getData, ClassInstanceExpr newIntent |
127-
// startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) and
128-
// getData.getMethod().overrides*(any(AndroidGetDataMethod m)) and
129-
// newIntent.getConstructedType() instanceof TypeIntent and
130-
// DataFlow::localExprFlow(newIntent, startActivity.getArgument(0)) and
131-
// newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() =
132-
// getData.getReceiverType() and
133-
// n1.asExpr() = startActivity.getArgument(0) and
134-
// n2.asExpr() = getData
135-
// )
136-
// }
137-
// }
138-
// /**
139-
// * A taint step from the Intent argument of a `startActivity` call to
140-
// * a `Intent.getData` call in the Activity the Intent pointed to in its constructor.
141-
// */
142-
// private class StartActivityGetExtrasStep extends AdditionalTaintStep {
143-
// override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
144-
// exists(MethodAccess startActivity, MethodAccess getExtras, ClassInstanceExpr newIntent |
145-
// startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) and
146-
// getExtras.getMethod().overrides*(any(AndroidGetExtrasMethod m)) and
147-
// newIntent.getConstructedType() instanceof TypeIntent and
148-
// DataFlow::localExprFlow(newIntent, startActivity.getArgument(0)) and
149-
// newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() =
150-
// getExtras.getReceiverType() and
151-
// n1.asExpr() = startActivity.getArgument(0) and
152-
// n2.asExpr() = getExtras
153-
// )
154-
// }
155-
// }

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

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
import java
22
private import semmle.code.java.dataflow.DataFlow
3+
//private import semmle.code.java.dataflow.DataFlow2
34
private import semmle.code.java.dataflow.ExternalFlow
45
private import semmle.code.java.dataflow.FlowSteps
6+
private import semmle.code.java.frameworks.android.DeepLink
57

6-
// ! Remember to add 'private' annotation as needed to all new classes/predicates below.
8+
// ! Remember to add 'private' annotation as needed to new classes/predicates below.
79
// ! and clean-up comments, etc. in below in general...
810
/**
911
* The class `android.content.Intent`.
@@ -119,24 +121,31 @@ class ContextStartActivityMethod extends Method {
119121
}
120122
}
121123

122-
// ! finish QLDoc
124+
// ! maybe reword below QLDoc?
123125
/**
124-
* The method `Activity.startActivity` or ...
126+
* The method `Activity.startActivity`,`startActivities`,
127+
* `startActivityForResult`, `startActivityIfNeeded`,
128+
* `startNextMatchingActivity`, `startActivityFromChild`,
129+
* or `startActivityFromFragment`.
125130
*/
126131
class ActivityStartActivityMethod extends Method {
127132
ActivityStartActivityMethod() {
128-
// ! captures all `startAct` methods in the Activity class
129-
this.getName().matches("start%Activit%") and // ! better to list all instead of using matches for any reason?
133+
this.getName().matches("start%Activit%") and
130134
this.getDeclaringType() instanceof TypeActivity
131135
}
132136
}
133137

138+
// ! maybe reword below QLDoc?
134139
/**
135-
* The method `Context.sendBroadcast`.
140+
* The method `Context.sendBroadcast`, `sendBroadcastAsUser`,
141+
* `sendOrderedBroadcast`, `sendOrderedBroadcastAsUser`,
142+
* `sendStickyBroadcast`, `sendStickyBroadcastAsUser`,
143+
* `sendStickyOrderedBroadcast`, `sendStickyOrderedBroadcastAsUser`,
144+
* or `sendBroadcastWithMultiplePermissions`.
136145
*/
137146
class ContextSendBroadcastMethod extends Method {
138147
ContextSendBroadcastMethod() {
139-
this.getName().matches("send%Broadcast%") and // ! Note to self: matches the 9 sendBroadcast methods
148+
this.getName().matches("send%Broadcast%") and
140149
this.getDeclaringType() instanceof TypeContext
141150
}
142151
}
@@ -322,7 +331,7 @@ class StartActivityIntentStep extends AdditionalValueStep {
322331
override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
323332
exists(MethodAccess startActivity, MethodAccess getIntent, ClassInstanceExpr newIntent |
324333
(
325-
// ! is there a better way to do this?
334+
// ! look for better way to handle the below
326335
startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) or
327336
startActivity.getMethod().overrides*(any(ActivityStartActivityMethod m))
328337
) and
@@ -331,10 +340,6 @@ class StartActivityIntentStep extends AdditionalValueStep {
331340
DataFlow::localExprFlow(newIntent, getStartActivityIntentArg(startActivity)) and
332341
getIntentConstructorClassArg(newIntent).getType().(ParameterizedType).getATypeArgument() =
333342
getIntent.getReceiverType() and
334-
// ! below uses predicate `getArgumentByType` that I added to the Expr.qll lib, prbly should not add new predicate there.
335-
// argType.getName().matches("Class<%>") and
336-
// newIntent.getArgumentByType(argType).getType().(ParameterizedType).getATypeArgument() =
337-
// getIntent.getReceiverType() and
338343
n1.asExpr() = getStartActivityIntentArg(startActivity) and
339344
n2.asExpr() = getIntent
340345
)

‎java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_RemoteSources.ql

Lines changed: 52 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,43 @@
1717
// where startServiceIntStep.step(n1, n2)
1818
// select n2, "placeholder"
1919
// * experiment with taint-tracking
20+
// import java
21+
// import semmle.code.java.dataflow.TaintTracking
22+
// import semmle.code.java.frameworks.android.DeepLink
23+
// import semmle.code.java.frameworks.android.Intent
24+
// import semmle.code.java.frameworks.android.Android
25+
// import semmle.code.java.dataflow.DataFlow
26+
// import semmle.code.java.dataflow.FlowSteps
27+
// import semmle.code.java.dataflow.FlowSources
28+
// import semmle.code.java.dataflow.ExternalFlow
29+
// import semmle.code.xml.AndroidManifest
30+
// import semmle.code.java.dataflow.TaintTracking
31+
// class MyTaintTrackingConfiguration extends TaintTracking::Configuration {
32+
// MyTaintTrackingConfiguration() { this = "MyTaintTrackingConfiguration" }
33+
// override predicate isSource(DataFlow::Node source) {
34+
// // exists(AndroidActivityXmlElement andActXmlElem |
35+
// // andActXmlElem.hasDeepLink() and
36+
// // source.asExpr() instanceof TypeActivity
37+
// // )
38+
// source instanceof RemoteFlowSource and //AndroidIntentInput
39+
// exists(AndroidComponent andComp |
40+
// andComp.getAndroidComponentXmlElement().(AndroidActivityXmlElement).hasDeepLink() and
41+
// source.asExpr().getFile() = andComp.getFile() // ! ugly, see if better way to do this
42+
// )
43+
// }
44+
// override predicate isSink(DataFlow::Node sink) {
45+
// exists(MethodAccess m |
46+
// m.getMethod().hasName("getIntent") and
47+
// sink.asExpr() = m
48+
// )
49+
// }
50+
// }
51+
// from DataFlow::Node src, DataFlow::Node sink, MyTaintTrackingConfiguration config
52+
// where config.hasFlow(src, sink)
53+
// select src, "This environment variable constructs a URL $@.", sink, "here"
54+
// * experiment with GLOBAL FLOW
2055
import java
2156
import semmle.code.java.dataflow.TaintTracking
22-
import semmle.code.java.frameworks.android.DeepLink
2357
import semmle.code.java.frameworks.android.Intent
2458
import semmle.code.java.frameworks.android.Android
2559
import semmle.code.java.dataflow.DataFlow
@@ -29,29 +63,30 @@ import semmle.code.java.dataflow.ExternalFlow
2963
import semmle.code.xml.AndroidManifest
3064
import semmle.code.java.dataflow.TaintTracking
3165

32-
class MyTaintTrackingConfiguration extends TaintTracking::Configuration {
33-
MyTaintTrackingConfiguration() { this = "MyTaintTrackingConfiguration" }
66+
class StartComponentConfiguration extends DataFlow::Configuration {
67+
StartComponentConfiguration() { this = "StartComponentConfiguration" }
3468

69+
// TODO: Override `isSource` and `isSink`.
3570
override predicate isSource(DataFlow::Node source) {
36-
// exists(AndroidActivityXmlElement andActXmlElem |
37-
// andActXmlElem.hasDeepLink() and
38-
// source.asExpr() instanceof TypeActivity
39-
// )
40-
source instanceof RemoteFlowSource and //AndroidIntentInput
41-
exists(AndroidComponent andComp |
42-
andComp.getAndroidComponentXmlElement().(AndroidActivityXmlElement).hasDeepLink() and
43-
source.asExpr().getFile() = andComp.getFile() // ! ugly, see if better way to do this
71+
exists(ClassInstanceExpr classInstanceExpr |
72+
classInstanceExpr.getConstructedType() instanceof TypeIntent and
73+
source.asExpr() = classInstanceExpr
4474
)
4575
}
4676

4777
override predicate isSink(DataFlow::Node sink) {
48-
exists(MethodAccess m |
49-
m.getMethod().hasName("getIntent") and
50-
sink.asExpr() = m
78+
exists(MethodAccess startActivity |
79+
(
80+
startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) or
81+
startActivity.getMethod().overrides*(any(ActivityStartActivityMethod m))
82+
) and
83+
sink.asExpr() = startActivity.getArgument(0)
5184
)
5285
}
5386
}
5487

55-
from DataFlow::Node src, DataFlow::Node sink, MyTaintTrackingConfiguration config
56-
where config.hasFlow(src, sink)
57-
select src, "This environment variable constructs a URL $@.", sink, "here"
88+
from DataFlow::Node src, DataFlow::Node sink, StartComponentConfiguration config
89+
where
90+
config.hasFlow(src, sink) and
91+
sink.asExpr().getFile().getBaseName() = "MainActivity.java" // ! just for faster testing, remove when done
92+
select src, "This source flows to this $@.", sink, "sink"

0 commit comments

Comments
 (0)