From 6cf3898101b6414a7f675bf7d4f92dbc9db85b4b Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Wed, 7 Sep 2022 18:46:15 -0400 Subject: [PATCH] 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"