github · aschackmull · Jan 8, 2021 · Nov 16, 2020 · Nov 17, 2020 · Nov 19, 2020
diff --git a/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.java b/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.java
@@ -0,0 +1,39 @@
+public void testSetSharedPrefs(Context context, String name, String password)
+{
+	{
+		// BAD - save sensitive information in cleartext
+		SharedPreferences sharedPrefs = context.getSharedPreferences("user_prefs", Context.MODE_PRIVATE);
+		Editor editor = sharedPrefs.edit();
+		editor.putString("name", name);
+		editor.putString("password", password);
+		editor.commit();
+	}
+
+	{
+		// GOOD - save sensitive information in encrypted format
+		SharedPreferences sharedPrefs = context.getSharedPreferences("user_prefs", Context.MODE_PRIVATE);
+		Editor editor = sharedPrefs.edit();
+		editor.putString("name", encrypt(name));
+		editor.putString("password", encrypt(password));
+		editor.commit();
+	}
+
+	{
+		// GOOD - save sensitive information using the built-in `EncryptedSharedPreferences` class in androidx.
+		MasterKey masterKey = new MasterKey.Builder(context, MasterKey.DEFAULT_MASTER_KEY_ALIAS)
+			.setKeyScheme(MasterKey.KeyScheme.AES256_GCM)
+			.build();
+
+		SharedPreferences sharedPreferences = EncryptedSharedPreferences.create(
+			context,
+			"secret_shared_prefs",
+			masterKey,
+			EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV,
+			EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM);
+
+		SharedPreferences.Editor editor = sharedPreferences.edit();
+		editor.putString("name", name);
+		editor.putString("password", password);
+		editor.commit();
+	}
+}
diff --git a/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.qhelp b/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.qhelp
@@ -0,0 +1,40 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+  <overview>
+    <p>
+      <code>SharedPreferences</code> 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 the user's profile. However, sensitive information should 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. 
+    </p>
+  </overview>
+
+  <recommendation>
+    <p>
+      Use the <code>EncryptedSharedPreferences</code> API or other encryption algorithms for storing sensitive information.
+    </p>
+  </recommendation>
+
+  <example>
+    <p>
+      In the first example, sensitive user information is stored in cleartext.
+    </p>
+
+    <p>
+      In the second and third examples, the code encrypts sensitive information before saving it to the device.
+    </p>
+    <sample src="ClearTextStorageSharedPrefs.java" />
+  </example>
+
+  <references>
+    <li>
+      CWE:
+      <a href="https://cwe.mitre.org/data/definitions/312.html">CWE-312: Cleartext Storage of Sensitive Information</a>
+    </li>
+    <li>
+      Android Developers:
+      <a href="https://developer.android.com/topic/security/data">Work with data more securely</a>
+    </li>
+    <li>
+      ProAndroidDev:
+      <a href="https://proandroiddev.com/encrypted-preferences-in-android-af57a89af7c8">Encrypted Preferences in Android</a>
+    </li>
+  </references>
+</qhelp>
diff --git a/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.ql b/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.ql
@@ -0,0 +1,22 @@
+/**
+ * @name Cleartext storage of sensitive information using `SharedPreferences` on Android
+ * @description Cleartext Storage of Sensitive Information using SharedPreferences on Android allows access for users with root privileges or unexpected exposure from chained vulnerabilities.
+ * @kind problem
+ * @id java/android/cleartext-storage-shared-prefs
+ * @tags security
+ *       external/cwe/cwe-312
+ */
+
+import java
+import SensitiveStorage
+
+from SensitiveSource data, SharedPreferencesEditor s, Expr input, Expr store
+where
+  input = s.getAnInput() and
+  store = s.getAStore() and
+  data.flowsToCached(input) and
+  // Exclude results in test code.
+  not testMethod(store.getEnclosingCallable()) and
+  not testMethod(data.getEnclosingCallable())
+select store, "'SharedPreferences' class $@ containing $@ is stored here. Data was added $@.", s,
+  s.toString(), data, "sensitive data", input, "here"
diff --git a/java/ql/src/Security/CWE/CWE-312/SensitiveStorage.qll b/java/ql/src/Security/CWE/CWE-312/SensitiveStorage.qll
@@ -1,9 +1,11 @@
 import java
 import semmle.code.java.frameworks.Properties
 import semmle.code.java.frameworks.JAXB
+import semmle.code.java.frameworks.android.SharedPreferences
 import semmle.code.java.dataflow.TaintTracking
 import semmle.code.java.dataflow.DataFlow3
 import semmle.code.java.dataflow.DataFlow4
+import semmle.code.java.dataflow.DataFlow5
 import semmle.code.java.security.SensitiveActions
 
 /** Test code filter. */
@@ -28,6 +30,11 @@ private class SensitiveSourceFlowConfig extends TaintTracking::Configuration {
       m.getMethod() instanceof PropertiesSetPropertyMethod and sink.asExpr() = m.getArgument(1)
     )
     or
+    exists(MethodAccess m |
+      m.getMethod() instanceof SharedPreferences::SharedPreferencesSetMethod and
+      sink.asExpr() = m.getArgument(1)
+    )
+    or
     sink.asExpr() = getInstanceInput(_, _)
   }
 
@@ -243,3 +250,111 @@ class Marshallable extends ClassStore {
     )
   }
 }
+
+/* Holds if the method call is a setter method of `SharedPreferences`. */
+private predicate sharedPreferencesInput(DataFlow::Node sharedPrefs, Expr input) {
+  exists(MethodAccess m |
+    m.getMethod() instanceof SharedPreferences::SharedPreferencesSetMethod and
+    input = m.getArgument(1) and
+    not exists(EncryptedValueFlowConfig conf | conf.hasFlow(_, DataFlow::exprNode(input))) and
+    sharedPrefs.asExpr() = m.getQualifier()
+  )
+}
+
+/* Holds if the method call is the store method of `SharedPreferences`. */
+private predicate sharedPreferencesStore(DataFlow::Node sharedPrefs, Expr store) {
+  exists(MethodAccess m |
+    m.getMethod() instanceof SharedPreferences::SharedPreferencesStoreMethod and
+    store = m and
+    sharedPrefs.asExpr() = m.getQualifier()
+  )
+}
+
+/* Flow from `SharedPreferences` to either a setter or a store method. */
+class SharedPreferencesFlowConfig extends TaintTracking::Configuration {
+  SharedPreferencesFlowConfig() { this = "SensitiveStorage::SharedPreferencesFlowConfig" }
+
+  override predicate isSource(DataFlow::Node src) {
+    src.asExpr() instanceof SharedPreferencesEditor
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    sharedPreferencesInput(sink, _) or
+    sharedPreferencesStore(sink, _)
+  }
+}
+
+/**
+ * Method call of encrypting sensitive information.
+ * As there are various implementations of encryption (reversible and non-reversible) from both JDK and third parties, this class simply checks method name to take a best guess to reduce false positives.
+ */
+class EncryptedSensitiveMethodAccess extends MethodAccess {
+  EncryptedSensitiveMethodAccess() {
+    getMethod().getName().toLowerCase().matches(["%encrypt%", "%hash%"])
+  }
+}
+
+/* Flow configuration of encrypting sensitive information. */
+class EncryptedValueFlowConfig extends DataFlow5::Configuration {
+  EncryptedValueFlowConfig() { this = "SensitiveStorage::EncryptedValueFlowConfig" }
+
+  override predicate isSource(DataFlow5::Node src) {
+    exists(EncryptedSensitiveMethodAccess ema | src.asExpr() = ema.getAnArgument())
+  }
+
+  override predicate isSink(DataFlow5::Node sink) {
+    exists(MethodAccess ma |
+      ma.getMethod() instanceof SharedPreferences::SharedPreferencesSetMethod and
+      sink.asExpr() = ma.getArgument(1)
+    )
+  }
+
+  override predicate isAdditionalFlowStep(DataFlow5::Node n1, DataFlow5::Node n2) {
+    exists(EncryptedSensitiveMethodAccess ema |
+      n1.asExpr() = ema.getAnArgument() and
+      n2.asExpr() = ema
+    )
+  }
+}
+
+/* Flow from the create method of `androidx.security.crypto.EncryptedSharedPreferences` to its instance. */
+private class EncryptedSharedPrefFlowConfig extends DataFlow3::Configuration {
+  EncryptedSharedPrefFlowConfig() { this = "SensitiveStorage::EncryptedSharedPrefFlowConfig" }
+
+  override predicate isSource(DataFlow::Node src) {
+    src.asExpr().(MethodAccess).getMethod() instanceof
+      SharedPreferences::EncryptedSharedPrefsCreateMethod
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    sink.asExpr().getType() instanceof SharedPreferences::TypeSharedPreferences
+  }
+}
+
+/** The call to get a `SharedPreferences.Editor` object, which can set shared preferences or be stored to device. */
+class SharedPreferencesEditor extends MethodAccess {
+  SharedPreferencesEditor() {
+    this.getMethod() instanceof SharedPreferences::SharedPreferencesGetEditorMethod and
+    not exists(
+      EncryptedSharedPrefFlowConfig config // not exists `SharedPreferences sharedPreferences = EncryptedSharedPreferences.create(...)`
+    |
+      config.hasFlow(_, DataFlow::exprNode(this.getQualifier()))
+    )
+  }
+
+  /** Gets an input, for example `input` in `editor.putString("password", password);`. */
+  Expr getAnInput() {
+    exists(SharedPreferencesFlowConfig conf, DataFlow::Node n |
+      sharedPreferencesInput(n, result) and
+      conf.hasFlow(DataFlow::exprNode(this), n)
+    )
+  }
+
+  /** Gets a store, for example `editor.commit();`. */
+  Expr getAStore() {
+    exists(SharedPreferencesFlowConfig conf, DataFlow::Node n |
+      sharedPreferencesStore(n, result) and
+      conf.hasFlow(DataFlow::exprNode(this), n)
+    )
+  }
+}
@@ -0,0 +1,64 @@
+import semmle.code.java.Type
+
+/* Definitions related to `android.content.SharedPreferences`. */
+module SharedPreferences {
+  /* The interface `android.content.SharedPreferences` */
+  class TypeSharedPreferences extends Interface {
+    TypeSharedPreferences() { hasQualifiedName("android.content", "SharedPreferences") }
+  }
+
+  /* The class `androidx.security.crypto.EncryptedSharedPreferences`, which implements `SharedPreferences` with encryption support. */
+  class TypeEncryptedSharedPreferences extends Class {
+    TypeEncryptedSharedPreferences() {
+      hasQualifiedName("androidx.security.crypto", "EncryptedSharedPreferences")
+    }
+  }
+
+  /* The create method of `androidx.security.crypto.EncryptedSharedPreferences` */
+  class EncryptedSharedPrefsCreateMethod extends Method {
+    EncryptedSharedPrefsCreateMethod() {
+      getDeclaringType() instanceof TypeEncryptedSharedPreferences and
+      hasName("create")
+    }
+  }
+
+  /* A getter method of `android.content.SharedPreferences`. */
+  class SharedPreferencesGetMethod extends Method {
+    SharedPreferencesGetMethod() {
+      getDeclaringType() instanceof TypeSharedPreferences and
+      getName().matches("get%")
+    }
+  }
+
+  /* Returns `android.content.SharedPreferences.Editor` from the `edit` call of `android.content.SharedPreferences`. */
+  class SharedPreferencesGetEditorMethod extends Method {
+    SharedPreferencesGetEditorMethod() {
+      getDeclaringType() instanceof TypeSharedPreferences and
+      hasName("edit") and
+      getReturnType() instanceof TypeSharedPreferencesEditor
+    }
+  }
+
+  /* Definitions related to `android.content.SharedPreferences.Editor`. */
+  class TypeSharedPreferencesEditor extends Interface {
+    TypeSharedPreferencesEditor() {
+      hasQualifiedName("android.content", "SharedPreferences$Editor")
+    }
+  }
+
+  /* A setter method for `android.content.SharedPreferences`. */
+  class SharedPreferencesSetMethod extends Method {
+    SharedPreferencesSetMethod() {
+      getDeclaringType() instanceof TypeSharedPreferencesEditor and
+      getName().matches("put%")
+    }
+  }
+
+  /* A setter method for `android.content.SharedPreferences`. */
+  class SharedPreferencesStoreMethod extends Method {
+    SharedPreferencesStoreMethod() {
+      getDeclaringType() instanceof TypeSharedPreferencesEditor and
+      hasName(["commit", "apply"])
+    }
+  }
+}
diff --git a/java/ql/test/query-tests/security/CWE-312/semmle/tests/ClearTextStorageSharedPrefs.expected b/java/ql/test/query-tests/security/CWE-312/semmle/tests/ClearTextStorageSharedPrefs.expected
@@ -0,0 +1 @@
+| ClearTextStorageSharedPrefs.java:19:3:19:17 | commit(...) | 'SharedPreferences' class $@ containing $@ is stored here. Data was added $@. | ClearTextStorageSharedPrefs.java:16:19:16:36 | edit(...) | edit(...) | ClearTextStorageSharedPrefs.java:18:32:18:39 | password | sensitive data | ClearTextStorageSharedPrefs.java:18:32:18:39 | password | here |
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		\| ClearTextStorageSharedPrefs.java:19:3:19:17 \| commit(...) \| 'SharedPreferences' class $@ containing $@ is stored here. Data was added $@. \| ClearTextStorageSharedPrefs.java:16:19:16:36 \| edit(...) \| edit(...) \| ClearTextStorageSharedPrefs.java:18:32:18:39 \| password \| sensitive data \| ClearTextStorageSharedPrefs.java:18:32:18:39 \| password \| here \|