Skip to content

Commit 525f271

Browse files
joefarebrotherjoefarebrother
authored
Merge pull request github#15396 from joefarebrother/android-sensitive-ui-text
Java: Add query for sensitive data exposed in text fields
2 parents 6fbbb82 + 3878192 commit 525f271

File tree

16 files changed

+490
-65
lines changed

16 files changed

+490
-65
lines changed

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

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
/** Provides classes and predicates for working with Android layouts and UI elements. */
2+
3+
import java
4+
import semmle.code.xml.AndroidManifest
5+
private import semmle.code.java.dataflow.DataFlow
6+
7+
/** An Android Layout XML file. */
8+
class AndroidLayoutXmlFile extends XmlFile {
9+
AndroidLayoutXmlFile() { this.getRelativePath().matches("%/res/layout/%.xml") }
10+
}
11+
12+
/** A component declared in an Android layout file. */
13+
class AndroidLayoutXmlElement extends XmlElement {
14+
AndroidLayoutXmlElement() { this.getFile() instanceof AndroidLayoutXmlFile }
15+
16+
/** Gets the ID of this component, if any. */
17+
string getId() { result = this.getAttribute("id").getValue() }
18+
19+
/** Gets the class of this component. */
20+
Class getClass() {
21+
this.getName() = "view" and
22+
this.getAttribute("class").getValue() = result.getQualifiedName()
23+
or
24+
this.getName() = result.getQualifiedName()
25+
or
26+
result.hasQualifiedName(["android.widget", "android.view"], this.getName())
27+
}
28+
}
29+
30+
/** An XML element that represents an editable text field. */
31+
class AndroidEditableXmlElement extends AndroidLayoutXmlElement {
32+
AndroidEditableXmlElement() {
33+
this.getClass().getASourceSupertype*().hasQualifiedName("android.widget", "EditText")
34+
}
35+
36+
/** Gets the input type of this field, if any. */
37+
string getInputType() { result = this.getAttribute("inputType").(AndroidXmlAttribute).getValue() }
38+
}
39+
40+
/** A `findViewById` or `requireViewById` method on `Activity` or `View`. */
41+
private class FindViewMethod extends Method {
42+
FindViewMethod() {
43+
exists(Method m | this.getAnOverride*() = m |
44+
m.hasQualifiedName("android.app", "Activity", ["findViewById", "requireViewById"])
45+
or
46+
m.hasQualifiedName("android.view", "View", ["findViewById", "requireViewById"])
47+
or
48+
m.hasQualifiedName("android.app", "Dialog", ["findViewById", "requireViewById"])
49+
)
50+
}
51+
}
52+
53+
/** Gets a use of the view that has the given id. (that is, from a call to a method like `findViewById`) */
54+
MethodCall getAUseOfViewWithId(string id) {
55+
exists(string name, NestedClass r_id, Field id_field |
56+
id = ["@+id/", "@id/"] + name and
57+
result.getMethod() instanceof FindViewMethod and
58+
r_id.getEnclosingType().hasName("R") and
59+
r_id.hasName("id") and
60+
id_field.getDeclaringType() = r_id and
61+
id_field.hasName(name)
62+
|
63+
DataFlow::localExprFlow(id_field.getAnAccess(), result.getArgument(0))
64+
)
65+
}

‎java/ql/lib/semmle/code/java/security/SensitiveKeyboardCacheQuery.qll

Lines changed: 1 addition & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -3,71 +3,7 @@
33
import java
44
import semmle.code.java.dataflow.DataFlow
55
import semmle.code.java.security.SensitiveActions
6-
import semmle.code.xml.AndroidManifest
7-
8-
/** An Android Layout XML file. */
9-
private class AndroidLayoutXmlFile extends XmlFile {
10-
AndroidLayoutXmlFile() { this.getRelativePath().matches("%/res/layout/%.xml") }
11-
}
12-
13-
/** A component declared in an Android layout file. */
14-
class AndroidLayoutXmlElement extends XmlElement {
15-
AndroidXmlAttribute id;
16-
17-
AndroidLayoutXmlElement() {
18-
this.getFile() instanceof AndroidLayoutXmlFile and
19-
id = this.getAttribute("id")
20-
}
21-
22-
/** Gets the ID of this component. */
23-
string getId() { result = id.getValue() }
24-
25-
/** Gets the class of this component. */
26-
Class getClass() {
27-
this.getName() = "view" and
28-
this.getAttribute("class").getValue() = result.getQualifiedName()
29-
or
30-
this.getName() = result.getQualifiedName()
31-
or
32-
result.hasQualifiedName(["android.widget", "android.view"], this.getName())
33-
}
34-
}
35-
36-
/** An XML element that represents an editable text field. */
37-
class AndroidEditableXmlElement extends AndroidLayoutXmlElement {
38-
AndroidEditableXmlElement() {
39-
this.getClass().getASourceSupertype*().hasQualifiedName("android.widget", "EditText")
40-
}
41-
42-
/** Gets the input type of this field, if any. */
43-
string getInputType() { result = this.getAttribute("inputType").(AndroidXmlAttribute).getValue() }
44-
}
45-
46-
/** A `findViewById` or `requireViewById` method on `Activity` or `View`. */
47-
private class FindViewMethod extends Method {
48-
FindViewMethod() {
49-
this.hasQualifiedName("android.view", "View", ["findViewById", "requireViewById"])
50-
or
51-
exists(Method m |
52-
m.hasQualifiedName("android.app", "Activity", ["findViewById", "requireViewById"]) and
53-
this = m.getAnOverride*()
54-
)
55-
}
56-
}
57-
58-
/** Gets a use of the view that has the given id. */
59-
private MethodCall getAUseOfViewWithId(string id) {
60-
exists(string name, NestedClass r_id, Field id_field |
61-
id = "@+id/" + name and
62-
result.getMethod() instanceof FindViewMethod and
63-
r_id.getEnclosingType().hasName("R") and
64-
r_id.hasName("id") and
65-
id_field.getDeclaringType() = r_id and
66-
id_field.hasName(name)
67-
|
68-
DataFlow::localExprFlow(id_field.getAnAccess(), result.getArgument(0))
69-
)
70-
}
6+
import semmle.code.java.frameworks.android.Layout
717

728
/** Gets the argument of a use of `setInputType` called on the view with the given id. */
739
private Argument setInputTypeForId(string id) {

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

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ import java
44
private import semmle.code.java.dataflow.ExternalFlow
55
private import semmle.code.java.dataflow.TaintTracking
66
private import semmle.code.java.security.SensitiveActions
7+
private import semmle.code.java.frameworks.android.Layout
8+
private import semmle.code.java.security.Sanitizers
79

810
/** A configuration for tracking sensitive information to system notifications. */
911
private module NotificationTrackingConfig implements DataFlow::ConfigSig {
@@ -20,3 +22,94 @@ private module NotificationTrackingConfig implements DataFlow::ConfigSig {
2022

2123
/** Taint tracking flow for sensitive data flowing to system notifications. */
2224
module NotificationTracking = TaintTracking::Global<NotificationTrackingConfig>;
25+
26+
/** A call to a method that sets the text of a `TextView`. */
27+
private class SetTextCall extends MethodCall {
28+
SetTextCall() {
29+
this.getMethod()
30+
.getAnOverride*()
31+
.hasQualifiedName("android.widget", "TextView", ["append", "setText", "setHint"]) and
32+
(
33+
this.getMethod()
34+
.getParameter(0)
35+
.getType()
36+
.(RefType)
37+
.hasQualifiedName("java.lang", "CharSequence")
38+
or
39+
this.getMethod().getParameter(0).getType().(Array).getElementType() instanceof CharacterType
40+
)
41+
}
42+
43+
/** Gets the string argument of this call. */
44+
Expr getStringArgument() { result = this.getArgument(0) }
45+
}
46+
47+
/** A call to a method indicating that the contents of a UI element are safely masked. */
48+
private class MaskCall extends MethodCall {
49+
MaskCall() {
50+
this.getMethod().getAnOverride*().hasQualifiedName("android.widget", "TextView", "setInputType")
51+
or
52+
this.getMethod().getAnOverride*().hasQualifiedName("android.view", "View", "setVisibility")
53+
}
54+
}
55+
56+
/** A configuration for tracking sensitive information to text fields. */
57+
private module TextFieldTrackingConfig implements DataFlow::ConfigSig {
58+
predicate isSource(DataFlow::Node src) { src.asExpr() instanceof SensitiveExpr }
59+
60+
predicate isSink(DataFlow::Node sink) {
61+
exists(SetTextCall call |
62+
sink.asExpr() = call.getStringArgument() and
63+
not setTextCallIsMasked(call)
64+
)
65+
}
66+
67+
predicate isBarrier(DataFlow::Node node) { node instanceof SimpleTypeSanitizer }
68+
69+
predicate isBarrierIn(DataFlow::Node node) { isSource(node) }
70+
}
71+
72+
/** A local flow step that also flows through access to fields containing `View`s */
73+
private predicate localViewFieldFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
74+
DataFlow::localFlowStep(node1, node2)
75+
or
76+
exists(Field f |
77+
f.getType().(Class).getASupertype*().hasQualifiedName("android.view", "View") and
78+
node1.asExpr() = f.getAnAccess().(FieldWrite).getASource() and
79+
node2.asExpr() = f.getAnAccess().(FieldRead)
80+
)
81+
}
82+
83+
/** Holds if data can flow from `node1` to `node2` with local flow steps as well as flow through fields containing `View`s */
84+
private predicate localViewFieldFlow(DataFlow::Node node1, DataFlow::Node node2) {
85+
localViewFieldFlowStep*(node1, node2)
86+
}
87+
88+
/** Holds if data can flow from `e1` to `e2` with local flow steps as well as flow through fields containing `View`s */
89+
private predicate localViewFieldExprFlow(Expr e1, Expr e2) {
90+
localViewFieldFlow(DataFlow::exprNode(e1), DataFlow::exprNode(e2))
91+
}
92+
93+
/** Holds if the given view may be properly masked. */
94+
private predicate viewIsMasked(AndroidLayoutXmlElement view) {
95+
localViewFieldExprFlow(getAUseOfViewWithId(view.getId()), any(MaskCall mcall).getQualifier())
96+
or
97+
view.getAttribute("inputType")
98+
.(AndroidXmlAttribute)
99+
.getValue()
100+
.regexpMatch("(?i).*(text|number)(web)?password.*")
101+
or
102+
view.getAttribute("visibility").(AndroidXmlAttribute).getValue().toLowerCase() =
103+
["invisible", "gone"]
104+
}
105+
106+
/** Holds if the qualifier of `call` may be properly masked. */
107+
private predicate setTextCallIsMasked(SetTextCall call) {
108+
exists(AndroidLayoutXmlElement view |
109+
localViewFieldExprFlow(getAUseOfViewWithId(view.getId()), call.getQualifier()) and
110+
viewIsMasked(view.getParent*())
111+
)
112+
}
113+
114+
/** Taint tracking flow for sensitive data flowing to text fields. */
115+
module TextFieldTracking = TaintTracking::Global<TextFieldTrackingConfig>;

‎java/ql/src/Security/CWE/CWE-200/AndroidSensitiveTextBad.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
TextView pwView = getViewById(R.id.pw_text);
2+
pwView.setText("Your password is: " + password);

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
Sensitive information such as passwords should not be displayed in UI components unless explicitly required, to mitigate shoulder-surfing attacks.
8+
</p>
9+
</overview>
10+
11+
<recommendation>
12+
<p>
13+
For editable text fields containing sensitive information, the <code>inputType</code> should be set to <code>textPassword</code> or similar to ensure it is properly masked.
14+
Otherwise, sensitive data that must be displayed should be hidden by default, and only revealed based on an explicit user action.
15+
</p>
16+
</recommendation>
17+
18+
<example>
19+
<p>
20+
In the following (bad) case, sensitive information in <code>password</code> is exposed to the <code>TextView</code>.
21+
</p>
22+
23+
<sample src="AndroidSensitiveTextBad.java"/>
24+
25+
<p>
26+
In the following (good) case, the user must press a button to reveal sensitive information.
27+
</p>
28+
29+
<sample src="AndroidSensitiveTextGood.java"/>
30+
</example>
31+
32+
<references>
33+
<li>
34+
OWASP Mobile Application Security: <a href="https://mas.owasp.org/MASTG/Android/0x05d-Testing-Data-Storage/#ui-components">Android Data Storage - UI Components</a>
35+
</li>
36+
</references>
37+
38+
</qhelp>

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
/**
2+
* @name Exposure of sensitive information to UI text views.
3+
* @id java/android/sensitive-text
4+
* @kind path-problem
5+
* @description Sensitive information displayed in UI text views should be properly masked.
6+
* @problem.severity warning
7+
* @precision medium
8+
* @security-severity 6.5
9+
* @tags security
10+
* external/cwe/cwe-200
11+
*/
12+
13+
import java
14+
import java
15+
import semmle.code.java.security.SensitiveUiQuery
16+
import TextFieldTracking::PathGraph
17+
18+
from TextFieldTracking::PathNode source, TextFieldTracking::PathNode sink
19+
where TextFieldTracking::flowPath(source, sink)
20+
select sink, source, sink, "This $@ is exposed in a text view.", source, "sensitive information"

‎java/ql/src/Security/CWE/CWE-200/AndroidSensitiveTextGood.java

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
TextView pwView = findViewById(R.id.pw_text);
2+
pwView.setVisibility(View.INVISIBLE);
3+
pwView.setText("Your password is: " + password);
4+
5+
Button showButton = findViewById(R.id.show_pw_button);
6+
showButton.setOnClickListener(new View.OnClickListener() {
7+
public void onClick(View v) {
8+
pwView.setVisibility(View.VISIBLE);
9+
}
10+
});

‎java/ql/src/change-notes/2024-01-29-android-sensitive-text-field-query.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: newQuery
3+
---
4+
* Added a new query `java/android/sensitive-text` to detect instances of sensitive data being exposed through text fields without being properly masked.

‎java/ql/test/query-tests/security/CWE-200/semmle/tests/SensitiveTextView/AndroidManifest.xml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
<?xml version="1.0" encoding="utf-8"?>
2+
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
3+
xmlns:tools="http://schemas.android.com/tools"
4+
package="com.example.test">
5+
</manifest>

‎java/ql/test/query-tests/security/CWE-200/semmle/tests/SensitiveTextView/R.java

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
package com.example.test;
2+
3+
public final class R {
4+
public static final class id {
5+
public static final int test1 = 1;
6+
public static final int test2 = 2;
7+
public static final int test3 = 3;
8+
public static final int test4 = 4;
9+
public static final int test5 = 5;
10+
public static final int test6 = 6;
11+
public static final int test7 = 7;
12+
public static final int test8 = 8;
13+
public static final int test9 = 9;
14+
public static final int test10 = 10;
15+
public static final int test11 = 11;
16+
public static final int test12 = 12;
17+
public static final int test13 = 13;
18+
public static final int test14 = 14;
19+
}
20+
21+
public static final class string {
22+
public static final int password_prompt = 0;
23+
}
24+
}

0 commit comments

Comments
 (0)