From 8c658ac93069b42c8db1e7cbe0da9c5882a7d5b1 Mon Sep 17 00:00:00 2001 From: Alexander Bakker Date: Sun, 26 May 2019 23:52:20 +0200 Subject: [PATCH] Use the old encode method for passwords over 64 bytes and repair the slot (#98) Commit afb9e597110014117722fc8353333b18d3492007 fixed a bug where the password encode function would add null bytes to the end of the output. Luckily (I thought), PBKDF2 produces collisions for inputs with trailing null bytes and thus scrypt does this as well, so we could safely change that function to remove the null bytes without any impact. Unfortunately, that doesn't hold up if the password is over 64 bytes in size. So after that change, the KDF started producing different keys than before for such passwords and thus some users could no longer unlock their vault. This patch addresses the issue by using the old password encode function for passwords over 64 bytes and repairing the affected password slot. --- .../aegis/crypto/CryptoUtils.java | 7 + .../aegis/db/slots/PasswordSlot.java | 24 +++- .../beemdevelopment/aegis/db/slots/Slot.java | 3 +- .../aegis/db/slots/SlotList.java | 23 ++++ .../aegis/ui/AuthActivity.java | 25 ++-- .../aegis/ui/MainActivity.java | 19 +-- .../aegis/ui/tasks/SlotListTask.java | 126 +++++++++++++----- app/src/main/res/values/strings.xml | 1 + .../com/beemdevelopment/aegis/SCryptTest.java | 23 ++-- 9 files changed, 184 insertions(+), 67 deletions(-) diff --git a/app/src/main/java/com/beemdevelopment/aegis/crypto/CryptoUtils.java b/app/src/main/java/com/beemdevelopment/aegis/crypto/CryptoUtils.java index acc09667..3a32ebdd 100644 --- a/app/src/main/java/com/beemdevelopment/aegis/crypto/CryptoUtils.java +++ b/app/src/main/java/com/beemdevelopment/aegis/crypto/CryptoUtils.java @@ -137,4 +137,11 @@ public class CryptoUtils { byteBuf.get(bytes); return bytes; } + + @Deprecated + public static byte[] toBytesOld(char[] chars) { + CharBuffer charBuf = CharBuffer.wrap(chars); + ByteBuffer byteBuf = StandardCharsets.UTF_8.encode(charBuf); + return byteBuf.array(); + } } diff --git a/app/src/main/java/com/beemdevelopment/aegis/db/slots/PasswordSlot.java b/app/src/main/java/com/beemdevelopment/aegis/db/slots/PasswordSlot.java index f861fcf0..8efa41c7 100644 --- a/app/src/main/java/com/beemdevelopment/aegis/db/slots/PasswordSlot.java +++ b/app/src/main/java/com/beemdevelopment/aegis/db/slots/PasswordSlot.java @@ -2,6 +2,7 @@ package com.beemdevelopment.aegis.db.slots; import com.beemdevelopment.aegis.crypto.CryptParameters; import com.beemdevelopment.aegis.crypto.CryptoUtils; +import com.beemdevelopment.aegis.crypto.MasterKey; import com.beemdevelopment.aegis.crypto.SCryptParameters; import com.beemdevelopment.aegis.encoding.Hex; @@ -10,18 +11,21 @@ import org.json.JSONObject; import java.util.UUID; +import javax.crypto.Cipher; import javax.crypto.SecretKey; public class PasswordSlot extends RawSlot { + private boolean _repaired; private SCryptParameters _params; public PasswordSlot() { super(); } - protected PasswordSlot(UUID uuid, byte[] key, CryptParameters keyParams, SCryptParameters scryptParams) { + protected PasswordSlot(UUID uuid, byte[] key, CryptParameters keyParams, SCryptParameters scryptParams, boolean repaired) { super(uuid, key, keyParams); _params = scryptParams; + _repaired = repaired; } @Override @@ -32,6 +36,7 @@ public class PasswordSlot extends RawSlot { obj.put("r", _params.getR()); obj.put("p", _params.getP()); obj.put("salt", Hex.encode(_params.getSalt())); + obj.put("repaired", _repaired); return obj; } catch (JSONException e) { throw new RuntimeException(e); @@ -48,6 +53,23 @@ public class PasswordSlot extends RawSlot { return CryptoUtils.deriveKey(password, _params); } + public SecretKey deriveKey(byte[] data) { + return CryptoUtils.deriveKey(data, _params); + } + + @Override + public void setKey(MasterKey masterKey, Cipher cipher) throws SlotException { + super.setKey(masterKey, cipher); + _repaired = true; + } + + /** + * Reports whether this slot was repaired and is no longer affected by issue #95. + */ + public boolean isRepaired() { + return _repaired; + } + @Override public byte getType() { return TYPE_DERIVED; diff --git a/app/src/main/java/com/beemdevelopment/aegis/db/slots/Slot.java b/app/src/main/java/com/beemdevelopment/aegis/db/slots/Slot.java index 5d4da834..24608920 100644 --- a/app/src/main/java/com/beemdevelopment/aegis/db/slots/Slot.java +++ b/app/src/main/java/com/beemdevelopment/aegis/db/slots/Slot.java @@ -129,7 +129,8 @@ public abstract class Slot implements Serializable { obj.getInt("p"), Hex.decode(obj.getString("salt")) ); - slot = new PasswordSlot(uuid, key, keyParams, scryptParams); + boolean repaired = obj.optBoolean("repaired", false); + slot = new PasswordSlot(uuid, key, keyParams, scryptParams, repaired); break; case Slot.TYPE_FINGERPRINT: slot = new FingerprintSlot(uuid, key, keyParams); diff --git a/app/src/main/java/com/beemdevelopment/aegis/db/slots/SlotList.java b/app/src/main/java/com/beemdevelopment/aegis/db/slots/SlotList.java index 12ef0477..81934647 100644 --- a/app/src/main/java/com/beemdevelopment/aegis/db/slots/SlotList.java +++ b/app/src/main/java/com/beemdevelopment/aegis/db/slots/SlotList.java @@ -8,6 +8,7 @@ import java.io.Serializable; import java.util.ArrayList; import java.util.Iterator; import java.util.List; +import java.util.UUID; public class SlotList implements Iterable, Serializable { private List _slots = new ArrayList<>(); @@ -77,6 +78,28 @@ public class SlotList implements Iterable, Serializable { return find(type) != null; } + public void replace(Slot newSlot) { + Slot oldSlot = mustGetByUUID(newSlot.getUUID()); + _slots.set(_slots.indexOf(oldSlot), newSlot); + } + + public Slot getByUUID(UUID uuid) { + for (Slot slot : _slots) { + if (slot.getUUID().equals(uuid)) { + return slot; + } + } + return null; + } + + private Slot mustGetByUUID(UUID uuid) { + Slot slot = getByUUID(uuid); + if (slot == null) { + throw new AssertionError(String.format("no slot found with UUID: %s", uuid.toString())); + } + return slot; + } + @Override public Iterator iterator() { return _slots.iterator(); diff --git a/app/src/main/java/com/beemdevelopment/aegis/ui/AuthActivity.java b/app/src/main/java/com/beemdevelopment/aegis/ui/AuthActivity.java index 747309e4..defc1bc9 100644 --- a/app/src/main/java/com/beemdevelopment/aegis/ui/AuthActivity.java +++ b/app/src/main/java/com/beemdevelopment/aegis/ui/AuthActivity.java @@ -19,7 +19,6 @@ import android.widget.TextView; import com.beemdevelopment.aegis.R; import com.beemdevelopment.aegis.crypto.KeyStoreHandle; import com.beemdevelopment.aegis.crypto.KeyStoreHandleException; -import com.beemdevelopment.aegis.crypto.MasterKey; import com.beemdevelopment.aegis.db.DatabaseFileCredentials; import com.beemdevelopment.aegis.db.slots.FingerprintSlot; import com.beemdevelopment.aegis.db.slots.PasswordSlot; @@ -136,14 +135,6 @@ public class AuthActivity extends AegisActivity implements FingerprintUiHelper.C new SlotListTask<>(type, this, this).execute(params); } - private void setKey(MasterKey key) { - // send the master key back to the main activity - Intent result = new Intent(); - result.putExtra("creds", new DatabaseFileCredentials(key, _slots)); - setResult(RESULT_OK, result); - finish(); - } - private void selectPassword() { _textPassword.selectAll(); @@ -185,9 +176,19 @@ public class AuthActivity extends AegisActivity implements FingerprintUiHelper.C } @Override - public void onTaskFinished(MasterKey key) { - if (key != null) { - setKey(key); + public void onTaskFinished(SlotListTask.Result result) { + if (result != null) { + // replace the old slot with the repaired one + if (result.isSlotRepaired()) { + _slots.replace(result.getSlot()); + } + + // send the master key back to the main activity + Intent intent = new Intent(); + intent.putExtra("creds", new DatabaseFileCredentials(result.getKey(), _slots)); + intent.putExtra("repairedSlot", result.isSlotRepaired()); + setResult(RESULT_OK, intent); + finish(); } else { showError(); } diff --git a/app/src/main/java/com/beemdevelopment/aegis/ui/MainActivity.java b/app/src/main/java/com/beemdevelopment/aegis/ui/MainActivity.java index 667b32fb..d0467fd5 100644 --- a/app/src/main/java/com/beemdevelopment/aegis/ui/MainActivity.java +++ b/app/src/main/java/com/beemdevelopment/aegis/ui/MainActivity.java @@ -346,7 +346,13 @@ public class MainActivity extends AegisActivity implements EntryListView.Listene private void onDecryptResult(int resultCode, Intent intent) { DatabaseFileCredentials creds = (DatabaseFileCredentials) intent.getSerializableExtra("creds"); - unlockDatabase(creds); + boolean unlocked = unlockDatabase(creds); + + // save the database in case a slot was repaired + if (unlocked && intent.getBooleanExtra("repairedSlot", false)) { + _db.setCredentials(creds); + saveDatabase(); + } doShortcutActions(); } @@ -526,11 +532,7 @@ public class MainActivity extends AegisActivity implements EntryListView.Listene } } - private void unlockDatabase(DatabaseFileCredentials creds) { - if (_loaded) { - return; - } - + private boolean unlockDatabase(DatabaseFileCredentials creds) { try { if (!_db.isLoaded()) { _db.load(); @@ -538,7 +540,7 @@ public class MainActivity extends AegisActivity implements EntryListView.Listene if (_db.isLocked()) { if (creds == null) { startAuthActivity(); - return; + return false; } else { _db.unlock(creds); } @@ -546,10 +548,11 @@ public class MainActivity extends AegisActivity implements EntryListView.Listene } catch (DatabaseManagerException e) { Toast.makeText(this, getString(R.string.decryption_error), Toast.LENGTH_LONG).show(); startAuthActivity(); - return; + return false; } loadEntries(); + return true; } private void loadEntries() { diff --git a/app/src/main/java/com/beemdevelopment/aegis/ui/tasks/SlotListTask.java b/app/src/main/java/com/beemdevelopment/aegis/ui/tasks/SlotListTask.java index 3b4370f8..618a6716 100644 --- a/app/src/main/java/com/beemdevelopment/aegis/ui/tasks/SlotListTask.java +++ b/app/src/main/java/com/beemdevelopment/aegis/ui/tasks/SlotListTask.java @@ -1,8 +1,10 @@ package com.beemdevelopment.aegis.ui.tasks; +import android.app.ProgressDialog; import android.content.Context; import com.beemdevelopment.aegis.R; +import com.beemdevelopment.aegis.crypto.CryptoUtils; import com.beemdevelopment.aegis.crypto.MasterKey; import com.beemdevelopment.aegis.db.slots.FingerprintSlot; import com.beemdevelopment.aegis.db.slots.PasswordSlot; @@ -14,7 +16,7 @@ import com.beemdevelopment.aegis.db.slots.SlotList; import javax.crypto.Cipher; import javax.crypto.SecretKey; -public class SlotListTask extends ProgressDialogTask { +public class SlotListTask extends ProgressDialogTask { private Callback _cb; private Class _type; @@ -25,51 +27,77 @@ public class SlotListTask extends ProgressDialogTask 64) { + ProgressDialog dialog = getDialog(); + dialog.setMessage(dialog.getContext().getString(R.string.unlocking_vault_repair)); + + // try to decrypt the password slot with the old key + SecretKey oldKey = slot.deriveKey(oldPasswordBytes); + masterKey = decryptPasswordSlot(slot, oldKey); + } else { + masterKey = decryptPasswordSlot(slot, key); + } + + // if necessary, repair the slot by re-encrypting the master key with the correct key + // slots with passwords smaller than 64 bytes also get this treatment to make sure those also have 'repaired' set to true + boolean repaired = false; + if (!slot.isRepaired()) { + Cipher cipher = Slot.createEncryptCipher(key); + slot.setKey(masterKey, cipher); + repaired = true; + } + + return new Result(masterKey, slot, repaired); + } + + private MasterKey decryptPasswordSlot(PasswordSlot slot, SecretKey key) + throws SlotException, SlotIntegrityException { + Cipher cipher = slot.createDecryptCipher(key); + return slot.getKey(cipher); } @Override - protected void onPostExecute(MasterKey masterKey) { - super.onPostExecute(masterKey); - _cb.onTaskFinished(masterKey); + protected void onPostExecute(Result result) { + super.onPostExecute(result); + _cb.onTaskFinished(result); } public static class Params { @@ -90,7 +118,35 @@ public class SlotListTask extends ProgressDialogTaskAn error occurred while trying to read the QR code Raw Unlocking the vault + Unlocking the vault (repairing) You must have at least one password slot Remove slot Are you sure you want to remove this slot? diff --git a/app/src/test/java/com/beemdevelopment/aegis/SCryptTest.java b/app/src/test/java/com/beemdevelopment/aegis/SCryptTest.java index eacad2e2..80b750de 100644 --- a/app/src/test/java/com/beemdevelopment/aegis/SCryptTest.java +++ b/app/src/test/java/com/beemdevelopment/aegis/SCryptTest.java @@ -7,6 +7,8 @@ import com.beemdevelopment.aegis.encoding.HexException; import org.junit.Test; +import java.util.Arrays; + import javax.crypto.SecretKey; import static org.junit.Assert.*; @@ -22,19 +24,20 @@ public class SCryptTest { salt ); + byte[] head = new byte[]{'t', 'e', 's', 't'}; byte[] expectedKey = Hex.decode("41cd8110d0c66ede16f97ce84fd8e2bd2269c9318532a01437789dfbadd1392e"); - byte[][] inputs = new byte[][]{ - new byte[]{'t', 'e', 's', 't'}, - new byte[]{'t', 'e', 's', 't', '\0'}, - new byte[]{'t', 'e', 's', 't', '\0', '\0'}, - new byte[]{'t', 'e', 's', 't', '\0', '\0', '\0'}, - new byte[]{'t', 'e', 's', 't', '\0', '\0', '\0', '\0'}, - new byte[]{'t', 'e', 's', 't', '\0', '\0', '\0', '\0', '\0'}, - }; - for (byte[] input : inputs) { + for (int i = 0; i < 128; i += 4) { + byte[] input = new byte[head.length + i]; + System.arraycopy(head, 0, input, 0, head.length); + + // once the length of the input is over 64 bytes, trailing nulls do not cause a collision anymore SecretKey key = CryptoUtils.deriveKey(input, params); - assertArrayEquals(expectedKey, key.getEncoded()); + if (input.length <= 64) { + assertArrayEquals(expectedKey, key.getEncoded()); + } else { + assertFalse(Arrays.equals(expectedKey, key.getEncoded())); + } } } }