diff --git a/java/ql/src/experimental/Security/CWE/CWE-200/AndroidFileIntentSink.qll b/java/ql/src/experimental/Security/CWE/CWE-200/AndroidFileIntentSink.qll index 95b11e34849..c31a2fb235e 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-200/AndroidFileIntentSink.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-200/AndroidFileIntentSink.qll @@ -19,24 +19,30 @@ class AsyncTask extends RefType { AsyncTask() { this.hasQualifiedName("android.os", "AsyncTask") } } +/** The method that executes `AsyncTask` of Android. */ +abstract class ExecuteAsyncTaskMethod extends Method { + /** Returns index of the parameter that is tainted. */ + abstract int getParamIndex(); +} + /** The `execute` method of Android `AsyncTask`. */ -class AsyncTaskExecuteMethod extends Method { +class AsyncTaskExecuteMethod extends ExecuteAsyncTaskMethod { AsyncTaskExecuteMethod() { this.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof AsyncTask and this.getName() = "execute" } - int getParamIndex() { result = 0 } + override int getParamIndex() { result = 0 } } /** The `executeOnExecutor` method of Android `AsyncTask`. */ -class AsyncTaskExecuteOnExecutorMethod extends Method { +class AsyncTaskExecuteOnExecutorMethod extends ExecuteAsyncTaskMethod { AsyncTaskExecuteOnExecutorMethod() { this.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof AsyncTask and this.getName() = "executeOnExecutor" } - int getParamIndex() { result = 1 } + override int getParamIndex() { result = 1 } } /** The `doInBackground` method of Android `AsyncTask`. */ diff --git a/java/ql/src/experimental/Security/CWE/CWE-200/AndroidFileIntentSource.qll b/java/ql/src/experimental/Security/CWE/CWE-200/AndroidFileIntentSource.qll index 6df48f530a5..46b6e1059bb 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-200/AndroidFileIntentSource.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-200/AndroidFileIntentSource.qll @@ -15,7 +15,7 @@ class StartActivityForResultMethod extends Method { /** Android class instance of `GET_CONTENT` intent. */ class GetContentIntent extends ClassInstanceExpr { GetContentIntent() { - this.getConstructedType().getASupertype*() instanceof TypeIntent and + this.getConstructedType() instanceof TypeIntent and this.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "android.intent.action.GET_CONTENT" or diff --git a/java/ql/src/experimental/Security/CWE/CWE-200/SensitiveAndroidFileLeak.ql b/java/ql/src/experimental/Security/CWE/CWE-200/SensitiveAndroidFileLeak.ql index c4bdde170ba..a216892826d 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-200/SensitiveAndroidFileLeak.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-200/SensitiveAndroidFileLeak.ql @@ -3,7 +3,7 @@ * @description Getting file intent from user input without path validation could leak arbitrary * Android configuration file and sensitive user data. * @kind path-problem - * @id java/sensitive_android_file_leak + * @id java/sensitive-android-file-leak * @tags security * external/cwe/cwe-200 */ @@ -14,6 +14,19 @@ import AndroidFileIntentSource import DataFlow2::PathGraph import semmle.code.java.dataflow.TaintTracking2 +private class StartsWithSanitizer extends DataFlow2::BarrierGuard { + StartsWithSanitizer() { this.(MethodAccess).getMethod().hasName("startsWith") } + + override predicate checks(Expr e, boolean branch) { + e = + [ + this.(MethodAccess).getQualifier(), + this.(MethodAccess).getQualifier().(MethodAccess).getQualifier() + ] and + branch = false + } +} + class AndroidFileLeakConfig extends TaintTracking2::Configuration { AndroidFileLeakConfig() { this = "AndroidFileLeakConfig" } @@ -38,37 +51,23 @@ class AndroidFileLeakConfig extends TaintTracking2::Configuration { exists(MethodAccess aema, AsyncTaskRunInBackgroundMethod arm | // fileAsyncTask.execute(params) will invoke doInBackground(params) of FileAsyncTask aema.getQualifier().getType() = arm.getDeclaringType() and - ( - aema.getMethod() instanceof AsyncTaskExecuteMethod and - prev.asExpr() = aema.getArgument(0) - or - aema.getMethod() instanceof AsyncTaskExecuteOnExecutorMethod and - prev.asExpr() = aema.getArgument(1) - ) and - succ.asExpr() = arm.getParameter(0).getAnAccess() + aema.getMethod() instanceof ExecuteAsyncTaskMethod and + prev.asExpr() = aema.getArgument(aema.getMethod().(ExecuteAsyncTaskMethod).getParamIndex()) and + succ.asParameter() = arm.getParameter(0) ) or exists(MethodAccess csma, ServiceOnStartCommandMethod ssm, ClassInstanceExpr ce | csma.getMethod() instanceof ContextStartServiceMethod and ce.getConstructedType() instanceof TypeIntent and // Intent intent = new Intent(context, FileUploader.class); - ce.getArgument(1).getType().(ParameterizedType).getTypeArgument(0) = ssm.getDeclaringType() and + ce.getArgument(1).(TypeLiteral).getReferencedType() = ssm.getDeclaringType() and DataFlow2::localExprFlow(ce, csma.getArgument(0)) and // context.startService(intent); prev.asExpr() = csma.getArgument(0) and - succ.asExpr() = ssm.getParameter(0).getAnAccess() // public int onStartCommand(Intent intent, int flags, int startId) {...} in FileUploader + succ.asParameter() = ssm.getParameter(0) // public int onStartCommand(Intent intent, int flags, int startId) {...} in FileUploader ) } - override predicate isSanitizer(DataFlow2::Node node) { - exists( - MethodAccess startsWith // "startsWith" path check - | - startsWith.getMethod().hasName("startsWith") and - ( - DataFlow2::localExprFlow(node.asExpr(), startsWith.getQualifier()) or - DataFlow2::localExprFlow(node.asExpr(), - startsWith.getQualifier().(MethodAccess).getQualifier()) - ) - ) + override predicate isSanitizerGuard(DataFlow2::BarrierGuard guard) { + guard instanceof StartsWithSanitizer } } diff --git a/java/ql/test/experimental/query-tests/security/CWE-200/SensitiveAndroidFileLeak.expected b/java/ql/test/experimental/query-tests/security/CWE-200/SensitiveAndroidFileLeak.expected index db29fd64568..47f27356d87 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-200/SensitiveAndroidFileLeak.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-200/SensitiveAndroidFileLeak.expected @@ -1,26 +1,28 @@ edges +| FileService.java:20:31:20:43 | intent : Intent | FileService.java:21:28:21:33 | intent : Intent | +| FileService.java:20:31:20:43 | intent : Intent | FileService.java:25:42:25:50 | localPath : String | | FileService.java:21:28:21:33 | intent : Intent | FileService.java:21:28:21:64 | getStringExtra(...) : String | -| FileService.java:21:28:21:33 | intent : Intent | FileService.java:25:42:25:50 | localPath : String | | FileService.java:21:28:21:64 | getStringExtra(...) : String | FileService.java:25:42:25:50 | localPath : String | -| FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] | FileService.java:44:44:44:49 | params : Object[] | +| FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] | FileService.java:40:41:40:55 | params : Object[] | | FileService.java:25:13:25:51 | makeParamsToExecute(...) [[]] : String | FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] | | FileService.java:25:42:25:50 | localPath : String | FileService.java:25:13:25:51 | makeParamsToExecute(...) [[]] : String | +| FileService.java:40:41:40:55 | params : Object[] | FileService.java:44:33:44:52 | (...)... : Object | | FileService.java:44:33:44:52 | (...)... : Object | FileService.java:45:53:45:59 | ...[...] | -| FileService.java:44:44:44:49 | params : Object[] | FileService.java:44:33:44:52 | (...)... : Object | -| LeakFileActivity2.java:16:26:16:31 | intent : Intent | FileService.java:21:28:21:33 | intent : Intent | +| LeakFileActivity2.java:16:26:16:31 | intent : Intent | FileService.java:20:31:20:43 | intent : Intent | | LeakFileActivity.java:14:35:14:38 | data : Intent | LeakFileActivity.java:18:40:18:59 | contentIntent : Intent | | LeakFileActivity.java:18:40:18:59 | contentIntent : Intent | LeakFileActivity.java:19:31:19:43 | contentIntent : Intent | | LeakFileActivity.java:19:31:19:43 | contentIntent : Intent | LeakFileActivity.java:19:31:19:53 | getData(...) : Uri | | LeakFileActivity.java:19:31:19:53 | getData(...) : Uri | LeakFileActivity.java:21:58:21:72 | streamsToUpload : Uri | | LeakFileActivity.java:21:58:21:72 | streamsToUpload : Uri | LeakFileActivity.java:21:58:21:82 | getPath(...) | nodes +| FileService.java:20:31:20:43 | intent : Intent | semmle.label | intent : Intent | | FileService.java:21:28:21:33 | intent : Intent | semmle.label | intent : Intent | | FileService.java:21:28:21:64 | getStringExtra(...) : String | semmle.label | getStringExtra(...) : String | | FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] | semmle.label | makeParamsToExecute(...) : Object[] | | FileService.java:25:13:25:51 | makeParamsToExecute(...) [[]] : String | semmle.label | makeParamsToExecute(...) [[]] : String | | FileService.java:25:42:25:50 | localPath : String | semmle.label | localPath : String | +| FileService.java:40:41:40:55 | params : Object[] | semmle.label | params : Object[] | | FileService.java:44:33:44:52 | (...)... : Object | semmle.label | (...)... : Object | -| FileService.java:44:44:44:49 | params : Object[] | semmle.label | params : Object[] | | FileService.java:45:53:45:59 | ...[...] | semmle.label | ...[...] | | LeakFileActivity2.java:16:26:16:31 | intent : Intent | semmle.label | intent : Intent | | LeakFileActivity.java:14:35:14:38 | data : Intent | semmle.label | data : Intent |