Skip to content

Conversation

joefarebrother
Copy link
Contributor

@joefarebrother joefarebrother commented Oct 4, 2022

Query that detects instances of sensitive information that could be saved into the keyboard cache.

Considers cases in which an input field declared is declared via XML with a name that indicates it may contain sensitive information, but its input type, as declared either in XML or in code, indicates that it is saved to the keyboard cache.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2022

QHelp previews:

java/ql/src/Security/CWE/CWE-524/SensitiveKeyboardCache.qhelp

errors/warnings:

./java/ql/src/Security/CWE/CWE-524/SensitiveKeyboardCache.qhelp:1:1: Content is not allowed in prolog.
A fatal error occurred: 1 qhelp files could not be processed.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2022

QHelp previews:

java/ql/src/Security/CWE/CWE-524/SensitiveKeyboardCache.qhelp

errors/warnings:

./java/ql/src/Security/CWE/CWE-524/SensitiveKeyboardCache.qhelp:32:127: element "code" not allowed here; expected the element end-tag or text
./java/ql/src/Security/CWE/CWE-524/SensitiveKeyboardCache.qhelp:33:5: element "li" not allowed here; expected the element end-tag, text or element "a", "b", "blockquote", "code", "em", "i", "img", "ol", "p", "pre", "sample", "strong", "sub", "sup", "table", "tt", "ul" or "warning"
./java/ql/src/Security/CWE/CWE-524/SensitiveKeyboardCache.qhelp:35:3: The element type "li" must be terminated by the matching end-tag "</li>".
A fatal error occurred: 1 qhelp files could not be processed.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2022

QHelp previews:

java/ql/src/Security/CWE/CWE-524/SensitiveKeyboardCache.qhelp

errors/warnings:

./java/ql/src/Security/CWE/CWE-524/SensitiveKeyboardCache.qhelp:32:127: element "code" not allowed here; expected the element end-tag or text
./java/ql/src/Security/CWE/CWE-524/SensitiveKeyboardCache.qhelp:33:5: element "li" not allowed here; expected the element end-tag, text or element "a", "b", "blockquote", "code", "em", "i", "img", "ol", "p", "pre", "sample", "strong", "sub", "sup", "table", "tt", "ul" or "warning"
./java/ql/src/Security/CWE/CWE-524/SensitiveKeyboardCache.qhelp:35:3: The element type "li" must be terminated by the matching end-tag "</li>".
A fatal error occurred: 1 qhelp files could not be processed.

@joefarebrother joefarebrother marked this pull request as ready for review October 7, 2022 13:33
@joefarebrother joefarebrother requested a review from a team as a code owner October 7, 2022 13:33
@github-actions
Copy link
Contributor

github-actions bot commented Oct 7, 2022

QHelp previews:

java/ql/src/Security/CWE/CWE-524/SensitiveKeyboardCache.qhelp

Android sensitive keyboard cache

When a user enters information in a text input field on an Android application, their input is saved to a keyboard cache which provides autocomplete suggestions and predictions. There is a risk that sensitive user data, such as passwords or banking information, may be leaked to other applications via the keyboard cache.

Recommendation

For input fields expected to accept sensitive information, use input types such as "textNoSuggestions" (or "textPassword" for a password) to ensure the input does not get stored in the keyboard cache.

Optionally, instead of declaring an input type through XML, you can set the input type in your code using TextView.setInputType().

Example

In the following example, the field labeled BAD allows the password to be saved to the keyboard cache, whereas the field labeled GOOD uses the "textPassword" input type to ensure the password is not cached.

<?xml version="1.0" encoding="utf-8"?>
<LinearLayout
    xmlns:android="http://schemas.android.com/apk/res/android"
    xmlns:app="http://schemas.android.com/apk/res-auto">

    <!-- BAD: This password field uses the `text` input type, which allows the input to be saved to the keyboard cache. -->
    <EditText
        android:id="@+id/password_bad"
        android:inputType="text"/> 

    <!-- GOOD: This password field uses the `textPassword` input type, which ensures that the input is not saved to the keyboard cache. -->
    <EditText
        android:id="@+id/password_good"
        android:inputType="textPassword"/>  
</LinearLayout>

References

Copy link
Contributor

@atorralba atorralba left a comment

Choose a reason for hiding this comment

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

This mostly looks good, I added some minor comments.

Other than that, have you considered that the input type could be added programmatically via setInputType? I'm curious about how many alerts raised by this query could be discarded because of this. If there are enough, it could might sense to handle it as well.

WDYT?

Copy link
Contributor

@atorralba atorralba left a comment

Choose a reason for hiding this comment

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

Hey @joefarebrother thanks for working on this, I think it'll be a valuable addition. I added some more minor comments and a conceptual question, but otherwise this LGTM. If DCA and MRVA look good, we should ask for a docs review.

@joefarebrother joefarebrother force-pushed the android-keyboard-cache branch 3 times, most recently from b8e0779 to 3242bbb Compare November 8, 2022 15:07
@joefarebrother joefarebrother added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Nov 9, 2022
Copy link
Contributor

@sabrowning1 sabrowning1 left a comment

Choose a reason for hiding this comment

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

👋🏼 @joefarebrother, thanks for your work on this! I've suggested a few small changes to the .qhelp file to align with our style guide. Let me know if you have any questions!

Copy link
Contributor

@sabrowning1 sabrowning1 left a comment

Choose a reason for hiding this comment

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

Looks good for Docs, thanks @joefarebrother! 👍🏼

Copy link
Contributor

@atorralba atorralba left a comment

Choose a reason for hiding this comment

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

🎉

@joefarebrother joefarebrother merged commit d6c5132 into github:main Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Java ready-for-doc-review This PR requires and is ready for review from the GitHub docs team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants