Don't rely on the system locale for converting to/from bytes.

By default, when java converts Strings to bytes, it uses the
default system locale.  This can be specified by the -Dfile.encoding
option.  If no file encoding is specified, java uses ISO8859_1.

Unfortunately, not all unicode characters can be mapped to
ISO8859_1.  Unmappable characters may be replaced by a byte
within ISO8859_1, which may change the meaning of the String.
This is especially problematic for password strings, and has
been used to compromise the security of passwords in the
past.

Thankfully, Android uses UTF-8 by default, so this bug doesn't
effect Android devices.  However, it's recommended to explicitly
list the character set when converting to/from bytes to
avoid the potential ambiguity.

Change-Id: Iec927e27ed3fc103696c439f6bd3e8779a37ade8

2 files changed, 41 insertions, 11 deletions

diff --git a/keystore/java/android/security/KeyStore.java b/keystore/java/android/security/KeyStore.java
index 6041b83..4614b53 100644
--- a/keystore/java/android/security/KeyStore.java
+++ b/keystore/java/android/security/KeyStore.java
@@ -22,6 +22,7 @@ import android.net.LocalSocket;
 import java.io.InputStream;
 import java.io.IOException;
 import java.io.OutputStream;
+import java.io.UnsupportedEncodingException;
 import java.util.ArrayList;
 
 /**
@@ -57,12 +58,12 @@ public class KeyStore {
 
     public byte[] get(byte[] key) {
         ArrayList<byte[]> values = execute('g', key);
-        return (values == null || values.size() == 0) ? null : values.get(0);
+        return (values == null || values.isEmpty()) ? null : values.get(0);
     }
 
     public String get(String key) {
-        byte[] value = get(key.getBytes());
-        return (value == null) ? null : new String(value);
+        byte[] value = get(getBytes(key));
+        return (value == null) ? null : toString(value);
     }
 
     public boolean put(byte[] key, byte[] value) {
@@ -71,7 +72,7 @@ public class KeyStore {
     }
 
     public boolean put(String key, String value) {
-        return put(key.getBytes(), value.getBytes());
+        return put(getBytes(key), getBytes(value));
     }
 
     public boolean delete(byte[] key) {
@@ -80,7 +81,7 @@ public class KeyStore {
     }
 
     public boolean delete(String key) {
-        return delete(key.getBytes());
+        return delete(getBytes(key));
     }
 
     public boolean contains(byte[] key) {
@@ -89,7 +90,7 @@ public class KeyStore {
     }
 
     public boolean contains(String key) {
-        return contains(key.getBytes());
+        return contains(getBytes(key));
     }
 
     public byte[][] saw(byte[] prefix) {
@@ -98,13 +99,13 @@ public class KeyStore {
     }
 
     public String[] saw(String prefix) {
-        byte[][] values = saw(prefix.getBytes());
+        byte[][] values = saw(getBytes(prefix));
         if (values == null) {
             return null;
         }
         String[] strings = new String[values.length];
         for (int i = 0; i < values.length; ++i) {
-            strings[i] = new String(values[i]);
+            strings[i] = toString(values[i]);
         }
         return strings;
     }
@@ -120,7 +121,7 @@ public class KeyStore {
     }
 
     public boolean password(String oldPassword, String newPassword) {
-        return password(oldPassword.getBytes(), newPassword.getBytes());
+        return password(getBytes(oldPassword), getBytes(newPassword));
     }
 
     public boolean password(byte[] password) {
@@ -128,7 +129,7 @@ public class KeyStore {
     }
 
     public boolean password(String password) {
-        return password(password.getBytes());
+        return password(getBytes(password));
     }
 
     public boolean lock() {
@@ -142,7 +143,7 @@ public class KeyStore {
     }
 
     public boolean unlock(String password) {
-        return unlock(password.getBytes());
+        return unlock(getBytes(password));
     }
 
     public int getLastError() {
@@ -208,4 +209,22 @@ public class KeyStore {
         }
         return null;
     }
+
+    private static byte[] getBytes(String string) {
+        try {
+            return string.getBytes("UTF-8");
+        } catch (UnsupportedEncodingException e) {
+            // will never happen
+            throw new RuntimeException(e);
+        }
+    }
+
+    private static String toString(byte[] bytes) {
+        try {
+            return new String(bytes, "UTF-8");
+        } catch (UnsupportedEncodingException e) {
+            // will never happen
+            throw new RuntimeException(e);
+        }
+    }
 }
diff --git a/keystore/tests/src/android/security/KeyStoreTest.java b/keystore/tests/src/android/security/KeyStoreTest.java
index 569d8da..6630a4f 100755
--- a/keystore/tests/src/android/security/KeyStoreTest.java
+++ b/keystore/tests/src/android/security/KeyStoreTest.java
@@ -39,6 +39,9 @@ public class KeyStoreTest extends ActivityUnitTestCase<Activity> {
     private static final String TEST_KEYNAME2 = "testkey2";
     private static final String TEST_KEYVALUE = "test value";
 
+    // "Hello, World" in Chinese
+    private static final String TEST_I18N = "\u4F60\u597D, \u4E16\u754C";
+
     private KeyStore mKeyStore = null;
 
     public KeyStoreTest() {
@@ -83,6 +86,14 @@ public class KeyStoreTest extends ActivityUnitTestCase<Activity> {
         assertTrue(mKeyStore.put(TEST_KEYNAME, TEST_KEYVALUE));
     }
 
+    public void testI18n() throws Exception {
+        assertFalse(mKeyStore.put(TEST_I18N, TEST_I18N));
+        assertFalse(mKeyStore.contains(TEST_I18N));
+        mKeyStore.password(TEST_I18N);
+        assertTrue(mKeyStore.put(TEST_I18N, TEST_I18N));
+        assertTrue(mKeyStore.contains(TEST_I18N));
+    }
+
     public void testDelete() throws Exception {
         assertTrue(mKeyStore.delete(TEST_KEYNAME));
         mKeyStore.password(TEST_PASSWD);