Skip to content

Java: Query to detect cleartext storage of sensitive information using Android SharedPreferences #4675

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 11 commits into from
Jan 8, 2021
Merged

Java: Query to detect cleartext storage of sensitive information using Android SharedPreferences #4675

merged 11 commits into from
Jan 8, 2021

Conversation

luchua-bc
Copy link
Contributor

SharedPreferences is an Android API that stores application preferences using simple sets of data values. Almost every Android application uses this API. It allows to easily save, alter, and retrieve the values stored in SharedPreferences.

However, sensitive information shall not be saved in cleartext. Otherwise it can be accessed by any process or user on rooted devices, or can be disclosed through chained vulnerabilities e.g. unexpected access to its private storage through exposed components.

This query detects storage of sensitive information in cleartext using SharedPreferences on Android devices.

Please consider to merge the PR. Thanks.

@luchua-bc luchua-bc requested review from felicitymay and a team as code owners November 16, 2020 17:27
@luchua-bc luchua-bc changed the title Query for cleartext storage using Android SharedPreferences Java: Query for cleartext storage using Android SharedPreferences Nov 16, 2020
@luchua-bc luchua-bc changed the title Java: Query for cleartext storage using Android SharedPreferences Java: Query to detect cleartext storage of sensitive information using Android SharedPreferences Nov 16, 2020
@luchua-bc luchua-bc mentioned this pull request Nov 16, 2020
Closed
1 task
@smowton smowton self-assigned this Nov 19, 2020
smowton
smowton reviewed Nov 19, 2020
View reviewed changes
Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests are in experimental but the query itself is not?

Also check my understanding: the reason we can't simply say a setter call against a SharedPreferences.Editor is a sink is because we need to know that this instance of SharedPreferences is specifically not an EncryptedSharedPreferences, which is a subclass that uses the same Editor?

@luchua-bc
Copy link
Contributor Author

Thanks @smowton for reviewing this PR.

Yes, your understanding is correct. We need to know that this instance of SharedPreferences is specifically not an EncryptedSharedPreferences, which is a subclass that uses the same Editor.

Although there is no existing tests folder java/ql/test/query-tests/security/CWE-312/, I've created one and moved the tests from experimental since the query itself is integrated into existing CWE-312 queries.

smowton
smowton reviewed Nov 24, 2020
View reviewed changes
Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions, many of which are perhaps best answered with negative tests (i.e. tests that show the relevant vulnerability, but the query cannot detect) -- what are this query's restrictions? Where can it tolerate arbitrary dataflow, and where only local dataflow or no intermediate flow at all?

smowton
smowton reviewed Nov 25, 2020
View reviewed changes
Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could live in experimental, which will make the review process much easier -- as far as I can tell the only shared code with the existing SensitiveStorage file is your use of the SensitiveExpr class, which is already public so you should be able to use it from experimental?

@felicitymay felicitymay removed their request for review November 27, 2020 09:00
@smowton
Copy link
Contributor

smowton commented Nov 27, 2020

I've started an evaluation of this -- one problem I noticed doing so: the @kind is currently path-problem, but the top-level select does not pick a source-sink pair. Either change @kind to problem, or decide on a path you want the query to show.

@luchua-bc
Copy link
Contributor Author

I've changed the @kind tag of the query back to problem, please continue to evaluate.

aschackmull
aschackmull requested changes Jan 7, 2021
View reviewed changes
aschackmull
aschackmull requested changes Jan 7, 2021
View reviewed changes
Copy link
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more comments, otherwise it's looking good.

@aschackmull aschackmull merged commit e5b4975 into github:main Jan 8, 2021
@luchua-bc luchua-bc deleted the cleartext-storage-shared-prefs branch January 8, 2021 12:41
@atorralba atorralba mentioned this pull request Aug 11, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smowton smowton smowton left review comments

@Marcono1234 Marcono1234 Marcono1234 left review comments

@aschackmull aschackmull aschackmull approved these changes

Assignees

@smowton smowton

Labels
documentation Java
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@luchua-bc @smowton @Marcono1234 @aschackmull