Merge branch '1696-keystore-crash' into 'master'

Show a dialog instead of crashing if a hardware-backed key can't be loaded

Closes #1696

See merge request briar/briar!1233

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.DecryptionException;
 import org.briarproject.bramble.api.crypto.KeyStrengthener;
 import org.briarproject.bramble.api.crypto.SecretKey;
 import org.briarproject.bramble.api.db.DatabaseConfig;
@@ -17,6 +18,7 @@ import java.io.FileInputStream;
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStreamReader;
+import java.nio.charset.Charset;
 import java.util.logging.Logger;
 
 import javax.annotation.Nullable;
@@ -24,6 +26,7 @@ import javax.annotation.concurrent.GuardedBy;
 import javax.inject.Inject;
 
 import static java.util.logging.Level.WARNING;
+import static org.briarproject.bramble.api.crypto.DecryptionResult.INVALID_CIPHERTEXT;
 import static org.briarproject.bramble.util.LogUtils.logException;
 import static org.briarproject.bramble.util.StringUtils.fromHexString;
 import static org.briarproject.bramble.util.StringUtils.toHexString;
@@ -95,7 +98,7 @@ class AccountManagerImpl implements AccountManager {
 		}
 		try {
 			BufferedReader reader = new BufferedReader(new InputStreamReader(
-					new FileInputStream(f), "UTF-8"));
+					new FileInputStream(f), Charset.forName("UTF-8")));
 			String key = reader.readLine();
 			reader.close();
 			return key;
@@ -147,7 +150,7 @@ class AccountManagerImpl implements AccountManager {
 	@GuardedBy("stateChangeLock")
 	private void writeDbKeyToFile(String key, File f) throws IOException {
 		FileOutputStream out = new FileOutputStream(f);
-		out.write(key.getBytes("UTF-8"));
+		out.write(key.getBytes(Charset.forName("UTF-8")));
 		out.flush();
 		out.close();
 	}
@@ -192,31 +195,24 @@ class AccountManagerImpl implements AccountManager {
 	}
 
 	@Override
-	public boolean signIn(String password) {
+	public void signIn(String password) throws DecryptionException {
 		synchronized (stateChangeLock) {
-			SecretKey key = loadAndDecryptDatabaseKey(password);
-			if (key == null) return false;
-			databaseKey = key;
-			return true;
+			databaseKey = loadAndDecryptDatabaseKey(password);
 		}
 	}
 
 	@GuardedBy("stateChangeLock")
-	@Nullable
-	private SecretKey loadAndDecryptDatabaseKey(String password) {
+	private SecretKey loadAndDecryptDatabaseKey(String password)
+			throws DecryptionException {
 		String hex = loadEncryptedDatabaseKey();
 		if (hex == null) {
 			LOG.warning("Failed to load encrypted database key");
-			return null;
+			throw new DecryptionException(INVALID_CIPHERTEXT);
 		}
 		byte[] ciphertext = fromHexString(hex);
 		KeyStrengthener keyStrengthener = databaseConfig.getKeyStrengthener();
 		byte[] plaintext = crypto.decryptWithPassword(ciphertext, password,
 				keyStrengthener);
-		if (plaintext == null) {
-			LOG.info("Failed to decrypt database key");
-			return null;
-		}
 		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
@@ -229,10 +225,11 @@ class AccountManagerImpl implements AccountManager {
 	}
 
 	@Override
-	public boolean changePassword(String oldPassword, String newPassword) {
+	public void changePassword(String oldPassword, String newPassword)
+			throws DecryptionException {
 		synchronized (stateChangeLock) {
 			SecretKey key = loadAndDecryptDatabaseKey(oldPassword);
-			return key != null && encryptAndStoreDatabaseKey(key, newPassword);
+			encryptAndStoreDatabaseKey(key, newPassword);
 		}
 	}
 }

bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java

@@ -7,6 +7,7 @@ import net.i2p.crypto.eddsa.KeyPairGenerator;
 import org.briarproject.bramble.api.crypto.AgreementPrivateKey;
 import org.briarproject.bramble.api.crypto.AgreementPublicKey;
 import org.briarproject.bramble.api.crypto.CryptoComponent;
+import org.briarproject.bramble.api.crypto.DecryptionException;
 import org.briarproject.bramble.api.crypto.KeyPair;
 import org.briarproject.bramble.api.crypto.KeyParser;
 import org.briarproject.bramble.api.crypto.KeyStrengthener;
@@ -39,6 +40,9 @@ 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.crypto.DecryptionResult.INVALID_CIPHERTEXT;
+import static org.briarproject.bramble.api.crypto.DecryptionResult.INVALID_PASSWORD;
+import static org.briarproject.bramble.api.crypto.DecryptionResult.KEY_STRENGTHENER_ERROR;
 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;
@@ -359,16 +363,17 @@ class CryptoComponentImpl implements CryptoComponent {
 	}
 
 	@Override
-	@Nullable
 	public byte[] decryptWithPassword(byte[] input, String password,
-			@Nullable KeyStrengthener keyStrengthener) {
+			@Nullable KeyStrengthener keyStrengthener)
+			throws DecryptionException {
 		AuthenticatedCipher cipher = new XSalsa20Poly1305AuthenticatedCipher();
 		int macBytes = cipher.getMacBytes();
 		// The input contains the format version, salt, cost parameter, IV,
 		// ciphertext and MAC
 		if (input.length < 1 + PBKDF_SALT_BYTES + INT_32_BYTES
-				+ STORAGE_IV_BYTES + macBytes)
-			return null; // Invalid input
+				+ STORAGE_IV_BYTES + macBytes) {
+			throw new DecryptionException(INVALID_CIPHERTEXT);
+		}
 		int inputOff = 0;
 		// Format version
 		byte formatVersion = input[inputOff];
@@ -376,7 +381,7 @@ class CryptoComponentImpl implements CryptoComponent {
 		// Check whether we support this format version
 		if (formatVersion != PBKDF_FORMAT_SCRYPT &&
 				formatVersion != PBKDF_FORMAT_SCRYPT_STRENGTHENED) {
-			return null;
+			throw new DecryptionException(INVALID_CIPHERTEXT);
 		}
 		// Salt
 		byte[] salt = new byte[PBKDF_SALT_BYTES];
@@ -385,8 +390,9 @@ class CryptoComponentImpl implements CryptoComponent {
 		// Cost parameter
 		long cost = ByteUtils.readUint32(input, inputOff);
 		inputOff += INT_32_BYTES;
-		if (cost < 2 || cost > Integer.MAX_VALUE)
-			return null; // Invalid cost parameter
+		if (cost < 2 || cost > Integer.MAX_VALUE) {
+			throw new DecryptionException(INVALID_CIPHERTEXT);
+		}
 		// IV
 		byte[] iv = new byte[STORAGE_IV_BYTES];
 		arraycopy(input, inputOff, iv, 0, iv.length);
@@ -394,8 +400,10 @@ class CryptoComponentImpl implements CryptoComponent {
 		// Derive the decryption key from the password
 		SecretKey key = passwordBasedKdf.deriveKey(password, salt, (int) cost);
 		if (formatVersion == PBKDF_FORMAT_SCRYPT_STRENGTHENED) {
-			if (keyStrengthener == null || !keyStrengthener.isInitialised())
-				return null; // Can't derive the same strengthened key
+			if (keyStrengthener == null || !keyStrengthener.isInitialised()) {
+				// Can't derive the same strengthened key
+				throw new DecryptionException(KEY_STRENGTHENER_ERROR);
+			}
 			key = keyStrengthener.strengthenKey(key);
 		}
 		// Initialise the cipher
@@ -411,7 +419,7 @@ class CryptoComponentImpl implements CryptoComponent {
 			cipher.process(input, inputOff, inputLen, output, 0);
 			return output;
 		} catch (GeneralSecurityException e) {
-			return null; // Invalid ciphertext
+			throw new DecryptionException(INVALID_PASSWORD);
 		}
 	}
 

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.DecryptionException;
 import org.briarproject.bramble.api.crypto.KeyStrengthener;
 import org.briarproject.bramble.api.crypto.SecretKey;
 import org.briarproject.bramble.api.db.DatabaseConfig;
@@ -19,12 +20,15 @@ import java.io.FileInputStream;
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStreamReader;
+import java.nio.charset.Charset;
 
 import javax.annotation.Nullable;
 
 import static junit.framework.Assert.assertFalse;
 import static junit.framework.Assert.assertNull;
 import static junit.framework.Assert.assertTrue;
+import static org.briarproject.bramble.api.crypto.DecryptionResult.INVALID_CIPHERTEXT;
+import static org.briarproject.bramble.api.crypto.DecryptionResult.INVALID_PASSWORD;
 import static org.briarproject.bramble.test.TestUtils.deleteTestDirectory;
 import static org.briarproject.bramble.test.TestUtils.getIdentity;
 import static org.briarproject.bramble.test.TestUtils.getRandomBytes;
@@ -35,6 +39,7 @@ import static org.briarproject.bramble.util.StringUtils.toHexString;
 import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.fail;
 
 public class AccountManagerImplTest extends BrambleMockTestCase {
 
@@ -83,8 +88,13 @@ public class AccountManagerImplTest extends BrambleMockTestCase {
 	}
 
 	@Test
-	public void testSignInReturnsFalseIfDbKeyCannotBeLoaded() {
-		assertFalse(accountManager.signIn(password));
+	public void testSignInThrowsExceptionIfDbKeyCannotBeLoaded() {
+		try {
+			accountManager.signIn(password);
+			fail();
+		} catch (DecryptionException expected) {
+			assertEquals(INVALID_CIPHERTEXT, expected.getDecryptionResult());
+		}
 		assertFalse(accountManager.hasDatabaseKey());
 
 		assertFalse(keyFile.exists());
@@ -92,11 +102,11 @@ public class AccountManagerImplTest extends BrambleMockTestCase {
 	}
 
 	@Test
-	public void testSignInReturnsFalseIfPasswordIsWrong() throws Exception {
+	public void testSignInThrowsExceptionIfPasswordIsWrong() throws Exception {
 		context.checking(new Expectations() {{
 			oneOf(crypto).decryptWithPassword(encryptedKey, password,
 					keyStrengthener);
-			will(returnValue(null));
+			will(throwException(new DecryptionException(INVALID_PASSWORD)));
 		}});
 
 		storeDatabaseKey(keyFile, encryptedKeyHex);
@@ -105,7 +115,12 @@ public class AccountManagerImplTest extends BrambleMockTestCase {
 		assertEquals(encryptedKeyHex, loadDatabaseKey(keyFile));
 		assertEquals(encryptedKeyHex, loadDatabaseKey(keyBackupFile));
 
-		assertFalse(accountManager.signIn(password));
+		try {
+			accountManager.signIn(password);
+			fail();
+		} catch (DecryptionException expected) {
+			assertEquals(INVALID_PASSWORD, expected.getDecryptionResult());
+		}
 		assertFalse(accountManager.hasDatabaseKey());
 
 		assertEquals(encryptedKeyHex, loadDatabaseKey(keyFile));
@@ -128,7 +143,7 @@ public class AccountManagerImplTest extends BrambleMockTestCase {
 		assertEquals(encryptedKeyHex, loadDatabaseKey(keyFile));
 		assertEquals(encryptedKeyHex, loadDatabaseKey(keyBackupFile));
 
-		assertTrue(accountManager.signIn(password));
+		accountManager.signIn(password);
 		assertTrue(accountManager.hasDatabaseKey());
 		SecretKey decrypted = accountManager.getDatabaseKey();
 		assertNotNull(decrypted);
@@ -157,7 +172,7 @@ public class AccountManagerImplTest extends BrambleMockTestCase {
 		assertEquals(encryptedKeyHex, loadDatabaseKey(keyFile));
 		assertEquals(encryptedKeyHex, loadDatabaseKey(keyBackupFile));
 
-		assertTrue(accountManager.signIn(password));
+		accountManager.signIn(password);
 		assertTrue(accountManager.hasDatabaseKey());
 		SecretKey decrypted = accountManager.getDatabaseKey();
 		assertNotNull(decrypted);
@@ -266,26 +281,36 @@ public class AccountManagerImplTest extends BrambleMockTestCase {
 	}
 
 	@Test
-	public void testChangePasswordReturnsFalseIfDbKeyCannotBeLoaded() {
-		assertFalse(accountManager.changePassword(password, newPassword));
+	public void testChangePasswordThrowsExceptionIfDbKeyCannotBeLoaded() {
+		try {
+			accountManager.changePassword(password, newPassword);
+			fail();
+		} catch (DecryptionException expected) {
+			assertEquals(INVALID_CIPHERTEXT, expected.getDecryptionResult());
+		}
 
 		assertFalse(keyFile.exists());
 		assertFalse(keyBackupFile.exists());
 	}
 
 	@Test
-	public void testChangePasswordReturnsFalseIfPasswordIsWrong()
+	public void testChangePasswordThrowsExceptionIfPasswordIsWrong()
 			throws Exception {
 		context.checking(new Expectations() {{
 			oneOf(crypto).decryptWithPassword(encryptedKey, password,
 					keyStrengthener);
-			will(returnValue(null));
+			will(throwException(new DecryptionException(INVALID_PASSWORD)));
 		}});
 
 		storeDatabaseKey(keyFile, encryptedKeyHex);
 		storeDatabaseKey(keyBackupFile, encryptedKeyHex);
 
-		assertFalse(accountManager.changePassword(password, newPassword));
+		try {
+			accountManager.changePassword(password, newPassword);
+			fail();
+		} catch (DecryptionException expected) {
+			assertEquals(INVALID_PASSWORD, expected.getDecryptionResult());
+		}
 
 		assertEquals(encryptedKeyHex, loadDatabaseKey(keyFile));
 		assertEquals(encryptedKeyHex, loadDatabaseKey(keyBackupFile));
@@ -308,7 +333,7 @@ public class AccountManagerImplTest extends BrambleMockTestCase {
 		storeDatabaseKey(keyFile, encryptedKeyHex);
 		storeDatabaseKey(keyBackupFile, encryptedKeyHex);
 
-		assertTrue(accountManager.changePassword(password, newPassword));
+		accountManager.changePassword(password, newPassword);
 
 		assertEquals(newEncryptedKeyHex, loadDatabaseKey(keyFile));
 		assertEquals(newEncryptedKeyHex, loadDatabaseKey(keyBackupFile));
@@ -317,7 +342,7 @@ public class AccountManagerImplTest extends BrambleMockTestCase {
 	private void storeDatabaseKey(File f, String hex) throws IOException {
 		f.getParentFile().mkdirs();
 		FileOutputStream out = new FileOutputStream(f);
-		out.write(hex.getBytes("UTF-8"));
+		out.write(hex.getBytes(Charset.forName("UTF-8")));
 		out.flush();
 		out.close();
 	}
@@ -325,7 +350,7 @@ public class AccountManagerImplTest extends BrambleMockTestCase {
 	@Nullable
 	private String loadDatabaseKey(File f) throws IOException {
 		BufferedReader reader = new BufferedReader(new InputStreamReader(
-				new FileInputStream(f), "UTF-8"));
+				new FileInputStream(f), Charset.forName("UTF-8")));
 		String hex = reader.readLine();
 		reader.close();
 		return hex;

bramble-core/src/test/java/org/briarproject/bramble/crypto/PasswordBasedEncryptionTest.java

@@ -1,25 +1,35 @@
 package org.briarproject.bramble.crypto;
 
+import org.briarproject.bramble.api.crypto.DecryptionException;
+import org.briarproject.bramble.api.crypto.KeyStrengthener;
+import org.briarproject.bramble.api.crypto.SecretKey;
 import org.briarproject.bramble.system.SystemClock;
-import org.briarproject.bramble.test.BrambleTestCase;
+import org.briarproject.bramble.test.BrambleMockTestCase;
 import org.briarproject.bramble.test.TestSecureRandomProvider;
-import org.briarproject.bramble.test.TestUtils;
+import org.jmock.Expectations;
 import org.junit.Test;
 
-import java.util.Random;
-
+import static org.briarproject.bramble.api.crypto.DecryptionResult.INVALID_CIPHERTEXT;
+import static org.briarproject.bramble.api.crypto.DecryptionResult.INVALID_PASSWORD;
+import static org.briarproject.bramble.api.crypto.DecryptionResult.KEY_STRENGTHENER_ERROR;
+import static org.briarproject.bramble.test.TestUtils.getRandomBytes;
+import static org.briarproject.bramble.test.TestUtils.getSecretKey;
 import static org.junit.Assert.assertArrayEquals;
-import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
 
-public class PasswordBasedEncryptionTest extends BrambleTestCase {
+public class PasswordBasedEncryptionTest extends BrambleMockTestCase {
+
+	private final KeyStrengthener keyStrengthener =
+			context.mock(KeyStrengthener.class);
 
 	private final CryptoComponentImpl crypto =
 			new CryptoComponentImpl(new TestSecureRandomProvider(),
 					new ScryptKdf(new SystemClock()));
 
 	@Test
-	public void testEncryptionAndDecryption() {
-		byte[] input = TestUtils.getRandomBytes(1234);
+	public void testEncryptionAndDecryption() throws Exception {
+		byte[] input = getRandomBytes(1234);
 		String password = "password";
 		byte[] ciphertext = crypto.encryptWithPassword(input, password, null);
 		byte[] output = crypto.decryptWithPassword(ciphertext, password, null);
@@ -27,14 +37,80 @@ public class PasswordBasedEncryptionTest extends BrambleTestCase {
 	}
 
 	@Test
-	public void testInvalidCiphertextReturnsNull() {
-		byte[] input = TestUtils.getRandomBytes(1234);
+	public void testInvalidFormatVersionThrowsException() {
+		byte[] input = getRandomBytes(1234);
 		String password = "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, null);
-		assertNull(output);
+
+		// Modify the format version
+		ciphertext[0] ^= (byte) 0xFF;
+		try {
+			crypto.decryptWithPassword(ciphertext, password, null);
+			fail();
+		} catch (DecryptionException expected) {
+			assertEquals(INVALID_CIPHERTEXT, expected.getDecryptionResult());
+		}
+	}
+
+	@Test
+	public void testInvalidPasswordThrowsException() {
+		byte[] input = getRandomBytes(1234);
+		byte[] ciphertext = crypto.encryptWithPassword(input, "password", null);
+
+		// Try to decrypt with the wrong password
+		try {
+			crypto.decryptWithPassword(ciphertext, "wrong", null);
+			fail();
+		} catch (DecryptionException expected) {
+			assertEquals(INVALID_PASSWORD, expected.getDecryptionResult());
+		}
+	}
+
+	@Test
+	public void testMissingKeyStrengthenerThrowsException() {
+		SecretKey strengthened = getSecretKey();
+		context.checking(new Expectations() {{
+			oneOf(keyStrengthener).strengthenKey(with(any(SecretKey.class)));
+			will(returnValue(strengthened));
+		}});
+
+		// Use the key strengthener during encryption
+		byte[] input = getRandomBytes(1234);
+		String password = "password";
+		byte[] ciphertext =
+				crypto.encryptWithPassword(input, password, keyStrengthener);
+
+		// The key strengthener is missing during decryption
+		try {
+			crypto.decryptWithPassword(ciphertext, password, null);
+			fail();
+		} catch (DecryptionException expected) {
+			assertEquals(KEY_STRENGTHENER_ERROR, expected.getDecryptionResult());
+		}
+	}
+
+	@Test
+	public void testKeyStrengthenerFailureThrowsException() {
+		SecretKey strengthened = getSecretKey();
+		context.checking(new Expectations() {{
+			oneOf(keyStrengthener).strengthenKey(with(any(SecretKey.class)));
+			will(returnValue(strengthened));
+			oneOf(keyStrengthener).isInitialised();
+			will(returnValue(false));
+		}});
+
+		// Use the key strengthener during encryption
+		byte[] input = getRandomBytes(1234);
+		String password = "password";
+		byte[] ciphertext =
+				crypto.encryptWithPassword(input, password, keyStrengthener);
+
+		// The key strengthener fails during decryption
+		try {
+			crypto.decryptWithPassword(ciphertext, password, keyStrengthener);
+			fail();
+		} catch (DecryptionException expected) {
+			assertEquals(KEY_STRENGTHENER_ERROR, expected.getDecryptionResult());
+		}
 	}
 }