Merge pull request #6468 from atorralba/atorralba/promote-cleartext-sharedprefs

Java: Promote Cleartext storage of sensitive information using SharedPreferences from experimental

Author: atorralba

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

@@ -0,0 +1,79 @@
+/** Provides classes and predicates to reason about cleartext storage in Android's SharedPreferences. */
+
+import java
+import semmle.code.java.dataflow.DataFlow
+import semmle.code.java.frameworks.android.SharedPreferences
+import semmle.code.java.security.CleartextStorageQuery
+
+private class SharedPrefsCleartextStorageSink extends CleartextStorageSink {
+  SharedPrefsCleartextStorageSink() {
+    exists(MethodAccess m |
+      m.getMethod() instanceof PutSharedPreferenceMethod and
+      this.asExpr() = m.getArgument(1)
+    )
+  }
+}
+
+/**
+ * The call to get a `SharedPreferences.Editor` object, which can set shared preferences and be
+ * stored to the device.
+ */
+class SharedPreferencesEditorMethodAccess extends Storable, MethodAccess {
+  SharedPreferencesEditorMethodAccess() {
+    this.getMethod() instanceof GetSharedPreferencesEditorMethod and
+    not DataFlow::localExprFlow(any(MethodAccess ma |
+        ma.getMethod() instanceof CreateEncryptedSharedPreferencesMethod
+      ), this.getQualifier())
+  }
+
+  /** Gets an input, for example `password` in `editor.putString("password", password);`. */
+  override Expr getAnInput() {
+    exists(SharedPreferencesFlowConfig conf, DataFlow::Node editor |
+      sharedPreferencesInput(editor, result) and
+      conf.hasFlow(DataFlow::exprNode(this), editor)
+    )
+  }
+
+  /** Gets a store, for example `editor.commit();`. */
+  override Expr getAStore() {
+    exists(SharedPreferencesFlowConfig conf, DataFlow::Node editor |
+      sharedPreferencesStore(editor, result) and
+      conf.hasFlow(DataFlow::exprNode(this), editor)
+    )
+  }
+}
+
+/**
+ * Holds if `input` is the second argument of a setter method
+ * called on `editor`, which is an instance of `SharedPreferences$Editor`.
+ */
+private predicate sharedPreferencesInput(DataFlow::Node editor, Expr input) {
+  exists(MethodAccess m |
+    m.getMethod() instanceof PutSharedPreferenceMethod and
+    input = m.getArgument(1) and
+    editor.asExpr() = m.getQualifier()
+  )
+}
+
+/**
+ * Holds if `m` is a store method called on `editor`,
+ * which is an instance of `SharedPreferences$Editor`.
+ */
+private predicate sharedPreferencesStore(DataFlow::Node editor, MethodAccess m) {
+  m.getMethod() instanceof StoreSharedPreferenceMethod and
+  editor.asExpr() = m.getQualifier()
+}
+
+/** Flow from `SharedPreferences.Editor` to either a setter or a store method. */
+private class SharedPreferencesFlowConfig extends DataFlow::Configuration {
+  SharedPreferencesFlowConfig() { this = "SharedPreferencesFlowConfig" }
+
+  override predicate isSource(DataFlow::Node src) {
+    src.asExpr() instanceof SharedPreferencesEditorMethodAccess
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    sharedPreferencesInput(sink, _) or
+    sharedPreferencesStore(sink, _)
+  }
+}

java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.java renamed to java/ql/src/Security/CWE/CWE-312/CleartextStorageSharedPrefs.java

@@ -1,7 +1,7 @@
 public void testSetSharedPrefs(Context context, String name, String password)
 {
 	{
-		// BAD - save sensitive information in cleartext
+		// BAD - sensitive information saved in cleartext.
 		SharedPreferences sharedPrefs = context.getSharedPreferences("user_prefs", Context.MODE_PRIVATE);
 		Editor editor = sharedPrefs.edit();
 		editor.putString("name", name);
@@ -10,7 +10,7 @@ public void testSetSharedPrefs(Context context, String name, String password)
 	}
 
 	{
-		// GOOD - save sensitive information in encrypted format
+		// GOOD - save sensitive information encrypted with a custom method.
 		SharedPreferences sharedPrefs = context.getSharedPreferences("user_prefs", Context.MODE_PRIVATE);
 		Editor editor = sharedPrefs.edit();
 		editor.putString("name", encrypt(name));
@@ -19,7 +19,7 @@ public void testSetSharedPrefs(Context context, String name, String password)
 	}
 
 	{
-		// GOOD - save sensitive information using the built-in `EncryptedSharedPreferences` class in androidx.
+		// GOOD - sensitive information saved 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();
@@ -37,3 +37,12 @@ public void testSetSharedPrefs(Context context, String name, String password)
 		editor.commit();
 	}
 }
+
+private static String encrypt(String cleartext) throws Exception {
+	// Use an encryption or hashing algorithm in real world. The demo below just returns its
+	// hash.
+	MessageDigest digest = MessageDigest.getInstance("SHA-256");
+	byte[] hash = digest.digest(cleartext.getBytes(StandardCharsets.UTF_8));
+	String encoded = Base64.getEncoder().encodeToString(hash);
+	return encoded;
+}

java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.qhelp renamed to java/ql/src/Security/CWE/CWE-312/CleartextStorageSharedPrefs.qhelp

@@ -2,7 +2,7 @@
 <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. 
+      <code>SharedPreferences</code> is an Android API that stores application preferences using simple sets of data values. It allows you to easily save, alter, and retrieve the values stored in a user's profile. However, sensitive information should not be saved in cleartext. Otherwise it can be accessed by any process or user in rooted devices, or can be disclosed through chained vulnerabilities, like unexpected access to the private storage through exposed components.
     </p>
   </overview>
 
@@ -24,10 +24,6 @@
   </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>

java/ql/src/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql

@@ -0,0 +1,23 @@
+/**
+ * @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
+ * @problem.severity warning
+ * @precision medium
+ * @id java/android/cleartext-storage-shared-prefs
+ * @tags security
+ *       external/cwe/cwe-312
+ */
+
+import java
+import semmle.code.java.security.CleartextStorageSharedPrefsQuery
+
+from SensitiveSource data, SharedPreferencesEditorMethodAccess s, Expr input, Expr store
+where
+  input = s.getAnInput() and
+  store = s.getAStore() and
+  data.flowsTo(input)
+select store, "'SharedPreferences' class $@ containing $@ is stored $@. Data was added $@.", s,
+  s.toString(), data, "sensitive data", store, "here", input, "here"

java/ql/src/change-notes/2021-08-10-cleartext-storage-sharedprefs-query.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* The query "Cleartext storage of sensitive information using `SharedPreferences` on Android" (`java/android/cleartext-storage-shared-prefs`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @luchua-bc](https://github.com/github/codeql/pull/4675).