Skip to content

Commit 8c2fddb

Browse files
luchua-bcsmowton
luchua-bc
authored and
smowton
committed
Update the condition check and use DataFlow in the ql file
1 parent b0e652a commit 8c2fddb

File tree

4 files changed

+35
-48
lines changed

4 files changed

+35
-48
lines changed

‎java/ql/src/experimental/Security/CWE/CWE-200/AndroidFileIntentSink.qll

Lines changed: 10 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -19,30 +19,20 @@ class AsyncTask extends RefType {
1919
AsyncTask() { this.hasQualifiedName("android.os", "AsyncTask") }
2020
}
2121

22-
/** The method that executes `AsyncTask` of Android. */
23-
abstract class ExecuteAsyncTaskMethod extends Method {
24-
/** Returns index of the parameter that is tainted. */
25-
abstract int getParamIndex();
26-
}
27-
28-
/** The `execute` method of Android `AsyncTask`. */
29-
class AsyncTaskExecuteMethod extends ExecuteAsyncTaskMethod {
30-
AsyncTaskExecuteMethod() {
31-
this.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof AsyncTask and
32-
this.getName() = "execute"
33-
}
34-
35-
override int getParamIndex() { result = 0 }
36-
}
22+
/** The `execute` or `executeOnExecutor` method of Android `AsyncTask`. */
23+
class ExecuteAsyncTaskMethod extends Method {
24+
int paramIndex;
3725

38-
/** The `executeOnExecutor` method of Android `AsyncTask`. */
39-
class AsyncTaskExecuteOnExecutorMethod extends ExecuteAsyncTaskMethod {
40-
AsyncTaskExecuteOnExecutorMethod() {
26+
ExecuteAsyncTaskMethod() {
4127
this.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof AsyncTask and
42-
this.getName() = "executeOnExecutor"
28+
(
29+
this.getName() = "execute" and paramIndex = 0
30+
or
31+
this.getName() = "executeOnExecutor" and paramIndex = 1
32+
)
4333
}
4434

45-
override int getParamIndex() { result = 1 }
35+
int getParamIndex() { result = paramIndex }
4636
}
4737

4838
/** The `doInBackground` method of Android `AsyncTask`. */

‎java/ql/src/experimental/Security/CWE/CWE-200/AndroidFileIntentSource.qll

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import java
44
import semmle.code.java.dataflow.FlowSources
5+
import semmle.code.java.dataflow.TaintTracking2
56
import semmle.code.java.frameworks.android.Android
67

78
/** The `startActivityForResult` method of Android `Activity`. */
@@ -67,14 +68,14 @@ private class AndroidIntentDataModel extends SummaryModelCsv {
6768
}
6869

6970
/** Taint configuration for getting content intent. */
70-
class GetContentIntentConfig extends TaintTracking::Configuration {
71+
class GetContentIntentConfig extends TaintTracking2::Configuration {
7172
GetContentIntentConfig() { this = "GetContentIntentConfig" }
7273

73-
override predicate isSource(DataFlow::Node src) {
74+
override predicate isSource(DataFlow2::Node src) {
7475
exists(GetContentIntent gi | src.asExpr() = gi)
7576
}
7677

77-
override predicate isSink(DataFlow::Node sink) {
78+
override predicate isSink(DataFlow2::Node sink) {
7879
exists(MethodAccess ma |
7980
ma.getMethod() instanceof StartActivityForResultMethod and sink.asExpr() = ma.getArgument(0)
8081
)

‎java/ql/src/experimental/Security/CWE/CWE-200/SensitiveAndroidFileLeak.ql

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -4,17 +4,18 @@
44
* Android configuration file and sensitive user data.
55
* @kind path-problem
66
* @id java/sensitive-android-file-leak
7+
* @problem.severity warning
78
* @tags security
89
* external/cwe/cwe-200
910
*/
1011

1112
import java
13+
import semmle.code.java.controlflow.Guards
1214
import AndroidFileIntentSink
1315
import AndroidFileIntentSource
14-
import DataFlow2::PathGraph
15-
import semmle.code.java.dataflow.TaintTracking2
16+
import DataFlow::PathGraph
1617

17-
private class StartsWithSanitizer extends DataFlow2::BarrierGuard {
18+
private class StartsWithSanitizer extends DataFlow::BarrierGuard {
1819
StartsWithSanitizer() { this.(MethodAccess).getMethod().hasName("startsWith") }
1920

2021
override predicate checks(Expr e, boolean branch) {
@@ -27,27 +28,32 @@ private class StartsWithSanitizer extends DataFlow2::BarrierGuard {
2728
}
2829
}
2930

30-
class AndroidFileLeakConfig extends TaintTracking2::Configuration {
31+
class AndroidFileLeakConfig extends TaintTracking::Configuration {
3132
AndroidFileLeakConfig() { this = "AndroidFileLeakConfig" }
3233

33-
/** Holds if it is an access to file intent result. */
34-
override predicate isSource(DataFlow2::Node src) {
34+
/**
35+
* Holds if `src` is a read of some Intent-typed method argument guarded by a check like
36+
* `requestCode == REQUEST_CODE__SELECT_CONTENT_FROM_APPS`, where `requestCode` is the first
37+
* argument to `Activity.onActivityResult`.
38+
*/
39+
override predicate isSource(DataFlow::Node src) {
3540
exists(
36-
AndroidActivityResultInput ai, AndroidFileIntentInput fi, IfStmt ifs, VarAccess intentVar // if (requestCode == REQUEST_CODE__SELECT_CONTENT_FROM_APPS)
41+
AndroidActivityResultInput ai, AndroidFileIntentInput fi, ConditionBlock cb,
42+
VarAccess intentVar
3743
|
38-
ifs.getCondition().getAChildExpr().getAChildExpr().(CompileTimeConstantExpr).getIntValue() =
44+
cb.getCondition().getAChildExpr().(CompileTimeConstantExpr).getIntValue() =
3945
fi.getRequestCode() and
40-
ifs.getCondition().getAChildExpr().getAChildExpr() = ai.getRequestCodeVar() and
46+
cb.getCondition().getAChildExpr() = ai.getRequestCodeVar() and
4147
intentVar.getType() instanceof TypeIntent and
42-
intentVar.(Argument).getAnEnclosingStmt() = ifs.getThen() and
48+
cb.getBasicBlock() = intentVar.(Argument).getAnEnclosingStmt() and
4349
src.asExpr() = intentVar
4450
)
4551
}
4652

4753
/** Holds if it is a sink of file access in Android. */
48-
override predicate isSink(DataFlow2::Node sink) { sink instanceof AndroidFileSink }
54+
override predicate isSink(DataFlow::Node sink) { sink instanceof AndroidFileSink }
4955

50-
override predicate isAdditionalTaintStep(DataFlow2::Node prev, DataFlow2::Node succ) {
56+
override predicate isAdditionalTaintStep(DataFlow::Node prev, DataFlow::Node succ) {
5157
exists(MethodAccess aema, AsyncTaskRunInBackgroundMethod arm |
5258
// fileAsyncTask.execute(params) will invoke doInBackground(params) of FileAsyncTask
5359
aema.getQualifier().getType() = arm.getDeclaringType() and
@@ -60,18 +66,18 @@ class AndroidFileLeakConfig extends TaintTracking2::Configuration {
6066
csma.getMethod() instanceof ContextStartServiceMethod and
6167
ce.getConstructedType() instanceof TypeIntent and // Intent intent = new Intent(context, FileUploader.class);
6268
ce.getArgument(1).(TypeLiteral).getReferencedType() = ssm.getDeclaringType() and
63-
DataFlow2::localExprFlow(ce, csma.getArgument(0)) and // context.startService(intent);
69+
DataFlow::localExprFlow(ce, csma.getArgument(0)) and // context.startService(intent);
6470
prev.asExpr() = csma.getArgument(0) and
6571
succ.asParameter() = ssm.getParameter(0) // public int onStartCommand(Intent intent, int flags, int startId) {...} in FileUploader
6672
)
6773
}
6874

69-
override predicate isSanitizerGuard(DataFlow2::BarrierGuard guard) {
75+
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
7076
guard instanceof StartsWithSanitizer
7177
}
7278
}
7379

74-
from DataFlow2::PathNode source, DataFlow2::PathNode sink, AndroidFileLeakConfig conf
80+
from DataFlow::PathNode source, DataFlow::PathNode sink, AndroidFileLeakConfig conf
7581
where conf.hasFlowPath(source, sink)
7682
select sink.getNode(), source, sink, "Leaking arbitrary Android file from $@.", source.getNode(),
7783
"this user input"

‎java/ql/test/experimental/query-tests/security/CWE-200/SensitiveAndroidFileLeak.expected

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,6 @@ edges
66
| FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] | FileService.java:40:41:40:55 | params : Object[] |
77
| FileService.java:25:13:25:51 | makeParamsToExecute(...) [[]] : String | FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] |
88
| FileService.java:25:42:25:50 | localPath : String | FileService.java:25:13:25:51 | makeParamsToExecute(...) [[]] : String |
9-
| FileService.java:25:42:25:50 | localPath : String | FileService.java:32:13:32:28 | sourceUri : String |
10-
| FileService.java:32:13:32:28 | sourceUri : String | FileService.java:35:17:35:25 | sourceUri : String |
11-
| FileService.java:34:20:36:13 | {...} [[]] : String | FileService.java:34:20:36:13 | new Object[] [[]] : String |
12-
| FileService.java:35:17:35:25 | sourceUri : String | FileService.java:34:20:36:13 | {...} [[]] : String |
139
| FileService.java:40:41:40:55 | params : Object[] | FileService.java:44:33:44:52 | (...)... : Object |
1410
| FileService.java:44:33:44:52 | (...)... : Object | FileService.java:45:53:45:59 | ...[...] |
1511
| LeakFileActivity2.java:16:26:16:31 | intent : Intent | FileService.java:20:31:20:43 | intent : Intent |
@@ -25,10 +21,6 @@ nodes
2521
| FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] | semmle.label | makeParamsToExecute(...) : Object[] |
2622
| FileService.java:25:13:25:51 | makeParamsToExecute(...) [[]] : String | semmle.label | makeParamsToExecute(...) [[]] : String |
2723
| FileService.java:25:42:25:50 | localPath : String | semmle.label | localPath : String |
28-
| FileService.java:32:13:32:28 | sourceUri : String | semmle.label | sourceUri : String |
29-
| FileService.java:34:20:36:13 | new Object[] [[]] : String | semmle.label | new Object[] [[]] : String |
30-
| FileService.java:34:20:36:13 | {...} [[]] : String | semmle.label | {...} [[]] : String |
31-
| FileService.java:35:17:35:25 | sourceUri : String | semmle.label | sourceUri : String |
3224
| FileService.java:40:41:40:55 | params : Object[] | semmle.label | params : Object[] |
3325
| FileService.java:44:33:44:52 | (...)... : Object | semmle.label | (...)... : Object |
3426
| FileService.java:45:53:45:59 | ...[...] | semmle.label | ...[...] |
@@ -39,8 +31,6 @@ nodes
3931
| LeakFileActivity.java:19:31:19:53 | getData(...) : Uri | semmle.label | getData(...) : Uri |
4032
| LeakFileActivity.java:21:58:21:72 | streamsToUpload : Uri | semmle.label | streamsToUpload : Uri |
4133
| LeakFileActivity.java:21:58:21:82 | getPath(...) | semmle.label | getPath(...) |
42-
subpaths
43-
| FileService.java:25:42:25:50 | localPath : String | FileService.java:32:13:32:28 | sourceUri : String | FileService.java:34:20:36:13 | new Object[] [[]] : String | FileService.java:25:13:25:51 | makeParamsToExecute(...) [[]] : String |
4434
#select
4535
| FileService.java:45:53:45:59 | ...[...] | LeakFileActivity2.java:16:26:16:31 | intent : Intent | FileService.java:45:53:45:59 | ...[...] | Leaking arbitrary Android file from $@. | LeakFileActivity2.java:16:26:16:31 | intent | this user input |
4636
| LeakFileActivity.java:21:58:21:82 | getPath(...) | LeakFileActivity.java:14:35:14:38 | data : Intent | LeakFileActivity.java:21:58:21:82 | getPath(...) | Leaking arbitrary Android file from $@. | LeakFileActivity.java:14:35:14:38 | data | this user input |

0 commit comments

Comments
 (0)