From 63751bbbb2d830b7cb662e9c5fe84f4f6b82aa2b Mon Sep 17 00:00:00 2001 From: Jake Rowland Date: Fri, 6 Jul 2018 15:51:01 -0500 Subject: [PATCH 01/10] These changes are notes and fixes for multithreaded correctness within Vault. Possible synchronization issue... special note to look at the clearing of the mCachedSecretKey without a lock --- .../KeychainAuthenticatedKeyStorage.java | 6 +++- .../keys/storage/SharedPrefKeyStorage.java | 36 +++++++++++++------ 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/keys/storage/KeychainAuthenticatedKeyStorage.java b/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/keys/storage/KeychainAuthenticatedKeyStorage.java index a8c5eda..213500a 100644 --- a/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/keys/storage/KeychainAuthenticatedKeyStorage.java +++ b/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/keys/storage/KeychainAuthenticatedKeyStorage.java @@ -17,6 +17,7 @@ import android.annotation.TargetApi; import android.content.Context; +import android.nfc.Tag; import android.os.Build; import android.security.keystore.KeyGenParameterSpec; import android.security.keystore.KeyProperties; @@ -49,7 +50,10 @@ public class KeychainAuthenticatedKeyStorage implements KeyStorage { private final String mPadding; private final int mAuthDurationSeconds; - private final String mKeyLock = "keyLock"; + // private final String mKeyLock = "keyLock" + // Final equivalent strings over multiple classes are represented by the same String literal + // in memory https://www.javalobby.org//java/forums/t96352.html + private final Object mKeyLock = new Object(); public KeychainAuthenticatedKeyStorage(String keyAlias, String algorithm, String blockMode, String padding, int authDurationSeconds) { mKeyAlias = keyAlias; diff --git a/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/keys/storage/SharedPrefKeyStorage.java b/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/keys/storage/SharedPrefKeyStorage.java index 5eca0de..5645820 100644 --- a/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/keys/storage/SharedPrefKeyStorage.java +++ b/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/keys/storage/SharedPrefKeyStorage.java @@ -40,8 +40,17 @@ public class SharedPrefKeyStorage implements KeyStorage { private final String mPrefFileName; private final String mKeystoreAlias; private final String mCipherAlgorithm; - private SecretKey mCachedSecretKey; - private final String mKeyLock = "keyLock"; + + // Should mCachedSecretKey be Volatile? + // https://www.javamex.com/tutorials/synchronization_volatile.shtml + // Should be volatile with synchronous locks when accessing. This makes read, update, and write + // atomic functions + private volatile SecretKey mCachedSecretKey; + + // private final String mKeyLock = "keyLock" + // Final equivalent strings over multiple classes are represented by the same String literal + // in memory https://www.javalobby.org//java/forums/t96352.html + private final Object mKeyLock = new Object(); public SharedPrefKeyStorage(SecretKeyWrapper secretKeyWrapper, String prefFileName, String keystoreAlias, String cipherAlgorithm) { mSecretKeyWrapper = secretKeyWrapper; @@ -51,32 +60,39 @@ public SharedPrefKeyStorage(SecretKeyWrapper secretKeyWrapper, String prefFileNa } @Override - public SecretKey loadKey(Context context) { - if (mCachedSecretKey == null) { + public synchronized SecretKey loadKey(Context context) { + Log.v(TAG, "loadKey"); + // "Double-Check" Locking http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html + // if (mCachedSecretKey == null) { //Only allow one thread at a time load the key. - synchronized (mKeyLock) { - //If the other thread updated the key, don't re-load it. - if (mCachedSecretKey == null) { - mCachedSecretKey = loadSecretKey(context, mKeystoreAlias, mCipherAlgorithm); - } - } + //synchronized (mKeyLock) { + //If the other thread updated the key, don't re-load it. + if (mCachedSecretKey == null) { + mCachedSecretKey = loadSecretKey(context, mKeystoreAlias, mCipherAlgorithm); } + //} + //} return mCachedSecretKey; } @Override public boolean saveKey(Context context, SecretKey secretKey) { + Log.v(TAG, "saveKey"); boolean success; synchronized (mKeyLock) { success = storeSecretKey(context, mKeystoreAlias, secretKey); //Clear the cached key upon failure to save. + + // Inconsistent synchronization only when mCachedSecretKey is not volatile mCachedSecretKey = success ? secretKey : null; } return success; } + // Why is clearing the key not synchronous, couldn't other threads be using the cached key? @Override public void clearKey(Context context) { + // Not accessed synchronously mCachedSecretKey = null; storeSecretKey(context, mKeystoreAlias, null); try { From 4ff97a73b5c566d07c39e7a40649f3de0985ce49 Mon Sep 17 00:00:00 2001 From: Jake Rowland Date: Mon, 9 Jul 2018 13:14:19 -0500 Subject: [PATCH 02/10] Addes changes to KeychainAuth that were added to SharedPref. Synchronizes more methods that access the cached secret key. --- .../KeychainAuthenticatedKeyStorage.java | 142 +++++++++--------- .../keys/storage/SharedPrefKeyStorage.java | 33 +--- 2 files changed, 73 insertions(+), 102 deletions(-) diff --git a/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/keys/storage/KeychainAuthenticatedKeyStorage.java b/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/keys/storage/KeychainAuthenticatedKeyStorage.java index 213500a..f70b7dc 100644 --- a/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/keys/storage/KeychainAuthenticatedKeyStorage.java +++ b/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/keys/storage/KeychainAuthenticatedKeyStorage.java @@ -64,30 +64,28 @@ public KeychainAuthenticatedKeyStorage(String keyAlias, String algorithm, String } @Override - public SecretKey loadKey(Context context) { + public synchronized SecretKey loadKey(Context context) { SecretKey secretKey = null; - synchronized (mKeyLock) { - try { - KeyStore keyStore = KeyStore.getInstance(EncryptionConstants.ANDROID_KEY_STORE); - keyStore.load(null); - secretKey = (SecretKey) keyStore.getKey(mKeyAlias, null); - } catch (KeyStoreException e) { - Log.e(TAG, "Caught java.security.KeyStoreException", e); - } catch (CertificateException e) { - Log.e(TAG, "Caught java.security.cert.CertificateException", e); - } catch (NoSuchAlgorithmException e) { - Log.e(TAG, "Caught java.security.NoSuchAlgorithmException", e); - } catch (IOException e) { - Log.e(TAG, "Caught java.io.IOException", e); - } catch (UnrecoverableKeyException e) { - Log.e(TAG, "Caught java.security.UnrecoverableKeyException", e); - } + try { + KeyStore keyStore = KeyStore.getInstance(EncryptionConstants.ANDROID_KEY_STORE); + keyStore.load(null); + secretKey = (SecretKey) keyStore.getKey(mKeyAlias, null); + } catch (KeyStoreException e) { + Log.e(TAG, "Caught java.security.KeyStoreException", e); + } catch (CertificateException e) { + Log.e(TAG, "Caught java.security.cert.CertificateException", e); + } catch (NoSuchAlgorithmException e) { + Log.e(TAG, "Caught java.security.NoSuchAlgorithmException", e); + } catch (IOException e) { + Log.e(TAG, "Caught java.io.IOException", e); + } catch (UnrecoverableKeyException e) { + Log.e(TAG, "Caught java.security.UnrecoverableKeyException", e); } return secretKey; } @Override - public boolean saveKey(Context context, SecretKey secretKey) { + public synchronized boolean saveKey(Context context, SecretKey secretKey) { if (secretKey != null) { throw new IllegalArgumentException("Cannot be manually keyed. The key is generated by the Keystore itself. The argument secretKey must be null."); } @@ -96,72 +94,66 @@ public boolean saveKey(Context context, SecretKey secretKey) { @TargetApi(Build.VERSION_CODES.M) private boolean automaticallyCreateKey() { - synchronized (mKeyLock) { - try { - KeyStore keyStore = KeyStore.getInstance(EncryptionConstants.ANDROID_KEY_STORE); - keyStore.load(null); - KeyGenerator keyGenerator = KeyGenerator.getInstance(mAlgorithm, EncryptionConstants.ANDROID_KEY_STORE); - - keyGenerator.init(new KeyGenParameterSpec.Builder(mKeyAlias, KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT) - .setBlockModes(mBlockMode) - .setUserAuthenticationRequired(true) - .setUserAuthenticationValidityDurationSeconds(mAuthDurationSeconds) - .setEncryptionPaddings(mPadding) - .build()); - - return keyGenerator.generateKey() != null; - } catch (KeyStoreException e) { - Log.e(TAG, "Caught java.security.KeyStoreException", e); - } catch (CertificateException e) { - Log.e(TAG, "Caught java.security.cert.CertificateException", e); - } catch (NoSuchAlgorithmException e) { - Log.e(TAG, "Caught java.security.NoSuchAlgorithmException", e); - } catch (InvalidAlgorithmParameterException e) { - Log.e(TAG, "Caught java.security.InvalidAlgorithmParameterException", e); - } catch (NoSuchProviderException e) { - Log.e(TAG, "Caught java.security.NoSuchProviderException", e); - } catch (IOException e) { - Log.e(TAG, "Caught java.io.IOException", e); - } + try { + KeyStore keyStore = KeyStore.getInstance(EncryptionConstants.ANDROID_KEY_STORE); + keyStore.load(null); + KeyGenerator keyGenerator = KeyGenerator.getInstance(mAlgorithm, EncryptionConstants.ANDROID_KEY_STORE); + + keyGenerator.init(new KeyGenParameterSpec.Builder(mKeyAlias, KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT) + .setBlockModes(mBlockMode) + .setUserAuthenticationRequired(true) + .setUserAuthenticationValidityDurationSeconds(mAuthDurationSeconds) + .setEncryptionPaddings(mPadding) + .build()); + + return keyGenerator.generateKey() != null; + } catch (KeyStoreException e) { + Log.e(TAG, "Caught java.security.KeyStoreException", e); + } catch (CertificateException e) { + Log.e(TAG, "Caught java.security.cert.CertificateException", e); + } catch (NoSuchAlgorithmException e) { + Log.e(TAG, "Caught java.security.NoSuchAlgorithmException", e); + } catch (InvalidAlgorithmParameterException e) { + Log.e(TAG, "Caught java.security.InvalidAlgorithmParameterException", e); + } catch (NoSuchProviderException e) { + Log.e(TAG, "Caught java.security.NoSuchProviderException", e); + } catch (IOException e) { + Log.e(TAG, "Caught java.io.IOException", e); } return false; } @Override - public void clearKey(Context context) { - synchronized (mKeyLock) { - try { - KeyStore keyStore = KeyStore.getInstance(EncryptionConstants.ANDROID_KEY_STORE); - keyStore.load(null); - keyStore.deleteEntry(mKeyAlias); - } catch (KeyStoreException e) { - Log.e(TAG, "Caught java.security.KeyStoreException", e); - } catch (CertificateException e) { - Log.e(TAG, "Caught java.security.cert.CertificateException", e); - } catch (NoSuchAlgorithmException e) { - Log.e(TAG, "Caught java.security.NoSuchAlgorithmException", e); - } catch (IOException e) { - Log.e(TAG, "Caught java.io.IOException", e); - } + public synchronized void clearKey(Context context) { + try { + KeyStore keyStore = KeyStore.getInstance(EncryptionConstants.ANDROID_KEY_STORE); + keyStore.load(null); + keyStore.deleteEntry(mKeyAlias); + } catch (KeyStoreException e) { + Log.e(TAG, "Caught java.security.KeyStoreException", e); + } catch (CertificateException e) { + Log.e(TAG, "Caught java.security.cert.CertificateException", e); + } catch (NoSuchAlgorithmException e) { + Log.e(TAG, "Caught java.security.NoSuchAlgorithmException", e); + } catch (IOException e) { + Log.e(TAG, "Caught java.io.IOException", e); } } @Override - public boolean hasKey(Context context) { - synchronized (mKeyLock) { - try { - KeyStore keyStore = KeyStore.getInstance(EncryptionConstants.ANDROID_KEY_STORE); - keyStore.load(null); - return keyStore.containsAlias(mKeyAlias); - } catch (KeyStoreException e) { - Log.e(TAG, "Caught java.security.KeyStoreException", e); - } catch (CertificateException e) { - Log.e(TAG, "Caught java.security.cert.CertificateException", e); - } catch (NoSuchAlgorithmException e) { - Log.e(TAG, "Caught java.security.NoSuchAlgorithmException", e); - } catch (IOException e) { - Log.e(TAG, "Caught java.io.IOException", e); - } + public synchronized boolean hasKey(Context context) { + try { + KeyStore keyStore = KeyStore.getInstance(EncryptionConstants.ANDROID_KEY_STORE); + keyStore.load(null); + return keyStore.containsAlias(mKeyAlias); + } catch (KeyStoreException e) { + Log.e(TAG, "Caught java.security.KeyStoreException", e); + } catch (CertificateException e) { + Log.e(TAG, "Caught java.security.cert.CertificateException", e); + } catch (NoSuchAlgorithmException e) { + Log.e(TAG, "Caught java.security.NoSuchAlgorithmException", e); + } catch (IOException e) { + Log.e(TAG, "Caught java.io.IOException", e); } return false; } diff --git a/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/keys/storage/SharedPrefKeyStorage.java b/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/keys/storage/SharedPrefKeyStorage.java index 5645820..5f706d9 100644 --- a/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/keys/storage/SharedPrefKeyStorage.java +++ b/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/keys/storage/SharedPrefKeyStorage.java @@ -41,16 +41,8 @@ public class SharedPrefKeyStorage implements KeyStorage { private final String mKeystoreAlias; private final String mCipherAlgorithm; - // Should mCachedSecretKey be Volatile? - // https://www.javamex.com/tutorials/synchronization_volatile.shtml - // Should be volatile with synchronous locks when accessing. This makes read, update, and write - // atomic functions - private volatile SecretKey mCachedSecretKey; + private SecretKey mCachedSecretKey; - // private final String mKeyLock = "keyLock" - // Final equivalent strings over multiple classes are represented by the same String literal - // in memory https://www.javalobby.org//java/forums/t96352.html - private final Object mKeyLock = new Object(); public SharedPrefKeyStorage(SecretKeyWrapper secretKeyWrapper, String prefFileName, String keystoreAlias, String cipherAlgorithm) { mSecretKeyWrapper = secretKeyWrapper; @@ -62,37 +54,24 @@ public SharedPrefKeyStorage(SecretKeyWrapper secretKeyWrapper, String prefFileNa @Override public synchronized SecretKey loadKey(Context context) { Log.v(TAG, "loadKey"); - // "Double-Check" Locking http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html - // if (mCachedSecretKey == null) { - //Only allow one thread at a time load the key. - //synchronized (mKeyLock) { - //If the other thread updated the key, don't re-load it. if (mCachedSecretKey == null) { mCachedSecretKey = loadSecretKey(context, mKeystoreAlias, mCipherAlgorithm); } - //} - //} return mCachedSecretKey; } @Override - public boolean saveKey(Context context, SecretKey secretKey) { + public synchronized boolean saveKey(Context context, SecretKey secretKey) { Log.v(TAG, "saveKey"); - boolean success; - synchronized (mKeyLock) { - success = storeSecretKey(context, mKeystoreAlias, secretKey); - //Clear the cached key upon failure to save. + boolean success = storeSecretKey(context, mKeystoreAlias, secretKey); + //Clear the cached key upon failure to save. + mCachedSecretKey = success ? secretKey : null; - // Inconsistent synchronization only when mCachedSecretKey is not volatile - mCachedSecretKey = success ? secretKey : null; - } return success; } - // Why is clearing the key not synchronous, couldn't other threads be using the cached key? @Override - public void clearKey(Context context) { - // Not accessed synchronously + public synchronized void clearKey(Context context) { mCachedSecretKey = null; storeSecretKey(context, mKeystoreAlias, null); try { From 892f2ca0851e8a85efdd1f53063ba516414bec4e Mon Sep 17 00:00:00 2001 From: Jake Rowland Date: Thu, 12 Jul 2018 14:04:50 -0500 Subject: [PATCH 03/10] Notes for future changes --- .../bottlerocketstudios/vault/SharedPreferenceVault.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/SharedPreferenceVault.java b/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/SharedPreferenceVault.java index c5df204..54ff694 100644 --- a/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/SharedPreferenceVault.java +++ b/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/SharedPreferenceVault.java @@ -65,4 +65,10 @@ public interface SharedPreferenceVault extends SharedPreferences { * Method to find out expected security level of KeyStorage implementation being used. */ KeyStorageType getKeyStorageType(); + + /** + * + */ + //TODO: Implement and define callback listener for error handling through .apply + void setCallbackListener(); } From 7d8b19d8a83c4ead4cbbd4f81d182f15fbaed5c3 Mon Sep 17 00:00:00 2001 From: Jake Rowland Date: Thu, 19 Jul 2018 17:04:57 -0500 Subject: [PATCH 04/10] Added callback support for apply and commit. Callback is optional but adds extra infromation for apply() calls --- .../vault/SharedPreferenceVault.java | 15 ++++++++-- .../vault/StandardSharedPreferenceVault.java | 11 ++++++- .../StandardSharedPreferenceVaultEditor.java | 29 +++++++++++++++++-- 3 files changed, 48 insertions(+), 7 deletions(-) diff --git a/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/SharedPreferenceVault.java b/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/SharedPreferenceVault.java index 54ff694..1f75e64 100644 --- a/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/SharedPreferenceVault.java +++ b/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/SharedPreferenceVault.java @@ -25,6 +25,15 @@ * Shared Preferences backed vault for storing sensitive information. */ public interface SharedPreferenceVault extends SharedPreferences { + + /** + * Interface to handle atypical results from the standard {@link Editor#commit()} and {@link Editor#apply()} + */ + interface SharedPrefVaultWriteListener { + void onSuccess(); + void onError(); + } + /** * Remove all stored values and destroy cryptographic keys associated with the vault instance. * This will permanently destroy all data in the preference file. @@ -67,8 +76,8 @@ public interface SharedPreferenceVault extends SharedPreferences { KeyStorageType getKeyStorageType(); /** - * + * Add a listener to handle atypical behavior when {@link Editor#commit()} or {@link Editor#apply()} is used + * with Vault. */ - //TODO: Implement and define callback listener for error handling through .apply - void setCallbackListener(); + SharedPreferenceVault setSharedPrefVaultWriteListener(SharedPrefVaultWriteListener listener); } diff --git a/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/StandardSharedPreferenceVault.java b/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/StandardSharedPreferenceVault.java index dac85bc..333fe0b 100644 --- a/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/StandardSharedPreferenceVault.java +++ b/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/StandardSharedPreferenceVault.java @@ -58,6 +58,7 @@ public class StandardSharedPreferenceVault implements SharedPreferenceVault { private final List mSharedPreferenceChangeListenerList = Collections.synchronizedList(new LinkedList()); private SharedPreferences mSharedPreferences; + private SharedPrefVaultWriteListener mListener; private boolean mDebugEnabled; public StandardSharedPreferenceVault(Context context, KeyStorage keyStorage, String prefFileName, String transform, boolean enableExceptions) { @@ -66,6 +67,7 @@ public StandardSharedPreferenceVault(Context context, KeyStorage keyStorage, Str mSharedPreferenceName = prefFileName; mTransform = transform; mEnableExceptions = enableExceptions; + mListener = null; } boolean writeValues(boolean commit, boolean wasCleared, Set removalSet, StronglyTypedBundle stronglyTypedBundle) { @@ -291,7 +293,7 @@ public boolean contains(String key) { @Override public Editor edit() { - return new StandardSharedPreferenceVaultEditor(this); + return new StandardSharedPreferenceVaultEditor(this, mListener); } private void notifyListeners(Set preferenceKeySet) { @@ -351,6 +353,13 @@ public KeyStorageType getKeyStorageType() { return mKeyStorage.getKeyStorageType(); } + @Override + public SharedPreferenceVault setSharedPrefVaultWriteListener(SharedPrefVaultWriteListener listener) { + log("Adding a write listener to the SharedPreferenceVault."); + mListener = listener; + return this; + } + private void log(String message) { if (isDebugEnabled()) Log.e(TAG, message); } diff --git a/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/StandardSharedPreferenceVaultEditor.java b/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/StandardSharedPreferenceVaultEditor.java index 491d1c1..0c3f612 100644 --- a/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/StandardSharedPreferenceVaultEditor.java +++ b/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/StandardSharedPreferenceVaultEditor.java @@ -26,12 +26,15 @@ public class StandardSharedPreferenceVaultEditor implements SharedPreferences.Editor { private final StandardSharedPreferenceVault mStandardSharedPreferenceVault; + private final SharedPreferenceVault.SharedPrefVaultWriteListener mListener; private StronglyTypedBundle mStronglyTypedBundle = new StronglyTypedBundle(); private boolean mCleared; private Set mRemovalSet = new HashSet<>(); - public StandardSharedPreferenceVaultEditor(StandardSharedPreferenceVault standardSharedPreferenceVault) { + public StandardSharedPreferenceVaultEditor(StandardSharedPreferenceVault standardSharedPreferenceVault, + SharedPreferenceVault.SharedPrefVaultWriteListener listener) { mStandardSharedPreferenceVault = standardSharedPreferenceVault; + mListener = listener; } @Override @@ -85,11 +88,31 @@ public SharedPreferences.Editor clear() { @Override public boolean commit() { - return mStandardSharedPreferenceVault.writeValues(true, mCleared, mRemovalSet, mStronglyTypedBundle); + boolean success = mStandardSharedPreferenceVault.writeValues( + true, + mCleared, + mRemovalSet, + mStronglyTypedBundle); + if(mListener != null && success) { + mListener.onSuccess(); + } else if(mListener != null && !success) { + mListener.onError(); + } + + return success; } @Override public void apply() { - mStandardSharedPreferenceVault.writeValues(false, mCleared, mRemovalSet, mStronglyTypedBundle); + boolean success = mStandardSharedPreferenceVault.writeValues( + false, + mCleared, + mRemovalSet, + mStronglyTypedBundle); + if(mListener != null && success) { + mListener.onSuccess(); + } else if(mListener != null && !success) { + mListener.onError(); + } } } From 2229df1a758a1c9ffd48afc2005c34ae4421b7d6 Mon Sep 17 00:00:00 2001 From: Jake Rowland Date: Thu, 19 Jul 2018 17:05:42 -0500 Subject: [PATCH 05/10] Adds tests for the callback listener. Tests success, Null SecretKey, Null TypedArray, and Malformed Key --- .../vault/test/TestSharedPrefListener.java | 241 ++++++++++++++++++ 1 file changed, 241 insertions(+) create mode 100644 AndroidVault/vault/src/androidTest/java/com/bottlerocketstudios/vault/test/TestSharedPrefListener.java diff --git a/AndroidVault/vault/src/androidTest/java/com/bottlerocketstudios/vault/test/TestSharedPrefListener.java b/AndroidVault/vault/src/androidTest/java/com/bottlerocketstudios/vault/test/TestSharedPrefListener.java new file mode 100644 index 0000000..70d9a0d --- /dev/null +++ b/AndroidVault/vault/src/androidTest/java/com/bottlerocketstudios/vault/test/TestSharedPrefListener.java @@ -0,0 +1,241 @@ +/* + * Copyright (c) 2016. Bottle Rocket LLC + * 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 com.bottlerocketstudios.vault.test; + +import android.content.SharedPreferences; +import android.test.AndroidTestCase; +import android.util.Log; + +import com.bottlerocketstudios.vault.SharedPreferenceVault; +import com.bottlerocketstudios.vault.SharedPreferenceVaultFactory; +import com.bottlerocketstudios.vault.StandardSharedPreferenceVault; +import com.bottlerocketstudios.vault.keys.generator.Aes256RandomKeyFactory; + +import java.security.GeneralSecurityException; + +import javax.crypto.SecretKey; + +/* + * Test to see if the listener is called correctly when it should be. + */ +public class TestSharedPrefListener extends AndroidTestCase { + private static final String TAG = TestSharedPrefListener.class.getSimpleName(); + + private static final String KEY_FILE_NAME = "listenerKeyTest"; + private static final String PREF_FILE_NAME = "listenerPrefTest"; + private static final String KEY_ALIAS_1 = "listenerKeyAlias"; + private static final int KEY_INDEX_1 = 1; + private static final String PRESHARED_SECRET_1 = "a;sdl564546a6s6w2828d4fsdfbsijd;saj;9dj9"; + + private static final String TEST_STRING_KEY = "testKey"; + private static final String TEST_STRING_VALUE = " This is a test. "; + + private SharedPreferenceVault mSharedPreferenceVault; + + // Initialization function. Should be called by all tests but only initialized once. On previous + // invocations, the storage is cleared and that is it + private boolean shouldInit = true; + private void init() { + Log.i(TAG, "Initializing"); + if(shouldInit) { + mSharedPreferenceVault = null; + try { + Log.i(TAG, "Creating SharedPreference vault."); + mSharedPreferenceVault = SharedPreferenceVaultFactory.getAppKeyedCompatAes256Vault( + getContext(), + PREF_FILE_NAME, + KEY_FILE_NAME, + KEY_ALIAS_1, + KEY_INDEX_1, + PRESHARED_SECRET_1); + } catch (GeneralSecurityException e) { + Log.e(TAG, "Caught java.security.GeneralSecurityException", e); + assertTrue("Exception creating vault", false); + } + + assertNotNull("Unable to create initial vault", mSharedPreferenceVault); + assertTrue("Testing StandardSharedPreferenceVault", + mSharedPreferenceVault instanceof StandardSharedPreferenceVault); + + shouldInit = false; + } + cleanStorage(); + } + + private void cleanStorage() { + mSharedPreferenceVault.rekeyStorage(Aes256RandomKeyFactory.createKey()); + assertNull("Rekey of storage did not clear existing value", + mSharedPreferenceVault.getString(TEST_STRING_KEY, null)); + } + + public void testSuccessCommit() { + init(); + SharedPreferences.Editor editor = mSharedPreferenceVault.setSharedPrefVaultWriteListener( + new SharedPrefVaultWriteListenerExtension(new OnComplete() { + @Override + public void onComplete(boolean success) { + assertTrue("Error when committing to sharedPrefs", success); + } + }) + ).edit(); + + editor.putString(TEST_STRING_KEY, TEST_STRING_VALUE).commit(); + } + + public void testSuccessApply() { + init(); + SharedPreferences.Editor editor = mSharedPreferenceVault.setSharedPrefVaultWriteListener( + new SharedPrefVaultWriteListenerExtension(new OnComplete() { + @Override + public void onComplete(boolean success) { + assertTrue("Error when applying to sharedPrefs", success); + } + }) + ).edit(); + + editor.putString(TEST_STRING_KEY, TEST_STRING_VALUE).apply(); + } + + public void testNullTypedBundleFailureCommit() { + init(); + mSharedPreferenceVault.clearStorage(); + SharedPreferences.Editor editor = mSharedPreferenceVault.setSharedPrefVaultWriteListener( + new SharedPrefVaultWriteListenerExtension(new OnComplete() { + @Override + public void onComplete(boolean success) { + assertFalse("Data was added to the SharedPref", success); + } + }) + ).edit(); + + editor.commit(); + } + + public void testNullTypedBundleFailureApply() { + init(); + mSharedPreferenceVault.clearStorage(); + SharedPreferences.Editor editor = mSharedPreferenceVault.setSharedPrefVaultWriteListener( + new SharedPrefVaultWriteListenerExtension(new OnComplete() { + @Override + public void onComplete(boolean success) { + assertFalse("Data was Added to the SharedPref", success); + } + }) + ).edit(); + + editor.putString(TEST_STRING_KEY, TEST_STRING_VALUE).apply(); + } + + public void testNullKeyFailureCommit() { + init(); + mSharedPreferenceVault.clearStorage(); + SharedPreferences.Editor editor = mSharedPreferenceVault.setSharedPrefVaultWriteListener( + new SharedPrefVaultWriteListenerExtension(new OnComplete() { + @Override + public void onComplete(boolean success) { + assertFalse("The key is still initialized", success); + } + }) + ).edit(); + + editor.putString(TEST_STRING_KEY, TEST_STRING_VALUE).commit(); + } + + public void testNullKeyFailureApply() { + init(); + mSharedPreferenceVault.clearStorage(); + SharedPreferences.Editor editor = mSharedPreferenceVault.setSharedPrefVaultWriteListener( + new SharedPrefVaultWriteListenerExtension(new OnComplete() { + @Override + public void onComplete(boolean success) { + assertFalse("The key is still initialized", success); + } + }) + ).edit(); + + editor.putString(TEST_STRING_KEY, TEST_STRING_VALUE).apply(); + } + + private final SecretKey malformedSecretKey = new SecretKey() { + private final String mKey = "ThisIsMySuperSecretKey"; + + @Override + public String getAlgorithm() { + return "BestAlgorithEver"; + } + + @Override + public String getFormat() { + return mKey; + } + + @Override + public byte[] getEncoded() { + return mKey.getBytes(); + } + }; + public void testMalformedKeyFailureCommit() { + init(); + mSharedPreferenceVault.rekeyStorage(malformedSecretKey); + SharedPreferences.Editor editor = mSharedPreferenceVault.setSharedPrefVaultWriteListener( + new SharedPrefVaultWriteListenerExtension(new OnComplete() { + @Override + public void onComplete(boolean success) { + assertFalse("Malformed key still works", success); + } + }) + ).edit(); + + editor.putString(TEST_STRING_KEY, TEST_STRING_VALUE).commit(); + } + + public void testMalformedKeyFailureApply() { + init(); + mSharedPreferenceVault.rekeyStorage(malformedSecretKey); + SharedPreferences.Editor editor = mSharedPreferenceVault.setSharedPrefVaultWriteListener( + new SharedPrefVaultWriteListenerExtension(new OnComplete() { + @Override + public void onComplete(boolean success) { + assertFalse("Malformed key still works", success); + } + }) + ).edit(); + + editor.putString(TEST_STRING_KEY, TEST_STRING_VALUE).apply(); + } + + public interface OnComplete { + void onComplete(boolean success); + } + + private class SharedPrefVaultWriteListenerExtension implements SharedPreferenceVault.SharedPrefVaultWriteListener { + private final OnComplete mListener; + + public SharedPrefVaultWriteListenerExtension(OnComplete listener) { + mListener = listener; + } + + @Override + public void onSuccess() { + mListener.onComplete(true); + } + + @Override + public void onError() { + mListener.onComplete(false); + } + } +} From 0db427667bcb58a49c6f8fbb611bb41df5fae557 Mon Sep 17 00:00:00 2001 From: Jake Rowland Date: Fri, 20 Jul 2018 11:53:42 -0500 Subject: [PATCH 06/10] Updated the readme with changes and the the listener, fixed one spelling mistake --- README.md | 45 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 42 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 62e47e6..66eefe3 100644 --- a/README.md +++ b/README.md @@ -35,6 +35,7 @@ These components can be used independently of each other, but will be convenient * SharedPreferenceVaultRegistry - A centralized place to keep your SharedPreferenceVault instances. Guarantees the required uniqueness of values used to index the stored values. * StandardSharedPreferenceVault - Implementation used by the factory that is backed by an actual SharedPreference file. * StandardSharedPreferenceVaultEditor - Implements SharedPreference.Editor and provides the same behavior. + * SharedPrefVaultWriteListener - Improves the functionality of the standard commit and apply associated with SharedPreference * KeyStorage - Secure method to store your SecretKey objects. * CompatSharedPrefKeyStorageFactory - Self-upgrading SharedPreferences backed KeyStorage factory. * SharedPrefKeyStorage - Implementation used by the factory. @@ -158,9 +159,47 @@ You can use the SharedPreferenceVault with SecretKey generated entirely from the SharedPreferenceVaultRegistry.getInstance().getVault(VAULT_ID).rekeyStorage(secretKey); #### Rekeying -The vault can be rekeyed at any time. This will delete all values in the shared -preference file. This is completely irreversible. +The vault can be rekeyed at any time. This will delete all values in the shared preference file. This is completely irreversible. + +#### API Changes to Commit and Apply +Typical results of the SharedPreferenc commit and apply are slightly modified in Vault resulting of the extra encryption that is taking place. Because of this, commit and apply can both fail for reasons other than the SharedPreference write failing. The return type for commit is now more important. The recommendation is to **enable exceptions** when creating a SharedPreferenceVault. + +* `SharedPreferenceVaultFactory.getCompatAes256Vault()` +* `SharedPreferenceVaultFactory.getAppKeyedCompatAes256Vault()` +* `SharedPreferenceVaultFactory.getMemoryOnlyKeyAes256Vault()` + +These all have the ability to throw `GeneralSecurityException` or `UnsupportedEncodingException` with **exceptions enabled**. These exceptions can occur before the commit or apply is actually attempted and can result in a failure to write data to the SharedPreference file. + +1. If using `commit()` consider **enabling exceptions** to allow handling of these errors. Also, pay attention to the return type from `commit()` to make sure data was successfully written. +2. If using `apply()`, there is a new `SharedPrefVaultWriteListener` interface that can be used to handle callback should any of the errors mentioned above. The `SharedPreferenceVault` interface now supports a new method to add this Listener to the Vault. `setSharedPrefVaultWriteListener(SharedPrefVaultWriteListener listener)` This method can be chained but must be called **before** an editor is created. + * The actual result of the write is still ignored when using the new `apply()` with a listener. The listener is only to show potential encryption issues that happen *before* the write. + * Setting no listener will behave like the standard `SharedPreference.apply()`. All error are ignored. + * The listener will only provide basic information on success and fail, it is still recommended that if more information is required, to **enable exceptions**. + +Adding a listener is easy. Simply add an implementation of the `SharedPrefVaultWriteListener` to the `SharedPreferenceVault` **before** and editor is created from the Vault. + +```java +//Adding a listener to a SharedPreferenceVault Instance +SharedPreferenceVault secureVault = SharedPreferenceVaultFactory.getAppKeyedCompatAes256Vault( + context, + PREF_FILE_NAME, + KEY_FILE_NAME, + KEY_ALIAS, + VAULT_ID, + PRESHARED_SECRET + ).setSharedPrefVaultWriteListener(new SharedPrefVaultWriteListener() { + @Override + public void onSuccess() { + //On Success code + } + + @Override + public void onError() { + //On Error code + } + }); +``` ### Auditor Notes Automated testing tools are often built to trigger on potential cryptographic mishaps. That is fine and sunlight is often the best disinfectant, especially in crypto. That is part of why this is an open source library. However, this library will cause two irrelevant reports to occur. @@ -171,6 +210,6 @@ Automated testing tools are often built to trigger on potential cryptographic mi This project must be built with gradle. * Version Numbering - The version name should end with "-SNAPSHOT" for non release builds. This will cause the resulting binary, source, and javadoc files to be uploaded to the snapshot repository in Maven as a snapshot build. Removing snapshot from the version name will publish the build on jcenter. If that version is already published, it will not overwrite it. -* Execution - To build this libarary, associated tasks are dynamically generated by Android build tools in conjunction with Gradle. Example command for the production flavor of the release build type: +* Execution - To build this library, associated tasks are dynamically generated by Android build tools in conjunction with Gradle. Example command for the production flavor of the release build type: * Build and upload: `./gradlew --refresh-dependencies clean uploadToMaven` * Build only: `./gradlew --refresh-dependencies clean jarRelease` From ff6e4aa17592baf8a5cef41101d015770bced5c7 Mon Sep 17 00:00:00 2001 From: Jake Rowland Date: Mon, 23 Jul 2018 14:01:13 -0500 Subject: [PATCH 07/10] Removed unused import --- .../vault/keys/storage/KeychainAuthenticatedKeyStorage.java | 1 - 1 file changed, 1 deletion(-) diff --git a/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/keys/storage/KeychainAuthenticatedKeyStorage.java b/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/keys/storage/KeychainAuthenticatedKeyStorage.java index f70b7dc..f206310 100644 --- a/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/keys/storage/KeychainAuthenticatedKeyStorage.java +++ b/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/keys/storage/KeychainAuthenticatedKeyStorage.java @@ -17,7 +17,6 @@ import android.annotation.TargetApi; import android.content.Context; -import android.nfc.Tag; import android.os.Build; import android.security.keystore.KeyGenParameterSpec; import android.security.keystore.KeyProperties; From 6488f1fa681e3e4a8052da8f1d0ab886a5d73241 Mon Sep 17 00:00:00 2001 From: Jake Rowland Date: Mon, 23 Jul 2018 14:06:24 -0500 Subject: [PATCH 08/10] Removes unused lock --- .../vault/keys/storage/KeychainAuthenticatedKeyStorage.java | 5 ----- 1 file changed, 5 deletions(-) diff --git a/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/keys/storage/KeychainAuthenticatedKeyStorage.java b/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/keys/storage/KeychainAuthenticatedKeyStorage.java index f206310..633af2c 100644 --- a/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/keys/storage/KeychainAuthenticatedKeyStorage.java +++ b/AndroidVault/vault/src/main/java/com/bottlerocketstudios/vault/keys/storage/KeychainAuthenticatedKeyStorage.java @@ -49,11 +49,6 @@ public class KeychainAuthenticatedKeyStorage implements KeyStorage { private final String mPadding; private final int mAuthDurationSeconds; - // private final String mKeyLock = "keyLock" - // Final equivalent strings over multiple classes are represented by the same String literal - // in memory https://www.javalobby.org//java/forums/t96352.html - private final Object mKeyLock = new Object(); - public KeychainAuthenticatedKeyStorage(String keyAlias, String algorithm, String blockMode, String padding, int authDurationSeconds) { mKeyAlias = keyAlias; mAlgorithm = algorithm; From 21fcf528c02e82e5209ee23d495e9c7569997e1d Mon Sep 17 00:00:00 2001 From: Jake Rowland Date: Fri, 27 Jul 2018 13:45:47 -0500 Subject: [PATCH 09/10] Suggested changes from the PR and added two tests, NullSecretKey and NullSharedPrefKey --- .../vault/test/TestSharedPrefListener.java | 16 +++++++--------- README.md | 6 +++--- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/AndroidVault/vault/src/androidTest/java/com/bottlerocketstudios/vault/test/TestSharedPrefListener.java b/AndroidVault/vault/src/androidTest/java/com/bottlerocketstudios/vault/test/TestSharedPrefListener.java index 70d9a0d..955a9e1 100644 --- a/AndroidVault/vault/src/androidTest/java/com/bottlerocketstudios/vault/test/TestSharedPrefListener.java +++ b/AndroidVault/vault/src/androidTest/java/com/bottlerocketstudios/vault/test/TestSharedPrefListener.java @@ -109,29 +109,29 @@ public void onComplete(boolean success) { editor.putString(TEST_STRING_KEY, TEST_STRING_VALUE).apply(); } - public void testNullTypedBundleFailureCommit() { + public void testNullSecretKeyFailureCommit() { init(); mSharedPreferenceVault.clearStorage(); SharedPreferences.Editor editor = mSharedPreferenceVault.setSharedPrefVaultWriteListener( new SharedPrefVaultWriteListenerExtension(new OnComplete() { @Override public void onComplete(boolean success) { - assertFalse("Data was added to the SharedPref", success); + assertFalse("The key is still initialized", success); } }) ).edit(); - editor.commit(); + editor.putString(TEST_STRING_KEY, TEST_STRING_VALUE).commit(); } - public void testNullTypedBundleFailureApply() { + public void testNullSecretKeyFailureApply() { init(); mSharedPreferenceVault.clearStorage(); SharedPreferences.Editor editor = mSharedPreferenceVault.setSharedPrefVaultWriteListener( new SharedPrefVaultWriteListenerExtension(new OnComplete() { @Override public void onComplete(boolean success) { - assertFalse("Data was Added to the SharedPref", success); + assertFalse("The key is still initialized", success); } }) ).edit(); @@ -141,7 +141,6 @@ public void onComplete(boolean success) { public void testNullKeyFailureCommit() { init(); - mSharedPreferenceVault.clearStorage(); SharedPreferences.Editor editor = mSharedPreferenceVault.setSharedPrefVaultWriteListener( new SharedPrefVaultWriteListenerExtension(new OnComplete() { @Override @@ -151,12 +150,11 @@ public void onComplete(boolean success) { }) ).edit(); - editor.putString(TEST_STRING_KEY, TEST_STRING_VALUE).commit(); + editor.putString(null, TEST_STRING_VALUE).commit(); } public void testNullKeyFailureApply() { init(); - mSharedPreferenceVault.clearStorage(); SharedPreferences.Editor editor = mSharedPreferenceVault.setSharedPrefVaultWriteListener( new SharedPrefVaultWriteListenerExtension(new OnComplete() { @Override @@ -166,7 +164,7 @@ public void onComplete(boolean success) { }) ).edit(); - editor.putString(TEST_STRING_KEY, TEST_STRING_VALUE).apply(); + editor.putString(null, TEST_STRING_VALUE).apply(); } private final SecretKey malformedSecretKey = new SecretKey() { diff --git a/README.md b/README.md index 66eefe3..e4c9b7a 100644 --- a/README.md +++ b/README.md @@ -162,7 +162,7 @@ You can use the SharedPreferenceVault with SecretKey generated entirely from the The vault can be rekeyed at any time. This will delete all values in the shared preference file. This is completely irreversible. #### API Changes to Commit and Apply -Typical results of the SharedPreferenc commit and apply are slightly modified in Vault resulting of the extra encryption that is taking place. Because of this, commit and apply can both fail for reasons other than the SharedPreference write failing. The return type for commit is now more important. The recommendation is to **enable exceptions** when creating a SharedPreferenceVault. +Typical behavior of the SharedPreference commit and apply methods are slightly modified in Vault as a result of the added encryption. Because of this, commit and apply can both fail for reasons other than the SharedPreference write failing. This means that you should pay attention to the return value for commit and react appropriately in the case where it returns false. It is recommended to **enable exceptions** when creating a SharedPreferenceVault if you want to see more information on failures (by catching the thrown exception yourself) * `SharedPreferenceVaultFactory.getCompatAes256Vault()` * `SharedPreferenceVaultFactory.getAppKeyedCompatAes256Vault()` @@ -173,8 +173,8 @@ These all have the ability to throw `GeneralSecurityException` or `UnsupportedEn 1. If using `commit()` consider **enabling exceptions** to allow handling of these errors. Also, pay attention to the return type from `commit()` to make sure data was successfully written. 2. If using `apply()`, there is a new `SharedPrefVaultWriteListener` interface that can be used to handle callback should any of the errors mentioned above. The `SharedPreferenceVault` interface now supports a new method to add this Listener to the Vault. `setSharedPrefVaultWriteListener(SharedPrefVaultWriteListener listener)` This method can be chained but must be called **before** an editor is created. * The actual result of the write is still ignored when using the new `apply()` with a listener. The listener is only to show potential encryption issues that happen *before* the write. - * Setting no listener will behave like the standard `SharedPreference.apply()`. All error are ignored. - * The listener will only provide basic information on success and fail, it is still recommended that if more information is required, to **enable exceptions**. + * Setting no listener will behave like the standard `SharedPreference.apply()`. All errors are ignored. + * The listener will only provide basic information on success and fail. It is recommended to **enable exceptions** if more information on the failure is required. Adding a listener is easy. Simply add an implementation of the `SharedPrefVaultWriteListener` to the `SharedPreferenceVault` **before** and editor is created from the Vault. From dfacc40503c806d5d836e012ab3945152b6f8ac0 Mon Sep 17 00:00:00 2001 From: Jake Rowland Date: Fri, 27 Jul 2018 16:36:54 -0500 Subject: [PATCH 10/10] Removes the nullKey test --- .../vault/test/TestSharedPrefListener.java | 28 ------------------- 1 file changed, 28 deletions(-) diff --git a/AndroidVault/vault/src/androidTest/java/com/bottlerocketstudios/vault/test/TestSharedPrefListener.java b/AndroidVault/vault/src/androidTest/java/com/bottlerocketstudios/vault/test/TestSharedPrefListener.java index 955a9e1..afc0529 100644 --- a/AndroidVault/vault/src/androidTest/java/com/bottlerocketstudios/vault/test/TestSharedPrefListener.java +++ b/AndroidVault/vault/src/androidTest/java/com/bottlerocketstudios/vault/test/TestSharedPrefListener.java @@ -139,34 +139,6 @@ public void onComplete(boolean success) { editor.putString(TEST_STRING_KEY, TEST_STRING_VALUE).apply(); } - public void testNullKeyFailureCommit() { - init(); - SharedPreferences.Editor editor = mSharedPreferenceVault.setSharedPrefVaultWriteListener( - new SharedPrefVaultWriteListenerExtension(new OnComplete() { - @Override - public void onComplete(boolean success) { - assertFalse("The key is still initialized", success); - } - }) - ).edit(); - - editor.putString(null, TEST_STRING_VALUE).commit(); - } - - public void testNullKeyFailureApply() { - init(); - SharedPreferences.Editor editor = mSharedPreferenceVault.setSharedPrefVaultWriteListener( - new SharedPrefVaultWriteListenerExtension(new OnComplete() { - @Override - public void onComplete(boolean success) { - assertFalse("The key is still initialized", success); - } - }) - ).edit(); - - editor.putString(null, TEST_STRING_VALUE).apply(); - } - private final SecretKey malformedSecretKey = new SecretKey() { private final String mKey = "ThisIsMySuperSecretKey";