Java: Add query for Improper Verification of Intent by Broadcast Receiver (CWE-925) #8669

joefarebrother · 2022-04-05T13:24:03Z

No description provided.

github-advanced-security

Found 1 vulnerability.

java/ql/src/Security/CWE/CWE-925/ImproperIntentVerification.ql

atorralba

This is a good looking query! Good job including the dynamically declared receivers too, I'm eager to see what this catches when run at scale.

I added some inline comments.

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

java/ql/src/Security/CWE/CWE-925/Good.java

java/ql/src/change-notes/2022-04-27-intent-verification.md

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

atorralba · 2022-05-03T08:36:58Z

This is mostly looking good to me. I think we need FP ratio and performance evaluations before merging though.

atorralba · 2022-05-31T10:19:17Z

After experimenting a little with real world results, it seems that this vulnerability cannot be exploited through dynamically registered receivers (i.e. using Context.registerReceiver), because for this attack you need explicit Intents (to avoid the requirement of using a privileged action), but dynamically registered receivers cannot be targeted by explicit Intents. This article explains it in detail: https://onemikro2nd.blogspot.com/2013/09/darker-corners-of-android.html

So, I think we should restrict the query to only check statically registered receivers (i.e. through the Android manifest). I guess we could also handle dynamically registered receivers that have a filter for both a privileged action and an unprivileged action, but I'd say this is a pretty rare case and the chances of the receiver actually doing something sensitive would drop significantly. So I'll leave up to you to decide whether you want to handle that case — otherwise, we should just remove everything related to registerReceiver from this query.

mchammer01

@joefarebrother 👋🏻 - this LGTM, I've made a few comments and suggestions for your consideration.
Would be great if you could correct the typo, and reword the query description. Hope this helps 🙂

java/ql/src/Security/CWE/CWE-925/ImproperIntentVerification.ql

mchammer01 · 2022-06-06T09:54:01Z

java/ql/src/Security/CWE/CWE-925/ImproperIntentVerification.ql

@@ -0,0 +1,19 @@
+/**
+ * @name Improper Verification of Intent by Broadcast Receiver
+ * @description The Android application uses a Broadcast Receiver that receives an Intent but does not properly verify that the Intent came from an authorized source.


Same comment here, unless you absolutely need to use capitalization (like in Android).
Also, this is not the preferred syntax for a description ("Syntax X causes behavior Y."). Could you reword and remove the capitalization?

java/ql/src/change-notes/2022-04-27-intent-verification.md

java/ql/src/Security/CWE/CWE-925/ImproperIntentVerification.qhelp

mchammer01 · 2022-06-06T10:01:56Z

java/ql/src/Security/CWE/CWE-925/ImproperIntentVerification.qhelp

+<overview>
+<p>
+When an android application uses a <code>BroadcastReciever</code> to receive intents, 
+it is also able to receive explicit intents that are sent directly to it, regardless of its filter.


What does the its pronoun refer to here, the app or the BroadcastReceiver (sorry, not a developer)?

It refers to the BroadcastReceiver

java/ql/src/Security/CWE/CWE-925/ImproperIntentVerification.qhelp

mchammer01 · 2022-06-06T10:03:40Z

java/ql/src/Security/CWE/CWE-925/ImproperIntentVerification.qhelp

+
+Certain intent actions are only able to be sent by the operating system, not third-party applications. 
+However, a <code>BroadcastReceiver</code> that is registered to receive system intents is still able to receive 
+other intents from a third-party application, so it should check that the intent received has the expected action.


Not required?

Suggested change

other intents from a third-party application, so it should check that the intent received has the expected action.

intents from a third-party application, so it should check that the intent received has the expected action.

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

fix typos and capitilisation; reword description.

java/ql/src/Security/CWE/CWE-925/ImproperIntentVerification.ql

github-actions bot added the Java label Apr 5, 2022

joefarebrother force-pushed the intent-verification branch from 848309f to 9cbc4be Compare April 5, 2022 13:25

github-actions bot added the documentation label Apr 27, 2022

github-advanced-security bot found potential problems Apr 27, 2022

View reviewed changes

java/ql/src/Security/CWE/CWE-925/ImproperIntentVerification.ql Fixed Show fixed Hide fixed

joefarebrother marked this pull request as ready for review April 27, 2022 15:32

joefarebrother requested a review from a team as a code owner April 27, 2022 15:32

atorralba reviewed Apr 28, 2022

View reviewed changes

joefarebrother force-pushed the intent-verification branch from 9950f57 to f317ebf Compare April 28, 2022 13:14

joefarebrother force-pushed the intent-verification branch from f317ebf to e2cf7cc Compare May 16, 2022 15:43

atorralba added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Jun 2, 2022

mchammer01 self-requested a review June 6, 2022 09:25

mchammer01 reviewed Jun 6, 2022

View reviewed changes

joefarebrother and others added 10 commits June 14, 2022 14:56

Add Improper Intent Verification query

1959f49

Fix typos

87f26bf

Add test cases; fix handling of recievers declared through xml

4aed1a1

Add qhelp

8e2e8cc

Add security severity

2fc142f

Add change note

d88d216

Apply suggestions from code review - fix typos/style, make things pri…

9d048e7

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

Adress reveiw comments - make use of existing ql libraries

320c671

Remove checks for dynamically registered recievers

c71586e

Apply doc review suggestions -

a6736a9

fix typos and capitilisation; reword description.

joefarebrother force-pushed the intent-verification branch from 4717d32 to a6736a9 Compare June 14, 2022 13:56

github-advanced-security bot found potential problems Jun 14, 2022

View reviewed changes

java/ql/src/Security/CWE/CWE-925/ImproperIntentVerification.ql Fixed Show fixed Hide fixed

Fix misspellings

f46dd8c

atorralba approved these changes Jun 29, 2022

View reviewed changes

atorralba merged commit 12fa696 into github:main Jun 29, 2022

	other intents from a third-party application, so it should check that the intent received has the expected action.
	intents from a third-party application, so it should check that the intent received has the expected action.

Java: Add query for Improper Verification of Intent by Broadcast Receiver (CWE-925) #8669

Java: Add query for Improper Verification of Intent by Broadcast Receiver (CWE-925) #8669

Uh oh!

Conversation

joefarebrother commented Apr 5, 2022

Uh oh!

github-advanced-security bot left a comment

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

atorralba commented May 3, 2022

Uh oh!

atorralba commented May 31, 2022

Uh oh!

mchammer01 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mchammer01 Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mchammer01 Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

joefarebrother Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mchammer01 Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joefarebrother Jun 14, 2022 •

edited

Loading