From 8a6e886d09c6b4fc6ea5c1266a4bde4d39bc7eda Mon Sep 17 00:00:00 2001 From: akwizgran Date: Thu, 9 Jan 2020 11:22:39 +0000 Subject: [PATCH 1/9] Remove DB key migration code. --- .../account/AndroidAccountManager.java | 32 ------------------- .../account/AndroidAccountManagerTest.java | 29 ----------------- 2 files changed, 61 deletions(-) diff --git a/bramble-android/src/main/java/org/briarproject/bramble/account/AndroidAccountManager.java b/bramble-android/src/main/java/org/briarproject/bramble/account/AndroidAccountManager.java index 8a4648bfd..2dfc57287 100644 --- a/bramble-android/src/main/java/org/briarproject/bramble/account/AndroidAccountManager.java +++ b/bramble-android/src/main/java/org/briarproject/bramble/account/AndroidAccountManager.java @@ -29,8 +29,6 @@ class AndroidAccountManager extends AccountManagerImpl private static final Logger LOG = Logger.getLogger(AndroidAccountManager.class.getName()); - private static final String PREF_DB_KEY = "key"; - protected final Context appContext; private final SharedPreferences prefs; @@ -53,36 +51,6 @@ class AndroidAccountManager extends AccountManagerImpl return exists; } - // Locking: stateChangeLock - @Override - @Nullable - protected String loadEncryptedDatabaseKey() { - String key = getDatabaseKeyFromPreferences(); - if (key == null) key = super.loadEncryptedDatabaseKey(); - else migrateDatabaseKeyToFile(key); - return key; - } - - // Locking: stateChangeLock - @Nullable - private String getDatabaseKeyFromPreferences() { - String key = prefs.getString(PREF_DB_KEY, null); - if (key == null) LOG.info("No database key in preferences"); - else LOG.info("Found database key in preferences"); - return key; - } - - // Locking: stateChangeLock - private void migrateDatabaseKeyToFile(String key) { - if (storeEncryptedDatabaseKey(key)) { - if (prefs.edit().remove(PREF_DB_KEY).commit()) - LOG.info("Database key migrated to file"); - else LOG.warning("Database key not removed from preferences"); - } else { - LOG.warning("Database key not migrated to file"); - } - } - @Override public void deleteAccount() { synchronized (stateChangeLock) { diff --git a/bramble-android/src/test/java/org/briarproject/bramble/account/AndroidAccountManagerTest.java b/bramble-android/src/test/java/org/briarproject/bramble/account/AndroidAccountManagerTest.java index 89065b89e..55d7b08ff 100644 --- a/bramble-android/src/test/java/org/briarproject/bramble/account/AndroidAccountManagerTest.java +++ b/bramble-android/src/test/java/org/briarproject/bramble/account/AndroidAccountManagerTest.java @@ -16,13 +16,10 @@ import org.junit.Test; import java.io.File; -import static junit.framework.Assert.assertEquals; import static junit.framework.Assert.assertFalse; import static junit.framework.Assert.assertTrue; import static org.briarproject.bramble.test.TestUtils.deleteTestDirectory; -import static org.briarproject.bramble.test.TestUtils.getRandomBytes; import static org.briarproject.bramble.test.TestUtils.getTestDirectory; -import static org.briarproject.bramble.util.StringUtils.toHexString; public class AndroidAccountManagerTest extends BrambleMockTestCase { @@ -40,11 +37,8 @@ public class AndroidAccountManagerTest extends BrambleMockTestCase { private final Application app; private final ApplicationInfo applicationInfo; - private final String encryptedKeyHex = toHexString(getRandomBytes(123)); private final File testDir = getTestDirectory(); private final File keyDir = new File(testDir, "key"); - private final File keyFile = new File(keyDir, "db.key"); - private final File keyBackupFile = new File(keyDir, "db.key.bak"); private final File dbDir = new File(testDir, "db"); private AndroidAccountManager accountManager; @@ -75,29 +69,6 @@ public class AndroidAccountManagerTest extends BrambleMockTestCase { }; } - @Test - public void testDbKeyIsMigratedFromPreferencesToFile() { - context.checking(new Expectations() {{ - oneOf(prefs).getString("key", null); - will(returnValue(encryptedKeyHex)); - oneOf(prefs).edit(); - will(returnValue(editor)); - oneOf(editor).remove("key"); - will(returnValue(editor)); - oneOf(editor).commit(); - will(returnValue(true)); - }}); - - assertFalse(keyFile.exists()); - assertFalse(keyBackupFile.exists()); - - assertEquals(encryptedKeyHex, - accountManager.loadEncryptedDatabaseKey()); - - assertTrue(keyFile.exists()); - assertTrue(keyBackupFile.exists()); - } - @Test public void testDeleteAccountClearsSharedPrefsAndDeletesFiles() throws Exception { From 4d3c1b4fd2bf09fff18899f0d4cd24dc47921ec6 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Thu, 9 Jan 2020 14:06:42 +0000 Subject: [PATCH 2/9] Use Android keystore for encrypting DB key. Only for new accounts on API 23+. --- .../account/AndroidAccountManager.java | 3 +- .../bramble/api/crypto/CryptoComponent.java | 6 +- .../bramble/api/crypto/KeyStoreConfig.java | 19 +++++ .../bramble/api/db/DatabaseConfig.java | 6 ++ .../bramble/account/AccountManagerImpl.java | 25 +++--- .../bramble/crypto/CryptoComponentImpl.java | 83 +++++++++++++++++-- .../account/AccountManagerImplTest.java | 23 +++-- .../crypto/PasswordBasedEncryptionTest.java | 8 +- .../bramble/test/TestDatabaseConfig.java | 9 ++ .../briar/android/AndroidDatabaseConfig.java | 14 ++++ .../briar/android/AndroidKeyStoreConfig.java | 52 ++++++++++++ .../briar/headless/HeadlessDatabaseConfig.kt | 11 ++- 12 files changed, 222 insertions(+), 37 deletions(-) create mode 100644 bramble-api/src/main/java/org/briarproject/bramble/api/crypto/KeyStoreConfig.java create mode 100644 briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStoreConfig.java diff --git a/bramble-android/src/main/java/org/briarproject/bramble/account/AndroidAccountManager.java b/bramble-android/src/main/java/org/briarproject/bramble/account/AndroidAccountManager.java index 2dfc57287..012f76c4b 100644 --- a/bramble-android/src/main/java/org/briarproject/bramble/account/AndroidAccountManager.java +++ b/bramble-android/src/main/java/org/briarproject/bramble/account/AndroidAccountManager.java @@ -16,6 +16,7 @@ import java.util.Set; import java.util.logging.Logger; import javax.annotation.Nullable; +import javax.annotation.concurrent.GuardedBy; import javax.inject.Inject; import static android.os.Build.VERSION.SDK_INT; @@ -73,7 +74,7 @@ class AndroidAccountManager extends AccountManagerImpl return PreferenceManager.getDefaultSharedPreferences(appContext); } - // Locking: stateChangeLock + @GuardedBy("stateChangeLock") private void deleteAppData(SharedPreferences... clear) { // Clear and commit shared preferences for (SharedPreferences prefs : clear) { diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/CryptoComponent.java b/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/CryptoComponent.java index 9d498ec9d..95e89c2d0 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/CryptoComponent.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/CryptoComponent.java @@ -133,7 +133,8 @@ public interface CryptoComponent { * given password. The ciphertext will be decryptable using the same * password after the app restarts. */ - byte[] encryptWithPassword(byte[] plaintext, String password); + byte[] encryptWithPassword(byte[] plaintext, String password, + @Nullable KeyStoreConfig keyStoreConfig); /** * Decrypts and authenticates the given ciphertext that has been read from @@ -142,7 +143,8 @@ public interface CryptoComponent { * authenticated (for example, if the password is wrong). */ @Nullable - byte[] decryptWithPassword(byte[] ciphertext, String password); + byte[] decryptWithPassword(byte[] ciphertext, String password, + @Nullable KeyStoreConfig keyStoreConfig); /** * Encrypts the given plaintext to the given public key. diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/KeyStoreConfig.java b/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/KeyStoreConfig.java new file mode 100644 index 000000000..f3c44740b --- /dev/null +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/KeyStoreConfig.java @@ -0,0 +1,19 @@ +package org.briarproject.bramble.api.crypto; + +import org.briarproject.bramble.api.nullsafety.NotNullByDefault; + +import java.security.spec.AlgorithmParameterSpec; + +@NotNullByDefault +public interface KeyStoreConfig { + + String getKeyStoreType(); + + String getAlias(); + + String getProviderName(); + + String getMacAlgorithmName(); + + AlgorithmParameterSpec getParameterSpec(); +} diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/db/DatabaseConfig.java b/bramble-api/src/main/java/org/briarproject/bramble/api/db/DatabaseConfig.java index a01d12da5..bd916a9c6 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/db/DatabaseConfig.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/db/DatabaseConfig.java @@ -1,13 +1,19 @@ package org.briarproject.bramble.api.db; +import org.briarproject.bramble.api.crypto.KeyStoreConfig; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import java.io.File; +import javax.annotation.Nullable; + @NotNullByDefault public interface DatabaseConfig { File getDatabaseDirectory(); File getDatabaseKeyDirectory(); + + @Nullable + KeyStoreConfig getKeyStoreConfig(); } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/account/AccountManagerImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/account/AccountManagerImpl.java index c4b75b6c3..351572215 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/account/AccountManagerImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/account/AccountManagerImpl.java @@ -19,6 +19,7 @@ import java.io.InputStreamReader; import java.util.logging.Logger; import javax.annotation.Nullable; +import javax.annotation.concurrent.GuardedBy; import javax.inject.Inject; import static java.util.logging.Level.WARNING; @@ -68,9 +69,10 @@ class AccountManagerImpl implements AccountManager { return databaseKey; } - // Locking: stateChangeLock + // Package access for testing + @GuardedBy("stateChangeLock") @Nullable - protected String loadEncryptedDatabaseKey() { + String loadEncryptedDatabaseKey() { String key = readDbKeyFromFile(dbKeyFile); if (key == null) { LOG.info("No database key in primary file"); @@ -83,7 +85,7 @@ class AccountManagerImpl implements AccountManager { return key; } - // Locking: stateChangeLock + @GuardedBy("stateChangeLock") @Nullable private String readDbKeyFromFile(File f) { if (!f.exists()) { @@ -102,8 +104,9 @@ class AccountManagerImpl implements AccountManager { } } - // Locking: stateChangeLock - protected boolean storeEncryptedDatabaseKey(String hex) { + // Package access for testing + @GuardedBy("stateChangeLock") + boolean storeEncryptedDatabaseKey(String hex) { LOG.info("Storing database key in file"); // Create the directory if necessary if (databaseConfig.getDatabaseKeyDirectory().mkdirs()) @@ -140,7 +143,7 @@ class AccountManagerImpl implements AccountManager { } } - // Locking: stateChangeLock + @GuardedBy("stateChangeLock") private void writeDbKeyToFile(String key, File f) throws IOException { FileOutputStream out = new FileOutputStream(f); out.write(key.getBytes("UTF-8")); @@ -170,10 +173,11 @@ class AccountManagerImpl implements AccountManager { } } - // Locking: stateChangeLock + @GuardedBy("stateChangeLock") private boolean encryptAndStoreDatabaseKey(SecretKey key, String password) { byte[] plaintext = key.getBytes(); - byte[] ciphertext = crypto.encryptWithPassword(plaintext, password); + byte[] ciphertext = crypto.encryptWithPassword(plaintext, password, + databaseConfig.getKeyStoreConfig()); return storeEncryptedDatabaseKey(toHexString(ciphertext)); } @@ -197,7 +201,7 @@ class AccountManagerImpl implements AccountManager { } } - // Locking: stateChangeLock + @GuardedBy("stateChangeLock") @Nullable private SecretKey loadAndDecryptDatabaseKey(String password) { String hex = loadEncryptedDatabaseKey(); @@ -206,7 +210,8 @@ class AccountManagerImpl implements AccountManager { return null; } byte[] ciphertext = fromHexString(hex); - byte[] plaintext = crypto.decryptWithPassword(ciphertext, password); + byte[] plaintext = crypto.decryptWithPassword(ciphertext, password, + databaseConfig.getKeyStoreConfig()); if (plaintext == null) { LOG.info("Failed to decrypt database key"); return null; diff --git a/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java index 37aa5de3d..2bf96007c 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java @@ -9,6 +9,7 @@ import org.briarproject.bramble.api.crypto.AgreementPublicKey; import org.briarproject.bramble.api.crypto.CryptoComponent; import org.briarproject.bramble.api.crypto.KeyPair; import org.briarproject.bramble.api.crypto.KeyParser; +import org.briarproject.bramble.api.crypto.KeyStoreConfig; import org.briarproject.bramble.api.crypto.PrivateKey; import org.briarproject.bramble.api.crypto.PublicKey; import org.briarproject.bramble.api.crypto.SecretKey; @@ -24,7 +25,11 @@ import org.spongycastle.crypto.digests.Blake2bDigest; import org.whispersystems.curve25519.Curve25519; import org.whispersystems.curve25519.Curve25519KeyPair; +import java.io.IOException; import java.security.GeneralSecurityException; +import java.security.KeyStore; +import java.security.KeyStore.Entry; +import java.security.KeyStore.SecretKeyEntry; import java.security.NoSuchAlgorithmException; import java.security.Provider; import java.security.SecureRandom; @@ -32,12 +37,15 @@ import java.security.Security; import java.util.logging.Logger; import javax.annotation.Nullable; +import javax.crypto.KeyGenerator; +import javax.crypto.Mac; import javax.inject.Inject; import static java.lang.System.arraycopy; import static java.util.logging.Level.INFO; import static org.briarproject.bramble.api.crypto.CryptoConstants.KEY_TYPE_AGREEMENT; import static org.briarproject.bramble.api.crypto.CryptoConstants.KEY_TYPE_SIGNATURE; +import static org.briarproject.bramble.api.nullsafety.NullSafety.requireNonNull; import static org.briarproject.bramble.util.ByteUtils.INT_32_BYTES; import static org.briarproject.bramble.util.LogUtils.logDuration; import static org.briarproject.bramble.util.LogUtils.now; @@ -51,7 +59,8 @@ class CryptoComponentImpl implements CryptoComponent { private static final int SIGNATURE_KEY_PAIR_BITS = 256; private static final int STORAGE_IV_BYTES = 24; // 196 bits private static final int PBKDF_SALT_BYTES = 32; // 256 bits - private static final int PBKDF_FORMAT_SCRYPT = 0; + private static final byte PBKDF_FORMAT_SCRYPT = 0; + private static final byte PBKDF_FORMAT_SCRYPT_KEYSTORE = 1; private final SecureRandom secureRandom; private final PasswordBasedKdf passwordBasedKdf; @@ -311,7 +320,8 @@ class CryptoComponentImpl implements CryptoComponent { } @Override - public byte[] encryptWithPassword(byte[] input, String password) { + public byte[] encryptWithPassword(byte[] input, String password, + @Nullable KeyStoreConfig keyStoreConfig) { AuthenticatedCipher cipher = new XSalsa20Poly1305AuthenticatedCipher(); int macBytes = cipher.getMacBytes(); // Generate a random salt @@ -319,8 +329,10 @@ class CryptoComponentImpl implements CryptoComponent { secureRandom.nextBytes(salt); // Calibrate the KDF int cost = passwordBasedKdf.chooseCostParameter(); - // Derive the key from the password + // Derive the encryption key from the password SecretKey key = passwordBasedKdf.deriveKey(password, salt, cost); + if (keyStoreConfig != null) + key = requireNonNull(deriveKey(key, keyStoreConfig, true)); // Generate a random IV byte[] iv = new byte[STORAGE_IV_BYTES]; secureRandom.nextBytes(iv); @@ -331,7 +343,9 @@ class CryptoComponentImpl implements CryptoComponent { byte[] output = new byte[outputLen]; int outputOff = 0; // Format version - output[outputOff] = PBKDF_FORMAT_SCRYPT; + byte formatVersion = keyStoreConfig == null + ? PBKDF_FORMAT_SCRYPT : PBKDF_FORMAT_SCRYPT_KEYSTORE; + output[outputOff] = formatVersion; outputOff++; // Salt arraycopy(salt, 0, output, outputOff, salt.length); @@ -352,9 +366,53 @@ class CryptoComponentImpl implements CryptoComponent { } } + /** + * Derives a key from the given key and another key stored in a keystore. + * + * @param generateIfMissing Whether the stored key should be generated and + * stored if it doesn't already exist + * @return The derived key, or null if the stored key doesn't exist and + * {@code generateIfMissing} is false + */ + @Nullable + private SecretKey deriveKey(SecretKey key, KeyStoreConfig config, + boolean generateIfMissing) { + try { + // Load the keystore + KeyStore ks = KeyStore.getInstance(config.getKeyStoreType()); + ks.load(null); + // Load or generate the stored key + javax.crypto.SecretKey storedKey; + Entry e = ks.getEntry(config.getAlias(), null); + if (e == null) { + if (!generateIfMissing) { + LOG.warning("Key not found in keystore"); + return null; + } + KeyGenerator kg = KeyGenerator.getInstance( + config.getMacAlgorithmName(), config.getProviderName()); + kg.init(config.getParameterSpec()); + storedKey = kg.generateKey(); + LOG.info("Stored key in keystore"); + } else { + if (!(e instanceof SecretKeyEntry)) + throw new IllegalArgumentException(); + storedKey = ((SecretKeyEntry) e).getSecretKey(); + LOG.info("Loaded key from keystore"); + } + // Use the input key and the stored key to derive the output key + Mac mac = Mac.getInstance(config.getMacAlgorithmName()); + mac.init(storedKey); + return new SecretKey(mac.doFinal(key.getBytes())); + } catch (GeneralSecurityException | IOException e) { + throw new IllegalArgumentException(e); + } + } + @Override @Nullable - public byte[] decryptWithPassword(byte[] input, String password) { + public byte[] decryptWithPassword(byte[] input, String password, + @Nullable KeyStoreConfig keyStoreConfig) { AuthenticatedCipher cipher = new XSalsa20Poly1305AuthenticatedCipher(); int macBytes = cipher.getMacBytes(); // The input contains the format version, salt, cost parameter, IV, @@ -366,8 +424,13 @@ class CryptoComponentImpl implements CryptoComponent { // Format version byte formatVersion = input[inputOff]; inputOff++; - if (formatVersion != PBKDF_FORMAT_SCRYPT) - return null; // Unknown format + // Check whether we support this format version + if (formatVersion != PBKDF_FORMAT_SCRYPT && + formatVersion != PBKDF_FORMAT_SCRYPT_KEYSTORE) { + return null; + } + boolean useKeyStore = keyStoreConfig != null && + formatVersion == PBKDF_FORMAT_SCRYPT_KEYSTORE; // Salt byte[] salt = new byte[PBKDF_SALT_BYTES]; arraycopy(input, inputOff, salt, 0, salt.length); @@ -381,8 +444,12 @@ class CryptoComponentImpl implements CryptoComponent { byte[] iv = new byte[STORAGE_IV_BYTES]; arraycopy(input, inputOff, iv, 0, iv.length); inputOff += iv.length; - // Derive the key from the password + // Derive the decryption key from the password SecretKey key = passwordBasedKdf.deriveKey(password, salt, (int) cost); + if (useKeyStore) { + key = deriveKey(key, keyStoreConfig, false); + if (key == null) return null; + } // Initialise the cipher try { cipher.init(false, key, iv); diff --git a/bramble-core/src/test/java/org/briarproject/bramble/account/AccountManagerImplTest.java b/bramble-core/src/test/java/org/briarproject/bramble/account/AccountManagerImplTest.java index e1c1c1bb1..49fe8b202 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/account/AccountManagerImplTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/account/AccountManagerImplTest.java @@ -1,6 +1,7 @@ package org.briarproject.bramble.account; import org.briarproject.bramble.api.crypto.CryptoComponent; +import org.briarproject.bramble.api.crypto.KeyStoreConfig; import org.briarproject.bramble.api.crypto.SecretKey; import org.briarproject.bramble.api.db.DatabaseConfig; import org.briarproject.bramble.api.identity.Identity; @@ -39,6 +40,8 @@ public class AccountManagerImplTest extends BrambleMockTestCase { private final DatabaseConfig databaseConfig = context.mock(DatabaseConfig.class); + private final KeyStoreConfig keyStoreConfig = + context.mock(KeyStoreConfig.class); private final CryptoComponent crypto = context.mock(CryptoComponent.class); private final IdentityManager identityManager = context.mock(IdentityManager.class); @@ -68,6 +71,8 @@ public class AccountManagerImplTest extends BrambleMockTestCase { will(returnValue(dbDir)); allowing(databaseConfig).getDatabaseKeyDirectory(); will(returnValue(keyDir)); + allowing(databaseConfig).getKeyStoreConfig(); + will(returnValue(keyStoreConfig)); }}); accountManager = @@ -89,7 +94,8 @@ public class AccountManagerImplTest extends BrambleMockTestCase { @Test public void testSignInReturnsFalseIfPasswordIsWrong() throws Exception { context.checking(new Expectations() {{ - oneOf(crypto).decryptWithPassword(encryptedKey, password); + oneOf(crypto).decryptWithPassword(encryptedKey, password, + keyStoreConfig); will(returnValue(null)); }}); @@ -109,7 +115,8 @@ public class AccountManagerImplTest extends BrambleMockTestCase { @Test public void testSignInReturnsTrueIfPasswordIsRight() throws Exception { context.checking(new Expectations() {{ - oneOf(crypto).decryptWithPassword(encryptedKey, password); + oneOf(crypto).decryptWithPassword(encryptedKey, password, + keyStoreConfig); will(returnValue(key.getBytes())); }}); @@ -258,7 +265,8 @@ public class AccountManagerImplTest extends BrambleMockTestCase { oneOf(identityManager).registerIdentity(identity); oneOf(crypto).generateSecretKey(); will(returnValue(key)); - oneOf(crypto).encryptWithPassword(key.getBytes(), password); + oneOf(crypto).encryptWithPassword(key.getBytes(), password, + keyStoreConfig); will(returnValue(encryptedKey)); }}); @@ -287,7 +295,8 @@ public class AccountManagerImplTest extends BrambleMockTestCase { public void testChangePasswordReturnsFalseIfPasswordIsWrong() throws Exception { context.checking(new Expectations() {{ - oneOf(crypto).decryptWithPassword(encryptedKey, password); + oneOf(crypto).decryptWithPassword(encryptedKey, password, + keyStoreConfig); will(returnValue(null)); }}); @@ -304,9 +313,11 @@ public class AccountManagerImplTest extends BrambleMockTestCase { public void testChangePasswordReturnsTrueIfPasswordIsRight() throws Exception { context.checking(new Expectations() {{ - oneOf(crypto).decryptWithPassword(encryptedKey, password); + oneOf(crypto).decryptWithPassword(encryptedKey, password, + keyStoreConfig); will(returnValue(key.getBytes())); - oneOf(crypto).encryptWithPassword(key.getBytes(), newPassword); + oneOf(crypto).encryptWithPassword(key.getBytes(), newPassword, + keyStoreConfig); will(returnValue(newEncryptedKey)); }}); diff --git a/bramble-core/src/test/java/org/briarproject/bramble/crypto/PasswordBasedEncryptionTest.java b/bramble-core/src/test/java/org/briarproject/bramble/crypto/PasswordBasedEncryptionTest.java index 5efc3593e..e2380f655 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/crypto/PasswordBasedEncryptionTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/crypto/PasswordBasedEncryptionTest.java @@ -21,8 +21,8 @@ public class PasswordBasedEncryptionTest extends BrambleTestCase { public void testEncryptionAndDecryption() { byte[] input = TestUtils.getRandomBytes(1234); String password = "password"; - byte[] ciphertext = crypto.encryptWithPassword(input, password); - byte[] output = crypto.decryptWithPassword(ciphertext, password); + byte[] ciphertext = crypto.encryptWithPassword(input, password, null); + byte[] output = crypto.decryptWithPassword(ciphertext, password, null); assertArrayEquals(input, output); } @@ -30,11 +30,11 @@ public class PasswordBasedEncryptionTest extends BrambleTestCase { public void testInvalidCiphertextReturnsNull() { byte[] input = TestUtils.getRandomBytes(1234); String password = "password"; - byte[] ciphertext = crypto.encryptWithPassword(input, password); + byte[] ciphertext = crypto.encryptWithPassword(input, password, null); // Modify the ciphertext int position = new Random().nextInt(ciphertext.length); ciphertext[position] = (byte) (ciphertext[position] ^ 0xFF); - byte[] output = crypto.decryptWithPassword(ciphertext, password); + byte[] output = crypto.decryptWithPassword(ciphertext, password, null); assertNull(output); } } diff --git a/bramble-core/src/test/java/org/briarproject/bramble/test/TestDatabaseConfig.java b/bramble-core/src/test/java/org/briarproject/bramble/test/TestDatabaseConfig.java index adf0712b1..f285f1c80 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/test/TestDatabaseConfig.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/test/TestDatabaseConfig.java @@ -1,10 +1,13 @@ package org.briarproject.bramble.test; +import org.briarproject.bramble.api.crypto.KeyStoreConfig; import org.briarproject.bramble.api.db.DatabaseConfig; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import java.io.File; +import javax.annotation.Nullable; + @NotNullByDefault public class TestDatabaseConfig implements DatabaseConfig { @@ -24,4 +27,10 @@ public class TestDatabaseConfig implements DatabaseConfig { public File getDatabaseKeyDirectory() { return keyDir; } + + @Nullable + @Override + public KeyStoreConfig getKeyStoreConfig() { + return null; + } } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/AndroidDatabaseConfig.java b/briar-android/src/main/java/org/briarproject/briar/android/AndroidDatabaseConfig.java index f1fdc2492..c27cc2274 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/AndroidDatabaseConfig.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/AndroidDatabaseConfig.java @@ -1,18 +1,26 @@ package org.briarproject.briar.android; +import org.briarproject.bramble.api.crypto.KeyStoreConfig; import org.briarproject.bramble.api.db.DatabaseConfig; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import java.io.File; +import javax.annotation.Nullable; + +import static android.os.Build.VERSION.SDK_INT; + @NotNullByDefault class AndroidDatabaseConfig implements DatabaseConfig { private final File dbDir, keyDir; + @Nullable + private final KeyStoreConfig keyStoreConfig; AndroidDatabaseConfig(File dbDir, File keyDir) { this.dbDir = dbDir; this.keyDir = keyDir; + keyStoreConfig = SDK_INT >= 23 ? new AndroidKeyStoreConfig() : null; } @Override @@ -24,4 +32,10 @@ class AndroidDatabaseConfig implements DatabaseConfig { public File getDatabaseKeyDirectory() { return keyDir; } + + @Nullable + @Override + public KeyStoreConfig getKeyStoreConfig() { + return keyStoreConfig; + } } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStoreConfig.java b/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStoreConfig.java new file mode 100644 index 000000000..752272c15 --- /dev/null +++ b/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStoreConfig.java @@ -0,0 +1,52 @@ +package org.briarproject.briar.android; + +import android.security.keystore.KeyGenParameterSpec; + +import org.briarproject.bramble.api.crypto.KeyStoreConfig; +import org.briarproject.bramble.api.nullsafety.NotNullByDefault; + +import java.security.spec.AlgorithmParameterSpec; + +import androidx.annotation.RequiresApi; + +import static android.security.keystore.KeyProperties.PURPOSE_SIGN; +import static android.security.keystore.KeyProperties.PURPOSE_VERIFY; + +@RequiresApi(23) +@NotNullByDefault +class AndroidKeyStoreConfig implements KeyStoreConfig { + + private final KeyGenParameterSpec spec; + + AndroidKeyStoreConfig() { + int purposes = PURPOSE_SIGN | PURPOSE_VERIFY; + spec = new KeyGenParameterSpec.Builder("db", purposes) + .setKeySize(256) + .build(); + } + + @Override + public String getKeyStoreType() { + return "AndroidKeyStore"; + } + + @Override + public String getAlias() { + return "db"; + } + + @Override + public String getProviderName() { + return "AndroidKeyStore"; + } + + @Override + public String getMacAlgorithmName() { + return "HmacSHA256"; + } + + @Override + public AlgorithmParameterSpec getParameterSpec() { + return spec; + } +} diff --git a/briar-headless/src/main/java/org/briarproject/briar/headless/HeadlessDatabaseConfig.kt b/briar-headless/src/main/java/org/briarproject/briar/headless/HeadlessDatabaseConfig.kt index d7082e998..6dca9ae10 100644 --- a/briar-headless/src/main/java/org/briarproject/briar/headless/HeadlessDatabaseConfig.kt +++ b/briar-headless/src/main/java/org/briarproject/briar/headless/HeadlessDatabaseConfig.kt @@ -1,16 +1,15 @@ package org.briarproject.briar.headless +import org.briarproject.bramble.api.crypto.KeyStoreConfig import org.briarproject.bramble.api.db.DatabaseConfig import java.io.File internal class HeadlessDatabaseConfig(private val dbDir: File, private val keyDir: File) : DatabaseConfig { - override fun getDatabaseDirectory(): File { - return dbDir - } + override fun getDatabaseDirectory() = dbDir - override fun getDatabaseKeyDirectory(): File { - return keyDir - } + override fun getDatabaseKeyDirectory() = keyDir + + override fun getKeyStoreConfig(): KeyStoreConfig? = null } From d7b05dcba0b03331bbc5776af83922297ad5d06c Mon Sep 17 00:00:00 2001 From: akwizgran Date: Thu, 9 Jan 2020 14:17:55 +0000 Subject: [PATCH 3/9] Add javadocs. --- .../bramble/api/crypto/CryptoComponent.java | 7 +++++++ .../bramble/api/crypto/KeyStoreConfig.java | 10 +++++++++- .../briarproject/bramble/api/db/DatabaseConfig.java | 10 ++++++++++ .../bramble/crypto/CryptoComponentImpl.java | 2 +- .../briar/android/AndroidKeyStoreConfig.java | 2 +- 5 files changed, 28 insertions(+), 3 deletions(-) diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/CryptoComponent.java b/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/CryptoComponent.java index 95e89c2d0..1cfffa3c2 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/CryptoComponent.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/CryptoComponent.java @@ -132,6 +132,9 @@ public interface CryptoComponent { * storage. The encryption and authentication keys are derived from the * given password. The ciphertext will be decryptable using the same * password after the app restarts. + * + * @param keyStoreConfig Configures the use of a stored key to strengthen + * the password-based key. If null, no stored key will be used */ byte[] encryptWithPassword(byte[] plaintext, String password, @Nullable KeyStoreConfig keyStoreConfig); @@ -141,6 +144,10 @@ public interface CryptoComponent { * storage. The encryption and authentication keys are derived from the * given password. Returns null if the ciphertext cannot be decrypted and * authenticated (for example, if the password is wrong). + * + * @param keyStoreConfig Configures the use of a stored key to strengthen + * the password-based key. If null, or if no stored key was used when + * encrypting the ciphertext, then no stored key will be used */ @Nullable byte[] decryptWithPassword(byte[] ciphertext, String password, diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/KeyStoreConfig.java b/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/KeyStoreConfig.java index f3c44740b..39f545f2f 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/KeyStoreConfig.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/KeyStoreConfig.java @@ -4,12 +4,20 @@ import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import java.security.spec.AlgorithmParameterSpec; +/** + * Configures the use of a stored key to strengthen password-based encryption. + * The key may be stored in a hardware security module, but this is not + * guaranteed. See + * {@link CryptoComponent#encryptWithPassword(byte[], String, KeyStoreConfig)} + * and + * {@link CryptoComponent#decryptWithPassword(byte[], String, KeyStoreConfig)}. + */ @NotNullByDefault public interface KeyStoreConfig { String getKeyStoreType(); - String getAlias(); + String getKeyAlias(); String getProviderName(); diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/db/DatabaseConfig.java b/bramble-api/src/main/java/org/briarproject/bramble/api/db/DatabaseConfig.java index bd916a9c6..36982595d 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/db/DatabaseConfig.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/db/DatabaseConfig.java @@ -10,10 +10,20 @@ import javax.annotation.Nullable; @NotNullByDefault public interface DatabaseConfig { + /** + * Returns the directory where the database stores its data. + */ File getDatabaseDirectory(); + /** + * Returns the directory where the encrypted database key is stored. + */ File getDatabaseKeyDirectory(); + /** + * Returns a {@link KeyStoreConfig} for strengthening the encryption of the + * database key, or null if no keystore should be used. + */ @Nullable KeyStoreConfig getKeyStoreConfig(); } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java index 2bf96007c..27052d737 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java @@ -383,7 +383,7 @@ class CryptoComponentImpl implements CryptoComponent { ks.load(null); // Load or generate the stored key javax.crypto.SecretKey storedKey; - Entry e = ks.getEntry(config.getAlias(), null); + Entry e = ks.getEntry(config.getKeyAlias(), null); if (e == null) { if (!generateIfMissing) { LOG.warning("Key not found in keystore"); diff --git a/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStoreConfig.java b/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStoreConfig.java index 752272c15..83ce8db6b 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStoreConfig.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStoreConfig.java @@ -31,7 +31,7 @@ class AndroidKeyStoreConfig implements KeyStoreConfig { } @Override - public String getAlias() { + public String getKeyAlias() { return "db"; } From c11d09a8858566b2e4a3be436c27af0443359547 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Thu, 9 Jan 2020 14:32:11 +0000 Subject: [PATCH 4/9] Re-encrypt the DB key with the stored key. --- .../bramble/api/crypto/CryptoComponent.java | 7 ++++ .../bramble/account/AccountManagerImpl.java | 14 ++++++-- .../bramble/crypto/CryptoComponentImpl.java | 6 ++++ .../account/AccountManagerImplTest.java | 33 +++++++++++++++++++ 4 files changed, 58 insertions(+), 2 deletions(-) diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/CryptoComponent.java b/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/CryptoComponent.java index 1cfffa3c2..92feb2d28 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/CryptoComponent.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/CryptoComponent.java @@ -153,6 +153,13 @@ public interface CryptoComponent { byte[] decryptWithPassword(byte[] ciphertext, String password, @Nullable KeyStoreConfig keyStoreConfig); + /** + * Returns true if the given ciphertext was encrypted using a stored key + * to strengthen the password-based key. The validity of the ciphertext is + * not checked. + */ + boolean isEncryptedWithStoredKey(byte[] ciphertext); + /** * Encrypts the given plaintext to the given public key. */ diff --git a/bramble-core/src/main/java/org/briarproject/bramble/account/AccountManagerImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/account/AccountManagerImpl.java index 351572215..bec84eda6 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/account/AccountManagerImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/account/AccountManagerImpl.java @@ -2,6 +2,7 @@ package org.briarproject.bramble.account; import org.briarproject.bramble.api.account.AccountManager; import org.briarproject.bramble.api.crypto.CryptoComponent; +import org.briarproject.bramble.api.crypto.KeyStoreConfig; import org.briarproject.bramble.api.crypto.SecretKey; import org.briarproject.bramble.api.db.DatabaseConfig; import org.briarproject.bramble.api.identity.Identity; @@ -210,13 +211,22 @@ class AccountManagerImpl implements AccountManager { return null; } byte[] ciphertext = fromHexString(hex); + KeyStoreConfig keyStoreConfig = databaseConfig.getKeyStoreConfig(); byte[] plaintext = crypto.decryptWithPassword(ciphertext, password, - databaseConfig.getKeyStoreConfig()); + keyStoreConfig); if (plaintext == null) { LOG.info("Failed to decrypt database key"); return null; } - return new SecretKey(plaintext); + SecretKey key = new SecretKey(plaintext); + // If the DB key was encrypted without using a stored key and a stored + // key is now available, re-encrypt the DB key with the stored key + if (keyStoreConfig != null && + !crypto.isEncryptedWithStoredKey(ciphertext)) { + LOG.info("Re-encrypting database key with stored key"); + encryptAndStoreDatabaseKey(key, password); + } + return key; } @Override diff --git a/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java index 27052d737..0ae209b1b 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java @@ -467,6 +467,12 @@ class CryptoComponentImpl implements CryptoComponent { } } + @Override + public boolean isEncryptedWithStoredKey(byte[] ciphertext) { + return ciphertext.length > 0 && + ciphertext[0] == PBKDF_FORMAT_SCRYPT_KEYSTORE; + } + @Override public byte[] encryptToKey(PublicKey publicKey, byte[] plaintext) { try { diff --git a/bramble-core/src/test/java/org/briarproject/bramble/account/AccountManagerImplTest.java b/bramble-core/src/test/java/org/briarproject/bramble/account/AccountManagerImplTest.java index 49fe8b202..d06bfa633 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/account/AccountManagerImplTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/account/AccountManagerImplTest.java @@ -118,6 +118,8 @@ public class AccountManagerImplTest extends BrambleMockTestCase { oneOf(crypto).decryptWithPassword(encryptedKey, password, keyStoreConfig); will(returnValue(key.getBytes())); + oneOf(crypto).isEncryptedWithStoredKey(encryptedKey); + will(returnValue(true)); }}); storeDatabaseKey(keyFile, encryptedKeyHex); @@ -136,6 +138,35 @@ public class AccountManagerImplTest extends BrambleMockTestCase { assertEquals(encryptedKeyHex, loadDatabaseKey(keyBackupFile)); } + @Test + public void testSignInReEncryptsKey() throws Exception { + context.checking(new Expectations() {{ + oneOf(crypto).decryptWithPassword(encryptedKey, password, + keyStoreConfig); + will(returnValue(key.getBytes())); + oneOf(crypto).isEncryptedWithStoredKey(encryptedKey); + will(returnValue(false)); + oneOf(crypto).encryptWithPassword(key.getBytes(), password, + keyStoreConfig); + will(returnValue(newEncryptedKey)); + }}); + + storeDatabaseKey(keyFile, encryptedKeyHex); + storeDatabaseKey(keyBackupFile, encryptedKeyHex); + + assertEquals(encryptedKeyHex, loadDatabaseKey(keyFile)); + assertEquals(encryptedKeyHex, loadDatabaseKey(keyBackupFile)); + + assertTrue(accountManager.signIn(password)); + assertTrue(accountManager.hasDatabaseKey()); + SecretKey decrypted = accountManager.getDatabaseKey(); + assertNotNull(decrypted); + assertArrayEquals(key.getBytes(), decrypted.getBytes()); + + assertEquals(newEncryptedKeyHex, loadDatabaseKey(keyFile)); + assertEquals(newEncryptedKeyHex, loadDatabaseKey(keyBackupFile)); + } + @Test public void testDbKeyIsLoadedFromPrimaryFile() throws Exception { storeDatabaseKey(keyFile, encryptedKeyHex); @@ -316,6 +347,8 @@ public class AccountManagerImplTest extends BrambleMockTestCase { oneOf(crypto).decryptWithPassword(encryptedKey, password, keyStoreConfig); will(returnValue(key.getBytes())); + oneOf(crypto).isEncryptedWithStoredKey(encryptedKey); + will(returnValue(true)); oneOf(crypto).encryptWithPassword(key.getBytes(), newPassword, keyStoreConfig); will(returnValue(newEncryptedKey)); From fc6b59624196c4f5a74f9671d32aa63f1b2ef929 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Thu, 9 Jan 2020 15:03:46 +0000 Subject: [PATCH 5/9] Remove unnecessary key purpose. --- .../org/briarproject/briar/android/AndroidKeyStoreConfig.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStoreConfig.java b/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStoreConfig.java index 83ce8db6b..e6aff2dad 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStoreConfig.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStoreConfig.java @@ -10,7 +10,6 @@ import java.security.spec.AlgorithmParameterSpec; import androidx.annotation.RequiresApi; import static android.security.keystore.KeyProperties.PURPOSE_SIGN; -import static android.security.keystore.KeyProperties.PURPOSE_VERIFY; @RequiresApi(23) @NotNullByDefault @@ -19,8 +18,7 @@ class AndroidKeyStoreConfig implements KeyStoreConfig { private final KeyGenParameterSpec spec; AndroidKeyStoreConfig() { - int purposes = PURPOSE_SIGN | PURPOSE_VERIFY; - spec = new KeyGenParameterSpec.Builder("db", purposes) + spec = new KeyGenParameterSpec.Builder("db", PURPOSE_SIGN) .setKeySize(256) .build(); } From f76d08c19adb9d7a2445321a451d6fb8b09b2a82 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Thu, 9 Jan 2020 15:18:58 +0000 Subject: [PATCH 6/9] Use StrongBox on API 28+ if available. --- .../bramble/api/crypto/KeyStoreConfig.java | 7 ++++- .../bramble/crypto/CryptoComponentImpl.java | 22 +++++++++++---- .../briar/android/AndroidKeyStoreConfig.java | 28 +++++++++++++++---- 3 files changed, 45 insertions(+), 12 deletions(-) diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/KeyStoreConfig.java b/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/KeyStoreConfig.java index 39f545f2f..5533e994d 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/KeyStoreConfig.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/KeyStoreConfig.java @@ -3,6 +3,7 @@ package org.briarproject.bramble.api.crypto; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import java.security.spec.AlgorithmParameterSpec; +import java.util.List; /** * Configures the use of a stored key to strengthen password-based encryption. @@ -23,5 +24,9 @@ public interface KeyStoreConfig { String getMacAlgorithmName(); - AlgorithmParameterSpec getParameterSpec(); + /** + * Returns a list of {@link AlgorithmParameterSpec AlgorithmParameterSpecs} + * to use for key generation, in order of preference. + */ + List getParameterSpecs(); } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java index 0ae209b1b..a634a0c8b 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java @@ -34,6 +34,7 @@ import java.security.NoSuchAlgorithmException; import java.security.Provider; import java.security.SecureRandom; import java.security.Security; +import java.security.spec.AlgorithmParameterSpec; import java.util.logging.Logger; import javax.annotation.Nullable; @@ -382,17 +383,28 @@ class CryptoComponentImpl implements CryptoComponent { KeyStore ks = KeyStore.getInstance(config.getKeyStoreType()); ks.load(null); // Load or generate the stored key - javax.crypto.SecretKey storedKey; + javax.crypto.SecretKey storedKey = null; Entry e = ks.getEntry(config.getKeyAlias(), null); if (e == null) { if (!generateIfMissing) { LOG.warning("Key not found in keystore"); return null; } - KeyGenerator kg = KeyGenerator.getInstance( - config.getMacAlgorithmName(), config.getProviderName()); - kg.init(config.getParameterSpec()); - storedKey = kg.generateKey(); + // Try the parameter specs in order of preference + for (AlgorithmParameterSpec spec : config.getParameterSpecs()) { + try { + KeyGenerator kg = KeyGenerator.getInstance( + config.getMacAlgorithmName(), + config.getProviderName()); + kg.init(spec); + storedKey = kg.generateKey(); + } catch (GeneralSecurityException e1) { + if (LOG.isLoggable(INFO)) + LOG.info("Could not generate key: " + e1); + // Fall back to next spec + } + } + if (storedKey == null) throw new IllegalArgumentException(); LOG.info("Stored key in keystore"); } else { if (!(e instanceof SecretKeyEntry)) diff --git a/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStoreConfig.java b/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStoreConfig.java index e6aff2dad..5f9b4b1e8 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStoreConfig.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStoreConfig.java @@ -6,21 +6,37 @@ import org.briarproject.bramble.api.crypto.KeyStoreConfig; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import java.security.spec.AlgorithmParameterSpec; +import java.util.List; import androidx.annotation.RequiresApi; +import static android.os.Build.VERSION.SDK_INT; import static android.security.keystore.KeyProperties.PURPOSE_SIGN; +import static java.util.Arrays.asList; +import static java.util.Collections.singletonList; @RequiresApi(23) @NotNullByDefault class AndroidKeyStoreConfig implements KeyStoreConfig { - private final KeyGenParameterSpec spec; + private final List specs; AndroidKeyStoreConfig() { - spec = new KeyGenParameterSpec.Builder("db", PURPOSE_SIGN) - .setKeySize(256) - .build(); + KeyGenParameterSpec noStrongBox = + new KeyGenParameterSpec.Builder("db", PURPOSE_SIGN) + .setKeySize(256) + .build(); + if (SDK_INT >= 28) { + // Prefer StrongBox if available + KeyGenParameterSpec strongBox = + new KeyGenParameterSpec.Builder("db", PURPOSE_SIGN) + .setIsStrongBoxBacked(true) + .setKeySize(256) + .build(); + specs = asList(strongBox, noStrongBox); + } else { + specs = singletonList(noStrongBox); + } } @Override @@ -44,7 +60,7 @@ class AndroidKeyStoreConfig implements KeyStoreConfig { } @Override - public AlgorithmParameterSpec getParameterSpec() { - return spec; + public List getParameterSpecs() { + return specs; } } From 72a391b5067ef19379171f87bf3f0d8cff13f4f8 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Fri, 10 Jan 2020 12:22:47 +0000 Subject: [PATCH 7/9] Break out of loop after generating key. --- .../org/briarproject/bramble/crypto/CryptoComponentImpl.java | 1 + 1 file changed, 1 insertion(+) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java index a634a0c8b..76d764215 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java @@ -398,6 +398,7 @@ class CryptoComponentImpl implements CryptoComponent { config.getProviderName()); kg.init(spec); storedKey = kg.generateKey(); + break; } catch (GeneralSecurityException e1) { if (LOG.isLoggable(INFO)) LOG.info("Could not generate key: " + e1); From f650b2236e88dfe4f476166977270b71cadc720c Mon Sep 17 00:00:00 2001 From: akwizgran Date: Fri, 10 Jan 2020 16:15:56 +0000 Subject: [PATCH 8/9] Catch any Exception when generating stored key. --- .../org/briarproject/bramble/crypto/CryptoComponentImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java index 76d764215..d4a1785fa 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java @@ -399,7 +399,7 @@ class CryptoComponentImpl implements CryptoComponent { kg.init(spec); storedKey = kg.generateKey(); break; - } catch (GeneralSecurityException e1) { + } catch (Exception e1) { if (LOG.isLoggable(INFO)) LOG.info("Could not generate key: " + e1); // Fall back to next spec From c61c9bbc02a6005dc917c3f1380270a6af637f1c Mon Sep 17 00:00:00 2001 From: akwizgran Date: Fri, 10 Jan 2020 17:31:16 +0000 Subject: [PATCH 9/9] Refactor Android-specific code out of bramble-core. --- .../bramble/api/crypto/CryptoComponent.java | 21 ++-- .../bramble/api/crypto/KeyStoreConfig.java | 32 ----- .../bramble/api/crypto/KeyStrengthener.java | 23 ++++ .../bramble/api/db/DatabaseConfig.java | 8 +- .../bramble/account/AccountManagerImpl.java | 18 +-- .../bramble/crypto/CryptoComponentImpl.java | 93 +++----------- .../account/AccountManagerImplTest.java | 32 ++--- .../bramble/test/TestDatabaseConfig.java | 4 +- .../briar/android/AndroidDatabaseConfig.java | 15 ++- .../briar/android/AndroidKeyStoreConfig.java | 66 ---------- .../briar/android/AndroidKeyStrengthener.java | 118 ++++++++++++++++++ .../briarproject/briar/android/AppModule.java | 6 +- .../briar/headless/HeadlessDatabaseConfig.kt | 4 +- 13 files changed, 210 insertions(+), 230 deletions(-) delete mode 100644 bramble-api/src/main/java/org/briarproject/bramble/api/crypto/KeyStoreConfig.java create mode 100644 bramble-api/src/main/java/org/briarproject/bramble/api/crypto/KeyStrengthener.java delete mode 100644 briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStoreConfig.java create mode 100644 briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStrengthener.java diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/CryptoComponent.java b/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/CryptoComponent.java index 92feb2d28..d1cbc63da 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/CryptoComponent.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/CryptoComponent.java @@ -133,11 +133,11 @@ public interface CryptoComponent { * given password. The ciphertext will be decryptable using the same * password after the app restarts. * - * @param keyStoreConfig Configures the use of a stored key to strengthen - * the password-based key. If null, no stored key will be used + * @param keyStrengthener Used to strengthen the password-based key. If + * null, the password-based key will not be strengthened */ byte[] encryptWithPassword(byte[] plaintext, String password, - @Nullable KeyStoreConfig keyStoreConfig); + @Nullable KeyStrengthener keyStrengthener); /** * Decrypts and authenticates the given ciphertext that has been read from @@ -145,20 +145,19 @@ public interface CryptoComponent { * given password. Returns null if the ciphertext cannot be decrypted and * authenticated (for example, if the password is wrong). * - * @param keyStoreConfig Configures the use of a stored key to strengthen - * the password-based key. If null, or if no stored key was used when - * encrypting the ciphertext, then no stored key will be used + * @param keyStrengthener Used to strengthen the password-based key. If + * null, or if strengthening was not used when encrypting the ciphertext, + * the password-based key will not be strengthened */ @Nullable byte[] decryptWithPassword(byte[] ciphertext, String password, - @Nullable KeyStoreConfig keyStoreConfig); + @Nullable KeyStrengthener keyStrengthener); /** - * Returns true if the given ciphertext was encrypted using a stored key - * to strengthen the password-based key. The validity of the ciphertext is - * not checked. + * Returns true if the given ciphertext was encrypted using a strengthened + * key. The validity of the ciphertext is not checked. */ - boolean isEncryptedWithStoredKey(byte[] ciphertext); + boolean isEncryptedWithStrengthenedKey(byte[] ciphertext); /** * Encrypts the given plaintext to the given public key. diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/KeyStoreConfig.java b/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/KeyStoreConfig.java deleted file mode 100644 index 5533e994d..000000000 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/KeyStoreConfig.java +++ /dev/null @@ -1,32 +0,0 @@ -package org.briarproject.bramble.api.crypto; - -import org.briarproject.bramble.api.nullsafety.NotNullByDefault; - -import java.security.spec.AlgorithmParameterSpec; -import java.util.List; - -/** - * Configures the use of a stored key to strengthen password-based encryption. - * The key may be stored in a hardware security module, but this is not - * guaranteed. See - * {@link CryptoComponent#encryptWithPassword(byte[], String, KeyStoreConfig)} - * and - * {@link CryptoComponent#decryptWithPassword(byte[], String, KeyStoreConfig)}. - */ -@NotNullByDefault -public interface KeyStoreConfig { - - String getKeyStoreType(); - - String getKeyAlias(); - - String getProviderName(); - - String getMacAlgorithmName(); - - /** - * Returns a list of {@link AlgorithmParameterSpec AlgorithmParameterSpecs} - * to use for key generation, in order of preference. - */ - List getParameterSpecs(); -} diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/KeyStrengthener.java b/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/KeyStrengthener.java new file mode 100644 index 000000000..5913aae5d --- /dev/null +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/KeyStrengthener.java @@ -0,0 +1,23 @@ +package org.briarproject.bramble.api.crypto; + +import org.briarproject.bramble.api.nullsafety.NotNullByDefault; + +/** + * Interface for strengthening a password-based key, for example by using a + * key stored in a key management service or hardware security module. + */ +@NotNullByDefault +public interface KeyStrengthener { + + /** + * Returns true if the strengthener has been initialised. + */ + @SuppressWarnings("BooleanMethodIsAlwaysInverted") + boolean isInitialised(); + + /** + * Initialises the strengthener if necessary and returns a strong key + * derived from the given key. + */ + SecretKey strengthenKey(SecretKey k); +} diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/db/DatabaseConfig.java b/bramble-api/src/main/java/org/briarproject/bramble/api/db/DatabaseConfig.java index 36982595d..16d587c1d 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/db/DatabaseConfig.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/db/DatabaseConfig.java @@ -1,6 +1,6 @@ package org.briarproject.bramble.api.db; -import org.briarproject.bramble.api.crypto.KeyStoreConfig; +import org.briarproject.bramble.api.crypto.KeyStrengthener; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import java.io.File; @@ -21,9 +21,9 @@ public interface DatabaseConfig { File getDatabaseKeyDirectory(); /** - * Returns a {@link KeyStoreConfig} for strengthening the encryption of the - * database key, or null if no keystore should be used. + * Returns a {@link KeyStrengthener} for strengthening the encryption of + * the database key, or null if no strengthener should be used. */ @Nullable - KeyStoreConfig getKeyStoreConfig(); + KeyStrengthener getKeyStrengthener(); } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/account/AccountManagerImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/account/AccountManagerImpl.java index bec84eda6..c89353fa9 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/account/AccountManagerImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/account/AccountManagerImpl.java @@ -2,7 +2,7 @@ package org.briarproject.bramble.account; import org.briarproject.bramble.api.account.AccountManager; import org.briarproject.bramble.api.crypto.CryptoComponent; -import org.briarproject.bramble.api.crypto.KeyStoreConfig; +import org.briarproject.bramble.api.crypto.KeyStrengthener; import org.briarproject.bramble.api.crypto.SecretKey; import org.briarproject.bramble.api.db.DatabaseConfig; import org.briarproject.bramble.api.identity.Identity; @@ -178,7 +178,7 @@ class AccountManagerImpl implements AccountManager { private boolean encryptAndStoreDatabaseKey(SecretKey key, String password) { byte[] plaintext = key.getBytes(); byte[] ciphertext = crypto.encryptWithPassword(plaintext, password, - databaseConfig.getKeyStoreConfig()); + databaseConfig.getKeyStrengthener()); return storeEncryptedDatabaseKey(toHexString(ciphertext)); } @@ -211,19 +211,19 @@ class AccountManagerImpl implements AccountManager { return null; } byte[] ciphertext = fromHexString(hex); - KeyStoreConfig keyStoreConfig = databaseConfig.getKeyStoreConfig(); + KeyStrengthener keyStrengthener = databaseConfig.getKeyStrengthener(); byte[] plaintext = crypto.decryptWithPassword(ciphertext, password, - keyStoreConfig); + keyStrengthener); if (plaintext == null) { LOG.info("Failed to decrypt database key"); return null; } SecretKey key = new SecretKey(plaintext); - // If the DB key was encrypted without using a stored key and a stored - // key is now available, re-encrypt the DB key with the stored key - if (keyStoreConfig != null && - !crypto.isEncryptedWithStoredKey(ciphertext)) { - LOG.info("Re-encrypting database key with stored key"); + // If the DB key was encrypted with a weak key and a key strengthener + // is now available, re-encrypt the DB key with a strengthened key + if (keyStrengthener != null && + !crypto.isEncryptedWithStrengthenedKey(ciphertext)) { + LOG.info("Re-encrypting database key with strengthened key"); encryptAndStoreDatabaseKey(key, password); } return key; diff --git a/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java index d4a1785fa..6763193ac 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java @@ -9,7 +9,7 @@ import org.briarproject.bramble.api.crypto.AgreementPublicKey; import org.briarproject.bramble.api.crypto.CryptoComponent; import org.briarproject.bramble.api.crypto.KeyPair; import org.briarproject.bramble.api.crypto.KeyParser; -import org.briarproject.bramble.api.crypto.KeyStoreConfig; +import org.briarproject.bramble.api.crypto.KeyStrengthener; import org.briarproject.bramble.api.crypto.PrivateKey; import org.briarproject.bramble.api.crypto.PublicKey; import org.briarproject.bramble.api.crypto.SecretKey; @@ -25,28 +25,20 @@ import org.spongycastle.crypto.digests.Blake2bDigest; import org.whispersystems.curve25519.Curve25519; import org.whispersystems.curve25519.Curve25519KeyPair; -import java.io.IOException; import java.security.GeneralSecurityException; -import java.security.KeyStore; -import java.security.KeyStore.Entry; -import java.security.KeyStore.SecretKeyEntry; import java.security.NoSuchAlgorithmException; import java.security.Provider; import java.security.SecureRandom; import java.security.Security; -import java.security.spec.AlgorithmParameterSpec; import java.util.logging.Logger; import javax.annotation.Nullable; -import javax.crypto.KeyGenerator; -import javax.crypto.Mac; import javax.inject.Inject; import static java.lang.System.arraycopy; import static java.util.logging.Level.INFO; import static org.briarproject.bramble.api.crypto.CryptoConstants.KEY_TYPE_AGREEMENT; import static org.briarproject.bramble.api.crypto.CryptoConstants.KEY_TYPE_SIGNATURE; -import static org.briarproject.bramble.api.nullsafety.NullSafety.requireNonNull; import static org.briarproject.bramble.util.ByteUtils.INT_32_BYTES; import static org.briarproject.bramble.util.LogUtils.logDuration; import static org.briarproject.bramble.util.LogUtils.now; @@ -61,7 +53,7 @@ class CryptoComponentImpl implements CryptoComponent { private static final int STORAGE_IV_BYTES = 24; // 196 bits private static final int PBKDF_SALT_BYTES = 32; // 256 bits private static final byte PBKDF_FORMAT_SCRYPT = 0; - private static final byte PBKDF_FORMAT_SCRYPT_KEYSTORE = 1; + private static final byte PBKDF_FORMAT_SCRYPT_STRENGTHENED = 1; private final SecureRandom secureRandom; private final PasswordBasedKdf passwordBasedKdf; @@ -322,7 +314,7 @@ class CryptoComponentImpl implements CryptoComponent { @Override public byte[] encryptWithPassword(byte[] input, String password, - @Nullable KeyStoreConfig keyStoreConfig) { + @Nullable KeyStrengthener keyStrengthener) { AuthenticatedCipher cipher = new XSalsa20Poly1305AuthenticatedCipher(); int macBytes = cipher.getMacBytes(); // Generate a random salt @@ -332,8 +324,7 @@ class CryptoComponentImpl implements CryptoComponent { int cost = passwordBasedKdf.chooseCostParameter(); // Derive the encryption key from the password SecretKey key = passwordBasedKdf.deriveKey(password, salt, cost); - if (keyStoreConfig != null) - key = requireNonNull(deriveKey(key, keyStoreConfig, true)); + if (keyStrengthener != null) key = keyStrengthener.strengthenKey(key); // Generate a random IV byte[] iv = new byte[STORAGE_IV_BYTES]; secureRandom.nextBytes(iv); @@ -344,8 +335,8 @@ class CryptoComponentImpl implements CryptoComponent { byte[] output = new byte[outputLen]; int outputOff = 0; // Format version - byte formatVersion = keyStoreConfig == null - ? PBKDF_FORMAT_SCRYPT : PBKDF_FORMAT_SCRYPT_KEYSTORE; + byte formatVersion = keyStrengthener == null + ? PBKDF_FORMAT_SCRYPT : PBKDF_FORMAT_SCRYPT_STRENGTHENED; output[outputOff] = formatVersion; outputOff++; // Salt @@ -367,65 +358,10 @@ class CryptoComponentImpl implements CryptoComponent { } } - /** - * Derives a key from the given key and another key stored in a keystore. - * - * @param generateIfMissing Whether the stored key should be generated and - * stored if it doesn't already exist - * @return The derived key, or null if the stored key doesn't exist and - * {@code generateIfMissing} is false - */ - @Nullable - private SecretKey deriveKey(SecretKey key, KeyStoreConfig config, - boolean generateIfMissing) { - try { - // Load the keystore - KeyStore ks = KeyStore.getInstance(config.getKeyStoreType()); - ks.load(null); - // Load or generate the stored key - javax.crypto.SecretKey storedKey = null; - Entry e = ks.getEntry(config.getKeyAlias(), null); - if (e == null) { - if (!generateIfMissing) { - LOG.warning("Key not found in keystore"); - return null; - } - // Try the parameter specs in order of preference - for (AlgorithmParameterSpec spec : config.getParameterSpecs()) { - try { - KeyGenerator kg = KeyGenerator.getInstance( - config.getMacAlgorithmName(), - config.getProviderName()); - kg.init(spec); - storedKey = kg.generateKey(); - break; - } catch (Exception e1) { - if (LOG.isLoggable(INFO)) - LOG.info("Could not generate key: " + e1); - // Fall back to next spec - } - } - if (storedKey == null) throw new IllegalArgumentException(); - LOG.info("Stored key in keystore"); - } else { - if (!(e instanceof SecretKeyEntry)) - throw new IllegalArgumentException(); - storedKey = ((SecretKeyEntry) e).getSecretKey(); - LOG.info("Loaded key from keystore"); - } - // Use the input key and the stored key to derive the output key - Mac mac = Mac.getInstance(config.getMacAlgorithmName()); - mac.init(storedKey); - return new SecretKey(mac.doFinal(key.getBytes())); - } catch (GeneralSecurityException | IOException e) { - throw new IllegalArgumentException(e); - } - } - @Override @Nullable public byte[] decryptWithPassword(byte[] input, String password, - @Nullable KeyStoreConfig keyStoreConfig) { + @Nullable KeyStrengthener keyStrengthener) { AuthenticatedCipher cipher = new XSalsa20Poly1305AuthenticatedCipher(); int macBytes = cipher.getMacBytes(); // The input contains the format version, salt, cost parameter, IV, @@ -439,11 +375,9 @@ class CryptoComponentImpl implements CryptoComponent { inputOff++; // Check whether we support this format version if (formatVersion != PBKDF_FORMAT_SCRYPT && - formatVersion != PBKDF_FORMAT_SCRYPT_KEYSTORE) { + formatVersion != PBKDF_FORMAT_SCRYPT_STRENGTHENED) { return null; } - boolean useKeyStore = keyStoreConfig != null && - formatVersion == PBKDF_FORMAT_SCRYPT_KEYSTORE; // Salt byte[] salt = new byte[PBKDF_SALT_BYTES]; arraycopy(input, inputOff, salt, 0, salt.length); @@ -459,9 +393,10 @@ class CryptoComponentImpl implements CryptoComponent { inputOff += iv.length; // Derive the decryption key from the password SecretKey key = passwordBasedKdf.deriveKey(password, salt, (int) cost); - if (useKeyStore) { - key = deriveKey(key, keyStoreConfig, false); - if (key == null) return null; + if (formatVersion == PBKDF_FORMAT_SCRYPT_STRENGTHENED) { + if (keyStrengthener == null || !keyStrengthener.isInitialised()) + return null; // Can't derive the same strengthened key + key = keyStrengthener.strengthenKey(key); } // Initialise the cipher try { @@ -481,9 +416,9 @@ class CryptoComponentImpl implements CryptoComponent { } @Override - public boolean isEncryptedWithStoredKey(byte[] ciphertext) { + public boolean isEncryptedWithStrengthenedKey(byte[] ciphertext) { return ciphertext.length > 0 && - ciphertext[0] == PBKDF_FORMAT_SCRYPT_KEYSTORE; + ciphertext[0] == PBKDF_FORMAT_SCRYPT_STRENGTHENED; } @Override diff --git a/bramble-core/src/test/java/org/briarproject/bramble/account/AccountManagerImplTest.java b/bramble-core/src/test/java/org/briarproject/bramble/account/AccountManagerImplTest.java index d06bfa633..4b6b63e7e 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/account/AccountManagerImplTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/account/AccountManagerImplTest.java @@ -1,7 +1,7 @@ package org.briarproject.bramble.account; import org.briarproject.bramble.api.crypto.CryptoComponent; -import org.briarproject.bramble.api.crypto.KeyStoreConfig; +import org.briarproject.bramble.api.crypto.KeyStrengthener; import org.briarproject.bramble.api.crypto.SecretKey; import org.briarproject.bramble.api.db.DatabaseConfig; import org.briarproject.bramble.api.identity.Identity; @@ -40,8 +40,8 @@ public class AccountManagerImplTest extends BrambleMockTestCase { private final DatabaseConfig databaseConfig = context.mock(DatabaseConfig.class); - private final KeyStoreConfig keyStoreConfig = - context.mock(KeyStoreConfig.class); + private final KeyStrengthener keyStrengthener = + context.mock(KeyStrengthener.class); private final CryptoComponent crypto = context.mock(CryptoComponent.class); private final IdentityManager identityManager = context.mock(IdentityManager.class); @@ -71,8 +71,8 @@ public class AccountManagerImplTest extends BrambleMockTestCase { will(returnValue(dbDir)); allowing(databaseConfig).getDatabaseKeyDirectory(); will(returnValue(keyDir)); - allowing(databaseConfig).getKeyStoreConfig(); - will(returnValue(keyStoreConfig)); + allowing(databaseConfig).getKeyStrengthener(); + will(returnValue(keyStrengthener)); }}); accountManager = @@ -95,7 +95,7 @@ public class AccountManagerImplTest extends BrambleMockTestCase { public void testSignInReturnsFalseIfPasswordIsWrong() throws Exception { context.checking(new Expectations() {{ oneOf(crypto).decryptWithPassword(encryptedKey, password, - keyStoreConfig); + keyStrengthener); will(returnValue(null)); }}); @@ -116,9 +116,9 @@ public class AccountManagerImplTest extends BrambleMockTestCase { public void testSignInReturnsTrueIfPasswordIsRight() throws Exception { context.checking(new Expectations() {{ oneOf(crypto).decryptWithPassword(encryptedKey, password, - keyStoreConfig); + keyStrengthener); will(returnValue(key.getBytes())); - oneOf(crypto).isEncryptedWithStoredKey(encryptedKey); + oneOf(crypto).isEncryptedWithStrengthenedKey(encryptedKey); will(returnValue(true)); }}); @@ -142,12 +142,12 @@ public class AccountManagerImplTest extends BrambleMockTestCase { public void testSignInReEncryptsKey() throws Exception { context.checking(new Expectations() {{ oneOf(crypto).decryptWithPassword(encryptedKey, password, - keyStoreConfig); + keyStrengthener); will(returnValue(key.getBytes())); - oneOf(crypto).isEncryptedWithStoredKey(encryptedKey); + oneOf(crypto).isEncryptedWithStrengthenedKey(encryptedKey); will(returnValue(false)); oneOf(crypto).encryptWithPassword(key.getBytes(), password, - keyStoreConfig); + keyStrengthener); will(returnValue(newEncryptedKey)); }}); @@ -297,7 +297,7 @@ public class AccountManagerImplTest extends BrambleMockTestCase { oneOf(crypto).generateSecretKey(); will(returnValue(key)); oneOf(crypto).encryptWithPassword(key.getBytes(), password, - keyStoreConfig); + keyStrengthener); will(returnValue(encryptedKey)); }}); @@ -327,7 +327,7 @@ public class AccountManagerImplTest extends BrambleMockTestCase { throws Exception { context.checking(new Expectations() {{ oneOf(crypto).decryptWithPassword(encryptedKey, password, - keyStoreConfig); + keyStrengthener); will(returnValue(null)); }}); @@ -345,12 +345,12 @@ public class AccountManagerImplTest extends BrambleMockTestCase { throws Exception { context.checking(new Expectations() {{ oneOf(crypto).decryptWithPassword(encryptedKey, password, - keyStoreConfig); + keyStrengthener); will(returnValue(key.getBytes())); - oneOf(crypto).isEncryptedWithStoredKey(encryptedKey); + oneOf(crypto).isEncryptedWithStrengthenedKey(encryptedKey); will(returnValue(true)); oneOf(crypto).encryptWithPassword(key.getBytes(), newPassword, - keyStoreConfig); + keyStrengthener); will(returnValue(newEncryptedKey)); }}); diff --git a/bramble-core/src/test/java/org/briarproject/bramble/test/TestDatabaseConfig.java b/bramble-core/src/test/java/org/briarproject/bramble/test/TestDatabaseConfig.java index f285f1c80..ed4afd622 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/test/TestDatabaseConfig.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/test/TestDatabaseConfig.java @@ -1,6 +1,6 @@ package org.briarproject.bramble.test; -import org.briarproject.bramble.api.crypto.KeyStoreConfig; +import org.briarproject.bramble.api.crypto.KeyStrengthener; import org.briarproject.bramble.api.db.DatabaseConfig; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; @@ -30,7 +30,7 @@ public class TestDatabaseConfig implements DatabaseConfig { @Nullable @Override - public KeyStoreConfig getKeyStoreConfig() { + public KeyStrengthener getKeyStrengthener() { return null; } } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/AndroidDatabaseConfig.java b/briar-android/src/main/java/org/briarproject/briar/android/AndroidDatabaseConfig.java index c27cc2274..e13b1463a 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/AndroidDatabaseConfig.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/AndroidDatabaseConfig.java @@ -1,6 +1,6 @@ package org.briarproject.briar.android; -import org.briarproject.bramble.api.crypto.KeyStoreConfig; +import org.briarproject.bramble.api.crypto.KeyStrengthener; import org.briarproject.bramble.api.db.DatabaseConfig; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; @@ -8,19 +8,18 @@ import java.io.File; import javax.annotation.Nullable; -import static android.os.Build.VERSION.SDK_INT; - @NotNullByDefault class AndroidDatabaseConfig implements DatabaseConfig { private final File dbDir, keyDir; @Nullable - private final KeyStoreConfig keyStoreConfig; + private final KeyStrengthener keyStrengthener; - AndroidDatabaseConfig(File dbDir, File keyDir) { + AndroidDatabaseConfig(File dbDir, File keyDir, + @Nullable KeyStrengthener keyStrengthener) { this.dbDir = dbDir; this.keyDir = keyDir; - keyStoreConfig = SDK_INT >= 23 ? new AndroidKeyStoreConfig() : null; + this.keyStrengthener = keyStrengthener; } @Override @@ -35,7 +34,7 @@ class AndroidDatabaseConfig implements DatabaseConfig { @Nullable @Override - public KeyStoreConfig getKeyStoreConfig() { - return keyStoreConfig; + public KeyStrengthener getKeyStrengthener() { + return keyStrengthener; } } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStoreConfig.java b/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStoreConfig.java deleted file mode 100644 index 5f9b4b1e8..000000000 --- a/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStoreConfig.java +++ /dev/null @@ -1,66 +0,0 @@ -package org.briarproject.briar.android; - -import android.security.keystore.KeyGenParameterSpec; - -import org.briarproject.bramble.api.crypto.KeyStoreConfig; -import org.briarproject.bramble.api.nullsafety.NotNullByDefault; - -import java.security.spec.AlgorithmParameterSpec; -import java.util.List; - -import androidx.annotation.RequiresApi; - -import static android.os.Build.VERSION.SDK_INT; -import static android.security.keystore.KeyProperties.PURPOSE_SIGN; -import static java.util.Arrays.asList; -import static java.util.Collections.singletonList; - -@RequiresApi(23) -@NotNullByDefault -class AndroidKeyStoreConfig implements KeyStoreConfig { - - private final List specs; - - AndroidKeyStoreConfig() { - KeyGenParameterSpec noStrongBox = - new KeyGenParameterSpec.Builder("db", PURPOSE_SIGN) - .setKeySize(256) - .build(); - if (SDK_INT >= 28) { - // Prefer StrongBox if available - KeyGenParameterSpec strongBox = - new KeyGenParameterSpec.Builder("db", PURPOSE_SIGN) - .setIsStrongBoxBacked(true) - .setKeySize(256) - .build(); - specs = asList(strongBox, noStrongBox); - } else { - specs = singletonList(noStrongBox); - } - } - - @Override - public String getKeyStoreType() { - return "AndroidKeyStore"; - } - - @Override - public String getKeyAlias() { - return "db"; - } - - @Override - public String getProviderName() { - return "AndroidKeyStore"; - } - - @Override - public String getMacAlgorithmName() { - return "HmacSHA256"; - } - - @Override - public List getParameterSpecs() { - return specs; - } -} diff --git a/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStrengthener.java b/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStrengthener.java new file mode 100644 index 000000000..db44062f9 --- /dev/null +++ b/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStrengthener.java @@ -0,0 +1,118 @@ +package org.briarproject.briar.android; + +import android.security.keystore.KeyGenParameterSpec; + +import org.briarproject.bramble.api.crypto.KeyStrengthener; +import org.briarproject.bramble.api.crypto.SecretKey; +import org.briarproject.bramble.api.nullsafety.NotNullByDefault; + +import java.io.IOException; +import java.security.GeneralSecurityException; +import java.security.KeyStore; +import java.security.KeyStore.Entry; +import java.security.KeyStore.SecretKeyEntry; +import java.security.spec.AlgorithmParameterSpec; +import java.util.List; +import java.util.logging.Logger; + +import javax.annotation.Nullable; +import javax.annotation.concurrent.GuardedBy; +import javax.crypto.KeyGenerator; +import javax.crypto.Mac; + +import androidx.annotation.RequiresApi; + +import static android.os.Build.VERSION.SDK_INT; +import static android.security.keystore.KeyProperties.KEY_ALGORITHM_HMAC_SHA256; +import static android.security.keystore.KeyProperties.PURPOSE_SIGN; +import static java.util.Arrays.asList; +import static java.util.Collections.singletonList; +import static java.util.logging.Level.INFO; +import static java.util.logging.Logger.getLogger; + +@RequiresApi(23) +@NotNullByDefault +class AndroidKeyStrengthener implements KeyStrengthener { + + private static final Logger LOG = + getLogger(AndroidKeyStrengthener.class.getName()); + + private static final String KEY_STORE_TYPE = "AndroidKeyStore"; + private static final String PROVIDER_NAME = "AndroidKeyStore"; + private static final String KEY_ALIAS = "db"; + private static final int KEY_BITS = 256; + + private final List specs; + + AndroidKeyStrengthener() { + KeyGenParameterSpec noStrongBox = + new KeyGenParameterSpec.Builder(KEY_ALIAS, PURPOSE_SIGN) + .setKeySize(KEY_BITS) + .build(); + if (SDK_INT >= 28) { + // Prefer StrongBox if available + KeyGenParameterSpec strongBox = + new KeyGenParameterSpec.Builder(KEY_ALIAS, PURPOSE_SIGN) + .setIsStrongBoxBacked(true) + .setKeySize(KEY_BITS) + .build(); + specs = asList(strongBox, noStrongBox); + } else { + specs = singletonList(noStrongBox); + } + } + + @GuardedBy("this") + @Nullable + private javax.crypto.SecretKey storedKey = null; + + @Override + public synchronized boolean isInitialised() { + if (storedKey != null) return true; + try { + KeyStore ks = KeyStore.getInstance(KEY_STORE_TYPE); + ks.load(null); + Entry entry = ks.getEntry(KEY_ALIAS, null); + if (entry instanceof SecretKeyEntry) { + storedKey = ((SecretKeyEntry) entry).getSecretKey(); + LOG.info("Loaded key from keystore"); + return true; + } + return false; + } catch (GeneralSecurityException | IOException e) { + throw new RuntimeException(e); + } + } + + @Override + public synchronized SecretKey strengthenKey(SecretKey k) { + try { + if (!isInitialised()) initialise(); + // Use the input key and the stored key to derive the output key + Mac mac = Mac.getInstance(KEY_ALGORITHM_HMAC_SHA256); + mac.init(storedKey); + return new SecretKey(mac.doFinal(k.getBytes())); + } catch (GeneralSecurityException e) { + throw new RuntimeException(e); + } + } + + private synchronized void initialise() throws GeneralSecurityException { + // Try the parameter specs in order of preference + for (AlgorithmParameterSpec spec : specs) { + try { + KeyGenerator kg = KeyGenerator.getInstance( + KEY_ALGORITHM_HMAC_SHA256, PROVIDER_NAME); + kg.init(spec); + storedKey = kg.generateKey(); + LOG.info("Stored key in keystore"); + return; + } catch (Exception e) { + if (LOG.isLoggable(INFO)) + LOG.info("Could not generate key: " + e); + // Fall back to next spec + } + } + throw new GeneralSecurityException("Could not generate key"); + } +} diff --git a/briar-android/src/main/java/org/briarproject/briar/android/AppModule.java b/briar-android/src/main/java/org/briarproject/briar/android/AppModule.java index 5da45a9c5..41c728e5e 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/AppModule.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/AppModule.java @@ -10,6 +10,7 @@ import com.vanniktech.emoji.RecentEmoji; import org.briarproject.bramble.api.FeatureFlags; import org.briarproject.bramble.api.battery.BatteryManager; import org.briarproject.bramble.api.crypto.CryptoComponent; +import org.briarproject.bramble.api.crypto.KeyStrengthener; import org.briarproject.bramble.api.crypto.PublicKey; import org.briarproject.bramble.api.db.DatabaseConfig; import org.briarproject.bramble.api.event.EventBus; @@ -56,6 +57,7 @@ import dagger.Module; import dagger.Provides; import static android.content.Context.MODE_PRIVATE; +import static android.os.Build.VERSION.SDK_INT; import static java.util.Arrays.asList; import static java.util.Collections.emptyList; import static org.briarproject.bramble.api.reporting.ReportingConstants.DEV_ONION_ADDRESS; @@ -101,7 +103,9 @@ public class AppModule { File dbDir = app.getApplicationContext().getDir("db", MODE_PRIVATE); File keyDir = app.getApplicationContext().getDir("key", MODE_PRIVATE); StrictMode.setThreadPolicy(tp); - return new AndroidDatabaseConfig(dbDir, keyDir); + KeyStrengthener keyStrengthener = SDK_INT >= 23 + ? new AndroidKeyStrengthener() : null; + return new AndroidDatabaseConfig(dbDir, keyDir, keyStrengthener); } @Provides diff --git a/briar-headless/src/main/java/org/briarproject/briar/headless/HeadlessDatabaseConfig.kt b/briar-headless/src/main/java/org/briarproject/briar/headless/HeadlessDatabaseConfig.kt index 6dca9ae10..620bdc826 100644 --- a/briar-headless/src/main/java/org/briarproject/briar/headless/HeadlessDatabaseConfig.kt +++ b/briar-headless/src/main/java/org/briarproject/briar/headless/HeadlessDatabaseConfig.kt @@ -1,6 +1,6 @@ package org.briarproject.briar.headless -import org.briarproject.bramble.api.crypto.KeyStoreConfig +import org.briarproject.bramble.api.crypto.KeyStrengthener import org.briarproject.bramble.api.db.DatabaseConfig import java.io.File @@ -11,5 +11,5 @@ internal class HeadlessDatabaseConfig(private val dbDir: File, private val keyDi override fun getDatabaseKeyDirectory() = keyDir - override fun getKeyStoreConfig(): KeyStoreConfig? = null + override fun getKeyStrengthener(): KeyStrengthener? = null }