Merge pull request #9975 from atorralba/atorralba/asynctask-improvs

Java: Improve AsyncTask data flow support
2026-04-18 21:44:02 +02:00 · 2022-08-09 17:10:09 +02:00
parent add9e9dac4 98b930cd67
commit 7f5fe85e2e
5 changed files with 111 additions and 31 deletions
--- a/java/ql/lib/change-notes/2022-08-05-asynctask-improvements.md
+++ b/java/ql/lib/change-notes/2022-08-05-asynctask-improvements.md
@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* 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.
--- a/java/ql/lib/semmle/code/java/frameworks/android/AsyncTask.qll
+++ b/java/ql/lib/semmle/code/java/frameworks/android/AsyncTask.qll
@@ -4,19 +4,62 @@ import java
 private import semmle.code.java.dataflow.DataFlow
 private import semmle.code.java.dataflow.FlowSteps

-/**
- * Models the value-preserving step from `asyncTask.execute(params)` to `AsyncTask::doInBackground(params)`.
+/*
+ * The following flow steps aim to model the life-cycle of `AsyncTask`s described here:
+ * https://developer.android.com/reference/android/os/AsyncTask#the-4-steps
 */
-private class AsyncTaskAdditionalValueStep extends AdditionalValueStep {
+
+/**
+ * A taint step from the vararg arguments of `AsyncTask::execute` and `AsyncTask::executeOnExecutor`
+ * to the parameter of `AsyncTask::doInBackground`.
+ */
+private class AsyncTaskExecuteAdditionalValueStep extends AdditionalTaintStep {
  override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
    exists(ExecuteAsyncTaskMethodAccess ma, AsyncTaskRunInBackgroundMethod m |
-      DataFlow::getInstanceArgument(ma).getType() = m.getDeclaringType() and
+      DataFlow::getInstanceArgument(ma).getType() = m.getDeclaringType()
+    |
      node1.asExpr() = ma.getParamsArgument() and
      node2.asParameter() = m.getParameter(0)
    )
  }
 }

+/**
+ * A value-preserving step from the return value of `AsyncTask::doInBackground`
+ * to the parameter of `AsyncTask::onPostExecute`.
+ */
+private class AsyncTaskOnPostExecuteAdditionalValueStep extends AdditionalValueStep {
+  override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
+    exists(
+      AsyncTaskRunInBackgroundMethod runInBackground, AsyncTaskOnPostExecuteMethod onPostExecute
+    |
+      onPostExecute.getDeclaringType() = runInBackground.getDeclaringType()
+    |
+      node1.asExpr() = any(ReturnStmt r | r.getEnclosingCallable() = runInBackground).getResult() and
+      node2.asParameter() = onPostExecute.getParameter(0)
+    )
+  }
+}
+
+/**
+ * A value-preserving step from field initializers in `AsyncTask`'s constructor or initializer method
+ * to the instance parameter of `AsyncTask::runInBackground` and `AsyncTask::onPostExecute`.
+ */
+private class AsyncTaskFieldInitQualifierToInstanceParameterStep extends AdditionalValueStep {
+  override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
+    exists(AsyncTaskInit init, Callable receiver |
+      n1.(DataFlow::PostUpdateNode).getPreUpdateNode() =
+        DataFlow::getFieldQualifier(any(FieldWrite f | f.getEnclosingCallable() = init)) and
+      n2.(DataFlow::InstanceParameterNode).getCallable() = receiver and
+      receiver.getDeclaringType() = init.getDeclaringType() and
+      (
+        receiver instanceof AsyncTaskRunInBackgroundMethod or
+        receiver instanceof AsyncTaskOnPostExecuteMethod
+      )
+    )
+  }
+}
+
 /**
 * The Android class `android.os.AsyncTask`.
 */
@@ -24,29 +67,38 @@ private class AsyncTask extends RefType {
  AsyncTask() { this.hasQualifiedName("android.os", "AsyncTask") }
 }

+/** The constructor or initializer method of the `android.os.AsyncTask` class. */
+private class AsyncTaskInit extends Callable {
+  AsyncTaskInit() {
+    this.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof AsyncTask and
+    (this instanceof Constructor or this instanceof InitializerMethod)
+  }
+}
+
 /** A call to the `execute` or `executeOnExecutor` methods of the `android.os.AsyncTask` class. */
 private class ExecuteAsyncTaskMethodAccess extends MethodAccess {
-  Argument paramsArgument;
-
  ExecuteAsyncTaskMethodAccess() {
-    exists(Method m |
-      this.getMethod() = m and
-      m.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof AsyncTask
-    |
-      m.getName() = "execute" and not m.isStatic() and paramsArgument = this.getArgument(0)
-      or
-      m.getName() = "executeOnExecutor" and paramsArgument = this.getArgument(1)
-    )
+    this.getMethod().hasName(["execute", "executeOnExecutor"]) and
+    this.getMethod().getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof
+      AsyncTask
  }

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

 /** The `doInBackground` method of the `android.os.AsyncTask` class. */
 private class AsyncTaskRunInBackgroundMethod extends Method {
  AsyncTaskRunInBackgroundMethod() {
    this.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof AsyncTask and
-    this.getName() = "doInBackground"
+    this.hasName("doInBackground")
+  }
+}
+
+/** The `onPostExecute` method of the `android.os.AsyncTask` class. */
+private class AsyncTaskOnPostExecuteMethod extends Method {
+  AsyncTaskOnPostExecuteMethod() {
+    this.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof AsyncTask and
+    this.hasName("onPostExecute")
  }
 }
--- a/java/ql/test/experimental/query-tests/security/CWE-200/SensitiveAndroidFileLeak.expected
+++ b/java/ql/test/experimental/query-tests/security/CWE-200/SensitiveAndroidFileLeak.expected
@@ -4,17 +4,13 @@ edges
 | FileService.java:21:28:21:64 | getStringExtra(...) : Object | FileService.java:25:42:25:50 | localPath : Object |
 | FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] | FileService.java:40:41:40:55 | params : Object[] |
 | FileService.java:25:13:25:51 | makeParamsToExecute(...) [[]] : Object | FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] |
-| FileService.java:25:13:25:51 | makeParamsToExecute(...) [[]] : Object | FileService.java:40:41:40:55 | params [[]] : Object |
 | FileService.java:25:42:25:50 | localPath : Object | FileService.java:25:13:25:51 | makeParamsToExecute(...) [[]] : Object |
 | FileService.java:25:42:25:50 | localPath : Object | FileService.java:32:13:32:28 | sourceUri : Object |
 | FileService.java:32:13:32:28 | sourceUri : Object | FileService.java:35:17:35:25 | sourceUri : Object |
 | FileService.java:34:20:36:13 | {...} [[]] : Object | FileService.java:34:20:36:13 | new Object[] [[]] : Object |
 | FileService.java:35:17:35:25 | sourceUri : Object | FileService.java:34:20:36:13 | {...} [[]] : Object |
 | FileService.java:40:41:40:55 | params : Object[] | FileService.java:44:33:44:52 | (...)... : Object |
-| FileService.java:40:41:40:55 | params [[]] : Object | FileService.java:44:44:44:49 | params [[]] : Object |
 | FileService.java:44:33:44:52 | (...)... : Object | FileService.java:45:53:45:59 | ...[...] |
-| FileService.java:44:44:44:49 | params [[]] : Object | FileService.java:44:44:44:52 | ...[...] : Object |
-| FileService.java:44:44:44:52 | ...[...] : Object | FileService.java:44:33:44:52 | (...)... : Object |
 | LeakFileActivity2.java:15:13:15:18 | intent : Intent | LeakFileActivity2.java:16:26:16:31 | intent : Intent |
 | LeakFileActivity2.java:16:26:16:31 | intent : Intent | FileService.java:20:31:20:43 | intent : Intent |
 | LeakFileActivity.java:14:35:14:38 | data : Intent | LeakFileActivity.java:18:40:18:59 | contentIntent : Intent |
@@ -34,10 +30,7 @@ nodes
 | FileService.java:34:20:36:13 | {...} [[]] : Object | semmle.label | {...} [[]] : Object |
 | FileService.java:35:17:35:25 | sourceUri : Object | semmle.label | sourceUri : Object |
 | FileService.java:40:41:40:55 | params : Object[] | semmle.label | params : Object[] |
-| FileService.java:40:41:40:55 | params [[]] : Object | semmle.label | params [[]] : Object |
 | FileService.java:44:33:44:52 | (...)... : Object | semmle.label | (...)... : Object |
-| FileService.java:44:44:44:49 | params [[]] : Object | semmle.label | params [[]] : Object |
-| FileService.java:44:44:44:52 | ...[...] : Object | semmle.label | ...[...] : Object |
 | FileService.java:45:53:45:59 | ...[...] | semmle.label | ...[...] |
 | LeakFileActivity2.java:15:13:15:18 | intent : Intent | semmle.label | intent : Intent |
 | LeakFileActivity2.java:16:26:16:31 | intent : Intent | semmle.label | intent : Intent |
--- a/java/ql/test/library-tests/frameworks/android/asynctask/Test.java
+++ b/java/ql/test/library-tests/frameworks/android/asynctask/Test.java
@@ -10,16 +10,19 @@ public class Test {

    public void test() {
        TestAsyncTask t = new TestAsyncTask();
-        t.execute(source("execute"));
-        t.executeOnExecutor(null, source("executeOnExecutor"));
+        t.execute(source("execute"), null);
+        t.executeOnExecutor(null, source("executeOnExecutor"), null);
        SafeAsyncTask t2 = new SafeAsyncTask();
        t2.execute("safe");
+        TestConstructorTask t3 = new TestConstructorTask(source("constructor"), "safe");
+        t3.execute(source("params"));
    }

    private class TestAsyncTask extends AsyncTask<Object, Object, Object> {
        @Override
        protected Object doInBackground(Object... params) {
-            sink(params); // $ hasValueFlow=execute hasValueFlow=executeOnExecutor
+            sink(params[0]); // $ hasTaintFlow=execute hasTaintFlow=executeOnExecutor
+            sink(params[1]); // $ SPURIOUS: hasTaintFlow=execute hasTaintFlow=executeOnExecutor
            return null;
        }
    }
@@ -27,8 +30,40 @@ public class Test {
    private class SafeAsyncTask extends AsyncTask<Object, Object, Object> {
        @Override
        protected Object doInBackground(Object... params) {
-            sink(params); // Safe
+            sink(params[0]); // Safe
            return null;
        }
    }
+
+    static class TestConstructorTask extends AsyncTask<Object, Object, Object> {
+        private Object field;
+        private Object safeField;
+        private Object initField;
+        {
+            initField = Test.source("init");
+        }
+
+        public TestConstructorTask(Object field, Object safeField) {
+            this.field = field;
+            this.safeField = safeField;
+        }
+
+        @Override
+        protected Object doInBackground(Object... params) {
+            sink(params[0]); // $ hasTaintFlow=params
+            sink(field); // $ hasValueFlow=constructor
+            sink(safeField); // Safe
+            sink(initField); // $ hasValueFlow=init
+            return params[0];
+        }
+
+        @Override
+        protected void onPostExecute(Object param) {
+            sink(param); // $ hasTaintFlow=params
+            sink(field); // $ hasValueFlow=constructor
+            sink(safeField); // Safe
+            sink(initField); // $ hasValueFlow=init
+        }
+
+    }
 }
--- a/java/ql/test/library-tests/frameworks/android/asynctask/test.ql
+++ b/java/ql/test/library-tests/frameworks/android/asynctask/test.ql
@@ -1,6 +1,2 @@
 import java
 import TestUtilities.InlineFlowTest
-
-class AsyncTaskTest extends InlineFlowTest {
-  override TaintTracking::Configuration getTaintFlowConfig() { none() }
-}