Java: Sensitive broadcast

[luchua-bc]

This PR is to address CWE-927 "Use of Implicit Intent for Sensitive Communication".

Vulnerable Android applications use an implicit intent for transmitting sensitive data to other applications. Since an implicit intent does not specify a particular application to receive the data, any application can process the intent by using an Intent Filter for that intent. This can allow untrusted applications to obtain sensitive data.

Query in this PR detects the following patterns:

1. No targeted application component is specified, which means implicit intent.
2. No receiver permission is specified.

Please consider to merge the PR. Thanks.

[smowton]

Looks like a good idea, but we should use dataflow to track how intents acquire taint, rather than hard-code the structure of the put*Extra steps.

[luchua-bc]

Thanks @smowton for all the suggestions and I've revamped the query to address all of them. Now the query is much cleaner and more elegant, please let me know if more changes are required.

[smowton]

This is looking much better now it's phrased using dataflow -- just some smaller things to fix and the testsuite should be expanded a bit to make sure you're covering the whole query.

[smowton]

One more test case to add -- how about getExtras().putString(...)? As this query is phrased I don't think that'll work, but it's a little tricky to phrase. Feel free to mark that case BAD (but not yet detected); again this is testing things just outside what the query is capable of to help future authors.

[luchua-bc]

@smowton As per your suggestion, I've added one more test case sendBroadcast12 with a comment stating getExtras().putString(...) is not yet detected thus is beyond what this query is capable of.

Thanks for all your help with this PR.

[smowton]

Make sure SensitiveInfoLog.ql's behaviour is unchanged, otherwise LGTM

[smowton]

Suggestion from seclab review: the userid possibility is causing most false-positives. Unless you know a case where this is important, suggest dropping userid and username (i.e. if they are already spotted by SensitiveLogInfo, move them out of common and back into SensitiveLogInfo-only regex, then we'll assess whether to drop it from that query as well in our own time)

[luchua-bc]

@smowton Sorry for the confusion. I've removed the newly added userid from the regex in SensitiveActions.qll and put username back to SensitiveInfoLog.ql as what was originally reviewed and tested. We can further investigate in a separate PR when changes to SensitiveInfoLog.ql are needed.

[luchua-bc]

Or I can remove username from SensitiveInfoLog.ql as well since it's not that sensitive. Please advise. Thanks.

[smowton]

I think this is good as it stands now -- just waiting for @github/codeql-java to weigh in as the code owners.

[luchua-bc]

Thanks @smowton for all your help with this PR.

[smowton]

Done (but still pending code-owner review)

[aschackmull]

Qhelp could use a few more references, otherwise LGTM.

[aschackmull]

Oh, and we need to autoformat:

[ERROR] Input file ql/java/ql/src/experimental/Security/CWE/CWE-532/SensitiveInfoLog.ql is not correctly formatted

[luchua-bc]

Sorry that I forgot to autoformat SensitiveInfoLog.ql, I've committed the updated version.