From 2cd2f732416ae05a22b0da4d299fdc85f567ada7 Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Mon, 15 Feb 2021 12:16:07 -0300 Subject: [PATCH 1/3] Use a TestApp for UnitTests so we can use different modules for injection --- .../java/com/stevesoltys/seedvault/App.kt | 36 ++++++++------- .../java/com/stevesoltys/seedvault/TestApp.kt | 45 +++++++++++++++++++ .../seedvault/metadata/MetadataManagerTest.kt | 6 ++- .../seedvault/plugins/saf/DocumentFileTest.kt | 6 ++- .../restore/install/DeviceInfoTest.kt | 6 ++- 5 files changed, 79 insertions(+), 20 deletions(-) create mode 100644 app/src/test/java/com/stevesoltys/seedvault/TestApp.kt diff --git a/app/src/main/java/com/stevesoltys/seedvault/App.kt b/app/src/main/java/com/stevesoltys/seedvault/App.kt index 295f906a..a99e08a6 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/App.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/App.kt @@ -34,7 +34,7 @@ import org.koin.dsl.module * @author Steve Soltys * @author Torsten Grote */ -class App : Application() { +open class App : Application() { private val appModule = module { single { SettingsManager(this@App) } @@ -52,22 +52,7 @@ class App : Application() { override fun onCreate() { super.onCreate() - startKoin { - androidLogger() - androidContext(this@App) - modules( - listOf( - cryptoModule, - headerModule, - metadataModule, - documentsProviderModule, // storage plugin - backupModule, - restoreModule, - installModule, - appModule - ) - ) - } + startKoin() if (isDebugBuild()) { StrictMode.setThreadPolicy( StrictMode.ThreadPolicy.Builder() @@ -88,6 +73,23 @@ class App : Application() { } } + protected open fun startKoin() = startKoin { + androidLogger() + androidContext(this@App) + modules( + listOf( + cryptoModule, + headerModule, + metadataModule, + documentsProviderModule, // storage plugin + backupModule, + restoreModule, + installModule, + appModule + ) + ) + } + private val settingsManager: SettingsManager by inject() private val metadataManager: MetadataManager by inject() diff --git a/app/src/test/java/com/stevesoltys/seedvault/TestApp.kt b/app/src/test/java/com/stevesoltys/seedvault/TestApp.kt new file mode 100644 index 00000000..b1ad45a5 --- /dev/null +++ b/app/src/test/java/com/stevesoltys/seedvault/TestApp.kt @@ -0,0 +1,45 @@ +package com.stevesoltys.seedvault + +import com.stevesoltys.seedvault.crypto.CipherFactory +import com.stevesoltys.seedvault.crypto.CipherFactoryImpl +import com.stevesoltys.seedvault.crypto.Crypto +import com.stevesoltys.seedvault.crypto.CryptoImpl +import com.stevesoltys.seedvault.crypto.KeyManager +import com.stevesoltys.seedvault.crypto.KeyManagerTestImpl +import com.stevesoltys.seedvault.header.headerModule +import com.stevesoltys.seedvault.metadata.metadataModule +import com.stevesoltys.seedvault.plugins.saf.documentsProviderModule +import com.stevesoltys.seedvault.restore.install.installModule +import com.stevesoltys.seedvault.transport.backup.backupModule +import com.stevesoltys.seedvault.transport.restore.restoreModule +import org.koin.android.ext.koin.androidContext +import org.koin.core.context.startKoin +import org.koin.dsl.module + +class TestApp : App() { + + private val testCryptoModule = module { + factory { CipherFactoryImpl(get()) } + single { KeyManagerTestImpl() } + single { CryptoImpl(get(), get(), get()) } + } + private val appModule = module { + single { Clock() } + } + + override fun startKoin() = startKoin { + androidContext(this@TestApp) + modules( + listOf( + testCryptoModule, + headerModule, + metadataModule, + documentsProviderModule, // storage plugin + backupModule, + restoreModule, + installModule, + appModule + ) + ) + } +} diff --git a/app/src/test/java/com/stevesoltys/seedvault/metadata/MetadataManagerTest.kt b/app/src/test/java/com/stevesoltys/seedvault/metadata/MetadataManagerTest.kt index 6b356016..0aae4113 100644 --- a/app/src/test/java/com/stevesoltys/seedvault/metadata/MetadataManagerTest.kt +++ b/app/src/test/java/com/stevesoltys/seedvault/metadata/MetadataManagerTest.kt @@ -8,6 +8,7 @@ import android.content.pm.ApplicationInfo.FLAG_SYSTEM import android.content.pm.PackageInfo import androidx.test.ext.junit.runners.AndroidJUnit4 import com.stevesoltys.seedvault.Clock +import com.stevesoltys.seedvault.TestApp import com.stevesoltys.seedvault.getRandomByteArray import com.stevesoltys.seedvault.getRandomString import com.stevesoltys.seedvault.metadata.PackageState.APK_AND_DATA @@ -37,7 +38,10 @@ import kotlin.random.Random @Suppress("DEPRECATION") @RunWith(AndroidJUnit4::class) -@Config(sdk = [29]) // robolectric does not support 30, yet +@Config( + sdk = [29], // robolectric does not support 30, yet + application = TestApp::class +) class MetadataManagerTest { private val context: Context = mockk() diff --git a/app/src/test/java/com/stevesoltys/seedvault/plugins/saf/DocumentFileTest.kt b/app/src/test/java/com/stevesoltys/seedvault/plugins/saf/DocumentFileTest.kt index 83ed6e29..49a182e4 100644 --- a/app/src/test/java/com/stevesoltys/seedvault/plugins/saf/DocumentFileTest.kt +++ b/app/src/test/java/com/stevesoltys/seedvault/plugins/saf/DocumentFileTest.kt @@ -5,6 +5,7 @@ import android.net.Uri import android.provider.DocumentsContract import androidx.documentfile.provider.DocumentFile import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.stevesoltys.seedvault.TestApp import io.mockk.mockk import org.junit.After import org.junit.Assert.assertEquals @@ -15,7 +16,10 @@ import org.koin.core.context.stopKoin import org.robolectric.annotation.Config @RunWith(AndroidJUnit4::class) -@Config(sdk = [29]) // robolectric does not support 30, yet +@Config( + sdk = [29], // robolectric does not support 30, yet + application = TestApp::class +) internal class DocumentFileTest { private val context: Context = mockk() diff --git a/app/src/test/java/com/stevesoltys/seedvault/restore/install/DeviceInfoTest.kt b/app/src/test/java/com/stevesoltys/seedvault/restore/install/DeviceInfoTest.kt index 4afe4ec7..414f5c1d 100644 --- a/app/src/test/java/com/stevesoltys/seedvault/restore/install/DeviceInfoTest.kt +++ b/app/src/test/java/com/stevesoltys/seedvault/restore/install/DeviceInfoTest.kt @@ -6,6 +6,7 @@ import android.util.DisplayMetrics import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 import com.stevesoltys.seedvault.R +import com.stevesoltys.seedvault.TestApp import com.stevesoltys.seedvault.getRandomString import io.mockk.every import io.mockk.mockk @@ -20,7 +21,10 @@ import org.robolectric.annotation.Config import kotlin.random.Random @RunWith(AndroidJUnit4::class) -@Config(sdk = [29]) // robolectric does not support 30, yet +@Config( + sdk = [29], // robolectric does not support 30, yet + application = TestApp::class +) internal class DeviceInfoTest { @After From 851407037e2c9690d6a16272d5691fff79fd4cd1 Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Mon, 15 Feb 2021 10:35:16 -0300 Subject: [PATCH 2/3] Store main key for key derivations from 512-bit BIP39 recovery code This main key will be used later to derive sub-keys for other crypto operations. --- .../seedvault/crypto/CryptoModule.kt | 12 ++- .../seedvault/crypto/KeyManager.kt | 76 +++++++++++++++---- .../ui/recoverycode/RecoveryCodeViewModel.kt | 1 + .../seedvault/crypto/KeyManagerTestImpl.kt | 12 +++ .../seedvault/crypto/KeyManagerImplTest.kt | 65 ++++++++++++++++ 5 files changed, 149 insertions(+), 17 deletions(-) create mode 100644 app/src/test/java/com/stevesoltys/seedvault/crypto/KeyManagerImplTest.kt diff --git a/app/src/main/java/com/stevesoltys/seedvault/crypto/CryptoModule.kt b/app/src/main/java/com/stevesoltys/seedvault/crypto/CryptoModule.kt index 7beb019c..d15c9606 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/crypto/CryptoModule.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/crypto/CryptoModule.kt @@ -1,9 +1,19 @@ package com.stevesoltys.seedvault.crypto import org.koin.dsl.module +import java.security.KeyStore + +private const val ANDROID_KEY_STORE = "AndroidKeyStore" val cryptoModule = module { factory { CipherFactoryImpl(get()) } - single { KeyManagerImpl() } + single { + val keyStore by lazy { + KeyStore.getInstance(ANDROID_KEY_STORE).apply { + load(null) + } + } + KeyManagerImpl(keyStore) + } single { CryptoImpl(get(), get(), get()) } } diff --git a/app/src/main/java/com/stevesoltys/seedvault/crypto/KeyManager.kt b/app/src/main/java/com/stevesoltys/seedvault/crypto/KeyManager.kt index eedbdeee..35ffaada 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/crypto/KeyManager.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/crypto/KeyManager.kt @@ -4,6 +4,8 @@ import android.security.keystore.KeyProperties.BLOCK_MODE_GCM import android.security.keystore.KeyProperties.ENCRYPTION_PADDING_NONE import android.security.keystore.KeyProperties.PURPOSE_DECRYPT import android.security.keystore.KeyProperties.PURPOSE_ENCRYPT +import android.security.keystore.KeyProperties.PURPOSE_SIGN +import android.security.keystore.KeyProperties.PURPOSE_VERIFY import android.security.keystore.KeyProtection import java.security.KeyStore import java.security.KeyStore.SecretKeyEntry @@ -12,8 +14,10 @@ import javax.crypto.spec.SecretKeySpec internal const val KEY_SIZE = 256 internal const val KEY_SIZE_BYTES = KEY_SIZE / 8 -private const val KEY_ALIAS = "com.stevesoltys.seedvault" -private const val ANDROID_KEY_STORE = "AndroidKeyStore" +private const val KEY_ALIAS_BACKUP = "com.stevesoltys.seedvault" +private const val KEY_ALIAS_MAIN = "com.stevesoltys.seedvault.main" +private const val KEY_ALGORITHM_BACKUP = "AES" +private const val KEY_ALGORITHM_MAIN = "HmacSHA256" interface KeyManager { /** @@ -23,11 +27,24 @@ interface KeyManager { */ fun storeBackupKey(seed: ByteArray) + /** + * Store a new main key derived from the given [seed]. + * + * The seed needs to be larger or equal to two times [KEY_SIZE_BYTES] + * and is usually the same as for [storeBackupKey]. + */ + fun storeMainKey(seed: ByteArray) + /** * @return true if a backup key already exists in the [KeyStore]. */ fun hasBackupKey(): Boolean + /** + * @return true if a main key already exists in the [KeyStore]. + */ + fun hasMainKey(): Boolean + /** * Returns the backup key, so it can be used for encryption or decryption. * @@ -35,32 +52,49 @@ interface KeyManager { * because the key can not leave the [KeyStore]'s hardware security module. */ fun getBackupKey(): SecretKey + + /** + * Returns the main key, so it can be used for deriving sub-keys. + * + * Note that any attempt to export the key will return null or an empty [ByteArray], + * because the key can not leave the [KeyStore]'s hardware security module. + */ + fun getMainKey(): SecretKey } -internal class KeyManagerImpl : KeyManager { - - private val keyStore by lazy { - KeyStore.getInstance(ANDROID_KEY_STORE).apply { - load(null) - } - } +internal class KeyManagerImpl( + private val keyStore: KeyStore +) : KeyManager { override fun storeBackupKey(seed: ByteArray) { if (seed.size < KEY_SIZE_BYTES) throw IllegalArgumentException() - val secretKeySpec = SecretKeySpec(seed, 0, KEY_SIZE_BYTES, "AES") - val ksEntry = SecretKeyEntry(secretKeySpec) - keyStore.setEntry(KEY_ALIAS, ksEntry, getKeyProtection()) + val backupKeyEntry = + SecretKeyEntry(SecretKeySpec(seed, 0, KEY_SIZE_BYTES, KEY_ALGORITHM_BACKUP)) + keyStore.setEntry(KEY_ALIAS_BACKUP, backupKeyEntry, getBackupKeyProtection()) } - override fun hasBackupKey() = keyStore.containsAlias(KEY_ALIAS) && - keyStore.entryInstanceOf(KEY_ALIAS, SecretKeyEntry::class.java) + override fun storeMainKey(seed: ByteArray) { + if (seed.size < KEY_SIZE_BYTES * 2) throw IllegalArgumentException() + val mainKeyEntry = + SecretKeyEntry(SecretKeySpec(seed, KEY_SIZE_BYTES, KEY_SIZE_BYTES, KEY_ALGORITHM_MAIN)) + keyStore.setEntry(KEY_ALIAS_MAIN, mainKeyEntry, getMainKeyProtection()) + } + + override fun hasBackupKey() = keyStore.containsAlias(KEY_ALIAS_BACKUP) + + override fun hasMainKey() = keyStore.containsAlias(KEY_ALIAS_MAIN) override fun getBackupKey(): SecretKey { - val ksEntry = keyStore.getEntry(KEY_ALIAS, null) as SecretKeyEntry + val ksEntry = keyStore.getEntry(KEY_ALIAS_BACKUP, null) as SecretKeyEntry return ksEntry.secretKey } - private fun getKeyProtection(): KeyProtection { + override fun getMainKey(): SecretKey { + val ksEntry = keyStore.getEntry(KEY_ALIAS_MAIN, null) as SecretKeyEntry + return ksEntry.secretKey + } + + private fun getBackupKeyProtection(): KeyProtection { val builder = KeyProtection.Builder(PURPOSE_ENCRYPT or PURPOSE_DECRYPT) .setBlockModes(BLOCK_MODE_GCM) .setEncryptionPaddings(ENCRYPTION_PADDING_NONE) @@ -70,4 +104,14 @@ internal class KeyManagerImpl : KeyManager { return builder.build() } + private fun getMainKeyProtection(): KeyProtection { + // let's not lock down the main key too much, because we have no second chance + // and don't want to repeat the issue with the locked down backup key + val builder = KeyProtection.Builder( + PURPOSE_ENCRYPT or PURPOSE_DECRYPT or + PURPOSE_SIGN or PURPOSE_VERIFY + ) + return builder.build() + } + } diff --git a/app/src/main/java/com/stevesoltys/seedvault/ui/recoverycode/RecoveryCodeViewModel.kt b/app/src/main/java/com/stevesoltys/seedvault/ui/recoverycode/RecoveryCodeViewModel.kt index 74f9d757..65e2a62a 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/ui/recoverycode/RecoveryCodeViewModel.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/ui/recoverycode/RecoveryCodeViewModel.kt @@ -70,6 +70,7 @@ class RecoveryCodeViewModel( val seed = SeedCalculator(JavaxPBKDF2WithHmacSHA512.INSTANCE).calculateSeed(mnemonic, "") if (forVerifyingNewCode) { keyManager.storeBackupKey(seed) + keyManager.storeMainKey(seed) mRecoveryCodeSaved.setEvent(true) } else { mExistingCodeChecked.setEvent(crypto.verifyBackupKey(seed)) diff --git a/app/src/sharedTest/java/com/stevesoltys/seedvault/crypto/KeyManagerTestImpl.kt b/app/src/sharedTest/java/com/stevesoltys/seedvault/crypto/KeyManagerTestImpl.kt index bce7f8a4..372f023b 100644 --- a/app/src/sharedTest/java/com/stevesoltys/seedvault/crypto/KeyManagerTestImpl.kt +++ b/app/src/sharedTest/java/com/stevesoltys/seedvault/crypto/KeyManagerTestImpl.kt @@ -15,12 +15,24 @@ class KeyManagerTestImpl : KeyManager { throw NotImplementedError("not implemented") } + override fun storeMainKey(seed: ByteArray) { + throw NotImplementedError("not implemented") + } + override fun hasBackupKey(): Boolean { return true } + override fun hasMainKey(): Boolean { + throw NotImplementedError("not implemented") + } + override fun getBackupKey(): SecretKey { return key } + override fun getMainKey(): SecretKey { + throw NotImplementedError("not implemented") + } + } diff --git a/app/src/test/java/com/stevesoltys/seedvault/crypto/KeyManagerImplTest.kt b/app/src/test/java/com/stevesoltys/seedvault/crypto/KeyManagerImplTest.kt new file mode 100644 index 00000000..7f7ee538 --- /dev/null +++ b/app/src/test/java/com/stevesoltys/seedvault/crypto/KeyManagerImplTest.kt @@ -0,0 +1,65 @@ +package com.stevesoltys.seedvault.crypto + +import com.stevesoltys.seedvault.getRandomByteArray +import io.mockk.Runs +import io.mockk.every +import io.mockk.just +import io.mockk.mockk +import io.mockk.slot +import junit.framework.Assert.assertTrue +import org.junit.Assert.assertArrayEquals +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.TestInstance +import org.junit.jupiter.api.TestInstance.Lifecycle.PER_METHOD +import org.junit.jupiter.api.assertThrows +import java.security.KeyStore + +@TestInstance(PER_METHOD) +class KeyManagerImplTest { + + private val keyStore: KeyStore = mockk() + private val keyManager = KeyManagerImpl(keyStore) + + @Test + fun `31 byte seed gets rejected for backup key`() { + val seed = getRandomByteArray(31) + assertThrows { + keyManager.storeBackupKey(seed) + } + } + + @Test + fun `63 byte seed gets rejected for main key`() { + val seed = getRandomByteArray(63) + assertThrows { + keyManager.storeMainKey(seed) + } + } + + @Test + fun `32 byte seed gets accepted for backup key`() { + val seed = getRandomByteArray(32) + val keyEntry = slot() + + every { keyStore.setEntry(any(), capture(keyEntry), any()) } just Runs + + keyManager.storeBackupKey(seed) + + assertTrue(keyEntry.isCaptured) + assertArrayEquals(seed.sliceArray(0 until 32), keyEntry.captured.secretKey.encoded) + } + + @Test + fun `64 byte seed gets accepted for main key`() { + val seed = getRandomByteArray(64) + val keyEntry = slot() + + every { keyStore.setEntry(any(), capture(keyEntry), any()) } just Runs + + keyManager.storeMainKey(seed) + + assertTrue(keyEntry.isCaptured) + assertArrayEquals(seed.sliceArray(32 until 64), keyEntry.captured.secretKey.encoded) + } + +} From 68543dcb919acdbbecd33429da64dadd68b8d0ca Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Mon, 15 Feb 2021 11:13:12 -0300 Subject: [PATCH 3/3] Store main key also when verifying recovery code in case it wasn't stored before --- .../seedvault/ui/recoverycode/RecoveryCodeViewModel.kt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/com/stevesoltys/seedvault/ui/recoverycode/RecoveryCodeViewModel.kt b/app/src/main/java/com/stevesoltys/seedvault/ui/recoverycode/RecoveryCodeViewModel.kt index 65e2a62a..c3069475 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/ui/recoverycode/RecoveryCodeViewModel.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/ui/recoverycode/RecoveryCodeViewModel.kt @@ -73,7 +73,9 @@ class RecoveryCodeViewModel( keyManager.storeMainKey(seed) mRecoveryCodeSaved.setEvent(true) } else { - mExistingCodeChecked.setEvent(crypto.verifyBackupKey(seed)) + val verified = crypto.verifyBackupKey(seed) + if (verified && !keyManager.hasMainKey()) keyManager.storeMainKey(seed) + mExistingCodeChecked.setEvent(verified) } }