From a3749530d6c15b19ef7001eba31d246b6c37fe66 Mon Sep 17 00:00:00 2001 From: Lukas Abfalterer Date: Mon, 3 Mar 2025 10:20:46 +0100 Subject: [PATCH 1/5] The query should only report cases when the method is not empty. --- java/ql/src/Security/CWE/CWE-925/ImproperIntentVerification.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-925/ImproperIntentVerification.ql b/java/ql/src/Security/CWE/CWE-925/ImproperIntentVerification.ql index 605fabc25b51..611ac03627df 100644 --- a/java/ql/src/Security/CWE/CWE-925/ImproperIntentVerification.ql +++ b/java/ql/src/Security/CWE/CWE-925/ImproperIntentVerification.ql @@ -14,6 +14,6 @@ import java import semmle.code.java.security.ImproperIntentVerificationQuery from AndroidReceiverXmlElement reg, Method orm, SystemActionName sa -where unverifiedSystemReceiver(reg, orm, sa) +where unverifiedSystemReceiver(reg, orm, sa) and orm.getBody().getBlock().getNumStmt() > 0 select orm, "This reciever doesn't verify intents it receives, and $@ to receive $@.", reg, "it is registered", sa, "the system action " + sa.getName() From c9b75afc2a5fab7e3225bbc96b79c7b21a60cb0f Mon Sep 17 00:00:00 2001 From: Lukas Abfalterer Date: Wed, 5 Mar 2025 10:23:35 +0100 Subject: [PATCH 2/5] Fix QLL and add change notes with tests --- .../java/security/ImproperIntentVerificationQuery.qll | 4 +++- .../Security/CWE/CWE-925/ImproperIntentVerification.ql | 2 +- .../2025-03-03-fix-improper-intent-verification-query.md | 4 ++++ .../query-tests/security/CWE-925/AndroidManifest.xml | 7 ++++++- .../query-tests/security/CWE-925/EmptyReceiverXml.java | 9 +++++++++ 5 files changed, 23 insertions(+), 3 deletions(-) create mode 100644 java/ql/src/change-notes/2025-03-03-fix-improper-intent-verification-query.md create mode 100644 java/ql/test/query-tests/security/CWE-925/EmptyReceiverXml.java diff --git a/java/ql/lib/semmle/code/java/security/ImproperIntentVerificationQuery.qll b/java/ql/lib/semmle/code/java/security/ImproperIntentVerificationQuery.qll index 995f10ad3c9a..ff5ebe862178 100644 --- a/java/ql/lib/semmle/code/java/security/ImproperIntentVerificationQuery.qll +++ b/java/ql/lib/semmle/code/java/security/ImproperIntentVerificationQuery.qll @@ -51,7 +51,9 @@ private module VerifiedIntentFlow = DataFlow::Global; /** An `onReceive` method that doesn't verify the action of the intent it receives. */ private class UnverifiedOnReceiveMethod extends OnReceiveMethod { UnverifiedOnReceiveMethod() { - not VerifiedIntentFlow::flow(DataFlow::parameterNode(this.getIntentParameter()), _) + not VerifiedIntentFlow::flow(DataFlow::parameterNode(this.getIntentParameter()), _) and + // Empty methods do not need to be verified since they do not perform any actions. + this.getBody().getNumStmt() > 0 } } diff --git a/java/ql/src/Security/CWE/CWE-925/ImproperIntentVerification.ql b/java/ql/src/Security/CWE/CWE-925/ImproperIntentVerification.ql index 611ac03627df..605fabc25b51 100644 --- a/java/ql/src/Security/CWE/CWE-925/ImproperIntentVerification.ql +++ b/java/ql/src/Security/CWE/CWE-925/ImproperIntentVerification.ql @@ -14,6 +14,6 @@ import java import semmle.code.java.security.ImproperIntentVerificationQuery from AndroidReceiverXmlElement reg, Method orm, SystemActionName sa -where unverifiedSystemReceiver(reg, orm, sa) and orm.getBody().getBlock().getNumStmt() > 0 +where unverifiedSystemReceiver(reg, orm, sa) select orm, "This reciever doesn't verify intents it receives, and $@ to receive $@.", reg, "it is registered", sa, "the system action " + sa.getName() diff --git a/java/ql/src/change-notes/2025-03-03-fix-improper-intent-verification-query.md b/java/ql/src/change-notes/2025-03-03-fix-improper-intent-verification-query.md new file mode 100644 index 000000000000..de4eda47c3eb --- /dev/null +++ b/java/ql/src/change-notes/2025-03-03-fix-improper-intent-verification-query.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Fixed false positive in CWE-925 by requiring the `onReceive` method must be non-empty diff --git a/java/ql/test/query-tests/security/CWE-925/AndroidManifest.xml b/java/ql/test/query-tests/security/CWE-925/AndroidManifest.xml index f9e11a1ee812..f58941eb557a 100644 --- a/java/ql/test/query-tests/security/CWE-925/AndroidManifest.xml +++ b/java/ql/test/query-tests/security/CWE-925/AndroidManifest.xml @@ -5,5 +5,10 @@ + + + + + - \ No newline at end of file + diff --git a/java/ql/test/query-tests/security/CWE-925/EmptyReceiverXml.java b/java/ql/test/query-tests/security/CWE-925/EmptyReceiverXml.java new file mode 100644 index 000000000000..44a81db62302 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-925/EmptyReceiverXml.java @@ -0,0 +1,9 @@ +package test; +import android.content.Intent; +import android.content.Context; +import android.content.BroadcastReceiver; + +class EmptyReceiverXml extends BroadcastReceiver { + @Override + public void onReceive(Context ctx, Intent intent) { } +} From 41e9a837e513807ca0e9c3dd3ecc132bcb149994 Mon Sep 17 00:00:00 2001 From: Lukas Abfalterer Date: Wed, 5 Mar 2025 12:50:54 +0100 Subject: [PATCH 3/5] Fix naming Co-authored-by: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com> --- java/ql/test/query-tests/security/CWE-925/AndroidManifest.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/test/query-tests/security/CWE-925/AndroidManifest.xml b/java/ql/test/query-tests/security/CWE-925/AndroidManifest.xml index f58941eb557a..485c28dbdc6d 100644 --- a/java/ql/test/query-tests/security/CWE-925/AndroidManifest.xml +++ b/java/ql/test/query-tests/security/CWE-925/AndroidManifest.xml @@ -5,7 +5,7 @@ - + From f2947f7066da877efd98bb0eae9ee363da31d4e9 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com> Date: Wed, 5 Mar 2025 14:13:53 +0000 Subject: [PATCH 4/5] Fix indentation --- java/ql/test/query-tests/security/CWE-925/AndroidManifest.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/test/query-tests/security/CWE-925/AndroidManifest.xml b/java/ql/test/query-tests/security/CWE-925/AndroidManifest.xml index 485c28dbdc6d..5fd3986f82c2 100644 --- a/java/ql/test/query-tests/security/CWE-925/AndroidManifest.xml +++ b/java/ql/test/query-tests/security/CWE-925/AndroidManifest.xml @@ -8,7 +8,7 @@ - + From 32e15897454166bf327b7606226e3c3cf5979887 Mon Sep 17 00:00:00 2001 From: Lukas Abfalterer Date: Thu, 6 Mar 2025 09:57:16 +0100 Subject: [PATCH 5/5] Update java/ql/src/change-notes/2025-03-03-fix-improper-intent-verification-query.md Co-authored-by: Edward Minnix III --- .../2025-03-03-fix-improper-intent-verification-query.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/change-notes/2025-03-03-fix-improper-intent-verification-query.md b/java/ql/src/change-notes/2025-03-03-fix-improper-intent-verification-query.md index de4eda47c3eb..b07ffc99a969 100644 --- a/java/ql/src/change-notes/2025-03-03-fix-improper-intent-verification-query.md +++ b/java/ql/src/change-notes/2025-03-03-fix-improper-intent-verification-query.md @@ -1,4 +1,4 @@ --- category: minorAnalysis --- -* Fixed false positive in CWE-925 by requiring the `onReceive` method must be non-empty +* Overrides of `BroadcastReceiver::onReceive` with no statements in their body are no longer considered unverified by the `java/improper-intent-verification` query. This will reduce false positives from `onReceive` methods which do not perform any actions.