Android keystore issue in KitKat

[SyntaxPolice]

After upgrading a device to KitKat, key encryption & decryption does not work. Debugging this on our seqrd branch, I found that:

Secret.decrypt(Secret.encrypt(x)) != x.

The second half of the key is correct, but the first half is incorrect.

Note that this is causes login to fail 100% of the time for Tiqr on my test device (on all KitKat devices?)

[SyntaxPolice]

I spent some time today looking at this, and I believe I know the issue and have a proposed solution. I confirmed with the emulator that this happens in kitkat.

I note that your encryption function does not pass an IV to the init call as shown for instance in this example.

For AES CBC, you should use a 16 byte random IV that is different for each message. When you don't specify an IV in the init function, the behavior seems to vary widely. I've tried 3 different devices and gotten different behaviors:

• In KitKat, the app generated a random looking IV during encrypt, which was different (at least for each new instance of Cipher) and used all zeros during decrypt; between runs, the IV varies, so it will never decrypt the same way twice. This is what makes the app break.
• On 4.3, the app used an IV of all zeros for both encrypt and decrypt so it always encrypts and decrypts the same
• On a rooted cyanogenmod 4.2.2 device, it used an random looking IV that was the same between runs and between encrypt and decrypt. I don't know where it's getting / storing this iv.

This will make it challenging for you to support backward compatible keys. I'm not sure how you should address that. Instead of relying on this undefined behavior, you should generate a random IV and store it with the encrypted key.

I've seen this writeup on password based encryption on Android recommended.

[ijansch]

Hi Isaac,

This is indeed what we think the problem is. The cyanmod behavior is probably the 'old' behavior back in android 2 when it didn't even support manually setting the iv. 

The fix that we are investigating is twofold:

1. Force the iv to be zeroes during decrypt for existing identities so that the ones upgraded from 4.3 to a higher version will decode the same way they were encoded. If this fails, the identity must have been created in 4.4 and will not have worked anyway, so that identity must be discarded. 
2. Upgrade any existing key to use a self supplied iv for the future. We have a versioning mechanism that we also used in the ios version to be able to auto upgrade identities. We haven't needed this mechanism in the android version yet, but we should add that now. 

Greetings,

Ivo  — Sent from Mailbox for iPhone

On Wed, Jan 15, 2014 at 2:57 AM, Isaac Potoczny-Jones notifications@github.com wrote:

I spent some time today looking at this, and I believe I know the issue and have a proposed solution. I confirmed with the emulator that this happens in kitkat. I note that your encryption function does not pass an IV to the init call as shown for instance in this example. For AES CBC, you should use a 16 byte random IV that is different for each message. When you don't specify an IV in the init function, the behavior seems to vary widely. I've tried 3 different devices and gotten different behaviors:

• In KitKat, the app generated a random looking IV during encrypt, which was different (at least for each new instance of Cipher) and used all zeros during decrypt; between runs, the IV varies, so it will never decrypt the same way twice. This is what makes the app break.
• On 4.3, the app used an IV of all zeros for both encrypt and decrypt so it always encrypts and decrypts the same

• On a rooted cyanogenmod 4.2.2 device, it used an random looking IV that was the same between runs and between encrypt and decrypt. I don't know where it's getting / storing this iv. This will make it challenging for you to support backward compatible keys. I'm not sure how you should address that. Instead of relying on this undefined behavior, you should generate a random IV and store it with the encrypted key.

  Reply to this email directly or view it on GitHub: https://github.com/SURFnet/tiqr/issues/42#issuecomment-32328028

[SyntaxPolice]

Thanks, Ivo. I fixed this in our branch and want to share our approach with you in case you decide to do something similar.

You're correct; on that cyanogenmod phone, I can pass a random IV to the cipher.init function, but then cipher.getIV returns a different IV. Therefore, when storing the IV, it's important to store the one returned from cipher.getIV().

Please find attached a diff that represents our approach to solving this. I'd be interested in your feedback on this. Note that the debug logs aren't meant for production use; we log the random IV, which isn't good, but I left it in for you to experiment with.

• We introduce a new type that's basically a tuple for crypto text and IV. Since these two always travel together, it makes sense to me to introduce this new type, plus that helps me to make sure I change it everywhere that's necessary
• We generate a random IV during encryption
• We set that IV during the init function
• We then ask the cipher what iv it's using. If it's different from the one we just set, we log that
• We store cipher.getIV() in the keystore with a key ID derived from the identity's id (e.g. 5-org.tiqr.iv)
• When looking up the encrypted key for 5, we also look up 5-org.tiqr.iv and return that tuple
• During decryption, we pass that tuple along
  • If the iv part of the tuple is null, we assume that this is an old style key and try to do things the old way
• Key upgrade
  • During key loading and after decrypt, if the iv is null, we attempt to upgrade to the new ciphertext/iv tuple
  • On systems where the IV was random-fixed, this will store the random-fixed IV so in the future if they upgrade to kitkat, it won't be lost forever
  • On systems where the IV was zero, this will upgrade the IV to something random
  • On systems where the IV was random (kittkat), this shouldn't happen, but if it did, it would just store it.

--- a/MobileAuth/src/org/tiqr/authenticator/security/Encryption.java
+++ b/MobileAuth/src/org/tiqr/authenticator/security/Encryption.java
@@ -1,10 +1,15 @@
 package org.tiqr.authenticator.security;

+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.security.InvalidAlgorithmParameterException;
 import java.security.InvalidKeyException;
 import java.security.Key;
 import java.security.NoSuchAlgorithmException;
+import java.security.NoSuchProviderException;
 import java.security.SecureRandom;
 import java.security.spec.InvalidKeySpecException;
+import java.util.Arrays;

 import javax.crypto.BadPaddingException;
 import javax.crypto.Cipher;
@@ -14,6 +19,7 @@ import javax.crypto.NoSuchPaddingException;
 import javax.crypto.SecretKey;
 import javax.crypto.SecretKeyFactory;
 import javax.crypto.ShortBufferException;
+import javax.crypto.spec.IvParameterSpec;
 import javax.crypto.spec.PBEKeySpec;
 import javax.crypto.spec.SecretKeySpec;

@@ -21,6 +27,7 @@ import org.tiqr.authenticator.exceptions.SecurityFeaturesException;

 import android.content.Context;
 import android.content.SharedPreferences;
+import android.util.Log;
 import biz.source_code.base64Coder.Base64Coder;

 /**
@@ -44,6 +51,7 @@ public class Encryption
     public static final String MASTER_KEY_ALGORITHM = "HMACSHA256";
     private static final int CIPHER_KEY_ITERATIONS = 1500;
     private static final int CIPHER_KEY_SIZE = 16;
+   private static final int IV_LENGTH = 16;
     private static final String CIPHER_TRANSFORMATION = "AES/CBC/NoPadding";

@@ -79,29 +87,44 @@ public class Encryption
      * Encrypt a plaintext using the method defined in CIPHER_TRANSFORMATION.
      * Depending on the transformation, you may need to pass text in correct
      * blocksize (current implementation should be multiples of 16 bytes)
+     * 
+     * Returns a tuple with the ciphertext and randomly generated iv.
+     * 
      * @param text
      * @param key
      * @return
      * @throws SecurityFeaturesException
      */
-    public static byte[] encrypt(byte[] text, Key key) throws SecurityFeaturesException
+    public static CipherTextAndIV encrypt(byte[] text, Key key) throws SecurityFeaturesException
     {
         try {
             Cipher cipher = Cipher.getInstance(CIPHER_TRANSFORMATION);
-                        
-            cipher.init(Cipher.ENCRYPT_MODE, key);
+            
+            byte [] generatedIV = generateIv();
+            cipher.init(Cipher.ENCRYPT_MODE, key, new IvParameterSpec(generatedIV));
+            byte [] iv = cipher.getIV();
+            
+            /* Some versions of Android don't actually set the IV in that case
+             * so we'll test for that and log it, but return the iv that the system
+             * says is the one that's been used */
+            if (! Arrays.equals(generatedIV, iv)) {
+               Log.i("encryption", "Not able to set random IV on this system.");
+            }

             byte[] cipherText = new byte[cipher.getOutputSize(text.length)];
             int ctLength = cipher.update(text, 0, text.length, cipherText, 0);
             ctLength += cipher.doFinal(cipherText, ctLength);
-            return cipherText;
+            
+            return new CipherTextAndIV (cipherText, iv);
         } catch (NoSuchAlgorithmException e) {
         } catch (NoSuchPaddingException e) {
         } catch (InvalidKeyException e) {
         } catch (ShortBufferException e) {
         } catch (IllegalBlockSizeException e) {
         } catch (BadPaddingException e) {
-        }
+        } catch (NoSuchProviderException e) {
+       } catch (InvalidAlgorithmParameterException e) {
+       }

         // If any of these fail, we're dealing with a device that can't handle our level of encryption
         throw new SecurityFeaturesException();
@@ -109,22 +132,28 @@ public class Encryption
     }

     /**
-     * Decrypts the plaintext according to CIPHER_TRANSFORMATION.
+     * Decrypts the ciphertext according to CIPHER_TRANSFORMATION.
      * Note that if the cipher transformation defines a padding scheme, then the decrypted
      * string will have padding bytes (length will be a multiple of 16). Since we know
      * the length of what we encoded, we should use substrings to retrieve the result from 
      * what decrypt returns to us.
-     * @param text
+     * @param civ is both the ciphertext and iv, or if no iv, civ.iv is null
      * @param key
      * @return
      * @throws InvalidKeyException
      * @throws SecurityFeaturesException
      */
-    public static byte[] decrypt(byte[] original, Key key) throws InvalidKeyException, SecurityFeaturesException 
+    public static byte[] decrypt(CipherTextAndIV civ, Key key) throws InvalidKeyException, SecurityFeaturesException 
     {
+       byte [] original = civ.cipherText;
+       byte [] iv       = civ.iv;
         try {
             Cipher cipher = Cipher.getInstance(CIPHER_TRANSFORMATION);
-            cipher.init(Cipher.DECRYPT_MODE, key);
+            if (iv != null) {
+               cipher.init(Cipher.DECRYPT_MODE, key, new IvParameterSpec(iv));
+            } else { // handle old key types
+               cipher.init(Cipher.DECRYPT_MODE, key);
+            }
        //     byte[] original = Base64Coder.decode(text);
             byte[] plainText = new byte[cipher.getOutputSize(original.length)];
             int ptLength = cipher.update(original, 0, original.length, plainText, 0);
@@ -149,7 +178,10 @@ public class Encryption
         } catch (BadPaddingException e) {
             // Probably a wrong PIN
             throw new InvalidKeyException();
-        }
+        } catch (InvalidAlgorithmParameterException e) {
+            // IV was messed up
+            throw new InvalidKeyException();
+       }

     }

@@ -283,5 +315,22 @@ public class Encryption
         }   
         return sb.toString();   
     }  
+    
+
+    private static byte[] generateIv() throws NoSuchAlgorithmException, NoSuchProviderException {
+            SecureRandom random = SecureRandom.getInstance(RANDOM_ALGORITHM);
+            byte[] iv = new byte[IV_LENGTH];
+            random.nextBytes(iv);
+            return iv;
+    }

+    public static class CipherTextAndIV {
+       CipherTextAndIV (byte [] c, byte[] i) {
+           cipherText = c;
+           iv = i;
+       }
+       byte[] cipherText;
+       byte[] iv;
+    }
+    
 }
diff --git a/MobileAuth/src/org/tiqr/authenticator/security/Secret.java b/MobileAuth/src/org/tiqr/authenticator/security/Secret.java
index dabab88..8d0a8df 100644
--- a/MobileAuth/src/org/tiqr/authenticator/security/Secret.java
+++ b/MobileAuth/src/org/tiqr/authenticator/security/Secret.java
@@ -7,6 +7,7 @@ import javax.crypto.spec.SecretKeySpec;

 import android.app.Activity;
 import android.content.Context;
+import android.util.Log;

 import org.tiqr.authenticator.datamodel.Identity;
 import org.tiqr.authenticator.exceptions.SecurityFeaturesException;
@@ -34,7 +35,7 @@ public class Secret
     public SecretKey getSecret(SecretKey sessionKey) throws InvalidKeyException, SecurityFeaturesException
     {        
         if (_secret==null) {
-            _secret = _loadFromKeyStore(sessionKey);
+            _loadFromKeyStore(sessionKey);
         } 
         return _secret;
     }
@@ -46,17 +47,25 @@ public class Secret

     private SecretKey _loadFromKeyStore(SecretKey sessionKey) throws SecurityFeaturesException, InvalidKeyException
     {
-        SecretKey x = _store.getSecretKey(Long.toString(_identity.getId()), Encryption.getDeviceKey(_ctx));
-        if (x == null) {
+       String id = Long.toString(_identity.getId());
+       SecretKey deviceKey = Encryption.getDeviceKey(_ctx);
+       Encryption.CipherTextAndIV civ = _store.getSecretKey(id, deviceKey);
+        if (civ.cipherText == null) {
            throw new InvalidKeyException("Requested key not found.");
         }
-        return new SecretKeySpec(Encryption.decrypt(x.getEncoded(), sessionKey), "RAW");
+        _secret = new SecretKeySpec(Encryption.decrypt(civ, sessionKey), "RAW");
+        if (civ.iv == null) {
+           // Old keys didn't store the iv, so upgrade it to a new key.
+           Log.i("encryption", "Found old style key; upgrading to new key.");
+           storeInKeyStore(sessionKey);
+        }
+        return _secret;
     }

     public void storeInKeyStore(SecretKey sessionKey) throws SecurityFeaturesException
     {
-        SecretKey encrypted = new SecretKeySpec(Encryption.encrypt(_secret.getEncoded(), sessionKey), "RAW");
-         _store.setSecretKey(Long.toString(_identity.getId()), encrypted, Encryption.getDeviceKey(_ctx));
+        Encryption.CipherTextAndIV civ = Encryption.encrypt(_secret.getEncoded(), sessionKey);
+         _store.setSecretKey(Long.toString(_identity.getId()), civ, Encryption.getDeviceKey(_ctx));
     }

     public void deleteFromKeyStore(SecretKey sessionKey) throws SecurityFeaturesException
diff --git a/MobileAuth/src/org/tiqr/authenticator/security/SecretStore.java b/MobileAuth/src/org/tiqr/authenticator/security/SecretStore.java
index dbc59cd..d968f1c 100644
--- a/MobileAuth/src/org/tiqr/authenticator/security/SecretStore.java
+++ b/MobileAuth/src/org/tiqr/authenticator/security/SecretStore.java
@@ -13,6 +13,9 @@ import java.security.cert.CertificateException;
 import java.util.Enumeration;

 import javax.crypto.SecretKey;
+import javax.crypto.spec.SecretKeySpec;
+
+import biz.source_code.base64Coder.Base64Coder;

 import android.content.Context;
 import android.util.Log;
@@ -23,6 +26,7 @@ public class SecretStore
     public static String filenameKeyStore = "MobileAuthDb.kstore";
     private Context _ctx;
     private boolean _initialized = false;
+    private static String ivSuffix="-org.tiqr.iv";

     public SecretStore(Context ctx)
     {
@@ -112,16 +116,23 @@ public class SecretStore
         return result;
     }

-    public SecretKey getSecretKey(String identity, SecretKey sessionKey)
+    public Encryption.CipherTextAndIV getSecretKey(String identity, SecretKey sessionKey)
     {
         _initializeKeyStore(sessionKey);

         try {
-            for(Enumeration e= _keyStore.aliases(); e.hasMoreElements();) {
-                String alias = e.nextElement();
+            SecretKeyEntry ctEntry = (SecretKeyEntry)_keyStore.getEntry(identity, new KeyStore.PasswordProtection(_sessionKeyToCharArray(sessionKey)));
+            SecretKeyEntry ivEntry = (SecretKeyEntry)_keyStore.getEntry(identity + ivSuffix, new KeyStore.PasswordProtection(_sessionKeyToCharArray(sessionKey)));
+            byte[] ivBytes;
+            // For old keys, we don't store the IV:
+            if (ivEntry == null || ivEntry.getSecretKey() == null) {
+               ivBytes = null;
+               Log.i("encryption", "No IV found for: " + identity);
+            } else {
+               ivBytes = ivEntry.getSecretKey().getEncoded();
+               Log.i("encryption", "IV for: " + identity + " is " + new String(Base64Coder.encode(ivBytes))); ;
             }
-            SecretKeyEntry entry = (SecretKeyEntry)_keyStore.getEntry(identity, new KeyStore.PasswordProtection(_sessionKeyToCharArray(sessionKey)));
-            return entry.getSecretKey();
+            return new Encryption.CipherTextAndIV(ctEntry.getSecretKey().getEncoded(),ivBytes);
         } catch (Exception e) {
             // TODO Auto-generated catch block
             e.printStackTrace();
@@ -131,15 +142,19 @@ public class SecretStore

     }

-    public void setSecretKey(String identity, SecretKey key, SecretKey sessionKey)
+    public void setSecretKey(String identity, Encryption.CipherTextAndIV civ, SecretKey sessionKey)
     {
         _initializeKeyStore(sessionKey);

-        KeyStore.SecretKeyEntry entry = new KeyStore.SecretKeyEntry(key);
+        SecretKeySpec cipherText =  new SecretKeySpec(civ.cipherText, "RAW");
+        KeyStore.SecretKeyEntry ctEntry = new KeyStore.SecretKeyEntry(cipherText);
+        
+        SecretKeySpec iv =  new SecretKeySpec(civ.iv, "RAW");
+        KeyStore.SecretKeyEntry ivEntry = new KeyStore.SecretKeyEntry(iv);

         try {
-            _keyStore.setEntry(identity, entry, new KeyStore.PasswordProtection(_sessionKeyToCharArray(sessionKey)));
-            
+            _keyStore.setEntry(identity,            ctEntry, new KeyStore.PasswordProtection(_sessionKeyToCharArray(sessionKey)));            
+            _keyStore.setEntry(identity + ivSuffix, ivEntry, new KeyStore.PasswordProtection(_sessionKeyToCharArray(sessionKey)));
             _saveKeyStore(sessionKey);

         } catch (KeyStoreException e) {

[lucia987]

Hello!

I cannot authenticate in your demo using Android 4.4.2 that uses "com.android.org.bouncycastle.jcajce.provider.symmetric.util.BCPBEKey". I can authenticate on an older version of Android that uses "org.bouncycastle.jce.provider.JCEPBEKey". Encryption.decrypt(Encryption.encrypt(text)) does not return text and the salt is not the issue. I have used the github repo to download the source code but cannot put my finger on the source of this bug. Please help.

[scastromx]

Cannot authenticate using Android client on KitKat, it always complains about wrong PIN number. IOS client works with same server so maybe it is the same issue this thread is about. Any workaround?

Thanks,

[ijansch]

We are working on a fix that should be available shortly.

[ijansch]

The above fixes have been applied and will be in the next release.