Merge pull request #15396 from joefarebrother/android-sensitive-ui-text

Java: Add query for sensitive data exposed in text fields

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

@@ -0,0 +1,65 @@
+/** Provides classes and predicates for working with Android layouts and UI elements. */
+
+import java
+import semmle.code.xml.AndroidManifest
+private import semmle.code.java.dataflow.DataFlow
+
+/** An Android Layout XML file. */
+class AndroidLayoutXmlFile extends XmlFile {
+  AndroidLayoutXmlFile() { this.getRelativePath().matches("%/res/layout/%.xml") }
+}
+
+/** A component declared in an Android layout file. */
+class AndroidLayoutXmlElement extends XmlElement {
+  AndroidLayoutXmlElement() { this.getFile() instanceof AndroidLayoutXmlFile }
+
+  /** Gets the ID of this component, if any. */
+  string getId() { result = this.getAttribute("id").getValue() }
+
+  /** Gets the class of this component. */
+  Class getClass() {
+    this.getName() = "view" and
+    this.getAttribute("class").getValue() = result.getQualifiedName()
+    or
+    this.getName() = result.getQualifiedName()
+    or
+    result.hasQualifiedName(["android.widget", "android.view"], this.getName())
+  }
+}
+
+/** An XML element that represents an editable text field. */
+class AndroidEditableXmlElement extends AndroidLayoutXmlElement {
+  AndroidEditableXmlElement() {
+    this.getClass().getASourceSupertype*().hasQualifiedName("android.widget", "EditText")
+  }
+
+  /** Gets the input type of this field, if any. */
+  string getInputType() { result = this.getAttribute("inputType").(AndroidXmlAttribute).getValue() }
+}
+
+/** A `findViewById` or `requireViewById` method on `Activity` or `View`. */
+private class FindViewMethod extends Method {
+  FindViewMethod() {
+    exists(Method m | this.getAnOverride*() = m |
+      m.hasQualifiedName("android.app", "Activity", ["findViewById", "requireViewById"])
+      or
+      m.hasQualifiedName("android.view", "View", ["findViewById", "requireViewById"])
+      or
+      m.hasQualifiedName("android.app", "Dialog", ["findViewById", "requireViewById"])
+    )
+  }
+}
+
+/** Gets a use of the view that has the given id. (that is, from a call to a method like `findViewById`) */
+MethodCall getAUseOfViewWithId(string id) {
+  exists(string name, NestedClass r_id, Field id_field |
+    id = ["@+id/", "@id/"] + name and
+    result.getMethod() instanceof FindViewMethod and
+    r_id.getEnclosingType().hasName("R") and
+    r_id.hasName("id") and
+    id_field.getDeclaringType() = r_id and
+    id_field.hasName(name)
+  |
+    DataFlow::localExprFlow(id_field.getAnAccess(), result.getArgument(0))
+  )
+}

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

@@ -3,71 +3,7 @@
 import java
 import semmle.code.java.dataflow.DataFlow
 import semmle.code.java.security.SensitiveActions
-import semmle.code.xml.AndroidManifest
-
-/** An Android Layout XML file. */
-private class AndroidLayoutXmlFile extends XmlFile {
-  AndroidLayoutXmlFile() { this.getRelativePath().matches("%/res/layout/%.xml") }
-}
-
-/** A component declared in an Android layout file. */
-class AndroidLayoutXmlElement extends XmlElement {
-  AndroidXmlAttribute id;
-
-  AndroidLayoutXmlElement() {
-    this.getFile() instanceof AndroidLayoutXmlFile and
-    id = this.getAttribute("id")
-  }
-
-  /** Gets the ID of this component. */
-  string getId() { result = id.getValue() }
-
-  /** Gets the class of this component. */
-  Class getClass() {
-    this.getName() = "view" and
-    this.getAttribute("class").getValue() = result.getQualifiedName()
-    or
-    this.getName() = result.getQualifiedName()
-    or
-    result.hasQualifiedName(["android.widget", "android.view"], this.getName())
-  }
-}
-
-/** An XML element that represents an editable text field. */
-class AndroidEditableXmlElement extends AndroidLayoutXmlElement {
-  AndroidEditableXmlElement() {
-    this.getClass().getASourceSupertype*().hasQualifiedName("android.widget", "EditText")
-  }
-
-  /** Gets the input type of this field, if any. */
-  string getInputType() { result = this.getAttribute("inputType").(AndroidXmlAttribute).getValue() }
-}
-
-/** A `findViewById` or `requireViewById` method on `Activity` or `View`. */
-private class FindViewMethod extends Method {
-  FindViewMethod() {
-    this.hasQualifiedName("android.view", "View", ["findViewById", "requireViewById"])
-    or
-    exists(Method m |
-      m.hasQualifiedName("android.app", "Activity", ["findViewById", "requireViewById"]) and
-      this = m.getAnOverride*()
-    )
-  }
-}
-
-/** Gets a use of the view that has the given id. */
-private MethodCall getAUseOfViewWithId(string id) {
-  exists(string name, NestedClass r_id, Field id_field |
-    id = "@+id/" + name and
-    result.getMethod() instanceof FindViewMethod and
-    r_id.getEnclosingType().hasName("R") and
-    r_id.hasName("id") and
-    id_field.getDeclaringType() = r_id and
-    id_field.hasName(name)
-  |
-    DataFlow::localExprFlow(id_field.getAnAccess(), result.getArgument(0))
-  )
-}
+import semmle.code.java.frameworks.android.Layout
 
 /** Gets the argument of a use of `setInputType` called on the view with the given id. */
 private Argument setInputTypeForId(string id) {

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

@@ -4,6 +4,8 @@ import java
 private import semmle.code.java.dataflow.ExternalFlow
 private import semmle.code.java.dataflow.TaintTracking
 private import semmle.code.java.security.SensitiveActions
+private import semmle.code.java.frameworks.android.Layout
+private import semmle.code.java.security.Sanitizers
 
 /** A configuration for tracking sensitive information to system notifications. */
 private module NotificationTrackingConfig implements DataFlow::ConfigSig {
@@ -20,3 +22,94 @@ private module NotificationTrackingConfig implements DataFlow::ConfigSig {
 
 /** Taint tracking flow for sensitive data flowing to system notifications. */
 module NotificationTracking = TaintTracking::Global<NotificationTrackingConfig>;
+
+/** A call to a method that sets the text of a `TextView`. */
+private class SetTextCall extends MethodCall {
+  SetTextCall() {
+    this.getMethod()
+        .getAnOverride*()
+        .hasQualifiedName("android.widget", "TextView", ["append", "setText", "setHint"]) and
+    (
+      this.getMethod()
+          .getParameter(0)
+          .getType()
+          .(RefType)
+          .hasQualifiedName("java.lang", "CharSequence")
+      or
+      this.getMethod().getParameter(0).getType().(Array).getElementType() instanceof CharacterType
+    )
+  }
+
+  /** Gets the string argument of this call. */
+  Expr getStringArgument() { result = this.getArgument(0) }
+}
+
+/** A call to a method indicating that the contents of a UI element are safely masked. */
+private class MaskCall extends MethodCall {
+  MaskCall() {
+    this.getMethod().getAnOverride*().hasQualifiedName("android.widget", "TextView", "setInputType")
+    or
+    this.getMethod().getAnOverride*().hasQualifiedName("android.view", "View", "setVisibility")
+  }
+}
+
+/** A configuration for tracking sensitive information to text fields. */
+private module TextFieldTrackingConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node src) { src.asExpr() instanceof SensitiveExpr }
+
+  predicate isSink(DataFlow::Node sink) {
+    exists(SetTextCall call |
+      sink.asExpr() = call.getStringArgument() and
+      not setTextCallIsMasked(call)
+    )
+  }
+
+  predicate isBarrier(DataFlow::Node node) { node instanceof SimpleTypeSanitizer }
+
+  predicate isBarrierIn(DataFlow::Node node) { isSource(node) }
+}
+
+/** A local flow step that also flows through access to fields containing `View`s */
+private predicate localViewFieldFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
+  DataFlow::localFlowStep(node1, node2)
+  or
+  exists(Field f |
+    f.getType().(Class).getASupertype*().hasQualifiedName("android.view", "View") and
+    node1.asExpr() = f.getAnAccess().(FieldWrite).getASource() and
+    node2.asExpr() = f.getAnAccess().(FieldRead)
+  )
+}
+
+/** Holds if data can flow from `node1` to `node2` with local flow steps as well as flow through fields containing `View`s */
+private predicate localViewFieldFlow(DataFlow::Node node1, DataFlow::Node node2) {
+  localViewFieldFlowStep*(node1, node2)
+}
+
+/** 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) {
+  localViewFieldFlow(DataFlow::exprNode(e1), DataFlow::exprNode(e2))
+}
+
+/** Holds if the given view may be properly masked. */
+private predicate viewIsMasked(AndroidLayoutXmlElement view) {
+  localViewFieldExprFlow(getAUseOfViewWithId(view.getId()), any(MaskCall mcall).getQualifier())
+  or
+  view.getAttribute("inputType")
+      .(AndroidXmlAttribute)
+      .getValue()
+      .regexpMatch("(?i).*(text|number)(web)?password.*")
+  or
+  view.getAttribute("visibility").(AndroidXmlAttribute).getValue().toLowerCase() =
+    ["invisible", "gone"]
+}
+
+/** Holds if the qualifier of `call` may be properly masked. */
+private predicate setTextCallIsMasked(SetTextCall call) {
+  exists(AndroidLayoutXmlElement view |
+    localViewFieldExprFlow(getAUseOfViewWithId(view.getId()), call.getQualifier()) and
+    viewIsMasked(view.getParent*())
+  )
+}
+
+/** Taint tracking flow for sensitive data flowing to text fields. */
+module TextFieldTracking = TaintTracking::Global<TextFieldTrackingConfig>;

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

@@ -0,0 +1,2 @@
+TextView pwView = getViewById(R.id.pw_text);
+pwView.setText("Your password is: " + password);

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

@@ -0,0 +1,38 @@
+<!DOCTYPE qhelp PUBLIC
+ "-//Semmle//qhelp//EN"
+ "qhelp.dtd">
+<qhelp>
+  <overview>
+    <p>
+      Sensitive information such as passwords should not be displayed in UI components unless explicitly required, to mitigate shoulder-surfing attacks.
+    </p>
+  </overview>
+
+  <recommendation>
+    <p>
+    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.
+    Otherwise, sensitive data that must be displayed should be hidden by default, and only revealed based on an explicit user action.
+    </p>
+  </recommendation>
+
+  <example>
+    <p>
+    In the following (bad) case, sensitive information in <code>password</code> is exposed to the <code>TextView</code>.
+    </p>
+
+    <sample src="AndroidSensitiveTextBad.java"/>
+
+    <p>
+    In the following (good) case, the user must press a button to reveal sensitive information.
+    </p>
+
+    <sample src="AndroidSensitiveTextGood.java"/>
+  </example>
+
+  <references>
+    <li>
+      OWASP Mobile Application Security: <a href="https://mas.owasp.org/MASTG/Android/0x05d-Testing-Data-Storage/#ui-components">Android Data Storage - UI Components</a> 
+    </li>
+  </references>
+
+</qhelp>

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

@@ -0,0 +1,20 @@
+/**
+ * @name Exposure of sensitive information to UI text views.
+ * @id java/android/sensitive-text
+ * @kind path-problem
+ * @description Sensitive information displayed in UI text views should be properly masked.
+ * @problem.severity warning
+ * @precision medium
+ * @security-severity 6.5
+ * @tags security
+ *       external/cwe/cwe-200
+ */
+
+import java
+import java
+import semmle.code.java.security.SensitiveUiQuery
+import TextFieldTracking::PathGraph
+
+from TextFieldTracking::PathNode source, TextFieldTracking::PathNode sink
+where TextFieldTracking::flowPath(source, sink)
+select sink, source, sink, "This $@ is exposed in a text view.", source, "sensitive information"

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

@@ -0,0 +1,10 @@
+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);
+    }
+});

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

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* 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

@@ -0,0 +1,5 @@
+<?xml version="1.0" encoding="utf-8"?>
+<manifest xmlns:android="http://schemas.android.com/apk/res/android"
+    xmlns:tools="http://schemas.android.com/tools"
+    package="com.example.test">
+</manifest>

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

@@ -0,0 +1,24 @@
+package com.example.test;
+
+public final class R {
+    public static final class id {
+        public static final int test1 = 1;
+        public static final int test2 = 2;
+        public static final int test3 = 3;
+        public static final int test4 = 4;
+        public static final int test5 = 5;
+        public static final int test6 = 6;
+        public static final int test7 = 7;
+        public static final int test8 = 8;
+        public static final int test9 = 9;
+        public static final int test10 = 10;
+        public static final int test11 = 11;
+        public static final int test12 = 12;
+        public static final int test13 = 13;
+        public static final int test14 = 14;
+    }
+
+    public static final class string {
+        public static final int password_prompt = 0;
+    }
+}

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

@@ -0,0 +1,77 @@
+package com.example.test;
+
+import android.app.Activity;
+import android.widget.EditText;
+import android.widget.TextView;
+import android.widget.LinearLayout;
+import android.view.View;
+import android.text.InputType;
+
+class Test extends Activity {
+    void test(String password) {
+        EditText test1 = findViewById(R.id.test1);
+        // BAD: Exposing sensitive data to text view
+        test1.setText(password); // $sensitive-text
+        test1.setHint(password); // $sensitive-text
+        test1.append(password); // $sensitive-text
+        // GOOD: resource constant is not sensitive info
+        test1.setText(R.string.password_prompt); 
+
+        // GOOD: Visibility is dynamically set
+        TextView test2 = findViewById(R.id.test2);
+        test2.setVisibility(View.INVISIBLE);
+        test2.setText(password);
+
+        // GOOD: Input type is dynamically set
+        EditText test3 = findViewById(R.id.test3);
+        test3.setInputType(InputType.TYPE_CLASS_TEXT | InputType.TYPE_TEXT_VARIATION_PASSWORD);
+        test3.setText(password);
+
+        // GOOD: Visibility of parent is dynamically set
+        LinearLayout test4 = findViewById(R.id.test4);
+        TextView test5 = findViewById(R.id.test5);
+        test4.setVisibility(View.INVISIBLE);
+        test5.setText(password);
+
+        // GOOD: Input type set to textPassword in XML
+        EditText test6 = findViewById(R.id.test6);
+        test6.setText(password);
+
+        // GOOD: Input type set to textWebPassword in XML
+        EditText test7 = findViewById(R.id.test7);
+        test7.setText(password);
+
+        // GOOD: Input type set to numberPassword in XML
+        EditText test8 = findViewById(R.id.test8);
+        test8.setText(password);
+
+        // BAD: Input type set to textVisiblePassword in XML, which is not hidden
+        EditText test9 = findViewById(R.id.test9);
+        test9.setText(password); // $sensitive-text
+
+        // GOOD: Visibility set to invisible in XML
+        EditText test10 = findViewById(R.id.test10);
+        test10.setText(password);
+
+        // GOOD: Visibility set to gone in XML
+        EditText test11 = findViewById(R.id.test11);
+        test11.setText(password);
+
+        // GOOD: Visibility of parent set to invisible in XML
+        EditText test12 = findViewById(R.id.test12);
+        test12.setText(password);
+
+        // GOOD: Input type set to textPassword in XML
+        EditText test13 = findViewById(R.id.test13);
+        test13.setText(password);
+
+        test14 = findViewById(R.id.test14);
+    }
+
+    EditText test14;
+
+    void test2(String password) {
+        // GOOD: Input type set to textPassword in XML
+        test14.setText(password);
+    }
+}

java/ql/test/query-tests/security/CWE-200/semmle/tests/SensitiveTextView/options

@@ -0,0 +1 @@
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../../../stubs/google-android-9.0.0

java/ql/test/query-tests/security/CWE-200/semmle/tests/SensitiveTextView/res/layout/Test.xml

@@ -0,0 +1,61 @@
+<?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">
+
+
+    <EditText
+        android:id="@+id/test1"
+        android:inputType="text"/>
+
+    <TextView
+        android:id="@+id/test2"/>
+
+    <EditText
+        android:id="@+id/test3"/>
+
+    <LinearLayout
+        android:id="@+id/test4">
+        <TextView
+            android:id="@+id/test5"/>
+    </LinearLayout>
+
+    <EditText
+        android:id="@+id/test6"
+        android:inputType="textPassword"/>
+
+    <EditText
+        android:id="@+id/test7"
+        android:inputType="textWebPassword"/>
+
+    <EditText
+        android:id="@+id/test8"
+        android:inputType="numberPassword"/>
+
+    <EditText
+        android:id="@+id/test9"
+        android:inputType="textVisiblePassword"/>
+
+    <EditText
+        android:id="@+id/test10"
+        android:visibility="invisible"/>
+
+    <EditText
+        android:id="@+id/test11"
+        android:visibility="gone"/>
+
+    <LinearLayout
+        android:visibility="invisible">
+        <TextView
+            android:id="@+id/test12"/>
+    </LinearLayout>
+
+    <EditText
+        android:id="@id/test13"
+        android:inputType="textPassword"/>
+
+    <EditText
+        android:id="@+id/test14"
+        android:inputType="textPassword"/>
+
+</LinearLayout>

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

@@ -0,0 +1,2 @@
+testFailures
+failures

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

@@ -0,0 +1,19 @@
+import java
+import TestUtilities.InlineExpectationsTest
+import semmle.code.java.dataflow.DataFlow
+import semmle.code.java.security.SensitiveUiQuery
+
+module SensitiveTextTest implements TestSig {
+  string getARelevantTag() { result = "sensitive-text" }
+
+  predicate hasActualResult(Location location, string element, string tag, string value) {
+    tag = "sensitive-text" and
+    exists(DataFlow::Node sink | TextFieldTracking::flowTo(sink) |
+      sink.getLocation() = location and
+      element = sink.toString() and
+      value = ""
+    )
+  }
+}
+
+import MakeTest<SensitiveTextTest>

java/ql/test/stubs/google-android-9.0.0/android/widget/LinearLayout.java

@@ -0,0 +1,68 @@
+// Generated automatically from android.widget.LinearLayout for testing purposes
+
+package android.widget;
+
+import android.content.Context;
+import android.graphics.Canvas;
+import android.graphics.drawable.Drawable;
+import android.util.AttributeSet;
+import android.view.ViewGroup;
+
+public class LinearLayout extends ViewGroup
+{
+    protected LinearLayout() {}
+    protected LinearLayout.LayoutParams generateDefaultLayoutParams(){ return null; }
+    protected LinearLayout.LayoutParams generateLayoutParams(ViewGroup.LayoutParams p0){ return null; }
+    protected boolean checkLayoutParams(ViewGroup.LayoutParams p0){ return false; }
+    protected void onDraw(Canvas p0){}
+    protected void onLayout(boolean p0, int p1, int p2, int p3, int p4){}
+    protected void onMeasure(int p0, int p1){}
+    public CharSequence getAccessibilityClassName(){ return null; }
+    public Drawable getDividerDrawable(){ return null; }
+    public LinearLayout(Context p0){}
+    public LinearLayout(Context p0, AttributeSet p1){}
+    public LinearLayout(Context p0, AttributeSet p1, int p2){}
+    public LinearLayout(Context p0, AttributeSet p1, int p2, int p3){}
+    public LinearLayout.LayoutParams generateLayoutParams(AttributeSet p0){ return null; }
+    public boolean isBaselineAligned(){ return false; }
+    public boolean isMeasureWithLargestChildEnabled(){ return false; }
+    public boolean shouldDelayChildPressedState(){ return false; }
+    public float getWeightSum(){ return 0; }
+    public int getBaseline(){ return 0; }
+    public int getBaselineAlignedChildIndex(){ return 0; }
+    public int getDividerPadding(){ return 0; }
+    public int getGravity(){ return 0; }
+    public int getOrientation(){ return 0; }
+    public int getShowDividers(){ return 0; }
+    public static int HORIZONTAL = 0;
+    public static int SHOW_DIVIDER_BEGINNING = 0;
+    public static int SHOW_DIVIDER_END = 0;
+    public static int SHOW_DIVIDER_MIDDLE = 0;
+    public static int SHOW_DIVIDER_NONE = 0;
+    public static int VERTICAL = 0;
+    public void onRtlPropertiesChanged(int p0){}
+    public void setBaselineAligned(boolean p0){}
+    public void setBaselineAlignedChildIndex(int p0){}
+    public void setDividerDrawable(Drawable p0){}
+    public void setDividerPadding(int p0){}
+    public void setGravity(int p0){}
+    public void setHorizontalGravity(int p0){}
+    public void setMeasureWithLargestChildEnabled(boolean p0){}
+    public void setOrientation(int p0){}
+    public void setShowDividers(int p0){}
+    public void setVerticalGravity(int p0){}
+    public void setWeightSum(float p0){}
+    static public class LayoutParams extends ViewGroup.MarginLayoutParams
+    {
+        protected LayoutParams() {}
+        public LayoutParams(Context p0, AttributeSet p1){}
+        public LayoutParams(LinearLayout.LayoutParams p0){}
+        public LayoutParams(ViewGroup.LayoutParams p0){}
+        public LayoutParams(ViewGroup.MarginLayoutParams p0){}
+        public LayoutParams(int p0, int p1){}
+        public LayoutParams(int p0, int p1, float p2){}
+        public String debug(String p0){ return null; }
+        public float weight = 0;
+        public int gravity = 0;
+    }
+}