From f88c8a64a1d3c49244bb8668dc16a703192842e6 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 6 Oct 2021 17:37:21 +0100 Subject: [PATCH] Copyedit --- .../CWE/CWE-200/AndroidFileIntentSink.qll | 8 +++---- .../CWE/CWE-200/AndroidFileIntentSource.qll | 22 +++++-------------- .../CWE-200/SensitiveAndroidFileLeak.qhelp | 12 +++++----- .../CWE/CWE-200/SensitiveAndroidFileLeak.ql | 17 +++++++------- 4 files changed, 24 insertions(+), 35 deletions(-) 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 c3f5eebccc4..597590dbaa1 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-200/AndroidFileIntentSink.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-200/AndroidFileIntentSink.qll @@ -19,7 +19,7 @@ class AsyncTask extends RefType { AsyncTask() { this.hasQualifiedName("android.os", "AsyncTask") } } -/** The `execute` or `executeOnExecutor` method of Android `AsyncTask`. */ +/** The `execute` or `executeOnExecutor` method of Android's `AsyncTask` class. */ class ExecuteAsyncTaskMethod extends Method { int paramIndex; @@ -35,7 +35,7 @@ class ExecuteAsyncTaskMethod extends Method { int getParamIndex() { result = paramIndex } } -/** The `doInBackground` method of Android `AsyncTask`. */ +/** The `doInBackground` method of Android's `AsyncTask` class. */ class AsyncTaskRunInBackgroundMethod extends Method { AsyncTaskRunInBackgroundMethod() { this.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof AsyncTask and @@ -43,7 +43,7 @@ class AsyncTaskRunInBackgroundMethod extends Method { } } -/** The service start method of Android context. */ +/** The service start method of Android's `Context` class. */ class ContextStartServiceMethod extends Method { ContextStartServiceMethod() { this.getName() = ["startService", "startForegroundService"] and @@ -51,7 +51,7 @@ class ContextStartServiceMethod extends Method { } } -/** The `onStartCommand` method of Android service. */ +/** The `onStartCommand` method of Android's `Service` class. */ class ServiceOnStartCommandMethod extends Method { ServiceOnStartCommandMethod() { this.hasName("onStartCommand") and 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 88e554e6a7c..22935997afe 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-200/AndroidFileIntentSource.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-200/AndroidFileIntentSource.qll @@ -5,7 +5,7 @@ import semmle.code.java.dataflow.FlowSources import semmle.code.java.dataflow.TaintTracking2 import semmle.code.java.frameworks.android.Android -/** The `startActivityForResult` method of Android `Activity`. */ +/** The `startActivityForResult` method of Android's `Activity` class. */ class StartActivityForResultMethod extends Method { StartActivityForResultMethod() { this.getDeclaringType().getASupertype*() instanceof AndroidActivity and @@ -13,7 +13,7 @@ class StartActivityForResultMethod extends Method { } } -/** Android class instance of `GET_CONTENT` intent. */ +/** An instance of `android.content.Intent` constructed passing `GET_CONTENT` to the constructor. */ class GetContentIntent extends ClassInstanceExpr { GetContentIntent() { this.getConstructedType() instanceof TypeIntent and @@ -28,7 +28,7 @@ class GetContentIntent extends ClassInstanceExpr { } } -/** Taint configuration for getting content intent. */ +/** Taint configuration that identifies `GET_CONTENT` `Intent` instances passed to `startActivityForResult`. */ class GetContentIntentConfig extends TaintTracking2::Configuration { GetContentIntentConfig() { this = "GetContentIntentConfig" } @@ -56,8 +56,8 @@ class GetContentIntentConfig extends TaintTracking2::Configuration { } } -/** Android `Intent` input to request file loading. */ -class AndroidFileIntentInput extends LocalUserInput { +/** A `GET_CONTENT` `Intent` instances that is passed to `startActivityForResult`. */ +class AndroidFileIntentInput extends DataFlow::Node { MethodAccess ma; AndroidFileIntentInput() { @@ -68,7 +68,7 @@ class AndroidFileIntentInput extends LocalUserInput { ) } - /** The request code identifying a specific intent, which is to be matched in `onActivityResult()`. */ + /** The request code passed to `startActivityForResult`, which is to be matched in `onActivityResult()`. */ int getRequestCode() { result = ma.getArgument(1).(CompileTimeConstantExpr).getIntValue() } } @@ -79,13 +79,3 @@ class OnActivityForResultMethod extends Method { this.getName() = "onActivityResult" } } - -/** Input of Android activity result from the same application or another application. */ -class AndroidActivityResultInput extends DataFlow::Node { - OnActivityForResultMethod m; - - AndroidActivityResultInput() { this.asExpr() = m.getParameter(2).getAnAccess() } - - /** The request code matching a specific intent request. */ - VarAccess getRequestCodeVar() { result = m.getParameter(0).getAnAccess() } -} diff --git a/java/ql/src/experimental/Security/CWE/CWE-200/SensitiveAndroidFileLeak.qhelp b/java/ql/src/experimental/Security/CWE/CWE-200/SensitiveAndroidFileLeak.qhelp index 7b6e60d7ca0..ca4a7e668ea 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-200/SensitiveAndroidFileLeak.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-200/SensitiveAndroidFileLeak.qhelp @@ -5,22 +5,22 @@

The Android API allows to start an activity in another mobile application and receive a result back. When starting an activity to retrieve a file from another application, missing input validation can -lead to leaking of sensitive configuration file or user data because the intent is from the application -itself that is allowed to access its protected data therefore bypassing the access control. +lead to leaking of sensitive configuration file or user data because the intent could refer to paths +which are accessible to the receiver application, but are intended to be application-private.

-When loading file data from an activity of another application, validate that the file path is not its own +When loading file data from an activity of another application, validate that the file path is not the receiver's protected directory, which is a subdirectory of the Android application directory /data/data/.

-The following examples show the bad situation and the good situation respectively. In bad situation, a -file is loaded without path validation. In good situation, a file is loaded with path validation. +The following examples show a bad situation and a good situation respectively. In the bad situation, a +file is loaded without path validation. In the good situation, a file is loaded with path validation.

@@ -33,6 +33,6 @@ Google:
  • CVE: CVE-2021-32695: File Sharing Flow Initiated by a Victim Leaks Sensitive Data to a Malicious App. -
    • + \ No newline at end of file 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 d84fe2453b6..e392446c188 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-200/SensitiveAndroidFileLeak.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-200/SensitiveAndroidFileLeak.ql @@ -1,6 +1,6 @@ /** * @name Leaking sensitive Android file - * @description Getting file intent from user input without path validation could leak arbitrary + * @description Using a path specified in an Android Intent without validation could leak arbitrary * Android configuration file and sensitive user data. * @kind path-problem * @id java/sensitive-android-file-leak @@ -35,18 +35,17 @@ class AndroidFileLeakConfig extends TaintTracking::Configuration { /** * Holds if `src` is a read of some Intent-typed variable guarded by a check like - * `requestCode == REQUEST_CODE__SELECT_CONTENT_FROM_APPS`, where `requestCode` is the first - * argument to `Activity.onActivityResult` and `REQUEST_CODE__SELECT_CONTENT_FROM_APPS` is - * any request code in a call to `startActivityForResult(intent, code)`. + * `requestCode == someCode`, where `requestCode` is the first + * argument to `Activity.onActivityResult` and `someCode` is + * any request code used in a call to `startActivityForResult(intent, someCode)`. */ override predicate isSource(DataFlow::Node src) { exists( - AndroidActivityResultInput ai, AndroidFileIntentInput fi, ConditionBlock cb, EQExpr ee, - CompileTimeConstantExpr cc, VarAccess intentVar + OnActivityForResultMethod oafr, ConditionBlock cb, CompileTimeConstantExpr cc, + VarAccess intentVar | - cb.getCondition() = ee and - ee.hasOperands(ai.getRequestCodeVar(), cc) and - cc.getIntValue() = fi.getRequestCode() and + cb.getCondition().(EQExpr).hasOperands(oafr.getParameter(0).getAnAccess(), cc) and + cc.getIntValue() = any(AndroidFileIntentInput fi).getRequestCode() and intentVar.getType() instanceof TypeIntent and cb.controls(intentVar.getBasicBlock(), true) and src.asExpr() = intentVar