From 0bd6255c41a79903219408aba71f9fe3335326ba Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Mon, 16 Nov 2020 17:23:01 +0000 Subject: [PATCH 01/11] Query for cleartext storage using Android SharedPreferences --- .../CWE-312/ClearTextStorageSharedPrefs.java | 39 ++ .../CWE-312/ClearTextStorageSharedPrefs.qhelp | 40 ++ .../CWE-312/ClearTextStorageSharedPrefs.ql | 22 ++ .../Security/CWE/CWE-312/SensitiveStorage.qll | 74 ++++ .../frameworks/android/SharedPreferences.qll | 52 +++ .../ClearTextStorageSharedPrefs.expected | 1 + .../CWE-312/ClearTextStorageSharedPrefs.java | 54 +++ .../CWE-312/ClearTextStorageSharedPrefs.qlref | 1 + .../query-tests/security/CWE-312/options | 1 + .../android/content/Context.java | 123 +++++- .../android/content/SharedPreferences.java | 362 ++++++++++++++++++ .../crypto/EncryptedSharedPreferences.java | 171 +++++++++ .../androidx/security/crypto/MasterKey.java | 191 +++++++++ 13 files changed, 1120 insertions(+), 11 deletions(-) create mode 100644 java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.java create mode 100644 java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.qhelp create mode 100644 java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.ql create mode 100644 java/ql/src/semmle/code/java/frameworks/android/SharedPreferences.qll create mode 100644 java/ql/test/experimental/query-tests/security/CWE-312/ClearTextStorageSharedPrefs.expected create mode 100644 java/ql/test/experimental/query-tests/security/CWE-312/ClearTextStorageSharedPrefs.java create mode 100644 java/ql/test/experimental/query-tests/security/CWE-312/ClearTextStorageSharedPrefs.qlref create mode 100644 java/ql/test/experimental/query-tests/security/CWE-312/options create mode 100644 java/ql/test/stubs/google-android-9.0.0/android/content/SharedPreferences.java create mode 100644 java/ql/test/stubs/google-android-9.0.0/androidx/security/crypto/EncryptedSharedPreferences.java create mode 100644 java/ql/test/stubs/google-android-9.0.0/androidx/security/crypto/MasterKey.java diff --git a/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.java b/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.java new file mode 100644 index 000000000000..259ee23bf819 --- /dev/null +++ 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 new file mode 100644 index 000000000000..f040ed4132d4 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.qhelp @@ -0,0 +1,40 @@ + + + +

+ 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. +

+ + + +

+ Use the EncryptedSharedPreferences API or other encryption algorithms for storing sensitive information. +

+ + + +

+ In the first example, sensitive user information is stored in cleartext. +

+ +

+ In the second and third examples, the code encrypts sensitive information before saving to the device. +

+ + + + +
  • + CWE: + CWE-312: Cleartext Storage of Sensitive Information +
    • +
  • + Android Developers: + Work with data more securely +
    • +
  • + PRO ANDROID DEV: + Encrypted Preferences in Android +
    • +     +     diff --git a/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.ql b/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.ql new file mode 100644 index 000000000000..5807ea196840 --- /dev/null +++ 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 user with root privileges to access 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 index b07105e3bf5a..a63d5e43891f 100644 --- a/java/ql/src/Security/CWE/CWE-312/SensitiveStorage.qll +++ b/java/ql/src/Security/CWE/CWE-312/SensitiveStorage.qll @@ -1,6 +1,7 @@ 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 @@ -28,6 +29,10 @@ private class SensitiveSourceFlowConfig extends TaintTracking::Configuration { m.getMethod() instanceof PropertiesSetPropertyMethod and sink.asExpr() = m.getArgument(1) ) or + exists(MethodAccess m | + m.getMethod() instanceof SharedPreferencesSetMethod and sink.asExpr() = m.getArgument(1) + ) + or sink.asExpr() = getInstanceInput(_, _) } @@ -243,3 +248,72 @@ 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 + input = m.getArgument(1) and + sharedPrefs.asExpr() = m.getQualifier() + ) +} + +/* Holds if the method call is the save method of `SharedPreferences`. */ +private predicate sharedPreferencesStore(DataFlow::Node sharedPrefs, Expr store) { + exists(MethodAccess m | + m.getMethod() instanceof SharedPreferencesStoreMethod and + store = m and + sharedPrefs.asExpr() = m.getQualifier() + ) +} + +/* Flow from `SharedPreferences` to the method call changing its value. */ +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, _) + } + + override predicate isSanitizer(DataFlow::Node n) { + exists(MethodAccess ma | + ma.getMethod().getName().toLowerCase().matches("%encrypt%") and + n.asExpr() = ma.getAnArgument() + ) + } +} + +/** 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 + not exists( + MethodAccess cma // not exists `SharedPreferences sharedPreferences = EncryptedSharedPreferences.create(...)` + | + cma.getQualifier().getType() instanceof TypeEncryptedSharedPreferences and + cma.getMethod().hasName("create") and + cma.getParent().(VariableAssign).getDestVar().getAnAccess() = 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) + ) + } +} diff --git a/java/ql/src/semmle/code/java/frameworks/android/SharedPreferences.qll b/java/ql/src/semmle/code/java/frameworks/android/SharedPreferences.qll new file mode 100644 index 000000000000..7ac1f3bb1df8 --- /dev/null +++ b/java/ql/src/semmle/code/java/frameworks/android/SharedPreferences.qll @@ -0,0 +1,52 @@ +/* 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") } +} + +/* 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"]) + } +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-312/ClearTextStorageSharedPrefs.expected b/java/ql/test/experimental/query-tests/security/CWE-312/ClearTextStorageSharedPrefs.expected new file mode 100644 index 000000000000..63ddf22798cf --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-312/ClearTextStorageSharedPrefs.expected @@ -0,0 +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 | diff --git a/java/ql/test/experimental/query-tests/security/CWE-312/ClearTextStorageSharedPrefs.java b/java/ql/test/experimental/query-tests/security/CWE-312/ClearTextStorageSharedPrefs.java new file mode 100644 index 000000000000..73423a065cc0 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-312/ClearTextStorageSharedPrefs.java @@ -0,0 +1,54 @@ +import android.app.Activity; +import android.content.Context; +import android.content.SharedPreferences; +import android.content.SharedPreferences.Editor; +import androidx.security.crypto.MasterKey; +import androidx.security.crypto.EncryptedSharedPreferences; + +/** 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) { + 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 + public void testSetSharedPrefs2(Context context, String name, String password) { + 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(); + } + + 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; + } + + // GOOD - save sensitive information using the built-in `EncryptedSharedPreferences` class in androidx. + public void testSetSharedPrefs3(Context context, String name, String password) { + 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); + + // Use the shared preferences and editor as you normally would + SharedPreferences.Editor editor = sharedPreferences.edit(); + editor.putString("name", name); + editor.putString("password", password); + editor.commit(); + } + +} \ No newline at end of file diff --git a/java/ql/test/experimental/query-tests/security/CWE-312/ClearTextStorageSharedPrefs.qlref b/java/ql/test/experimental/query-tests/security/CWE-312/ClearTextStorageSharedPrefs.qlref new file mode 100644 index 000000000000..a7547b790fd7 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-312/ClearTextStorageSharedPrefs.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-312/ClearTextStorageSharedPrefs.ql diff --git a/java/ql/test/experimental/query-tests/security/CWE-312/options b/java/ql/test/experimental/query-tests/security/CWE-312/options new file mode 100644 index 000000000000..43e25f608b67 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-312/options @@ -0,0 +1 @@ +// semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/google-android-9.0.0 diff --git a/java/ql/test/stubs/google-android-9.0.0/android/content/Context.java b/java/ql/test/stubs/google-android-9.0.0/android/content/Context.java index e1ce2140a063..6507cbd371d3 100644 --- a/java/ql/test/stubs/google-android-9.0.0/android/content/Context.java +++ b/java/ql/test/stubs/google-android-9.0.0/android/content/Context.java @@ -26,6 +26,84 @@ * broadcasting and receiving intents, etc. */ public abstract class Context { + /** + * File creation mode: the default mode, where the created file can only + * be accessed by the calling application (or all applications sharing the + * same user ID). + * @see #MODE_WORLD_READABLE + * @see #MODE_WORLD_WRITEABLE + */ + public static final int MODE_PRIVATE = 0x0000; + + /** + * @deprecated Creating world-readable files is very dangerous, and likely + * to cause security holes in applications. It is strongly discouraged; + * instead, applications should use more formal mechanism for interactions + * such as {@link ContentProvider}, {@link BroadcastReceiver}, and + * {@link android.app.Service}. There are no guarantees that this + * access mode will remain on a file, such as when it goes through a + * backup and restore. + * File creation mode: allow all other applications to have read access + * to the created file. + * @see #MODE_PRIVATE + * @see #MODE_WORLD_WRITEABLE + */ + @Deprecated + public static final int MODE_WORLD_READABLE = 0x0001; + + /** + * @deprecated Creating world-writable files is very dangerous, and likely + * to cause security holes in applications. It is strongly discouraged; + * instead, applications should use more formal mechanism for interactions + * such as {@link ContentProvider}, {@link BroadcastReceiver}, and + * {@link android.app.Service}. There are no guarantees that this + * access mode will remain on a file, such as when it goes through a + * backup and restore. + * File creation mode: allow all other applications to have write access + * to the created file. + * @see #MODE_PRIVATE + * @see #MODE_WORLD_READABLE + */ + @Deprecated + public static final int MODE_WORLD_WRITEABLE = 0x0002; + + /** + * File creation mode: for use with {@link #openFileOutput}, if the file + * already exists then write data to the end of the existing file + * instead of erasing it. + * @see #openFileOutput + */ + public static final int MODE_APPEND = 0x8000; + + /** + * SharedPreference loading flag: when set, the file on disk will + * be checked for modification even if the shared preferences + * instance is already loaded in this process. This behavior is + * sometimes desired in cases where the application has multiple + * processes, all writing to the same SharedPreferences file. + * Generally there are better forms of communication between + * processes, though. + * + *

    This was the legacy (but undocumented) behavior in and + * before Gingerbread (Android 2.3) and this flag is implied when + * targetting such releases. For applications targetting SDK + * versions greater than Android 2.3, this flag must be + * explicitly set if desired. + * + * @see #getSharedPreferences + */ + public static final int MODE_MULTI_PROCESS = 0x0004; + + /** + * Database open flag: when set, the database is opened with write-ahead + * logging enabled by default. + * + * @see #openOrCreateDatabase(String, int, CursorFactory) + * @see #openOrCreateDatabase(String, int, CursorFactory, DatabaseErrorHandler) + * @see SQLiteDatabase#enableWriteAheadLogging + */ + public static final int MODE_ENABLE_WRITE_AHEAD_LOGGING = 0x0008; + /** * Return the context of the single, global Application object of the current * process. This generally should only be used if you need a Context whose @@ -76,19 +154,42 @@ public abstract class Context { public abstract File getFileStreamPath(String name); /** - * Returns the absolute path on the filesystem where a file created with - * {@link #getSharedPreferences(String, int)} is stored. - *

    - * The returned path may change over time if the calling app is moved to an - * adopted storage device, so only relative paths should be persisted. + * {@hide} + * Return the full path to the shared prefs file for the given prefs group name. * - * @param name The name of the shared preferences for which you would like to - * get a path. - * @return An absolute path to the given file. - * @see #getSharedPreferences(String, int) - * @removed + *

    Note: this is not generally useful for applications, since they should + * not be directly accessing the file system. + */ + public abstract File getSharedPrefsFile(String name); + + /** + * Retrieve and hold the contents of the preferences file 'name', returning + * a SharedPreferences through which you can retrieve and modify its + * values. Only one instance of the SharedPreferences object is returned + * to any callers for the same name, meaning they will see each other's + * edits as soon as they are made. + * + * @param name Desired preferences file. If a preferences file by this name + * does not exist, it will be created when you retrieve an + * editor (SharedPreferences.edit()) and then commit changes (Editor.commit()). + * @param mode Operating mode. Use 0 or {@link #MODE_PRIVATE} for the + * default operation, {@link #MODE_WORLD_READABLE} + * and {@link #MODE_WORLD_WRITEABLE} to control permissions. The bit + * {@link #MODE_MULTI_PROCESS} can also be used if multiple processes + * are mutating the same SharedPreferences file. {@link #MODE_MULTI_PROCESS} + * is always on in apps targetting Gingerbread (Android 2.3) and below, and + * off by default in later versions. + * + * @return Returns the single SharedPreferences instance that can be used + * to retrieve and modify the preference values. + * + * @see #MODE_PRIVATE + * @see #MODE_WORLD_READABLE + * @see #MODE_WORLD_WRITEABLE + * @see #MODE_MULTI_PROCESS */ - public abstract File getSharedPreferencesPath(String name); + public abstract SharedPreferences getSharedPreferences(String name, + int mode); /** * Returns the absolute path to the directory on the filesystem where all diff --git a/java/ql/test/stubs/google-android-9.0.0/android/content/SharedPreferences.java b/java/ql/test/stubs/google-android-9.0.0/android/content/SharedPreferences.java new file mode 100644 index 000000000000..faac2f1d41e7 --- /dev/null +++ b/java/ql/test/stubs/google-android-9.0.0/android/content/SharedPreferences.java @@ -0,0 +1,362 @@ +/* + * Copyright (C) 2006 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package android.content; + +import java.util.Map; +import java.util.Set; +/** + * Interface for accessing and modifying preference data returned by {@link + * Context#getSharedPreferences}. For any particular set of preferences, + * there is a single instance of this class that all clients share. + * Modifications to the preferences must go through an {@link Editor} object + * to ensure the preference values remain in a consistent state and control + * when they are committed to storage. Objects that are returned from the + * various get methods must be treated as immutable by the application. + * + *

    Note: currently this class does not support use across multiple + * processes. This will be added later. + * + *

    + *

    Developer Guides

    + *

    For more information about using SharedPreferences, read the + * Data Storage + * developer guide.

    + * + * @see Context#getSharedPreferences + */ +public interface SharedPreferences { + /** + * Interface definition for a callback to be invoked when a shared + * preference is changed. + */ + public interface OnSharedPreferenceChangeListener { + /** + * Called when a shared preference is changed, added, or removed. This + * may be called even if a preference is set to its existing value. + * + *

    This callback will be run on your main thread. + * + * @param sharedPreferences The {@link SharedPreferences} that received + * the change. + * @param key The key of the preference that was changed, added, or + * removed. + */ + void onSharedPreferenceChanged(SharedPreferences sharedPreferences, String key); + } + + /** + * Interface used for modifying values in a {@link SharedPreferences} + * object. All changes you make in an editor are batched, and not copied + * back to the original {@link SharedPreferences} until you call {@link #commit} + * or {@link #apply} + */ + public interface Editor { + /** + * Set a String value in the preferences editor, to be written back once + * {@link #commit} or {@link #apply} are called. + * + * @param key The name of the preference to modify. + * @param value The new value for the preference. Supplying {@code null} + * as the value is equivalent to calling {@link #remove(String)} with + * this key. + * + * @return Returns a reference to the same Editor object, so you can + * chain put calls together. + */ + Editor putString(String key, String value); + + /** + * Set a set of String values in the preferences editor, to be written + * back once {@link #commit} is called. + * + * @param key The name of the preference to modify. + * @param values The set of new values for the preference. Passing {@code null} + * for this argument is equivalent to calling {@link #remove(String)} with + * this key. + * @return Returns a reference to the same Editor object, so you can + * chain put calls together. + */ + Editor putStringSet(String key, Set values); + + /** + * Set an int value in the preferences editor, to be written back once + * {@link #commit} or {@link #apply} are called. + * + * @param key The name of the preference to modify. + * @param value The new value for the preference. + * + * @return Returns a reference to the same Editor object, so you can + * chain put calls together. + */ + Editor putInt(String key, int value); + + /** + * Set a long value in the preferences editor, to be written back once + * {@link #commit} or {@link #apply} are called. + * + * @param key The name of the preference to modify. + * @param value The new value for the preference. + * + * @return Returns a reference to the same Editor object, so you can + * chain put calls together. + */ + Editor putLong(String key, long value); + + /** + * Set a float value in the preferences editor, to be written back once + * {@link #commit} or {@link #apply} are called. + * + * @param key The name of the preference to modify. + * @param value The new value for the preference. + * + * @return Returns a reference to the same Editor object, so you can + * chain put calls together. + */ + Editor putFloat(String key, float value); + + /** + * Set a boolean value in the preferences editor, to be written back + * once {@link #commit} or {@link #apply} are called. + * + * @param key The name of the preference to modify. + * @param value The new value for the preference. + * + * @return Returns a reference to the same Editor object, so you can + * chain put calls together. + */ + Editor putBoolean(String key, boolean value); + /** + * Mark in the editor that a preference value should be removed, which + * will be done in the actual preferences once {@link #commit} is + * called. + * + *

    Note that when committing back to the preferences, all removals + * are done first, regardless of whether you called remove before + * or after put methods on this editor. + * + * @param key The name of the preference to remove. + * + * @return Returns a reference to the same Editor object, so you can + * chain put calls together. + */ + Editor remove(String key); + /** + * Mark in the editor to remove all values from the + * preferences. Once commit is called, the only remaining preferences + * will be any that you have defined in this editor. + * + *

    Note that when committing back to the preferences, the clear + * is done first, regardless of whether you called clear before + * or after put methods on this editor. + * + * @return Returns a reference to the same Editor object, so you can + * chain put calls together. + */ + Editor clear(); + /** + * Commit your preferences changes back from this Editor to the + * {@link SharedPreferences} object it is editing. This atomically + * performs the requested modifications, replacing whatever is currently + * in the SharedPreferences. + * + *

    Note that when two editors are modifying preferences at the same + * time, the last one to call commit wins. + * + *

    If you don't care about the return value and you're + * using this from your application's main thread, consider + * using {@link #apply} instead. + * + * @return Returns true if the new values were successfully written + * to persistent storage. + */ + boolean commit(); + /** + * Commit your preferences changes back from this Editor to the + * {@link SharedPreferences} object it is editing. This atomically + * performs the requested modifications, replacing whatever is currently + * in the SharedPreferences. + * + *

    Note that when two editors are modifying preferences at the same + * time, the last one to call apply wins. + * + *

    Unlike {@link #commit}, which writes its preferences out + * to persistent storage synchronously, {@link #apply} + * commits its changes to the in-memory + * {@link SharedPreferences} immediately but starts an + * asynchronous commit to disk and you won't be notified of + * any failures. If another editor on this + * {@link SharedPreferences} does a regular {@link #commit} + * while a {@link #apply} is still outstanding, the + * {@link #commit} will block until all async commits are + * completed as well as the commit itself. + * + *

    As {@link SharedPreferences} instances are singletons within + * a process, it's safe to replace any instance of {@link #commit} with + * {@link #apply} if you were already ignoring the return value. + * + *

    You don't need to worry about Android component + * lifecycles and their interaction with apply() + * writing to disk. The framework makes sure in-flight disk + * writes from apply() complete before switching + * states. + * + *

    The SharedPreferences.Editor interface + * isn't expected to be implemented directly. However, if you + * previously did implement it and are now getting errors + * about missing apply(), you can simply call + * {@link #commit} from apply(). + */ + void apply(); + } + /** + * Retrieve all values from the preferences. + * + *

    Note that you must not modify the collection returned + * by this method, or alter any of its contents. The consistency of your + * stored data is not guaranteed if you do. + * + * @return Returns a map containing a list of pairs key/value representing + * the preferences. + * + * @throws NullPointerException + */ + Map getAll(); + /** + * Retrieve a String value from the preferences. + * + * @param key The name of the preference to retrieve. + * @param defValue Value to return if this preference does not exist. + * + * @return Returns the preference value if it exists, or defValue. Throws + * ClassCastException if there is a preference with this name that is not + * a String. + * + * @throws ClassCastException + */ + String getString(String key, String defValue); + + /** + * Retrieve a set of String values from the preferences. + * + *

    Note that you must not modify the set instance returned + * by this call. The consistency of the stored data is not guaranteed + * if you do, nor is your ability to modify the instance at all. + * + * @param key The name of the preference to retrieve. + * @param defValues Values to return if this preference does not exist. + * + * @return Returns the preference values if they exist, or defValues. + * Throws ClassCastException if there is a preference with this name + * that is not a Set. + * + * @throws ClassCastException + */ + Set getStringSet(String key, Set defValues); + + /** + * Retrieve an int value from the preferences. + * + * @param key The name of the preference to retrieve. + * @param defValue Value to return if this preference does not exist. + * + * @return Returns the preference value if it exists, or defValue. Throws + * ClassCastException if there is a preference with this name that is not + * an int. + * + * @throws ClassCastException + */ + int getInt(String key, int defValue); + + /** + * Retrieve a long value from the preferences. + * + * @param key The name of the preference to retrieve. + * @param defValue Value to return if this preference does not exist. + * + * @return Returns the preference value if it exists, or defValue. Throws + * ClassCastException if there is a preference with this name that is not + * a long. + * + * @throws ClassCastException + */ + long getLong(String key, long defValue); + + /** + * Retrieve a float value from the preferences. + * + * @param key The name of the preference to retrieve. + * @param defValue Value to return if this preference does not exist. + * + * @return Returns the preference value if it exists, or defValue. Throws + * ClassCastException if there is a preference with this name that is not + * a float. + * + * @throws ClassCastException + */ + float getFloat(String key, float defValue); + + /** + * Retrieve a boolean value from the preferences. + * + * @param key The name of the preference to retrieve. + * @param defValue Value to return if this preference does not exist. + * + * @return Returns the preference value if it exists, or defValue. Throws + * ClassCastException if there is a preference with this name that is not + * a boolean. + * + * @throws ClassCastException + */ + boolean getBoolean(String key, boolean defValue); + /** + * Checks whether the preferences contains a preference. + * + * @param key The name of the preference to check. + * @return Returns true if the preference exists in the preferences, + * otherwise false. + */ + boolean contains(String key); + + /** + * Create a new Editor for these preferences, through which you can make + * modifications to the data in the preferences and atomically commit those + * changes back to the SharedPreferences object. + * + *

    Note that you must call {@link Editor#commit} to have any + * changes you perform in the Editor actually show up in the + * SharedPreferences. + * + * @return Returns a new instance of the {@link Editor} interface, allowing + * you to modify the values in this SharedPreferences object. + */ + Editor edit(); + + /** + * Registers a callback to be invoked when a change happens to a preference. + * + * @param listener The callback that will run. + * @see #unregisterOnSharedPreferenceChangeListener + */ + void registerOnSharedPreferenceChangeListener(OnSharedPreferenceChangeListener listener); + + /** + * Unregisters a previous callback. + * + * @param listener The callback that should be unregistered. + * @see #registerOnSharedPreferenceChangeListener + */ + void unregisterOnSharedPreferenceChangeListener(OnSharedPreferenceChangeListener listener); +} diff --git a/java/ql/test/stubs/google-android-9.0.0/androidx/security/crypto/EncryptedSharedPreferences.java b/java/ql/test/stubs/google-android-9.0.0/androidx/security/crypto/EncryptedSharedPreferences.java new file mode 100644 index 000000000000..db918d448f21 --- /dev/null +++ b/java/ql/test/stubs/google-android-9.0.0/androidx/security/crypto/EncryptedSharedPreferences.java @@ -0,0 +1,171 @@ +/* + * Copyright 2019 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package androidx.security.crypto; + +import android.content.Context; +import android.content.SharedPreferences; +import java.io.IOException; +import java.security.GeneralSecurityException; + +import java.util.Map; +import java.util.Set; + +/** + * An implementation of {@link SharedPreferences} that encrypts keys and values. + * + * 

    + *  String masterKeyAlias = MasterKeys.getOrCreate(MasterKeys.AES256_GCM_SPEC);
+ *
+ *  SharedPreferences sharedPreferences = EncryptedSharedPreferences.create(
+ *      "secret_shared_prefs",
+ *      masterKeyAlias,
+ *      context,
+ *      EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV,
+ *      EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM
+ *  );
+ *
+ *  // use the shared preferences and editor as you normally would
+ *  SharedPreferences.Editor editor = sharedPreferences.edit();
+ *
    + */ +public final class EncryptedSharedPreferences implements SharedPreferences { + /** + * Opens an instance of encrypted SharedPreferences + * + * @param fileName The name of the file to open; can not contain path + * separators. + * @param masterKey The master key to use. + * @param prefKeyEncryptionScheme The scheme to use for encrypting keys. + * @param prefValueEncryptionScheme The scheme to use for encrypting values. + * @return The SharedPreferences instance that encrypts all data. + * @throws GeneralSecurityException when a bad master key or keyset has been attempted + * @throws IOException when fileName can not be used + */ + public static SharedPreferences create(Context context, + String fileName, + MasterKey masterKey, + PrefKeyEncryptionScheme prefKeyEncryptionScheme, + PrefValueEncryptionScheme prefValueEncryptionScheme) + throws GeneralSecurityException, IOException { + return null; + } + + /** + * Opens an instance of encrypted SharedPreferences + * + * @param fileName The name of the file to open; can not contain path + * separators. + * @param masterKeyAlias The alias of the master key to use. + * @param context The context to use to open the preferences file. + * @param prefKeyEncryptionScheme The scheme to use for encrypting keys. + * @param prefValueEncryptionScheme The scheme to use for encrypting values. + * @return The SharedPreferences instance that encrypts all data. + * @throws GeneralSecurityException when a bad master key or keyset has been attempted + * @throws IOException when fileName can not be used + * @deprecated Use {@link #create(Context, String, MasterKey, + * PrefKeyEncryptionScheme, PrefValueEncryptionScheme)} instead. + */ + @Deprecated + public static SharedPreferences create(String fileName, + String masterKeyAlias, + Context context, + PrefKeyEncryptionScheme prefKeyEncryptionScheme, + PrefValueEncryptionScheme prefValueEncryptionScheme) + throws GeneralSecurityException, IOException { + } + + /** + * The encryption scheme to encrypt keys. + */ + public enum PrefKeyEncryptionScheme { + /** + * Pref keys are encrypted deterministically with AES256-SIV-CMAC (RFC 5297). + * + * For more information please see the Tink documentation: + * + * AesSivKeyManager.aes256SivTemplate() + */ + AES256_SIV; + } + /** + * The encryption scheme to encrypt values. + */ + public enum PrefValueEncryptionScheme { + /** + * Pref values are encrypted with AES256-GCM. The associated data is the encrypted pref key. + * + * For more information please see the Tink documentation: + * + * AesGcmKeyManager.aes256GcmTemplate() + */ + AES256_GCM; + } + + // SharedPreferences methods + @Override + public Map getAll() { + return null; + } + + @Override + public String getString(String key, String defValue) { + return null; + } + + @Override + public Set getStringSet(String key, Set defValues) { + return null; + } + + @Override + public int getInt(String key, int defValue) { + return -1; + } + + @Override + public long getLong(String key, long defValue) { + return -1; + } + + @Override + public float getFloat(String key, float defValue) { + return -1; + } + + @Override + public boolean getBoolean(String key, boolean defValue) { + return false; + } + + @Override + public boolean contains(String key) { + return false; + } + + @Override + public SharedPreferences.Editor edit() { + return null; + } + + @Override + public void registerOnSharedPreferenceChangeListener( + OnSharedPreferenceChangeListener listener) { + } + @Override + public void unregisterOnSharedPreferenceChangeListener( + OnSharedPreferenceChangeListener listener) { + } +} \ No newline at end of file diff --git a/java/ql/test/stubs/google-android-9.0.0/androidx/security/crypto/MasterKey.java b/java/ql/test/stubs/google-android-9.0.0/androidx/security/crypto/MasterKey.java new file mode 100644 index 000000000000..c8f696db300c --- /dev/null +++ b/java/ql/test/stubs/google-android-9.0.0/androidx/security/crypto/MasterKey.java @@ -0,0 +1,191 @@ +/* + * Copyright 2020 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package androidx.security.crypto; + +import android.content.Context; + +import java.io.IOException; +import java.security.GeneralSecurityException; + +/** + * Wrapper for a master key used in the library. + * + * On Android M (API 23) and above, this is class references a key that's stored in the + * Android Keystore. On Android L (API 21, 22), there isn't a master key. + */ +public final class MasterKey { + static final String KEYSTORE_PATH_URI = "android-keystore://"; + /** + * The default master key alias. + */ + public static final String DEFAULT_MASTER_KEY_ALIAS = "_androidx_security_master_key_"; + + /** + * The default and recommended size for the master key. + */ + public static final int DEFAULT_AES_GCM_MASTER_KEY_SIZE = 256; + + /** + * Algorithm/Cipher choices used for the master key. + */ + public enum KeyScheme { + AES256_GCM + } + + /** + * The default validity period for authentication in seconds. + */ + public static int getDefaultAuthenticationValidityDurationSeconds() { + return -1; + } + + /* package */ MasterKey(String keyAlias, Object keyGenParameterSpec) { + } + + /** + * Checks if this key is backed by the Android Keystore. + * + * @return {@code true} if the key is in Android Keystore, {@code false} otherwise. This + * method always returns false when called on Android Lollipop (API 21 and 22). + */ + public boolean isKeyStoreBacked() { + return false; + } + + /** + * Gets whether user authentication is required to use this key. + * + * This method always returns {@code false} on Android L (API 21 + 22). + */ + public boolean isUserAuthenticationRequired() { + return false; + } + + /** + * Gets the duration in seconds that the key is unlocked for following user authentication. + * + * The value returned for this method is only meaningful on Android M+ (API 23) when + * {@link #isUserAuthenticationRequired()} returns {@code true}. + * + * @return The duration the key is unlocked for in seconds. + */ + public int getUserAuthenticationValidityDurationSeconds() { + return -1; + } + + /** + * Gets whether the key is backed by strong box. + */ + public boolean isStrongBoxBacked() { + return false; + } + + /* package */ String getKeyAlias() { + return null; + } + + /** + * Builder for generating a {@link MasterKey}. + */ + public static final class Builder { + /** + * Creates a builder for a {@link MasterKey} using the default alias of + * {@link #DEFAULT_MASTER_KEY_ALIAS}. + * + * @param context The context to use with this master key. + */ + public Builder(Context context) { + } + + /** + * Creates a builder for a {@link MasterKey}. + * + * @param context The context to use with this master key. + */ + public Builder(Context context, String keyAlias) { + } + + /** + * Sets a {@link KeyScheme} to be used for the master key. + * This uses a default {@link KeyGenParameterSpec} associated with the provided + * {@code KeyScheme}. + * NOTE: Either this method OR {@link #setKeyGenParameterSpec} should be used to set + * the parameters to use for building the master key. Calling either function after + * the other will throw an {@link IllegalArgumentException}. + * + * @param keyScheme The KeyScheme to use. + * @return This builder. + */ + public Builder setKeyScheme(KeyScheme keyScheme) { + return null; + } + + /** + * When used with {@link #setKeyScheme(KeyScheme)}, sets that the built master key should + * require the user to authenticate before it's unlocked, probably using the + * androidx.biometric library. + * + * This method sets the validity duration of the key to + * {@link #getDefaultAuthenticationValidityDurationSeconds()}. + * + * @param authenticationRequired Whether user authentication should be required to use + * the key. + * @return This builder. + */ + public Builder setUserAuthenticationRequired(boolean authenticationRequired) { + return null; + } + + /** + * When used with {@link #setKeyScheme(KeyScheme)}, sets that the built master key should + * require the user to authenticate before it's unlocked, probably using the + * androidx.biometric library, and that the key should remain unlocked for the provided + * duration. + * + * @param authenticationRequired Whether user authentication should be + * required to use the key. + * @param userAuthenticationValidityDurationSeconds Duration in seconds that the key + * should remain unlocked following user + * authentication. + * @return This builder. + */ + public Builder setUserAuthenticationRequired(boolean authenticationRequired, + int userAuthenticationValidityDurationSeconds) { + return null; + } + + /** + * Sets whether or not to request this key is strong box backed. This setting is only + * applicable on {@link Build.VERSION_CODES#P} and above, and only on devices that + * support Strongbox. + * + * @param requestStrongBoxBacked Whether to request to use strongbox + * @return This builder. + */ + public Builder setRequestStrongBoxBacked(boolean requestStrongBoxBacked) { + return null; + } + + /** + * Builds a {@link MasterKey} from this builder. + * + * @return The master key. + */ + public MasterKey build() throws GeneralSecurityException, IOException { + return null; + } + } +} \ No newline at end of file From 85434ca410328469bb03f781b309002cf2e5bd05 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Tue, 17 Nov 2020 21:20:53 +0000 Subject: [PATCH 02/11] Format the source code and update qldoc --- .../CWE-312/ClearTextStorageSharedPrefs.java | 36 +++++++++--------- .../CWE-312/ClearTextStorageSharedPrefs.qhelp | 4 +- .../CWE-312/ClearTextStorageSharedPrefs.ql | 2 +- .../CWE-312/ClearTextStorageSharedPrefs.java | 37 +++++++++---------- 4 files changed, 39 insertions(+), 40 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.java b/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.java index 259ee23bf819..f4fbeaa230b4 100644 --- a/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.java +++ b/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.java @@ -1,7 +1,7 @@ -public void testSetSharedPrefs(Context context, String name, String password) +public void testSetSharedPrefs(Context context, String name, String password) { - { - // BAD - save sensitive information in cleartext + { + // BAD - save sensitive information in cleartext SharedPreferences sharedPrefs = context.getSharedPreferences("user_prefs", Context.MODE_PRIVATE); Editor editor = sharedPrefs.edit(); editor.putString("name", name); @@ -9,8 +9,8 @@ public void testSetSharedPrefs(Context context, String name, String password) editor.commit(); } - { - // GOOD - save sensitive information in encrypted format + { + // 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)); @@ -18,20 +18,20 @@ public void testSetSharedPrefs(Context context, String name, String 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(); + { + // 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(); + 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 index f040ed4132d4..d06b8f7bf262 100644 --- a/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.qhelp +++ b/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.qhelp @@ -18,7 +18,7 @@

    - In the second and third examples, the code encrypts sensitive information before saving to the device. + In the second and third examples, the code encrypts sensitive information before saving it to the device.

    @@ -33,7 +33,7 @@ Work with data more securely
  • - PRO ANDROID DEV: + ProAndroidDev: Encrypted Preferences in Android
    • diff --git a/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.ql b/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.ql index 5807ea196840..20b90c5a33de 100644 --- a/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.ql +++ b/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.ql @@ -1,6 +1,6 @@ /** * @name Cleartext storage of sensitive information using `SharedPreferences` on Android - * @description Cleartext Storage of Sensitive Information using SharedPreferences on Android allows user with root privileges to access or unexpected exposure from chained vulnerabilities. + * @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 diff --git a/java/ql/test/experimental/query-tests/security/CWE-312/ClearTextStorageSharedPrefs.java b/java/ql/test/experimental/query-tests/security/CWE-312/ClearTextStorageSharedPrefs.java index 73423a065cc0..d9484b82ef4b 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-312/ClearTextStorageSharedPrefs.java +++ b/java/ql/test/experimental/query-tests/security/CWE-312/ClearTextStorageSharedPrefs.java @@ -25,30 +25,29 @@ public void testSetSharedPrefs2(Context context, String name, String password) { editor.commit(); } - 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; - } + 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; + } // GOOD - save sensitive information using the built-in `EncryptedSharedPreferences` class in androidx. public void testSetSharedPrefs3(Context context, String name, String password) { - MasterKey masterKey = new MasterKey.Builder(context, MasterKey.DEFAULT_MASTER_KEY_ALIAS) - .setKeyScheme(MasterKey.KeyScheme.AES256_GCM) - .build(); + 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 sharedPreferences = EncryptedSharedPreferences.create( - context, - "secret_shared_prefs", - masterKey, - EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV, - EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM); - - // Use the shared preferences and editor as you normally would - SharedPreferences.Editor editor = sharedPreferences.edit(); + // Use the shared preferences and editor as you normally would + SharedPreferences.Editor editor = sharedPreferences.edit(); editor.putString("name", name); editor.putString("password", password); editor.commit(); } - -} \ No newline at end of file +} From a31146279123f20abca7204d71d4d094ca0e168c Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Thu, 19 Nov 2020 13:12:42 +0000 Subject: [PATCH 03/11] Move to query-test folder and update qldoc --- java/ql/src/Security/CWE/CWE-312/SensitiveStorage.qll | 2 +- .../CWE-312/semmle/tests}/ClearTextStorageSharedPrefs.expected | 0 .../CWE-312/semmle/tests}/ClearTextStorageSharedPrefs.java | 0 .../CWE-312/semmle/tests}/ClearTextStorageSharedPrefs.qlref | 0 .../security/CWE-312/semmle/tests}/options | 2 +- 5 files changed, 2 insertions(+), 2 deletions(-) rename java/ql/test/{experimental/query-tests/security/CWE-312 => query-tests/security/CWE-312/semmle/tests}/ClearTextStorageSharedPrefs.expected (100%) rename java/ql/test/{experimental/query-tests/security/CWE-312 => query-tests/security/CWE-312/semmle/tests}/ClearTextStorageSharedPrefs.java (100%) rename java/ql/test/{experimental/query-tests/security/CWE-312 => query-tests/security/CWE-312/semmle/tests}/ClearTextStorageSharedPrefs.qlref (100%) rename java/ql/test/{experimental/query-tests/security/CWE-312 => query-tests/security/CWE-312/semmle/tests}/options (64%) diff --git a/java/ql/src/Security/CWE/CWE-312/SensitiveStorage.qll b/java/ql/src/Security/CWE/CWE-312/SensitiveStorage.qll index a63d5e43891f..8b61faf37502 100644 --- a/java/ql/src/Security/CWE/CWE-312/SensitiveStorage.qll +++ b/java/ql/src/Security/CWE/CWE-312/SensitiveStorage.qll @@ -258,7 +258,7 @@ private predicate sharedPreferencesInput(DataFlow::Node sharedPrefs, Expr input) ) } -/* Holds if the method call is the save method of `SharedPreferences`. */ +/* 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 diff --git a/java/ql/test/experimental/query-tests/security/CWE-312/ClearTextStorageSharedPrefs.expected b/java/ql/test/query-tests/security/CWE-312/semmle/tests/ClearTextStorageSharedPrefs.expected similarity index 100% rename from java/ql/test/experimental/query-tests/security/CWE-312/ClearTextStorageSharedPrefs.expected rename to java/ql/test/query-tests/security/CWE-312/semmle/tests/ClearTextStorageSharedPrefs.expected diff --git a/java/ql/test/experimental/query-tests/security/CWE-312/ClearTextStorageSharedPrefs.java b/java/ql/test/query-tests/security/CWE-312/semmle/tests/ClearTextStorageSharedPrefs.java similarity index 100% rename from java/ql/test/experimental/query-tests/security/CWE-312/ClearTextStorageSharedPrefs.java rename to java/ql/test/query-tests/security/CWE-312/semmle/tests/ClearTextStorageSharedPrefs.java diff --git a/java/ql/test/experimental/query-tests/security/CWE-312/ClearTextStorageSharedPrefs.qlref b/java/ql/test/query-tests/security/CWE-312/semmle/tests/ClearTextStorageSharedPrefs.qlref similarity index 100% rename from java/ql/test/experimental/query-tests/security/CWE-312/ClearTextStorageSharedPrefs.qlref rename to java/ql/test/query-tests/security/CWE-312/semmle/tests/ClearTextStorageSharedPrefs.qlref diff --git a/java/ql/test/experimental/query-tests/security/CWE-312/options b/java/ql/test/query-tests/security/CWE-312/semmle/tests/options similarity index 64% rename from java/ql/test/experimental/query-tests/security/CWE-312/options rename to java/ql/test/query-tests/security/CWE-312/semmle/tests/options index 43e25f608b67..81fb6f970ab4 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-312/options +++ b/java/ql/test/query-tests/security/CWE-312/semmle/tests/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 From a49160423bccde35a442fb2e276064676f84087f Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Wed, 25 Nov 2020 04:33:26 +0000 Subject: [PATCH 04/11] Enhance the query and add more test cases --- .../CWE-312/ClearTextStorageSharedPrefs.qhelp | 2 +- .../Security/CWE/CWE-312/SensitiveStorage.qll | 65 +++++++++++--- .../frameworks/android/SharedPreferences.qll | 88 +++++++++++-------- .../ClearTextStorageSharedPrefs.expected | 2 +- .../tests/ClearTextStorageSharedPrefs.java | 66 ++++++++++++-- 5 files changed, 166 insertions(+), 57 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.qhelp b/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.qhelp index d06b8f7bf262..681ff9fe95e2 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 8b61faf37502..bc629532b9fc 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 7ac1f3bb1df8..59657ffb6e6d 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") + /* The class `androidx.security.crypto.EncryptedSharedPreferences`, which implements `SharedPreferences` with encryption support. */ + 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%") + /* The create method of `androidx.security.crypto.EncryptedSharedPreferences` */ + class EncryptedSharedPrefsCreateMethod extends Method { + EncryptedSharedPrefsCreateMethod() { + getDeclaringType() instanceof TypeEncryptedSharedPreferences and + hasName("create") + } } -} -/* 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 + /* A getter method of `android.content.SharedPreferences`. */ + class SharedPreferencesGetMethod extends Method { + SharedPreferencesGetMethod() { + getDeclaringType() instanceof TypeSharedPreferences and + getName().matches("get%") + } } -} -/* Definitions related to `android.content.SharedPreferences.Editor`. */ -library class TypeSharedPreferencesEditor extends Interface { - TypeSharedPreferencesEditor() { hasQualifiedName("android.content", "SharedPreferences$Editor") } -} + /* 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 + } + } -/* A setter method for `android.content.SharedPreferences`. */ -library class SharedPreferencesSetMethod extends Method { - SharedPreferencesSetMethod() { - getDeclaringType() instanceof TypeSharedPreferencesEditor and - getName().matches("put%") + /* 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`. */ -library class SharedPreferencesStoreMethod extends Method { - SharedPreferencesStoreMethod() { - getDeclaringType() instanceof TypeSharedPreferencesEditor and - hasName(["commit", "apply"]) + /* 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 63ddf22798cf..592a74db89d1 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 d9484b82ef4b..dbc1f54b59c3 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.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 void testSetSharedPrefs2(Context context, String name, String password) { } 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 void testSetSharedPrefs3(Context context, String name, String password) { 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(); + } } From 7ad031ca70c2dc1cb5ac27a5aebd824a94f80f03 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Thu, 26 Nov 2020 17:09:53 +0000 Subject: [PATCH 05/11] Move to experimental and update qldoc --- .../CWE-312/ClearTextStorageSharedPrefs.ql | 22 --- .../Security/CWE/CWE-312/SensitiveStorage.qll | 115 ------------ .../CWE-312/CleartextStorageSharedPrefs.java} | 0 .../CleartextStorageSharedPrefs.qhelp} | 2 +- .../CWE-312/CleartextStorageSharedPrefs.ql | 167 ++++++++++++++++++ .../frameworks/android/SharedPreferences.qll | 64 ++++--- .../CleartextStorageSharedPrefs.expected | 22 +++ .../CWE-312/CleartextStorageSharedPrefs.java} | 2 +- .../CWE-312/CleartextStorageSharedPrefs.qlref | 1 + .../query-tests/security/CWE-312}/options | 2 +- .../ClearTextStorageSharedPrefs.expected | 1 - .../tests/ClearTextStorageSharedPrefs.qlref | 1 - 12 files changed, 224 insertions(+), 175 deletions(-) delete mode 100644 java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.ql rename java/ql/src/{Security/CWE/CWE-312/ClearTextStorageSharedPrefs.java => experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.java} (100%) rename java/ql/src/{Security/CWE/CWE-312/ClearTextStorageSharedPrefs.qhelp => experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.qhelp} (96%) create mode 100644 java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql create mode 100644 java/ql/test/experimental/query-tests/security/CWE-312/CleartextStorageSharedPrefs.expected rename java/ql/test/{query-tests/security/CWE-312/semmle/tests/ClearTextStorageSharedPrefs.java => experimental/query-tests/security/CWE-312/CleartextStorageSharedPrefs.java} (98%) create mode 100644 java/ql/test/experimental/query-tests/security/CWE-312/CleartextStorageSharedPrefs.qlref rename java/ql/test/{query-tests/security/CWE-312/semmle/tests => experimental/query-tests/security/CWE-312}/options (64%) delete mode 100644 java/ql/test/query-tests/security/CWE-312/semmle/tests/ClearTextStorageSharedPrefs.expected delete mode 100644 java/ql/test/query-tests/security/CWE-312/semmle/tests/ClearTextStorageSharedPrefs.qlref diff --git a/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.ql b/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.ql deleted file mode 100644 index 20b90c5a33de..000000000000 --- a/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.ql +++ /dev/null @@ -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" diff --git a/java/ql/src/Security/CWE/CWE-312/SensitiveStorage.qll b/java/ql/src/Security/CWE/CWE-312/SensitiveStorage.qll index bc629532b9fc..b07105e3bf5a 100644 --- a/java/ql/src/Security/CWE/CWE-312/SensitiveStorage.qll +++ b/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) - ) - } -} diff --git a/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.java b/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.java similarity index 100% rename from java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.java rename to java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.java diff --git a/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.qhelp b/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.qhelp similarity index 96% rename from java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.qhelp rename to java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.qhelp index 681ff9fe95e2..90f87f30b91d 100644 --- a/java/ql/src/Security/CWE/CWE-312/ClearTextStorageSharedPrefs.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.qhelp @@ -20,7 +20,7 @@

    In the second and third examples, the code encrypts sensitive information before saving it to the device.

    - + diff --git a/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql b/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql new file mode 100644 index 000000000000..7d03b79882a6 --- /dev/null +++ b/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" 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 59657ffb6e6d..2f3a98162a0f 100644 --- a/java/ql/src/semmle/code/java/frameworks/android/SharedPreferences.qll +++ b/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"]) } } diff --git a/java/ql/test/experimental/query-tests/security/CWE-312/CleartextStorageSharedPrefs.expected b/java/ql/test/experimental/query-tests/security/CWE-312/CleartextStorageSharedPrefs.expected new file mode 100644 index 000000000000..ad8b4416e752 --- /dev/null +++ b/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 | diff --git a/java/ql/test/query-tests/security/CWE-312/semmle/tests/ClearTextStorageSharedPrefs.java b/java/ql/test/experimental/query-tests/security/CWE-312/CleartextStorageSharedPrefs.java similarity index 98% rename from java/ql/test/query-tests/security/CWE-312/semmle/tests/ClearTextStorageSharedPrefs.java rename to java/ql/test/experimental/query-tests/security/CWE-312/CleartextStorageSharedPrefs.java index dbc1f54b59c3..51a92314c67f 100644 --- a/java/ql/test/query-tests/security/CWE-312/semmle/tests/ClearTextStorageSharedPrefs.java +++ b/java/ql/test/experimental/query-tests/security/CWE-312/CleartextStorageSharedPrefs.java @@ -9,7 +9,7 @@ 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); diff --git a/java/ql/test/experimental/query-tests/security/CWE-312/CleartextStorageSharedPrefs.qlref b/java/ql/test/experimental/query-tests/security/CWE-312/CleartextStorageSharedPrefs.qlref new file mode 100644 index 000000000000..9319c78be055 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-312/CleartextStorageSharedPrefs.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql diff --git a/java/ql/test/query-tests/security/CWE-312/semmle/tests/options b/java/ql/test/experimental/query-tests/security/CWE-312/options similarity index 64% rename from java/ql/test/query-tests/security/CWE-312/semmle/tests/options rename to java/ql/test/experimental/query-tests/security/CWE-312/options index 81fb6f970ab4..43e25f608b67 100644 --- a/java/ql/test/query-tests/security/CWE-312/semmle/tests/options +++ b/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 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 deleted file mode 100644 index 592a74db89d1..000000000000 --- a/java/ql/test/query-tests/security/CWE-312/semmle/tests/ClearTextStorageSharedPrefs.expected +++ /dev/null @@ -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 | diff --git a/java/ql/test/query-tests/security/CWE-312/semmle/tests/ClearTextStorageSharedPrefs.qlref b/java/ql/test/query-tests/security/CWE-312/semmle/tests/ClearTextStorageSharedPrefs.qlref deleted file mode 100644 index a7547b790fd7..000000000000 --- a/java/ql/test/query-tests/security/CWE-312/semmle/tests/ClearTextStorageSharedPrefs.qlref +++ /dev/null @@ -1 +0,0 @@ -Security/CWE/CWE-312/ClearTextStorageSharedPrefs.ql From a83ddd66eb513094957bb08b85ef2ce46be89e6b Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Thu, 26 Nov 2020 17:41:46 +0000 Subject: [PATCH 06/11] Add comments about how the future promotion should go --- .../Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql b/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql index 7d03b79882a6..ddc88bda54b5 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql @@ -127,6 +127,10 @@ class SharedPreferencesEditor extends MethodAccess { } } +/** + * Flow from sensitive expressions to shared preferences. + * Note it can be merged into `SensitiveSourceFlowConfig` of `Security/CWE/CWE-312/SensitiveStorage.qll` when this query is promoted from the experimental directory. + */ private class SensitiveSharedPrefsFlowConfig extends TaintTracking::Configuration { SensitiveSharedPrefsFlowConfig() { this = "CleartextStorageSharedPrefs::SensitiveSharedPrefsFlowConfig" From ad0ac5b874158a0601498ee9d11dfd4b01abf3ef Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Fri, 27 Nov 2020 16:43:57 +0000 Subject: [PATCH 07/11] Change kind to problem --- .../CWE-312/CleartextStorageSharedPrefs.ql | 3 +-- .../CleartextStorageSharedPrefs.expected | 21 ------------------- 2 files changed, 1 insertion(+), 23 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql b/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql index ddc88bda54b5..e505b4fc6cd0 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql @@ -1,7 +1,7 @@ /** * @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 + * @kind problem * @id java/android/cleartext-storage-shared-prefs * @tags security * external/cwe/cwe-312 @@ -14,7 +14,6 @@ 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) { diff --git a/java/ql/test/experimental/query-tests/security/CWE-312/CleartextStorageSharedPrefs.expected b/java/ql/test/experimental/query-tests/security/CWE-312/CleartextStorageSharedPrefs.expected index ad8b4416e752..ab1db3fe099d 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-312/CleartextStorageSharedPrefs.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-312/CleartextStorageSharedPrefs.expected @@ -1,22 +1 @@ -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 | From 5690bf49f46f9cadfb99cb824338b1562a9e0831 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Wed, 6 Jan 2021 16:21:26 +0000 Subject: [PATCH 08/11] Optimize the query --- .../CWE/CWE-312/CleartextStorageSharedPrefs.ql | 13 +++---------- .../java/frameworks/android/SharedPreferences.qll | 4 +++- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql b/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql index e505b4fc6cd0..fe463a33eb65 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql @@ -35,7 +35,7 @@ private predicate sharedPreferencesStore(DataFlow::Node sharedPrefs, Expr store) } /** Flow from `SharedPreferences` to either a setter or a store method. */ -class SharedPreferencesFlowConfig extends TaintTracking::Configuration { +class SharedPreferencesFlowConfig extends DataFlow::Configuration { SharedPreferencesFlowConfig() { this = "CleartextStorageSharedPrefs::SharedPreferencesFlowConfig" } @@ -65,7 +65,7 @@ class EncryptedValueFlowConfig extends DataFlow5::Configuration { EncryptedValueFlowConfig() { this = "CleartextStorageSharedPrefs::EncryptedValueFlowConfig" } override predicate isSource(DataFlow5::Node src) { - exists(EncryptedSensitiveMethodAccess ema | src.asExpr() = ema.getAnArgument()) + exists(EncryptedSensitiveMethodAccess ema | src.asExpr() = ema) } override predicate isSink(DataFlow5::Node sink) { @@ -74,13 +74,6 @@ class EncryptedValueFlowConfig extends DataFlow5::Configuration { 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. */ @@ -109,7 +102,7 @@ class SharedPreferencesEditor extends MethodAccess { ) } - /** Gets an input, for example `input` in `editor.putString("password", password);`. */ + /** Gets an input, for example `password` in `editor.putString("password", password);`. */ Expr getAnInput() { exists(SharedPreferencesFlowConfig conf, DataFlow::Node n | sharedPreferencesInput(n, result) and 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 2f3a98162a0f..78cea4a1386f 100644 --- a/java/ql/src/semmle/code/java/frameworks/android/SharedPreferences.qll +++ b/java/ql/src/semmle/code/java/frameworks/android/SharedPreferences.qll @@ -1,4 +1,6 @@ -import semmle.code.java.Type +/** Provides classes related to `android.content.SharedPreferences`. */ + +import java /** Definitions related to `android.content.SharedPreferences`. */ module SharedPreferences { From f13b8814f5d00a2ae3e8826bbcce92b5c2a6c29f Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Wed, 6 Jan 2021 16:49:35 +0000 Subject: [PATCH 09/11] Update class/method names in the module --- .../CWE-312/CleartextStorageSharedPrefs.ql | 4 ++-- .../frameworks/android/SharedPreferences.qll | 20 +++++++++---------- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql b/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql index fe463a33eb65..e82430a74642 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql @@ -83,11 +83,11 @@ private class EncryptedSharedPrefFlowConfig extends DataFlow4::Configuration { } override predicate isSource(DataFlow4::Node src) { - src.asExpr().(MethodAccess).getMethod() instanceof SharedPreferences::CreateEncryptedPrefsMethod + src.asExpr().(MethodAccess).getMethod() instanceof SharedPreferences::CreateEncryptedMethod } override predicate isSink(DataFlow4::Node sink) { - sink.asExpr().getType() instanceof SharedPreferences::TypePrefs + sink.asExpr().getType() instanceof SharedPreferences::TypeBase } } 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 78cea4a1386f..28816eabd2d1 100644 --- a/java/ql/src/semmle/code/java/frameworks/android/SharedPreferences.qll +++ b/java/ql/src/semmle/code/java/frameworks/android/SharedPreferences.qll @@ -5,21 +5,19 @@ import java /** Definitions related to `android.content.SharedPreferences`. */ module SharedPreferences { /** The interface `android.content.SharedPreferences` */ - class TypePrefs extends Interface { - TypePrefs() { hasQualifiedName("android.content", "SharedPreferences") } + class TypeBase extends Interface { + TypeBase() { hasQualifiedName("android.content", "SharedPreferences") } } /** The class `androidx.security.crypto.EncryptedSharedPreferences`, which implements `SharedPreferences` with encryption support. */ - class TypeEncryptedPrefs extends Class { - TypeEncryptedPrefs() { - hasQualifiedName("androidx.security.crypto", "EncryptedSharedPreferences") - } + class TypeEncrypted extends Class { + TypeEncrypted() { hasQualifiedName("androidx.security.crypto", "EncryptedSharedPreferences") } } /** The create method of `androidx.security.crypto.EncryptedSharedPreferences` */ - class CreateEncryptedPrefsMethod extends Method { - CreateEncryptedPrefsMethod() { - getDeclaringType() instanceof TypeEncryptedPrefs and + class CreateEncryptedMethod extends Method { + CreateEncryptedMethod() { + getDeclaringType() instanceof TypeEncrypted and hasName("create") } } @@ -27,7 +25,7 @@ module SharedPreferences { /** A getter method of `android.content.SharedPreferences`. */ class GetPreferenceMethod extends Method { GetPreferenceMethod() { - getDeclaringType() instanceof TypePrefs and + getDeclaringType() instanceof TypeBase and getName().matches("get%") } } @@ -35,7 +33,7 @@ module SharedPreferences { /** Returns `android.content.SharedPreferences.Editor` from the `edit` call of `android.content.SharedPreferences`. */ class GetEditorMethod extends Method { GetEditorMethod() { - getDeclaringType() instanceof TypePrefs and + getDeclaringType() instanceof TypeBase and hasName("edit") and getReturnType() instanceof TypeEditor } From b54e5b1c4977d7a0c8e320893914cac927926863 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Thu, 7 Jan 2021 12:44:59 +0000 Subject: [PATCH 10/11] Revamp the library module --- .../CWE-312/CleartextStorageSharedPrefs.ql | 24 +++--- .../frameworks/android/SharedPreferences.qll | 85 ++++++++----------- 2 files changed, 49 insertions(+), 60 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql b/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql index e82430a74642..fcfe3f826516 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-312/CleartextStorageSharedPrefs.ql @@ -18,7 +18,7 @@ import semmle.code.java.security.SensitiveActions /** 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 + m.getMethod() instanceof PutSharedPreferenceMethod and input = m.getArgument(1) and not exists(EncryptedValueFlowConfig conf | conf.hasFlow(_, DataFlow::exprNode(input))) and sharedPrefs.asExpr() = m.getQualifier() @@ -28,7 +28,7 @@ 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 SharedPreferences::StorePreferenceMethod and + m.getMethod() instanceof StoreSharedPreferenceMethod and store = m and sharedPrefs.asExpr() = m.getQualifier() ) @@ -41,7 +41,7 @@ class SharedPreferencesFlowConfig extends DataFlow::Configuration { } override predicate isSource(DataFlow::Node src) { - src.asExpr() instanceof SharedPreferencesEditor + src.asExpr() instanceof SharedPreferencesEditorMethodAccess } override predicate isSink(DataFlow::Node sink) { @@ -56,7 +56,7 @@ class SharedPreferencesFlowConfig extends DataFlow::Configuration { */ class EncryptedSensitiveMethodAccess extends MethodAccess { EncryptedSensitiveMethodAccess() { - getMethod().getName().toLowerCase().matches(["%encrypt%", "%hash%"]) + this.getMethod().getName().toLowerCase().matches(["%encrypt%", "%hash%"]) } } @@ -70,7 +70,7 @@ class EncryptedValueFlowConfig extends DataFlow5::Configuration { override predicate isSink(DataFlow5::Node sink) { exists(MethodAccess ma | - ma.getMethod() instanceof SharedPreferences::SetPreferenceMethod and + ma.getMethod() instanceof PutSharedPreferenceMethod and sink.asExpr() = ma.getArgument(1) ) } @@ -83,18 +83,18 @@ private class EncryptedSharedPrefFlowConfig extends DataFlow4::Configuration { } override predicate isSource(DataFlow4::Node src) { - src.asExpr().(MethodAccess).getMethod() instanceof SharedPreferences::CreateEncryptedMethod + src.asExpr().(MethodAccess).getMethod() instanceof CreateEncryptedSharedPreferencesMethod } override predicate isSink(DataFlow4::Node sink) { - sink.asExpr().getType() instanceof SharedPreferences::TypeBase + sink.asExpr().getType() instanceof SharedPreferences } } /** 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 +class SharedPreferencesEditorMethodAccess extends MethodAccess { + SharedPreferencesEditorMethodAccess() { + this.getMethod() instanceof GetSharedPreferencesEditorMethod and not exists( EncryptedSharedPrefFlowConfig config // not exists `SharedPreferences sharedPreferences = EncryptedSharedPreferences.create(...)` | @@ -132,7 +132,7 @@ private class SensitiveSharedPrefsFlowConfig extends TaintTracking::Configuratio override predicate isSink(DataFlow::Node sink) { exists(MethodAccess m | - m.getMethod() instanceof SharedPreferences::SetPreferenceMethod and + m.getMethod() instanceof PutSharedPreferenceMethod and sink.asExpr() = m.getArgument(1) ) } @@ -154,7 +154,7 @@ class SensitiveSharedPrefsSource extends Expr { } } -from SensitiveSharedPrefsSource data, SharedPreferencesEditor s, Expr input, Expr store +from SensitiveSharedPrefsSource data, SharedPreferencesEditorMethodAccess s, Expr input, Expr store where input = s.getAnInput() and store = s.getAStore() and 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 28816eabd2d1..f526f1c272b8 100644 --- a/java/ql/src/semmle/code/java/frameworks/android/SharedPreferences.qll +++ b/java/ql/src/semmle/code/java/frameworks/android/SharedPreferences.qll @@ -1,62 +1,51 @@ -/** Provides classes related to `android.content.SharedPreferences`. */ - import java -/** Definitions related to `android.content.SharedPreferences`. */ -module SharedPreferences { - /** The interface `android.content.SharedPreferences` */ - class TypeBase extends Interface { - TypeBase() { hasQualifiedName("android.content", "SharedPreferences") } - } - - /** The class `androidx.security.crypto.EncryptedSharedPreferences`, which implements `SharedPreferences` with encryption support. */ - class TypeEncrypted extends Class { - TypeEncrypted() { hasQualifiedName("androidx.security.crypto", "EncryptedSharedPreferences") } - } +/** The interface `android.content.SharedPreferences`. */ +class SharedPreferences extends Interface { + SharedPreferences() { this.hasQualifiedName("android.content", "SharedPreferences") } +} - /** The create method of `androidx.security.crypto.EncryptedSharedPreferences` */ - class CreateEncryptedMethod extends Method { - CreateEncryptedMethod() { - getDeclaringType() instanceof TypeEncrypted and - hasName("create") - } +/** The class `androidx.security.crypto.EncryptedSharedPreferences`, which implements `SharedPreferences` with encryption support. */ +class EncryptedSharedPreferences extends Class { + EncryptedSharedPreferences() { + this.hasQualifiedName("androidx.security.crypto", "EncryptedSharedPreferences") } +} - /** A getter method of `android.content.SharedPreferences`. */ - class GetPreferenceMethod extends Method { - GetPreferenceMethod() { - getDeclaringType() instanceof TypeBase and - getName().matches("get%") - } +/** The `create` method of `androidx.security.crypto.EncryptedSharedPreferences`. */ +class CreateEncryptedSharedPreferencesMethod extends Method { + CreateEncryptedSharedPreferencesMethod() { + this.getDeclaringType() instanceof EncryptedSharedPreferences and + this.hasName("create") } +} - /** Returns `android.content.SharedPreferences.Editor` from the `edit` call of `android.content.SharedPreferences`. */ - class GetEditorMethod extends Method { - GetEditorMethod() { - getDeclaringType() instanceof TypeBase and - hasName("edit") and - getReturnType() instanceof TypeEditor - } +/** Returns `android.content.SharedPreferences.Editor` from the `edit` call of `android.content.SharedPreferences`. */ +class GetSharedPreferencesEditorMethod extends Method { + GetSharedPreferencesEditorMethod() { + this.getDeclaringType() instanceof SharedPreferences and + this.hasName("edit") and + this.getReturnType() instanceof SharedPreferencesEditor } +} - /** Definitions related to `android.content.SharedPreferences.Editor`. */ - class TypeEditor extends Interface { - TypeEditor() { hasQualifiedName("android.content", "SharedPreferences$Editor") } - } +/** The interface `android.content.SharedPreferences.Editor`. */ +class SharedPreferencesEditor extends Interface { + SharedPreferencesEditor() { this.hasQualifiedName("android.content", "SharedPreferences$Editor") } +} - /** A setter method for `android.content.SharedPreferences`. */ - class SetPreferenceMethod extends Method { - SetPreferenceMethod() { - getDeclaringType() instanceof TypeEditor and - getName().matches("put%") - } +/** A method that updates a key-value pair in a `android.content.SharedPreferences` through a `SharedPreferences.Editor`. The value is not written until a `StorePreferenceMethod` is called. */ +class PutSharedPreferenceMethod extends Method { + PutSharedPreferenceMethod() { + this.getDeclaringType() instanceof SharedPreferencesEditor and + this.getName().matches("put%") } +} - /** A setter method for `android.content.SharedPreferences`. */ - class StorePreferenceMethod extends Method { - StorePreferenceMethod() { - getDeclaringType() instanceof TypeEditor and - hasName(["commit", "apply"]) - } +/** A method on `SharedPreferences.Editor` that writes the pending changes to the underlying `android.content.SharedPreferences`. */ +class StoreSharedPreferenceMethod extends Method { + StoreSharedPreferenceMethod() { + this.getDeclaringType() instanceof SharedPreferencesEditor and + this.hasName(["commit", "apply"]) } } From 606d0946fcfbd0a4dbabda4885313bc70b1a095f Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Thu, 7 Jan 2021 14:05:12 +0000 Subject: [PATCH 11/11] Update qldoc --- .../code/java/frameworks/android/SharedPreferences.qll | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) 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 f526f1c272b8..a3298fd70d87 100644 --- a/java/ql/src/semmle/code/java/frameworks/android/SharedPreferences.qll +++ b/java/ql/src/semmle/code/java/frameworks/android/SharedPreferences.qll @@ -1,3 +1,5 @@ +/** Provides classes related to `android.content.SharedPreferences`. */ + import java /** The interface `android.content.SharedPreferences`. */ @@ -20,7 +22,7 @@ class CreateEncryptedSharedPreferencesMethod extends Method { } } -/** Returns `android.content.SharedPreferences.Editor` from the `edit` call of `android.content.SharedPreferences`. */ +/** The method `android.content.SharedPreferences::edit`, which returns an `android.content.SharedPreferences.Editor`. */ class GetSharedPreferencesEditorMethod extends Method { GetSharedPreferencesEditorMethod() { this.getDeclaringType() instanceof SharedPreferences and @@ -34,7 +36,11 @@ class SharedPreferencesEditor extends Interface { SharedPreferencesEditor() { this.hasQualifiedName("android.content", "SharedPreferences$Editor") } } -/** A method that updates a key-value pair in a `android.content.SharedPreferences` through a `SharedPreferences.Editor`. The value is not written until a `StorePreferenceMethod` is called. */ +/** + * A method that updates a key-value pair in a + * `android.content.SharedPreferences` through a `SharedPreferences.Editor`. The + * value is not written until a `StorePreferenceMethod` is called. + */ class PutSharedPreferenceMethod extends Method { PutSharedPreferenceMethod() { this.getDeclaringType() instanceof SharedPreferencesEditor and