From 757604721428cf8c1a420e0b9127b4b3ecee253a Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Mon, 29 Aug 2022 09:31:26 -0400 Subject: [PATCH 01/20] create simple query and initial experimentation --- .../code/java/frameworks/android/DeepLink.qll | 173 ++++++++++++++++++ .../lib/semmle/code/xml/AndroidManifest.qll | 23 +++ .../CWE-939/AndroidDeeplinks_RemoteSources.ql | 126 +++++++++++++ .../CWE-939/AndroidDeeplinks_SimpleQuery.ql | 20 ++ .../frameworks/android/deeplink/Test.java | 72 ++++++++ .../frameworks/android/deeplink/options | 1 + .../frameworks/android/deeplink/test.expected | 0 .../frameworks/android/deeplink/test.ql | 2 + 8 files changed, 417 insertions(+) create mode 100644 java/ql/lib/semmle/code/java/frameworks/android/DeepLink.qll create mode 100644 java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_RemoteSources.ql create mode 100644 java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_SimpleQuery.ql create mode 100644 java/ql/test/library-tests/frameworks/android/deeplink/Test.java create mode 100644 java/ql/test/library-tests/frameworks/android/deeplink/options create mode 100644 java/ql/test/library-tests/frameworks/android/deeplink/test.expected create mode 100644 java/ql/test/library-tests/frameworks/android/deeplink/test.ql diff --git a/java/ql/lib/semmle/code/java/frameworks/android/DeepLink.qll b/java/ql/lib/semmle/code/java/frameworks/android/DeepLink.qll new file mode 100644 index 00000000000..2409dc5e86d --- /dev/null +++ b/java/ql/lib/semmle/code/java/frameworks/android/DeepLink.qll @@ -0,0 +1,173 @@ +/** Provides classes and predicates to reason about deep links in Android. */ + +import java +private import semmle.code.java.frameworks.android.Intent +private import semmle.code.java.frameworks.android.AsyncTask +private import semmle.code.java.dataflow.DataFlow +private import semmle.code.java.dataflow.FlowSteps +private import semmle.code.java.dataflow.ExternalFlow + +/** + * The method `Intent.getSerializableExtra` + */ +class AndroidGetSerializableExtraMethod extends Method { + AndroidGetSerializableExtraMethod() { + this.hasName("getSerializableExtra") and this.getDeclaringType() instanceof TypeIntent + } +} + +/** + * The method `Context.startService`. + */ +class ContextStartServiceMethod extends Method { + ContextStartServiceMethod() { + this.hasName("startService") and + this.getDeclaringType() instanceof TypeContext + } +} + +// /** +// * A value-preserving step from the Intent argument of a `startService` call to +// * a `getSerializableExtra` call in the Service the Intent pointed to in its constructor. +// */ +// class StartServiceIntentStep extends AdditionalValueStep { +// override predicate step(DataFlow::Node n1, DataFlow::Node n2) { +// exists( +// MethodAccess startService, MethodAccess getSerializableExtra, ClassInstanceExpr newIntent +// | +// startService.getMethod().overrides*(any(ContextStartServiceMethod m)) and +// getSerializableExtra.getMethod().overrides*(any(AndroidGetSerializableExtraMethod m)) and +// newIntent.getConstructedType() instanceof TypeIntent and +// DataFlow::localExprFlow(newIntent, startService.getArgument(0)) and +// //newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = +// // getSerializableExtra.getReceiverType() and +// newIntent.getArgument(1).toString() = "FetcherService.class" and // BAD +// getSerializableExtra.getFile().getBaseName() = "RouterActivity.java" and // BAD +// n1.asExpr() = startService.getArgument(0) and +// n2.asExpr() = getSerializableExtra +// ) +// } +// } +/** + * A value-preserving step from the Intent argument of a `startService` call to + * an `Intent` TypeAccess in the Service the Intent pointed to in its constructor. + */ +class StartServiceIntentStep extends AdditionalValueStep { + override predicate step(DataFlow::Node n1, DataFlow::Node n2) { + exists(MethodAccess startService, VarAccess intentVar, ClassInstanceExpr newIntent | + startService.getMethod().overrides*(any(ContextStartServiceMethod m)) and + //getSerializableExtra.getMethod().overrides*(any(AndroidGetSerializableExtraMethod m)) and + intentVar.getType() instanceof TypeIntent and + newIntent.getConstructedType() instanceof TypeIntent and + DataFlow::localExprFlow(newIntent, startService.getArgument(0)) and + // newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = + // intentVar.getBasicBlock().getBasicBlock() and + // newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = + // intent.getType().(ParameterizedType).getATypeArgument() and + newIntent.getArgument(1).toString() = "FetcherService.class" and // BAD + intentVar.getFile().getBaseName() = "RouterActivity.java" and // BAD + n1.asExpr() = startService.getArgument(0) and + n2.asExpr() = intentVar + ) + } +} + +// ************************************************************************************************* +/* + * 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 + */ + +/** + * 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() + | + 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`. + */ +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 { + ExecuteAsyncTaskMethodAccess() { + this.getMethod().hasName(["execute", "executeOnExecutor"]) and + this.getMethod().getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof + AsyncTask + } + + /** Returns the `params` argument of this call. */ + 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.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") + } +} diff --git a/java/ql/lib/semmle/code/xml/AndroidManifest.qll b/java/ql/lib/semmle/code/xml/AndroidManifest.qll index f53da67a650..a96b15b65d3 100644 --- a/java/ql/lib/semmle/code/xml/AndroidManifest.qll +++ b/java/ql/lib/semmle/code/xml/AndroidManifest.qll @@ -129,6 +129,29 @@ class AndroidApplicationXmlElement extends XmlElement { */ class AndroidActivityXmlElement extends AndroidComponentXmlElement { AndroidActivityXmlElement() { this.getName() = "activity" } + + // ! Double-check that no other components can have deep links. + // ! Consider moving this to its own .qll file like for Implicit Export Query. + // ! Also double-check that the below actions and categories are REQUIRED for it to + // ! count as a deep link versus just recommended (e.g. should I just look for the + // ! data element instead?). + /** + * Holds if this `` element has a deep link. + */ + predicate hasDeepLink() { + //exists(this.getAnIntentFilterElement()) and // has an intent filter - below all show that it has an intent-filter, duplicates work + this.getAnIntentFilterElement().getAnActionElement().getActionName() = + "android.intent.action.VIEW" and + this.getAnIntentFilterElement().getACategoryElement().getCategoryName() = + "android.intent.category.BROWSABLE" and + this.getAnIntentFilterElement().getACategoryElement().getCategoryName() = + "android.intent.category.DEFAULT" and + //this.getAnIntentFilterElement().getAChild("data").hasAttribute("scheme") // use below instead for 'android' prefix + exists(AndroidXmlAttribute attr | + this.getAnIntentFilterElement().getAChild("data").getAnAttribute() = attr and + attr.getName() = "scheme" + ) + } } /** diff --git a/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_RemoteSources.ql b/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_RemoteSources.ql new file mode 100644 index 00000000000..1ab6c0f1c8d --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_RemoteSources.ql @@ -0,0 +1,126 @@ +/** + * @name Android deep links + * @description Android deep links + * @problem.severity recommendation + * @security-severity 0.1 + * @id java/android/deeplinks + * @tags security + * external/cwe/cwe-939 + * @precision high + */ + +// import java +// import semmle.code.xml.AndroidManifest +// import semmle.code.java.frameworks.android.Android +// import semmle.code.java.frameworks.android.Intent +// import semmle.code.java.frameworks.android.AsyncTask +// import semmle.code.java.frameworks.android.DeepLink +// import semmle.code.java.dataflow.DataFlow +// import semmle.code.java.dataflow.TaintTracking +// import semmle.code.java.dataflow.FlowSources +// import semmle.code.java.dataflow.FlowSteps +// import semmle.code.java.dataflow.ExternalFlow +//* select getData() method access in RouterActivity +// from AndroidComponent andComp, MethodAccess ma +// where +// andComp +// .getAndroidComponentXmlElement() +// .getAnIntentFilterElement() +// .getAnActionElement() +// .getActionName() = "android.intent.action.VIEW" and +// andComp +// .getAndroidComponentXmlElement() +// .getAnIntentFilterElement() +// .getACategoryElement() +// .getCategoryName() = "android.intent.category.BROWSABLE" and +// andComp +// .getAndroidComponentXmlElement() +// .getAnIntentFilterElement() +// .getACategoryElement() +// .getCategoryName() = "android.intent.category.DEFAULT" and +// andComp +// .getAndroidComponentXmlElement() +// .getAnIntentFilterElement() +// .getAChild("data") +// .hasAttribute("scheme") and // make sure to check for 'android' prefix in real query +// ma.getMethod().hasName("getData") and +// //ma.getCompilationUnit().toString() = andComp.toString() // string is "RouterActivity" +// andComp.getFile() = ma.getFile() +// select ma, "getData usage related to deeplink" +// * play with taint/data +// class DeepLinkConfiguration extends TaintTracking::Configuration { +// DeepLinkConfiguration() { this = "DeepLinkConfiguration" } +// override predicate isSource(DataFlow::Node source) { +// exists(MethodAccess ma, AndroidComponent andComp | +// ma.getMethod().hasName("getData") and +// andComp +// .getAndroidComponentXmlElement() +// .getAnIntentFilterElement() +// .getAnActionElement() +// .getActionName() = "android.intent.action.VIEW" and +// andComp +// .getAndroidComponentXmlElement() +// .getAnIntentFilterElement() +// .getACategoryElement() +// .getCategoryName() = "android.intent.category.BROWSABLE" and +// andComp +// .getAndroidComponentXmlElement() +// .getAnIntentFilterElement() +// .getACategoryElement() +// .getCategoryName() = "android.intent.category.DEFAULT" and +// andComp +// .getAndroidComponentXmlElement() +// .getAnIntentFilterElement() +// .getAChild("data") +// .hasAttribute("scheme") and +// andComp.getFile() = ma.getFile() and +// source.asExpr() = ma +// ) +// } +// override predicate isSink(DataFlow::Node sink) { +// exists(Variable v | v.hasName("currentUrl") and sink.asExpr() = v.getAnAccess()) +// } +// } +// from DataFlow::Node src, DataFlow::Node sink, DeepLinkConfiguration config +// where config.hasFlow(src, sink) +// select src, "This environment variable constructs a URL $@.", sink, "here" +// * Intent experimentation: +//from +// AndroidComponent andComp, MethodAccess ma, StartActivityIntentStep startActIntStep, +// DataFlow::Node n1, DataFlow::Node n2 +// where +// andComp +// .getAndroidComponentXmlElement() +// .getAnIntentFilterElement() +// .getAnActionElement() +// .getActionName() = "android.intent.action.VIEW" and +// andComp +// .getAndroidComponentXmlElement() +// .getAnIntentFilterElement() +// .getACategoryElement() +// .getCategoryName() = "android.intent.category.BROWSABLE" and +// andComp +// .getAndroidComponentXmlElement() +// .getAnIntentFilterElement() +// .getACategoryElement() +// .getCategoryName() = "android.intent.category.DEFAULT" and +// andComp +// .getAndroidComponentXmlElement() +// .getAnIntentFilterElement() +// .getAChild("data") +// .hasAttribute("scheme") and // make sure to check for 'android' prefix in real query +// //ma.getMethod().hasName("getData") and +// //andComp.getFile() = ma.getFile() and +// //n1.asExpr() = ma and +// //n1.asExpr() = ma and +// andComp.getAnAnnotation() = n1.asExpr() and +// startActIntStep.step(n1, n2) +// select n1, "deeplink" +// * experiment with StartActivityIntentStep +import java +import semmle.code.java.frameworks.android.DeepLink +import semmle.code.java.dataflow.DataFlow + +from StartServiceIntentStep startServiceIntStep, DataFlow::Node n1, DataFlow::Node n2 +where startServiceIntStep.step(n1, n2) +select n2, "placeholder" diff --git a/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_SimpleQuery.ql b/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_SimpleQuery.ql new file mode 100644 index 00000000000..62dc035c7ed --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_SimpleQuery.ql @@ -0,0 +1,20 @@ +/** + * @name Android deep links + * @description Android deep links + * @kind problem + * @problem.severity recommendation + * @security-severity 0.1 + * @id java/android/deeplinks + * @tags security + * external/cwe/cwe-939 + * @precision high + */ + +import java +import semmle.code.xml.AndroidManifest + +from AndroidActivityXmlElement actXmlElement +where + actXmlElement.hasDeepLink() and + not actXmlElement.getFile().(AndroidManifestXmlFile).isInBuildDirectory() +select actXmlElement, "A deeplink is used here." diff --git a/java/ql/test/library-tests/frameworks/android/deeplink/Test.java b/java/ql/test/library-tests/frameworks/android/deeplink/Test.java new file mode 100644 index 00000000000..752129d045b --- /dev/null +++ b/java/ql/test/library-tests/frameworks/android/deeplink/Test.java @@ -0,0 +1,72 @@ + +// !!! From AsyncTask, update for DeepLinks... !!! + +import android.os.AsyncTask; + +public class Test { + + private static Object source(String kind) { + return null; + } + + private static void sink(Object o) {} + + public void test() { + TestAsyncTask t = new TestAsyncTask(); + 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 { + @Override + protected Object doInBackground(Object... params) { + sink(params[0]); // $ hasTaintFlow=execute hasTaintFlow=executeOnExecutor + sink(params[1]); // $ SPURIOUS: hasTaintFlow=execute hasTaintFlow=executeOnExecutor + return null; + } + } + + private class SafeAsyncTask extends AsyncTask { + @Override + protected Object doInBackground(Object... params) { + sink(params[0]); // Safe + return null; + } + } + + static class TestConstructorTask extends AsyncTask { + 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 + } + + } +} diff --git a/java/ql/test/library-tests/frameworks/android/deeplink/options b/java/ql/test/library-tests/frameworks/android/deeplink/options new file mode 100644 index 00000000000..33cdc1ea940 --- /dev/null +++ b/java/ql/test/library-tests/frameworks/android/deeplink/options @@ -0,0 +1 @@ +//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/google-android-9.0.0 diff --git a/java/ql/test/library-tests/frameworks/android/deeplink/test.expected b/java/ql/test/library-tests/frameworks/android/deeplink/test.expected new file mode 100644 index 00000000000..e69de29bb2d diff --git a/java/ql/test/library-tests/frameworks/android/deeplink/test.ql b/java/ql/test/library-tests/frameworks/android/deeplink/test.ql new file mode 100644 index 00000000000..5d91e4e8e26 --- /dev/null +++ b/java/ql/test/library-tests/frameworks/android/deeplink/test.ql @@ -0,0 +1,2 @@ +import java +import TestUtilities.InlineFlowTest From 11ce910c38ad4ab80fee04d172156612bdeadd16 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Wed, 28 Sep 2022 16:30:23 -0400 Subject: [PATCH 02/20] resolved merge conflict in FlowSources --- .../code/java/frameworks/android/DeepLink.qll | 357 ++++++++++++------ .../CWE-939/AndroidDeeplinks_SimpleQuery.ql | 13 +- 2 files changed, 253 insertions(+), 117 deletions(-) diff --git a/java/ql/lib/semmle/code/java/frameworks/android/DeepLink.qll b/java/ql/lib/semmle/code/java/frameworks/android/DeepLink.qll index 2409dc5e86d..e0108214ccf 100644 --- a/java/ql/lib/semmle/code/java/frameworks/android/DeepLink.qll +++ b/java/ql/lib/semmle/code/java/frameworks/android/DeepLink.qll @@ -3,171 +3,304 @@ import java private import semmle.code.java.frameworks.android.Intent private import semmle.code.java.frameworks.android.AsyncTask +private import semmle.code.java.frameworks.android.Android private import semmle.code.java.dataflow.DataFlow private import semmle.code.java.dataflow.FlowSteps private import semmle.code.java.dataflow.ExternalFlow +// ! Remember to add 'private' annotation as needed to all new classes/predicates below. +/* ********* OTHER COMPONENTS (SERVICE, RECEIVER) *********** */ /** - * The method `Intent.getSerializableExtra` + * The class `android.app.Service`. */ -class AndroidGetSerializableExtraMethod extends Method { - AndroidGetSerializableExtraMethod() { - this.hasName("getSerializableExtra") and this.getDeclaringType() instanceof TypeIntent - } +class TypeService extends Class { + TypeService() { this.hasQualifiedName("android.app", "Service") } } /** - * The method `Context.startService`. + * The method `Context.startService` or `Context.startForegroundService`. */ class ContextStartServiceMethod extends Method { ContextStartServiceMethod() { - this.hasName("startService") and + (this.hasName("startService") or this.hasName("startForegroundService")) and this.getDeclaringType() instanceof TypeContext } } -// /** -// * A value-preserving step from the Intent argument of a `startService` call to -// * a `getSerializableExtra` call in the Service the Intent pointed to in its constructor. -// */ -// class StartServiceIntentStep extends AdditionalValueStep { -// override predicate step(DataFlow::Node n1, DataFlow::Node n2) { -// exists( -// MethodAccess startService, MethodAccess getSerializableExtra, ClassInstanceExpr newIntent -// | -// startService.getMethod().overrides*(any(ContextStartServiceMethod m)) and -// getSerializableExtra.getMethod().overrides*(any(AndroidGetSerializableExtraMethod m)) and -// newIntent.getConstructedType() instanceof TypeIntent and -// DataFlow::localExprFlow(newIntent, startService.getArgument(0)) and -// //newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = -// // getSerializableExtra.getReceiverType() and -// newIntent.getArgument(1).toString() = "FetcherService.class" and // BAD -// getSerializableExtra.getFile().getBaseName() = "RouterActivity.java" and // BAD -// n1.asExpr() = startService.getArgument(0) and -// n2.asExpr() = getSerializableExtra -// ) -// } -// } +/** + * A method of type Service that receives an Intent as a parameter. + * Namely: `Service.onStart`, `Service.onStartCommand`, + * `Service.onBind`, `Service.onRebind` + * `Service.onUnbind`, or + * `Service.onTaskRemoved` + */ +class AndroidServeIntentMethod extends Method { + AndroidServeIntentMethod() { + ( + this.hasName("onStart") or + this.hasName("onStartCommand") or + // this.getName.matches("onStart%") or // could maybe switch with the above + this.hasName("onBind") or + this.hasName("onRebind") or + this.hasName("onUnbind") or + // this.getName.matches("on%bind") or // could maybe switch with the above + this.hasName("onTaskRemoved") + ) and + this.getDeclaringType() instanceof TypeService + } +} + +/** + * The method `Service.onStart`, `Service.onStartCommand`, + * `Service.onBind`, `Service.onRebind` + * `Service.onUnbind`, or + * `Service.onTaskRemoved` + */ +class AndroidServeIntentMethod2 extends Method { + AndroidServeIntentMethod2() { + ( + this instanceof ServiceOnStartMethod or + this instanceof ServiceOnBindMethod or + this instanceof ServiceOnUnbindMethod or + this instanceof ServiceOnTaskRemovedMethod + ) and + this.getDeclaringType() instanceof TypeService + } +} + +/** + * The method `Service.onStart` or `Service.onStartCommand`. + */ +class ServiceOnStartMethod extends Method { + ServiceOnStartMethod() { + (this.hasName("onStart") or this.hasName("onStartCommand")) and + this.getDeclaringType() instanceof TypeService + } +} + +/** + * The method `Service.onBind` or `Service.onRebind`. + */ +class ServiceOnBindMethod extends Method { + ServiceOnBindMethod() { + (this.hasName("onBind") or this.hasName("onRebind")) and + this.getDeclaringType() instanceof TypeService + } +} + +/** + * The method `Service.onUnbind`. + */ +class ServiceOnUnbindMethod extends Method { + ServiceOnUnbindMethod() { + this.hasName("onUnbind") and + this.getDeclaringType() instanceof TypeService + } +} + +/** + * The method `Service.onTaskRemoved`. + */ +class ServiceOnTaskRemovedMethod extends Method { + ServiceOnTaskRemovedMethod() { + this.hasName("onTaskRemoved") and + this.getDeclaringType() instanceof TypeService + } +} + +// ! update QLDoc more if needed (e.g. remove `onStart` reference) /** * A value-preserving step from the Intent argument of a `startService` call to - * an `Intent` TypeAccess in the Service the Intent pointed to in its constructor. + * the `Intent` Parameter in the `onStart` method of the Service the Intent pointed + * to in its constructor. */ class StartServiceIntentStep extends AdditionalValueStep { override predicate step(DataFlow::Node n1, DataFlow::Node n2) { - exists(MethodAccess startService, VarAccess intentVar, ClassInstanceExpr newIntent | + exists(MethodAccess startService, Method onStart, ClassInstanceExpr newIntent | startService.getMethod().overrides*(any(ContextStartServiceMethod m)) and - //getSerializableExtra.getMethod().overrides*(any(AndroidGetSerializableExtraMethod m)) and - intentVar.getType() instanceof TypeIntent and + //onStart.overrides*(any(ServiceOnStartMethod m)) and + //onStart.overrides*(any(AndroidServeIntentMethod m)) and + onStart.overrides*(any(AndroidServeIntentMethod2 m)) and newIntent.getConstructedType() instanceof TypeIntent and DataFlow::localExprFlow(newIntent, startService.getArgument(0)) and - // newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = - // intentVar.getBasicBlock().getBasicBlock() and - // newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = - // intent.getType().(ParameterizedType).getATypeArgument() and - newIntent.getArgument(1).toString() = "FetcherService.class" and // BAD - intentVar.getFile().getBaseName() = "RouterActivity.java" and // BAD + newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = + onStart.getDeclaringType() and n1.asExpr() = startService.getArgument(0) and - n2.asExpr() = intentVar + n2.asParameter() = onStart.getParameter(0) ) } } -// ************************************************************************************************* -/* - * 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 - */ - +// /** +// * The class `android.content.BroadcastReceiver`. +// */ +// class TypeBroadcastReceiver extends Class { +// TypeBroadcastReceiver() { this.hasQualifiedName("android.content", "BroadcastReceiver") } +// } /** - * A taint step from the vararg arguments of `AsyncTask::execute` and `AsyncTask::executeOnExecutor` - * to the parameter of `AsyncTask::doInBackground`. + * The method `Context.sendBroadcast`. */ -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() - | - node1.asExpr() = ma.getParamsArgument() and - node2.asParameter() = m.getParameter(0) - ) +class ContextSendBroadcastMethod extends Method { + ContextSendBroadcastMethod() { + this.getName().matches("send%Broadcast%") and // ! double-check this - seems to work based on quick-eval + this.getDeclaringType() instanceof TypeContext } } +// ! do I need to model this as well? /** - * A value-preserving step from the return value of `AsyncTask::doInBackground` - * to the parameter of `AsyncTask::onPostExecute`. + * The method `BroadcastReceiver.peekService`. */ -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) - ) +class BroadcastReceiverPeekServiceIntentMethod extends Method { + BroadcastReceiverPeekServiceIntentMethod() { + this.hasName("peekService") and this.getDeclaringType() instanceof TypeBroadcastReceiver } } +// /** +// * The method `BroadcastReceiver.onReceive`. +// */ +// class AndroidReceiveIntentMethod extends Method { +// AndroidReceiveIntentMethod() { +// this.hasName("onReceive") and this.getDeclaringType() instanceof TypeBroadcastReceiver +// } +// } /** - * A value-preserving step from field initializers in `AsyncTask`'s constructor or initializer method - * to the instance parameter of `AsyncTask::runInBackground` and `AsyncTask::onPostExecute`. + * A value-preserving step from the Intent argument of a `sendBroadcast` call to + * the `Intent` Parameter in the `onStart` method of the BroadcastReceiver the + * Intent pointed to in its constructor. */ -private class AsyncTaskFieldInitQualifierToInstanceParameterStep extends AdditionalValueStep { +class SendBroadcastReceiverIntentStep 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 - ) + exists(MethodAccess sendBroadcast, Method onReceive, ClassInstanceExpr newIntent | + sendBroadcast.getMethod().overrides*(any(ContextSendBroadcastMethod m)) and + onReceive.overrides*(any(AndroidReceiveIntentMethod m)) and + newIntent.getConstructedType() instanceof TypeIntent and + DataFlow::localExprFlow(newIntent, sendBroadcast.getArgument(0)) and + newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = + onReceive.getDeclaringType() and + n1.asExpr() = sendBroadcast.getArgument(0) and + n2.asParameter() = onReceive.getParameter(1) ) } } /** - * The Android class `android.os.AsyncTask`. + * The method `Activity.startActivity`. */ -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) +class ActivityStartActivityMethod extends Method { + ActivityStartActivityMethod() { + // should capture all `startAct` methods in the Activity class + // except for `startNextMatchingActivity`, which I'm leaving out for now since seems potentially more complicated + this.getName().matches("startActivit%") and + this.getDeclaringType() instanceof TypeActivity } } -/** A call to the `execute` or `executeOnExecutor` methods of the `android.os.AsyncTask` class. */ -private class ExecuteAsyncTaskMethodAccess extends MethodAccess { - ExecuteAsyncTaskMethodAccess() { - this.getMethod().hasName(["execute", "executeOnExecutor"]) and - this.getMethod().getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof - AsyncTask - } - - /** Returns the `params` argument of this call. */ - 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.hasName("doInBackground") +/** + * A value-preserving step from the Intent argument of a `startActivity` call from + * the Activity classs (not Context like the original StartActivityIntentStep in Intent.qll) + * to a `getIntent` call in the Activity the Intent pointed to in its constructor. + */ +private class StartActivityIntentStep_ActivityClass extends AdditionalValueStep { + override predicate step(DataFlow::Node n1, DataFlow::Node n2) { + exists(MethodAccess startActivity, MethodAccess getIntent, ClassInstanceExpr newIntent | + startActivity.getMethod().overrides*(any(ActivityStartActivityMethod m)) and + getIntent.getMethod().overrides*(any(AndroidGetIntentMethod m)) and + newIntent.getConstructedType() instanceof TypeIntent and + DataFlow::localExprFlow(newIntent, startActivity.getArgument(0)) and // ! startActivityFromChild and startActivityFromFragment have Intent as argument(1)... + newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = + getIntent.getReceiverType() and + n1.asExpr() = startActivity.getArgument(0) and // ! startActivityFromChild and startActivityFromFragment have Intent as argument(1)... + n2.asExpr() = getIntent + ) } } -/** 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") +/* ********* INTENT METHODS, E.G. parseUri, getData, getExtras, etc. *********** */ +// ! Check if can use pre-existing Synthetic Field instead of the below. +/** + * The method `Intent.get%Extra` or `Intent.getExtras`. + */ +class AndroidGetExtrasMethod extends Method { + AndroidGetExtrasMethod() { + (this.hasName("getExtras") or this.getName().matches("get%Extra")) and // ! switch to get%Extra% instead, I think wildcard holds for nothing there + this.getDeclaringType() instanceof TypeIntent + } +} + +/** + * The method `Intent.getData` + */ +class AndroidGetDataMethod extends Method { + AndroidGetDataMethod() { + this.hasName("getData") and this.getDeclaringType() instanceof TypeIntent + } +} + +/** + * The method `Intent.parseUri` + */ +class AndroidParseUriMethod extends Method { + AndroidParseUriMethod() { + (this.hasName("parseUri") or this.hasName("getIntent")) and // getIntent for older versions before deprecation to parseUri + this.getDeclaringType() instanceof TypeIntent + } +} + +/** + * A taint step from the Intent argument of a `startActivity` call to + * a `Intent.parseUri` call in the Activity the Intent pointed to in its constructor. + */ +private class StartActivityParseUriStep extends AdditionalTaintStep { + override predicate step(DataFlow::Node n1, DataFlow::Node n2) { + exists(MethodAccess startActivity, MethodAccess parseUri, ClassInstanceExpr newIntent | + startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) and + parseUri.getMethod().overrides*(any(AndroidParseUriMethod m)) and + newIntent.getConstructedType() instanceof TypeIntent and + DataFlow::localExprFlow(newIntent, startActivity.getArgument(0)) and + newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = + parseUri.getReceiverType() and + n1.asExpr() = startActivity.getArgument(0) and + n2.asExpr() = parseUri + ) + } +} + +/** + * A taint step from the Intent argument of a `startActivity` call to + * a `Intent.get%Extra%` call in the Activity the Intent pointed to in its constructor. + */ +private class StartActivityGetDataStep extends AdditionalTaintStep { + override predicate step(DataFlow::Node n1, DataFlow::Node n2) { + exists(MethodAccess startActivity, MethodAccess getData, ClassInstanceExpr newIntent | + startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) and + getData.getMethod().overrides*(any(AndroidGetDataMethod m)) and + newIntent.getConstructedType() instanceof TypeIntent and + DataFlow::localExprFlow(newIntent, startActivity.getArgument(0)) and + newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = + getData.getReceiverType() and + n1.asExpr() = startActivity.getArgument(0) and + n2.asExpr() = getData + ) + } +} + +/** + * A taint step from the Intent argument of a `startActivity` call to + * a `Intent.getData` call in the Activity the Intent pointed to in its constructor. + */ +private class StartActivityGetExtrasStep extends AdditionalTaintStep { + override predicate step(DataFlow::Node n1, DataFlow::Node n2) { + exists(MethodAccess startActivity, MethodAccess getExtras, ClassInstanceExpr newIntent | + startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) and + getExtras.getMethod().overrides*(any(AndroidGetExtrasMethod m)) and + newIntent.getConstructedType() instanceof TypeIntent and + DataFlow::localExprFlow(newIntent, startActivity.getArgument(0)) and + newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = + getExtras.getReceiverType() and + n1.asExpr() = startActivity.getArgument(0) and + n2.asExpr() = getExtras + ) } } diff --git a/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_SimpleQuery.ql b/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_SimpleQuery.ql index 62dc035c7ed..4791a481fa5 100644 --- a/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_SimpleQuery.ql +++ b/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_SimpleQuery.ql @@ -13,8 +13,11 @@ import java import semmle.code.xml.AndroidManifest -from AndroidActivityXmlElement actXmlElement -where - actXmlElement.hasDeepLink() and - not actXmlElement.getFile().(AndroidManifestXmlFile).isInBuildDirectory() -select actXmlElement, "A deeplink is used here." +// from AndroidActivityXmlElement actXmlElement +// where +// actXmlElement.hasDeepLink() and +// not actXmlElement.getFile().(AndroidManifestXmlFile).isInBuildDirectory() +// select actXmlElement, "A deeplink is used here." +from MethodAccess ma +where ma.getMethod().hasName("parseUri") +select ma, "parseUri access" From 9947b3244654afb7691a408caac6d1720243dad0 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Wed, 28 Sep 2022 16:32:55 -0400 Subject: [PATCH 03/20] resolve merge conflict --- java/ql/lib/semmle/code/java/Expr.qll | 13 + .../code/java/frameworks/android/DeepLink.qll | 357 +++++------------- .../code/java/frameworks/android/Intent.qll | 179 ++++++++- .../lib/semmle/code/xml/AndroidManifest.qll | 9 +- .../CWE-939/AndroidDeeplinks_RemoteSources.ql | 147 ++------ .../CWE-939/AndroidDeeplinks_SimpleQuery.ql | 14 +- 6 files changed, 340 insertions(+), 379 deletions(-) diff --git a/java/ql/lib/semmle/code/java/Expr.qll b/java/ql/lib/semmle/code/java/Expr.qll index 2c969a8d442..4b21a7e054b 100644 --- a/java/ql/lib/semmle/code/java/Expr.qll +++ b/java/ql/lib/semmle/code/java/Expr.qll @@ -1316,6 +1316,19 @@ class ClassInstanceExpr extends Expr, ConstructorCall, @classinstancexpr { result = this.getAnArgument() } + // ! remove below predicate after experimentation + /** + * Gets the argument provided to the constructor of this class instance creation expression + * of the specified Type. + */ + Expr getArgumentByType(Type type) { + exists(Argument arg | + arg = this.getAnArgument() and + arg.getType() = type and + result = arg + ) + } + /** * Gets a type argument provided to the constructor of this class instance creation expression. * diff --git a/java/ql/lib/semmle/code/java/frameworks/android/DeepLink.qll b/java/ql/lib/semmle/code/java/frameworks/android/DeepLink.qll index e0108214ccf..58eefe6544e 100644 --- a/java/ql/lib/semmle/code/java/frameworks/android/DeepLink.qll +++ b/java/ql/lib/semmle/code/java/frameworks/android/DeepLink.qll @@ -2,229 +2,81 @@ import java private import semmle.code.java.frameworks.android.Intent -private import semmle.code.java.frameworks.android.AsyncTask +//private import semmle.code.java.frameworks.android.AsyncTask private import semmle.code.java.frameworks.android.Android private import semmle.code.java.dataflow.DataFlow private import semmle.code.java.dataflow.FlowSteps -private import semmle.code.java.dataflow.ExternalFlow +//private import semmle.code.java.dataflow.ExternalFlow +//private import semmle.code.java.dataflow.TaintTracking +private import semmle.code.xml.AndroidManifest +// ! if keeping this class, should prbly move to security folder. // ! Remember to add 'private' annotation as needed to all new classes/predicates below. -/* ********* OTHER COMPONENTS (SERVICE, RECEIVER) *********** */ +// ! and clean-up comments, etc. in below in general... /** - * The class `android.app.Service`. + * A value-preserving step from the Intent argument of a method call that starts a component to + * a `getIntent` call or `Intent` parameter in the component that the Intent pointed to in its constructor. */ -class TypeService extends Class { - TypeService() { this.hasQualifiedName("android.app", "Service") } -} - -/** - * The method `Context.startService` or `Context.startForegroundService`. - */ -class ContextStartServiceMethod extends Method { - ContextStartServiceMethod() { - (this.hasName("startService") or this.hasName("startForegroundService")) and - this.getDeclaringType() instanceof TypeContext - } -} - -/** - * A method of type Service that receives an Intent as a parameter. - * Namely: `Service.onStart`, `Service.onStartCommand`, - * `Service.onBind`, `Service.onRebind` - * `Service.onUnbind`, or - * `Service.onTaskRemoved` - */ -class AndroidServeIntentMethod extends Method { - AndroidServeIntentMethod() { +// ! experimental - make a DeepLink step that combine Activity, Service, Receiver, etc. +private class DeepLinkIntentStep extends AdditionalValueStep { + // DeepLinkIntentStep() { + // this instanceof StartActivityIntentStep_ContextAndActivity or + // this instanceof SendBroadcastReceiverIntentStep or + // this instanceof StartServiceIntentStep + // } + override predicate step(DataFlow::Node n1, DataFlow::Node n2) { + // ! simplify below ( - this.hasName("onStart") or - this.hasName("onStartCommand") or - // this.getName.matches("onStart%") or // could maybe switch with the above - this.hasName("onBind") or - this.hasName("onRebind") or - this.hasName("onUnbind") or - // this.getName.matches("on%bind") or // could maybe switch with the above - this.hasName("onTaskRemoved") + exists(StartServiceIntentStep startServiceIntentStep | startServiceIntentStep.step(n1, n2)) + or + exists(SendBroadcastReceiverIntentStep sendBroadcastIntentStep | + sendBroadcastIntentStep.step(n1, n2) + ) + or + exists(StartActivityIntentStep startActivityIntentStep | startActivityIntentStep.step(n1, n2)) ) and - this.getDeclaringType() instanceof TypeService - } -} - -/** - * The method `Service.onStart`, `Service.onStartCommand`, - * `Service.onBind`, `Service.onRebind` - * `Service.onUnbind`, or - * `Service.onTaskRemoved` - */ -class AndroidServeIntentMethod2 extends Method { - AndroidServeIntentMethod2() { - ( - this instanceof ServiceOnStartMethod or - this instanceof ServiceOnBindMethod or - this instanceof ServiceOnUnbindMethod or - this instanceof ServiceOnTaskRemovedMethod - ) and - this.getDeclaringType() instanceof TypeService - } -} - -/** - * The method `Service.onStart` or `Service.onStartCommand`. - */ -class ServiceOnStartMethod extends Method { - ServiceOnStartMethod() { - (this.hasName("onStart") or this.hasName("onStartCommand")) and - this.getDeclaringType() instanceof TypeService - } -} - -/** - * The method `Service.onBind` or `Service.onRebind`. - */ -class ServiceOnBindMethod extends Method { - ServiceOnBindMethod() { - (this.hasName("onBind") or this.hasName("onRebind")) and - this.getDeclaringType() instanceof TypeService - } -} - -/** - * The method `Service.onUnbind`. - */ -class ServiceOnUnbindMethod extends Method { - ServiceOnUnbindMethod() { - this.hasName("onUnbind") and - this.getDeclaringType() instanceof TypeService - } -} - -/** - * The method `Service.onTaskRemoved`. - */ -class ServiceOnTaskRemovedMethod extends Method { - ServiceOnTaskRemovedMethod() { - this.hasName("onTaskRemoved") and - this.getDeclaringType() instanceof TypeService - } -} - -// ! update QLDoc more if needed (e.g. remove `onStart` reference) -/** - * A value-preserving step from the Intent argument of a `startService` call to - * the `Intent` Parameter in the `onStart` method of the Service the Intent pointed - * to in its constructor. - */ -class StartServiceIntentStep extends AdditionalValueStep { - override predicate step(DataFlow::Node n1, DataFlow::Node n2) { - exists(MethodAccess startService, Method onStart, ClassInstanceExpr newIntent | - startService.getMethod().overrides*(any(ContextStartServiceMethod m)) and - //onStart.overrides*(any(ServiceOnStartMethod m)) and - //onStart.overrides*(any(AndroidServeIntentMethod m)) and - onStart.overrides*(any(AndroidServeIntentMethod2 m)) and - newIntent.getConstructedType() instanceof TypeIntent and - DataFlow::localExprFlow(newIntent, startService.getArgument(0)) and - newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = - onStart.getDeclaringType() and - n1.asExpr() = startService.getArgument(0) and - n2.asParameter() = onStart.getParameter(0) + exists(AndroidComponent andComp | + andComp.getAndroidComponentXmlElement().(AndroidActivityXmlElement).hasDeepLink() and + n1.asExpr().getFile() = andComp.getFile() // ! ugly, see if better way to do this ) } } -// /** -// * The class `android.content.BroadcastReceiver`. -// */ -// class TypeBroadcastReceiver extends Class { -// TypeBroadcastReceiver() { this.hasQualifiedName("android.content", "BroadcastReceiver") } -// } +// ! experimentation with global flow issue - REMOVE /** - * The method `Context.sendBroadcast`. + * A value-preserving step from the Intent variable + * the `Intent` Parameter in the `startActivity`. */ -class ContextSendBroadcastMethod extends Method { - ContextSendBroadcastMethod() { - this.getName().matches("send%Broadcast%") and // ! double-check this - seems to work based on quick-eval - this.getDeclaringType() instanceof TypeContext - } -} - -// ! do I need to model this as well? -/** - * The method `BroadcastReceiver.peekService`. - */ -class BroadcastReceiverPeekServiceIntentMethod extends Method { - BroadcastReceiverPeekServiceIntentMethod() { - this.hasName("peekService") and this.getDeclaringType() instanceof TypeBroadcastReceiver - } -} - -// /** -// * The method `BroadcastReceiver.onReceive`. -// */ -// class AndroidReceiveIntentMethod extends Method { -// AndroidReceiveIntentMethod() { -// this.hasName("onReceive") and this.getDeclaringType() instanceof TypeBroadcastReceiver -// } -// } -/** - * A value-preserving step from the Intent argument of a `sendBroadcast` call to - * the `Intent` Parameter in the `onStart` method of the BroadcastReceiver the - * Intent pointed to in its constructor. - */ -class SendBroadcastReceiverIntentStep extends AdditionalValueStep { +class IntentVariableToStartActivityStep extends AdditionalValueStep { override predicate step(DataFlow::Node n1, DataFlow::Node n2) { - exists(MethodAccess sendBroadcast, Method onReceive, ClassInstanceExpr newIntent | - sendBroadcast.getMethod().overrides*(any(ContextSendBroadcastMethod m)) and - onReceive.overrides*(any(AndroidReceiveIntentMethod m)) and - newIntent.getConstructedType() instanceof TypeIntent and - DataFlow::localExprFlow(newIntent, sendBroadcast.getArgument(0)) and - newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = - onReceive.getDeclaringType() and - n1.asExpr() = sendBroadcast.getArgument(0) and - n2.asParameter() = onReceive.getParameter(1) + exists(MethodAccess startActivity, Variable intentTypeTest | + ( + // ! is there a better way to do this? + startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) or + startActivity.getMethod().overrides*(any(ActivityStartActivityMethod m)) + ) and + intentTypeTest.getType() instanceof TypeIntent and + //startActivity.getFile().getBaseName() = "MainActivity.java" and // ! REMOVE + DataFlow::localExprFlow(intentTypeTest.getInitializer(), startActivity.getArgument(0)) and + n1.asExpr() = intentTypeTest.getInitializer() and + n2.asExpr() = startActivity.getArgument(0) // ! switch to getStartActivityIntentArg(startActivity) ) } } -/** - * The method `Activity.startActivity`. +/* ********************* INTENT METHODS, E.G. parseUri, getData, getExtras, etc. ********************* */ +/* + * Below is a Draft/Test of modelling `Intent.parseUri`, `Intent.getData`, + * and `Intent.getExtras` methods */ -class ActivityStartActivityMethod extends Method { - ActivityStartActivityMethod() { - // should capture all `startAct` methods in the Activity class - // except for `startNextMatchingActivity`, which I'm leaving out for now since seems potentially more complicated - this.getName().matches("startActivit%") and - this.getDeclaringType() instanceof TypeActivity - } -} -/** - * A value-preserving step from the Intent argument of a `startActivity` call from - * the Activity classs (not Context like the original StartActivityIntentStep in Intent.qll) - * to a `getIntent` call in the Activity the Intent pointed to in its constructor. - */ -private class StartActivityIntentStep_ActivityClass extends AdditionalValueStep { - override predicate step(DataFlow::Node n1, DataFlow::Node n2) { - exists(MethodAccess startActivity, MethodAccess getIntent, ClassInstanceExpr newIntent | - startActivity.getMethod().overrides*(any(ActivityStartActivityMethod m)) and - getIntent.getMethod().overrides*(any(AndroidGetIntentMethod m)) and - newIntent.getConstructedType() instanceof TypeIntent and - DataFlow::localExprFlow(newIntent, startActivity.getArgument(0)) and // ! startActivityFromChild and startActivityFromFragment have Intent as argument(1)... - newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = - getIntent.getReceiverType() and - n1.asExpr() = startActivity.getArgument(0) and // ! startActivityFromChild and startActivityFromFragment have Intent as argument(1)... - n2.asExpr() = getIntent - ) - } -} - -/* ********* INTENT METHODS, E.G. parseUri, getData, getExtras, etc. *********** */ -// ! Check if can use pre-existing Synthetic Field instead of the below. +// ! Check if can use pre-existing Synthetic Field. /** * The method `Intent.get%Extra` or `Intent.getExtras`. */ class AndroidGetExtrasMethod extends Method { AndroidGetExtrasMethod() { - (this.hasName("getExtras") or this.getName().matches("get%Extra")) and // ! switch to get%Extra% instead, I think wildcard holds for nothing there + this.getName().matches("get%Extra%") and this.getDeclaringType() instanceof TypeIntent } } @@ -243,64 +95,61 @@ class AndroidGetDataMethod extends Method { */ class AndroidParseUriMethod extends Method { AndroidParseUriMethod() { - (this.hasName("parseUri") or this.hasName("getIntent")) and // getIntent for older versions before deprecation to parseUri + (this.hasName("parseUri") or this.hasName("getIntent")) and // ! Note to self: getIntent for older versions before deprecation to parseUri this.getDeclaringType() instanceof TypeIntent } } - -/** - * A taint step from the Intent argument of a `startActivity` call to - * a `Intent.parseUri` call in the Activity the Intent pointed to in its constructor. - */ -private class StartActivityParseUriStep extends AdditionalTaintStep { - override predicate step(DataFlow::Node n1, DataFlow::Node n2) { - exists(MethodAccess startActivity, MethodAccess parseUri, ClassInstanceExpr newIntent | - startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) and - parseUri.getMethod().overrides*(any(AndroidParseUriMethod m)) and - newIntent.getConstructedType() instanceof TypeIntent and - DataFlow::localExprFlow(newIntent, startActivity.getArgument(0)) and - newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = - parseUri.getReceiverType() and - n1.asExpr() = startActivity.getArgument(0) and - n2.asExpr() = parseUri - ) - } -} - -/** - * A taint step from the Intent argument of a `startActivity` call to - * a `Intent.get%Extra%` call in the Activity the Intent pointed to in its constructor. - */ -private class StartActivityGetDataStep extends AdditionalTaintStep { - override predicate step(DataFlow::Node n1, DataFlow::Node n2) { - exists(MethodAccess startActivity, MethodAccess getData, ClassInstanceExpr newIntent | - startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) and - getData.getMethod().overrides*(any(AndroidGetDataMethod m)) and - newIntent.getConstructedType() instanceof TypeIntent and - DataFlow::localExprFlow(newIntent, startActivity.getArgument(0)) and - newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = - getData.getReceiverType() and - n1.asExpr() = startActivity.getArgument(0) and - n2.asExpr() = getData - ) - } -} - -/** - * A taint step from the Intent argument of a `startActivity` call to - * a `Intent.getData` call in the Activity the Intent pointed to in its constructor. - */ -private class StartActivityGetExtrasStep extends AdditionalTaintStep { - override predicate step(DataFlow::Node n1, DataFlow::Node n2) { - exists(MethodAccess startActivity, MethodAccess getExtras, ClassInstanceExpr newIntent | - startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) and - getExtras.getMethod().overrides*(any(AndroidGetExtrasMethod m)) and - newIntent.getConstructedType() instanceof TypeIntent and - DataFlow::localExprFlow(newIntent, startActivity.getArgument(0)) and - newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = - getExtras.getReceiverType() and - n1.asExpr() = startActivity.getArgument(0) and - n2.asExpr() = getExtras - ) - } -} +// /** +// * A taint step from the Intent argument of a `startActivity` call to +// * a `Intent.parseUri` call in the Activity the Intent pointed to in its constructor. +// */ +// private class StartActivityParseUriStep extends AdditionalTaintStep { +// override predicate step(DataFlow::Node n1, DataFlow::Node n2) { +// exists(MethodAccess startActivity, MethodAccess parseUri, ClassInstanceExpr newIntent | +// startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) and +// parseUri.getMethod().overrides*(any(AndroidParseUriMethod m)) and +// newIntent.getConstructedType() instanceof TypeIntent and +// DataFlow::localExprFlow(newIntent, startActivity.getArgument(0)) and +// newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = +// parseUri.getReceiverType() and +// n1.asExpr() = startActivity.getArgument(0) and +// n2.asExpr() = parseUri +// ) +// } +// } +// /** +// * A taint step from the Intent argument of a `startActivity` call to +// * a `Intent.get%Extra%` call in the Activity the Intent pointed to in its constructor. +// */ +// private class StartActivityGetDataStep extends AdditionalTaintStep { +// override predicate step(DataFlow::Node n1, DataFlow::Node n2) { +// exists(MethodAccess startActivity, MethodAccess getData, ClassInstanceExpr newIntent | +// startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) and +// getData.getMethod().overrides*(any(AndroidGetDataMethod m)) and +// newIntent.getConstructedType() instanceof TypeIntent and +// DataFlow::localExprFlow(newIntent, startActivity.getArgument(0)) and +// newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = +// getData.getReceiverType() and +// n1.asExpr() = startActivity.getArgument(0) and +// n2.asExpr() = getData +// ) +// } +// } +// /** +// * A taint step from the Intent argument of a `startActivity` call to +// * a `Intent.getData` call in the Activity the Intent pointed to in its constructor. +// */ +// private class StartActivityGetExtrasStep extends AdditionalTaintStep { +// override predicate step(DataFlow::Node n1, DataFlow::Node n2) { +// exists(MethodAccess startActivity, MethodAccess getExtras, ClassInstanceExpr newIntent | +// startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) and +// getExtras.getMethod().overrides*(any(AndroidGetExtrasMethod m)) and +// newIntent.getConstructedType() instanceof TypeIntent and +// DataFlow::localExprFlow(newIntent, startActivity.getArgument(0)) and +// newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = +// getExtras.getReceiverType() and +// n1.asExpr() = startActivity.getArgument(0) and +// n2.asExpr() = getExtras +// ) +// } +// } diff --git a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll index ad260c9545c..97af99fd966 100644 --- a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll +++ b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll @@ -3,6 +3,8 @@ private import semmle.code.java.dataflow.DataFlow private import semmle.code.java.dataflow.ExternalFlow private import semmle.code.java.dataflow.FlowSteps +// ! Remember to add 'private' annotation as needed to all new classes/predicates below. +// ! and clean-up comments, etc. in below in general... /** * The class `android.content.Intent`. */ @@ -64,6 +66,38 @@ class AndroidReceiveIntentMethod extends Method { } } +// ! confirm if peekService should be modelled since it takes an Intent as a parameter +/** + * The method `BroadcastReceiver.peekService`. + */ +class BroadcastReceiverPeekServiceIntentMethod extends Method { + BroadcastReceiverPeekServiceIntentMethod() { + this.hasName("peekService") and this.getDeclaringType() instanceof TypeBroadcastReceiver + } +} + +// ! potentially reword the below QLDoc +/** + * A method of type Service that receives an Intent as a parameter. + * Namely, `Service.onStart`, `onStartCommand`, `onBind`, `onRebind` + * `onUnbind`, or `onTaskRemoved` + */ +class AndroidServiceIntentMethod extends Method { + AndroidServiceIntentMethod() { + ( + // this.hasName("onStart") or + // this.hasName("onStartCommand") or + this.getName().matches("onStart%") or + // this.hasName("onBind") or + // this.hasName("onRebind") or + // this.hasName("onUnbind") or + this.getName().matches("on%ind") or + this.hasName("onTaskRemoved") + ) and + this.getDeclaringType() instanceof TypeService + } +} + /** * The method `Service.onStart`, `onStartCommand`, * `onBind`, `onRebind`, `onUnbind`, or `onTaskRemoved`. @@ -85,6 +119,51 @@ class ContextStartActivityMethod extends Method { } } +// ! finish QLDoc +/** + * The method `Activity.startActivity` or ... + */ +class ActivityStartActivityMethod extends Method { + ActivityStartActivityMethod() { + // ! captures all `startAct` methods in the Activity class + this.getName().matches("start%Activit%") and // ! better to list all instead of using matches for any reason? + this.getDeclaringType() instanceof TypeActivity + } +} + +/** + * The method `Context.sendBroadcast`. + */ +class ContextSendBroadcastMethod extends Method { + ContextSendBroadcastMethod() { + this.getName().matches("send%Broadcast%") and // ! Note to self: matches the 9 sendBroadcast methods + this.getDeclaringType() instanceof TypeContext + } +} + +// ! update below QLDoc +/** + * The method `Context.startService`, `startForegroundService`, + * `bindIsolatedService`, `bindService`, or `bindServiceAsUser`. + * From https://developer.android.com/reference/android/app/Service: + * "Services can be started with Context.startService() and Context.bindService()." + */ +class ContextStartServiceMethod extends Method { + ContextStartServiceMethod() { + ( + // this.hasName("startService") or + // this.hasName("startForegroundService") or + this.getName().matches("start%Service") + or + // this.hasName("bindIsolatedService") or + // this.hasName("bindService") or + // this.hasName("bindServiceAsUser") + this.getName().matches("bind%Service%") + ) and + this.getDeclaringType() instanceof TypeContext + } +} + /** * Specifies that if an `Intent` is tainted, then so are its synthetic fields. */ @@ -194,25 +273,115 @@ class GrantWriteUriPermissionFlag extends GrantUriPermissionFlag { GrantWriteUriPermissionFlag() { this.hasName("FLAG_GRANT_WRITE_URI_PERMISSION") } } +// ! OLD VERSION +// /** +// * A value-preserving step from the Intent argument of a `startActivity` call to +// * a `getIntent` call in the Activity the Intent pointed to in its constructor. +// */ +// private class StartActivityIntentStep extends AdditionalValueStep { +// override predicate step(DataFlow::Node n1, DataFlow::Node n2) { +// exists(MethodAccess startActivity, MethodAccess getIntent, ClassInstanceExpr newIntent | +// startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) and +// getIntent.getMethod().overrides*(any(AndroidGetIntentMethod m)) and +// newIntent.getConstructedType() instanceof TypeIntent and +// DataFlow::localExprFlow(newIntent, startActivity.getArgument(0)) and +// newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = +// getIntent.getReceiverType() and +// n1.asExpr() = startActivity.getArgument(0) and +// n2.asExpr() = getIntent +// ) +// } +// } /** * A value-preserving step from the Intent argument of a `startActivity` call to * a `getIntent` call in the Activity the Intent pointed to in its constructor. */ -private class StartActivityIntentStep extends AdditionalValueStep { +class StartActivityIntentStep extends AdditionalValueStep { + // ! startActivityFromChild and startActivityFromFragment have Intent as argument(1), + // ! but rest have Intent as argument(0)... + // ! startActivityFromChild and startActivityFromFragment are also deprecated and + // ! may need to look into modelling androidx.fragment.app.Fragment.startActivity() as well + private Argument getStartActivityIntentArg(MethodAccess startActMethodAccess) { + if + startActMethodAccess.getMethod().hasName("startActivityFromChild") or + startActMethodAccess.getMethod().hasName("startActivityFromFragment") + then result = startActMethodAccess.getArgument(1) + else result = startActMethodAccess.getArgument(0) + } + + // ! Intent has two constructors with Class parameter, only the first one with argument + // ! at position 1 was modelled before leading to lost flow. The second constructor with + // ! argument at position 3 needs to be modelled as well. + // ! See https://developer.android.com/reference/android/content/Intent#public-constructors + private Argument getIntentConstructorClassArg(ClassInstanceExpr intent) { + if intent.getNumArgument() = 2 + then result = intent.getArgument(1) + else result = intent.getArgument(3) + } + override predicate step(DataFlow::Node n1, DataFlow::Node n2) { exists(MethodAccess startActivity, MethodAccess getIntent, ClassInstanceExpr newIntent | - startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) and + ( + // ! is there a better way to do this? + startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) or + startActivity.getMethod().overrides*(any(ActivityStartActivityMethod m)) + ) and getIntent.getMethod().overrides*(any(AndroidGetIntentMethod m)) and newIntent.getConstructedType() instanceof TypeIntent and - DataFlow::localExprFlow(newIntent, startActivity.getArgument(0)) and - newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = + DataFlow::localExprFlow(newIntent, getStartActivityIntentArg(startActivity)) and + getIntentConstructorClassArg(newIntent).getType().(ParameterizedType).getATypeArgument() = getIntent.getReceiverType() and - n1.asExpr() = startActivity.getArgument(0) and + // ! below uses predicate `getArgumentByType` that I added to the Expr.qll lib, prbly should not add new predicate there. + // argType.getName().matches("Class<%>") and + // newIntent.getArgumentByType(argType).getType().(ParameterizedType).getATypeArgument() = + // getIntent.getReceiverType() and + n1.asExpr() = getStartActivityIntentArg(startActivity) and n2.asExpr() = getIntent ) } } +/** + * A value-preserving step from the Intent argument of a `sendBroadcast` call to + * the `Intent` Parameter in the `onStart` method of the BroadcastReceiver the + * Intent pointed to in its constructor. + */ +class SendBroadcastReceiverIntentStep extends AdditionalValueStep { + override predicate step(DataFlow::Node n1, DataFlow::Node n2) { + exists(MethodAccess sendBroadcast, Method onReceive, ClassInstanceExpr newIntent | + sendBroadcast.getMethod().overrides*(any(ContextSendBroadcastMethod m)) and + onReceive.overrides*(any(AndroidReceiveIntentMethod m)) and + newIntent.getConstructedType() instanceof TypeIntent and + DataFlow::localExprFlow(newIntent, sendBroadcast.getArgument(0)) and + newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = + onReceive.getDeclaringType() and + n1.asExpr() = sendBroadcast.getArgument(0) and + n2.asParameter() = onReceive.getParameter(1) + ) + } +} + +// ! potentially update QLDoc (e.g. remove `onStart` reference) +/** + * A value-preserving step from the Intent argument of a `startService` call to + * the `Intent` Parameter in the `onStart` method of the Service the Intent pointed + * to in its constructor. + */ +class StartServiceIntentStep extends AdditionalValueStep { + override predicate step(DataFlow::Node n1, DataFlow::Node n2) { + exists(MethodAccess startService, Method serviceIntent, ClassInstanceExpr newIntent | + startService.getMethod().overrides*(any(ContextStartServiceMethod m)) and + serviceIntent.overrides*(any(AndroidServiceIntentMethod m)) and + newIntent.getConstructedType() instanceof TypeIntent and + DataFlow::localExprFlow(newIntent, startService.getArgument(0)) and + newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = + serviceIntent.getDeclaringType() and + n1.asExpr() = startService.getArgument(0) and + n2.asParameter() = serviceIntent.getParameter(0) + ) + } +} + private class IntentBundleFlowSteps extends SummaryModelCsv { override predicate row(string row) { row = diff --git a/java/ql/lib/semmle/code/xml/AndroidManifest.qll b/java/ql/lib/semmle/code/xml/AndroidManifest.qll index a96b15b65d3..0a8e92b0252 100644 --- a/java/ql/lib/semmle/code/xml/AndroidManifest.qll +++ b/java/ql/lib/semmle/code/xml/AndroidManifest.qll @@ -130,11 +130,12 @@ class AndroidApplicationXmlElement extends XmlElement { class AndroidActivityXmlElement extends AndroidComponentXmlElement { AndroidActivityXmlElement() { this.getName() = "activity" } - // ! Double-check that no other components can have deep links. - // ! Consider moving this to its own .qll file like for Implicit Export Query. - // ! Also double-check that the below actions and categories are REQUIRED for it to - // ! count as a deep link versus just recommended (e.g. should I just look for the + // ! Consider moving this to its own .qll file under `security` like for Implicit Export Query. + // ! Double-check that the below actions and categories are REQUIRED for it to + // ! count as a deep link versus just recommended (e.g. should I just look at the // ! data element instead?). + // ! Reference: https://developer.android.com/training/app-links/deep-linking#adding-filters + // ! Note: not excluding App Links since those are a subset of deep links that can still cause issues. /** * Holds if this `` element has a deep link. */ diff --git a/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_RemoteSources.ql b/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_RemoteSources.ql index 1ab6c0f1c8d..3b5402e5c01 100644 --- a/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_RemoteSources.ql +++ b/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_RemoteSources.ql @@ -9,118 +9,49 @@ * @precision high */ +// * experiment with StartActivityIntentStep // import java -// import semmle.code.xml.AndroidManifest -// import semmle.code.java.frameworks.android.Android -// import semmle.code.java.frameworks.android.Intent -// import semmle.code.java.frameworks.android.AsyncTask // import semmle.code.java.frameworks.android.DeepLink // import semmle.code.java.dataflow.DataFlow -// import semmle.code.java.dataflow.TaintTracking -// import semmle.code.java.dataflow.FlowSources -// import semmle.code.java.dataflow.FlowSteps -// import semmle.code.java.dataflow.ExternalFlow -//* select getData() method access in RouterActivity -// from AndroidComponent andComp, MethodAccess ma -// where -// andComp -// .getAndroidComponentXmlElement() -// .getAnIntentFilterElement() -// .getAnActionElement() -// .getActionName() = "android.intent.action.VIEW" and -// andComp -// .getAndroidComponentXmlElement() -// .getAnIntentFilterElement() -// .getACategoryElement() -// .getCategoryName() = "android.intent.category.BROWSABLE" and -// andComp -// .getAndroidComponentXmlElement() -// .getAnIntentFilterElement() -// .getACategoryElement() -// .getCategoryName() = "android.intent.category.DEFAULT" and -// andComp -// .getAndroidComponentXmlElement() -// .getAnIntentFilterElement() -// .getAChild("data") -// .hasAttribute("scheme") and // make sure to check for 'android' prefix in real query -// ma.getMethod().hasName("getData") and -// //ma.getCompilationUnit().toString() = andComp.toString() // string is "RouterActivity" -// andComp.getFile() = ma.getFile() -// select ma, "getData usage related to deeplink" -// * play with taint/data -// class DeepLinkConfiguration extends TaintTracking::Configuration { -// DeepLinkConfiguration() { this = "DeepLinkConfiguration" } -// override predicate isSource(DataFlow::Node source) { -// exists(MethodAccess ma, AndroidComponent andComp | -// ma.getMethod().hasName("getData") and -// andComp -// .getAndroidComponentXmlElement() -// .getAnIntentFilterElement() -// .getAnActionElement() -// .getActionName() = "android.intent.action.VIEW" and -// andComp -// .getAndroidComponentXmlElement() -// .getAnIntentFilterElement() -// .getACategoryElement() -// .getCategoryName() = "android.intent.category.BROWSABLE" and -// andComp -// .getAndroidComponentXmlElement() -// .getAnIntentFilterElement() -// .getACategoryElement() -// .getCategoryName() = "android.intent.category.DEFAULT" and -// andComp -// .getAndroidComponentXmlElement() -// .getAnIntentFilterElement() -// .getAChild("data") -// .hasAttribute("scheme") and -// andComp.getFile() = ma.getFile() and -// source.asExpr() = ma -// ) -// } -// override predicate isSink(DataFlow::Node sink) { -// exists(Variable v | v.hasName("currentUrl") and sink.asExpr() = v.getAnAccess()) -// } -// } -// from DataFlow::Node src, DataFlow::Node sink, DeepLinkConfiguration config -// where config.hasFlow(src, sink) -// select src, "This environment variable constructs a URL $@.", sink, "here" -// * Intent experimentation: -//from -// AndroidComponent andComp, MethodAccess ma, StartActivityIntentStep startActIntStep, -// DataFlow::Node n1, DataFlow::Node n2 -// where -// andComp -// .getAndroidComponentXmlElement() -// .getAnIntentFilterElement() -// .getAnActionElement() -// .getActionName() = "android.intent.action.VIEW" and -// andComp -// .getAndroidComponentXmlElement() -// .getAnIntentFilterElement() -// .getACategoryElement() -// .getCategoryName() = "android.intent.category.BROWSABLE" and -// andComp -// .getAndroidComponentXmlElement() -// .getAnIntentFilterElement() -// .getACategoryElement() -// .getCategoryName() = "android.intent.category.DEFAULT" and -// andComp -// .getAndroidComponentXmlElement() -// .getAnIntentFilterElement() -// .getAChild("data") -// .hasAttribute("scheme") and // make sure to check for 'android' prefix in real query -// //ma.getMethod().hasName("getData") and -// //andComp.getFile() = ma.getFile() and -// //n1.asExpr() = ma and -// //n1.asExpr() = ma and -// andComp.getAnAnnotation() = n1.asExpr() and -// startActIntStep.step(n1, n2) -// select n1, "deeplink" -// * experiment with StartActivityIntentStep +// from StartServiceIntentStep startServiceIntStep, DataFlow::Node n1, DataFlow::Node n2 +// where startServiceIntStep.step(n1, n2) +// select n2, "placeholder" +// * experiment with taint-tracking import java +import semmle.code.java.dataflow.TaintTracking import semmle.code.java.frameworks.android.DeepLink +import semmle.code.java.frameworks.android.Intent +import semmle.code.java.frameworks.android.Android import semmle.code.java.dataflow.DataFlow +import semmle.code.java.dataflow.FlowSteps +import semmle.code.java.dataflow.FlowSources +import semmle.code.java.dataflow.ExternalFlow +import semmle.code.xml.AndroidManifest +import semmle.code.java.dataflow.TaintTracking -from StartServiceIntentStep startServiceIntStep, DataFlow::Node n1, DataFlow::Node n2 -where startServiceIntStep.step(n1, n2) -select n2, "placeholder" +class MyTaintTrackingConfiguration extends TaintTracking::Configuration { + MyTaintTrackingConfiguration() { this = "MyTaintTrackingConfiguration" } + + override predicate isSource(DataFlow::Node source) { + // exists(AndroidActivityXmlElement andActXmlElem | + // andActXmlElem.hasDeepLink() and + // source.asExpr() instanceof TypeActivity + // ) + source instanceof RemoteFlowSource and //AndroidIntentInput + exists(AndroidComponent andComp | + andComp.getAndroidComponentXmlElement().(AndroidActivityXmlElement).hasDeepLink() and + source.asExpr().getFile() = andComp.getFile() // ! ugly, see if better way to do this + ) + } + + override predicate isSink(DataFlow::Node sink) { + exists(MethodAccess m | + m.getMethod().hasName("getIntent") and + sink.asExpr() = m + ) + } +} + +from DataFlow::Node src, DataFlow::Node sink, MyTaintTrackingConfiguration config +where config.hasFlow(src, sink) +select src, "This environment variable constructs a URL $@.", sink, "here" diff --git a/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_SimpleQuery.ql b/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_SimpleQuery.ql index 4791a481fa5..4c6ce50cd3d 100644 --- a/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_SimpleQuery.ql +++ b/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_SimpleQuery.ql @@ -13,11 +13,9 @@ import java import semmle.code.xml.AndroidManifest -// from AndroidActivityXmlElement actXmlElement -// where -// actXmlElement.hasDeepLink() and -// not actXmlElement.getFile().(AndroidManifestXmlFile).isInBuildDirectory() -// select actXmlElement, "A deeplink is used here." -from MethodAccess ma -where ma.getMethod().hasName("parseUri") -select ma, "parseUri access" +// simple query for testing and MRVA results +from AndroidActivityXmlElement actXmlElement +where + actXmlElement.hasDeepLink() and + not actXmlElement.getFile().(AndroidManifestXmlFile).isInBuildDirectory() +select actXmlElement, "A deeplink is used here." From 6cf3898101b6414a7f675bf7d4f92dbc9db85b4b Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Wed, 7 Sep 2022 18:46:15 -0400 Subject: [PATCH 04/20] add experimental global flow config, and clean-up some code --- java/ql/lib/semmle/code/java/Expr.qll | 4 + .../code/java/frameworks/android/DeepLink.qll | 129 ++++++++---------- .../code/java/frameworks/android/Intent.qll | 29 ++-- .../CWE-939/AndroidDeeplinks_RemoteSources.ql | 69 +++++++--- 4 files changed, 132 insertions(+), 99 deletions(-) diff --git a/java/ql/lib/semmle/code/java/Expr.qll b/java/ql/lib/semmle/code/java/Expr.qll index 4b21a7e054b..423de505070 100644 --- a/java/ql/lib/semmle/code/java/Expr.qll +++ b/java/ql/lib/semmle/code/java/Expr.qll @@ -1327,6 +1327,10 @@ class ClassInstanceExpr extends Expr, ConstructorCall, @classinstancexpr { arg.getType() = type and result = arg ) + // ! e.g. use above in below code in `StartActivityIntentStep` in Intent.qll + // argType.getName().matches("Class<%>") and + // newIntent.getArgumentByType(argType).getType().(ParameterizedType).getATypeArgument() = + // getIntent.getReceiverType() and } /** diff --git a/java/ql/lib/semmle/code/java/frameworks/android/DeepLink.qll b/java/ql/lib/semmle/code/java/frameworks/android/DeepLink.qll index 58eefe6544e..6f5c5b5c690 100644 --- a/java/ql/lib/semmle/code/java/frameworks/android/DeepLink.qll +++ b/java/ql/lib/semmle/code/java/frameworks/android/DeepLink.qll @@ -5,7 +5,9 @@ private import semmle.code.java.frameworks.android.Intent //private import semmle.code.java.frameworks.android.AsyncTask private import semmle.code.java.frameworks.android.Android private import semmle.code.java.dataflow.DataFlow +//private import semmle.code.java.dataflow.DataFlow2 private import semmle.code.java.dataflow.FlowSteps +//private import semmle.code.java.dataflow.FlowSources //private import semmle.code.java.dataflow.ExternalFlow //private import semmle.code.java.dataflow.TaintTracking private import semmle.code.xml.AndroidManifest @@ -37,31 +39,71 @@ private class DeepLinkIntentStep extends AdditionalValueStep { ) and exists(AndroidComponent andComp | andComp.getAndroidComponentXmlElement().(AndroidActivityXmlElement).hasDeepLink() and - n1.asExpr().getFile() = andComp.getFile() // ! ugly, see if better way to do this + n1.asExpr().getFile() = andComp.getFile() // ! see if better way to do this ) } } -// ! experimentation with global flow issue - REMOVE -/** - * A value-preserving step from the Intent variable - * the `Intent` Parameter in the `startActivity`. - */ -class IntentVariableToStartActivityStep extends AdditionalValueStep { - override predicate step(DataFlow::Node n1, DataFlow::Node n2) { - exists(MethodAccess startActivity, Variable intentTypeTest | +// // ! experimentation with global flow issue - REMOVE +// /** +// * A value-preserving step from the Intent variable +// * the `Intent` Parameter in the `startActivity`. +// */ +// class IntentVariableToStartActivityStep extends AdditionalValueStep { +// override predicate step(DataFlow::Node n1, DataFlow::Node n2) { +// exists( +// MethodAccess startActivity, Variable intentTypeTest, DataFlow2::Node source, +// DataFlow2::Node sink //ClassInstanceExpr intentTypeTest | +// | +// ( +// startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) or +// startActivity.getMethod().overrides*(any(ActivityStartActivityMethod m)) +// ) and +// intentTypeTest.getType() instanceof TypeIntent and // Variable +// //intentTypeTest.getConstructedType() instanceof TypeIntent and // ClassInstanceExpr +// startActivity.getFile().getBaseName() = "MainActivity.java" and // ! REMOVE - for testing only +// //exists(StartComponentConfiguration cfg | cfg.hasFlow(source, sink)) and // GLOBAL FLOW ATTEMPT +// DataFlow::localExprFlow(intentTypeTest.getInitializer(), startActivity.getArgument(0)) and // Variable - gives 5 results - misses the 1st ProfileActivity result since no variable with that one +// //DataFlow::localExprFlow(intentTypeTest, startActivity.getArgument(0)) and // ClassInstanceExpr +// n1.asExpr() = intentTypeTest.getInitializer() and // Variable +// //n1.asExpr() = intentTypeTest and // ClassInstanceExpr +// n2.asExpr() = startActivity.getArgument(0) // ! switch to getStartActivityIntentArg(startActivity) +// ) +// } +// } +// ! rename? +// ! below works as intended when run by itself (see latest query in AndroidDeeplinks_RemoteSources.ql), +// ! but not when combined with existing flow steps (non-monotonic recursion) +// ! need to figure out how to combine, or wrap all in global flow? +class StartComponentConfiguration extends DataFlow::Configuration { + StartComponentConfiguration() { this = "StartComponentConfiguration" } + + // Override `isSource` and `isSink`. + override predicate isSource(DataFlow::Node source) { + exists(ClassInstanceExpr classInstanceExpr | + classInstanceExpr.getConstructedType() instanceof TypeIntent and + source.asExpr() = classInstanceExpr + ) + } + + override predicate isSink(DataFlow::Node sink) { + exists(MethodAccess startActivity | + // ! need to handle for all components, not just Activity ( - // ! is there a better way to do this? startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) or startActivity.getMethod().overrides*(any(ActivityStartActivityMethod m)) ) and - intentTypeTest.getType() instanceof TypeIntent and - //startActivity.getFile().getBaseName() = "MainActivity.java" and // ! REMOVE - DataFlow::localExprFlow(intentTypeTest.getInitializer(), startActivity.getArgument(0)) and - n1.asExpr() = intentTypeTest.getInitializer() and - n2.asExpr() = startActivity.getArgument(0) // ! switch to getStartActivityIntentArg(startActivity) + sink.asExpr() = startActivity.getArgument(0) ) } + // Optionally override `isBarrier`. + // Optionally override `isAdditionalFlowStep`. + // Then, to query whether there is flow between some `source` and `sink`, + // write + // + // ```ql + // exists(MyAnalysisConfiguration cfg | cfg.hasFlow(source, sink)) + // ``` } /* ********************* INTENT METHODS, E.G. parseUri, getData, getExtras, etc. ********************* */ @@ -95,61 +137,8 @@ class AndroidGetDataMethod extends Method { */ class AndroidParseUriMethod extends Method { AndroidParseUriMethod() { - (this.hasName("parseUri") or this.hasName("getIntent")) and // ! Note to self: getIntent for older versions before deprecation to parseUri + // ! Note to self: getIntent for older versions before deprecation to parseUri + (this.hasName("parseUri") or this.hasName("getIntent")) and this.getDeclaringType() instanceof TypeIntent } } -// /** -// * A taint step from the Intent argument of a `startActivity` call to -// * a `Intent.parseUri` call in the Activity the Intent pointed to in its constructor. -// */ -// private class StartActivityParseUriStep extends AdditionalTaintStep { -// override predicate step(DataFlow::Node n1, DataFlow::Node n2) { -// exists(MethodAccess startActivity, MethodAccess parseUri, ClassInstanceExpr newIntent | -// startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) and -// parseUri.getMethod().overrides*(any(AndroidParseUriMethod m)) and -// newIntent.getConstructedType() instanceof TypeIntent and -// DataFlow::localExprFlow(newIntent, startActivity.getArgument(0)) and -// newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = -// parseUri.getReceiverType() and -// n1.asExpr() = startActivity.getArgument(0) and -// n2.asExpr() = parseUri -// ) -// } -// } -// /** -// * A taint step from the Intent argument of a `startActivity` call to -// * a `Intent.get%Extra%` call in the Activity the Intent pointed to in its constructor. -// */ -// private class StartActivityGetDataStep extends AdditionalTaintStep { -// override predicate step(DataFlow::Node n1, DataFlow::Node n2) { -// exists(MethodAccess startActivity, MethodAccess getData, ClassInstanceExpr newIntent | -// startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) and -// getData.getMethod().overrides*(any(AndroidGetDataMethod m)) and -// newIntent.getConstructedType() instanceof TypeIntent and -// DataFlow::localExprFlow(newIntent, startActivity.getArgument(0)) and -// newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = -// getData.getReceiverType() and -// n1.asExpr() = startActivity.getArgument(0) and -// n2.asExpr() = getData -// ) -// } -// } -// /** -// * A taint step from the Intent argument of a `startActivity` call to -// * a `Intent.getData` call in the Activity the Intent pointed to in its constructor. -// */ -// private class StartActivityGetExtrasStep extends AdditionalTaintStep { -// override predicate step(DataFlow::Node n1, DataFlow::Node n2) { -// exists(MethodAccess startActivity, MethodAccess getExtras, ClassInstanceExpr newIntent | -// startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) and -// getExtras.getMethod().overrides*(any(AndroidGetExtrasMethod m)) and -// newIntent.getConstructedType() instanceof TypeIntent and -// DataFlow::localExprFlow(newIntent, startActivity.getArgument(0)) and -// newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = -// getExtras.getReceiverType() and -// n1.asExpr() = startActivity.getArgument(0) and -// n2.asExpr() = getExtras -// ) -// } -// } diff --git a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll index 97af99fd966..7bdd9dd9911 100644 --- a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll +++ b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll @@ -1,9 +1,11 @@ import java private import semmle.code.java.dataflow.DataFlow +//private import semmle.code.java.dataflow.DataFlow2 private import semmle.code.java.dataflow.ExternalFlow private import semmle.code.java.dataflow.FlowSteps +private import semmle.code.java.frameworks.android.DeepLink -// ! Remember to add 'private' annotation as needed to all new classes/predicates below. +// ! Remember to add 'private' annotation as needed to new classes/predicates below. // ! and clean-up comments, etc. in below in general... /** * The class `android.content.Intent`. @@ -119,24 +121,31 @@ class ContextStartActivityMethod extends Method { } } -// ! finish QLDoc +// ! maybe reword below QLDoc? /** - * The method `Activity.startActivity` or ... + * The method `Activity.startActivity`,`startActivities`, + * `startActivityForResult`, `startActivityIfNeeded`, + * `startNextMatchingActivity`, `startActivityFromChild`, + * or `startActivityFromFragment`. */ class ActivityStartActivityMethod extends Method { ActivityStartActivityMethod() { - // ! captures all `startAct` methods in the Activity class - this.getName().matches("start%Activit%") and // ! better to list all instead of using matches for any reason? + this.getName().matches("start%Activit%") and this.getDeclaringType() instanceof TypeActivity } } +// ! maybe reword below QLDoc? /** - * The method `Context.sendBroadcast`. + * The method `Context.sendBroadcast`, `sendBroadcastAsUser`, + * `sendOrderedBroadcast`, `sendOrderedBroadcastAsUser`, + * `sendStickyBroadcast`, `sendStickyBroadcastAsUser`, + * `sendStickyOrderedBroadcast`, `sendStickyOrderedBroadcastAsUser`, + * or `sendBroadcastWithMultiplePermissions`. */ class ContextSendBroadcastMethod extends Method { ContextSendBroadcastMethod() { - this.getName().matches("send%Broadcast%") and // ! Note to self: matches the 9 sendBroadcast methods + this.getName().matches("send%Broadcast%") and this.getDeclaringType() instanceof TypeContext } } @@ -322,7 +331,7 @@ class StartActivityIntentStep extends AdditionalValueStep { override predicate step(DataFlow::Node n1, DataFlow::Node n2) { exists(MethodAccess startActivity, MethodAccess getIntent, ClassInstanceExpr newIntent | ( - // ! is there a better way to do this? + // ! look for better way to handle the below startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) or startActivity.getMethod().overrides*(any(ActivityStartActivityMethod m)) ) and @@ -331,10 +340,6 @@ class StartActivityIntentStep extends AdditionalValueStep { DataFlow::localExprFlow(newIntent, getStartActivityIntentArg(startActivity)) and getIntentConstructorClassArg(newIntent).getType().(ParameterizedType).getATypeArgument() = getIntent.getReceiverType() and - // ! below uses predicate `getArgumentByType` that I added to the Expr.qll lib, prbly should not add new predicate there. - // argType.getName().matches("Class<%>") and - // newIntent.getArgumentByType(argType).getType().(ParameterizedType).getATypeArgument() = - // getIntent.getReceiverType() and n1.asExpr() = getStartActivityIntentArg(startActivity) and n2.asExpr() = getIntent ) diff --git a/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_RemoteSources.ql b/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_RemoteSources.ql index 3b5402e5c01..d33cf4f3efb 100644 --- a/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_RemoteSources.ql +++ b/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_RemoteSources.ql @@ -17,9 +17,43 @@ // where startServiceIntStep.step(n1, n2) // select n2, "placeholder" // * experiment with taint-tracking +// import java +// import semmle.code.java.dataflow.TaintTracking +// import semmle.code.java.frameworks.android.DeepLink +// import semmle.code.java.frameworks.android.Intent +// import semmle.code.java.frameworks.android.Android +// import semmle.code.java.dataflow.DataFlow +// import semmle.code.java.dataflow.FlowSteps +// import semmle.code.java.dataflow.FlowSources +// import semmle.code.java.dataflow.ExternalFlow +// import semmle.code.xml.AndroidManifest +// import semmle.code.java.dataflow.TaintTracking +// class MyTaintTrackingConfiguration extends TaintTracking::Configuration { +// MyTaintTrackingConfiguration() { this = "MyTaintTrackingConfiguration" } +// override predicate isSource(DataFlow::Node source) { +// // exists(AndroidActivityXmlElement andActXmlElem | +// // andActXmlElem.hasDeepLink() and +// // source.asExpr() instanceof TypeActivity +// // ) +// source instanceof RemoteFlowSource and //AndroidIntentInput +// exists(AndroidComponent andComp | +// andComp.getAndroidComponentXmlElement().(AndroidActivityXmlElement).hasDeepLink() and +// source.asExpr().getFile() = andComp.getFile() // ! ugly, see if better way to do this +// ) +// } +// override predicate isSink(DataFlow::Node sink) { +// exists(MethodAccess m | +// m.getMethod().hasName("getIntent") and +// sink.asExpr() = m +// ) +// } +// } +// from DataFlow::Node src, DataFlow::Node sink, MyTaintTrackingConfiguration config +// where config.hasFlow(src, sink) +// select src, "This environment variable constructs a URL $@.", sink, "here" +// * experiment with GLOBAL FLOW import java import semmle.code.java.dataflow.TaintTracking -import semmle.code.java.frameworks.android.DeepLink import semmle.code.java.frameworks.android.Intent import semmle.code.java.frameworks.android.Android import semmle.code.java.dataflow.DataFlow @@ -29,29 +63,30 @@ import semmle.code.java.dataflow.ExternalFlow import semmle.code.xml.AndroidManifest import semmle.code.java.dataflow.TaintTracking -class MyTaintTrackingConfiguration extends TaintTracking::Configuration { - MyTaintTrackingConfiguration() { this = "MyTaintTrackingConfiguration" } +class StartComponentConfiguration extends DataFlow::Configuration { + StartComponentConfiguration() { this = "StartComponentConfiguration" } + // TODO: Override `isSource` and `isSink`. override predicate isSource(DataFlow::Node source) { - // exists(AndroidActivityXmlElement andActXmlElem | - // andActXmlElem.hasDeepLink() and - // source.asExpr() instanceof TypeActivity - // ) - source instanceof RemoteFlowSource and //AndroidIntentInput - exists(AndroidComponent andComp | - andComp.getAndroidComponentXmlElement().(AndroidActivityXmlElement).hasDeepLink() and - source.asExpr().getFile() = andComp.getFile() // ! ugly, see if better way to do this + exists(ClassInstanceExpr classInstanceExpr | + classInstanceExpr.getConstructedType() instanceof TypeIntent and + source.asExpr() = classInstanceExpr ) } override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess m | - m.getMethod().hasName("getIntent") and - sink.asExpr() = m + exists(MethodAccess startActivity | + ( + startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) or + startActivity.getMethod().overrides*(any(ActivityStartActivityMethod m)) + ) and + sink.asExpr() = startActivity.getArgument(0) ) } } -from DataFlow::Node src, DataFlow::Node sink, MyTaintTrackingConfiguration config -where config.hasFlow(src, sink) -select src, "This environment variable constructs a URL $@.", sink, "here" +from DataFlow::Node src, DataFlow::Node sink, StartComponentConfiguration config +where + config.hasFlow(src, sink) and + sink.asExpr().getFile().getBaseName() = "MainActivity.java" // ! just for faster testing, remove when done +select src, "This source flows to this $@.", sink, "sink" From d07babe3c52f0fbdee42f31534869c9ce309ffd2 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Wed, 7 Sep 2022 21:50:22 -0400 Subject: [PATCH 05/20] add initial framework for service and receiver test cases --- .../code/java/frameworks/android/DeepLink.qll | 2 +- .../frameworks/android/deeplink/Test.java | 69 +------------------ .../intent/TestStartComponentToIntent.java | 56 +++++++++++++++ 3 files changed, 58 insertions(+), 69 deletions(-) create mode 100644 java/ql/test/library-tests/frameworks/android/intent/TestStartComponentToIntent.java diff --git a/java/ql/lib/semmle/code/java/frameworks/android/DeepLink.qll b/java/ql/lib/semmle/code/java/frameworks/android/DeepLink.qll index 6f5c5b5c690..946e2e8af4f 100644 --- a/java/ql/lib/semmle/code/java/frameworks/android/DeepLink.qll +++ b/java/ql/lib/semmle/code/java/frameworks/android/DeepLink.qll @@ -22,7 +22,7 @@ private import semmle.code.xml.AndroidManifest // ! experimental - make a DeepLink step that combine Activity, Service, Receiver, etc. private class DeepLinkIntentStep extends AdditionalValueStep { // DeepLinkIntentStep() { - // this instanceof StartActivityIntentStep_ContextAndActivity or + // this instanceof StartActivityIntentStep or // this instanceof SendBroadcastReceiverIntentStep or // this instanceof StartServiceIntentStep // } diff --git a/java/ql/test/library-tests/frameworks/android/deeplink/Test.java b/java/ql/test/library-tests/frameworks/android/deeplink/Test.java index 752129d045b..67736c7e8fb 100644 --- a/java/ql/test/library-tests/frameworks/android/deeplink/Test.java +++ b/java/ql/test/library-tests/frameworks/android/deeplink/Test.java @@ -1,72 +1,5 @@ - -// !!! From AsyncTask, update for DeepLinks... !!! - -import android.os.AsyncTask; - +// ! adding tests in `intent` directory instead for now public class Test { - private static Object source(String kind) { - return null; - } - private static void sink(Object o) {} - - public void test() { - TestAsyncTask t = new TestAsyncTask(); - 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 { - @Override - protected Object doInBackground(Object... params) { - sink(params[0]); // $ hasTaintFlow=execute hasTaintFlow=executeOnExecutor - sink(params[1]); // $ SPURIOUS: hasTaintFlow=execute hasTaintFlow=executeOnExecutor - return null; - } - } - - private class SafeAsyncTask extends AsyncTask { - @Override - protected Object doInBackground(Object... params) { - sink(params[0]); // Safe - return null; - } - } - - static class TestConstructorTask extends AsyncTask { - 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 - } - - } } diff --git a/java/ql/test/library-tests/frameworks/android/intent/TestStartComponentToIntent.java b/java/ql/test/library-tests/frameworks/android/intent/TestStartComponentToIntent.java new file mode 100644 index 00000000000..889b50af762 --- /dev/null +++ b/java/ql/test/library-tests/frameworks/android/intent/TestStartComponentToIntent.java @@ -0,0 +1,56 @@ +import android.app.Activity; +import android.app.Service; +import android.content.BroadcastReceiver; +import android.content.Context; +import android.content.Intent; + +public class TestStartComponentToIntent { + + static Object source() { + return null; + } + + static void sink(Object sink) {} + + public void testActivity(Context ctx) { + Intent intent = new Intent(null, SomeActivity.class); + intent.putExtra("data", (String) source()); + ctx.startActivity(intent); + } + + static class SomeActivity extends Activity { + + public void testActivity() { + sink(getIntent().getStringExtra("data")); // $ hasValueFlow + } + } + + // ! WIP + public void testService(Context ctx) { + Intent intent = new Intent(null, SomeService.class); + intent.putExtra("data", (String) source()); + ctx.startService(intent); + } + + public void testBroadcastReceiver(Context ctx) { + Intent intent = new Intent(null, SomeBroadcastReceiver.class); + intent.putExtra("data", (String) source()); + ctx.sendBroadcast(intent); + } + + static class SomeService extends Service { + + public void test() { + // ! WIP + sink(getIntent().getStringExtra("data")); // $ hasValueFlow + } + } + + static class SomeBroadcastReceiver extends BroadcastReceiver { + + public void test() { + // ! WIP + sink(getIntent().getStringExtra("data")); // $ hasValueFlow + } + } +} From 47fcbdd4b46dd83343f989f0ce093340a2ff1398 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Wed, 28 Sep 2022 16:36:48 -0400 Subject: [PATCH 06/20] resolve merge conflict --- java/ql/lib/semmle/code/java/Expr.qll | 17 -- .../code/java/frameworks/android/DeepLink.qll | 144 --------------- .../code/java/frameworks/android/Intent.qll | 171 ++++++++---------- .../semmle/code/java/security/DeepLink.qll | 50 +++++ .../CWE-939/AndroidDeeplinks_Experimental.ql | 62 +++++++ .../CWE-939/AndroidDeeplinks_RemoteSources.ql | 92 ---------- .../CWE-939/AndroidDeeplinks_SimpleQuery.ql | 1 + .../frameworks/android/deeplink/Test.java | 5 - .../frameworks/android/deeplink/options | 1 - .../frameworks/android/deeplink/test.expected | 0 .../frameworks/android/deeplink/test.ql | 2 - .../intent/TestStartComponentToIntent.java | 11 +- 12 files changed, 198 insertions(+), 358 deletions(-) delete mode 100644 java/ql/lib/semmle/code/java/frameworks/android/DeepLink.qll create mode 100644 java/ql/lib/semmle/code/java/security/DeepLink.qll create mode 100644 java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_Experimental.ql delete mode 100644 java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_RemoteSources.ql delete mode 100644 java/ql/test/library-tests/frameworks/android/deeplink/Test.java delete mode 100644 java/ql/test/library-tests/frameworks/android/deeplink/options delete mode 100644 java/ql/test/library-tests/frameworks/android/deeplink/test.expected delete mode 100644 java/ql/test/library-tests/frameworks/android/deeplink/test.ql diff --git a/java/ql/lib/semmle/code/java/Expr.qll b/java/ql/lib/semmle/code/java/Expr.qll index 423de505070..2c969a8d442 100644 --- a/java/ql/lib/semmle/code/java/Expr.qll +++ b/java/ql/lib/semmle/code/java/Expr.qll @@ -1316,23 +1316,6 @@ class ClassInstanceExpr extends Expr, ConstructorCall, @classinstancexpr { result = this.getAnArgument() } - // ! remove below predicate after experimentation - /** - * Gets the argument provided to the constructor of this class instance creation expression - * of the specified Type. - */ - Expr getArgumentByType(Type type) { - exists(Argument arg | - arg = this.getAnArgument() and - arg.getType() = type and - result = arg - ) - // ! e.g. use above in below code in `StartActivityIntentStep` in Intent.qll - // argType.getName().matches("Class<%>") and - // newIntent.getArgumentByType(argType).getType().(ParameterizedType).getATypeArgument() = - // getIntent.getReceiverType() and - } - /** * Gets a type argument provided to the constructor of this class instance creation expression. * diff --git a/java/ql/lib/semmle/code/java/frameworks/android/DeepLink.qll b/java/ql/lib/semmle/code/java/frameworks/android/DeepLink.qll deleted file mode 100644 index 946e2e8af4f..00000000000 --- a/java/ql/lib/semmle/code/java/frameworks/android/DeepLink.qll +++ /dev/null @@ -1,144 +0,0 @@ -/** Provides classes and predicates to reason about deep links in Android. */ - -import java -private import semmle.code.java.frameworks.android.Intent -//private import semmle.code.java.frameworks.android.AsyncTask -private import semmle.code.java.frameworks.android.Android -private import semmle.code.java.dataflow.DataFlow -//private import semmle.code.java.dataflow.DataFlow2 -private import semmle.code.java.dataflow.FlowSteps -//private import semmle.code.java.dataflow.FlowSources -//private import semmle.code.java.dataflow.ExternalFlow -//private import semmle.code.java.dataflow.TaintTracking -private import semmle.code.xml.AndroidManifest - -// ! if keeping this class, should prbly move to security folder. -// ! Remember to add 'private' annotation as needed to all new classes/predicates below. -// ! and clean-up comments, etc. in below in general... -/** - * A value-preserving step from the Intent argument of a method call that starts a component to - * a `getIntent` call or `Intent` parameter in the component that the Intent pointed to in its constructor. - */ -// ! experimental - make a DeepLink step that combine Activity, Service, Receiver, etc. -private class DeepLinkIntentStep extends AdditionalValueStep { - // DeepLinkIntentStep() { - // this instanceof StartActivityIntentStep or - // this instanceof SendBroadcastReceiverIntentStep or - // this instanceof StartServiceIntentStep - // } - override predicate step(DataFlow::Node n1, DataFlow::Node n2) { - // ! simplify below - ( - exists(StartServiceIntentStep startServiceIntentStep | startServiceIntentStep.step(n1, n2)) - or - exists(SendBroadcastReceiverIntentStep sendBroadcastIntentStep | - sendBroadcastIntentStep.step(n1, n2) - ) - or - exists(StartActivityIntentStep startActivityIntentStep | startActivityIntentStep.step(n1, n2)) - ) and - exists(AndroidComponent andComp | - andComp.getAndroidComponentXmlElement().(AndroidActivityXmlElement).hasDeepLink() and - n1.asExpr().getFile() = andComp.getFile() // ! see if better way to do this - ) - } -} - -// // ! experimentation with global flow issue - REMOVE -// /** -// * A value-preserving step from the Intent variable -// * the `Intent` Parameter in the `startActivity`. -// */ -// class IntentVariableToStartActivityStep extends AdditionalValueStep { -// override predicate step(DataFlow::Node n1, DataFlow::Node n2) { -// exists( -// MethodAccess startActivity, Variable intentTypeTest, DataFlow2::Node source, -// DataFlow2::Node sink //ClassInstanceExpr intentTypeTest | -// | -// ( -// startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) or -// startActivity.getMethod().overrides*(any(ActivityStartActivityMethod m)) -// ) and -// intentTypeTest.getType() instanceof TypeIntent and // Variable -// //intentTypeTest.getConstructedType() instanceof TypeIntent and // ClassInstanceExpr -// startActivity.getFile().getBaseName() = "MainActivity.java" and // ! REMOVE - for testing only -// //exists(StartComponentConfiguration cfg | cfg.hasFlow(source, sink)) and // GLOBAL FLOW ATTEMPT -// DataFlow::localExprFlow(intentTypeTest.getInitializer(), startActivity.getArgument(0)) and // Variable - gives 5 results - misses the 1st ProfileActivity result since no variable with that one -// //DataFlow::localExprFlow(intentTypeTest, startActivity.getArgument(0)) and // ClassInstanceExpr -// n1.asExpr() = intentTypeTest.getInitializer() and // Variable -// //n1.asExpr() = intentTypeTest and // ClassInstanceExpr -// n2.asExpr() = startActivity.getArgument(0) // ! switch to getStartActivityIntentArg(startActivity) -// ) -// } -// } -// ! rename? -// ! below works as intended when run by itself (see latest query in AndroidDeeplinks_RemoteSources.ql), -// ! but not when combined with existing flow steps (non-monotonic recursion) -// ! need to figure out how to combine, or wrap all in global flow? -class StartComponentConfiguration extends DataFlow::Configuration { - StartComponentConfiguration() { this = "StartComponentConfiguration" } - - // Override `isSource` and `isSink`. - override predicate isSource(DataFlow::Node source) { - exists(ClassInstanceExpr classInstanceExpr | - classInstanceExpr.getConstructedType() instanceof TypeIntent and - source.asExpr() = classInstanceExpr - ) - } - - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess startActivity | - // ! need to handle for all components, not just Activity - ( - startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) or - startActivity.getMethod().overrides*(any(ActivityStartActivityMethod m)) - ) and - sink.asExpr() = startActivity.getArgument(0) - ) - } - // Optionally override `isBarrier`. - // Optionally override `isAdditionalFlowStep`. - // Then, to query whether there is flow between some `source` and `sink`, - // write - // - // ```ql - // exists(MyAnalysisConfiguration cfg | cfg.hasFlow(source, sink)) - // ``` -} - -/* ********************* INTENT METHODS, E.G. parseUri, getData, getExtras, etc. ********************* */ -/* - * Below is a Draft/Test of modelling `Intent.parseUri`, `Intent.getData`, - * and `Intent.getExtras` methods - */ - -// ! Check if can use pre-existing Synthetic Field. -/** - * The method `Intent.get%Extra` or `Intent.getExtras`. - */ -class AndroidGetExtrasMethod extends Method { - AndroidGetExtrasMethod() { - this.getName().matches("get%Extra%") and - this.getDeclaringType() instanceof TypeIntent - } -} - -/** - * The method `Intent.getData` - */ -class AndroidGetDataMethod extends Method { - AndroidGetDataMethod() { - this.hasName("getData") and this.getDeclaringType() instanceof TypeIntent - } -} - -/** - * The method `Intent.parseUri` - */ -class AndroidParseUriMethod extends Method { - AndroidParseUriMethod() { - // ! Note to self: getIntent for older versions before deprecation to parseUri - (this.hasName("parseUri") or this.hasName("getIntent")) and - this.getDeclaringType() instanceof TypeIntent - } -} diff --git a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll index 7bdd9dd9911..71d0df0b351 100644 --- a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll +++ b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll @@ -1,12 +1,10 @@ import java private import semmle.code.java.dataflow.DataFlow -//private import semmle.code.java.dataflow.DataFlow2 private import semmle.code.java.dataflow.ExternalFlow private import semmle.code.java.dataflow.FlowSteps -private import semmle.code.java.frameworks.android.DeepLink // ! Remember to add 'private' annotation as needed to new classes/predicates below. -// ! and clean-up comments, etc. in below in general... +// ! and clean-up comments, etc. in below in general before marking as ready-for-review. /** * The class `android.content.Intent`. */ @@ -68,31 +66,18 @@ class AndroidReceiveIntentMethod extends Method { } } -// ! confirm if peekService should be modelled since it takes an Intent as a parameter +// ! not sure if I like the name of the below class, but +// ! trying to be consistent with `AndroidReceiveIntentMethod` +// ! and `AndroidGetIntentMethod`... /** - * The method `BroadcastReceiver.peekService`. - */ -class BroadcastReceiverPeekServiceIntentMethod extends Method { - BroadcastReceiverPeekServiceIntentMethod() { - this.hasName("peekService") and this.getDeclaringType() instanceof TypeBroadcastReceiver - } -} - -// ! potentially reword the below QLDoc -/** - * A method of type Service that receives an Intent as a parameter. - * Namely, `Service.onStart`, `onStartCommand`, `onBind`, `onRebind` - * `onUnbind`, or `onTaskRemoved` + * A method of type Service that receives an Intent. + * Namely, `Service.onStart`, `onStartCommand`, `onBind`, + * `onRebind`, `onUnbind`, or `onTaskRemoved` */ class AndroidServiceIntentMethod extends Method { AndroidServiceIntentMethod() { ( - // this.hasName("onStart") or - // this.hasName("onStartCommand") or this.getName().matches("onStart%") or - // this.hasName("onBind") or - // this.hasName("onRebind") or - // this.hasName("onUnbind") or this.getName().matches("on%ind") or this.hasName("onTaskRemoved") ) and @@ -112,30 +97,22 @@ class AndroidServiceIntentMethod extends Method { } /** - * The method `Context.startActivity` or `startActivities`. + * The method `Context.startActivity`, `Context.startActivities`, + * `Activity.startActivity`,`Activity.startActivities`, + * `Activity.startActivityForResult`, `Activity.startActivityIfNeeded`, + * `Activity.startNextMatchingActivity`, `Activity.startActivityFromChild`, + * or `Activity.startActivityFromFragment`. */ -class ContextStartActivityMethod extends Method { - ContextStartActivityMethod() { - (this.hasName("startActivity") or this.hasName("startActivities")) and - this.getDeclaringType() instanceof TypeContext - } -} - -// ! maybe reword below QLDoc? -/** - * The method `Activity.startActivity`,`startActivities`, - * `startActivityForResult`, `startActivityIfNeeded`, - * `startNextMatchingActivity`, `startActivityFromChild`, - * or `startActivityFromFragment`. - */ -class ActivityStartActivityMethod extends Method { - ActivityStartActivityMethod() { +class StartActivityMethod extends Method { + StartActivityMethod() { this.getName().matches("start%Activit%") and - this.getDeclaringType() instanceof TypeActivity + ( + this.getDeclaringType() instanceof TypeContext or + this.getDeclaringType() instanceof TypeActivity + ) } } -// ! maybe reword below QLDoc? /** * The method `Context.sendBroadcast`, `sendBroadcastAsUser`, * `sendOrderedBroadcast`, `sendOrderedBroadcastAsUser`, @@ -143,30 +120,25 @@ class ActivityStartActivityMethod extends Method { * `sendStickyOrderedBroadcast`, `sendStickyOrderedBroadcastAsUser`, * or `sendBroadcastWithMultiplePermissions`. */ -class ContextSendBroadcastMethod extends Method { - ContextSendBroadcastMethod() { +class SendBroadcastMethod extends Method { + SendBroadcastMethod() { this.getName().matches("send%Broadcast%") and this.getDeclaringType() instanceof TypeContext } } -// ! update below QLDoc +// ! remove reference from below QLDoc? /** * The method `Context.startService`, `startForegroundService`, * `bindIsolatedService`, `bindService`, or `bindServiceAsUser`. + * * From https://developer.android.com/reference/android/app/Service: * "Services can be started with Context.startService() and Context.bindService()." */ -class ContextStartServiceMethod extends Method { - ContextStartServiceMethod() { +class StartServiceMethod extends Method { + StartServiceMethod() { ( - // this.hasName("startService") or - // this.hasName("startForegroundService") or - this.getName().matches("start%Service") - or - // this.hasName("bindIsolatedService") or - // this.hasName("bindService") or - // this.hasName("bindServiceAsUser") + this.getName().matches("start%Service") or this.getName().matches("bind%Service%") ) and this.getDeclaringType() instanceof TypeContext @@ -282,7 +254,7 @@ class GrantWriteUriPermissionFlag extends GrantUriPermissionFlag { GrantWriteUriPermissionFlag() { this.hasName("FLAG_GRANT_WRITE_URI_PERMISSION") } } -// ! OLD VERSION +// ! OLD VERSION - need to delete - keeping for now for reference // /** // * A value-preserving step from the Intent argument of a `startActivity` call to // * a `getIntent` call in the Activity the Intent pointed to in its constructor. @@ -301,46 +273,61 @@ class GrantWriteUriPermissionFlag extends GrantUriPermissionFlag { // ) // } // } +/* + * // ! TODO: create a parent class for the below three steps? + * // ! e.g. something like the below? + * // ! could put `getClassArgOfIntentConstructor` in parent class? + * + * // ! Also look into whether it's possible to reduce any code duplication + * // ! across the three steps in general. + * // class StartComponentIntentStep extends AdditionalValueStep { } + */ + +// The `android.Content.Intent` class has two constructors with an argument of type +// `Class`. One has the argument at position 1 and the other at position 3. +// https://developer.android.com/reference/android/content/Intent#public-constructors +private Argument getClassArgOfIntentConstructor(ClassInstanceExpr classInstanceExpr) { + classInstanceExpr.getConstructedType() instanceof TypeIntent and + if classInstanceExpr.getNumArgument() = 2 + then result = classInstanceExpr.getArgument(1) + else result = classInstanceExpr.getArgument(3) +} + /** * A value-preserving step from the Intent argument of a `startActivity` call to * a `getIntent` call in the Activity the Intent pointed to in its constructor. */ -class StartActivityIntentStep extends AdditionalValueStep { - // ! startActivityFromChild and startActivityFromFragment have Intent as argument(1), - // ! but rest have Intent as argument(0)... - // ! startActivityFromChild and startActivityFromFragment are also deprecated and - // ! may need to look into modelling androidx.fragment.app.Fragment.startActivity() as well - private Argument getStartActivityIntentArg(MethodAccess startActMethodAccess) { +private class StartActivityIntentStep extends AdditionalValueStep { + // The `startActivityFromChild` and `startActivityFromFragment` methods have + // an argument of type `Intent` at position 1, but the rest of the methods of + // type `StartActivityMethod` have an argument of type `Intent` at position 0. + private Argument getIntentArgOfStartActMethod(MethodAccess methodAccess) { + methodAccess.getMethod().overrides*(any(StartActivityMethod m)) and if - startActMethodAccess.getMethod().hasName("startActivityFromChild") or - startActMethodAccess.getMethod().hasName("startActivityFromFragment") - then result = startActMethodAccess.getArgument(1) - else result = startActMethodAccess.getArgument(0) - } - - // ! Intent has two constructors with Class parameter, only the first one with argument - // ! at position 1 was modelled before leading to lost flow. The second constructor with - // ! argument at position 3 needs to be modelled as well. - // ! See https://developer.android.com/reference/android/content/Intent#public-constructors - private Argument getIntentConstructorClassArg(ClassInstanceExpr intent) { - if intent.getNumArgument() = 2 - then result = intent.getArgument(1) - else result = intent.getArgument(3) + methodAccess.getMethod().hasName("startActivityFromChild") or + methodAccess.getMethod().hasName("startActivityFromFragment") + then result = methodAccess.getArgument(1) + else result = methodAccess.getArgument(0) } + // // The `android.Content.Intent` class has two constructors with an argument of type + // // `Class`. One has the argument at position 1 and the other at position 3. + // // https://developer.android.com/reference/android/content/Intent#public-constructors + // private Argument getClassArgOfIntentConstructor(ClassInstanceExpr classInstanceExpr) { + // classInstanceExpr.getConstructedType() instanceof TypeIntent and + // if classInstanceExpr.getNumArgument() = 2 + // then result = classInstanceExpr.getArgument(1) + // else result = classInstanceExpr.getArgument(3) + // } override predicate step(DataFlow::Node n1, DataFlow::Node n2) { exists(MethodAccess startActivity, MethodAccess getIntent, ClassInstanceExpr newIntent | - ( - // ! look for better way to handle the below - startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) or - startActivity.getMethod().overrides*(any(ActivityStartActivityMethod m)) - ) and + startActivity.getMethod().overrides*(any(StartActivityMethod m)) and getIntent.getMethod().overrides*(any(AndroidGetIntentMethod m)) and newIntent.getConstructedType() instanceof TypeIntent and - DataFlow::localExprFlow(newIntent, getStartActivityIntentArg(startActivity)) and - getIntentConstructorClassArg(newIntent).getType().(ParameterizedType).getATypeArgument() = + DataFlow::localExprFlow(newIntent, getIntentArgOfStartActMethod(startActivity)) and + getClassArgOfIntentConstructor(newIntent).getType().(ParameterizedType).getATypeArgument() = getIntent.getReceiverType() and - n1.asExpr() = getStartActivityIntentArg(startActivity) and + n1.asExpr() = getIntentArgOfStartActMethod(startActivity) and n2.asExpr() = getIntent ) } @@ -348,17 +335,17 @@ class StartActivityIntentStep extends AdditionalValueStep { /** * A value-preserving step from the Intent argument of a `sendBroadcast` call to - * the `Intent` Parameter in the `onStart` method of the BroadcastReceiver the + * the `Intent` parameter in the `onReceive` method of the BroadcastReceiver the * Intent pointed to in its constructor. */ -class SendBroadcastReceiverIntentStep extends AdditionalValueStep { +private class SendBroadcastReceiverIntentStep extends AdditionalValueStep { override predicate step(DataFlow::Node n1, DataFlow::Node n2) { exists(MethodAccess sendBroadcast, Method onReceive, ClassInstanceExpr newIntent | - sendBroadcast.getMethod().overrides*(any(ContextSendBroadcastMethod m)) and + sendBroadcast.getMethod().overrides*(any(SendBroadcastMethod m)) and onReceive.overrides*(any(AndroidReceiveIntentMethod m)) and newIntent.getConstructedType() instanceof TypeIntent and DataFlow::localExprFlow(newIntent, sendBroadcast.getArgument(0)) and - newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = + getClassArgOfIntentConstructor(newIntent).getType().(ParameterizedType).getATypeArgument() = onReceive.getDeclaringType() and n1.asExpr() = sendBroadcast.getArgument(0) and n2.asParameter() = onReceive.getParameter(1) @@ -366,20 +353,20 @@ class SendBroadcastReceiverIntentStep extends AdditionalValueStep { } } -// ! potentially update QLDoc (e.g. remove `onStart` reference) +// ! potentially reword QLDoc /** * A value-preserving step from the Intent argument of a `startService` call to - * the `Intent` Parameter in the `onStart` method of the Service the Intent pointed - * to in its constructor. + * the `Intent` parameter in an `AndroidServiceIntentMethod` of the Service the + * Intent pointed to in its constructor. */ -class StartServiceIntentStep extends AdditionalValueStep { +private class StartServiceIntentStep extends AdditionalValueStep { override predicate step(DataFlow::Node n1, DataFlow::Node n2) { exists(MethodAccess startService, Method serviceIntent, ClassInstanceExpr newIntent | - startService.getMethod().overrides*(any(ContextStartServiceMethod m)) and + startService.getMethod().overrides*(any(StartServiceMethod m)) and serviceIntent.overrides*(any(AndroidServiceIntentMethod m)) and newIntent.getConstructedType() instanceof TypeIntent and DataFlow::localExprFlow(newIntent, startService.getArgument(0)) and - newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = + getClassArgOfIntentConstructor(newIntent).getType().(ParameterizedType).getATypeArgument() = serviceIntent.getDeclaringType() and n1.asExpr() = startService.getArgument(0) and n2.asParameter() = serviceIntent.getParameter(0) diff --git a/java/ql/lib/semmle/code/java/security/DeepLink.qll b/java/ql/lib/semmle/code/java/security/DeepLink.qll new file mode 100644 index 00000000000..62d27187e84 --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/DeepLink.qll @@ -0,0 +1,50 @@ +/** Provides classes and predicates to reason about deep links in Android. */ + +import java +private import semmle.code.java.frameworks.android.Intent +private import semmle.code.java.frameworks.android.Android +private import semmle.code.java.dataflow.DataFlow +private import semmle.code.java.dataflow.TaintTracking +private import semmle.code.java.dataflow.FlowSteps +private import semmle.code.xml.AndroidManifest + +// ! Experimentation file +// /** +// * A value-preserving step from the Intent argument of a method call that starts a component to +// * a `getIntent` call or `Intent` parameter in the component that the Intent pointed to in its constructor. +// */ +// // ! experimental - make a DeepLink step that combine Activity, Service, Receiver, etc. +// private class DeepLinkIntentStep extends AdditionalValueStep { +// // DeepLinkIntentStep() { +// // this instanceof StartActivityIntentStep or +// // this instanceof SendBroadcastReceiverIntentStep or +// // this instanceof StartServiceIntentStep +// // } +// override predicate step(DataFlow::Node n1, DataFlow::Node n2) { +// // ! simplify below +// ( +// exists(StartServiceIntentStep startServiceIntentStep | startServiceIntentStep.step(n1, n2)) +// or +// exists(SendBroadcastReceiverIntentStep sendBroadcastIntentStep | +// sendBroadcastIntentStep.step(n1, n2) +// ) +// or +// exists(StartActivityIntentStep startActivityIntentStep | startActivityIntentStep.step(n1, n2)) +// ) and +// exists(AndroidComponent andComp | +// andComp.getAndroidComponentXmlElement().(AndroidActivityXmlElement).hasDeepLink() and +// n1.asExpr().getFile() = andComp.getFile() // ! see if better way to do this +// ) +// } +// } +// ! experimental modelling of `parseUri` +/** + * The method `Intent.parseUri` + */ +class AndroidParseUriMethod extends Method { + AndroidParseUriMethod() { + // ! Note: getIntent for older versions before deprecation to parseUri + (this.hasName("parseUri") or this.hasName("getIntent")) and + this.getDeclaringType() instanceof TypeIntent + } +} diff --git a/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_Experimental.ql b/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_Experimental.ql new file mode 100644 index 00000000000..f7ebc70a165 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_Experimental.ql @@ -0,0 +1,62 @@ +/** + * @name Android deep links + * @description Android deep links + * @problem.severity recommendation + * @security-severity 0.1 + * @id java/android/deeplinks + * @tags security + * external/cwe/cwe-939 + * @precision high + */ + +// ! REMOVE this file +// * experiment with StartActivityIntentStep +import java +import semmle.code.xml.AndroidManifest + +// import semmle.code.java.dataflow.DataFlow +// from StartServiceIntentStep startServiceIntStep, DataFlow::Node n1, DataFlow::Node n2 +// where startServiceIntStep.step(n1, n2) +// select n2, "placeholder" +// * experiment with Global Flow +// import java +// import semmle.code.java.dataflow.TaintTracking +// import semmle.code.java.frameworks.android.Intent +// import semmle.code.java.frameworks.android.Android +// import semmle.code.java.dataflow.DataFlow +// import semmle.code.java.dataflow.FlowSteps +// import semmle.code.java.dataflow.FlowSources +// import semmle.code.java.dataflow.ExternalFlow +// import semmle.code.xml.AndroidManifest +// import semmle.code.java.dataflow.TaintTracking +// class StartComponentConfiguration extends DataFlow::Configuration { +// StartComponentConfiguration() { this = "StartComponentConfiguration" } +// // Override `isSource` and `isSink`. +// override predicate isSource(DataFlow::Node source) { +// exists(ClassInstanceExpr classInstanceExpr | +// classInstanceExpr.getConstructedType() instanceof TypeIntent and +// source.asExpr() = classInstanceExpr +// ) +// } +// override predicate isSink(DataFlow::Node sink) { +// exists(MethodAccess startActivity | +// ( +// startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) or +// startActivity.getMethod().overrides*(any(ActivityStartActivityMethod m)) +// ) and +// sink.asExpr() = startActivity.getArgument(0) +// ) +// } +// } +// from +// DataFlow::Node src, DataFlow::Node sink, StartComponentConfiguration config, +// where +// config.hasFlow(src, sink) and +// sink.asExpr().getFile().getBaseName() = "MainActivity.java" // ! just for faster testing, remove when done +// select src, "This source flows to this $@.", sink, "sink" +// * simple query as placeholder +from AndroidActivityXmlElement actXmlElement +where + actXmlElement.hasDeepLink() and + not actXmlElement.getFile().(AndroidManifestXmlFile).isInBuildDirectory() +select actXmlElement, "A deeplink is used here." diff --git a/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_RemoteSources.ql b/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_RemoteSources.ql deleted file mode 100644 index d33cf4f3efb..00000000000 --- a/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_RemoteSources.ql +++ /dev/null @@ -1,92 +0,0 @@ -/** - * @name Android deep links - * @description Android deep links - * @problem.severity recommendation - * @security-severity 0.1 - * @id java/android/deeplinks - * @tags security - * external/cwe/cwe-939 - * @precision high - */ - -// * experiment with StartActivityIntentStep -// import java -// import semmle.code.java.frameworks.android.DeepLink -// import semmle.code.java.dataflow.DataFlow -// from StartServiceIntentStep startServiceIntStep, DataFlow::Node n1, DataFlow::Node n2 -// where startServiceIntStep.step(n1, n2) -// select n2, "placeholder" -// * experiment with taint-tracking -// import java -// import semmle.code.java.dataflow.TaintTracking -// import semmle.code.java.frameworks.android.DeepLink -// import semmle.code.java.frameworks.android.Intent -// import semmle.code.java.frameworks.android.Android -// import semmle.code.java.dataflow.DataFlow -// import semmle.code.java.dataflow.FlowSteps -// import semmle.code.java.dataflow.FlowSources -// import semmle.code.java.dataflow.ExternalFlow -// import semmle.code.xml.AndroidManifest -// import semmle.code.java.dataflow.TaintTracking -// class MyTaintTrackingConfiguration extends TaintTracking::Configuration { -// MyTaintTrackingConfiguration() { this = "MyTaintTrackingConfiguration" } -// override predicate isSource(DataFlow::Node source) { -// // exists(AndroidActivityXmlElement andActXmlElem | -// // andActXmlElem.hasDeepLink() and -// // source.asExpr() instanceof TypeActivity -// // ) -// source instanceof RemoteFlowSource and //AndroidIntentInput -// exists(AndroidComponent andComp | -// andComp.getAndroidComponentXmlElement().(AndroidActivityXmlElement).hasDeepLink() and -// source.asExpr().getFile() = andComp.getFile() // ! ugly, see if better way to do this -// ) -// } -// override predicate isSink(DataFlow::Node sink) { -// exists(MethodAccess m | -// m.getMethod().hasName("getIntent") and -// sink.asExpr() = m -// ) -// } -// } -// from DataFlow::Node src, DataFlow::Node sink, MyTaintTrackingConfiguration config -// where config.hasFlow(src, sink) -// select src, "This environment variable constructs a URL $@.", sink, "here" -// * experiment with GLOBAL FLOW -import java -import semmle.code.java.dataflow.TaintTracking -import semmle.code.java.frameworks.android.Intent -import semmle.code.java.frameworks.android.Android -import semmle.code.java.dataflow.DataFlow -import semmle.code.java.dataflow.FlowSteps -import semmle.code.java.dataflow.FlowSources -import semmle.code.java.dataflow.ExternalFlow -import semmle.code.xml.AndroidManifest -import semmle.code.java.dataflow.TaintTracking - -class StartComponentConfiguration extends DataFlow::Configuration { - StartComponentConfiguration() { this = "StartComponentConfiguration" } - - // TODO: Override `isSource` and `isSink`. - override predicate isSource(DataFlow::Node source) { - exists(ClassInstanceExpr classInstanceExpr | - classInstanceExpr.getConstructedType() instanceof TypeIntent and - source.asExpr() = classInstanceExpr - ) - } - - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess startActivity | - ( - startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) or - startActivity.getMethod().overrides*(any(ActivityStartActivityMethod m)) - ) and - sink.asExpr() = startActivity.getArgument(0) - ) - } -} - -from DataFlow::Node src, DataFlow::Node sink, StartComponentConfiguration config -where - config.hasFlow(src, sink) and - sink.asExpr().getFile().getBaseName() = "MainActivity.java" // ! just for faster testing, remove when done -select src, "This source flows to this $@.", sink, "sink" diff --git a/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_SimpleQuery.ql b/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_SimpleQuery.ql index 4c6ce50cd3d..8ca8506ef90 100644 --- a/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_SimpleQuery.ql +++ b/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_SimpleQuery.ql @@ -14,6 +14,7 @@ import java import semmle.code.xml.AndroidManifest // simple query for testing and MRVA results +// ! REMOVE this file from AndroidActivityXmlElement actXmlElement where actXmlElement.hasDeepLink() and diff --git a/java/ql/test/library-tests/frameworks/android/deeplink/Test.java b/java/ql/test/library-tests/frameworks/android/deeplink/Test.java deleted file mode 100644 index 67736c7e8fb..00000000000 --- a/java/ql/test/library-tests/frameworks/android/deeplink/Test.java +++ /dev/null @@ -1,5 +0,0 @@ -// ! adding tests in `intent` directory instead for now -public class Test { - - -} diff --git a/java/ql/test/library-tests/frameworks/android/deeplink/options b/java/ql/test/library-tests/frameworks/android/deeplink/options deleted file mode 100644 index 33cdc1ea940..00000000000 --- a/java/ql/test/library-tests/frameworks/android/deeplink/options +++ /dev/null @@ -1 +0,0 @@ -//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/google-android-9.0.0 diff --git a/java/ql/test/library-tests/frameworks/android/deeplink/test.expected b/java/ql/test/library-tests/frameworks/android/deeplink/test.expected deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/java/ql/test/library-tests/frameworks/android/deeplink/test.ql b/java/ql/test/library-tests/frameworks/android/deeplink/test.ql deleted file mode 100644 index 5d91e4e8e26..00000000000 --- a/java/ql/test/library-tests/frameworks/android/deeplink/test.ql +++ /dev/null @@ -1,2 +0,0 @@ -import java -import TestUtilities.InlineFlowTest diff --git a/java/ql/test/library-tests/frameworks/android/intent/TestStartComponentToIntent.java b/java/ql/test/library-tests/frameworks/android/intent/TestStartComponentToIntent.java index 889b50af762..bcc192958b4 100644 --- a/java/ql/test/library-tests/frameworks/android/intent/TestStartComponentToIntent.java +++ b/java/ql/test/library-tests/frameworks/android/intent/TestStartComponentToIntent.java @@ -10,7 +10,8 @@ public class TestStartComponentToIntent { return null; } - static void sink(Object sink) {} + static void sink(Object sink) { + } public void testActivity(Context ctx) { Intent intent = new Intent(null, SomeActivity.class); @@ -26,19 +27,19 @@ public class TestStartComponentToIntent { } // ! WIP - public void testService(Context ctx) { + public void testService(Context ctx) { Intent intent = new Intent(null, SomeService.class); intent.putExtra("data", (String) source()); ctx.startService(intent); } - public void testBroadcastReceiver(Context ctx) { + public void testBroadcastReceiver(Context ctx) { Intent intent = new Intent(null, SomeBroadcastReceiver.class); intent.putExtra("data", (String) source()); ctx.sendBroadcast(intent); } - static class SomeService extends Service { + static class SomeService extends Service { public void test() { // ! WIP @@ -46,7 +47,7 @@ public class TestStartComponentToIntent { } } - static class SomeBroadcastReceiver extends BroadcastReceiver { + static class SomeBroadcastReceiver extends BroadcastReceiver { public void test() { // ! WIP From af812cf40768352cab9f10c25e6abe5e439b0c67 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Tue, 20 Sep 2022 15:19:26 -0400 Subject: [PATCH 07/20] fix code scanning bot warnings --- java/ql/lib/semmle/code/java/frameworks/android/Intent.qll | 4 ++-- java/ql/lib/semmle/code/java/security/DeepLink.qll | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll index 71d0df0b351..0faa83742a2 100644 --- a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll +++ b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll @@ -324,10 +324,10 @@ private class StartActivityIntentStep extends AdditionalValueStep { startActivity.getMethod().overrides*(any(StartActivityMethod m)) and getIntent.getMethod().overrides*(any(AndroidGetIntentMethod m)) and newIntent.getConstructedType() instanceof TypeIntent and - DataFlow::localExprFlow(newIntent, getIntentArgOfStartActMethod(startActivity)) and + DataFlow::localExprFlow(newIntent, this.getIntentArgOfStartActMethod(startActivity)) and getClassArgOfIntentConstructor(newIntent).getType().(ParameterizedType).getATypeArgument() = getIntent.getReceiverType() and - n1.asExpr() = getIntentArgOfStartActMethod(startActivity) and + n1.asExpr() = this.getIntentArgOfStartActMethod(startActivity) and n2.asExpr() = getIntent ) } diff --git a/java/ql/lib/semmle/code/java/security/DeepLink.qll b/java/ql/lib/semmle/code/java/security/DeepLink.qll index 62d27187e84..c00810a27fc 100644 --- a/java/ql/lib/semmle/code/java/security/DeepLink.qll +++ b/java/ql/lib/semmle/code/java/security/DeepLink.qll @@ -37,7 +37,7 @@ private import semmle.code.xml.AndroidManifest // ) // } // } -// ! experimental modelling of `parseUri` +// ! experimental modeling of `parseUri` /** * The method `Intent.parseUri` */ From 0f643610655852e8cb937a2df6ca6ab8575c9590 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Tue, 20 Sep 2022 15:39:36 -0400 Subject: [PATCH 08/20] remove simple query --- .../lib/semmle/code/xml/AndroidManifest.qll | 24 ------- .../CWE-939/AndroidDeeplinks_Experimental.ql | 62 ------------------- .../CWE-939/AndroidDeeplinks_SimpleQuery.ql | 22 ------- 3 files changed, 108 deletions(-) delete mode 100644 java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_Experimental.ql delete mode 100644 java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_SimpleQuery.ql diff --git a/java/ql/lib/semmle/code/xml/AndroidManifest.qll b/java/ql/lib/semmle/code/xml/AndroidManifest.qll index 0a8e92b0252..f53da67a650 100644 --- a/java/ql/lib/semmle/code/xml/AndroidManifest.qll +++ b/java/ql/lib/semmle/code/xml/AndroidManifest.qll @@ -129,30 +129,6 @@ class AndroidApplicationXmlElement extends XmlElement { */ class AndroidActivityXmlElement extends AndroidComponentXmlElement { AndroidActivityXmlElement() { this.getName() = "activity" } - - // ! Consider moving this to its own .qll file under `security` like for Implicit Export Query. - // ! Double-check that the below actions and categories are REQUIRED for it to - // ! count as a deep link versus just recommended (e.g. should I just look at the - // ! data element instead?). - // ! Reference: https://developer.android.com/training/app-links/deep-linking#adding-filters - // ! Note: not excluding App Links since those are a subset of deep links that can still cause issues. - /** - * Holds if this `` element has a deep link. - */ - predicate hasDeepLink() { - //exists(this.getAnIntentFilterElement()) and // has an intent filter - below all show that it has an intent-filter, duplicates work - this.getAnIntentFilterElement().getAnActionElement().getActionName() = - "android.intent.action.VIEW" and - this.getAnIntentFilterElement().getACategoryElement().getCategoryName() = - "android.intent.category.BROWSABLE" and - this.getAnIntentFilterElement().getACategoryElement().getCategoryName() = - "android.intent.category.DEFAULT" and - //this.getAnIntentFilterElement().getAChild("data").hasAttribute("scheme") // use below instead for 'android' prefix - exists(AndroidXmlAttribute attr | - this.getAnIntentFilterElement().getAChild("data").getAnAttribute() = attr and - attr.getName() = "scheme" - ) - } } /** diff --git a/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_Experimental.ql b/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_Experimental.ql deleted file mode 100644 index f7ebc70a165..00000000000 --- a/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_Experimental.ql +++ /dev/null @@ -1,62 +0,0 @@ -/** - * @name Android deep links - * @description Android deep links - * @problem.severity recommendation - * @security-severity 0.1 - * @id java/android/deeplinks - * @tags security - * external/cwe/cwe-939 - * @precision high - */ - -// ! REMOVE this file -// * experiment with StartActivityIntentStep -import java -import semmle.code.xml.AndroidManifest - -// import semmle.code.java.dataflow.DataFlow -// from StartServiceIntentStep startServiceIntStep, DataFlow::Node n1, DataFlow::Node n2 -// where startServiceIntStep.step(n1, n2) -// select n2, "placeholder" -// * experiment with Global Flow -// import java -// import semmle.code.java.dataflow.TaintTracking -// import semmle.code.java.frameworks.android.Intent -// import semmle.code.java.frameworks.android.Android -// import semmle.code.java.dataflow.DataFlow -// import semmle.code.java.dataflow.FlowSteps -// import semmle.code.java.dataflow.FlowSources -// import semmle.code.java.dataflow.ExternalFlow -// import semmle.code.xml.AndroidManifest -// import semmle.code.java.dataflow.TaintTracking -// class StartComponentConfiguration extends DataFlow::Configuration { -// StartComponentConfiguration() { this = "StartComponentConfiguration" } -// // Override `isSource` and `isSink`. -// override predicate isSource(DataFlow::Node source) { -// exists(ClassInstanceExpr classInstanceExpr | -// classInstanceExpr.getConstructedType() instanceof TypeIntent and -// source.asExpr() = classInstanceExpr -// ) -// } -// override predicate isSink(DataFlow::Node sink) { -// exists(MethodAccess startActivity | -// ( -// startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) or -// startActivity.getMethod().overrides*(any(ActivityStartActivityMethod m)) -// ) and -// sink.asExpr() = startActivity.getArgument(0) -// ) -// } -// } -// from -// DataFlow::Node src, DataFlow::Node sink, StartComponentConfiguration config, -// where -// config.hasFlow(src, sink) and -// sink.asExpr().getFile().getBaseName() = "MainActivity.java" // ! just for faster testing, remove when done -// select src, "This source flows to this $@.", sink, "sink" -// * simple query as placeholder -from AndroidActivityXmlElement actXmlElement -where - actXmlElement.hasDeepLink() and - not actXmlElement.getFile().(AndroidManifestXmlFile).isInBuildDirectory() -select actXmlElement, "A deeplink is used here." diff --git a/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_SimpleQuery.ql b/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_SimpleQuery.ql deleted file mode 100644 index 8ca8506ef90..00000000000 --- a/java/ql/src/Security/CWE/CWE-939/AndroidDeeplinks_SimpleQuery.ql +++ /dev/null @@ -1,22 +0,0 @@ -/** - * @name Android deep links - * @description Android deep links - * @kind problem - * @problem.severity recommendation - * @security-severity 0.1 - * @id java/android/deeplinks - * @tags security - * external/cwe/cwe-939 - * @precision high - */ - -import java -import semmle.code.xml.AndroidManifest - -// simple query for testing and MRVA results -// ! REMOVE this file -from AndroidActivityXmlElement actXmlElement -where - actXmlElement.hasDeepLink() and - not actXmlElement.getFile().(AndroidManifestXmlFile).isInBuildDirectory() -select actXmlElement, "A deeplink is used here." From 0a135a7f211b5dcda8d907cffad17892205bb29a Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Wed, 28 Sep 2022 16:40:47 -0400 Subject: [PATCH 09/20] resolve merge conflict --- .../intent/TestStartActivityToGetIntent.java | 58 +++++++- .../TestStartBroadcastReceiverToIntent.java | 42 ++++++ .../intent/TestStartComponentToIntent.java | 4 +- .../intent/TestStartServiceToIntent.java | 99 +++++++++++++ .../android/app/Service.java | 132 +++++++++++------- 5 files changed, 275 insertions(+), 60 deletions(-) create mode 100644 java/ql/test/library-tests/frameworks/android/intent/TestStartBroadcastReceiverToIntent.java create mode 100644 java/ql/test/library-tests/frameworks/android/intent/TestStartServiceToIntent.java diff --git a/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java b/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java index 3d497aac93d..115b5673eab 100644 --- a/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java +++ b/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java @@ -2,24 +2,68 @@ import android.app.Activity; import android.content.Context; import android.content.Intent; +// ! Original - saving for reference +// public class TestStartActivityToGetIntent { + +// static Object source() { +// return null; +// } + +// static void sink(Object sink) { +// } + +// public void test(Context ctx) { +// Intent intent = new Intent(null, SomeActivity.class); +// intent.putExtra("data", (String) source()); +// ctx.startActivity(intent); +// } + +// static class SomeActivity extends Activity { + +// public void test() { +// sink(getIntent().getStringExtra("data")); // $ hasValueFlow +// } +// } +// } + public class TestStartActivityToGetIntent { - static Object source() { + static Object source(String kind) { return null; } - static void sink(Object sink) {} + static void sink(Object sink) { + } - public void test(Context ctx) { - Intent intent = new Intent(null, SomeActivity.class); - intent.putExtra("data", (String) source()); - ctx.startActivity(intent); + public void test(Context ctx, Activity act) { + { + Intent intentCtx = new Intent(null, SomeActivity.class); + Intent intentAct = new Intent(null, SomeActivity.class); + intentCtx.putExtra("data", (String) source("context")); + intentAct.putExtra("data", (String) source("activity")); + ctx.startActivity(intentCtx); + act.startActivity(intentAct); + } + + { + Intent intentCtx = new Intent(null, SafeActivity.class); + Intent intentAct = new Intent(null, SafeActivity.class); + ctx.startActivity(intentCtx); + act.startActivity(intentAct); + } } static class SomeActivity extends Activity { public void test() { - sink(getIntent().getStringExtra("data")); // $ hasValueFlow + sink(getIntent().getStringExtra("data")); // $ hasValueFlow=context hasValueFlow=activity + } + } + + static class SafeActivity extends Activity { + + public void test() { + sink(getIntent().getStringExtra("data")); // Safe } } } diff --git a/java/ql/test/library-tests/frameworks/android/intent/TestStartBroadcastReceiverToIntent.java b/java/ql/test/library-tests/frameworks/android/intent/TestStartBroadcastReceiverToIntent.java new file mode 100644 index 00000000000..4cbc4f6e563 --- /dev/null +++ b/java/ql/test/library-tests/frameworks/android/intent/TestStartBroadcastReceiverToIntent.java @@ -0,0 +1,42 @@ +import android.content.BroadcastReceiver; +import android.content.Context; +import android.content.Intent; + +public class TestStartBroadcastReceiverToIntent { + + static Object source() { + return null; + } + + static void sink(Object sink) { + } + + public void test(Context ctx) { + { + Intent intent = new Intent(null, SomeBroadcastReceiver.class); + intent.putExtra("data", (String) source()); + ctx.sendBroadcast(intent); + } + + { + Intent intent = new Intent(null, SafeBroadcastReceiver.class); + ctx.sendBroadcast(intent); + } + } + + static class SomeBroadcastReceiver extends BroadcastReceiver { + + @Override + public void onReceive(Context context, Intent intent) { + sink(intent.getStringExtra("data")); // $ hasValueFlow + } + } + + static class SafeBroadcastReceiver extends BroadcastReceiver { + + @Override + public void onReceive(Context context, Intent intent) { + sink(intent.getStringExtra("data")); // Safe + } + } +} diff --git a/java/ql/test/library-tests/frameworks/android/intent/TestStartComponentToIntent.java b/java/ql/test/library-tests/frameworks/android/intent/TestStartComponentToIntent.java index bcc192958b4..ea5eee19d5b 100644 --- a/java/ql/test/library-tests/frameworks/android/intent/TestStartComponentToIntent.java +++ b/java/ql/test/library-tests/frameworks/android/intent/TestStartComponentToIntent.java @@ -1,6 +1,6 @@ import android.app.Activity; -import android.app.Service; -import android.content.BroadcastReceiver; +//import android.app.Service; +//import android.content.BroadcastReceiver; import android.content.Context; import android.content.Intent; diff --git a/java/ql/test/library-tests/frameworks/android/intent/TestStartServiceToIntent.java b/java/ql/test/library-tests/frameworks/android/intent/TestStartServiceToIntent.java new file mode 100644 index 00000000000..edfa3d18864 --- /dev/null +++ b/java/ql/test/library-tests/frameworks/android/intent/TestStartServiceToIntent.java @@ -0,0 +1,99 @@ +import android.app.Service; +import android.os.IBinder; +import android.content.Context; +import android.content.Intent; + +public class TestStartServiceToIntent { + + static Object source() { + return null; + } + + static void sink(Object sink) { + } + + public void test(Context ctx) { + { + Intent intent = new Intent(null, SomeService.class); + intent.putExtra("data", (String) source()); + ctx.startService(intent); + } + + { + Intent intent = new Intent(null, SafeService.class); + ctx.startService(intent); + } + } + + static class SomeService extends Service { + + @Override + public void onStart(Intent intent, int startId) { + sink(intent.getStringExtra("data")); // $ hasValueFlow + } + + @Override + public int onStartCommand(Intent intent, int flags, int startId) { + sink(intent.getStringExtra("data")); // $ hasValueFlow + return -1; + } + + @Override + public IBinder onBind(Intent intent) { + sink(intent.getStringExtra("data")); // $ hasValueFlow + return null; + } + + @Override + public boolean onUnbind(Intent intent) { + sink(intent.getStringExtra("data")); // $ hasValueFlow + return false; + } + + @Override + public void onRebind(Intent intent) { + sink(intent.getStringExtra("data")); // $ hasValueFlow + } + + @Override + public void onTaskRemoved(Intent intent) { + sink(intent.getStringExtra("data")); // $ hasValueFlow + } + } + + static class SafeService extends Service { + + @Override + public void onStart(Intent intent, int startId) { + sink(intent.getStringExtra("data")); // Safe + } + + @Override + public int onStartCommand(Intent intent, int flags, int startId) { + sink(intent.getStringExtra("data")); // Safe + return -1; + } + + @Override + public IBinder onBind(Intent intent) { + sink(intent.getStringExtra("data")); // Safe + return null; + } + + @Override + public boolean onUnbind(Intent intent) { + sink(intent.getStringExtra("data")); // Safe + return false; + } + + @Override + public void onRebind(Intent intent) { + sink(intent.getStringExtra("data")); // Safe + } + + @Override + public void onTaskRemoved(Intent intent) { + sink(intent.getStringExtra("data")); // Safe + } + } +} diff --git a/java/ql/test/stubs/google-android-9.0.0/android/app/Service.java b/java/ql/test/stubs/google-android-9.0.0/android/app/Service.java index 8250b8917c2..d47dadcce4f 100644 --- a/java/ql/test/stubs/google-android-9.0.0/android/app/Service.java +++ b/java/ql/test/stubs/google-android-9.0.0/android/app/Service.java @@ -20,12 +20,13 @@ import android.content.ContextWrapper; import android.os.IBinder; /** - * A Service is an application component representing either an application's desire + * A Service is an application component representing either an application's + * desire * to perform a longer-running operation while not interacting with the user - * or to supply functionality for other applications to use. Each service + * or to supply functionality for other applications to use. Each service * class must have a corresponding * {@link android.R.styleable#AndroidManifestService <service>} - * declaration in its package's AndroidManifest.xml. Services + * declaration in its package's AndroidManifest.xml. Services * can be started with * {@link android.content.Context#startService Context.startService()} and * {@link android.content.Context#bindService Context.bindService()}. @@ -34,9 +35,10 @@ import android.os.IBinder; * thread of their hosting process. This means that, if your service is going * to do any CPU intensive (such as MP3 playback) or blocking (such as * networking) operations, it should spawn its own thread in which to do that - * work. More information on this can be found in - * Processes and - * Threads. The {@link IntentService} class is available + * work. More information on this can be found in + * Processes and + * Threads. The {@link IntentService} class is available * as a standard implementation of Service that has its own thread where it * schedules its work to be done.

* @@ -47,13 +49,18 @@ import android.os.IBinder; *
  • Permissions *
  • Process Lifecycle *
  • Local Service Sample - *
  • Remote Messenger Service Sample + *
  • Remote Messenger Service + * Sample * * *
    *

    Developer Guides

    - *

    For a detailed discussion about how to create services, read the - * Services developer guide.

    + *

    + * For a detailed discussion about how to create services, read the + * Services developer + * guide. + *

    *
    * * @@ -63,10 +70,10 @@ import android.os.IBinder; * it is not:

    * *
      - *
    • A Service is not a separate process. The Service object itself + *
    • A Service is not a separate process. The Service object itself * does not imply it is running in its own process; unless otherwise specified, * it runs in the same process as the application it is part of. - *
    • A Service is not a thread. It is not a means itself to do work off + *
    • A Service is not a thread. It is not a means itself to do work off * of the main thread (to avoid Application Not Responding errors). *
    * @@ -75,12 +82,12 @@ import android.os.IBinder; *
      *
    • A facility for the application to tell the system about * something it wants to be doing in the background (even when the user is not - * directly interacting with the application). This corresponds to calls to + * directly interacting with the application). This corresponds to calls to * {@link android.content.Context#startService Context.startService()}, which * ask the system to schedule work for the service, to be run until the service * or someone else explicitly stop it. *
    • A facility for an application to expose some of its functionality to - * other applications. This corresponds to calls to + * other applications. This corresponds to calls to * {@link android.content.Context#bindService Context.bindService()}, which * allows a long-standing connection to be made to the service in order to * interact with it. @@ -105,11 +112,14 @@ import android.os.IBinder; * calls {@link android.content.Context#startService Context.startService()} then the system will * retrieve the service (creating it and calling its {@link #onCreate} method * if needed) and then call its {@link #onStartCommand} method with the - * arguments supplied by the client. The service will at this point continue - * running until {@link android.content.Context#stopService Context.stopService()} or - * {@link #stopSelf()} is called. Note that multiple calls to - * Context.startService() do not nest (though they do result in multiple corresponding - * calls to onStartCommand()), so no matter how many times it is started a service + * arguments supplied by the client. The service will at this point continue + * running until {@link android.content.Context#stopService + * Context.stopService()} or + * {@link #stopSelf()} is called. Note that multiple calls to + * Context.startService() do not nest (though they do result in multiple + * corresponding + * calls to onStartCommand()), so no matter how many times it is started a + * service * will be stopped once Context.stopService() or stopSelf() is called; however, * services can use their {@link #stopSelf(int)} method to ensure the service is * not stopped until started intents have been processed. @@ -119,28 +129,29 @@ import android.os.IBinder; * onStartCommand(): {@link #START_STICKY} is used for services that are * explicitly started and stopped as needed, while {@link #START_NOT_STICKY} * or {@link #START_REDELIVER_INTENT} are used for services that should only - * remain running while processing any commands sent to them. See the linked + * remain running while processing any commands sent to them. See the linked * documentation for more detail on the semantics. * *

      Clients can also use {@link android.content.Context#bindService Context.bindService()} to * obtain a persistent connection to a service. This likewise creates the * service if it is not already running (calling {@link #onCreate} while - * doing so), but does not call onStartCommand(). The client will receive the + * doing so), but does not call onStartCommand(). The client will receive the * {@link android.os.IBinder} object that the service returns from its * {@link #onBind} method, allowing the client to then make calls back - * to the service. The service will remain running as long as the connection + * to the service. The service will remain running as long as the connection * is established (whether or not the client retains a reference on the - * service's IBinder). Usually the IBinder returned is for a complex - * interface that has been written + * service's IBinder). Usually the IBinder returned is for a complex + * interface that has been + * written * in aidl. * *

      A service can be both started and have connections bound to it. In such * a case, the system will keep the service running as long as either it is * started or there are one or more connections to it with the * {@link android.content.Context#BIND_AUTO_CREATE Context.BIND_AUTO_CREATE} - * flag. Once neither + * flag. Once neither * of these situations hold, the service's {@link #onDestroy} method is called - * and the service is effectively terminated. All cleanup (stopping threads, + * and the service is effectively terminated. All cleanup (stopping threads, * unregistering receivers) should be complete upon returning from onDestroy(). * * @@ -148,24 +159,28 @@ import android.os.IBinder; * *

      Global access to a service can be enforced when it is declared in its * manifest's {@link android.R.styleable#AndroidManifestService <service>} - * tag. By doing so, other applications will need to declare a corresponding - * {@link android.R.styleable#AndroidManifestUsesPermission <uses-permission>} + * tag. By doing so, other applications will need to declare a corresponding + * {@link android.R.styleable#AndroidManifestUsesPermission + * <uses-permission>} * element in their own manifest to be able to start, stop, or bind to * the service. * - *

      As of {@link android.os.Build.VERSION_CODES#GINGERBREAD}, when using + *

      + * As of {@link android.os.Build.VERSION_CODES#GINGERBREAD}, when using * {@link Context#startService(Intent) Context.startService(Intent)}, you can * also set {@link Intent#FLAG_GRANT_READ_URI_PERMISSION - * Intent.FLAG_GRANT_READ_URI_PERMISSION} and/or {@link Intent#FLAG_GRANT_WRITE_URI_PERMISSION - * Intent.FLAG_GRANT_WRITE_URI_PERMISSION} on the Intent. This will grant the - * Service temporary access to the specific URIs in the Intent. Access will + * Intent.FLAG_GRANT_READ_URI_PERMISSION} and/or + * {@link Intent#FLAG_GRANT_WRITE_URI_PERMISSION + * Intent.FLAG_GRANT_WRITE_URI_PERMISSION} on the Intent. This will grant the + * Service temporary access to the specific URIs in the Intent. Access will * remain until the Service has called {@link #stopSelf(int)} for that start * command or a later one, or until the Service has been completely stopped. * This works for granting access to the other apps that have not requested * the permission protecting the Service, or even when the Service is not * exported at all. * - *

      In addition, a service can protect individual IPC calls into it with + *

      + * In addition, a service can protect individual IPC calls into it with * permissions, by calling the * {@link #checkCallingPermission} * method before executing the implementation of that call. @@ -183,32 +198,44 @@ import android.os.IBinder; * following possibilities: * *

        - *

      • If the service is currently executing code in its + *

      • + *

        + * If the service is currently executing code in its * {@link #onCreate onCreate()}, {@link #onStartCommand onStartCommand()}, * or {@link #onDestroy onDestroy()} methods, then the hosting process will * be a foreground process to ensure this code can execute without * being killed. - *

      • If the service has been started, then its hosting process is considered + *

      • + *

        + * If the service has been started, then its hosting process is considered * to be less important than any processes that are currently visible to the - * user on-screen, but more important than any process not visible. Because + * user on-screen, but more important than any process not visible. Because * only a few processes are generally visible to the user, this means that - * the service should not be killed except in low memory conditions. However, since - * the user is not directly aware of a background service, in that state it is + * the service should not be killed except in low memory conditions. However, + * since + * the user is not directly aware of a background service, in that state it + * is * considered a valid candidate to kill, and you should be prepared for this to - * happen. In particular, long-running services will be increasingly likely to + * happen. In particular, long-running services will be increasingly likely to * kill and are guaranteed to be killed (and restarted if appropriate) if they * remain started long enough. - *

      • If there are clients bound to the service, then the service's hosting - * process is never less important than the most important client. That is, + *

      • + *

        + * If there are clients bound to the service, then the service's hosting + * process is never less important than the most important client. That is, * if one of its clients is visible to the user, then the service itself is - * considered to be visible. The way a client's importance impacts the service's + * considered to be visible. The way a client's importance impacts the service's * importance can be adjusted through {@link Context#BIND_ABOVE_CLIENT}, - * {@link Context#BIND_ALLOW_OOM_MANAGEMENT}, {@link Context#BIND_WAIVE_PRIORITY}, - * {@link Context#BIND_IMPORTANT}, and {@link Context#BIND_ADJUST_WITH_ACTIVITY}. - *

      • A started service can use the {@link #startForeground(int, Notification)} + * {@link Context#BIND_ALLOW_OOM_MANAGEMENT}, + * {@link Context#BIND_WAIVE_PRIORITY}, + * {@link Context#BIND_IMPORTANT}, and + * {@link Context#BIND_ADJUST_WITH_ACTIVITY}. + *

      • + *

        + * A started service can use the {@link #startForeground(int, Notification)} * API to put the service in a foreground state, where the system considers * it to be something the user is actively aware of and thus not a candidate - * for killing when low on memory. (It is still theoretically possible for + * for killing when low on memory. (It is still theoretically possible for * the service to be killed under extreme memory pressure from the current * foreground application, but in practice this should not be a concern.) *

      @@ -232,7 +259,7 @@ import android.os.IBinder; * *

      One of the most common uses of a Service is as a secondary component * running alongside other parts of an application, in the same process as - * the rest of the components. All components of an .apk run in the same + * the rest of the components. All components of an .apk run in the same * process unless explicitly stated otherwise, so this is a typical situation. * *

      When used in this way, by assuming the @@ -283,11 +310,12 @@ import android.os.IBinder; * messages back as well: * * {@sample development/samples/ApiDemos/src/com/example/android/apis/app/MessengerServiceActivities.java - * bind} + * bind} */ public abstract class Service extends ContextWrapper { /** - * Called by the system when the service is first created. Do not call this method directly. + * Called by the system when the service is first created. Do not call this + * method directly. */ public void onCreate() { } @@ -335,10 +363,12 @@ public abstract class Service extends ContextWrapper { } /** - * Called by the system to notify a Service that it is no longer used and is being removed. The + * Called by the system to notify a Service that it is no longer used and is + * being removed. The * service should clean up any resources it holds (threads, registered - * receivers, etc) at this point. Upon return, there will be no more calls - * in to this Service object and it is effectively dead. Do not call this method directly. + * receivers, etc) at this point. Upon return, there will be no more calls + * in to this Service object and it is effectively dead. Do not call this method + * directly. */ public void onDestroy() { } From 66b3c4687dbe4e88fb89fc162ac0db7d9a3c14a9 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Wed, 28 Sep 2022 16:42:52 -0400 Subject: [PATCH 10/20] resolve merge conflict --- .../code/java/frameworks/android/Intent.qll | 74 +++++-------------- .../semmle/code/java/security/DeepLink.qll | 50 ------------- .../android/intent/AndroidManifest.xml | 25 +++++++ .../intent/TestStartActivityToGetIntent.java | 24 ------ .../intent/TestStartComponentToIntent.java | 57 -------------- 5 files changed, 43 insertions(+), 187 deletions(-) delete mode 100644 java/ql/lib/semmle/code/java/security/DeepLink.qll delete mode 100644 java/ql/test/library-tests/frameworks/android/intent/TestStartComponentToIntent.java diff --git a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll index 0faa83742a2..5d02360d830 100644 --- a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll +++ b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll @@ -3,8 +3,6 @@ private import semmle.code.java.dataflow.DataFlow private import semmle.code.java.dataflow.ExternalFlow private import semmle.code.java.dataflow.FlowSteps -// ! Remember to add 'private' annotation as needed to new classes/predicates below. -// ! and clean-up comments, etc. in below in general before marking as ready-for-review. /** * The class `android.content.Intent`. */ @@ -66,21 +64,13 @@ class AndroidReceiveIntentMethod extends Method { } } -// ! not sure if I like the name of the below class, but -// ! trying to be consistent with `AndroidReceiveIntentMethod` -// ! and `AndroidGetIntentMethod`... /** - * A method of type Service that receives an Intent. - * Namely, `Service.onStart`, `onStartCommand`, `onBind`, - * `onRebind`, `onUnbind`, or `onTaskRemoved` + * The method `Service.onStart`, `onStartCommand`, + * `onBind`, `onRebind`, `onUnbind`, or `onTaskRemoved`. */ class AndroidServiceIntentMethod extends Method { AndroidServiceIntentMethod() { - ( - this.getName().matches("onStart%") or - this.getName().matches("on%ind") or - this.hasName("onTaskRemoved") - ) and + this.getName().matches(["onStart%", "on%ind", "onTaskRemoved"]) and this.getDeclaringType() instanceof TypeService } } @@ -127,20 +117,13 @@ class SendBroadcastMethod extends Method { } } -// ! remove reference from below QLDoc? /** * The method `Context.startService`, `startForegroundService`, * `bindIsolatedService`, `bindService`, or `bindServiceAsUser`. - * - * From https://developer.android.com/reference/android/app/Service: - * "Services can be started with Context.startService() and Context.bindService()." */ class StartServiceMethod extends Method { StartServiceMethod() { - ( - this.getName().matches("start%Service") or - this.getName().matches("bind%Service%") - ) and + this.getName().matches(["start%Service", "bind%Service%"]) and this.getDeclaringType() instanceof TypeContext } } @@ -254,25 +237,6 @@ class GrantWriteUriPermissionFlag extends GrantUriPermissionFlag { GrantWriteUriPermissionFlag() { this.hasName("FLAG_GRANT_WRITE_URI_PERMISSION") } } -// ! OLD VERSION - need to delete - keeping for now for reference -// /** -// * A value-preserving step from the Intent argument of a `startActivity` call to -// * a `getIntent` call in the Activity the Intent pointed to in its constructor. -// */ -// private class StartActivityIntentStep extends AdditionalValueStep { -// override predicate step(DataFlow::Node n1, DataFlow::Node n2) { -// exists(MethodAccess startActivity, MethodAccess getIntent, ClassInstanceExpr newIntent | -// startActivity.getMethod().overrides*(any(ContextStartActivityMethod m)) and -// getIntent.getMethod().overrides*(any(AndroidGetIntentMethod m)) and -// newIntent.getConstructedType() instanceof TypeIntent and -// DataFlow::localExprFlow(newIntent, startActivity.getArgument(0)) and -// newIntent.getArgument(1).getType().(ParameterizedType).getATypeArgument() = -// getIntent.getReceiverType() and -// n1.asExpr() = startActivity.getArgument(0) and -// n2.asExpr() = getIntent -// ) -// } -// } /* * // ! TODO: create a parent class for the below three steps? * // ! e.g. something like the below? @@ -283,9 +247,13 @@ class GrantWriteUriPermissionFlag extends GrantUriPermissionFlag { * // class StartComponentIntentStep extends AdditionalValueStep { } */ -// The `android.Content.Intent` class has two constructors with an argument of type -// `Class`. One has the argument at position 1 and the other at position 3. -// https://developer.android.com/reference/android/content/Intent#public-constructors +/** + * Gets the `Class` argument of an `android.content.Intent`constructor. + * + * The `android.content.Intent` class has two constructors with an argument of type + * `Class`. One has the argument at position 1 and the other at position 3. + * https://developer.android.com/reference/android/content/Intent#public-constructors + */ private Argument getClassArgOfIntentConstructor(ClassInstanceExpr classInstanceExpr) { classInstanceExpr.getConstructedType() instanceof TypeIntent and if classInstanceExpr.getNumArgument() = 2 @@ -298,9 +266,13 @@ private Argument getClassArgOfIntentConstructor(ClassInstanceExpr classInstanceE * a `getIntent` call in the Activity the Intent pointed to in its constructor. */ private class StartActivityIntentStep extends AdditionalValueStep { - // The `startActivityFromChild` and `startActivityFromFragment` methods have - // an argument of type `Intent` at position 1, but the rest of the methods of - // type `StartActivityMethod` have an argument of type `Intent` at position 0. + /** + * Gets the `Intent` argument of an Android `StartActivityMethod`. + * + * The `startActivityFromChild` and `startActivityFromFragment` methods have + * an argument of type `Intent` at position 1, but the rest of the methods of + * type `StartActivityMethod` have an argument of type `Intent` at position 0. + */ private Argument getIntentArgOfStartActMethod(MethodAccess methodAccess) { methodAccess.getMethod().overrides*(any(StartActivityMethod m)) and if @@ -310,15 +282,6 @@ private class StartActivityIntentStep extends AdditionalValueStep { else result = methodAccess.getArgument(0) } - // // The `android.Content.Intent` class has two constructors with an argument of type - // // `Class`. One has the argument at position 1 and the other at position 3. - // // https://developer.android.com/reference/android/content/Intent#public-constructors - // private Argument getClassArgOfIntentConstructor(ClassInstanceExpr classInstanceExpr) { - // classInstanceExpr.getConstructedType() instanceof TypeIntent and - // if classInstanceExpr.getNumArgument() = 2 - // then result = classInstanceExpr.getArgument(1) - // else result = classInstanceExpr.getArgument(3) - // } override predicate step(DataFlow::Node n1, DataFlow::Node n2) { exists(MethodAccess startActivity, MethodAccess getIntent, ClassInstanceExpr newIntent | startActivity.getMethod().overrides*(any(StartActivityMethod m)) and @@ -353,7 +316,6 @@ private class SendBroadcastReceiverIntentStep extends AdditionalValueStep { } } -// ! potentially reword QLDoc /** * A value-preserving step from the Intent argument of a `startService` call to * the `Intent` parameter in an `AndroidServiceIntentMethod` of the Service the diff --git a/java/ql/lib/semmle/code/java/security/DeepLink.qll b/java/ql/lib/semmle/code/java/security/DeepLink.qll deleted file mode 100644 index c00810a27fc..00000000000 --- a/java/ql/lib/semmle/code/java/security/DeepLink.qll +++ /dev/null @@ -1,50 +0,0 @@ -/** Provides classes and predicates to reason about deep links in Android. */ - -import java -private import semmle.code.java.frameworks.android.Intent -private import semmle.code.java.frameworks.android.Android -private import semmle.code.java.dataflow.DataFlow -private import semmle.code.java.dataflow.TaintTracking -private import semmle.code.java.dataflow.FlowSteps -private import semmle.code.xml.AndroidManifest - -// ! Experimentation file -// /** -// * A value-preserving step from the Intent argument of a method call that starts a component to -// * a `getIntent` call or `Intent` parameter in the component that the Intent pointed to in its constructor. -// */ -// // ! experimental - make a DeepLink step that combine Activity, Service, Receiver, etc. -// private class DeepLinkIntentStep extends AdditionalValueStep { -// // DeepLinkIntentStep() { -// // this instanceof StartActivityIntentStep or -// // this instanceof SendBroadcastReceiverIntentStep or -// // this instanceof StartServiceIntentStep -// // } -// override predicate step(DataFlow::Node n1, DataFlow::Node n2) { -// // ! simplify below -// ( -// exists(StartServiceIntentStep startServiceIntentStep | startServiceIntentStep.step(n1, n2)) -// or -// exists(SendBroadcastReceiverIntentStep sendBroadcastIntentStep | -// sendBroadcastIntentStep.step(n1, n2) -// ) -// or -// exists(StartActivityIntentStep startActivityIntentStep | startActivityIntentStep.step(n1, n2)) -// ) and -// exists(AndroidComponent andComp | -// andComp.getAndroidComponentXmlElement().(AndroidActivityXmlElement).hasDeepLink() and -// n1.asExpr().getFile() = andComp.getFile() // ! see if better way to do this -// ) -// } -// } -// ! experimental modeling of `parseUri` -/** - * The method `Intent.parseUri` - */ -class AndroidParseUriMethod extends Method { - AndroidParseUriMethod() { - // ! Note: getIntent for older versions before deprecation to parseUri - (this.hasName("parseUri") or this.hasName("getIntent")) and - this.getDeclaringType() instanceof TypeIntent - } -} diff --git a/java/ql/test/library-tests/frameworks/android/intent/AndroidManifest.xml b/java/ql/test/library-tests/frameworks/android/intent/AndroidManifest.xml index 0be6a0ae8f8..0b7cc21200b 100644 --- a/java/ql/test/library-tests/frameworks/android/intent/AndroidManifest.xml +++ b/java/ql/test/library-tests/frameworks/android/intent/AndroidManifest.xml @@ -18,5 +18,30 @@ android:exported="false"> + + + + + + + + + + + + + + + diff --git a/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java b/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java index 115b5673eab..e9e07d88a96 100644 --- a/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java +++ b/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java @@ -2,30 +2,6 @@ import android.app.Activity; import android.content.Context; import android.content.Intent; -// ! Original - saving for reference -// public class TestStartActivityToGetIntent { - -// static Object source() { -// return null; -// } - -// static void sink(Object sink) { -// } - -// public void test(Context ctx) { -// Intent intent = new Intent(null, SomeActivity.class); -// intent.putExtra("data", (String) source()); -// ctx.startActivity(intent); -// } - -// static class SomeActivity extends Activity { - -// public void test() { -// sink(getIntent().getStringExtra("data")); // $ hasValueFlow -// } -// } -// } - public class TestStartActivityToGetIntent { static Object source(String kind) { diff --git a/java/ql/test/library-tests/frameworks/android/intent/TestStartComponentToIntent.java b/java/ql/test/library-tests/frameworks/android/intent/TestStartComponentToIntent.java deleted file mode 100644 index ea5eee19d5b..00000000000 --- a/java/ql/test/library-tests/frameworks/android/intent/TestStartComponentToIntent.java +++ /dev/null @@ -1,57 +0,0 @@ -import android.app.Activity; -//import android.app.Service; -//import android.content.BroadcastReceiver; -import android.content.Context; -import android.content.Intent; - -public class TestStartComponentToIntent { - - static Object source() { - return null; - } - - static void sink(Object sink) { - } - - public void testActivity(Context ctx) { - Intent intent = new Intent(null, SomeActivity.class); - intent.putExtra("data", (String) source()); - ctx.startActivity(intent); - } - - static class SomeActivity extends Activity { - - public void testActivity() { - sink(getIntent().getStringExtra("data")); // $ hasValueFlow - } - } - - // ! WIP - public void testService(Context ctx) { - Intent intent = new Intent(null, SomeService.class); - intent.putExtra("data", (String) source()); - ctx.startService(intent); - } - - public void testBroadcastReceiver(Context ctx) { - Intent intent = new Intent(null, SomeBroadcastReceiver.class); - intent.putExtra("data", (String) source()); - ctx.sendBroadcast(intent); - } - - static class SomeService extends Service { - - public void test() { - // ! WIP - sink(getIntent().getStringExtra("data")); // $ hasValueFlow - } - } - - static class SomeBroadcastReceiver extends BroadcastReceiver { - - public void test() { - // ! WIP - sink(getIntent().getStringExtra("data")); // $ hasValueFlow - } - } -} From da7f27a7f2a6939b81a4beec01d98b8ea6fbe18e Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Thu, 22 Sep 2022 16:28:51 -0400 Subject: [PATCH 11/20] add change note --- .../change-notes/2022-09-22-android-deeplink-flow-steps.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 java/ql/lib/change-notes/2022-09-22-android-deeplink-flow-steps.md diff --git a/java/ql/lib/change-notes/2022-09-22-android-deeplink-flow-steps.md b/java/ql/lib/change-notes/2022-09-22-android-deeplink-flow-steps.md new file mode 100644 index 00000000000..dd7c654faed --- /dev/null +++ b/java/ql/lib/change-notes/2022-09-22-android-deeplink-flow-steps.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Added data flow steps for tainted Android intents that are sent to services and receivers. From 012cfebd7ac53de49f33b00442dc7479d5e7418e Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Wed, 28 Sep 2022 16:45:45 -0400 Subject: [PATCH 12/20] resolve merge conflict --- .../code/java/frameworks/android/Intent.qll | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll index 5d02360d830..a87899fde3b 100644 --- a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll +++ b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll @@ -86,6 +86,18 @@ class AndroidServiceIntentMethod extends Method { } } +/** + * The method `Context.startActivity` or `startActivities`. + * + * DEPRECATED: Use `StartActivityMethod` instead. + */ +deprecated class ContextStartActivityMethod extends Method { + ContextStartActivityMethod() { + (this.hasName("startActivity") or this.hasName("startActivities")) and + this.getDeclaringType() instanceof TypeContext + } +} + /** * The method `Context.startActivity`, `Context.startActivities`, * `Activity.startActivity`,`Activity.startActivities`, @@ -237,16 +249,6 @@ class GrantWriteUriPermissionFlag extends GrantUriPermissionFlag { GrantWriteUriPermissionFlag() { this.hasName("FLAG_GRANT_WRITE_URI_PERMISSION") } } -/* - * // ! TODO: create a parent class for the below three steps? - * // ! e.g. something like the below? - * // ! could put `getClassArgOfIntentConstructor` in parent class? - * - * // ! Also look into whether it's possible to reduce any code duplication - * // ! across the three steps in general. - * // class StartComponentIntentStep extends AdditionalValueStep { } - */ - /** * Gets the `Class` argument of an `android.content.Intent`constructor. * From c7e7e24cf8014980f81c4c1e91fb19d8b8e7b222 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Fri, 23 Sep 2022 17:09:37 -0400 Subject: [PATCH 13/20] clean up files --- .../2022-09-22-android-deeplink-flow-steps.md | 1 + .../intent/TestStartActivityToGetIntent.java | 3 +- .../TestStartBroadcastReceiverToIntent.java | 3 +- .../intent/TestStartServiceToIntent.java | 3 +- .../android/app/Service.java | 132 +++++++----------- 5 files changed, 55 insertions(+), 87 deletions(-) diff --git a/java/ql/lib/change-notes/2022-09-22-android-deeplink-flow-steps.md b/java/ql/lib/change-notes/2022-09-22-android-deeplink-flow-steps.md index dd7c654faed..69b38b97c26 100644 --- a/java/ql/lib/change-notes/2022-09-22-android-deeplink-flow-steps.md +++ b/java/ql/lib/change-notes/2022-09-22-android-deeplink-flow-steps.md @@ -2,3 +2,4 @@ category: minorAnalysis --- * Added data flow steps for tainted Android intents that are sent to services and receivers. +* Updated data flow step for tainted Android intents that are sent to activities. diff --git a/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java b/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java index e9e07d88a96..e2cc1c78e85 100644 --- a/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java +++ b/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java @@ -8,8 +8,7 @@ public class TestStartActivityToGetIntent { return null; } - static void sink(Object sink) { - } + static void sink(Object sink) {} public void test(Context ctx, Activity act) { { diff --git a/java/ql/test/library-tests/frameworks/android/intent/TestStartBroadcastReceiverToIntent.java b/java/ql/test/library-tests/frameworks/android/intent/TestStartBroadcastReceiverToIntent.java index 4cbc4f6e563..bddbf52ab99 100644 --- a/java/ql/test/library-tests/frameworks/android/intent/TestStartBroadcastReceiverToIntent.java +++ b/java/ql/test/library-tests/frameworks/android/intent/TestStartBroadcastReceiverToIntent.java @@ -8,8 +8,7 @@ public class TestStartBroadcastReceiverToIntent { return null; } - static void sink(Object sink) { - } + static void sink(Object sink) {} public void test(Context ctx) { { diff --git a/java/ql/test/library-tests/frameworks/android/intent/TestStartServiceToIntent.java b/java/ql/test/library-tests/frameworks/android/intent/TestStartServiceToIntent.java index edfa3d18864..233657faf60 100644 --- a/java/ql/test/library-tests/frameworks/android/intent/TestStartServiceToIntent.java +++ b/java/ql/test/library-tests/frameworks/android/intent/TestStartServiceToIntent.java @@ -9,8 +9,7 @@ public class TestStartServiceToIntent { return null; } - static void sink(Object sink) { - } + static void sink(Object sink) {} public void test(Context ctx) { { diff --git a/java/ql/test/stubs/google-android-9.0.0/android/app/Service.java b/java/ql/test/stubs/google-android-9.0.0/android/app/Service.java index d47dadcce4f..8250b8917c2 100644 --- a/java/ql/test/stubs/google-android-9.0.0/android/app/Service.java +++ b/java/ql/test/stubs/google-android-9.0.0/android/app/Service.java @@ -20,13 +20,12 @@ import android.content.ContextWrapper; import android.os.IBinder; /** - * A Service is an application component representing either an application's - * desire + * A Service is an application component representing either an application's desire * to perform a longer-running operation while not interacting with the user - * or to supply functionality for other applications to use. Each service + * or to supply functionality for other applications to use. Each service * class must have a corresponding * {@link android.R.styleable#AndroidManifestService <service>} - * declaration in its package's AndroidManifest.xml. Services + * declaration in its package's AndroidManifest.xml. Services * can be started with * {@link android.content.Context#startService Context.startService()} and * {@link android.content.Context#bindService Context.bindService()}. @@ -35,10 +34,9 @@ import android.os.IBinder; * thread of their hosting process. This means that, if your service is going * to do any CPU intensive (such as MP3 playback) or blocking (such as * networking) operations, it should spawn its own thread in which to do that - * work. More information on this can be found in - * Processes and - * Threads. The {@link IntentService} class is available + * work. More information on this can be found in + * Processes and + * Threads. The {@link IntentService} class is available * as a standard implementation of Service that has its own thread where it * schedules its work to be done.

      * @@ -49,18 +47,13 @@ import android.os.IBinder; *
    • Permissions *
    • Process Lifecycle *
    • Local Service Sample - *
    • Remote Messenger Service - * Sample + *
    • Remote Messenger Service Sample * * *
      *

      Developer Guides

      - *

      - * For a detailed discussion about how to create services, read the - * Services developer - * guide. - *

      + *

      For a detailed discussion about how to create services, read the + * Services developer guide.

      *
      * * @@ -70,10 +63,10 @@ import android.os.IBinder; * it is not:

      * *
        - *
      • A Service is not a separate process. The Service object itself + *
      • A Service is not a separate process. The Service object itself * does not imply it is running in its own process; unless otherwise specified, * it runs in the same process as the application it is part of. - *
      • A Service is not a thread. It is not a means itself to do work off + *
      • A Service is not a thread. It is not a means itself to do work off * of the main thread (to avoid Application Not Responding errors). *
      * @@ -82,12 +75,12 @@ import android.os.IBinder; *
        *
      • A facility for the application to tell the system about * something it wants to be doing in the background (even when the user is not - * directly interacting with the application). This corresponds to calls to + * directly interacting with the application). This corresponds to calls to * {@link android.content.Context#startService Context.startService()}, which * ask the system to schedule work for the service, to be run until the service * or someone else explicitly stop it. *
      • A facility for an application to expose some of its functionality to - * other applications. This corresponds to calls to + * other applications. This corresponds to calls to * {@link android.content.Context#bindService Context.bindService()}, which * allows a long-standing connection to be made to the service in order to * interact with it. @@ -112,14 +105,11 @@ import android.os.IBinder; * calls {@link android.content.Context#startService Context.startService()} then the system will * retrieve the service (creating it and calling its {@link #onCreate} method * if needed) and then call its {@link #onStartCommand} method with the - * arguments supplied by the client. The service will at this point continue - * running until {@link android.content.Context#stopService - * Context.stopService()} or - * {@link #stopSelf()} is called. Note that multiple calls to - * Context.startService() do not nest (though they do result in multiple - * corresponding - * calls to onStartCommand()), so no matter how many times it is started a - * service + * arguments supplied by the client. The service will at this point continue + * running until {@link android.content.Context#stopService Context.stopService()} or + * {@link #stopSelf()} is called. Note that multiple calls to + * Context.startService() do not nest (though they do result in multiple corresponding + * calls to onStartCommand()), so no matter how many times it is started a service * will be stopped once Context.stopService() or stopSelf() is called; however, * services can use their {@link #stopSelf(int)} method to ensure the service is * not stopped until started intents have been processed. @@ -129,29 +119,28 @@ import android.os.IBinder; * onStartCommand(): {@link #START_STICKY} is used for services that are * explicitly started and stopped as needed, while {@link #START_NOT_STICKY} * or {@link #START_REDELIVER_INTENT} are used for services that should only - * remain running while processing any commands sent to them. See the linked + * remain running while processing any commands sent to them. See the linked * documentation for more detail on the semantics. * *

        Clients can also use {@link android.content.Context#bindService Context.bindService()} to * obtain a persistent connection to a service. This likewise creates the * service if it is not already running (calling {@link #onCreate} while - * doing so), but does not call onStartCommand(). The client will receive the + * doing so), but does not call onStartCommand(). The client will receive the * {@link android.os.IBinder} object that the service returns from its * {@link #onBind} method, allowing the client to then make calls back - * to the service. The service will remain running as long as the connection + * to the service. The service will remain running as long as the connection * is established (whether or not the client retains a reference on the - * service's IBinder). Usually the IBinder returned is for a complex - * interface that has been - * written + * service's IBinder). Usually the IBinder returned is for a complex + * interface that has been written * in aidl. * *

        A service can be both started and have connections bound to it. In such * a case, the system will keep the service running as long as either it is * started or there are one or more connections to it with the * {@link android.content.Context#BIND_AUTO_CREATE Context.BIND_AUTO_CREATE} - * flag. Once neither + * flag. Once neither * of these situations hold, the service's {@link #onDestroy} method is called - * and the service is effectively terminated. All cleanup (stopping threads, + * and the service is effectively terminated. All cleanup (stopping threads, * unregistering receivers) should be complete upon returning from onDestroy(). * * @@ -159,28 +148,24 @@ import android.os.IBinder; * *

        Global access to a service can be enforced when it is declared in its * manifest's {@link android.R.styleable#AndroidManifestService <service>} - * tag. By doing so, other applications will need to declare a corresponding - * {@link android.R.styleable#AndroidManifestUsesPermission - * <uses-permission>} + * tag. By doing so, other applications will need to declare a corresponding + * {@link android.R.styleable#AndroidManifestUsesPermission <uses-permission>} * element in their own manifest to be able to start, stop, or bind to * the service. * - *

        - * As of {@link android.os.Build.VERSION_CODES#GINGERBREAD}, when using + *

        As of {@link android.os.Build.VERSION_CODES#GINGERBREAD}, when using * {@link Context#startService(Intent) Context.startService(Intent)}, you can * also set {@link Intent#FLAG_GRANT_READ_URI_PERMISSION - * Intent.FLAG_GRANT_READ_URI_PERMISSION} and/or - * {@link Intent#FLAG_GRANT_WRITE_URI_PERMISSION - * Intent.FLAG_GRANT_WRITE_URI_PERMISSION} on the Intent. This will grant the - * Service temporary access to the specific URIs in the Intent. Access will + * Intent.FLAG_GRANT_READ_URI_PERMISSION} and/or {@link Intent#FLAG_GRANT_WRITE_URI_PERMISSION + * Intent.FLAG_GRANT_WRITE_URI_PERMISSION} on the Intent. This will grant the + * Service temporary access to the specific URIs in the Intent. Access will * remain until the Service has called {@link #stopSelf(int)} for that start * command or a later one, or until the Service has been completely stopped. * This works for granting access to the other apps that have not requested * the permission protecting the Service, or even when the Service is not * exported at all. * - *

        - * In addition, a service can protect individual IPC calls into it with + *

        In addition, a service can protect individual IPC calls into it with * permissions, by calling the * {@link #checkCallingPermission} * method before executing the implementation of that call. @@ -198,44 +183,32 @@ import android.os.IBinder; * following possibilities: * *

          - *
        • - *

          - * If the service is currently executing code in its + *

        • If the service is currently executing code in its * {@link #onCreate onCreate()}, {@link #onStartCommand onStartCommand()}, * or {@link #onDestroy onDestroy()} methods, then the hosting process will * be a foreground process to ensure this code can execute without * being killed. - *

        • - *

          - * If the service has been started, then its hosting process is considered + *

        • If the service has been started, then its hosting process is considered * to be less important than any processes that are currently visible to the - * user on-screen, but more important than any process not visible. Because + * user on-screen, but more important than any process not visible. Because * only a few processes are generally visible to the user, this means that - * the service should not be killed except in low memory conditions. However, - * since - * the user is not directly aware of a background service, in that state it - * is + * the service should not be killed except in low memory conditions. However, since + * the user is not directly aware of a background service, in that state it is * considered a valid candidate to kill, and you should be prepared for this to - * happen. In particular, long-running services will be increasingly likely to + * happen. In particular, long-running services will be increasingly likely to * kill and are guaranteed to be killed (and restarted if appropriate) if they * remain started long enough. - *

        • - *

          - * If there are clients bound to the service, then the service's hosting - * process is never less important than the most important client. That is, + *

        • If there are clients bound to the service, then the service's hosting + * process is never less important than the most important client. That is, * if one of its clients is visible to the user, then the service itself is - * considered to be visible. The way a client's importance impacts the service's + * considered to be visible. The way a client's importance impacts the service's * importance can be adjusted through {@link Context#BIND_ABOVE_CLIENT}, - * {@link Context#BIND_ALLOW_OOM_MANAGEMENT}, - * {@link Context#BIND_WAIVE_PRIORITY}, - * {@link Context#BIND_IMPORTANT}, and - * {@link Context#BIND_ADJUST_WITH_ACTIVITY}. - *

        • - *

          - * A started service can use the {@link #startForeground(int, Notification)} + * {@link Context#BIND_ALLOW_OOM_MANAGEMENT}, {@link Context#BIND_WAIVE_PRIORITY}, + * {@link Context#BIND_IMPORTANT}, and {@link Context#BIND_ADJUST_WITH_ACTIVITY}. + *

        • A started service can use the {@link #startForeground(int, Notification)} * API to put the service in a foreground state, where the system considers * it to be something the user is actively aware of and thus not a candidate - * for killing when low on memory. (It is still theoretically possible for + * for killing when low on memory. (It is still theoretically possible for * the service to be killed under extreme memory pressure from the current * foreground application, but in practice this should not be a concern.) *

        @@ -259,7 +232,7 @@ import android.os.IBinder; * *

        One of the most common uses of a Service is as a secondary component * running alongside other parts of an application, in the same process as - * the rest of the components. All components of an .apk run in the same + * the rest of the components. All components of an .apk run in the same * process unless explicitly stated otherwise, so this is a typical situation. * *

        When used in this way, by assuming the @@ -310,12 +283,11 @@ import android.os.IBinder; * messages back as well: * * {@sample development/samples/ApiDemos/src/com/example/android/apis/app/MessengerServiceActivities.java - * bind} + * bind} */ public abstract class Service extends ContextWrapper { /** - * Called by the system when the service is first created. Do not call this - * method directly. + * Called by the system when the service is first created. Do not call this method directly. */ public void onCreate() { } @@ -363,12 +335,10 @@ public abstract class Service extends ContextWrapper { } /** - * Called by the system to notify a Service that it is no longer used and is - * being removed. The + * Called by the system to notify a Service that it is no longer used and is being removed. The * service should clean up any resources it holds (threads, registered - * receivers, etc) at this point. Upon return, there will be no more calls - * in to this Service object and it is effectively dead. Do not call this method - * directly. + * receivers, etc) at this point. Upon return, there will be no more calls + * in to this Service object and it is effectively dead. Do not call this method directly. */ public void onDestroy() { } From 9a7cf7db65e0fe500285545fb57ede629b2cf150 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Sun, 25 Sep 2022 22:25:47 -0400 Subject: [PATCH 14/20] simplify hasName usage --- java/ql/lib/semmle/code/java/frameworks/android/Intent.qll | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll index a87899fde3b..e6724257ec3 100644 --- a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll +++ b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll @@ -277,9 +277,7 @@ private class StartActivityIntentStep extends AdditionalValueStep { */ private Argument getIntentArgOfStartActMethod(MethodAccess methodAccess) { methodAccess.getMethod().overrides*(any(StartActivityMethod m)) and - if - methodAccess.getMethod().hasName("startActivityFromChild") or - methodAccess.getMethod().hasName("startActivityFromFragment") + if methodAccess.getMethod().hasName(["startActivityFromChild", "startActivityFromFragment"]) then result = methodAccess.getArgument(1) else result = methodAccess.getArgument(0) } From 834927c50b8402a18fca2f49c3ad1de054ccccd5 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Sun, 25 Sep 2022 22:32:47 -0400 Subject: [PATCH 15/20] update tests --- .../intent/TestStartActivityToGetIntent.java | 88 ++++++++++++++++--- .../TestStartBroadcastReceiverToIntent.java | 58 +++++++++++- .../intent/TestStartServiceToIntent.java | 48 ++++++++-- 3 files changed, 172 insertions(+), 22 deletions(-) diff --git a/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java b/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java index e2cc1c78e85..977380d1361 100644 --- a/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java +++ b/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java @@ -11,30 +11,96 @@ public class TestStartActivityToGetIntent { static void sink(Object sink) {} public void test(Context ctx, Activity act) { + + // test all methods that start an activity { - Intent intentCtx = new Intent(null, SomeActivity.class); - Intent intentAct = new Intent(null, SomeActivity.class); - intentCtx.putExtra("data", (String) source("context")); - intentAct.putExtra("data", (String) source("activity")); - ctx.startActivity(intentCtx); - act.startActivity(intentAct); + Intent intent = new Intent(null, SomeActivity.class); + intent.putExtra("data", (String) source("ctx-start")); + ctx.startActivity(intent); + } + { + // Intent intent1 = new Intent(null, SomeActivity2.class); + // intent1.putExtra("data", (String) source("ctx-starts")); + // Intent intent2 = new Intent(null, SomeActivity3.class); + // intent2.putExtra("data", (String) source("ctx-starts")); + // Intent[] intents = {intent1, intent2}; + // ctx.startActivities(intents); + } + { + Intent intent = new Intent(null, SomeActivity.class); + intent.putExtra("data", (String) source("act-start")); + act.startActivity(intent); + } + { + // Intent[] intent = {new Intent(null, SomeActivity.class)}; + // intent[0].putExtra("data", (String) source("act-starts")); + // act.startActivities(intent); + } + { + Intent intent = new Intent(null, SomeActivity.class); + intent.putExtra("data", (String) source("start-for-result")); + act.startActivityForResult(intent, 0); + } + { + Intent intent = new Intent(null, SomeActivity.class); + intent.putExtra("data", (String) source("start-if-needed")); + act.startActivityIfNeeded(intent, 0); + } + { + Intent intent = new Intent(null, SomeActivity.class); + intent.putExtra("data", (String) source("start-matching")); + act.startNextMatchingActivity(intent); + } + { + Intent intent = new Intent(null, SomeActivity.class); + intent.putExtra("data", (String) source("start-from-child")); + act.startActivityFromChild(null, intent, 0); + } + { + Intent intent = new Intent(null, SomeActivity.class); + intent.putExtra("data", (String) source("start-from-frag")); + act.startActivityFromFragment(null, intent, 0); } + // test 4-arg Intent constructor { - Intent intentCtx = new Intent(null, SafeActivity.class); - Intent intentAct = new Intent(null, SafeActivity.class); - ctx.startActivity(intentCtx); - act.startActivity(intentAct); + Intent intent = new Intent(null, null, null, SomeActivity.class); + intent.putExtra("data", (String) source("4-arg")); + ctx.startActivity(intent); + } + + // safe test + { + Intent intent = new Intent(null, SafeActivity.class); + intent.putExtra("data", "safe"); + ctx.startActivity(intent); } } static class SomeActivity extends Activity { public void test() { - sink(getIntent().getStringExtra("data")); // $ hasValueFlow=context hasValueFlow=activity + sink(getIntent().getStringExtra("data")); // $ hasValueFlow=ctx-start hasValueFlow=act-start hasValueFlow=start-for-result hasValueFlow=start-if-needed hasValueFlow=start-matching hasValueFlow=start-from-child hasValueFlow=start-from-frag hasValueFlow=4-arg } + } + // static class SomeActivity2 extends Activity { + + // public void test() { + // sink(getIntent().getStringExtra("data")); // $ hasValueFlow=ctx-starts + // } + + // } + + // static class SomeActivity3 extends Activity { + + // public void test() { + // sink(getIntent().getStringExtra("data")); // $ hasValueFlow=ctx-starts + // } + + // } + static class SafeActivity extends Activity { public void test() { diff --git a/java/ql/test/library-tests/frameworks/android/intent/TestStartBroadcastReceiverToIntent.java b/java/ql/test/library-tests/frameworks/android/intent/TestStartBroadcastReceiverToIntent.java index bddbf52ab99..f317b3ed2e8 100644 --- a/java/ql/test/library-tests/frameworks/android/intent/TestStartBroadcastReceiverToIntent.java +++ b/java/ql/test/library-tests/frameworks/android/intent/TestStartBroadcastReceiverToIntent.java @@ -4,30 +4,82 @@ import android.content.Intent; public class TestStartBroadcastReceiverToIntent { - static Object source() { + static Object source(String kind) { return null; } static void sink(Object sink) {} public void test(Context ctx) { + + // test all methods that send a broadcast { Intent intent = new Intent(null, SomeBroadcastReceiver.class); - intent.putExtra("data", (String) source()); + intent.putExtra("data", (String) source("send")); + ctx.sendBroadcast(intent); + } + { + Intent intent = new Intent(null, SomeBroadcastReceiver.class); + intent.putExtra("data", (String) source("send-as-user")); + ctx.sendBroadcastAsUser(intent, null); + } + { + Intent intent = new Intent(null, SomeBroadcastReceiver.class); + intent.putExtra("data", (String) source("send-with-perm")); + ctx.sendBroadcastWithMultiplePermissions(intent, null); + } + { + Intent intent = new Intent(null, SomeBroadcastReceiver.class); + intent.putExtra("data", (String) source("send-ordered")); + ctx.sendOrderedBroadcast(intent, null); + } + { + Intent intent = new Intent(null, SomeBroadcastReceiver.class); + intent.putExtra("data", (String) source("send-ordered-as-user")); + ctx.sendOrderedBroadcastAsUser(intent, null, null, null, null, 0, null, null); + } + { + Intent intent = new Intent(null, SomeBroadcastReceiver.class); + intent.putExtra("data", (String) source("send-sticky")); + ctx.sendStickyBroadcast(intent); + } + { + Intent intent = new Intent(null, SomeBroadcastReceiver.class); + intent.putExtra("data", (String) source("send-sticky-as-user")); + ctx.sendStickyBroadcastAsUser(intent, null); + } + { + Intent intent = new Intent(null, SomeBroadcastReceiver.class); + intent.putExtra("data", (String) source("send-sticky-ordered")); + ctx.sendStickyOrderedBroadcast(intent, null, null, 0, null, null); + } + { + Intent intent = new Intent(null, SomeBroadcastReceiver.class); + intent.putExtra("data", (String) source("send-sticky-ordered-as-user")); + ctx.sendStickyOrderedBroadcastAsUser(intent, null, null, null, 0, null, null); + } + + // test 4-arg Intent constructor + { + Intent intent = new Intent(null, null, null, SomeBroadcastReceiver.class); + intent.putExtra("data", (String) source("4-arg")); ctx.sendBroadcast(intent); } + // safe test { Intent intent = new Intent(null, SafeBroadcastReceiver.class); + intent.putExtra("data", "safe"); ctx.sendBroadcast(intent); } } static class SomeBroadcastReceiver extends BroadcastReceiver { + // test method that receives an Intent as a parameter @Override public void onReceive(Context context, Intent intent) { - sink(intent.getStringExtra("data")); // $ hasValueFlow + sink(intent.getStringExtra("data")); // $ hasValueFlow=send hasValueFlow=send-as-user hasValueFlow=send-with-perm hasValueFlow=send-ordered hasValueFlow=send-ordered-as-user hasValueFlow=send-sticky hasValueFlow=send-sticky-as-user hasValueFlow=send-sticky-ordered hasValueFlow=send-sticky-ordered-as-user hasValueFlow=4-arg } } diff --git a/java/ql/test/library-tests/frameworks/android/intent/TestStartServiceToIntent.java b/java/ql/test/library-tests/frameworks/android/intent/TestStartServiceToIntent.java index 233657faf60..98404eed1b7 100644 --- a/java/ql/test/library-tests/frameworks/android/intent/TestStartServiceToIntent.java +++ b/java/ql/test/library-tests/frameworks/android/intent/TestStartServiceToIntent.java @@ -5,58 +5,90 @@ import android.content.Intent; public class TestStartServiceToIntent { - static Object source() { + static Object source(String kind) { return null; } static void sink(Object sink) {} public void test(Context ctx) { + + // test all methods that start a service { Intent intent = new Intent(null, SomeService.class); - intent.putExtra("data", (String) source()); + intent.putExtra("data", (String) source("bind")); + ctx.bindService(intent, null, 0); + } + { + Intent intent = new Intent(null, SomeService.class); + intent.putExtra("data", (String) source("bind-as-user")); + ctx.bindServiceAsUser(intent, null, 0, null); + } + { + Intent intent = new Intent(null, SomeService.class); + intent.putExtra("data", (String) source("bind-isolated")); + ctx.bindIsolatedService(intent, 0, null, null, null); + } + { + Intent intent = new Intent(null, SomeService.class); + intent.putExtra("data", (String) source("start")); + ctx.startService(intent); + } + { + Intent intent = new Intent(null, SomeService.class); + intent.putExtra("data", (String) source("start-foreground")); + ctx.startForegroundService(intent); + } + + // test 4-arg Intent constructor + { + Intent intent = new Intent(null, null, null, SomeService.class); + intent.putExtra("data", (String) source("4-arg")); ctx.startService(intent); } + // safe test { Intent intent = new Intent(null, SafeService.class); + intent.putExtra("data", "safe"); ctx.startService(intent); } } static class SomeService extends Service { + // test methods that receive an Intent as a parameter @Override public void onStart(Intent intent, int startId) { - sink(intent.getStringExtra("data")); // $ hasValueFlow + sink(intent.getStringExtra("data")); // $ hasValueFlow=bind hasValueFlow=bind-as-user hasValueFlow=bind-isolated hasValueFlow=start hasValueFlow=start-foreground hasValueFlow=4-arg } @Override public int onStartCommand(Intent intent, int flags, int startId) { - sink(intent.getStringExtra("data")); // $ hasValueFlow + sink(intent.getStringExtra("data")); // $ hasValueFlow=bind hasValueFlow=bind-as-user hasValueFlow=bind-isolated hasValueFlow=start hasValueFlow=start-foreground hasValueFlow=4-arg return -1; } @Override public IBinder onBind(Intent intent) { - sink(intent.getStringExtra("data")); // $ hasValueFlow + sink(intent.getStringExtra("data")); // $ hasValueFlow=bind hasValueFlow=bind-as-user hasValueFlow=bind-isolated hasValueFlow=start hasValueFlow=start-foreground hasValueFlow=4-arg return null; } @Override public boolean onUnbind(Intent intent) { - sink(intent.getStringExtra("data")); // $ hasValueFlow + sink(intent.getStringExtra("data")); // $ hasValueFlow=bind hasValueFlow=bind-as-user hasValueFlow=bind-isolated hasValueFlow=start hasValueFlow=start-foreground hasValueFlow=4-arg return false; } @Override public void onRebind(Intent intent) { - sink(intent.getStringExtra("data")); // $ hasValueFlow + sink(intent.getStringExtra("data")); // $ hasValueFlow=bind hasValueFlow=bind-as-user hasValueFlow=bind-isolated hasValueFlow=start hasValueFlow=start-foreground hasValueFlow=4-arg } @Override public void onTaskRemoved(Intent intent) { - sink(intent.getStringExtra("data")); // $ hasValueFlow + sink(intent.getStringExtra("data")); // $ hasValueFlow=bind hasValueFlow=bind-as-user hasValueFlow=bind-isolated hasValueFlow=start hasValueFlow=start-foreground hasValueFlow=4-arg } } From 00b0a6bf38d31d14b7a7724a277517e5d39f85f2 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Mon, 26 Sep 2022 10:05:27 -0400 Subject: [PATCH 16/20] update act tests --- .../intent/TestStartActivityToGetIntent.java | 31 +------------------ 1 file changed, 1 insertion(+), 30 deletions(-) diff --git a/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java b/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java index 977380d1361..8ebfb72e701 100644 --- a/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java +++ b/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java @@ -12,30 +12,17 @@ public class TestStartActivityToGetIntent { public void test(Context ctx, Activity act) { - // test all methods that start an activity + // test methods that start an activity { Intent intent = new Intent(null, SomeActivity.class); intent.putExtra("data", (String) source("ctx-start")); ctx.startActivity(intent); } - { - // Intent intent1 = new Intent(null, SomeActivity2.class); - // intent1.putExtra("data", (String) source("ctx-starts")); - // Intent intent2 = new Intent(null, SomeActivity3.class); - // intent2.putExtra("data", (String) source("ctx-starts")); - // Intent[] intents = {intent1, intent2}; - // ctx.startActivities(intents); - } { Intent intent = new Intent(null, SomeActivity.class); intent.putExtra("data", (String) source("act-start")); act.startActivity(intent); } - { - // Intent[] intent = {new Intent(null, SomeActivity.class)}; - // intent[0].putExtra("data", (String) source("act-starts")); - // act.startActivities(intent); - } { Intent intent = new Intent(null, SomeActivity.class); intent.putExtra("data", (String) source("start-for-result")); @@ -85,22 +72,6 @@ public class TestStartActivityToGetIntent { } - // static class SomeActivity2 extends Activity { - - // public void test() { - // sink(getIntent().getStringExtra("data")); // $ hasValueFlow=ctx-starts - // } - - // } - - // static class SomeActivity3 extends Activity { - - // public void test() { - // sink(getIntent().getStringExtra("data")); // $ hasValueFlow=ctx-starts - // } - - // } - static class SafeActivity extends Activity { public void test() { From 1857a5d311a91b11526374fcc4493529ab97f39f Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 28 Sep 2022 13:02:53 +0200 Subject: [PATCH 17/20] Refactor Intent flow steps --- .../code/java/frameworks/android/Intent.qll | 117 +++++++++--------- 1 file changed, 60 insertions(+), 57 deletions(-) diff --git a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll index e6724257ec3..91bd616318a 100644 --- a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll +++ b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll @@ -249,88 +249,91 @@ class GrantWriteUriPermissionFlag extends GrantUriPermissionFlag { GrantWriteUriPermissionFlag() { this.hasName("FLAG_GRANT_WRITE_URI_PERMISSION") } } -/** - * Gets the `Class` argument of an `android.content.Intent`constructor. - * - * The `android.content.Intent` class has two constructors with an argument of type - * `Class`. One has the argument at position 1 and the other at position 3. - * https://developer.android.com/reference/android/content/Intent#public-constructors - */ -private Argument getClassArgOfIntentConstructor(ClassInstanceExpr classInstanceExpr) { - classInstanceExpr.getConstructedType() instanceof TypeIntent and - if classInstanceExpr.getNumArgument() = 2 - then result = classInstanceExpr.getArgument(1) - else result = classInstanceExpr.getArgument(3) +/** The instantiation of an `android.content.Intent` instance. */ +private class NewIntent extends ClassInstanceExpr { + NewIntent() { this.getConstructedType() instanceof TypeIntent } + + /** Gets the `Class` argument of this call. */ + Argument getClassArg() { + result.getType() instanceof TypeClass and + result = this.getAnArgument() + } } -/** - * A value-preserving step from the Intent argument of a `startActivity` call to - * a `getIntent` call in the Activity the Intent pointed to in its constructor. - */ -private class StartActivityIntentStep extends AdditionalValueStep { - /** - * Gets the `Intent` argument of an Android `StartActivityMethod`. - * - * The `startActivityFromChild` and `startActivityFromFragment` methods have - * an argument of type `Intent` at position 1, but the rest of the methods of - * type `StartActivityMethod` have an argument of type `Intent` at position 0. - */ - private Argument getIntentArgOfStartActMethod(MethodAccess methodAccess) { - methodAccess.getMethod().overrides*(any(StartActivityMethod m)) and - if methodAccess.getMethod().hasName(["startActivityFromChild", "startActivityFromFragment"]) - then result = methodAccess.getArgument(1) - else result = methodAccess.getArgument(0) +/** A call to a method that starts an Android component */ +private class StartComponentMethodAccess extends MethodAccess { + StartComponentMethodAccess() { + this.getMethod().overrides*(any(StartActivityMethod m)) or + this.getMethod().overrides*(any(StartServiceMethod m)) or + this.getMethod().overrides*(any(SendBroadcastMethod m)) } - override predicate step(DataFlow::Node n1, DataFlow::Node n2) { - exists(MethodAccess startActivity, MethodAccess getIntent, ClassInstanceExpr newIntent | - startActivity.getMethod().overrides*(any(StartActivityMethod m)) and - getIntent.getMethod().overrides*(any(AndroidGetIntentMethod m)) and - newIntent.getConstructedType() instanceof TypeIntent and - DataFlow::localExprFlow(newIntent, this.getIntentArgOfStartActMethod(startActivity)) and - getClassArgOfIntentConstructor(newIntent).getType().(ParameterizedType).getATypeArgument() = - getIntent.getReceiverType() and - n1.asExpr() = this.getIntentArgOfStartActMethod(startActivity) and - n2.asExpr() = getIntent + /** Gets the intent argument of this call. */ + Argument getIntentArg() { + result.getType() instanceof TypeIntent and + result = this.getAnArgument() + } + + /** Holds if this targets a component of type `targetType`. */ + predicate targetsComponentType(RefType targetType) { + exists(NewIntent newIntent | + DataFlow::localExprFlow(newIntent, this.getIntentArg()) and + newIntent.getClassArg().getType().(ParameterizedType).getATypeArgument() = targetType ) } } /** - * A value-preserving step from the Intent argument of a `sendBroadcast` call to - * the `Intent` parameter in the `onReceive` method of the BroadcastReceiver the - * Intent pointed to in its constructor. + * Holds if there is a step from the intent argument `n1` of a `startActivity` call + * to a `getIntent` call `n2` in the activity `n1` targets. + */ +private predicate startActivityIntentStep(DataFlow::Node n1, DataFlow::Node n2) { + exists(StartComponentMethodAccess startActivity, MethodAccess getIntent | + startActivity.getMethod().overrides*(any(StartActivityMethod m)) and + getIntent.getMethod().overrides*(any(AndroidGetIntentMethod m)) and + startActivity.targetsComponentType(getIntent.getReceiverType()) and + n1.asExpr() = startActivity.getIntentArg() and + n2.asExpr() = getIntent + ) +} + +/** + * A value-preserving step from the intent argument of a `startActivity` call to + * a `getIntent` call in the activity the intent targeted in its constructor. + */ +private class StartActivityIntentStep extends AdditionalValueStep { + override predicate step(DataFlow::Node n1, DataFlow::Node n2) { startActivityIntentStep(n1, n2) } +} + +/** + * A value-preserving step from the intent argument of a `sendBroadcast` call to + * the intent parameter in the `onReceive` method of the receiver the + * intent targeted in its constructor. */ private class SendBroadcastReceiverIntentStep extends AdditionalValueStep { override predicate step(DataFlow::Node n1, DataFlow::Node n2) { - exists(MethodAccess sendBroadcast, Method onReceive, ClassInstanceExpr newIntent | + exists(StartComponentMethodAccess sendBroadcast, Method onReceive | sendBroadcast.getMethod().overrides*(any(SendBroadcastMethod m)) and onReceive.overrides*(any(AndroidReceiveIntentMethod m)) and - newIntent.getConstructedType() instanceof TypeIntent and - DataFlow::localExprFlow(newIntent, sendBroadcast.getArgument(0)) and - getClassArgOfIntentConstructor(newIntent).getType().(ParameterizedType).getATypeArgument() = - onReceive.getDeclaringType() and - n1.asExpr() = sendBroadcast.getArgument(0) and + sendBroadcast.targetsComponentType(onReceive.getDeclaringType()) and + n1.asExpr() = sendBroadcast.getIntentArg() and n2.asParameter() = onReceive.getParameter(1) ) } } /** - * A value-preserving step from the Intent argument of a `startService` call to - * the `Intent` parameter in an `AndroidServiceIntentMethod` of the Service the - * Intent pointed to in its constructor. + * A value-preserving step from the intent argument of a `startService` call to + * the intent parameter in an `AndroidServiceIntentMethod` of the service the + * intent targeted in its constructor. */ private class StartServiceIntentStep extends AdditionalValueStep { override predicate step(DataFlow::Node n1, DataFlow::Node n2) { - exists(MethodAccess startService, Method serviceIntent, ClassInstanceExpr newIntent | + exists(StartComponentMethodAccess startService, Method serviceIntent | startService.getMethod().overrides*(any(StartServiceMethod m)) and serviceIntent.overrides*(any(AndroidServiceIntentMethod m)) and - newIntent.getConstructedType() instanceof TypeIntent and - DataFlow::localExprFlow(newIntent, startService.getArgument(0)) and - getClassArgOfIntentConstructor(newIntent).getType().(ParameterizedType).getATypeArgument() = - serviceIntent.getDeclaringType() and - n1.asExpr() = startService.getArgument(0) and + startService.targetsComponentType(serviceIntent.getDeclaringType()) and + n1.asExpr() = startService.getIntentArg() and n2.asParameter() = serviceIntent.getParameter(0) ) } From 960e9db2fba4fc9c8fbe33130967ac1d65bab5ee Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Wed, 28 Sep 2022 16:22:49 -0400 Subject: [PATCH 18/20] add missing expectation to tests --- .../intent/TestStartActivityToGetIntent.java | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java b/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java index 8ebfb72e701..35884a23a58 100644 --- a/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java +++ b/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java @@ -12,17 +12,29 @@ public class TestStartActivityToGetIntent { public void test(Context ctx, Activity act) { - // test methods that start an activity + // test all methods that start an activity { Intent intent = new Intent(null, SomeActivity.class); intent.putExtra("data", (String) source("ctx-start")); ctx.startActivity(intent); } + { + Intent intent = new Intent(null, SomeActivity.class); + intent.putExtra("data", (String) source("ctx-start-acts")); + Intent[] intents = new Intent[] {intent}; + ctx.startActivities(intents); + } { Intent intent = new Intent(null, SomeActivity.class); intent.putExtra("data", (String) source("act-start")); act.startActivity(intent); } + { + Intent intent = new Intent(null, SomeActivity.class); + intent.putExtra("data", (String) source("act-start-acts")); + Intent[] intents = new Intent[] {intent}; + act.startActivities(intents); + } { Intent intent = new Intent(null, SomeActivity.class); intent.putExtra("data", (String) source("start-for-result")); @@ -67,7 +79,7 @@ public class TestStartActivityToGetIntent { static class SomeActivity extends Activity { public void test() { - sink(getIntent().getStringExtra("data")); // $ hasValueFlow=ctx-start hasValueFlow=act-start hasValueFlow=start-for-result hasValueFlow=start-if-needed hasValueFlow=start-matching hasValueFlow=start-from-child hasValueFlow=start-from-frag hasValueFlow=4-arg + sink(getIntent().getStringExtra("data")); // $ hasValueFlow=ctx-start hasValueFlow=act-start hasValueFlow=start-for-result hasValueFlow=start-if-needed hasValueFlow=start-matching hasValueFlow=start-from-child hasValueFlow=start-from-frag hasValueFlow=4-arg MISSING: hasValueFlow=ctx-start-acts hasValueFlow=act-start-acts } } From 91db1be399e89cc334a95a78f4ec9d340b151658 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Wed, 28 Sep 2022 17:52:40 -0400 Subject: [PATCH 19/20] update Intent file --- .../code/java/frameworks/android/Intent.qll | 80 ++++++------------- 1 file changed, 23 insertions(+), 57 deletions(-) diff --git a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll index 91bd616318a..d6ff265be50 100644 --- a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll +++ b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll @@ -3,9 +3,7 @@ private import semmle.code.java.dataflow.DataFlow private import semmle.code.java.dataflow.ExternalFlow private import semmle.code.java.dataflow.FlowSteps -/** - * The class `android.content.Intent`. - */ +/** The class `android.content.Intent`. */ class TypeIntent extends Class { TypeIntent() { this.hasQualifiedName("android.content", "Intent") } } @@ -15,23 +13,17 @@ class TypeComponentName extends Class { TypeComponentName() { this.hasQualifiedName("android.content", "ComponentName") } } -/** - * The class `android.app.Activity`. - */ +/** The class `android.app.Activity`. */ class TypeActivity extends Class { TypeActivity() { this.hasQualifiedName("android.app", "Activity") } } -/** - * The class `android.app.Service`. - */ +/** The class `android.app.Service`. */ class TypeService extends Class { TypeService() { this.hasQualifiedName("android.app", "Service") } } -/** - * The class `android.content.Context`. - */ +/** The class `android.content.Context`. */ class TypeContext extends RefType { // Not inlining this makes it more likely to be used as a sentinel, // which is useful when running Android queries on non-Android projects. @@ -39,42 +31,25 @@ class TypeContext extends RefType { TypeContext() { this.hasQualifiedName("android.content", "Context") } } -/** - * The class `android.content.BroadcastReceiver`. - */ +/** The class `android.content.BroadcastReceiver`. */ class TypeBroadcastReceiver extends Class { TypeBroadcastReceiver() { this.hasQualifiedName("android.content", "BroadcastReceiver") } } -/** - * The method `Activity.getIntent` - */ +/** The method `Activity.getIntent` */ class AndroidGetIntentMethod extends Method { AndroidGetIntentMethod() { this.hasName("getIntent") and this.getDeclaringType() instanceof TypeActivity } } -/** - * The method `BroadcastReceiver.onReceive`. - */ +/** The method `BroadcastReceiver.onReceive`. */ class AndroidReceiveIntentMethod extends Method { AndroidReceiveIntentMethod() { this.hasName("onReceive") and this.getDeclaringType() instanceof TypeBroadcastReceiver } } -/** - * The method `Service.onStart`, `onStartCommand`, - * `onBind`, `onRebind`, `onUnbind`, or `onTaskRemoved`. - */ -class AndroidServiceIntentMethod extends Method { - AndroidServiceIntentMethod() { - this.getName().matches(["onStart%", "on%ind", "onTaskRemoved"]) and - this.getDeclaringType() instanceof TypeService - } -} - /** * The method `Service.onStart`, `onStartCommand`, * `onBind`, `onRebind`, `onUnbind`, or `onTaskRemoved`. @@ -135,22 +110,21 @@ class SendBroadcastMethod extends Method { */ class StartServiceMethod extends Method { StartServiceMethod() { - this.getName().matches(["start%Service", "bind%Service%"]) and + this.hasName([ + "startService", "startForegroundService", "bindIsolatedService", "bindService", + "bindServiceAsUser" + ]) and this.getDeclaringType() instanceof TypeContext } } -/** - * Specifies that if an `Intent` is tainted, then so are its synthetic fields. - */ +/** Specifies that if an `Intent` is tainted, then so are its synthetic fields. */ private class IntentFieldsInheritTaint extends DataFlow::SyntheticFieldContent, TaintInheritingContent { IntentFieldsInheritTaint() { this.getField().matches("android.content.Intent.%") } } -/** - * The method `Intent.getParcelableExtra`. - */ +/** The method `Intent.getParcelableExtra`. */ class IntentGetParcelableExtraMethod extends Method { IntentGetParcelableExtraMethod() { this.hasName("getParcelableExtra") and @@ -212,9 +186,7 @@ private class BundleExtrasSyntheticField extends SyntheticField { override RefType getType() { result instanceof AndroidBundle } } -/** - * Holds if extras may be implicitly read from the Intent `node`. - */ +/** Holds if extras may be implicitly read from the Intent `node`. */ predicate allowIntentExtrasImplicitRead(DataFlow::Node node, DataFlow::Content c) { node.getType() instanceof TypeIntent and ( @@ -283,26 +255,20 @@ private class StartComponentMethodAccess extends MethodAccess { } } -/** - * Holds if there is a step from the intent argument `n1` of a `startActivity` call - * to a `getIntent` call `n2` in the activity `n1` targets. - */ -private predicate startActivityIntentStep(DataFlow::Node n1, DataFlow::Node n2) { - exists(StartComponentMethodAccess startActivity, MethodAccess getIntent | - startActivity.getMethod().overrides*(any(StartActivityMethod m)) and - getIntent.getMethod().overrides*(any(AndroidGetIntentMethod m)) and - startActivity.targetsComponentType(getIntent.getReceiverType()) and - n1.asExpr() = startActivity.getIntentArg() and - n2.asExpr() = getIntent - ) -} - /** * A value-preserving step from the intent argument of a `startActivity` call to * a `getIntent` call in the activity the intent targeted in its constructor. */ private class StartActivityIntentStep extends AdditionalValueStep { - override predicate step(DataFlow::Node n1, DataFlow::Node n2) { startActivityIntentStep(n1, n2) } + override predicate step(DataFlow::Node n1, DataFlow::Node n2) { + exists(StartComponentMethodAccess startActivity, MethodAccess getIntent | + startActivity.getMethod().overrides*(any(StartActivityMethod m)) and + getIntent.getMethod().overrides*(any(AndroidGetIntentMethod m)) and + startActivity.targetsComponentType(getIntent.getReceiverType()) and + n1.asExpr() = startActivity.getIntentArg() and + n2.asExpr() = getIntent + ) + } } /** From 25cb3236a238ed488b863e5b0116c220f3fda1b7 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Thu, 29 Sep 2022 15:53:00 -0400 Subject: [PATCH 20/20] apply review suggestions --- .../change-notes/2022-09-22-android-deeplink-flow-steps.md | 2 +- ...2022-09-22-android-deprecate-contextstartactivitymethod.md | 4 ++++ java/ql/lib/semmle/code/java/frameworks/android/Intent.qll | 4 ++-- 3 files changed, 7 insertions(+), 3 deletions(-) create mode 100644 java/ql/lib/change-notes/2022-09-22-android-deprecate-contextstartactivitymethod.md diff --git a/java/ql/lib/change-notes/2022-09-22-android-deeplink-flow-steps.md b/java/ql/lib/change-notes/2022-09-22-android-deeplink-flow-steps.md index 69b38b97c26..1ed229b1e05 100644 --- a/java/ql/lib/change-notes/2022-09-22-android-deeplink-flow-steps.md +++ b/java/ql/lib/change-notes/2022-09-22-android-deeplink-flow-steps.md @@ -2,4 +2,4 @@ category: minorAnalysis --- * Added data flow steps for tainted Android intents that are sent to services and receivers. -* Updated data flow step for tainted Android intents that are sent to activities. +* Improved the data flow step for tainted Android intents that are sent to activities so that more cases are covered. diff --git a/java/ql/lib/change-notes/2022-09-22-android-deprecate-contextstartactivitymethod.md b/java/ql/lib/change-notes/2022-09-22-android-deprecate-contextstartactivitymethod.md new file mode 100644 index 00000000000..3500322afad --- /dev/null +++ b/java/ql/lib/change-notes/2022-09-22-android-deprecate-contextstartactivitymethod.md @@ -0,0 +1,4 @@ +--- +category: deprecated +--- +* Deprecated `ContextStartActivityMethod`. Use `StartActivityMethod` instead. diff --git a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll index d6ff265be50..be38b83e5a7 100644 --- a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll +++ b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll @@ -221,7 +221,7 @@ class GrantWriteUriPermissionFlag extends GrantUriPermissionFlag { GrantWriteUriPermissionFlag() { this.hasName("FLAG_GRANT_WRITE_URI_PERMISSION") } } -/** The instantiation of an `android.content.Intent` instance. */ +/** An instantiation of `android.content.Intent`. */ private class NewIntent extends ClassInstanceExpr { NewIntent() { this.getConstructedType() instanceof TypeIntent } @@ -232,7 +232,7 @@ private class NewIntent extends ClassInstanceExpr { } } -/** A call to a method that starts an Android component */ +/** A call to a method that starts an Android component. */ private class StartComponentMethodAccess extends MethodAccess { StartComponentMethodAccess() { this.getMethod().overrides*(any(StartActivityMethod m)) or