Move to experimental and update qldoc

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

@@ -1,22 +0,0 @@
-/**
- * @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"

java/ql/src/Security/CWE/CWE-312/SensitiveStorage.qll

@@ -1,11 +1,9 @@
 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. */
@@ -30,11 +28,6 @@ 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(_, _)
   }
 
@@ -250,111 +243,3 @@ 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)
-    )
-  }
-}

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

@@ -20,7 +20,7 @@
     <p>
       In the second and third examples, the code encrypts sensitive information before saving it to the device.
     </p>
-    <sample src="ClearTextStorageSharedPrefs.java" />
+    <sample src="CleartextStorageSharedPrefs.java" />
   </example>
 
   <references>

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

@@ -0,0 +1,167 @@
+/**
+ * @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 path-problem
+ * @id java/android/cleartext-storage-shared-prefs
+ * @tags security
+ *       external/cwe/cwe-312
+ */
+
+import java
+import semmle.code.java.dataflow.DataFlow4
+import semmle.code.java.dataflow.DataFlow5
+import semmle.code.java.dataflow.TaintTracking
+import semmle.code.java.frameworks.android.Intent
+import semmle.code.java.frameworks.android.SharedPreferences
+import semmle.code.java.security.SensitiveActions
+import DataFlow::PathGraph
+
+/** 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::SetPreferenceMethod 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::StorePreferenceMethod 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 = "CleartextStorageSharedPrefs::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 = "CleartextStorageSharedPrefs::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::SetPreferenceMethod 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 DataFlow4::Configuration {
+  EncryptedSharedPrefFlowConfig() {
+    this = "CleartextStorageSharedPrefs::EncryptedSharedPrefFlowConfig"
+  }
+
+  override predicate isSource(DataFlow4::Node src) {
+    src.asExpr().(MethodAccess).getMethod() instanceof SharedPreferences::CreateEncryptedPrefsMethod
+  }
+
+  override predicate isSink(DataFlow4::Node sink) {
+    sink.asExpr().getType() instanceof SharedPreferences::TypePrefs
+  }
+}
+
+/** 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::GetEditorMethod 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)
+    )
+  }
+}
+
+private class SensitiveSharedPrefsFlowConfig extends TaintTracking::Configuration {
+  SensitiveSharedPrefsFlowConfig() {
+    this = "CleartextStorageSharedPrefs::SensitiveSharedPrefsFlowConfig"
+  }
+
+  override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof SensitiveExpr }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(MethodAccess m |
+      m.getMethod() instanceof SharedPreferences::SetPreferenceMethod and
+      sink.asExpr() = m.getArgument(1)
+    )
+  }
+}
+
+/** Class for shared preferences that may contain 'sensitive' information */
+class SensitiveSharedPrefsSource extends Expr {
+  SensitiveSharedPrefsSource() {
+    // SensitiveExpr is abstract, this lets us inherit from it without
+    // being a technical subclass
+    this instanceof SensitiveExpr
+  }
+
+  /** Holds if this source flows to the `sink`. */
+  predicate flowsTo(Expr sink) {
+    exists(SensitiveSharedPrefsFlowConfig conf |
+      conf.hasFlow(DataFlow::exprNode(this), DataFlow::exprNode(sink))
+    )
+  }
+}
+
+from SensitiveSharedPrefsSource data, SharedPreferencesEditor s, Expr input, Expr store
+where
+  input = s.getAnInput() and
+  store = s.getAStore() and
+  data.flowsTo(input)
+select store, "'SharedPreferences' class $@ containing $@ is stored here. Data was added $@.", s,
+  s.toString(), data, "sensitive data", input, "here"

java/ql/src/semmle/code/java/frameworks/android/SharedPreferences.qll

@@ -1,63 +1,61 @@
 import semmle.code.java.Type
 
-/* Definitions related to `android.content.SharedPreferences`. */
+/** Definitions related to `android.content.SharedPreferences`. */
 module SharedPreferences {
-  /* The interface `android.content.SharedPreferences` */
-  class TypeSharedPreferences extends Interface {
-    TypeSharedPreferences() { hasQualifiedName("android.content", "SharedPreferences") }
+  /** The interface `android.content.SharedPreferences` */
+  class TypePrefs extends Interface {
+    TypePrefs() { hasQualifiedName("android.content", "SharedPreferences") }
   }
 
-  /* The class `androidx.security.crypto.EncryptedSharedPreferences`, which implements `SharedPreferences` with encryption support. */
-  class TypeEncryptedSharedPreferences extends Class {
-    TypeEncryptedSharedPreferences() {
+  /** The class `androidx.security.crypto.EncryptedSharedPreferences`, which implements `SharedPreferences` with encryption support. */
+  class TypeEncryptedPrefs extends Class {
+    TypeEncryptedPrefs() {
       hasQualifiedName("androidx.security.crypto", "EncryptedSharedPreferences")
     }
   }
 
-  /* The create method of `androidx.security.crypto.EncryptedSharedPreferences` */
-  class EncryptedSharedPrefsCreateMethod extends Method {
-    EncryptedSharedPrefsCreateMethod() {
-      getDeclaringType() instanceof TypeEncryptedSharedPreferences and
+  /** The create method of `androidx.security.crypto.EncryptedSharedPreferences` */
+  class CreateEncryptedPrefsMethod extends Method {
+    CreateEncryptedPrefsMethod() {
+      getDeclaringType() instanceof TypeEncryptedPrefs and
       hasName("create")
     }
   }
 
-  /* A getter method of `android.content.SharedPreferences`. */
-  class SharedPreferencesGetMethod extends Method {
-    SharedPreferencesGetMethod() {
-      getDeclaringType() instanceof TypeSharedPreferences and
+  /** A getter method of `android.content.SharedPreferences`. */
+  class GetPreferenceMethod extends Method {
+    GetPreferenceMethod() {
+      getDeclaringType() instanceof TypePrefs 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
+  /** Returns `android.content.SharedPreferences.Editor` from the `edit` call of `android.content.SharedPreferences`. */
+  class GetEditorMethod extends Method {
+    GetEditorMethod() {
+      getDeclaringType() instanceof TypePrefs and
       hasName("edit") and
-      getReturnType() instanceof TypeSharedPreferencesEditor
+      getReturnType() instanceof TypeEditor
     }
   }
 
-  /* Definitions related to `android.content.SharedPreferences.Editor`. */
-  class TypeSharedPreferencesEditor extends Interface {
-    TypeSharedPreferencesEditor() {
-      hasQualifiedName("android.content", "SharedPreferences$Editor")
-    }
+  /** Definitions related to `android.content.SharedPreferences.Editor`. */
+  class TypeEditor extends Interface {
+    TypeEditor() { hasQualifiedName("android.content", "SharedPreferences$Editor") }
   }
 
-  /* A setter method for `android.content.SharedPreferences`. */
-  class SharedPreferencesSetMethod extends Method {
-    SharedPreferencesSetMethod() {
-      getDeclaringType() instanceof TypeSharedPreferencesEditor and
+  /** A setter method for `android.content.SharedPreferences`. */
+  class SetPreferenceMethod extends Method {
+    SetPreferenceMethod() {
+      getDeclaringType() instanceof TypeEditor and
       getName().matches("put%")
     }
   }
 
-  /* A setter method for `android.content.SharedPreferences`. */
-  class SharedPreferencesStoreMethod extends Method {
-    SharedPreferencesStoreMethod() {
-      getDeclaringType() instanceof TypeSharedPreferencesEditor and
+  /** A setter method for `android.content.SharedPreferences`. */
+  class StorePreferenceMethod extends Method {
+    StorePreferenceMethod() {
+      getDeclaringType() instanceof TypeEditor and
       hasName(["commit", "apply"])
     }
   }

java/ql/test/experimental/query-tests/security/CWE-312/CleartextStorageSharedPrefs.expected

@@ -0,0 +1,22 @@
+edges
+| CleartextStorageSharedPrefs.java:16:19:16:36 | edit(...) : Editor | CleartextStorageSharedPrefs.java:17:3:17:8 | editor |
+| CleartextStorageSharedPrefs.java:16:19:16:36 | edit(...) : Editor | CleartextStorageSharedPrefs.java:18:3:18:8 | editor |
+| CleartextStorageSharedPrefs.java:16:19:16:36 | edit(...) : Editor | CleartextStorageSharedPrefs.java:19:3:19:8 | editor |
+| CleartextStorageSharedPrefs.java:25:19:25:36 | edit(...) : Editor | CleartextStorageSharedPrefs.java:28:3:28:8 | editor |
+| CleartextStorageSharedPrefs.java:44:19:44:36 | edit(...) : Editor | CleartextStorageSharedPrefs.java:47:3:47:8 | editor |
+nodes
+| CleartextStorageSharedPrefs.java:16:19:16:36 | edit(...) : Editor | semmle.label | edit(...) : Editor |
+| CleartextStorageSharedPrefs.java:17:3:17:8 | editor | semmle.label | editor |
+| CleartextStorageSharedPrefs.java:18:3:18:8 | editor | semmle.label | editor |
+| CleartextStorageSharedPrefs.java:18:32:18:39 | password | semmle.label | password |
+| CleartextStorageSharedPrefs.java:19:3:19:8 | editor | semmle.label | editor |
+| CleartextStorageSharedPrefs.java:25:19:25:36 | edit(...) : Editor | semmle.label | edit(...) : Editor |
+| CleartextStorageSharedPrefs.java:28:3:28:8 | editor | semmle.label | editor |
+| CleartextStorageSharedPrefs.java:44:19:44:36 | edit(...) : Editor | semmle.label | edit(...) : Editor |
+| CleartextStorageSharedPrefs.java:46:32:46:42 | encPassword | semmle.label | encPassword |
+| CleartextStorageSharedPrefs.java:47:3:47:8 | editor | semmle.label | editor |
+| CleartextStorageSharedPrefs.java:67:32:67:39 | password | semmle.label | password |
+| CleartextStorageSharedPrefs.java:87:32:87:39 | password | semmle.label | password |
+| CleartextStorageSharedPrefs.java:105:27:105:34 | password | semmle.label | password |
+#select
+| 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 |

java/ql/test/experimental/query-tests/security/CWE-312/CleartextStorageSharedPrefs.java

@@ -9,7 +9,7 @@ import java.util.Base64;
 import java.security.MessageDigest;
 
 /* Android activity that tests saving sensitive information in `SharedPreferences` */
-public class ClearTextStorageSharedPrefs extends Activity {
+public class CleartextStorageSharedPrefs extends Activity {
 	// BAD - save sensitive information in cleartext
 	public void testSetSharedPrefs1(Context context, String name, String password) {
 		SharedPreferences sharedPrefs = context.getSharedPreferences("user_prefs", Context.MODE_PRIVATE);

java/ql/test/experimental/query-tests/security/CWE-312/CleartextStorageSharedPrefs.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql

java/ql/test/experimental/query-tests/security/CWE-312/options

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

java/ql/test/query-tests/security/CWE-312/semmle/tests/ClearTextStorageSharedPrefs.expected

@@ -1 +0,0 @@
-| 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 |

java/ql/test/query-tests/security/CWE-312/semmle/tests/ClearTextStorageSharedPrefs.qlref

@@ -1 +0,0 @@
-Security/CWE/CWE-312/ClearTextStorageSharedPrefs.ql