Enhance the query to use sanitizer and null/empty array flow

2026-05-02 12:15:17 +02:00 · 2020-10-25 15:33:09 +00:00
parent 478771ccc5
commit d9c140dc6c
5 changed files with 166 additions and 65 deletions
--- 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);
+    }
 }
--- 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 @@
 <qhelp>

 <overview>
-<p>Broadcasted intents in an Android application are visible to all applications installed on the same mobile device, exposing all sensitive information they contain.</p>
-<p>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.</p>
+<p>Broadcast intents in an Android application are visible to all applications installed on the same mobile device, exposing all sensitive information they contain.</p>
+<p>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.</p>
 </overview>

 <recommendation>
-<p>Specify receiver permission or specify receiver application in broadcasted intents, or switch to <code>LocalBroadcastManager</code> or the latest <code>LiveData</code> library.</p>
+<p>Specify a receiver permission or application when broadcasting intents, or switch to <code>LocalBroadcastManager</code> or the latest <code>LiveData</code> library.</p>
 </recommendation>

 <example>
-<p>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.</p>
+<p>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.</p>
 <sample src="SensitiveBroadcast.java" />
 </example>

--- 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()
    )
  }
 }
--- 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 |
--- 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");
+    }        
 }