Skip to content

Commit efcc696

Browse files
geoffw0geoffw0
committed
Merge branch 'main' into defaulttaint
2 parents 36f410b + f2fead7 commit efcc696

File tree

16 files changed

+257
-37
lines changed

16 files changed

+257
-37
lines changed

‎java/ql/lib/change-notes/2022-08-05-asynctask-improvements.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Improved analysis of the Android class `AsyncTask` so that data can properly flow through its methods according to the life-cycle steps described here: https://developer.android.com/reference/android/os/AsyncTask#the-4-steps.

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

Lines changed: 67 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -4,49 +4,101 @@ import java
44
private import semmle.code.java.dataflow.DataFlow
55
private import semmle.code.java.dataflow.FlowSteps
66

7+
/*
8+
* The following flow steps aim to model the life-cycle of `AsyncTask`s described here:
9+
* https://developer.android.com/reference/android/os/AsyncTask#the-4-steps
10+
*/
11+
712
/**
8-
* Models the value-preserving step from `asyncTask.execute(params)` to `AsyncTask::doInBackground(params)`.
13+
* A taint step from the vararg arguments of `AsyncTask::execute` and `AsyncTask::executeOnExecutor`
14+
* to the parameter of `AsyncTask::doInBackground`.
915
*/
10-
private class AsyncTaskAdditionalValueStep extends AdditionalValueStep {
16+
private class AsyncTaskExecuteAdditionalValueStep extends AdditionalTaintStep {
1117
override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
1218
exists(ExecuteAsyncTaskMethodAccess ma, AsyncTaskRunInBackgroundMethod m |
13-
DataFlow::getInstanceArgument(ma).getType() = m.getDeclaringType() and
19+
DataFlow::getInstanceArgument(ma).getType() = m.getDeclaringType()
20+
|
1421
node1.asExpr() = ma.getParamsArgument() and
1522
node2.asParameter() = m.getParameter(0)
1623
)
1724
}
1825
}
1926

27+
/**
28+
* A value-preserving step from the return value of `AsyncTask::doInBackground`
29+
* to the parameter of `AsyncTask::onPostExecute`.
30+
*/
31+
private class AsyncTaskOnPostExecuteAdditionalValueStep extends AdditionalValueStep {
32+
override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
33+
exists(
34+
AsyncTaskRunInBackgroundMethod runInBackground, AsyncTaskOnPostExecuteMethod onPostExecute
35+
|
36+
onPostExecute.getDeclaringType() = runInBackground.getDeclaringType()
37+
|
38+
node1.asExpr() = any(ReturnStmt r | r.getEnclosingCallable() = runInBackground).getResult() and
39+
node2.asParameter() = onPostExecute.getParameter(0)
40+
)
41+
}
42+
}
43+
44+
/**
45+
* A value-preserving step from field initializers in `AsyncTask`'s constructor or initializer method
46+
* to the instance parameter of `AsyncTask::runInBackground` and `AsyncTask::onPostExecute`.
47+
*/
48+
private class AsyncTaskFieldInitQualifierToInstanceParameterStep extends AdditionalValueStep {
49+
override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
50+
exists(AsyncTaskInit init, Callable receiver |
51+
n1.(DataFlow::PostUpdateNode).getPreUpdateNode() =
52+
DataFlow::getFieldQualifier(any(FieldWrite f | f.getEnclosingCallable() = init)) and
53+
n2.(DataFlow::InstanceParameterNode).getCallable() = receiver and
54+
receiver.getDeclaringType() = init.getDeclaringType() and
55+
(
56+
receiver instanceof AsyncTaskRunInBackgroundMethod or
57+
receiver instanceof AsyncTaskOnPostExecuteMethod
58+
)
59+
)
60+
}
61+
}
62+
2063
/**
2164
* The Android class `android.os.AsyncTask`.
2265
*/
2366
private class AsyncTask extends RefType {
2467
AsyncTask() { this.hasQualifiedName("android.os", "AsyncTask") }
2568
}
2669

70+
/** The constructor or initializer method of the `android.os.AsyncTask` class. */
71+
private class AsyncTaskInit extends Callable {
72+
AsyncTaskInit() {
73+
this.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof AsyncTask and
74+
(this instanceof Constructor or this instanceof InitializerMethod)
75+
}
76+
}
77+
2778
/** A call to the `execute` or `executeOnExecutor` methods of the `android.os.AsyncTask` class. */
2879
private class ExecuteAsyncTaskMethodAccess extends MethodAccess {
29-
Argument paramsArgument;
30-
3180
ExecuteAsyncTaskMethodAccess() {
32-
exists(Method m |
33-
this.getMethod() = m and
34-
m.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof AsyncTask
35-
|
36-
m.getName() = "execute" and not m.isStatic() and paramsArgument = this.getArgument(0)
37-
or
38-
m.getName() = "executeOnExecutor" and paramsArgument = this.getArgument(1)
39-
)
81+
this.getMethod().hasName(["execute", "executeOnExecutor"]) and
82+
this.getMethod().getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof
83+
AsyncTask
4084
}
4185

4286
/** Returns the `params` argument of this call. */
43-
Argument getParamsArgument() { result = paramsArgument }
87+
Argument getParamsArgument() { result = this.getAnArgument() and result.isVararg() }
4488
}
4589

4690
/** The `doInBackground` method of the `android.os.AsyncTask` class. */
4791
private class AsyncTaskRunInBackgroundMethod extends Method {
4892
AsyncTaskRunInBackgroundMethod() {
4993
this.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof AsyncTask and
50-
this.getName() = "doInBackground"
94+
this.hasName("doInBackground")
95+
}
96+
}
97+
98+
/** The `onPostExecute` method of the `android.os.AsyncTask` class. */
99+
private class AsyncTaskOnPostExecuteMethod extends Method {
100+
AsyncTaskOnPostExecuteMethod() {
101+
this.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof AsyncTask and
102+
this.hasName("onPostExecute")
51103
}
52104
}

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

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,17 +4,13 @@ edges
44
| FileService.java:21:28:21:64 | getStringExtra(...) : Object | FileService.java:25:42:25:50 | localPath : Object |
55
| FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] | FileService.java:40:41:40:55 | params : Object[] |
66
| FileService.java:25:13:25:51 | makeParamsToExecute(...) [[]] : Object | FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] |
7-
| FileService.java:25:13:25:51 | makeParamsToExecute(...) [[]] : Object | FileService.java:40:41:40:55 | params [[]] : Object |
87
| FileService.java:25:42:25:50 | localPath : Object | FileService.java:25:13:25:51 | makeParamsToExecute(...) [[]] : Object |
98
| FileService.java:25:42:25:50 | localPath : Object | FileService.java:32:13:32:28 | sourceUri : Object |
109
| FileService.java:32:13:32:28 | sourceUri : Object | FileService.java:35:17:35:25 | sourceUri : Object |
1110
| FileService.java:34:20:36:13 | {...} [[]] : Object | FileService.java:34:20:36:13 | new Object[] [[]] : Object |
1211
| FileService.java:35:17:35:25 | sourceUri : Object | FileService.java:34:20:36:13 | {...} [[]] : Object |
1312
| FileService.java:40:41:40:55 | params : Object[] | FileService.java:44:33:44:52 | (...)... : Object |
14-
| FileService.java:40:41:40:55 | params [[]] : Object | FileService.java:44:44:44:49 | params [[]] : Object |
1513
| FileService.java:44:33:44:52 | (...)... : Object | FileService.java:45:53:45:59 | ...[...] |
16-
| FileService.java:44:44:44:49 | params [[]] : Object | FileService.java:44:44:44:52 | ...[...] : Object |
17-
| FileService.java:44:44:44:52 | ...[...] : Object | FileService.java:44:33:44:52 | (...)... : Object |
1814
| LeakFileActivity2.java:15:13:15:18 | intent : Intent | LeakFileActivity2.java:16:26:16:31 | intent : Intent |
1915
| LeakFileActivity2.java:16:26:16:31 | intent : Intent | FileService.java:20:31:20:43 | intent : Intent |
2016
| LeakFileActivity.java:14:35:14:38 | data : Intent | LeakFileActivity.java:18:40:18:59 | contentIntent : Intent |
@@ -34,10 +30,7 @@ nodes
3430
| FileService.java:34:20:36:13 | {...} [[]] : Object | semmle.label | {...} [[]] : Object |
3531
| FileService.java:35:17:35:25 | sourceUri : Object | semmle.label | sourceUri : Object |
3632
| FileService.java:40:41:40:55 | params : Object[] | semmle.label | params : Object[] |
37-
| FileService.java:40:41:40:55 | params [[]] : Object | semmle.label | params [[]] : Object |
3833
| FileService.java:44:33:44:52 | (...)... : Object | semmle.label | (...)... : Object |
39-
| FileService.java:44:44:44:49 | params [[]] : Object | semmle.label | params [[]] : Object |
40-
| FileService.java:44:44:44:52 | ...[...] : Object | semmle.label | ...[...] : Object |
4134
| FileService.java:45:53:45:59 | ...[...] | semmle.label | ...[...] |
4235
| LeakFileActivity2.java:15:13:15:18 | intent : Intent | semmle.label | intent : Intent |
4336
| LeakFileActivity2.java:16:26:16:31 | intent : Intent | semmle.label | intent : Intent |

‎java/ql/test/library-tests/frameworks/android/asynctask/Test.java

Lines changed: 39 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,25 +10,60 @@ private static void sink(Object o) {}
1010

1111
public void test() {
1212
TestAsyncTask t = new TestAsyncTask();
13-
t.execute(source("execute"));
14-
t.executeOnExecutor(null, source("executeOnExecutor"));
13+
t.execute(source("execute"), null);
14+
t.executeOnExecutor(null, source("executeOnExecutor"), null);
1515
SafeAsyncTask t2 = new SafeAsyncTask();
1616
t2.execute("safe");
17+
TestConstructorTask t3 = new TestConstructorTask(source("constructor"), "safe");
18+
t3.execute(source("params"));
1719
}
1820

1921
private class TestAsyncTask extends AsyncTask<Object, Object, Object> {
2022
@Override
2123
protected Object doInBackground(Object... params) {
22-
sink(params); // $ hasValueFlow=execute hasValueFlow=executeOnExecutor
24+
sink(params[0]); // $ hasTaintFlow=execute hasTaintFlow=executeOnExecutor
25+
sink(params[1]); // $ SPURIOUS: hasTaintFlow=execute hasTaintFlow=executeOnExecutor
2326
return null;
2427
}
2528
}
2629

2730
private class SafeAsyncTask extends AsyncTask<Object, Object, Object> {
2831
@Override
2932
protected Object doInBackground(Object... params) {
30-
sink(params); // Safe
33+
sink(params[0]); // Safe
3134
return null;
3235
}
3336
}
37+
38+
static class TestConstructorTask extends AsyncTask<Object, Object, Object> {
39+
private Object field;
40+
private Object safeField;
41+
private Object initField;
42+
{
43+
initField = Test.source("init");
44+
}
45+
46+
public TestConstructorTask(Object field, Object safeField) {
47+
this.field = field;
48+
this.safeField = safeField;
49+
}
50+
51+
@Override
52+
protected Object doInBackground(Object... params) {
53+
sink(params[0]); // $ hasTaintFlow=params
54+
sink(field); // $ hasValueFlow=constructor
55+
sink(safeField); // Safe
56+
sink(initField); // $ hasValueFlow=init
57+
return params[0];
58+
}
59+
60+
@Override
61+
protected void onPostExecute(Object param) {
62+
sink(param); // $ hasTaintFlow=params
63+
sink(field); // $ hasValueFlow=constructor
64+
sink(safeField); // Safe
65+
sink(initField); // $ hasValueFlow=init
66+
}
67+
68+
}
3469
}

‎java/ql/test/library-tests/frameworks/android/asynctask/test.ql

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,2 @@
11
import java
22
import TestUtilities.InlineFlowTest
3-
4-
class AsyncTaskTest extends InlineFlowTest {
5-
override TaintTracking::Configuration getTaintFlowConfig() { none() }
6-
}

‎javascript/ql/lib/semmle/javascript/JSON.qll

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,9 @@ class JsonValue extends @json_value, Locatable {
5454
int getIntValue() { result = this.(JsonNumber).getValue().toInt() }
5555

5656
/** If this is a boolean constant, gets its boolean value. */
57-
boolean getBooleanValue() { result.toString() = this.(JsonBoolean).getValue() }
57+
boolean getBooleanValue() {
58+
result.toString() = this.(JsonBoolean).getValue() and result = [true, false]
59+
}
5860

5961
override string getAPrimaryQlClass() { result = "JsonValue" }
6062
}

‎javascript/ql/lib/semmle/javascript/NodeModuleResolutionImpl.qll

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,11 @@ private string getASrcFolderName() { result = ["ts", "js", "src", "lib"] }
148148
class MainModulePath extends PathExpr, @json_string {
149149
PackageJson pkg;
150150

151-
MainModulePath() { this = pkg.getPropValue(["main", "module"]) }
151+
MainModulePath() {
152+
this = pkg.getPropValue(["main", "module"])
153+
or
154+
this = getAJsonChild*(pkg.getPropValue("exports"))
155+
}
152156

153157
/** Gets the `package.json` file in which this path occurs. */
154158
PackageJson getPackageJson() { result = pkg }
@@ -164,6 +168,9 @@ class MainModulePath extends PathExpr, @json_string {
164168
}
165169
}
166170

171+
/** Gets the value of a property from the JSON object `obj`. */
172+
private JsonValue getAJsonChild(JsonObject obj) { result = obj.getPropValue(_) }
173+
167174
module MainModulePath {
168175
MainModulePath of(PackageJson pkg) { result.getPackageJson() = pkg }
169176
}

‎javascript/ql/test/query-tests/Performance/ReDoS/PolynomialBackTracking.expected

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@
3838
| lib/snapdragon.js:15:26:15:27 | a* | Strings starting with 'a' and with many repetitions of 'a' can start matching anywhere after the start of the preceeding aa*$ |
3939
| lib/snapdragon.js:23:22:23:23 | a* | Strings starting with 'a' and with many repetitions of 'a' can start matching anywhere after the start of the preceeding aa*$ |
4040
| lib/subLib4/factory.js:8:3:8:4 | f* | Strings with many repetitions of 'f' can start matching anywhere after the start of the preceeding f*g |
41+
| lib/subLib5/feature.js:2:3:2:4 | a* | Strings with many repetitions of 'a' can start matching anywhere after the start of the preceeding a*b |
42+
| lib/subLib5/main.js:2:3:2:4 | a* | Strings with many repetitions of 'a' can start matching anywhere after the start of the preceeding a*b |
4143
| lib/sublib/factory.js:13:14:13:15 | f* | Strings with many repetitions of 'f' can start matching anywhere after the start of the preceeding f*g |
4244
| polynomial-redos.js:7:24:7:26 | \\s+ | Strings with many repetitions of ' ' can start matching anywhere after the start of the preceeding \\s+$ |
4345
| polynomial-redos.js:8:17:8:18 | * | Strings with many repetitions of ' ' can start matching anywhere after the start of the preceeding *, * |

‎javascript/ql/test/query-tests/Performance/ReDoS/PolynomialReDoS.expected

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,14 @@ nodes
5151
| lib/subLib4/factory.js:7:27:7:30 | name |
5252
| lib/subLib4/factory.js:8:13:8:16 | name |
5353
| lib/subLib4/factory.js:8:13:8:16 | name |
54+
| lib/subLib5/feature.js:1:28:1:31 | name |
55+
| lib/subLib5/feature.js:1:28:1:31 | name |
56+
| lib/subLib5/feature.js:2:13:2:16 | name |
57+
| lib/subLib5/feature.js:2:13:2:16 | name |
58+
| lib/subLib5/main.js:1:28:1:31 | name |
59+
| lib/subLib5/main.js:1:28:1:31 | name |
60+
| lib/subLib5/main.js:2:13:2:16 | name |
61+
| lib/subLib5/main.js:2:13:2:16 | name |
5462
| lib/sublib/factory.js:12:26:12:29 | name |
5563
| lib/sublib/factory.js:12:26:12:29 | name |
5664
| lib/sublib/factory.js:13:24:13:27 | name |
@@ -256,6 +264,14 @@ edges
256264
| lib/subLib4/factory.js:7:27:7:30 | name | lib/subLib4/factory.js:8:13:8:16 | name |
257265
| lib/subLib4/factory.js:7:27:7:30 | name | lib/subLib4/factory.js:8:13:8:16 | name |
258266
| lib/subLib4/factory.js:7:27:7:30 | name | lib/subLib4/factory.js:8:13:8:16 | name |
267+
| lib/subLib5/feature.js:1:28:1:31 | name | lib/subLib5/feature.js:2:13:2:16 | name |
268+
| lib/subLib5/feature.js:1:28:1:31 | name | lib/subLib5/feature.js:2:13:2:16 | name |
269+
| lib/subLib5/feature.js:1:28:1:31 | name | lib/subLib5/feature.js:2:13:2:16 | name |
270+
| lib/subLib5/feature.js:1:28:1:31 | name | lib/subLib5/feature.js:2:13:2:16 | name |
271+
| lib/subLib5/main.js:1:28:1:31 | name | lib/subLib5/main.js:2:13:2:16 | name |
272+
| lib/subLib5/main.js:1:28:1:31 | name | lib/subLib5/main.js:2:13:2:16 | name |
273+
| lib/subLib5/main.js:1:28:1:31 | name | lib/subLib5/main.js:2:13:2:16 | name |
274+
| lib/subLib5/main.js:1:28:1:31 | name | lib/subLib5/main.js:2:13:2:16 | name |
259275
| lib/sublib/factory.js:12:26:12:29 | name | lib/sublib/factory.js:13:24:13:27 | name |
260276
| lib/sublib/factory.js:12:26:12:29 | name | lib/sublib/factory.js:13:24:13:27 | name |
261277
| lib/sublib/factory.js:12:26:12:29 | name | lib/sublib/factory.js:13:24:13:27 | name |
@@ -418,6 +434,8 @@ edges
418434
| lib/snapdragon.js:15:13:15:30 | this.match(/aa*$/) | lib/snapdragon.js:12:34:12:38 | input | lib/snapdragon.js:15:13:15:16 | this | This $@ that depends on $@ may run slow on strings starting with 'a' and with many repetitions of 'a'. | lib/snapdragon.js:15:26:15:27 | a* | regular expression | lib/snapdragon.js:12:34:12:38 | input | library input |
419435
| lib/snapdragon.js:23:5:23:26 | node.va ... /aa*$/) | lib/snapdragon.js:20:34:20:38 | input | lib/snapdragon.js:23:5:23:12 | node.val | This $@ that depends on $@ may run slow on strings starting with 'a' and with many repetitions of 'a'. | lib/snapdragon.js:23:22:23:23 | a* | regular expression | lib/snapdragon.js:20:34:20:38 | input | library input |
420436
| lib/subLib4/factory.js:8:2:8:17 | /f*g/.test(name) | lib/subLib4/factory.js:7:27:7:30 | name | lib/subLib4/factory.js:8:13:8:16 | name | This $@ that depends on $@ may run slow on strings with many repetitions of 'f'. | lib/subLib4/factory.js:8:3:8:4 | f* | regular expression | lib/subLib4/factory.js:7:27:7:30 | name | library input |
437+
| lib/subLib5/feature.js:2:2:2:17 | /a*b/.test(name) | lib/subLib5/feature.js:1:28:1:31 | name | lib/subLib5/feature.js:2:13:2:16 | name | This $@ that depends on $@ may run slow on strings with many repetitions of 'a'. | lib/subLib5/feature.js:2:3:2:4 | a* | regular expression | lib/subLib5/feature.js:1:28:1:31 | name | library input |
438+
| lib/subLib5/main.js:2:2:2:17 | /a*b/.test(name) | lib/subLib5/main.js:1:28:1:31 | name | lib/subLib5/main.js:2:13:2:16 | name | This $@ that depends on $@ may run slow on strings with many repetitions of 'a'. | lib/subLib5/main.js:2:3:2:4 | a* | regular expression | lib/subLib5/main.js:1:28:1:31 | name | library input |
421439
| lib/sublib/factory.js:13:13:13:28 | /f*g/.test(name) | lib/sublib/factory.js:12:26:12:29 | name | lib/sublib/factory.js:13:24:13:27 | name | This $@ that depends on $@ may run slow on strings with many repetitions of 'f'. | lib/sublib/factory.js:13:14:13:15 | f* | regular expression | lib/sublib/factory.js:12:26:12:29 | name | library input |
422440
| polynomial-redos.js:7:2:7:34 | tainted ... /g, '') | polynomial-redos.js:5:16:5:32 | req.query.tainted | polynomial-redos.js:7:2:7:8 | tainted | This $@ that depends on $@ may run slow on strings with many repetitions of ' '. | polynomial-redos.js:7:24:7:26 | \\s+ | regular expression | polynomial-redos.js:5:16:5:32 | req.query.tainted | a user-provided value |
423441
| polynomial-redos.js:8:2:8:23 | tainted ... *, */) | polynomial-redos.js:5:16:5:32 | req.query.tainted | polynomial-redos.js:8:2:8:8 | tainted | This $@ that depends on $@ may run slow on strings with many repetitions of ' '. | polynomial-redos.js:8:17:8:18 | * | regular expression | polynomial-redos.js:5:16:5:32 | req.query.tainted | a user-provided value |

‎javascript/ql/test/query-tests/Performance/ReDoS/lib/subLib5/feature.js

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
module.exports = function (name) {
2+
/a*b/.test(name); // NOT OK
3+
};

0 commit comments

Comments
 (0)