Java: CWE-200 - Query to detect exposure of sensitive information from android file intent

[luchua-bc]

The Android API allows to start an activity in another mobile application and receive a result back. When starting an activity to retrieve a file from another application, missing input validation can lead to leaking of sensitive configuration file or user data because the intent is from the application itself that is allowed to access its protected data therefore bypassing the access control.

The query detects the exposure of sensitive information from android file intents and it also models the following common scenarios of file access in Android:

• In an Android asynchronous task thread AsyncTask to execute in the background
• In an Android backend service

Please consider to merge the PR. Thanks.

[smowton]

This mostly looks good now, just resolving what we do with the android class models, which intersect with a few other PRs currently in flight, with the team working on droid.

[luchua-bc]

Thanks @smowton for the update and all your help with this PR.

[smowton]

Note there are test failures

[smowton]

Failing:

• java/ql/test/library-tests/dataflow/taintsources/remote.ql
• java/ql/test/experimental/query-tests/security/CWE-200/SensitiveAndroidFileLeak.qlref

[luchua-bc]

Hi @smowton,

I tested the query on my staging server and the test passed so the CodeQL version is identical to my local copy. As the link shows a 404 "page not found" error, probably because it's an internal page, I cannot see details of "Java Language Tests (GitHub Actions)".

Would you please shed some light on what is wrong? Thanks.

[smowton]

Pushed test fixes; some were due to a new feature introduced last week that requires a rebase on main to see the test fail.

[smowton]

Also removed the CSV models of AsyncTask, which are too complicated to represent in that form anyway.

[smowton]

In fact there's quite a few of the modelled methods that aren't used in this PR: do you intend to only track exploitable data types, i.e. mostly strings? Or any sort of influence over the Intent? If it's the former, the int flags seems likely to be harmless, for example?

[luchua-bc]

I intend to only track exploitable data types, i.e. mostly strings.

However, as an invocation can be:

intent.setType("*/*").addFlags(Intent.FLAG_ACTIVITY_NEW_TASK).addCategory(Intent.CATEGORY_OPENABLE).putExtra(Intent.EXTRA_ALLOW_MULTIPLE, true);

I added those methods since I think they can propagate the taint. They shall be harmless.

[smowton]

Oh I see-- these are actually fluent methods, and should be modelled like

package;Class;true;methodName;;;Argument[-1];Argument[0];value

The use of value rather than taint means that the exact value of the qualifier (arg -1) is returned, and the dataflow library will use this information to propagate taint.

There's no need to add those edges here because we have a more complete model of Intent intended to land this week at which point I'll edit this PR to remove it, but in future value edges are the way to go whenever a function might return the exact value passed to one of its arguments.

[luchua-bc]

Thanks @smowton for the explanation and the offer to modify the query and merge with the new intent model to be released. I will use fluent methods for new queries in the future.

[smowton]

android.net.Uri models pulled out and expanded as #6685

[smowton]

@luchua-bc I've updated this to use the shared Intent models; will merge this on green

[github-actions]

⚠️ 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.*``,45,233,70,,,3,67,,,
+    Android,``android.*``,45,245,70,,,3,67,,,
-    Totals,,143,5125,408,13,6,10,107,33,1,66
+    Totals,,143,5137,408,13,6,10,107,33,1,66

• Changes to framework-coverage-java.csv:

- android.content,8,27,61,,,,,,,,,,,,,8,,,,,,27,,4,57
+ android.content,8,27,73,,,,,,,,,,,,,8,,,,,,27,,8,65

[luchua-bc]

Thanks @smowton a lot for all your help with this PR.

[luchua-bc]

Thanks @smowton a lot for the change of IntentFieldsInheritTaint and detailed comments. I learned a new technique of modelling tainted objects with TaintInheritingContent.