Java: Add query for sensitive data exposed in text fields #15396

joefarebrother · 2024-01-22T15:47:00Z

This query checks for sensitive data being exposed to text fields.
Fields that have their visibility or input type set (dynamically or via xml) are considered safe and excluded.

github-actions · 2024-01-22T15:48:13Z

QHelp previews:

java/ql/src/Security/CWE/CWE-200/AndroidSensitiveNotifications.qhelp

Exposure of sensitive information to notifications

Sensitive information such as passwords or two-factor authentication (2FA) codes should not be exposed in a system notification. Notifications should not be considered secure, as other untrusted applications may be able to use a NotificationListenerService to read the contents of notifications.

Recommendation

Do not expose sensitive data in notifications.

Example

In the following sample, the password is sent as part of a notification. This can allow another application to read this password.

// BAD: `password` is exposed in a notification.
void confirmPassword(String password) {
    NotificationManager manager = NotificationManager.from(this);
    manager.send(
        new Notification.Builder(this, CHANNEL_ID)
        .setContentText("Your password is: " + password)
        .build());
}

References

OWASP Mobile Application Security: Android Data Storage - Application Notifications
Common Weakness Enumeration: CWE-200.

java/ql/lib/semmle/code/java/security/SensitiveUiQuery.qll

github-actions · 2024-01-22T15:50:55Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. A recent commit removed the previously reported differences.

github-actions · 2024-01-24T14:12:01Z

QHelp previews:

java/ql/src/Security/CWE/CWE-200/AndroidSensitiveTextField.qhelp

Exposure of sensitive information to UI text views.

Sensitive information such as passwords should not be displayed in UI components unless explicitly required, to mitigate shoulder-surfing attacks.

Recommendation

For editable text fields containing sensitive information, the inputType should be set to textPassword or similar to ensure it is properly masked. Otherwise, sensitive data that must be displayed should be hidden by default, and only revealed based on an explicit user action.

Example

In the following (bad) case, sensitive information in password is exposed to the TextView.

TextView pwView = getViewById(R.id.pw_text);
pwView.setText("Your password is: " + password);

In the following (good) case, the user must press a button to reveal sensitive information.

TextView pwView = findViewById(R.id.pw_text);
pwView.setVisibility(View.INVISIBLE);
pwView.setText("Your password is: " + password);

Button showButton = findViewById(R.id.show_pw_button);
showButton.setOnClickListener(new View.OnClickListener() {
    public void onClick(View v) {
      pwView.setVisibility(View.VISIBLE);
    }
});

References

OWASP Mobile Application Security: Android Data Storage - UI Components
Common Weakness Enumeration: CWE-200.

…w is masked, and find more instances of findViewById.

atorralba

Looks quite good to me! I like the introduction of the Layout.qll library and moving everything layout-related there.

I added some minor comments for your consideration. As always, DCA, MRVA and docs review will have the final say.

java/ql/lib/semmle/code/java/security/SensitiveUiQuery.qll

atorralba · 2024-01-30T10:45:06Z

java/ql/lib/semmle/code/java/security/SensitiveUiQuery.qll

+private class MaskCall extends MethodCall {
+  MaskCall() {
+    this.getMethod().getAnOverride*().hasQualifiedName("android.widget", "TextView", "setInputType")
+    or
+    this.getMethod().getAnOverride*().hasQualifiedName("android.view", "View", "setVisibility")
+  }
+}


Is it worth checking the arguments of the method call? Either that it's View.INVISIBLE or its literal value, or some InputType.*PASSWORD* variation, its literal value, or a binary OR operation with one of those as an operand.

It's a potential precision improvement to reduce FNs, but I'm not sure it's worthwhile. For setVisibility, it seems redundant; as the visibility is either being set from invisible to visible, or from visible to invisible; either implying it would have been invisible at some point. For setInputType, ignoring it yields a few more results in just one project, which set input type to values other than passwords so would appear with a more precise check here, but i'm not sure whether they are TPs.

Ok, let's just roll with it and revisit this in the future if we start seeing FNs.

java/ql/lib/semmle/code/java/frameworks/android/Layout.qll

atorralba · 2024-01-30T10:52:46Z

java/ql/lib/semmle/code/java/security/SensitiveUiQuery.qll

+}
+
+/** Holds if data can flow from `e1` to `e2` with local flow steps as well as flow through fields containing `View`s */
+private predicate localViewFieldExprFlow(Expr e1, Expr e2) {


I'm a bit unsure about how this will perform. If you see any indications of this performing badly in DCA let me know, there are a couple of tricks we can use as alternative to this kind of local flow (the modules SimpleGlobal and LocalTaintFlow), and we can also tweak the field read-write step to use the FieldValueNode instead. But only really needed if this predicate gives problems.

Performance does not seem to be much of an issue at the moment. DCA looks fine and MRVA runs take about 6 mins, the same as other queries like sql injection.

atorralba · 2024-01-30T10:55:22Z

java/ql/lib/semmle/code/java/frameworks/android/Layout.qll

+    id_field.getDeclaringType() = r_id and
+    id_field.hasName(name)
+  |
+    DataFlow::localExprFlow(id_field.getAnAccess(), result.getArgument(0))


Consider whether SimpleGlobal flow may be more appropriate (if you think you need more than local flow for tracking the ID).

I've not seen any cases where local flow is insufficient to find the id used by a call to findViewById

atorralba · 2024-01-30T10:59:25Z

java/ql/lib/semmle/code/java/security/SensitiveUiQuery.qll

+}
+
+/** A local flow step that also flows through access to fields containing `View`s */
+private predicate localViewFieldFlowStep(DataFlow::Node node1, DataFlow::Node node2) {


Just to confirm, this is for tracking something like the following, right?

EditText test15; void test1() { test15 = findViewById(R.id.test15); } void test2(String password) { test15.setVisibility(View.INVISIBLE); test15.setText(password); }

Indeed; it was a common FP in one of my MRVA runs for a view defined in xml as textPassword to be stored in a field this way.

subatoi

Looks good, thank you! Only two very small comments 👍

java/ql/src/Security/CWE/CWE-200/AndroidSensitiveTextField.qhelp

Co-authored-by: Ben Ahmady <32935794+subatoi@users.noreply.github.com>

joefarebrother · 2024-02-05T13:38:13Z

@subatoi Those changes have been made - can you re-approve?

subatoi

Thank you!

github-actions bot added documentation Java labels Jan 22, 2024

github-advanced-security bot found potential problems Jan 22, 2024

View reviewed changes

joefarebrother mentioned this pull request Jan 23, 2024

Java: Add query for exposure of sensitive information to android notifications #15281

Merged

joefarebrother force-pushed the android-sensitive-ui-text branch 2 times, most recently from 45f414a to 40cff8c Compare January 24, 2024 14:10

joefarebrother force-pushed the android-sensitive-ui-text branch from 40cff8c to 22d8269 Compare January 24, 2024 14:53

joefarebrother added 8 commits January 29, 2024 16:25

Add sensitive text flow query

5dd0add

Implement checks for calls that may safely mask information

1b13597

Implement checks for parent views being hidden

8582093

Add unit tests + make some fixes

6081f18

Implement checks for elements hidden by their xml attributes

aa78050

Add documentation

8d20162

Fix FPs - consider flow through fields when determining whether a vie…

94075ef

…w is masked, and find more instances of findViewById.

Add change note

3abd670

joefarebrother force-pushed the android-sensitive-ui-text branch from 22d8269 to 3abd670 Compare January 29, 2024 16:33

Add additional test cases

460ffc8

joefarebrother changed the title ~~[Draft] Java: Add query for sensitive data exposed in text fields~~ Java: Add query for sensitive data exposed in text fields Jan 29, 2024

joefarebrother marked this pull request as ready for review January 29, 2024 22:45

joefarebrother requested a review from a team as a code owner January 29, 2024 22:45

atorralba reviewed Jan 30, 2024

View reviewed changes

Address reviews - use SimpleTypeSanitizer and alter qldoc style

9130603

joefarebrother added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Feb 2, 2024

subatoi self-assigned this Feb 2, 2024

subatoi previously approved these changes Feb 2, 2024

View reviewed changes

java/ql/src/Security/CWE/CWE-200/AndroidSensitiveTextField.qhelp Outdated Show resolved Hide resolved

java/ql/src/Security/CWE/CWE-200/AndroidSensitiveTextField.qhelp Outdated Show resolved Hide resolved

Apply suggestions from documentation review

3878192

Co-authored-by: Ben Ahmady <32935794+subatoi@users.noreply.github.com>

joefarebrother dismissed subatoi’s stale review via 3878192 February 2, 2024 17:21

subatoi approved these changes Feb 5, 2024

View reviewed changes

joefarebrother merged commit 525f271 into github:main Feb 5, 2024
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java: Add query for sensitive data exposed in text fields #15396

Java: Add query for sensitive data exposed in text fields #15396

joefarebrother commented Jan 22, 2024 •

edited

Loading

github-actions bot commented Jan 22, 2024

Exposure of sensitive information to notifications

Recommendation

Example

References

github-actions bot commented Jan 22, 2024 •

edited

Loading

github-actions bot commented Jan 24, 2024 •

edited

Loading

Exposure of sensitive information to UI text views.

Recommendation

Example

References

atorralba left a comment

atorralba Jan 30, 2024

joefarebrother Jan 31, 2024

atorralba Feb 1, 2024

atorralba Jan 30, 2024

joefarebrother Jan 31, 2024

atorralba Jan 30, 2024

joefarebrother Jan 31, 2024

atorralba Jan 30, 2024

joefarebrother Jan 31, 2024

subatoi left a comment

joefarebrother commented Feb 5, 2024

subatoi left a comment

Java: Add query for sensitive data exposed in text fields #15396

Java: Add query for sensitive data exposed in text fields #15396

Conversation

joefarebrother commented Jan 22, 2024 • edited Loading

github-actions bot commented Jan 22, 2024

Exposure of sensitive information to notifications

Recommendation

Example

References

github-actions bot commented Jan 22, 2024 • edited Loading

github-actions bot commented Jan 24, 2024 • edited Loading

Exposure of sensitive information to UI text views.

Recommendation

Example

References

atorralba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subatoi left a comment

Choose a reason for hiding this comment

joefarebrother commented Feb 5, 2024

subatoi left a comment

Choose a reason for hiding this comment

joefarebrother commented Jan 22, 2024 •

edited

Loading

github-actions bot commented Jan 22, 2024 •

edited

Loading

github-actions bot commented Jan 24, 2024 •

edited

Loading