diff --git a/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.qhelp b/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.qhelp index d06b8f7bf26..681ff9fe95e 100644 --- a/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.qhelp +++ b/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.qhelp @@ -2,7 +2,7 @@

- SharedPreferences 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 SharedPreferences. However, sensitive information shall 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. + SharedPreferences 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.

diff --git a/java/ql/src/Security/CWE/CWE-312/SensitiveStorage.qll b/java/ql/src/Security/CWE/CWE-312/SensitiveStorage.qll index 8b61faf3750..bc629532b9f 100644 --- a/java/ql/src/Security/CWE/CWE-312/SensitiveStorage.qll +++ b/java/ql/src/Security/CWE/CWE-312/SensitiveStorage.qll @@ -5,6 +5,7 @@ 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,7 +31,8 @@ private class SensitiveSourceFlowConfig extends TaintTracking::Configuration { ) or exists(MethodAccess m | - m.getMethod() instanceof SharedPreferencesSetMethod and sink.asExpr() = m.getArgument(1) + m.getMethod() instanceof SharedPreferences::SharedPreferencesSetMethod and + sink.asExpr() = m.getArgument(1) ) or sink.asExpr() = getInstanceInput(_, _) @@ -252,8 +254,9 @@ 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 SharedPreferencesSetMethod and + 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() ) } @@ -261,13 +264,13 @@ private predicate sharedPreferencesInput(DataFlow::Node sharedPrefs, Expr input) /* 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 SharedPreferencesStoreMethod and + m.getMethod() instanceof SharedPreferences::SharedPreferencesStoreMethod and store = m and sharedPrefs.asExpr() = m.getQualifier() ) } -/* Flow from `SharedPreferences` to the method call changing its value. */ +/* Flow from `SharedPreferences` to either a setter or a store method. */ class SharedPreferencesFlowConfig extends TaintTracking::Configuration { SharedPreferencesFlowConfig() { this = "SensitiveStorage::SharedPreferencesFlowConfig" } @@ -279,25 +282,63 @@ class SharedPreferencesFlowConfig extends TaintTracking::Configuration { sharedPreferencesInput(sink, _) or sharedPreferencesStore(sink, _) } +} - override predicate isSanitizer(DataFlow::Node n) { +/** + * 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().getName().toLowerCase().matches("%encrypt%") and - n.asExpr() = ma.getAnArgument() + 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 SharedPreferencesGetEditorMethod and + this.getMethod() instanceof SharedPreferences::SharedPreferencesGetEditorMethod and not exists( - MethodAccess cma // not exists `SharedPreferences sharedPreferences = EncryptedSharedPreferences.create(...)` + EncryptedSharedPrefFlowConfig config // not exists `SharedPreferences sharedPreferences = EncryptedSharedPreferences.create(...)` | - cma.getQualifier().getType() instanceof TypeEncryptedSharedPreferences and - cma.getMethod().hasName("create") and - cma.getParent().(VariableAssign).getDestVar().getAnAccess() = this.getQualifier() + config.hasFlow(_, DataFlow::exprNode(this.getQualifier())) ) } diff --git a/java/ql/src/semmle/code/java/frameworks/android/SharedPreferences.qll b/java/ql/src/semmle/code/java/frameworks/android/SharedPreferences.qll index 7ac1f3bb1df..59657ffb6e6 100644 --- a/java/ql/src/semmle/code/java/frameworks/android/SharedPreferences.qll +++ b/java/ql/src/semmle/code/java/frameworks/android/SharedPreferences.qll @@ -1,52 +1,64 @@ -/* Definitions related to `android.content.SharedPreferences`. */ import semmle.code.java.Type -/* The interface `android.content.SharedPreferences` */ -library class TypeSharedPreferences extends Interface { - TypeSharedPreferences() { hasQualifiedName("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 class `androidx.security.crypto.EncryptedSharedPreferences`, which implements `SharedPreferences` with encryption support. */ -library class TypeEncryptedSharedPreferences extends Class { - TypeEncryptedSharedPreferences() { - hasQualifiedName("androidx.security.crypto", "EncryptedSharedPreferences") - } -} - -/* A getter method of `android.content.SharedPreferences`. */ -library 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`. */ -library class SharedPreferencesGetEditorMethod extends Method { - SharedPreferencesGetEditorMethod() { - getDeclaringType() instanceof TypeSharedPreferences and - hasName("edit") and - getReturnType() instanceof TypeSharedPreferencesEditor - } -} - -/* Definitions related to `android.content.SharedPreferences.Editor`. */ -library class TypeSharedPreferencesEditor extends Interface { - TypeSharedPreferencesEditor() { hasQualifiedName("android.content", "SharedPreferences$Editor") } -} - -/* A setter method for `android.content.SharedPreferences`. */ -library class SharedPreferencesSetMethod extends Method { - SharedPreferencesSetMethod() { - getDeclaringType() instanceof TypeSharedPreferencesEditor and - getName().matches("put%") - } -} - -/* A setter method for `android.content.SharedPreferences`. */ -library class SharedPreferencesStoreMethod extends Method { - SharedPreferencesStoreMethod() { - getDeclaringType() instanceof TypeSharedPreferencesEditor and - hasName(["commit", "apply"]) + /* 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 index 63ddf22798c..592a74db89d 100644 --- 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 @@ -1 +1 @@ -| ClearTextStorageSharedPrefs.java:16:3:16:17 | commit(...) | 'SharedPreferences' class $@ containing $@ is stored here. Data was added $@. | ClearTextStorageSharedPrefs.java:13:19:13:36 | edit(...) | edit(...) | ClearTextStorageSharedPrefs.java:15:32:15:39 | password | sensitive data | ClearTextStorageSharedPrefs.java:15:32:15:39 | password | here | +| 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 | diff --git a/java/ql/test/query-tests/security/CWE-312/semmle/tests/ClearTextStorageSharedPrefs.java b/java/ql/test/query-tests/security/CWE-312/semmle/tests/ClearTextStorageSharedPrefs.java index d9484b82ef4..dbc1f54b59c 100644 --- a/java/ql/test/query-tests/security/CWE-312/semmle/tests/ClearTextStorageSharedPrefs.java +++ b/java/ql/test/query-tests/security/CWE-312/semmle/tests/ClearTextStorageSharedPrefs.java @@ -4,8 +4,11 @@ import android.content.SharedPreferences; import android.content.SharedPreferences.Editor; import androidx.security.crypto.MasterKey; import androidx.security.crypto.EncryptedSharedPreferences; +import java.nio.charset.StandardCharsets; +import java.util.Base64; +import java.security.MessageDigest; -/** Android activity that tests saving sensitive information in `SharedPreferences` */ +/* Android activity that tests saving sensitive information in `SharedPreferences` */ public class ClearTextStorageSharedPrefs extends Activity { // BAD - save sensitive information in cleartext public void testSetSharedPrefs1(Context context, String name, String password) { @@ -26,13 +29,27 @@ public class ClearTextStorageSharedPrefs extends Activity { } private static String encrypt(String cleartext) { - //Use an encryption or hashing algorithm in real world. The demo below just returns an arbitrary value. - String cipher = "whatever_encrypted"; - return cipher; + // 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; } - // GOOD - save sensitive information using the built-in `EncryptedSharedPreferences` class in androidx. + // GOOD - save sensitive information in encrypted format using separate variables public void testSetSharedPrefs3(Context context, String name, String password) { + String encUsername = encrypt(name); + String encPassword = encrypt(password); + SharedPreferences sharedPrefs = context.getSharedPreferences("user_prefs", Context.MODE_PRIVATE); + Editor editor = sharedPrefs.edit(); + editor.putString("name", encUsername); + editor.putString("password", encPassword); + editor.commit(); + } + + + // GOOD - save sensitive information using the built-in `EncryptedSharedPreferences` class in androidx + public void testSetSharedPrefs4(Context context, String name, String password) { MasterKey masterKey = new MasterKey.Builder(context, MasterKey.DEFAULT_MASTER_KEY_ALIAS) .setKeyScheme(MasterKey.KeyScheme.AES256_GCM) .build(); @@ -50,4 +67,43 @@ public class ClearTextStorageSharedPrefs extends Activity { editor.putString("password", password); editor.commit(); } + + // GOOD - save sensitive information using the built-in `EncryptedSharedPreferences` class in androidx + public void testSetSharedPrefs5(Context context, String name, String password) { + MasterKey masterKey = new MasterKey.Builder(context, MasterKey.DEFAULT_MASTER_KEY_ALIAS) + .setKeyScheme(MasterKey.KeyScheme.AES256_GCM) + .build(); + + SharedPreferences.Editor editor = EncryptedSharedPreferences.create( + context, + "secret_shared_prefs", + masterKey, + EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV, + EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM) + .edit(); + + // Use the shared preferences and editor as you normally would + editor.putString("name", name); + editor.putString("password", password); + editor.commit(); + } + + // GOOD - save sensitive information using the built-in `EncryptedSharedPreferences` class in androidx + public void testSetSharedPrefs6(Context context, String name, String password) { + MasterKey masterKey = new MasterKey.Builder(context, MasterKey.DEFAULT_MASTER_KEY_ALIAS) + .setKeyScheme(MasterKey.KeyScheme.AES256_GCM) + .build(); + + SharedPreferences.Editor editor = EncryptedSharedPreferences.create( + context, + "secret_shared_prefs", + masterKey, + EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV, + EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM) + .edit() + .putString("name", name) // Use the shared preferences and editor as you normally would + .putString("password", password); + + editor.commit(); + } }