From 0122282a72511324bdf6654b33d523db0a972f2d Mon Sep 17 00:00:00 2001 From: akwizgran Date: Mon, 24 Feb 2020 10:09:38 +0000 Subject: [PATCH] Migrate away from hardware-backed keys until keymaster issues are fixed. --- .../bramble/api/crypto/KeyStrengthener.java | 3 +++ .../bramble/account/AccountManagerImpl.java | 23 ++++++++++--------- .../account/AccountManagerImplTest.java | 14 ++++------- 3 files changed, 20 insertions(+), 20 deletions(-) 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 index 5913aae5d..d558f4d6a 100644 --- 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 @@ -5,6 +5,9 @@ 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. + * + * TODO: Remove after a reasonable migration period unless we can work around + * Android keymaster bugs. Added 2020-02-24 */ @NotNullByDefault public interface KeyStrengthener { 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 4636dfad3..8677c4f89 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 @@ -179,8 +179,9 @@ class AccountManagerImpl implements AccountManager { @GuardedBy("stateChangeLock") private boolean encryptAndStoreDatabaseKey(SecretKey key, String password) { byte[] plaintext = key.getBytes(); - byte[] ciphertext = crypto.encryptWithPassword(plaintext, password, - databaseConfig.getKeyStrengthener()); + // Don't use a key strengthener as the Android keymaster isn't reliable + byte[] ciphertext = + crypto.encryptWithPassword(plaintext, password, null); return storeEncryptedDatabaseKey(toHexString(ciphertext)); } @@ -197,13 +198,13 @@ class AccountManagerImpl implements AccountManager { @Override public void signIn(String password) throws DecryptionException { synchronized (stateChangeLock) { - databaseKey = loadAndDecryptDatabaseKey(password); + databaseKey = loadAndDecryptDatabaseKey(password, false); } } @GuardedBy("stateChangeLock") - private SecretKey loadAndDecryptDatabaseKey(String password) - throws DecryptionException { + private SecretKey loadAndDecryptDatabaseKey(String password, + boolean changing) throws DecryptionException { String hex = loadEncryptedDatabaseKey(); if (hex == null) { LOG.warning("Failed to load encrypted database key"); @@ -214,11 +215,11 @@ class AccountManagerImpl implements AccountManager { byte[] plaintext = crypto.decryptWithPassword(ciphertext, password, keyStrengthener); SecretKey key = new SecretKey(plaintext); - // 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"); + // If the DB key was encrypted with a hardware-backed key, re-encrypt + // it without the hardware-backed key so keymaster bugs don't delete + // the user's account + if (!changing && crypto.isEncryptedWithStrengthenedKey(ciphertext)) { + LOG.info("Re-encrypting database key without strengthened key"); encryptAndStoreDatabaseKey(key, password); } return key; @@ -228,7 +229,7 @@ class AccountManagerImpl implements AccountManager { public void changePassword(String oldPassword, String newPassword) throws DecryptionException { synchronized (stateChangeLock) { - SecretKey key = loadAndDecryptDatabaseKey(oldPassword); + SecretKey key = loadAndDecryptDatabaseKey(oldPassword, true); encryptAndStoreDatabaseKey(key, newPassword); } } 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 e5c345e53..8be3d70f8 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 @@ -134,7 +134,7 @@ public class AccountManagerImplTest extends BrambleMockTestCase { keyStrengthener); will(returnValue(key.getBytes())); oneOf(crypto).isEncryptedWithStrengthenedKey(encryptedKey); - will(returnValue(true)); + will(returnValue(false)); }}); storeDatabaseKey(keyFile, encryptedKeyHex); @@ -160,9 +160,8 @@ public class AccountManagerImplTest extends BrambleMockTestCase { keyStrengthener); will(returnValue(key.getBytes())); oneOf(crypto).isEncryptedWithStrengthenedKey(encryptedKey); - will(returnValue(false)); - oneOf(crypto).encryptWithPassword(key.getBytes(), password, - keyStrengthener); + will(returnValue(true)); + oneOf(crypto).encryptWithPassword(key.getBytes(), password, null); will(returnValue(newEncryptedKey)); }}); @@ -262,8 +261,7 @@ public class AccountManagerImplTest extends BrambleMockTestCase { oneOf(identityManager).registerIdentity(identity); oneOf(crypto).generateSecretKey(); will(returnValue(key)); - oneOf(crypto).encryptWithPassword(key.getBytes(), password, - keyStrengthener); + oneOf(crypto).encryptWithPassword(key.getBytes(), password, null); will(returnValue(encryptedKey)); }}); @@ -323,10 +321,8 @@ public class AccountManagerImplTest extends BrambleMockTestCase { oneOf(crypto).decryptWithPassword(encryptedKey, password, keyStrengthener); will(returnValue(key.getBytes())); - oneOf(crypto).isEncryptedWithStrengthenedKey(encryptedKey); - will(returnValue(true)); oneOf(crypto).encryptWithPassword(key.getBytes(), newPassword, - keyStrengthener); + null); will(returnValue(newEncryptedKey)); }});