Java: Query to detect cleartext storage of sensitive information using Android SharedPreferences - #4675
Conversation
Sorry, something went wrong.
smowton
left a comment
There was a problem hiding this comment.
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?
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
smowton
left a comment
There was a problem hiding this comment.
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?
Sorry, something went wrong.
smowton
left a comment
There was a problem hiding this comment.
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?
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
I've changed the @kind tag of the query back to problem, please continue to evaluate. |
Sorry, something went wrong.
aschackmull
left a comment
There was a problem hiding this comment.
A few more comments, otherwise it's looking good.
Sorry, something went wrong.
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.