From 2c2aab6ffcd076db3deae2e91a846aa0bd823c09 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Mon, 19 Oct 2020 16:16:13 +0000 Subject: [PATCH 01/11] Sensitive broadcast --- .../CWE/CWE-927/SensitiveBroadcast.java | 20 +++ .../CWE/CWE-927/SensitiveBroadcast.qhelp | 25 ++++ .../CWE/CWE-927/SensitiveBroadcast.ql | 120 ++++++++++++++++++ .../CWE-927/SensitiveBroadcast.expected | 24 ++++ .../security/CWE-927/SensitiveBroadcast.java | 66 ++++++++++ .../security/CWE-927/SensitiveBroadcast.qlref | 1 + .../query-tests/security/CWE-927/options | 1 + .../android/content/Intent.java | 73 +++++++++++ 8 files changed, 330 insertions(+) create mode 100644 java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.java create mode 100644 java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.qhelp create mode 100644 java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql create mode 100644 java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.expected create mode 100644 java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.java create mode 100644 java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.qlref create mode 100644 java/ql/test/experimental/query-tests/security/CWE-927/options diff --git a/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.java b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.java new file mode 100644 index 00000000000..e9fd7b15dc1 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.java @@ -0,0 +1,20 @@ +public void sendBroadcast1(Context context, String token, String refreshToken) +{ + { + // BAD: broadcast sensitive information without permission + Intent intent = new Intent(); + intent.setAction("com.example.custom_action"); + intent.putExtra("token", token); + intent.putExtra("refreshToken", refreshToken); + context.sendBroadcast(intent); + } + + { + // GOOD: broadcast sensitive information with permission + Intent intent = new Intent(); + intent.setAction("com.example.custom_action"); + intent.putExtra("token", token); + intent.putExtra("refreshToken", refreshToken); + context.sendBroadcast(intent, "com.example.user_permission"); + } +} \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.qhelp b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.qhelp new file mode 100644 index 00000000000..b9acfd7b7c9 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.qhelp @@ -0,0 +1,25 @@ + + + + +

Broadcasted intents in an Android application are visible to all applications installed on the same mobile device, exposing all sensitive information they contain.

+

Broadcasts are vulnerable to passive eavesdropping or active denial of service attacks when an intent is broadcasted without specifying any receiver permission or receiver application.

+ + + +

Specify receiver permission or specify receiver application in broadcasted intents, or switch to LocalBroadcastManager or the latest LiveData library.

+ + + +

The following example shows two ways of broadcasting intents. In the 'BAD' case, no "receiver permission" is specified. In the 'GOOD' case, "receiver permission" is specified.

+ + + + +
  • +CWE-927: Use of Implicit Intent for Sensitive Communication +
    • +     +     \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql new file mode 100644 index 00000000000..9b024a9866e --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql @@ -0,0 +1,120 @@ +/** + * @name Use of Implicit Intent for Sensitive Communication + * @id java/sensitive-broadcast + * @description An Android application uses implicit intents to broadcast sensitive data to all applications without specifying any receiver permission. + * @kind path-problem + * @tags security + * external/cwe-927 + */ + +import java +import semmle.code.java.frameworks.android.Intent +import semmle.code.java.dataflow.TaintTracking +import DataFlow +import PathGraph + +/** + * Gets a regular expression for matching names of variables that indicate the value being held contains sensitive information. + */ +private string getSensitiveInfoRegex() { + result = "(?i).*challenge|pass(wd|word|code|phrase)(?!.*question).*" or + result = "(?i).*(token|email|phone|username|userid|ticket).*" +} + +/** + * Method call to pass information to the `Intent` object either directly through intent extra or indirectly through intent extra bundle. + */ +class PutExtraMethodAccess extends MethodAccess { + PutExtraMethodAccess() { + getMethod().getName().regexpMatch("put\\w*Extra(s*)") and + getMethod().getDeclaringType() instanceof TypeIntent and + not exists( + MethodAccess setPackageVa // Intent without specifying receiving package name of the 3rd party app + | + setPackageVa.getMethod().hasName(["setPackage", "setClass", "setClassName", "setComponent"]) and + setPackageVa.getQualifier().(VarAccess).getVariable().getAnAccess() = getQualifier() + ) + or + getMethod().getName().regexpMatch("put\\w+") and + getMethod().getDeclaringType().hasQualifiedName("android.os", "Bundle") + } +} + +/** Finds variables that hold sensitive information judging by their names. */ +class SensitiveInfoExpr extends Expr { + SensitiveInfoExpr() { + exists(Variable v | this = v.getAnAccess() | + v.getName().toLowerCase().regexpMatch(getSensitiveInfoRegex()) + ) + } +} + +/** + * The method access of `context.sendBroadcast`. + */ +class SendBroadcastMethodAccess extends MethodAccess { + SendBroadcastMethodAccess() { + this.getMethod().getDeclaringType() instanceof TypeContext and + this.getMethod().getName().matches("send%Broadcast%") + } +} + +/** + * Holds if a `sendBroadcast` call doesn't specify receiver permission. + */ +predicate isSensitiveBroadcastSink(DataFlow::Node sink) { + exists(SendBroadcastMethodAccess ma | + sink.asExpr() = ma.getAnArgument() and + ( + ma.getMethod().hasName("sendBroadcast") and + ( + ma.getNumArgument() = 1 or // sendBroadcast(Intent intent) + ma.getArgument(1) instanceof NullLiteral // sendBroadcast(Intent intent, String receiverPermission) + ) + or + ma.getMethod().hasName("sendBroadcastAsUser") and + ( + ma.getNumArgument() = 2 or // sendBroadcastAsUser(Intent intent, UserHandle user) + ma.getArgument(2) instanceof NullLiteral // sendBroadcastAsUser(Intent intent, UserHandle user, String receiverPermission) + ) + or + ma.getMethod().hasName("sendBroadcastWithMultiplePermissions") and + ma.getArgument(1) instanceof NullLiteral // sendBroadcastWithMultiplePermissions(Intent intent, String[] receiverPermissions) + or + //Method calls of `sendOrderedBroadcast` whose second argument is always `receiverPermission` + ma.getMethod().hasName("sendOrderedBroadcast") and + ma.getArgument(1) instanceof NullLiteral + or + //Method call of `sendOrderedBroadcastAsUser(Intent intent, UserHandle user, String receiverPermission, BroadcastReceiver resultReceiver, Handler scheduler, int initialCode, String initialData, Bundle initialExtras)` + ma.getMethod().hasName("sendOrderedBroadcastAsUser") and + ma.getArgument(2) instanceof NullLiteral + ) + ) +} + +/** + * Taint configuration tracking flow from variables containing sensitive information to broadcasted intents. + */ +class SensitiveBroadcastConfig extends DataFlow::Configuration { + SensitiveBroadcastConfig() { this = "Sensitive Broadcast Configuration" } + + override predicate isSource(DataFlow::Node source) { + source.asExpr() instanceof SensitiveInfoExpr + } + + override predicate isSink(DataFlow::Node sink) { isSensitiveBroadcastSink(sink) } + + /** + * Holds if there is an additional flow step from `PutExtraMethodAccess` to a broadcasted intent. + */ + override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(PutExtraMethodAccess pa | + node1.asExpr() = pa.getAnArgument() and node2.asExpr() = pa.getQualifier() + ) + } +} + +from SensitiveBroadcastConfig cfg, DataFlow::PathNode source, DataFlow::PathNode sink +where cfg.hasFlowPath(source, sink) +select sink.getNode(), source, sink, "Sending $@ to broadcast.", source.getNode(), + "sensitive information" diff --git a/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.expected b/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.expected new file mode 100644 index 00000000000..163a34e6a49 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.expected @@ -0,0 +1,24 @@ +edges +| SensitiveBroadcast.java:11:34:11:38 | token : String | SensitiveBroadcast.java:13:31:13:36 | intent | +| SensitiveBroadcast.java:12:41:12:52 | refreshToken : String | SensitiveBroadcast.java:13:31:13:36 | intent | +| SensitiveBroadcast.java:23:33:23:40 | username : String | SensitiveBroadcast.java:25:31:25:36 | intent | +| SensitiveBroadcast.java:24:32:24:39 | password : String | SensitiveBroadcast.java:25:31:25:36 | intent | +| SensitiveBroadcast.java:36:40:36:47 | username : String | SensitiveBroadcast.java:39:31:39:36 | intent | +| SensitiveBroadcast.java:37:39:37:46 | password : String | SensitiveBroadcast.java:39:31:39:36 | intent | +nodes +| SensitiveBroadcast.java:11:34:11:38 | token : String | semmle.label | token : String | +| SensitiveBroadcast.java:12:41:12:52 | refreshToken : String | semmle.label | refreshToken : String | +| SensitiveBroadcast.java:13:31:13:36 | intent | semmle.label | intent | +| SensitiveBroadcast.java:23:33:23:40 | username : String | semmle.label | username : String | +| SensitiveBroadcast.java:24:32:24:39 | password : String | semmle.label | password : String | +| SensitiveBroadcast.java:25:31:25:36 | intent | semmle.label | intent | +| SensitiveBroadcast.java:36:40:36:47 | username : String | semmle.label | username : String | +| SensitiveBroadcast.java:37:39:37:46 | password : String | semmle.label | password : String | +| SensitiveBroadcast.java:39:31:39:36 | intent | semmle.label | intent | +#select +| SensitiveBroadcast.java:13:31:13:36 | intent | SensitiveBroadcast.java:11:34:11:38 | token : String | SensitiveBroadcast.java:13:31:13:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:11:34:11:38 | token | sensitive information | +| SensitiveBroadcast.java:13:31:13:36 | intent | SensitiveBroadcast.java:12:41:12:52 | refreshToken : String | SensitiveBroadcast.java:13:31:13:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:12:41:12:52 | refreshToken | sensitive information | +| SensitiveBroadcast.java:25:31:25:36 | intent | SensitiveBroadcast.java:23:33:23:40 | username : String | SensitiveBroadcast.java:25:31:25:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:23:33:23:40 | username | sensitive information | +| SensitiveBroadcast.java:25:31:25:36 | intent | SensitiveBroadcast.java:24:32:24:39 | password : String | SensitiveBroadcast.java:25:31:25:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:24:32:24:39 | password | sensitive information | +| SensitiveBroadcast.java:39:31:39:36 | intent | SensitiveBroadcast.java:36:40:36:47 | username : String | SensitiveBroadcast.java:39:31:39:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:36:40:36:47 | username | sensitive information | +| SensitiveBroadcast.java:39:31:39:36 | intent | SensitiveBroadcast.java:37:39:37:46 | password : String | SensitiveBroadcast.java:39:31:39:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:37:39:37:46 | password | sensitive information | diff --git a/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.java b/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.java new file mode 100644 index 00000000000..c84e709a8d6 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.java @@ -0,0 +1,66 @@ +import android.content.Context; +import android.content.Intent; +import android.os.Bundle; + +class SensitiveBroadcast { + + //Tests broadcast of access token with intent extra. + public void sendBroadcast1(Context context, String token, String refreshToken) { + Intent intent = new Intent(); + intent.setAction("com.example.custom_action"); + intent.putExtra("token", token); + intent.putExtra("refreshToken", refreshToken); + context.sendBroadcast(intent); + } + + //Tests broadcast of sensitive user information with intent extra. + public void sendBroadcast2(Context context) { + String username = "test123"; + String password = "abc12345"; + + Intent intent = new Intent(); + intent.setAction("com.example.custom_action"); + intent.putExtra("name", username); + intent.putExtra("pwd", password); + context.sendBroadcast(intent); + } + + //Tests broadcast of sensitive user information with extra bundle. + public void sendBroadcast3(Context context) { + String username = "test123"; + String password = "abc12345"; + + Intent intent = new Intent(); + intent.setAction("com.example.custom_action"); + Bundle bundle = new Bundle(); + bundle.putCharSequence("name", username); + bundle.putCharSequence("pwd", password); + intent.putExtras(bundle); + context.sendBroadcast(intent); + } + + //Tests broadcast of sensitive user information with permission. + public void sendBroadcast4(Context context) { + String username = "test123"; + String password = "abc12345"; + + Intent intent = new Intent(); + intent.setAction("com.example.custom_action"); + intent.putExtra("name", username); + intent.putExtra("pwd", password); + context.sendBroadcast(intent, "com.example.user_permission"); + } + + //Tests broadcast of sensitive user information to a specific application. + public void sendBroadcast5(Context context) { + String username = "test123"; + String password = "abc12345"; + + Intent intent = new Intent(); + intent.setAction("com.example.custom_action"); + intent.setClassName("com.example2", "com.example2.UserInfoHandler"); + intent.putExtra("name", username); + intent.putExtra("pwd", password); + context.sendBroadcast(intent); + } +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.qlref b/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.qlref new file mode 100644 index 00000000000..20c5250d80d --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql diff --git a/java/ql/test/experimental/query-tests/security/CWE-927/options b/java/ql/test/experimental/query-tests/security/CWE-927/options new file mode 100644 index 00000000000..43e25f608b6 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-927/options @@ -0,0 +1 @@ +// semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/google-android-9.0.0 diff --git a/java/ql/test/stubs/google-android-9.0.0/android/content/Intent.java b/java/ql/test/stubs/google-android-9.0.0/android/content/Intent.java index e3ef6277dba..292069c0f71 100644 --- a/java/ql/test/stubs/google-android-9.0.0/android/content/Intent.java +++ b/java/ql/test/stubs/google-android-9.0.0/android/content/Intent.java @@ -1996,4 +1996,77 @@ public class Intent implements Parcelable, Cloneable { public void readFromParcel(Parcel in) { } + + /** + * Retrieve the application package name this Intent is limited to. When + * resolving an Intent, if non-null this limits the resolution to only + * components in the given application package. + * + * @return The name of the application package for the Intent. + * + * @see #resolveActivity + * @see #setPackage + */ + public String getPackage() { + return null; + } + + /** + * (Usually optional) Set an explicit application package name that limits + * the components this Intent will resolve to. If left to the default + * value of null, all components in all applications will considered. + * If non-null, the Intent can only match the components in the given + * application package. + * + * @param packageName The name of the application package to handle the + * intent, or null to allow any application package. + * + * @return Returns the same Intent object, for chaining multiple calls + * into a single statement. + * + * @see #getPackage + * @see #resolveActivity + */ + public Intent setPackage(String packageName) { + return null; + } + + /** + * Convenience for calling {@link #setComponent} with an + * explicit class name. + * + * @param packageContext A Context of the application package implementing + * this class. + * @param className The name of a class inside of the application package + * that will be used as the component for this Intent. + * + * @return Returns the same Intent object, for chaining multiple calls + * into a single statement. + * + * @see #setComponent + * @see #setClass + */ + public Intent setClassName(Context packageContext, String className) { + return null; + } + + /** + * Convenience for calling {@link #setComponent} with an + * explicit application package name and class name. + * + * @param packageName The name of the package implementing the desired + * component. + * @param className The name of a class inside of the application package + * that will be used as the component for this Intent. + * + * @return Returns the same Intent object, for chaining multiple calls + * into a single statement. + * + * @see #setComponent + * @see #setClass + */ + public Intent setClassName(String packageName, String className) { + return null; + } + } \ No newline at end of file From 478771ccc50affd8ec1a3e879c1a3fab87893288 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Wed, 21 Oct 2020 02:49:53 +0000 Subject: [PATCH 02/11] Fix issues with method signature check --- .../CWE/CWE-927/SensitiveBroadcast.ql | 17 +- .../android/content/BroadcastReceiver.java | 255 ++++++++++++++++++ .../android/content/Context.java | 47 ++++ 3 files changed, 317 insertions(+), 2 deletions(-) create mode 100644 java/ql/test/stubs/google-android-9.0.0/android/content/BroadcastReceiver.java diff --git a/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql index 9b024a9866e..68dffdce50f 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql @@ -79,11 +79,24 @@ predicate isSensitiveBroadcastSink(DataFlow::Node sink) { ) or ma.getMethod().hasName("sendBroadcastWithMultiplePermissions") and - ma.getArgument(1) instanceof NullLiteral // sendBroadcastWithMultiplePermissions(Intent intent, String[] receiverPermissions) + ( + ma.getArgument(1).(ArrayCreationExpr).getFirstDimensionSize() = 0 // sendBroadcastWithMultiplePermissions(Intent intent, String[] receiverPermissions) + or + exists(Variable v | + v.getAnAccess() = ma.getArgument(1) and + v.getAnAssignedValue().(ArrayCreationExpr).getFirstDimensionSize() = 0 + ) + ) or //Method calls of `sendOrderedBroadcast` whose second argument is always `receiverPermission` ma.getMethod().hasName("sendOrderedBroadcast") and - ma.getArgument(1) instanceof NullLiteral + ( + ma.getArgument(1) instanceof NullLiteral and ma.getNumArgument() <=7 + or + ma.getArgument(1) instanceof NullLiteral and + ma.getArgument(2) instanceof NullLiteral and + ma.getNumArgument() = 8 + ) or //Method call of `sendOrderedBroadcastAsUser(Intent intent, UserHandle user, String receiverPermission, BroadcastReceiver resultReceiver, Handler scheduler, int initialCode, String initialData, Bundle initialExtras)` ma.getMethod().hasName("sendOrderedBroadcastAsUser") and diff --git a/java/ql/test/stubs/google-android-9.0.0/android/content/BroadcastReceiver.java b/java/ql/test/stubs/google-android-9.0.0/android/content/BroadcastReceiver.java new file mode 100644 index 00000000000..1d73018c96d --- /dev/null +++ b/java/ql/test/stubs/google-android-9.0.0/android/content/BroadcastReceiver.java @@ -0,0 +1,255 @@ +/* + * Copyright (C) 2006 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package android.content; + +import android.os.Bundle; +/** + * Base class for code that will receive intents sent by sendBroadcast(). + * + *

    If you don't need to send broadcasts across applications, consider using + * this class with {@link android.support.v4.content.LocalBroadcastManager} instead + * of the more general facilities described below. This will give you a much + * more efficient implementation (no cross-process communication needed) and allow + * you to avoid thinking about any security issues related to other applications + * being able to receive or send your broadcasts. + * + *

    You can either dynamically register an instance of this class with + * {@link Context#registerReceiver Context.registerReceiver()} + * or statically publish an implementation through the + * {@link android.R.styleable#AndroidManifestReceiver <receiver>} + * tag in your AndroidManifest.xml. + * + *

    Note: + *    If registering a receiver in your + * {@link android.app.Activity#onResume() Activity.onResume()} + * implementation, you should unregister it in + * {@link android.app.Activity#onPause() Activity.onPause()}. + * (You won't receive intents when paused, + * and this will cut down on unnecessary system overhead). Do not unregister in + * {@link android.app.Activity#onSaveInstanceState(android.os.Bundle) Activity.onSaveInstanceState()}, + * because this won't be called if the user moves back in the history + * stack. + * + *

    There are two major classes of broadcasts that can be received:

    + * + * + *

    Even in the case of normal broadcasts, the system may in some + * situations revert to delivering the broadcast one receiver at a time. In + * particular, for receivers that may require the creation of a process, only + * one will be run at a time to avoid overloading the system with new processes. + * In this situation, however, the non-ordered semantics hold: these receivers still + * cannot return results or abort their broadcast.

    + * + *

    Note that, although the Intent class is used for sending and receiving + * these broadcasts, the Intent broadcast mechanism here is completely separate + * from Intents that are used to start Activities with + * {@link Context#startActivity Context.startActivity()}. + * There is no way for a BroadcastReceiver + * to see or capture Intents used with startActivity(); likewise, when + * you broadcast an Intent, you will never find or start an Activity. + * These two operations are semantically very different: starting an + * Activity with an Intent is a foreground operation that modifies what the + * user is currently interacting with; broadcasting an Intent is a background + * operation that the user is not normally aware of. + * + *

    The BroadcastReceiver class (when launched as a component through + * a manifest's {@link android.R.styleable#AndroidManifestReceiver <receiver>} + * tag) is an important part of an + * application's overall lifecycle.

    + * + *

    Topics covered here: + *

      + *
    1. Security + *
    2. Receiver Lifecycle + *
    3. Process Lifecycle + *
    + * + *
    + *

    Developer Guides

    + *

    For information about how to use this class to receive and resolve intents, read the + * Intents and Intent Filters + * developer guide.

    + *
    + * + * + *

    Security

    + * + *

    Receivers used with the {@link Context} APIs are by their nature a + * cross-application facility, so you must consider how other applications + * may be able to abuse your use of them. Some things to consider are: + * + *

    + * + *

    None of these issues exist when using + * {@link android.support.v4.content.LocalBroadcastManager}, since intents + * broadcast it never go outside of the current process. + * + *

    Access permissions can be enforced by either the sender or receiver + * of a broadcast. + * + *

    To enforce a permission when sending, you supply a non-null + * permission argument to + * {@link Context#sendBroadcast(Intent, String)} or + * {@link Context#sendOrderedBroadcast(Intent, String, BroadcastReceiver, android.os.Handler, int, String, Bundle)}. + * Only receivers who have been granted this permission + * (by requesting it with the + * {@link android.R.styleable#AndroidManifestUsesPermission <uses-permission>} + * tag in their AndroidManifest.xml) will be able to receive + * the broadcast. + * + *

    To enforce a permission when receiving, you supply a non-null + * permission when registering your receiver -- either when calling + * {@link Context#registerReceiver(BroadcastReceiver, IntentFilter, String, android.os.Handler)} + * or in the static + * {@link android.R.styleable#AndroidManifestReceiver <receiver>} + * tag in your AndroidManifest.xml. Only broadcasters who have + * been granted this permission (by requesting it with the + * {@link android.R.styleable#AndroidManifestUsesPermission <uses-permission>} + * tag in their AndroidManifest.xml) will be able to send an + * Intent to the receiver. + * + *

    See the Security and Permissions + * document for more information on permissions and security in general. + * + * + *

    Receiver Lifecycle

    + * + *

    A BroadcastReceiver object is only valid for the duration of the call + * to {@link #onReceive}. Once your code returns from this function, + * the system considers the object to be finished and no longer active. + * + *

    This has important repercussions to what you can do in an + * {@link #onReceive} implementation: anything that requires asynchronous + * operation is not available, because you will need to return from the + * function to handle the asynchronous operation, but at that point the + * BroadcastReceiver is no longer active and thus the system is free to kill + * its process before the asynchronous operation completes. + * + *

    In particular, you may not show a dialog or bind to a service from + * within a BroadcastReceiver. For the former, you should instead use the + * {@link android.app.NotificationManager} API. For the latter, you can + * use {@link android.content.Context#startService Context.startService()} to + * send a command to the service. + * + * + *

    Process Lifecycle

    + * + *

    A process that is currently executing a BroadcastReceiver (that is, + * currently running the code in its {@link #onReceive} method) is + * considered to be a foreground process and will be kept running by the + * system except under cases of extreme memory pressure. + * + *

    Once you return from onReceive(), the BroadcastReceiver is no longer + * active, and its hosting process is only as important as any other application + * components that are running in it. This is especially important because if + * that process was only hosting the BroadcastReceiver (a common case for + * applications that the user has never or not recently interacted with), then + * upon returning from onReceive() the system will consider its process + * to be empty and aggressively kill it so that resources are available for other + * more important processes. + * + *

    This means that for longer-running operations you will often use + * a {@link android.app.Service} in conjunction with a BroadcastReceiver to keep + * the containing process active for the entire time of your operation. + */ +public abstract class BroadcastReceiver { + + /** + * State for a result that is pending for a broadcast receiver. Returned + * by {@link BroadcastReceiver#goAsync() goAsync()} + * while in {@link BroadcastReceiver#onReceive BroadcastReceiver.onReceive()}. + * This allows you to return from onReceive() without having the broadcast + * terminate; you must call {@link #finish()} once you are done with the + * broadcast. This allows you to process the broadcast off of the main + * thread of your app. + * + *

    Note on threading: the state inside of this class is not itself + * thread-safe, however you can use it from any thread if you properly + * sure that you do not have races. Typically this means you will hand + * the entire object to another thread, which will be solely responsible + * for setting any results and finally calling {@link #finish()}. + */ + + public BroadcastReceiver() { + } + /** + * This method is called when the BroadcastReceiver is receiving an Intent + * broadcast. During this time you can use the other methods on + * BroadcastReceiver to view/modify the current result values. This method + * is always called within the main thread of its process, unless you + * explicitly asked for it to be scheduled on a different thread using + * {@link android.content.Context#registerReceiver(BroadcastReceiver, + * IntentFilter, String, android.os.Handler)}. When it runs on the main + * thread you should + * never perform long-running operations in it (there is a timeout of + * 10 seconds that the system allows before considering the receiver to + * be blocked and a candidate to be killed). You cannot launch a popup dialog + * in your implementation of onReceive(). + * + *

    If this BroadcastReceiver was launched through a <receiver> tag, + * then the object is no longer alive after returning from this + * function. This means you should not perform any operations that + * return a result to you asynchronously -- in particular, for interacting + * with services, you should use + * {@link Context#startService(Intent)} instead of + * {@link Context#bindService(Intent, ServiceConnection, int)}. If you wish + * to interact with a service that is already running, you can use + * {@link #peekService}. + * + *

    The Intent filters used in {@link android.content.Context#registerReceiver} + * and in application manifests are not guaranteed to be exclusive. They + * are hints to the operating system about how to find suitable recipients. It is + * possible for senders to force delivery to specific recipients, bypassing filter + * resolution. For this reason, {@link #onReceive(Context, Intent) onReceive()} + * implementations should respond only to known actions, ignoring any unexpected + * Intents that they may receive. + * + * @param context The Context in which the receiver is running. + * @param intent The Intent being received. + */ + public abstract void onReceive(Context context, Intent intent); +} \ No newline at end of file diff --git a/java/ql/test/stubs/google-android-9.0.0/android/content/Context.java b/java/ql/test/stubs/google-android-9.0.0/android/content/Context.java index 8c528a58e41..e1ce2140a06 100644 --- a/java/ql/test/stubs/google-android-9.0.0/android/content/Context.java +++ b/java/ql/test/stubs/google-android-9.0.0/android/content/Context.java @@ -813,4 +813,51 @@ public abstract class Context { * @hide */ public abstract void sendBroadcast(Intent intent, String receiverPermission, int appOp); + + /** + * Broadcast the given intent to all interested BroadcastReceivers, allowing + * an array of required permissions to be enforced. This call is asynchronous; it returns + * immediately, and you will continue executing while the receivers are run. No results are + * propagated from receivers and receivers can not abort the broadcast. If you want to allow + * receivers to propagate results or abort the broadcast, you must send an ordered broadcast + * using {@link #sendOrderedBroadcast(Intent, String)}. + * + *

    See {@link BroadcastReceiver} for more information on Intent broadcasts. + * + * @param intent The Intent to broadcast; all receivers matching this + * Intent will receive the broadcast. + * @param receiverPermissions Array of names of permissions that a receiver must hold + * in order to receive your broadcast. + * If empty, no permissions are required. + * + * @see android.content.BroadcastReceiver + * @see #registerReceiver + * @see #sendBroadcast(Intent) + * @see #sendOrderedBroadcast(Intent, String) + * @see #sendOrderedBroadcast(Intent, String, BroadcastReceiver, Handler, int, String, Bundle) + * @hide + */ + public abstract void sendBroadcastWithMultiplePermissions (Intent intent, String[] receiverPermissions); + + /** + * Broadcast the given intent to all interested BroadcastReceivers, delivering + * them one at a time to allow more preferred receivers to consume the + * broadcast before it is delivered to less preferred receivers. This + * call is asynchronous; it returns immediately, and you will continue + * executing while the receivers are run. + * + *

    See {@link BroadcastReceiver} for more information on Intent broadcasts. + * + * @param intent The Intent to broadcast; all receivers matching this + * Intent will receive the broadcast. + * @param receiverPermission (optional) String naming a permissions that + * a receiver must hold in order to receive your broadcast. + * If null, no permission is required. + * + * @see android.content.BroadcastReceiver + * @see #registerReceiver + * @see #sendBroadcast(Intent) + * @see #sendOrderedBroadcast(Intent, String, BroadcastReceiver, Handler, int, String, Bundle) + */ + public abstract void sendOrderedBroadcast(Intent intent, String receiverPermission); } \ No newline at end of file From d9c140dc6c1139260ec6f0b05fa59c2a2b75f3b7 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Sun, 25 Oct 2020 15:33:09 +0000 Subject: [PATCH 03/11] Enhance the query to use sanitizer and null/empty array flow --- .../CWE/CWE-927/SensitiveBroadcast.java | 14 ++- .../CWE/CWE-927/SensitiveBroadcast.qhelp | 8 +- .../CWE/CWE-927/SensitiveBroadcast.ql | 117 ++++++++++++------ .../CWE-927/SensitiveBroadcast.expected | 24 +--- .../security/CWE-927/SensitiveBroadcast.java | 68 +++++++++- 5 files changed, 166 insertions(+), 65 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.java b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.java index e9fd7b15dc1..40e5f8e1bbc 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.java +++ b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.java @@ -1,7 +1,7 @@ public void sendBroadcast1(Context context, String token, String refreshToken) { { - // BAD: broadcast sensitive information without permission + // BAD: broadcast sensitive information to all listeners Intent intent = new Intent(); intent.setAction("com.example.custom_action"); intent.putExtra("token", token); @@ -10,11 +10,21 @@ public void sendBroadcast1(Context context, String token, String refreshToken) } { - // GOOD: broadcast sensitive information with permission + // GOOD: broadcast sensitive information only to those with permission Intent intent = new Intent(); intent.setAction("com.example.custom_action"); intent.putExtra("token", token); intent.putExtra("refreshToken", refreshToken); context.sendBroadcast(intent, "com.example.user_permission"); } + + { + // GOOD: broadcast sensitive information to a specific application + Intent intent = new Intent(); + intent.setAction("com.example.custom_action"); + intent.setClassName("com.example2", "com.example2.UserInfoHandler"); + intent.putExtra("token", token); + intent.putExtra("refreshToken", refreshToken); + context.sendBroadcast(intent); + } } \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.qhelp b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.qhelp index b9acfd7b7c9..c1f44a4abb0 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.qhelp @@ -4,16 +4,16 @@ -

    Broadcasted intents in an Android application are visible to all applications installed on the same mobile device, exposing all sensitive information they contain.

    -

    Broadcasts are vulnerable to passive eavesdropping or active denial of service attacks when an intent is broadcasted without specifying any receiver permission or receiver application.

    +

    Broadcast intents in an Android application are visible to all applications installed on the same mobile device, exposing all sensitive information they contain.

    +

    Broadcasts are vulnerable to passive eavesdropping or active denial of service attacks when an intent is broadcast without specifying any receiver permission or receiver application.

    -

    Specify receiver permission or specify receiver application in broadcasted intents, or switch to LocalBroadcastManager or the latest LiveData library.

    +

    Specify a receiver permission or application when broadcasting intents, or switch to LocalBroadcastManager or the latest LiveData library.

     -

    The following example shows two ways of broadcasting intents. In the 'BAD' case, no "receiver permission" is specified. In the 'GOOD' case, "receiver permission" is specified.

    +

    The following example shows two ways of broadcasting intents. In the 'BAD' case, no "receiver permission" is specified. In the 'GOOD' case, "receiver permission" or "receiver application" is specified.

     diff --git a/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql index 68dffdce50f..b22e8d22ad4 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql @@ -1,5 +1,5 @@ /** - * @name Use of Implicit Intent for Sensitive Communication + * @name Broadcasting sensitive data to all Android applicationss * @id java/sensitive-broadcast * @description An Android application uses implicit intents to broadcast sensitive data to all applications without specifying any receiver permission. * @kind path-problem @@ -10,33 +10,37 @@ import java import semmle.code.java.frameworks.android.Intent import semmle.code.java.dataflow.TaintTracking -import DataFlow -import PathGraph /** - * Gets a regular expression for matching names of variables that indicate the value being held contains sensitive information. + * Gets a regular expression for matching common names of variables that indicate the value being held contains sensitive information. */ -private string getSensitiveInfoRegex() { +private string getCommonSensitiveInfoRegex() { result = "(?i).*challenge|pass(wd|word|code|phrase)(?!.*question).*" or - result = "(?i).*(token|email|phone|username|userid|ticket).*" + result = "(?i).*(token|username|userid|secret).*" } /** - * Method call to pass information to the `Intent` object either directly through intent extra or indirectly through intent extra bundle. + * Gets regular expression for matching names of Android variables that indicate the value being held contains sensitive information. */ -class PutExtraMethodAccess extends MethodAccess { - PutExtraMethodAccess() { +private string getAndroidSensitiveInfoRegex() { result = "(?i).*(email|phone|ticket).*" } + +/** + * Method call to pass information to the `Intent` object. + */ +class PutIntentExtraMethodAccess extends MethodAccess { + PutIntentExtraMethodAccess() { getMethod().getName().regexpMatch("put\\w*Extra(s*)") and - getMethod().getDeclaringType() instanceof TypeIntent and - not exists( - MethodAccess setPackageVa // Intent without specifying receiving package name of the 3rd party app - | - setPackageVa.getMethod().hasName(["setPackage", "setClass", "setClassName", "setComponent"]) and - setPackageVa.getQualifier().(VarAccess).getVariable().getAnAccess() = getQualifier() - ) - or + getMethod().getDeclaringType() instanceof TypeIntent + } +} + +/** + * Method call to pass information to the intent extra bundle object. + */ +class PutBundleExtraMethodAccess extends MethodAccess { + PutBundleExtraMethodAccess() { getMethod().getName().regexpMatch("put\\w+") and - getMethod().getDeclaringType().hasQualifiedName("android.os", "Bundle") + getMethod().getDeclaringType().getASupertype*().hasQualifiedName("android.os", "BaseBundle") } } @@ -44,7 +48,10 @@ class PutExtraMethodAccess extends MethodAccess { class SensitiveInfoExpr extends Expr { SensitiveInfoExpr() { exists(Variable v | this = v.getAnAccess() | - v.getName().toLowerCase().regexpMatch(getSensitiveInfoRegex()) + ( + v.getName().toLowerCase().regexpMatch(getCommonSensitiveInfoRegex()) or + v.getName().toLowerCase().regexpMatch(getAndroidSensitiveInfoRegex()) + ) ) } } @@ -59,6 +66,24 @@ class SendBroadcastMethodAccess extends MethodAccess { } } +private class NullArgFlowConfig extends DataFlow2::Configuration { + NullArgFlowConfig() { this = "Flow configuration with a null argument" } + + override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof NullLiteral } + + override predicate isSink(DataFlow::Node sink) { any() } +} + +private class EmptyArrayArgFlowConfig extends DataFlow2::Configuration { + EmptyArrayArgFlowConfig() { this = "Flow configuration with an empty array argument" } + + override predicate isSource(DataFlow::Node src) { + src.asExpr().(ArrayCreationExpr).getFirstDimensionSize() = 0 + } + + override predicate isSink(DataFlow::Node sink) { any() } +} + /** * Holds if a `sendBroadcast` call doesn't specify receiver permission. */ @@ -68,47 +93,47 @@ predicate isSensitiveBroadcastSink(DataFlow::Node sink) { ( ma.getMethod().hasName("sendBroadcast") and ( - ma.getNumArgument() = 1 or // sendBroadcast(Intent intent) - ma.getArgument(1) instanceof NullLiteral // sendBroadcast(Intent intent, String receiverPermission) + ma.getNumArgument() = 1 // sendBroadcast(Intent intent) + or + // sendBroadcast(Intent intent, String receiverPermission) + exists(NullArgFlowConfig conf | conf.hasFlow(_, DataFlow::exprNode(ma.getArgument(1)))) ) or ma.getMethod().hasName("sendBroadcastAsUser") and ( ma.getNumArgument() = 2 or // sendBroadcastAsUser(Intent intent, UserHandle user) - ma.getArgument(2) instanceof NullLiteral // sendBroadcastAsUser(Intent intent, UserHandle user, String receiverPermission) + exists(NullArgFlowConfig conf | conf.hasFlow(_, DataFlow::exprNode(ma.getArgument(2)))) // sendBroadcastAsUser(Intent intent, UserHandle user, String receiverPermission) ) or ma.getMethod().hasName("sendBroadcastWithMultiplePermissions") and - ( - ma.getArgument(1).(ArrayCreationExpr).getFirstDimensionSize() = 0 // sendBroadcastWithMultiplePermissions(Intent intent, String[] receiverPermissions) - or - exists(Variable v | - v.getAnAccess() = ma.getArgument(1) and - v.getAnAssignedValue().(ArrayCreationExpr).getFirstDimensionSize() = 0 - ) + exists(EmptyArrayArgFlowConfig config | + config.hasFlow(_, DataFlow::exprNode(ma.getArgument(1))) // sendBroadcastWithMultiplePermissions(Intent intent, String[] receiverPermissions) ) or //Method calls of `sendOrderedBroadcast` whose second argument is always `receiverPermission` ma.getMethod().hasName("sendOrderedBroadcast") and ( - ma.getArgument(1) instanceof NullLiteral and ma.getNumArgument() <=7 + // sendOrderedBroadcast(Intent intent, String receiverPermission) or sendOrderedBroadcast(Intent intent, String receiverPermission, BroadcastReceiver resultReceiver, Handler scheduler, int initialCode, String initialData, Bundle initialExtras) + exists(NullArgFlowConfig conf | conf.hasFlow(_, DataFlow::exprNode(ma.getArgument(1)))) and + ma.getNumArgument() <= 7 or - ma.getArgument(1) instanceof NullLiteral and - ma.getArgument(2) instanceof NullLiteral and + // sendOrderedBroadcast(Intent intent, String receiverPermission, String receiverAppOp, BroadcastReceiver resultReceiver, Handler scheduler, int initialCode, String initialData, Bundle initialExtras) + exists(NullArgFlowConfig conf | conf.hasFlow(_, DataFlow::exprNode(ma.getArgument(1)))) and + exists(NullArgFlowConfig conf | conf.hasFlow(_, DataFlow::exprNode(ma.getArgument(2)))) and ma.getNumArgument() = 8 ) or //Method call of `sendOrderedBroadcastAsUser(Intent intent, UserHandle user, String receiverPermission, BroadcastReceiver resultReceiver, Handler scheduler, int initialCode, String initialData, Bundle initialExtras)` ma.getMethod().hasName("sendOrderedBroadcastAsUser") and - ma.getArgument(2) instanceof NullLiteral + exists(NullArgFlowConfig conf | conf.hasFlow(_, DataFlow::exprNode(ma.getArgument(2)))) ) ) } /** - * Taint configuration tracking flow from variables containing sensitive information to broadcasted intents. + * Taint configuration tracking flow from variables containing sensitive information to broadcast intents. */ -class SensitiveBroadcastConfig extends DataFlow::Configuration { +class SensitiveBroadcastConfig extends TaintTracking::Configuration { SensitiveBroadcastConfig() { this = "Sensitive Broadcast Configuration" } override predicate isSource(DataFlow::Node source) { @@ -118,11 +143,25 @@ class SensitiveBroadcastConfig extends DataFlow::Configuration { override predicate isSink(DataFlow::Node sink) { isSensitiveBroadcastSink(sink) } /** - * Holds if there is an additional flow step from `PutExtraMethodAccess` to a broadcasted intent. + * Holds if there is an additional flow step from `PutIntentExtraMethodAccess` or `PutBundleExtraMethodAccess` to a broadcast intent. */ - override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { - exists(PutExtraMethodAccess pa | - node1.asExpr() = pa.getAnArgument() and node2.asExpr() = pa.getQualifier() + override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(PutIntentExtraMethodAccess pia | + node1.asExpr() = pia.getAnArgument() and node2.asExpr() = pia.getQualifier() + ) + or + exists(PutBundleExtraMethodAccess pba | + node1.asExpr() = pba.getAnArgument() and node2.asExpr() = pba.getQualifier() + ) + } + + /** + * Holds if broadcast doesn't specify receiving package name of the 3rd party app + */ + override predicate isSanitizer(DataFlow::Node node) { + exists(MethodAccess setReceiverMa | + setReceiverMa.getMethod().hasName(["setPackage", "setClass", "setClassName", "setComponent"]) and + setReceiverMa.getQualifier().(VarAccess).getVariable().getAnAccess() = node.asExpr() ) } } diff --git a/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.expected b/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.expected index 163a34e6a49..346faa17f42 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.expected @@ -1,24 +1,12 @@ -edges -| SensitiveBroadcast.java:11:34:11:38 | token : String | SensitiveBroadcast.java:13:31:13:36 | intent | -| SensitiveBroadcast.java:12:41:12:52 | refreshToken : String | SensitiveBroadcast.java:13:31:13:36 | intent | -| SensitiveBroadcast.java:23:33:23:40 | username : String | SensitiveBroadcast.java:25:31:25:36 | intent | -| SensitiveBroadcast.java:24:32:24:39 | password : String | SensitiveBroadcast.java:25:31:25:36 | intent | -| SensitiveBroadcast.java:36:40:36:47 | username : String | SensitiveBroadcast.java:39:31:39:36 | intent | -| SensitiveBroadcast.java:37:39:37:46 | password : String | SensitiveBroadcast.java:39:31:39:36 | intent | -nodes -| SensitiveBroadcast.java:11:34:11:38 | token : String | semmle.label | token : String | -| SensitiveBroadcast.java:12:41:12:52 | refreshToken : String | semmle.label | refreshToken : String | -| SensitiveBroadcast.java:13:31:13:36 | intent | semmle.label | intent | -| SensitiveBroadcast.java:23:33:23:40 | username : String | semmle.label | username : String | -| SensitiveBroadcast.java:24:32:24:39 | password : String | semmle.label | password : String | -| SensitiveBroadcast.java:25:31:25:36 | intent | semmle.label | intent | -| SensitiveBroadcast.java:36:40:36:47 | username : String | semmle.label | username : String | -| SensitiveBroadcast.java:37:39:37:46 | password : String | semmle.label | password : String | -| SensitiveBroadcast.java:39:31:39:36 | intent | semmle.label | intent | -#select | SensitiveBroadcast.java:13:31:13:36 | intent | SensitiveBroadcast.java:11:34:11:38 | token : String | SensitiveBroadcast.java:13:31:13:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:11:34:11:38 | token | sensitive information | | SensitiveBroadcast.java:13:31:13:36 | intent | SensitiveBroadcast.java:12:41:12:52 | refreshToken : String | SensitiveBroadcast.java:13:31:13:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:12:41:12:52 | refreshToken | sensitive information | | SensitiveBroadcast.java:25:31:25:36 | intent | SensitiveBroadcast.java:23:33:23:40 | username : String | SensitiveBroadcast.java:25:31:25:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:23:33:23:40 | username | sensitive information | | SensitiveBroadcast.java:25:31:25:36 | intent | SensitiveBroadcast.java:24:32:24:39 | password : String | SensitiveBroadcast.java:25:31:25:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:24:32:24:39 | password | sensitive information | | SensitiveBroadcast.java:39:31:39:36 | intent | SensitiveBroadcast.java:36:40:36:47 | username : String | SensitiveBroadcast.java:39:31:39:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:36:40:36:47 | username | sensitive information | | SensitiveBroadcast.java:39:31:39:36 | intent | SensitiveBroadcast.java:37:39:37:46 | password : String | SensitiveBroadcast.java:39:31:39:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:37:39:37:46 | password | sensitive information | +| SensitiveBroadcast.java:89:54:89:59 | intent | SensitiveBroadcast.java:87:33:87:40 | username : String | SensitiveBroadcast.java:89:54:89:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:87:33:87:40 | username | sensitive information | +| SensitiveBroadcast.java:89:54:89:59 | intent | SensitiveBroadcast.java:88:32:88:39 | password : String | SensitiveBroadcast.java:89:54:89:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:88:32:88:39 | password | sensitive information | +| SensitiveBroadcast.java:102:54:102:59 | intent | SensitiveBroadcast.java:99:33:99:40 | username : String | SensitiveBroadcast.java:102:54:102:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:99:33:99:40 | username | sensitive information | +| SensitiveBroadcast.java:102:54:102:59 | intent | SensitiveBroadcast.java:100:32:100:39 | password : String | SensitiveBroadcast.java:102:54:102:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:100:32:100:39 | password | sensitive information | +| SensitiveBroadcast.java:116:54:116:59 | intent | SensitiveBroadcast.java:112:33:112:40 | username : String | SensitiveBroadcast.java:116:54:116:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:112:33:112:40 | username | sensitive information | +| SensitiveBroadcast.java:116:54:116:59 | intent | SensitiveBroadcast.java:113:32:113:39 | password : String | SensitiveBroadcast.java:116:54:116:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:113:32:113:39 | password | sensitive information | diff --git a/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.java b/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.java index c84e709a8d6..12cec1de601 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.java +++ b/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.java @@ -39,7 +39,7 @@ class SensitiveBroadcast { context.sendBroadcast(intent); } - //Tests broadcast of sensitive user information with permission. + //Tests broadcast of sensitive user information with permission using string literal. public void sendBroadcast4(Context context) { String username = "test123"; String password = "abc12345"; @@ -51,11 +51,24 @@ class SensitiveBroadcast { context.sendBroadcast(intent, "com.example.user_permission"); } - //Tests broadcast of sensitive user information to a specific application. + //Tests broadcast of sensitive user information with permission using string object. public void sendBroadcast5(Context context) { String username = "test123"; String password = "abc12345"; + Intent intent = new Intent(); + intent.setAction("com.example.custom_action"); + intent.putExtra("name", username); + intent.putExtra("pwd", password); + String perm = "com.example.user_permission"; + context.sendBroadcast(intent, perm); + } + + //Tests broadcast of sensitive user information to a specific application. + public void sendBroadcast6(Context context) { + String username = "test123"; + String password = "abc12345"; + Intent intent = new Intent(); intent.setAction("com.example.custom_action"); intent.setClassName("com.example2", "com.example2.UserInfoHandler"); @@ -63,4 +76,55 @@ class SensitiveBroadcast { intent.putExtra("pwd", password); context.sendBroadcast(intent); } + + //Tests broadcast of sensitive user information with multiple permissions using direct empty array initialization. + public void sendBroadcast7(Context context) { + String username = "test123"; + String password = "abc12345"; + + Intent intent = new Intent(); + intent.setAction("com.example.custom_action"); + intent.putExtra("name", username); + intent.putExtra("pwd", password); + context.sendBroadcastWithMultiplePermissions(intent, new String[]{}); + } + + //Tests broadcast of sensitive user information with multiple permissions using empty array initialization through a variable. + public void sendBroadcast8(Context context) { + String username = "test123"; + String password = "abc12345"; + + Intent intent = new Intent(); + intent.setAction("com.example.custom_action"); + intent.putExtra("name", username); + intent.putExtra("pwd", password); + String[] perms = new String[0]; + context.sendBroadcastWithMultiplePermissions(intent, perms); + } + + //Tests broadcast of sensitive user information with multiple permissions using empty array initialization through two variables. + public void sendBroadcast9(Context context) { + String username = "test123"; + String password = "abc12345"; + + Intent intent = new Intent(); + intent.setAction("com.example.custom_action"); + intent.putExtra("name", username); + intent.putExtra("pwd", password); + String[] perms = new String[0]; + String[] perms2 = perms; + context.sendBroadcastWithMultiplePermissions(intent, perms2); + } + + //Tests broadcast of sensitive user information with ordered broadcast. + public void sendBroadcast10(Context context) { + String username = "test123"; + String password = "abc12345"; + + Intent intent = new Intent(); + intent.setAction("com.example.custom_action"); + intent.putExtra("name", username); + intent.putExtra("pwd", password); + context.sendOrderedBroadcast(intent, "com.example.USER_PERM"); + } } From 07830aae05d0394541116567a15001d7374d41fc Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Sun, 25 Oct 2020 22:34:15 +0000 Subject: [PATCH 04/11] Fix typo --- .../experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql index b22e8d22ad4..0e606984a4d 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql @@ -1,5 +1,5 @@ /** - * @name Broadcasting sensitive data to all Android applicationss + * @name Broadcasting sensitive data to all Android applications * @id java/sensitive-broadcast * @description An Android application uses implicit intents to broadcast sensitive data to all applications without specifying any receiver permission. * @kind path-problem @@ -29,7 +29,7 @@ private string getAndroidSensitiveInfoRegex() { result = "(?i).*(email|phone|tic */ class PutIntentExtraMethodAccess extends MethodAccess { PutIntentExtraMethodAccess() { - getMethod().getName().regexpMatch("put\\w*Extra(s*)") and + getMethod().getName().regexpMatch("put\\w*Extra(s?)") and getMethod().getDeclaringType() instanceof TypeIntent } } From 99c79f4aa3bc62ae8d22141edf6e33a3e889dd72 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Wed, 28 Oct 2020 03:07:01 +0000 Subject: [PATCH 05/11] Enhance the dataflow sink and update test cases --- .../CWE/CWE-927/SensitiveBroadcast.ql | 33 +++---- .../code/java/security/SensitiveActions.qll | 8 ++ .../CWE-927/SensitiveBroadcast.expected | 18 ++-- .../security/CWE-927/SensitiveBroadcast.java | 93 ++++++++++++------- 4 files changed, 89 insertions(+), 63 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql index 0e606984a4d..3fd10b5d45b 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql @@ -8,16 +8,10 @@ */ import java -import semmle.code.java.frameworks.android.Intent +import semmle.code.java.dataflow.DataFlow3 import semmle.code.java.dataflow.TaintTracking - -/** - * Gets a regular expression for matching common names of variables that indicate the value being held contains sensitive information. - */ -private string getCommonSensitiveInfoRegex() { - result = "(?i).*challenge|pass(wd|word|code|phrase)(?!.*question).*" or - result = "(?i).*(token|username|userid|secret).*" -} +import semmle.code.java.frameworks.android.Intent +import semmle.code.java.security.SensitiveActions /** * Gets regular expression for matching names of Android variables that indicate the value being held contains sensitive information. @@ -48,16 +42,13 @@ class PutBundleExtraMethodAccess extends MethodAccess { class SensitiveInfoExpr extends Expr { SensitiveInfoExpr() { exists(Variable v | this = v.getAnAccess() | - ( - v.getName().toLowerCase().regexpMatch(getCommonSensitiveInfoRegex()) or - v.getName().toLowerCase().regexpMatch(getAndroidSensitiveInfoRegex()) - ) + v.getName().regexpMatch([getCommonSensitiveInfoRegex(), getAndroidSensitiveInfoRegex()]) ) } } /** - * The method access of `context.sendBroadcast`. + * A method access of the `context.sendBroadcast` family. */ class SendBroadcastMethodAccess extends MethodAccess { SendBroadcastMethodAccess() { @@ -71,17 +62,21 @@ private class NullArgFlowConfig extends DataFlow2::Configuration { override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof NullLiteral } - override predicate isSink(DataFlow::Node sink) { any() } + override predicate isSink(DataFlow::Node sink) { + exists(SendBroadcastMethodAccess ma | sink.asExpr() = ma.getAnArgument()) + } } -private class EmptyArrayArgFlowConfig extends DataFlow2::Configuration { +private class EmptyArrayArgFlowConfig extends DataFlow3::Configuration { EmptyArrayArgFlowConfig() { this = "Flow configuration with an empty array argument" } override predicate isSource(DataFlow::Node src) { src.asExpr().(ArrayCreationExpr).getFirstDimensionSize() = 0 } - override predicate isSink(DataFlow::Node sink) { any() } + override predicate isSink(DataFlow::Node sink) { + exists(SendBroadcastMethodAccess ma | sink.asExpr() = ma.getAnArgument()) + } } /** @@ -110,7 +105,7 @@ predicate isSensitiveBroadcastSink(DataFlow::Node sink) { config.hasFlow(_, DataFlow::exprNode(ma.getArgument(1))) // sendBroadcastWithMultiplePermissions(Intent intent, String[] receiverPermissions) ) or - //Method calls of `sendOrderedBroadcast` whose second argument is always `receiverPermission` + // Method calls of `sendOrderedBroadcast` whose second argument is always `receiverPermission` ma.getMethod().hasName("sendOrderedBroadcast") and ( // sendOrderedBroadcast(Intent intent, String receiverPermission) or sendOrderedBroadcast(Intent intent, String receiverPermission, BroadcastReceiver resultReceiver, Handler scheduler, int initialCode, String initialData, Bundle initialExtras) @@ -123,7 +118,7 @@ predicate isSensitiveBroadcastSink(DataFlow::Node sink) { ma.getNumArgument() = 8 ) or - //Method call of `sendOrderedBroadcastAsUser(Intent intent, UserHandle user, String receiverPermission, BroadcastReceiver resultReceiver, Handler scheduler, int initialCode, String initialData, Bundle initialExtras)` + // Method call of `sendOrderedBroadcastAsUser(Intent intent, UserHandle user, String receiverPermission, BroadcastReceiver resultReceiver, Handler scheduler, int initialCode, String initialData, Bundle initialExtras)` ma.getMethod().hasName("sendOrderedBroadcastAsUser") and exists(NullArgFlowConfig conf | conf.hasFlow(_, DataFlow::exprNode(ma.getArgument(2)))) ) diff --git a/java/ql/src/semmle/code/java/security/SensitiveActions.qll b/java/ql/src/semmle/code/java/security/SensitiveActions.qll index 90380439500..946799672b7 100644 --- a/java/ql/src/semmle/code/java/security/SensitiveActions.qll +++ b/java/ql/src/semmle/code/java/security/SensitiveActions.qll @@ -27,6 +27,14 @@ private string nonSuspicious() { result = "%crypt%" } +/** + * Gets a regular expression for matching common names of variables that indicate the value being held contains sensitive information. + */ +string getCommonSensitiveInfoRegex() { + result = "(?i).*challenge|pass(wd|word|code|phrase)(?!.*question).*" or + result = "(?i).*(token|username|userid|secret).*" +} + /** An expression that might contain sensitive data. */ abstract class SensitiveExpr extends Expr { } diff --git a/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.expected b/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.expected index 346faa17f42..867d03494ed 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.expected @@ -1,12 +1,12 @@ | SensitiveBroadcast.java:13:31:13:36 | intent | SensitiveBroadcast.java:11:34:11:38 | token : String | SensitiveBroadcast.java:13:31:13:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:11:34:11:38 | token | sensitive information | | SensitiveBroadcast.java:13:31:13:36 | intent | SensitiveBroadcast.java:12:41:12:52 | refreshToken : String | SensitiveBroadcast.java:13:31:13:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:12:41:12:52 | refreshToken | sensitive information | -| SensitiveBroadcast.java:25:31:25:36 | intent | SensitiveBroadcast.java:23:33:23:40 | username : String | SensitiveBroadcast.java:25:31:25:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:23:33:23:40 | username | sensitive information | +| SensitiveBroadcast.java:25:31:25:36 | intent | SensitiveBroadcast.java:23:33:23:40 | userName : String | SensitiveBroadcast.java:25:31:25:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:23:33:23:40 | userName | sensitive information | | SensitiveBroadcast.java:25:31:25:36 | intent | SensitiveBroadcast.java:24:32:24:39 | password : String | SensitiveBroadcast.java:25:31:25:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:24:32:24:39 | password | sensitive information | -| SensitiveBroadcast.java:39:31:39:36 | intent | SensitiveBroadcast.java:36:40:36:47 | username : String | SensitiveBroadcast.java:39:31:39:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:36:40:36:47 | username | sensitive information | -| SensitiveBroadcast.java:39:31:39:36 | intent | SensitiveBroadcast.java:37:39:37:46 | password : String | SensitiveBroadcast.java:39:31:39:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:37:39:37:46 | password | sensitive information | -| SensitiveBroadcast.java:89:54:89:59 | intent | SensitiveBroadcast.java:87:33:87:40 | username : String | SensitiveBroadcast.java:89:54:89:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:87:33:87:40 | username | sensitive information | -| SensitiveBroadcast.java:89:54:89:59 | intent | SensitiveBroadcast.java:88:32:88:39 | password : String | SensitiveBroadcast.java:89:54:89:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:88:32:88:39 | password | sensitive information | -| SensitiveBroadcast.java:102:54:102:59 | intent | SensitiveBroadcast.java:99:33:99:40 | username : String | SensitiveBroadcast.java:102:54:102:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:99:33:99:40 | username | sensitive information | -| SensitiveBroadcast.java:102:54:102:59 | intent | SensitiveBroadcast.java:100:32:100:39 | password : String | SensitiveBroadcast.java:102:54:102:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:100:32:100:39 | password | sensitive information | -| SensitiveBroadcast.java:116:54:116:59 | intent | SensitiveBroadcast.java:112:33:112:40 | username : String | SensitiveBroadcast.java:116:54:116:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:112:33:112:40 | username | sensitive information | -| SensitiveBroadcast.java:116:54:116:59 | intent | SensitiveBroadcast.java:113:32:113:39 | password : String | SensitiveBroadcast.java:116:54:116:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:113:32:113:39 | password | sensitive information | +| SensitiveBroadcast.java:37:31:37:36 | intent | SensitiveBroadcast.java:35:41:35:45 | email : String | SensitiveBroadcast.java:37:31:37:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:35:41:35:45 | email | sensitive information | +| SensitiveBroadcast.java:49:31:49:36 | intent | SensitiveBroadcast.java:47:33:47:40 | username : String | SensitiveBroadcast.java:49:31:49:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:47:33:47:40 | username | sensitive information | +| SensitiveBroadcast.java:49:31:49:36 | intent | SensitiveBroadcast.java:48:32:48:39 | password : String | SensitiveBroadcast.java:49:31:49:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:48:32:48:39 | password | sensitive information | +| SensitiveBroadcast.java:95:54:95:59 | intent | SensitiveBroadcast.java:94:35:94:40 | ticket : String | SensitiveBroadcast.java:95:54:95:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:94:35:94:40 | ticket | sensitive information | +| SensitiveBroadcast.java:108:54:108:59 | intent | SensitiveBroadcast.java:105:33:105:40 | username : String | SensitiveBroadcast.java:108:54:108:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:105:33:105:40 | username | sensitive information | +| SensitiveBroadcast.java:108:54:108:59 | intent | SensitiveBroadcast.java:106:32:106:39 | password : String | SensitiveBroadcast.java:108:54:108:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:106:32:106:39 | password | sensitive information | +| SensitiveBroadcast.java:139:54:139:59 | intent | SensitiveBroadcast.java:134:40:134:47 | username : String | SensitiveBroadcast.java:139:54:139:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:134:40:134:47 | username | sensitive information | +| SensitiveBroadcast.java:139:54:139:59 | intent | SensitiveBroadcast.java:135:39:135:46 | password : String | SensitiveBroadcast.java:139:54:139:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:135:39:135:46 | password | sensitive information | diff --git a/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.java b/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.java index 12cec1de601..907fa927a49 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.java +++ b/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.java @@ -4,7 +4,7 @@ import android.os.Bundle; class SensitiveBroadcast { - //Tests broadcast of access token with intent extra. + // BAD - Tests broadcast of access token with intent extra. public void sendBroadcast1(Context context, String token, String refreshToken) { Intent intent = new Intent(); intent.setAction("com.example.custom_action"); @@ -13,34 +13,44 @@ class SensitiveBroadcast { context.sendBroadcast(intent); } - //Tests broadcast of sensitive user information with intent extra. + // BAD - Tests broadcast of sensitive user information with intent extra. public void sendBroadcast2(Context context) { - String username = "test123"; + String userName = "test123"; String password = "abc12345"; Intent intent = new Intent(); intent.setAction("com.example.custom_action"); - intent.putExtra("name", username); + intent.putExtra("name", userName); intent.putExtra("pwd", password); context.sendBroadcast(intent); } - //Tests broadcast of sensitive user information with extra bundle. + // BAD - Tests broadcast of email information with extra bundle. public void sendBroadcast3(Context context) { + String email = "user123@example.com"; + + Intent intent = new Intent(); + intent.setAction("com.example.custom_action"); + Bundle bundle = new Bundle(); + bundle.putCharSequence("email", email); + intent.putExtras(bundle); + context.sendBroadcast(intent); + } + + // BAD - Tests broadcast of sensitive user information with null permission. + public void sendBroadcast4(Context context) { String username = "test123"; String password = "abc12345"; Intent intent = new Intent(); intent.setAction("com.example.custom_action"); - Bundle bundle = new Bundle(); - bundle.putCharSequence("name", username); - bundle.putCharSequence("pwd", password); - intent.putExtras(bundle); - context.sendBroadcast(intent); - } + intent.putExtra("name", username); + intent.putExtra("pwd", password); + context.sendBroadcast(intent, null); + } - //Tests broadcast of sensitive user information with permission using string literal. - public void sendBroadcast4(Context context) { + // GOOD - Tests broadcast of sensitive user information with permission using string literal. + public void sendBroadcast5(Context context) { String username = "test123"; String password = "abc12345"; @@ -51,21 +61,19 @@ class SensitiveBroadcast { context.sendBroadcast(intent, "com.example.user_permission"); } - //Tests broadcast of sensitive user information with permission using string object. - public void sendBroadcast5(Context context) { - String username = "test123"; - String password = "abc12345"; + // GOOD - Tests broadcast of access ticket with permission using string object. + public void sendBroadcast6(Context context) { + String ticket = "Tk9UIFNlY3VyZSBUaWNrZXQ="; Intent intent = new Intent(); intent.setAction("com.example.custom_action"); - intent.putExtra("name", username); - intent.putExtra("pwd", password); + intent.putExtra("ticket", ticket); String perm = "com.example.user_permission"; context.sendBroadcast(intent, perm); } - //Tests broadcast of sensitive user information to a specific application. - public void sendBroadcast6(Context context) { + // GOOD - Tests broadcast of sensitive user information to a specific application. + public void sendBroadcast7(Context context) { String username = "test123"; String password = "abc12345"; @@ -77,20 +85,18 @@ class SensitiveBroadcast { context.sendBroadcast(intent); } - //Tests broadcast of sensitive user information with multiple permissions using direct empty array initialization. - public void sendBroadcast7(Context context) { - String username = "test123"; - String password = "abc12345"; + // BAD - Tests broadcast of access ticket with multiple permissions using direct empty array initialization. + public void sendBroadcast8(Context context) { + String ticket = "Tk9UIFNlY3VyZSBUaWNrZXQ="; Intent intent = new Intent(); intent.setAction("com.example.custom_action"); - intent.putExtra("name", username); - intent.putExtra("pwd", password); + intent.putExtra("ticket", ticket); context.sendBroadcastWithMultiplePermissions(intent, new String[]{}); } - //Tests broadcast of sensitive user information with multiple permissions using empty array initialization through a variable. - public void sendBroadcast8(Context context) { + // BAD - Tests broadcast of sensitive user information with multiple permissions using empty array initialization through a variable. + public void sendBroadcast9(Context context) { String username = "test123"; String password = "abc12345"; @@ -102,22 +108,39 @@ class SensitiveBroadcast { context.sendBroadcastWithMultiplePermissions(intent, perms); } - //Tests broadcast of sensitive user information with multiple permissions using empty array initialization through two variables. - public void sendBroadcast9(Context context) { + // GOOD - Tests broadcast of sensitive user information with multiple permissions. + public void sendBroadcast10(Context context) { String username = "test123"; String password = "abc12345"; Intent intent = new Intent(); intent.setAction("com.example.custom_action"); - intent.putExtra("name", username); - intent.putExtra("pwd", password); + Bundle bundle = new Bundle(); + bundle.putCharSequence("name", username); + bundle.putCharSequence("pwd", password); + intent.putExtras(bundle); + String[] perms = new String[]{"com.example.custom_action", "com.example.custom_action2"}; + context.sendBroadcastWithMultiplePermissions(intent, perms); + } + + // BAD - Tests broadcast of sensitive user information with multiple permissions using empty array initialization through two variables. + public void sendBroadcast11(Context context) { + String username = "test123"; + String password = "abc12345"; + + Intent intent = new Intent(); + intent.setAction("com.example.custom_action"); + Bundle bundle = new Bundle(); + bundle.putCharSequence("name", username); + bundle.putCharSequence("pwd", password); + intent.putExtras(bundle); String[] perms = new String[0]; String[] perms2 = perms; context.sendBroadcastWithMultiplePermissions(intent, perms2); } - //Tests broadcast of sensitive user information with ordered broadcast. - public void sendBroadcast10(Context context) { + // GOOD - Tests broadcast of sensitive user information with ordered broadcast. + public void sendBroadcast12(Context context) { String username = "test123"; String password = "abc12345"; From 908d6599063201bfacc10e795658703538fee41c Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Wed, 28 Oct 2020 20:23:22 +0000 Subject: [PATCH 06/11] Minor updates --- .../Security/CWE/CWE-532/SensitiveInfoLog.ql | 8 +- .../CWE/CWE-927/SensitiveBroadcast.ql | 8 +- .../CWE-927/SensitiveBroadcast.expected | 58 +++++-- .../security/CWE-927/SensitiveBroadcast.java | 42 +++-- .../android/os/BaseBundle.java | 154 ++++++++++++++++++ 5 files changed, 241 insertions(+), 29 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-532/SensitiveInfoLog.ql b/java/ql/src/experimental/Security/CWE/CWE-532/SensitiveInfoLog.ql index b2ec0564e97..4c91523312e 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-532/SensitiveInfoLog.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-532/SensitiveInfoLog.ql @@ -9,21 +9,21 @@ import java import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.security.SensitiveActions import DataFlow import PathGraph /** - * Gets a regular expression for matching names of variables that indicate the value being held is a credential + * Gets a regular expression for matching names of variables that indicate the value being held may contain sensitive information */ private string getACredentialRegex() { - result = "(?i).*challenge|pass(wd|word|code|phrase)(?!.*question).*" or - result = "(?i)(.*username|.*secret|url).*" + result = "(?i)(.*uri|url).*" } /** Variable keeps sensitive information judging by its name * */ class CredentialExpr extends Expr { CredentialExpr() { - exists(Variable v | this = v.getAnAccess() | v.getName().regexpMatch(getACredentialRegex())) + exists(Variable v | this = v.getAnAccess() | v.getName().regexpMatch([getCommonSensitiveInfoRegex(), getACredentialRegex()])) } } diff --git a/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql index 3fd10b5d45b..8d57fe9b069 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql @@ -12,6 +12,7 @@ import semmle.code.java.dataflow.DataFlow3 import semmle.code.java.dataflow.TaintTracking import semmle.code.java.frameworks.android.Intent import semmle.code.java.security.SensitiveActions +import DataFlow::PathGraph /** * Gets regular expression for matching names of Android variables that indicate the value being held contains sensitive information. @@ -23,7 +24,10 @@ private string getAndroidSensitiveInfoRegex() { result = "(?i).*(email|phone|tic */ class PutIntentExtraMethodAccess extends MethodAccess { PutIntentExtraMethodAccess() { - getMethod().getName().regexpMatch("put\\w*Extra(s?)") and + ( + getMethod().getName().matches("put%Extra") or + getMethod().hasName("putExtras") + ) and getMethod().getDeclaringType() instanceof TypeIntent } } @@ -138,7 +142,7 @@ class SensitiveBroadcastConfig extends TaintTracking::Configuration { override predicate isSink(DataFlow::Node sink) { isSensitiveBroadcastSink(sink) } /** - * Holds if there is an additional flow step from `PutIntentExtraMethodAccess` or `PutBundleExtraMethodAccess` to a broadcast intent. + * Holds if there is an additional flow step from `PutIntentExtraMethodAccess` or `PutBundleExtraMethodAccess` that taints the `Intent` or its extras `Bundle`. */ override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { exists(PutIntentExtraMethodAccess pia | diff --git a/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.expected b/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.expected index 867d03494ed..df359555bc0 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.expected @@ -1,12 +1,46 @@ -| SensitiveBroadcast.java:13:31:13:36 | intent | SensitiveBroadcast.java:11:34:11:38 | token : String | SensitiveBroadcast.java:13:31:13:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:11:34:11:38 | token | sensitive information | -| SensitiveBroadcast.java:13:31:13:36 | intent | SensitiveBroadcast.java:12:41:12:52 | refreshToken : String | SensitiveBroadcast.java:13:31:13:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:12:41:12:52 | refreshToken | sensitive information | -| SensitiveBroadcast.java:25:31:25:36 | intent | SensitiveBroadcast.java:23:33:23:40 | userName : String | SensitiveBroadcast.java:25:31:25:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:23:33:23:40 | userName | sensitive information | -| SensitiveBroadcast.java:25:31:25:36 | intent | SensitiveBroadcast.java:24:32:24:39 | password : String | SensitiveBroadcast.java:25:31:25:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:24:32:24:39 | password | sensitive information | -| SensitiveBroadcast.java:37:31:37:36 | intent | SensitiveBroadcast.java:35:41:35:45 | email : String | SensitiveBroadcast.java:37:31:37:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:35:41:35:45 | email | sensitive information | -| SensitiveBroadcast.java:49:31:49:36 | intent | SensitiveBroadcast.java:47:33:47:40 | username : String | SensitiveBroadcast.java:49:31:49:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:47:33:47:40 | username | sensitive information | -| SensitiveBroadcast.java:49:31:49:36 | intent | SensitiveBroadcast.java:48:32:48:39 | password : String | SensitiveBroadcast.java:49:31:49:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:48:32:48:39 | password | sensitive information | -| SensitiveBroadcast.java:95:54:95:59 | intent | SensitiveBroadcast.java:94:35:94:40 | ticket : String | SensitiveBroadcast.java:95:54:95:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:94:35:94:40 | ticket | sensitive information | -| SensitiveBroadcast.java:108:54:108:59 | intent | SensitiveBroadcast.java:105:33:105:40 | username : String | SensitiveBroadcast.java:108:54:108:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:105:33:105:40 | username | sensitive information | -| SensitiveBroadcast.java:108:54:108:59 | intent | SensitiveBroadcast.java:106:32:106:39 | password : String | SensitiveBroadcast.java:108:54:108:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:106:32:106:39 | password | sensitive information | -| SensitiveBroadcast.java:139:54:139:59 | intent | SensitiveBroadcast.java:134:40:134:47 | username : String | SensitiveBroadcast.java:139:54:139:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:134:40:134:47 | username | sensitive information | -| SensitiveBroadcast.java:139:54:139:59 | intent | SensitiveBroadcast.java:135:39:135:46 | password : String | SensitiveBroadcast.java:139:54:139:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:135:39:135:46 | password | sensitive information | +edges +| SensitiveBroadcast.java:12:34:12:38 | token : String | SensitiveBroadcast.java:14:31:14:36 | intent | +| SensitiveBroadcast.java:13:41:13:52 | refreshToken : String | SensitiveBroadcast.java:14:31:14:36 | intent | +| SensitiveBroadcast.java:24:33:24:40 | userName : String | SensitiveBroadcast.java:26:31:26:36 | intent | +| SensitiveBroadcast.java:25:32:25:39 | password : String | SensitiveBroadcast.java:26:31:26:36 | intent | +| SensitiveBroadcast.java:36:35:36:39 | email : String | SensitiveBroadcast.java:38:31:38:36 | intent | +| SensitiveBroadcast.java:49:22:49:29 | username : String | SensitiveBroadcast.java:52:31:52:36 | intent | +| SensitiveBroadcast.java:50:22:50:29 | password : String | SensitiveBroadcast.java:52:31:52:36 | intent | +| SensitiveBroadcast.java:97:35:97:40 | ticket : String | SensitiveBroadcast.java:98:54:98:59 | intent | +| SensitiveBroadcast.java:108:33:108:40 | username : String | SensitiveBroadcast.java:111:54:111:59 | intent | +| SensitiveBroadcast.java:109:32:109:39 | password : String | SensitiveBroadcast.java:111:54:111:59 | intent | +| SensitiveBroadcast.java:135:34:135:41 | username : String | SensitiveBroadcast.java:140:54:140:59 | intent | +| SensitiveBroadcast.java:136:33:136:40 | password : String | SensitiveBroadcast.java:140:54:140:59 | intent | +nodes +| SensitiveBroadcast.java:12:34:12:38 | token : String | semmle.label | token : String | +| SensitiveBroadcast.java:13:41:13:52 | refreshToken : String | semmle.label | refreshToken : String | +| SensitiveBroadcast.java:14:31:14:36 | intent | semmle.label | intent | +| SensitiveBroadcast.java:24:33:24:40 | userName : String | semmle.label | userName : String | +| SensitiveBroadcast.java:25:32:25:39 | password : String | semmle.label | password : String | +| SensitiveBroadcast.java:26:31:26:36 | intent | semmle.label | intent | +| SensitiveBroadcast.java:36:35:36:39 | email : String | semmle.label | email : String | +| SensitiveBroadcast.java:38:31:38:36 | intent | semmle.label | intent | +| SensitiveBroadcast.java:49:22:49:29 | username : String | semmle.label | username : String | +| SensitiveBroadcast.java:50:22:50:29 | password : String | semmle.label | password : String | +| SensitiveBroadcast.java:52:31:52:36 | intent | semmle.label | intent | +| SensitiveBroadcast.java:97:35:97:40 | ticket : String | semmle.label | ticket : String | +| SensitiveBroadcast.java:98:54:98:59 | intent | semmle.label | intent | +| SensitiveBroadcast.java:108:33:108:40 | username : String | semmle.label | username : String | +| SensitiveBroadcast.java:109:32:109:39 | password : String | semmle.label | password : String | +| SensitiveBroadcast.java:111:54:111:59 | intent | semmle.label | intent | +| SensitiveBroadcast.java:135:34:135:41 | username : String | semmle.label | username : String | +| SensitiveBroadcast.java:136:33:136:40 | password : String | semmle.label | password : String | +| SensitiveBroadcast.java:140:54:140:59 | intent | semmle.label | intent | +#select +| SensitiveBroadcast.java:14:31:14:36 | intent | SensitiveBroadcast.java:12:34:12:38 | token : String | SensitiveBroadcast.java:14:31:14:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:12:34:12:38 | token | sensitive information | +| SensitiveBroadcast.java:14:31:14:36 | intent | SensitiveBroadcast.java:13:41:13:52 | refreshToken : String | SensitiveBroadcast.java:14:31:14:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:13:41:13:52 | refreshToken | sensitive information | +| SensitiveBroadcast.java:26:31:26:36 | intent | SensitiveBroadcast.java:24:33:24:40 | userName : String | SensitiveBroadcast.java:26:31:26:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:24:33:24:40 | userName | sensitive information | +| SensitiveBroadcast.java:26:31:26:36 | intent | SensitiveBroadcast.java:25:32:25:39 | password : String | SensitiveBroadcast.java:26:31:26:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:25:32:25:39 | password | sensitive information | +| SensitiveBroadcast.java:38:31:38:36 | intent | SensitiveBroadcast.java:36:35:36:39 | email : String | SensitiveBroadcast.java:38:31:38:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:36:35:36:39 | email | sensitive information | +| SensitiveBroadcast.java:52:31:52:36 | intent | SensitiveBroadcast.java:49:22:49:29 | username : String | SensitiveBroadcast.java:52:31:52:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:49:22:49:29 | username | sensitive information | +| SensitiveBroadcast.java:52:31:52:36 | intent | SensitiveBroadcast.java:50:22:50:29 | password : String | SensitiveBroadcast.java:52:31:52:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:50:22:50:29 | password | sensitive information | +| SensitiveBroadcast.java:98:54:98:59 | intent | SensitiveBroadcast.java:97:35:97:40 | ticket : String | SensitiveBroadcast.java:98:54:98:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:97:35:97:40 | ticket | sensitive information | +| SensitiveBroadcast.java:111:54:111:59 | intent | SensitiveBroadcast.java:108:33:108:40 | username : String | SensitiveBroadcast.java:111:54:111:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:108:33:108:40 | username | sensitive information | +| SensitiveBroadcast.java:111:54:111:59 | intent | SensitiveBroadcast.java:109:32:109:39 | password : String | SensitiveBroadcast.java:111:54:111:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:109:32:109:39 | password | sensitive information | +| SensitiveBroadcast.java:140:54:140:59 | intent | SensitiveBroadcast.java:135:34:135:41 | username : String | SensitiveBroadcast.java:140:54:140:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:135:34:135:41 | username | sensitive information | +| SensitiveBroadcast.java:140:54:140:59 | intent | SensitiveBroadcast.java:136:33:136:40 | password : String | SensitiveBroadcast.java:140:54:140:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:136:33:136:40 | password | sensitive information | diff --git a/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.java b/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.java index 907fa927a49..ef58730d730 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.java +++ b/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.java @@ -1,6 +1,7 @@ import android.content.Context; import android.content.Intent; import android.os.Bundle; +import java.util.ArrayList; class SensitiveBroadcast { @@ -32,7 +33,7 @@ class SensitiveBroadcast { Intent intent = new Intent(); intent.setAction("com.example.custom_action"); Bundle bundle = new Bundle(); - bundle.putCharSequence("email", email); + bundle.putString("email", email); intent.putExtras(bundle); context.sendBroadcast(intent); } @@ -44,8 +45,10 @@ class SensitiveBroadcast { Intent intent = new Intent(); intent.setAction("com.example.custom_action"); - intent.putExtra("name", username); - intent.putExtra("pwd", password); + ArrayList userinfo = new ArrayList(); + userinfo.add(username); + userinfo.add(password); + intent.putStringArrayListExtra("userinfo", userinfo); context.sendBroadcast(intent, null); } @@ -115,15 +118,13 @@ class SensitiveBroadcast { Intent intent = new Intent(); intent.setAction("com.example.custom_action"); - Bundle bundle = new Bundle(); - bundle.putCharSequence("name", username); - bundle.putCharSequence("pwd", password); - intent.putExtras(bundle); + intent.putExtra("name", username); + intent.putExtra("pwd", password); String[] perms = new String[]{"com.example.custom_action", "com.example.custom_action2"}; context.sendBroadcastWithMultiplePermissions(intent, perms); } - // BAD - Tests broadcast of sensitive user information with multiple permissions using empty array initialization through two variables. + // BAD - Tests broadcast of sensitive user information with multiple permissions using empty array initialization through two variables and `intent.putExtras(bundle)`. public void sendBroadcast11(Context context) { String username = "test123"; String password = "abc12345"; @@ -131,19 +132,38 @@ class SensitiveBroadcast { Intent intent = new Intent(); intent.setAction("com.example.custom_action"); Bundle bundle = new Bundle(); - bundle.putCharSequence("name", username); - bundle.putCharSequence("pwd", password); + bundle.putString("name", username); + bundle.putString("pwd", password); intent.putExtras(bundle); String[] perms = new String[0]; String[] perms2 = perms; context.sendBroadcastWithMultiplePermissions(intent, perms2); } - // GOOD - Tests broadcast of sensitive user information with ordered broadcast. + /** + * BAD - Tests broadcast of sensitive user information with multiple permissions using empty array initialization through two variables and `intent.getExtras().putString()`. + * Note this case of `getExtras().putString(...)` is not yet detected thus is beyond what the query is capable of. + */ public void sendBroadcast12(Context context) { String username = "test123"; String password = "abc12345"; + Intent intent = new Intent(); + intent.setAction("com.example.custom_action"); + Bundle bundle = new Bundle(); + intent.putExtras(bundle); + intent.getExtras().putString("name", username); + intent.getExtras().putString("pwd", password); + String[] perms = new String[0]; + String[] perms2 = perms; + context.sendBroadcastWithMultiplePermissions(intent, perms2); + } + + // GOOD - Tests broadcast of sensitive user information with ordered broadcast. + public void sendBroadcast13(Context context) { + String username = "test123"; + String password = "abc12345"; + Intent intent = new Intent(); intent.setAction("com.example.custom_action"); intent.putExtra("name", username); diff --git a/java/ql/test/stubs/google-android-9.0.0/android/os/BaseBundle.java b/java/ql/test/stubs/google-android-9.0.0/android/os/BaseBundle.java index 617ec97ae8c..4b85ae8e67a 100644 --- a/java/ql/test/stubs/google-android-9.0.0/android/os/BaseBundle.java +++ b/java/ql/test/stubs/google-android-9.0.0/android/os/BaseBundle.java @@ -129,6 +129,160 @@ public class BaseBundle { return false; } + /** + * Returns true if the given key is contained in the mapping + * of this Bundle. + * + * @param key a String key + * @return true if the key is part of the mapping, false otherwise + */ + public boolean containsKey(String key) { + return false; + } + + /** + * Returns the entry with the given key as an object. + * + * @param key a String key + * @return an Object, or null + */ + public Object get(String key) { + return null; + } + + /** + * Removes any entry with the given key from the mapping of this Bundle. + * + * @param key a String key + */ + public void remove(String key) { + } + + /** {@hide} */ + public void putObject(String key, Object value) { + } + + /** + * Inserts a Boolean value into the mapping of this Bundle, replacing + * any existing value for the given key. Either key or value may be null. + * + * @param key a String, or null + * @param value a boolean + */ + public void putBoolean(String key, boolean value) { + } + + /** + * Inserts a byte value into the mapping of this Bundle, replacing + * any existing value for the given key. + * + * @param key a String, or null + * @param value a byte + */ + void putByte(String key, byte value) { + } + + /** + * Inserts a char value into the mapping of this Bundle, replacing + * any existing value for the given key. + * + * @param key a String, or null + * @param value a char + */ + void putChar(String key, char value) { + } + + /** + * Inserts a short value into the mapping of this Bundle, replacing + * any existing value for the given key. + * + * @param key a String, or null + * @param value a short + */ + void putShort(String key, short value) { + } + + /** + * Inserts an int value into the mapping of this Bundle, replacing + * any existing value for the given key. + * + * @param key a String, or null + * @param value an int + */ + public void putInt(String key, int value) { + } + + /** + * Inserts a long value into the mapping of this Bundle, replacing + * any existing value for the given key. + * + * @param key a String, or null + * @param value a long + */ + public void putLong(String key, long value) { + } + + /** + * Inserts a float value into the mapping of this Bundle, replacing + * any existing value for the given key. + * + * @param key a String, or null + * @param value a float + */ + void putFloat(String key, float value) { + } + + /** + * Inserts a double value into the mapping of this Bundle, replacing + * any existing value for the given key. + * + * @param key a String, or null + * @param value a double + */ + public void putDouble(String key, double value) { + } + + /** + * Inserts a String value into the mapping of this Bundle, replacing + * any existing value for the given key. Either key or value may be null. + * + * @param key a String, or null + * @param value a String, or null + */ + public void putString(String key, String value) { + } + + /** + * Inserts a CharSequence value into the mapping of this Bundle, replacing + * any existing value for the given key. Either key or value may be null. + * + * @param key a String, or null + * @param value a CharSequence, or null + */ + void putCharSequence(String key, CharSequence value) { + } + + /** + * Inserts an ArrayList value into the mapping of this Bundle, replacing + * any existing value for the given key. Either key or value may be null. + * + * @param key a String, or null + * @param value an ArrayList object, or null + */ + void putIntegerArrayList(String key, ArrayList value) { + } + + /** + * Inserts an ArrayList value into the mapping of this Bundle, replacing + * any existing value for the given key. Either key or value may be null. + * + * @param key a String, or null + * @param value an ArrayList object, or null + */ + void putStringArrayList(String key, ArrayList value) { + } + + /** * Inserts an ArrayList value into the mapping of this Bundle, * replacing any existing value for the given key. Either key or value may be From 90d11812becd6fd14cb9d6fcd4f4fe53c1af9a95 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Thu, 29 Oct 2020 13:04:15 +0000 Subject: [PATCH 07/11] Update the regex to be the original one --- .../src/experimental/Security/CWE/CWE-532/SensitiveInfoLog.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-532/SensitiveInfoLog.ql b/java/ql/src/experimental/Security/CWE/CWE-532/SensitiveInfoLog.ql index 4c91523312e..0e82b9581f0 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-532/SensitiveInfoLog.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-532/SensitiveInfoLog.ql @@ -17,7 +17,7 @@ import PathGraph * Gets a regular expression for matching names of variables that indicate the value being held may contain sensitive information */ private string getACredentialRegex() { - result = "(?i)(.*uri|url).*" + result = "(?i)(url).*" } /** Variable keeps sensitive information judging by its name * */ From 5a6339c1afa41bd843179da5a9e5100314caf1c2 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Thu, 29 Oct 2020 15:46:05 +0000 Subject: [PATCH 08/11] Remove userid from the regex --- .../Security/CWE/CWE-532/SensitiveInfoLog.ql | 2 +- .../code/java/security/SensitiveActions.qll | 2 +- .../CWE-927/SensitiveBroadcast.expected | 24 +++++-------------- 3 files changed, 8 insertions(+), 20 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-532/SensitiveInfoLog.ql b/java/ql/src/experimental/Security/CWE/CWE-532/SensitiveInfoLog.ql index 0e82b9581f0..294b384e998 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-532/SensitiveInfoLog.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-532/SensitiveInfoLog.ql @@ -17,7 +17,7 @@ import PathGraph * Gets a regular expression for matching names of variables that indicate the value being held may contain sensitive information */ private string getACredentialRegex() { - result = "(?i)(url).*" + result = "(?i)(.*username|url).*" } /** Variable keeps sensitive information judging by its name * */ diff --git a/java/ql/src/semmle/code/java/security/SensitiveActions.qll b/java/ql/src/semmle/code/java/security/SensitiveActions.qll index 946799672b7..88ae44b4556 100644 --- a/java/ql/src/semmle/code/java/security/SensitiveActions.qll +++ b/java/ql/src/semmle/code/java/security/SensitiveActions.qll @@ -32,7 +32,7 @@ private string nonSuspicious() { */ string getCommonSensitiveInfoRegex() { result = "(?i).*challenge|pass(wd|word|code|phrase)(?!.*question).*" or - result = "(?i).*(token|username|userid|secret).*" + result = "(?i).*(token|secret).*" } /** An expression that might contain sensitive data. */ diff --git a/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.expected b/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.expected index df359555bc0..2b1384b845b 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.expected @@ -1,46 +1,34 @@ edges | SensitiveBroadcast.java:12:34:12:38 | token : String | SensitiveBroadcast.java:14:31:14:36 | intent | | SensitiveBroadcast.java:13:41:13:52 | refreshToken : String | SensitiveBroadcast.java:14:31:14:36 | intent | -| SensitiveBroadcast.java:24:33:24:40 | userName : String | SensitiveBroadcast.java:26:31:26:36 | intent | | SensitiveBroadcast.java:25:32:25:39 | password : String | SensitiveBroadcast.java:26:31:26:36 | intent | | SensitiveBroadcast.java:36:35:36:39 | email : String | SensitiveBroadcast.java:38:31:38:36 | intent | -| SensitiveBroadcast.java:49:22:49:29 | username : String | SensitiveBroadcast.java:52:31:52:36 | intent | | SensitiveBroadcast.java:50:22:50:29 | password : String | SensitiveBroadcast.java:52:31:52:36 | intent | | SensitiveBroadcast.java:97:35:97:40 | ticket : String | SensitiveBroadcast.java:98:54:98:59 | intent | -| SensitiveBroadcast.java:108:33:108:40 | username : String | SensitiveBroadcast.java:111:54:111:59 | intent | -| SensitiveBroadcast.java:109:32:109:39 | password : String | SensitiveBroadcast.java:111:54:111:59 | intent | -| SensitiveBroadcast.java:135:34:135:41 | username : String | SensitiveBroadcast.java:140:54:140:59 | intent | -| SensitiveBroadcast.java:136:33:136:40 | password : String | SensitiveBroadcast.java:140:54:140:59 | intent | +| SensitiveBroadcast.java:109:32:109:39 | passcode : String | SensitiveBroadcast.java:111:54:111:59 | intent | +| SensitiveBroadcast.java:136:33:136:38 | passwd : String | SensitiveBroadcast.java:140:54:140:59 | intent | nodes | SensitiveBroadcast.java:12:34:12:38 | token : String | semmle.label | token : String | | SensitiveBroadcast.java:13:41:13:52 | refreshToken : String | semmle.label | refreshToken : String | | SensitiveBroadcast.java:14:31:14:36 | intent | semmle.label | intent | -| SensitiveBroadcast.java:24:33:24:40 | userName : String | semmle.label | userName : String | | SensitiveBroadcast.java:25:32:25:39 | password : String | semmle.label | password : String | | SensitiveBroadcast.java:26:31:26:36 | intent | semmle.label | intent | | SensitiveBroadcast.java:36:35:36:39 | email : String | semmle.label | email : String | | SensitiveBroadcast.java:38:31:38:36 | intent | semmle.label | intent | -| SensitiveBroadcast.java:49:22:49:29 | username : String | semmle.label | username : String | | SensitiveBroadcast.java:50:22:50:29 | password : String | semmle.label | password : String | | SensitiveBroadcast.java:52:31:52:36 | intent | semmle.label | intent | | SensitiveBroadcast.java:97:35:97:40 | ticket : String | semmle.label | ticket : String | | SensitiveBroadcast.java:98:54:98:59 | intent | semmle.label | intent | -| SensitiveBroadcast.java:108:33:108:40 | username : String | semmle.label | username : String | -| SensitiveBroadcast.java:109:32:109:39 | password : String | semmle.label | password : String | +| SensitiveBroadcast.java:109:32:109:39 | passcode : String | semmle.label | passcode : String | | SensitiveBroadcast.java:111:54:111:59 | intent | semmle.label | intent | -| SensitiveBroadcast.java:135:34:135:41 | username : String | semmle.label | username : String | -| SensitiveBroadcast.java:136:33:136:40 | password : String | semmle.label | password : String | +| SensitiveBroadcast.java:136:33:136:38 | passwd : String | semmle.label | passwd : String | | SensitiveBroadcast.java:140:54:140:59 | intent | semmle.label | intent | #select | SensitiveBroadcast.java:14:31:14:36 | intent | SensitiveBroadcast.java:12:34:12:38 | token : String | SensitiveBroadcast.java:14:31:14:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:12:34:12:38 | token | sensitive information | | SensitiveBroadcast.java:14:31:14:36 | intent | SensitiveBroadcast.java:13:41:13:52 | refreshToken : String | SensitiveBroadcast.java:14:31:14:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:13:41:13:52 | refreshToken | sensitive information | -| SensitiveBroadcast.java:26:31:26:36 | intent | SensitiveBroadcast.java:24:33:24:40 | userName : String | SensitiveBroadcast.java:26:31:26:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:24:33:24:40 | userName | sensitive information | | SensitiveBroadcast.java:26:31:26:36 | intent | SensitiveBroadcast.java:25:32:25:39 | password : String | SensitiveBroadcast.java:26:31:26:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:25:32:25:39 | password | sensitive information | | SensitiveBroadcast.java:38:31:38:36 | intent | SensitiveBroadcast.java:36:35:36:39 | email : String | SensitiveBroadcast.java:38:31:38:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:36:35:36:39 | email | sensitive information | -| SensitiveBroadcast.java:52:31:52:36 | intent | SensitiveBroadcast.java:49:22:49:29 | username : String | SensitiveBroadcast.java:52:31:52:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:49:22:49:29 | username | sensitive information | | SensitiveBroadcast.java:52:31:52:36 | intent | SensitiveBroadcast.java:50:22:50:29 | password : String | SensitiveBroadcast.java:52:31:52:36 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:50:22:50:29 | password | sensitive information | | SensitiveBroadcast.java:98:54:98:59 | intent | SensitiveBroadcast.java:97:35:97:40 | ticket : String | SensitiveBroadcast.java:98:54:98:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:97:35:97:40 | ticket | sensitive information | -| SensitiveBroadcast.java:111:54:111:59 | intent | SensitiveBroadcast.java:108:33:108:40 | username : String | SensitiveBroadcast.java:111:54:111:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:108:33:108:40 | username | sensitive information | -| SensitiveBroadcast.java:111:54:111:59 | intent | SensitiveBroadcast.java:109:32:109:39 | password : String | SensitiveBroadcast.java:111:54:111:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:109:32:109:39 | password | sensitive information | -| SensitiveBroadcast.java:140:54:140:59 | intent | SensitiveBroadcast.java:135:34:135:41 | username : String | SensitiveBroadcast.java:140:54:140:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:135:34:135:41 | username | sensitive information | -| SensitiveBroadcast.java:140:54:140:59 | intent | SensitiveBroadcast.java:136:33:136:40 | password : String | SensitiveBroadcast.java:140:54:140:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:136:33:136:40 | password | sensitive information | +| SensitiveBroadcast.java:111:54:111:59 | intent | SensitiveBroadcast.java:109:32:109:39 | passcode : String | SensitiveBroadcast.java:111:54:111:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:109:32:109:39 | passcode | sensitive information | +| SensitiveBroadcast.java:140:54:140:59 | intent | SensitiveBroadcast.java:136:33:136:38 | passwd : String | SensitiveBroadcast.java:140:54:140:59 | intent | Sending $@ to broadcast. | SensitiveBroadcast.java:136:33:136:38 | passwd | sensitive information | From c89ebeeb5e1f78910c68f9a859fbf8b9035acae9 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Sun, 1 Nov 2020 00:39:00 +0000 Subject: [PATCH 09/11] Text changes --- .../security/CWE-927/SensitiveBroadcast.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.java b/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.java index ef58730d730..0529cf3c421 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.java +++ b/java/ql/test/experimental/query-tests/security/CWE-927/SensitiveBroadcast.java @@ -101,18 +101,18 @@ class SensitiveBroadcast { // BAD - Tests broadcast of sensitive user information with multiple permissions using empty array initialization through a variable. public void sendBroadcast9(Context context) { String username = "test123"; - String password = "abc12345"; + String passcode = "abc12345"; Intent intent = new Intent(); intent.setAction("com.example.custom_action"); intent.putExtra("name", username); - intent.putExtra("pwd", password); + intent.putExtra("pwd", passcode); String[] perms = new String[0]; context.sendBroadcastWithMultiplePermissions(intent, perms); } - // GOOD - Tests broadcast of sensitive user information with multiple permissions. - public void sendBroadcast10(Context context) { + // GOOD - Tests broadcast of sensitive user information with multiple permissions. + public void sendBroadcast10(Context context) { String username = "test123"; String password = "abc12345"; @@ -127,13 +127,13 @@ class SensitiveBroadcast { // BAD - Tests broadcast of sensitive user information with multiple permissions using empty array initialization through two variables and `intent.putExtras(bundle)`. public void sendBroadcast11(Context context) { String username = "test123"; - String password = "abc12345"; + String passwd = "abc12345"; Intent intent = new Intent(); intent.setAction("com.example.custom_action"); Bundle bundle = new Bundle(); bundle.putString("name", username); - bundle.putString("pwd", password); + bundle.putString("pwd", passwd); intent.putExtras(bundle); String[] perms = new String[0]; String[] perms2 = perms; From f8fd2ea8215f43f1f402276f36f2913885bdcb42 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Tue, 3 Nov 2020 12:23:40 +0000 Subject: [PATCH 10/11] Add qldoc and autoformat query --- .../Security/CWE/CWE-532/SensitiveInfoLog.ql | 8 +-- .../CWE/CWE-927/SensitiveBroadcast.qhelp | 63 +++++++++++++------ .../CWE/CWE-927/SensitiveBroadcast.ql | 2 +- 3 files changed, 49 insertions(+), 24 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-532/SensitiveInfoLog.ql b/java/ql/src/experimental/Security/CWE/CWE-532/SensitiveInfoLog.ql index 294b384e998..15690b7c32b 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-532/SensitiveInfoLog.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-532/SensitiveInfoLog.ql @@ -16,14 +16,14 @@ import PathGraph /** * Gets a regular expression for matching names of variables that indicate the value being held may contain sensitive information */ -private string getACredentialRegex() { - result = "(?i)(.*username|url).*" -} +private string getACredentialRegex() { result = "(?i)(.*username|url).*" } /** Variable keeps sensitive information judging by its name * */ class CredentialExpr extends Expr { CredentialExpr() { - exists(Variable v | this = v.getAnAccess() | v.getName().regexpMatch([getCommonSensitiveInfoRegex(), getACredentialRegex()])) + exists(Variable v | this = v.getAnAccess() | + v.getName().regexpMatch([getCommonSensitiveInfoRegex(), getACredentialRegex()]) + ) } } diff --git a/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.qhelp b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.qhelp index c1f44a4abb0..2bd7a2d8db9 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.qhelp @@ -1,25 +1,50 @@ - + - -

    Broadcast intents in an Android application are visible to all applications installed on the same mobile device, exposing all sensitive information they contain.

    -

    Broadcasts are vulnerable to passive eavesdropping or active denial of service attacks when an intent is broadcast without specifying any receiver permission or receiver application.

    -     + +

    Broadcast intents in an Android application are visible to all applications installed on the same mobile device, exposing all sensitive information they contain.

    +

    Broadcasts are vulnerable to passive eavesdropping or active denial of service attacks when an intent is broadcast without specifying any receiver permission or receiver application.

    +     - -

    Specify a receiver permission or application when broadcasting intents, or switch to LocalBroadcastManager or the latest LiveData library.

    -     + +

    + Specify a receiver permission or application when broadcasting intents, or switch to + LocalBroadcastManager + or the latest + LiveData + library. +

    +     - -

    The following example shows two ways of broadcasting intents. In the 'BAD' case, no "receiver permission" is specified. In the 'GOOD' case, "receiver permission" or "receiver application" is specified.

    - -     + +

    The following example shows two ways of broadcasting intents. In the 'BAD' case, no "receiver permission" is specified. In the 'GOOD' case, "receiver permission" or "receiver application" is specified.

    + +     - -
  • -CWE-927: Use of Implicit Intent for Sensitive Communication -
    • -     + +
  • + CWE: + CWE-927: Use of Implicit Intent for Sensitive Communication +
    • +
  • + Android Developers: + Security considerations and best practices for sending and receiving broadcasts +
    • +
  • + sonarsource: + Broadcasting intents is security-sensitive +
    • +
  • + Android Developer Fundamentals: + Restricting broadcasts +
    • +
  • + Carnegie Mellon University: + DRD03-J. Do not broadcast sensitive information using an implicit intent +
    • +
  • + Android Developers: + Android LiveData Overview +
    • +     \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql index 8d57fe9b069..785a0f5c91c 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.ql @@ -52,7 +52,7 @@ class SensitiveInfoExpr extends Expr { } /** - * A method access of the `context.sendBroadcast` family. + * A method access of the `Context.sendBroadcast` family. */ class SendBroadcastMethodAccess extends MethodAccess { SendBroadcastMethodAccess() { From 26495225e027a970192fdba004ed8c6b9bdc6706 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Wed, 4 Nov 2020 10:05:55 +0100 Subject: [PATCH 11/11] Update java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.qhelp Co-authored-by: Marcono1234 --- .../Security/CWE/CWE-927/SensitiveBroadcast.qhelp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.qhelp b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.qhelp index 2bd7a2d8db9..0879d01e295 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-927/SensitiveBroadcast.qhelp @@ -31,7 +31,7 @@ Security considerations and best practices for sending and receiving broadcasts
  • - sonarsource: + SonarSource: Broadcasting intents is security-sensitive
  • @@ -47,4 +47,4 @@ Android LiveData Overview
    • - \ No newline at end of file +