Use the old encode method for passwords over 64 bytes and repair the slot (#98)

Commit afb9e59711 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.

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();
+    }
 }

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;

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);

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<Slot>, Serializable {
     private List<Slot> _slots = new ArrayList<>();
@@ -77,6 +78,28 @@ public class SlotList implements Iterable<Slot>, 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<Slot> iterator() {
         return _slots.iterator();

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();
         }

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() {

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<T extends Slot> extends ProgressDialogTask<SlotListTask.Params, MasterKey> {
+public class SlotListTask<T extends Slot> extends ProgressDialogTask<SlotListTask.Params, SlotListTask.Result> {
     private Callback _cb;
     private Class<T> _type;
 
@@ -25,51 +27,77 @@ public class SlotListTask<T extends Slot> extends ProgressDialogTask<SlotListTas
     }
 
     @Override
-    protected MasterKey doInBackground(SlotListTask.Params... args) {
+    protected Result doInBackground(SlotListTask.Params... args) {
         setPriority();
 
         Params params = args[0];
         SlotList slots = params.getSlots();
-        try {
-            if (!slots.has(_type)) {
-                throw new RuntimeException();
-            }
-
-            MasterKey masterKey = null;
-            for (Slot slot : slots.findAll(_type)) {
-                try {
-                    if (slot instanceof PasswordSlot) {
-                        char[] password = (char[])params.getObj();
-                        SecretKey key = ((PasswordSlot)slot).deriveKey(password);
-                        Cipher cipher = slot.createDecryptCipher(key);
-                        masterKey = slot.getKey(cipher);
-                    } else if (slot instanceof FingerprintSlot) {
-                        masterKey = slot.getKey((Cipher)params.getObj());
-                    } else {
-                        throw new RuntimeException();
-                    }
-                    break;
-                } catch (SlotIntegrityException e) {
 
+        for (Slot slot : slots.findAll(_type)) {
+            try {
+                if (slot instanceof PasswordSlot) {
+                    char[] password = (char[]) params.getObj();
+                    return decryptPasswordSlot((PasswordSlot) slot, password);
+                } else if (slot instanceof FingerprintSlot) {
+                    return decryptFingerprintSlot((FingerprintSlot) slot, (Cipher) params.getObj());
                 }
-            }
+            } catch (SlotException e) {
+                throw new RuntimeException(e);
+            } catch (SlotIntegrityException ignored) {
 
-            if (masterKey == null) {
-                throw new SlotIntegrityException();
             }
+        }
 
-            return masterKey;
-        } catch (SlotIntegrityException e) {
-            return null;
-        } catch (SlotException e) {
-            throw new RuntimeException(e);
+        return null;
+    }
+
+    private Result decryptFingerprintSlot(FingerprintSlot slot, Cipher cipher)
+            throws SlotException, SlotIntegrityException {
+        MasterKey key = slot.getKey(cipher);
+        return new Result(key, slot);
+    }
+
+    private Result decryptPasswordSlot(PasswordSlot slot, char[] password)
+            throws SlotIntegrityException, SlotException {
+        MasterKey masterKey;
+        SecretKey key = slot.deriveKey(password);
+
+        // a bug introduced in afb9e59 caused 64-byte passwords to produce a different key than before
+        // so, try the old password encode function if the encoded password is longer than 64 bytes
+        byte[] oldPasswordBytes = CryptoUtils.toBytesOld(password);
+        if (!slot.isRepaired() && oldPasswordBytes.length > 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<T extends Slot> extends ProgressDialogTask<SlotListTas
         }
     }
 
+    public static class Result {
+        private MasterKey _key;
+        private Slot _slot;
+        private boolean _repaired;
+
+        public Result(MasterKey key, Slot slot, boolean repaired) {
+            _key = key;
+            _slot = slot;
+            _repaired = repaired;
+        }
+
+        public Result(MasterKey key, Slot slot) {
+            this(key, slot, false);
+        }
+
+        public MasterKey getKey() {
+            return _key;
+        }
+
+        public Slot getSlot() {
+            return _slot;
+        }
+
+        public boolean isSlotRepaired() {
+            return _repaired;
+        }
+    }
+
     public interface Callback {
-        void onTaskFinished(MasterKey key);
+        void onTaskFinished(Result result);
     }
 }

app/src/main/res/values/strings.xml

@@ -127,6 +127,7 @@
     <string name="read_qr_error">An error occurred while trying to read the QR code</string>
     <string name="authentication_method_raw">Raw</string>
     <string name="unlocking_vault">Unlocking the vault</string>
+    <string name="unlocking_vault_repair">Unlocking the vault (repairing)</string>
     <string name="password_slot_error">You must have at least one password slot</string>
     <string name="remove_slot">Remove slot</string>
     <string name="remove_slot_description">Are you sure you want to remove this slot?</string>

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()));
+            }
         }
     }
 }