Java: Promote android sensitive broadcast query by joefarebrother · Pull Request #6599 · github/codeql · GitHub

joefarebrother · 2021-09-03T13:02:38Z

This PR promotes the query contributed by #4512 from experimental.

Changes:

Moved everything from experimental and moved the main query logic to SensitiveBroadcastQuery.qll
Refactored the tests to use inline expectations
Replaced the extra cnfigurations with local flow
Added additional models to methods of Intent and Bundle
Allowed arbitrary read steps at the sink
Considered the (deprecated) sendStickyBroadcast methods as additional sinks
Minor qhelp changes

Considerations:

~~A similar vulnerability is in sending sensitive data via methods like startActivity and startService. Should sinks for those be part of this same query?~~
~~Should the additional models be a separate PR?~~

github-actions · 2021-09-03T13:04:46Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged. The following differences were found:

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    Android,``android.*``,18,34,70,,,3,67,,,
+    Android,``android.*``,18,136,70,,,3,67,,,
-    Totals,,84,3495,398,13,6,6,107,33,1,66
+    Totals,,84,3597,398,13,6,6,107,33,1,66

Changes to framework-coverage-java.csv:

- android.content,8,,4,,,,,,,,,,,,,8,,,,,,4,
+ android.content,8,,44,,,,,,,,,,,,,8,,,,,,4,40
+ android.os,,,62,,,,,,,,,,,,,,,,,,,,62

smowton · 2021-09-13T12:55:42Z

java/ql/lib/semmle/code/java/frameworks/android/Intent.qll

+        "android.os;Bundle;true;putSizeF;;;Argument[0];MapKey of Argument[-1];value",
+        "android.os;Bundle;true;putSparseParcelableArray;;;Argument[1];MapValue of Argument[-1];value",
+        "android.os;Bundle;true;putStringArrayList;;;Argument[1];MapValue of Argument[-1];value",
+        "android.content;Intent;true;getExtras;();;SyntheticField[android.content.Intent.extras] of Argument[-1];ReturnValue;value",


Missing fluent mutators: addCategory, addFlags.

atorralba

Added some inline comments. I still need to review the CSV models in detail.

java/ql/src/semmle/code/java/security/AndroidSensitiveBroadcastQuery.qll

atorralba · 2021-09-13T13:09:00Z

A similar vulnerability is in sending sensitive data via methods like startActivity and startService. Should sinks for those be part of this same query?

@joefarebrother IMHO yes, it would make sense, because you can send implicit (and thus, interceptable) Intents with those methods, although the exploitation is a little harder because the user needs to select the malicious app in a dialog for the Intent to reach it.

The query would need to identify implicit Intents (i.e. explicit component not set, just an action). I think some code from this PR could be reused for that.

joefarebrother · 2021-09-13T13:51:45Z

The query would need to identify implicit Intents (i.e. explicit component not set, just an action).

Do the existing sanitizers for setCompoent and similar methods suffice for that?

atorralba · 2021-09-13T13:59:31Z

The query would need to identify implicit Intents (i.e. explicit component not set, just an action).

Do the existing sanitizers for setCompoent and similar methods suffice for that?

Probably, now that you mention it. I was thinking that we could reuse some code since we're doing similar things, but the sanitizer approach is simpler. Adding the new sinks should be easier than I initially thought, then :)

java/ql/src/semmle/code/java/security/AndroidSensitiveBroadcastQuery.qll

java/ql/test/query-tests/security/CWE-927/SensitiveBroadcast.ql

atorralba

Added some more comments regarding CSV models. This covers the Intent extras pretty nicely, and the rest of the Intent methods (like get/setPackage) are or can be handled in other PRs like #6397.

Also, this will need a doc review at some point.

java/ql/lib/semmle/code/java/frameworks/android/Intent.qll

github-actions · 2021-09-14T16:57:22Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged. The following differences were found:

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    Android,``android.*``,18,34,70,,,3,67,,,
+    Android,``android.*``,18,148,70,,,3,67,,,
-    Totals,,116,3554,402,13,6,10,107,33,1,66
+    Totals,,116,3668,402,13,6,10,107,33,1,66

Changes to framework-coverage-java.csv:

- android.content,8,,4,,,,,,,,,,,,,8,,,,,,4,
+ android.content,8,,44,,,,,,,,,,,,,8,,,,,,4,40
+ android.os,,,74,,,,,,,,,,,,,,,,,,,2,72

smowton · 2021-09-16T10:27:48Z

@joefarebrother this has conflicts

java/ql/src/semmle/code/java/security/AndroidSensitiveBroadcastQuery.qll

smowton · 2021-09-16T10:37:27Z

java/ql/src/semmle/code/java/security/AndroidSensitiveBroadcastQuery.qll

+  }
+}
+
+private predicate isNullArg(Expr ex) {


Bad name (this indicates may-be-null, not is-null).

Also simplify this and isEmptyArrayArg using DataFlow::localExprFlow

smowton · 2021-09-16T10:39:17Z

java/ql/src/semmle/code/java/security/AndroidSensitiveBroadcastQuery.qll

+}
+
+/**
+ * Holds if a `sendBroadcast` call doesn't specify receiver permission.


Suggested change

* Holds if a `sendBroadcast` call doesn't specify receiver permission.

* Holds if a `sendBroadcast` call `sendBroadcastCall` doesn't specify receiver permission.

smowton · 2021-09-16T10:39:26Z

java/ql/src/semmle/code/java/security/AndroidSensitiveBroadcastQuery.qll

+/**
+ * Holds if a `sendBroadcast` call doesn't specify receiver permission.
+ */
+private predicate isSensitiveBroadcastSink(DataFlow::Node sink) {


Suggested change

private predicate isSensitiveBroadcastSink(DataFlow::Node sink) {

private predicate isSensitiveBroadcastSink(DataFlow::Node sendBroadcastCall) {

smowton · 2021-09-16T10:41:33Z

java/ql/src/semmle/code/java/security/AndroidSensitiveBroadcastQuery.qll

+  override predicate isSanitizer(DataFlow::Node node) {
+    exists(MethodAccess setReceiverMa |
+      setReceiverMa.getMethod().hasName(["setPackage", "setClass", "setClassName", "setComponent"]) and
+      setReceiverMa.getQualifier().(VarAccess).getVariable().getAnAccess() = node.asExpr()


Use localFlow instead of specifically one layer of access? Or (check by experiment) this might work just sanitising the qualifier due to use-use dataflow allowing sanitisers like this to interrupt dataflow paths to subsequent uses

(For example, in l = in; l.append("a"); l.append("b"), in Java the dataflow edges go in -> l, defn of l -> use of l in l.append("a"), use of l in l.append("a") -> use of l in l.append("b"), rather than both uses flowing from the definition as you might suppose)

java/ql/lib/semmle/code/java/frameworks/android/Intent.qll

smowton · 2021-09-16T10:56:22Z

java/ql/lib/semmle/code/java/frameworks/android/Intent.qll

+        "android.os;Bundle;true;putSizeF;;;Argument[0];MapKey of Argument[-1];value",
+        "android.os;Bundle;true;putSparseParcelableArray;;;Argument[1];MapValue of Argument[-1];value",
+        "android.os;Bundle;true;putStringArrayList;;;Argument[1];MapValue of Argument[-1];value",
+        "android.content;Intent;true;getExtras;();;SyntheticField[android.content.Intent.extras] of Argument[-1];ReturnValue;value",


java/ql/lib/semmle/code/java/frameworks/android/Intent.qll

bananabr · 2021-09-16T15:12:03Z

The context and class of the Intent can also be determined at construction time so the sanitizer needs to account for that.

Something like:

    // Handle the cases where the PackageContext and Class are set at construction time
    //    Intent(Context packageContext, Class<?> cls)
    // Create an intent for a specific component.
    //    Intent(String action, Uri uri, Context packageContext, Class<?> cls)
    exists(ConstructorCall cc |
      cc.getConstructedType() instanceof TypeIntent and
      cc.getNumArgument() > 1 and
      (
        cc.getArgument(0).getType() instanceof TypeContext and
        not exists(Expr classArg |
          classArg instanceof NullLiteral and DataFlow::localExprFlow(classArg, cc.getArgument(1))
        )
        or
        cc.getArgument(2).getType() instanceof TypeContext and
        not exists(Expr classArg |
          classArg instanceof NullLiteral and DataFlow::localExprFlow(classArg, cc.getArgument(3))
        )
      ) and
      DataFlow::localExprFlow(cc, node.asExpr())
    )

bananabr · 2021-09-16T15:22:45Z

There is also the case when the broadcast receiver is determined dynamically. An attacker can abuse the intent filter priority to make it the first available receiver. The current sanitizers would ignore this case as setClassName or setPackage are usually called.

I don't have the skills to propose a query for this case though.

Intent intent = new Intent("com.victim.ADD_CARD_ACTION");
intent.putExtra("credit_card_number", num.getText().toString());
intent.putExtra("holder_name", name.getText().toString());
//...
for(ResolveInfo info : getPackageManager().queryBroadcastReceivers(intent, 0)) {
    intent.setClassName(info.activityInfo.packageName, info.activityInfo.name);
    sendBroadcast(intent);
    return;
}

joefarebrother · 2021-09-23T11:11:40Z

Additional models have been split into #6739

github-actions · 2021-09-23T11:13:44Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged. The following differences were found:

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    Android,``android.*``,18,94,70,,,3,67,,,
+    Android,``android.*``,18,208,70,,,3,67,,,
-    Totals,,116,4169,402,13,6,10,107,33,1,66
+    Totals,,116,4283,402,13,6,10,107,33,1,66

Changes to framework-coverage-java.csv:

- android.content,8,,4,,,,,,,,,,,,,8,,,,,,4,
+ android.content,8,,44,,,,,,,,,,,,,8,,,,,,4,40
+ android.os,,,74,,,,,,,,,,,,,,,,,,,2,72

github-actions · 2021-10-20T16:12:40Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. A recent commit removed the previously reported differences.

atorralba

Some tests are failing, and I added a minor inline comment. Otherwise, LGTM.

atorralba · 2021-10-21T12:47:04Z

java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll

  private import semmle.code.java.frameworks.apache.Collections
  private import semmle.code.java.frameworks.apache.Lang
  private import semmle.code.java.frameworks.Flexjson
+  private import semmle.code.java.frameworks.android.Intent


Duplicates import in line 81.

Suggested change

private import semmle.code.java.frameworks.android.Intent

java/change-notes/2021-09-03-android-sensitive-broadcast.md

java/ql/src/Security/CWE/CWE-927/SensitiveCommunication.ql

java/ql/src/Security/CWE/CWE-927/SensitiveCommunication.qhelp

Co-authored-by: Tony Torralba <atorralba@users.noreply.github.com>

mchammer01 · 2021-10-22T13:16:58Z

Hi everyone 👋🏻 - Docs First Responder here, thanks for the ping.
I've added this PR to our review board ⚡

hubwriter

LGTM. A couple of little editorial suggestions. 👍

hubwriter · 2021-10-25T10:49:50Z

java/ql/src/Security/CWE/CWE-927/SensitiveCommunication.ql

@@ -0,0 +1,21 @@
+/**
+ * @name Leaking sensitive information through an implicit Intent


For consistency, use lowercase for intent - as elsewhere:

Suggested change

* @name Leaking sensitive information through an implicit Intent

* @name Leaking sensitive information through an implicit intent

Isn't Intent the name of the class? I'm not sure this should be lower-cased.

If so we should make it Intent throughout.

Indeed, it's the name of a class (just like Activity or Service), so I would use uppercase everywhere. Since there's more Android work coming, maybe a dedicated PR standardizing capitalization for this kind of thing in all files is due.

Let's fix the query name and change note of this query here in this PR instead of postponing these particular instances to a dedicated PR. Even if we need a general sweep to standardize, I don't think we should add to those needed changes.

java/change-notes/2021-09-03-android-sensitive-broadcast.md

Co-authored-by: hubwriter <hubwriter@github.com>

java/ql/lib/semmle/code/java/security/AndroidSensitiveCommunicationQuery.qll

java/ql/src/Security/CWE/CWE-927/SensitiveCommunication.ql

mchammer01 · 2021-10-25T15:15:45Z

I had set myself as a reviewer for this this morning and had actually started a review, but as this has been already reviewed by a Docs team member, I'll unassign myself 😄

atorralba

As suggested by @aschackmull and @hubwriter, let's capitalize the first letter in all occurrences of terms like Intent, Activity or Service.

java/ql/lib/semmle/code/java/security/AndroidSensitiveCommunicationQuery.qll

java/change-notes/2021-09-03-android-sensitive-broadcast.md

java/ql/src/Security/CWE/CWE-927/SensitiveCommunication.ql

java/ql/src/Security/CWE/CWE-927/SensitiveCommunication.qhelp

hubwriter

@atorralba - thanks for suggesting those changes.
LGTM 👍

Co-authored-by: Tony Torralba <atorralba@users.noreply.github.com>

joefarebrother requested a review from a team as a code owner September 3, 2021 13:02

github-actions bot added documentation Java labels Sep 3, 2021

joefarebrother requested a review from atorralba September 8, 2021 10:12

smowton reviewed Sep 13, 2021

View reviewed changes

atorralba reviewed Sep 13, 2021

View reviewed changes

java/ql/src/semmle/code/java/security/AndroidSensitiveBroadcastQuery.qll Outdated Show resolved Hide resolved

java/ql/src/semmle/code/java/security/AndroidSensitiveBroadcastQuery.qll Outdated Show resolved Hide resolved

This was referenced Sep 14, 2021

Java: add Gson support to unsafe-deserialization query #6463

Merged

Java: Create new Android Intent Redirection query #6397

Merged

atorralba reviewed Sep 14, 2021

View reviewed changes

java/ql/src/semmle/code/java/security/AndroidSensitiveBroadcastQuery.qll Outdated Show resolved Hide resolved

atorralba reviewed Sep 14, 2021

View reviewed changes

java/ql/test/query-tests/security/CWE-927/SensitiveBroadcast.ql Outdated Show resolved Hide resolved

atorralba reviewed Sep 14, 2021

View reviewed changes

joefarebrother force-pushed the android-sensitive-communication branch from 4062949 to cd7a8a7 Compare September 14, 2021 16:53

smowton reviewed Sep 16, 2021

View reviewed changes

joefarebrother force-pushed the android-sensitive-communication branch from 0511a17 to e815405 Compare September 23, 2021 11:11

atorralba mentioned this pull request Sep 30, 2021

Java: CWE-927 - Query to detect the use of implicit PendingIntents #6779

Merged

joefarebrother added 5 commits October 20, 2021 17:09

Move query and tests out of experimental

c68a707

Movee query logic to a qll file

143920e

Use subclasses of context for sinks

1e8dd7a

Remove existing get methods

bae0da8

Allow arbitrary read steps at the sink

4012866

joefarebrother added 3 commits October 20, 2021 17:09

Remove SendBroadcastMethodAccess

54e9c49

Rename from SensitiveBroadcast to SensitiveCommmunication

447e06d

Update docs

b6c584c

joefarebrother force-pushed the android-sensitive-communication branch from e815405 to b6c584c Compare October 20, 2021 16:10

Fix failing checks

3e3503a

atorralba reviewed Oct 21, 2021

View reviewed changes

joefarebrother added 2 commits October 21, 2021 16:46

Fix up test

a9dde41

Remove redundant imports from ExternalFlow

2d368a7

atorralba reviewed Oct 22, 2021

View reviewed changes

atorralba added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Oct 22, 2021

Apply suggestions from code review

c89178c

Co-authored-by: Tony Torralba <atorralba@users.noreply.github.com>

mchammer01 self-requested a review October 25, 2021 09:43

hubwriter previously approved these changes Oct 25, 2021

View reviewed changes

Write intent in lowercase consistently

0c1af24

Co-authored-by: hubwriter <hubwriter@github.com>

joefarebrother dismissed hubwriter’s stale review via 0c1af24 October 25, 2021 11:22

aschackmull reviewed Oct 25, 2021

View reviewed changes

java/ql/lib/semmle/code/java/security/AndroidSensitiveCommunicationQuery.qll Outdated Show resolved Hide resolved

aschackmull reviewed Oct 25, 2021

View reviewed changes

java/ql/lib/semmle/code/java/security/AndroidSensitiveCommunicationQuery.qll Show resolved Hide resolved

aschackmull reviewed Oct 25, 2021

View reviewed changes

java/ql/src/Security/CWE/CWE-927/SensitiveCommunication.ql Outdated Show resolved Hide resolved

Fix unneeded import and spelling mistake

6dac86b

hubwriter approved these changes Oct 25, 2021

View reviewed changes

mchammer01 removed their request for review October 25, 2021 15:16

atorralba reviewed Oct 26, 2021

View reviewed changes

hubwriter previously approved these changes Oct 26, 2021

View reviewed changes

Apply suggestions from code review

ba95d46

Co-authored-by: Tony Torralba <atorralba@users.noreply.github.com>

aschackmull dismissed hubwriter’s stale review via ba95d46 October 26, 2021 12:04

aschackmull approved these changes Oct 26, 2021

View reviewed changes

joefarebrother merged commit 02b440b into github:main Oct 26, 2021

	* Holds if a `sendBroadcast` call doesn't specify receiver permission.
	* Holds if a `sendBroadcast` call `sendBroadcastCall` doesn't specify receiver permission.

	private predicate isSensitiveBroadcastSink(DataFlow::Node sink) {
	private predicate isSensitiveBroadcastSink(DataFlow::Node sendBroadcastCall) {

		@@ -0,0 +1,21 @@
		/**
		* @name Leaking sensitive information through an implicit Intent

Conversation

joefarebrother commented Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 3, 2021

java

Generated file changes for java

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atorralba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

atorralba commented Sep 13, 2021

Uh oh!

joefarebrother commented Sep 13, 2021

Uh oh!

atorralba commented Sep 13, 2021

Uh oh!

Uh oh!

Uh oh!

atorralba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Sep 14, 2021

java

Generated file changes for java

Uh oh!

smowton commented Sep 16, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bananabr commented Sep 16, 2021

Uh oh!

bananabr commented Sep 16, 2021

Uh oh!

joefarebrother commented Sep 23, 2021

Uh oh!

github-actions bot commented Sep 23, 2021

java

Generated file changes for java

Uh oh!

github-actions bot commented Oct 20, 2021

Uh oh!

atorralba left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mchammer01 commented Oct 22, 2021

Uh oh!

hubwriter left a comment

Choose a reason for hiding this comment

Uh oh!

joefarebrother commented Sep 3, 2021 •

edited

Loading

mchammer01 commented Oct 25, 2021 •

edited

Loading

hubwriter left a comment •

edited

Loading