From d984e5fd6211e1b23c8847db4912ce89753e3e80 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Wed, 21 Jun 2023 05:28:10 +0000 Subject: [PATCH] Make LockscreenCredential remember whether it has invalid chars MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit http://ag/6283443 ("Refactor passwords/pins/patterns to byte[]" in packages/apps/Settings), which went into Android 10, introduced a bug where the chars of the lockscreen password are truncated to bytes before the password is validated to contain only ASCII 32–127. This causes characters outside the intended range to be accepted. Specifically, any character U-XXXX where XXXX mod 256 is in 32–127 is accepted and is treated as equivalent to some ASCII character. This reduces the entropy of the password, but also it can make it impossible for the user to unlock the device after rebooting. This happens if the chosen password uses a character that can only be entered on a third-party keyboard (IME) that is not direct boot aware or was uninstalled later. (The potential dependence on a third-party keyboard is one of the reasons that non-ASCII characters were never intended to be allowed in lockscreen passwords in the first place.) Unfortunately, it's likely that some users managed to set a password containing non-ASCII character(s) and are happily using it. To allow fixing this bug without locking out such users, this CL updates LockscreenCredential to keep track of whether it was instantiated using any invalid characters or not, while still keeping the truncation bug in place. Later CLs will use this "invalid chars" flag to reject new passwords that contain any invalid characters. Bug: 219511761 Bug: 232900169 Bug: 243881358 Test: atest LockscreenCredentialTest Test: atest com.android.server.locksettings Change-Id: I5c3c55367c3a294578cd0f97ac0e315a11ed517e --- .../internal/widget/LockscreenCredential.java | 88 +++++++++++++++---- .../widget/LockscreenCredentialTest.java | 57 +++++++++--- 2 files changed, 114 insertions(+), 31 deletions(-) diff --git a/core/java/com/android/internal/widget/LockscreenCredential.java b/core/java/com/android/internal/widget/LockscreenCredential.java index 6e63f790ab982..6bc1a43b8c206 100644 --- a/core/java/com/android/internal/widget/LockscreenCredential.java +++ b/core/java/com/android/internal/widget/LockscreenCredential.java @@ -64,6 +64,20 @@ public class LockscreenCredential implements Parcelable, AutoCloseable { // is represented as a byte array of length 0. private byte[] mCredential; + // This indicates that the credential is a password that used characters outside ASCII 32–127. + // + // Such passwords were never intended to be allowed. However, Android 10–14 had a bug where + // conversion from the chars the user entered to the credential bytes used a simple truncation. + // Thus, any 'char' whose remainder mod 256 was in the range 32–127 was accepted and was + // equivalent to some ASCII character. For example, ™, which is U+2122, was truncated to ASCII + // 0x22 which is the double-quote character ". + // + // We have to continue to allow a LockscreenCredential to be constructed with this bug, so that + // existing devices can be unlocked if their password used this bug. However, we prevent new + // passwords that use this bug from being set. The boolean below keeps track of the information + // needed to do that check, since the conversion to mCredential may have been lossy. + private final boolean mHasInvalidChars; + /** * Private constructor, use static builder methods instead. * @@ -71,7 +85,7 @@ public class LockscreenCredential implements Parcelable, AutoCloseable { * LockscreenCredential will only store the reference internally without copying. This is to * minimize the number of extra copies introduced. */ - private LockscreenCredential(int type, byte[] credential) { + private LockscreenCredential(int type, byte[] credential, boolean hasInvalidChars) { Objects.requireNonNull(credential); if (type == CREDENTIAL_TYPE_NONE) { Preconditions.checkArgument(credential.length == 0); @@ -88,15 +102,21 @@ public class LockscreenCredential implements Parcelable, AutoCloseable { // LockscreenCredential object to be constructed so that the validation logic can run, // even though the validation logic will ultimately reject the credential as too short. } + Preconditions.checkArgument(!hasInvalidChars || type == CREDENTIAL_TYPE_PASSWORD); mType = type; mCredential = credential; + mHasInvalidChars = hasInvalidChars; + } + + private LockscreenCredential(int type, CharSequence credential) { + this(type, charsToBytesTruncating(credential), hasInvalidChars(credential)); } /** * Creates a LockscreenCredential object representing a none credential. */ public static LockscreenCredential createNone() { - return new LockscreenCredential(CREDENTIAL_TYPE_NONE, new byte[0]); + return new LockscreenCredential(CREDENTIAL_TYPE_NONE, new byte[0], false); } /** @@ -104,15 +124,14 @@ public class LockscreenCredential implements Parcelable, AutoCloseable { */ public static LockscreenCredential createPattern(@NonNull List pattern) { return new LockscreenCredential(CREDENTIAL_TYPE_PATTERN, - LockPatternUtils.patternToByteArray(pattern)); + LockPatternUtils.patternToByteArray(pattern), /* hasInvalidChars= */ false); } /** * Creates a LockscreenCredential object representing the given alphabetic password. */ public static LockscreenCredential createPassword(@NonNull CharSequence password) { - return new LockscreenCredential(CREDENTIAL_TYPE_PASSWORD, - charSequenceToByteArray(password)); + return new LockscreenCredential(CREDENTIAL_TYPE_PASSWORD, password); } /** @@ -123,15 +142,14 @@ public class LockscreenCredential implements Parcelable, AutoCloseable { */ public static LockscreenCredential createManagedPassword(@NonNull byte[] password) { return new LockscreenCredential(CREDENTIAL_TYPE_PASSWORD, - Arrays.copyOf(password, password.length)); + Arrays.copyOf(password, password.length), /* hasInvalidChars= */ false); } /** * Creates a LockscreenCredential object representing the given numeric PIN. */ public static LockscreenCredential createPin(@NonNull CharSequence pin) { - return new LockscreenCredential(CREDENTIAL_TYPE_PIN, - charSequenceToByteArray(pin)); + return new LockscreenCredential(CREDENTIAL_TYPE_PIN, pin); } /** @@ -211,10 +229,17 @@ public class LockscreenCredential implements Parcelable, AutoCloseable { return mCredential.length; } + /** Returns true if this credential was constructed with any chars outside the allowed range */ + public boolean hasInvalidChars() { + ensureNotZeroized(); + return mHasInvalidChars; + } + /** Create a copy of the credential */ public LockscreenCredential duplicate() { return new LockscreenCredential(mType, - mCredential != null ? Arrays.copyOf(mCredential, mCredential.length) : null); + mCredential != null ? Arrays.copyOf(mCredential, mCredential.length) : null, + mHasInvalidChars); } /** @@ -323,6 +348,7 @@ public class LockscreenCredential implements Parcelable, AutoCloseable { public void writeToParcel(Parcel dest, int flags) { dest.writeInt(mType); dest.writeByteArray(mCredential); + dest.writeBoolean(mHasInvalidChars); } public static final Parcelable.Creator CREATOR = @@ -330,7 +356,8 @@ public class LockscreenCredential implements Parcelable, AutoCloseable { @Override public LockscreenCredential createFromParcel(Parcel source) { - return new LockscreenCredential(source.readInt(), source.createByteArray()); + return new LockscreenCredential(source.readInt(), source.createByteArray(), + source.readBoolean()); } @Override @@ -352,7 +379,7 @@ public class LockscreenCredential implements Parcelable, AutoCloseable { @Override public int hashCode() { // Effective Java — Always override hashCode when you override equals - return Objects.hash(mType, Arrays.hashCode(mCredential)); + return Objects.hash(mType, Arrays.hashCode(mCredential), mHasInvalidChars); } @Override @@ -360,20 +387,45 @@ public class LockscreenCredential implements Parcelable, AutoCloseable { if (o == this) return true; if (!(o instanceof LockscreenCredential)) return false; final LockscreenCredential other = (LockscreenCredential) o; - return mType == other.mType && Arrays.equals(mCredential, other.mCredential); + return mType == other.mType && Arrays.equals(mCredential, other.mCredential) + && mHasInvalidChars == other.mHasInvalidChars; + } + + private static boolean hasInvalidChars(CharSequence chars) { + // + // Consider the password to have invalid characters if it contains any non-ASCII characters + // or control characters. There are multiple reasons for this restriction: + // + // - Non-ASCII characters might only be possible to enter on a third-party keyboard app + // (IME) that is available when setting the password but not when verifying it after a + // reboot. This can happen if the keyboard is not direct boot aware or gets uninstalled. + // + // - Unicode strings that look identical to the user can map to different byte[]. Yet, only + // one byte[] can be accepted. Unicode normalization can solve this problem to some + // extent, but still many Unicode characters look similar and could cause confusion. + // + // - For backwards compatibility reasons, the upper 8 bits of the 16-bit 'chars' are + // discarded by charsToBytesTruncating(). Thus, as-is passwords with characters above + // U+00FF (255) are not as secure as they should be. IMPORTANT: Do not change the below + // code to allow characters above U+00FF (255) without fixing this issue! + // + for (int i = 0; i < chars.length(); i++) { + char c = chars.charAt(i); + if (c < 32 || c > 127) { + return true; + } + } + return false; } /** - * Converts a CharSequence to a byte array without requiring a toString(), which creates an - * additional copy. + * Converts a CharSequence to a byte array, intentionally truncating chars greater than 255 for + * backwards compatibility reasons. See {@link #mHasInvalidChars}. * * @param chars The CharSequence to convert * @return A byte array representing the input */ - private static byte[] charSequenceToByteArray(CharSequence chars) { - if (chars == null) { - return new byte[0]; - } + private static byte[] charsToBytesTruncating(CharSequence chars) { byte[] bytes = new byte[chars.length()]; for (int i = 0; i < chars.length(); i++) { bytes[i] = (byte) chars.charAt(i); diff --git a/core/tests/coretests/src/com/android/internal/widget/LockscreenCredentialTest.java b/core/tests/coretests/src/com/android/internal/widget/LockscreenCredentialTest.java index 434a8953b19f1..2ad367cb939f7 100644 --- a/core/tests/coretests/src/com/android/internal/widget/LockscreenCredentialTest.java +++ b/core/tests/coretests/src/com/android/internal/widget/LockscreenCredentialTest.java @@ -21,7 +21,7 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotSame; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -46,6 +46,7 @@ public class LockscreenCredentialTest { assertFalse(none.isPin()); assertFalse(none.isPassword()); assertFalse(none.isPattern()); + assertFalse(none.hasInvalidChars()); } @Test @@ -59,6 +60,7 @@ public class LockscreenCredentialTest { assertFalse(pin.isNone()); assertFalse(pin.isPassword()); assertFalse(pin.isPattern()); + assertFalse(pin.hasInvalidChars()); } @Test @@ -72,6 +74,7 @@ public class LockscreenCredentialTest { assertFalse(password.isNone()); assertFalse(password.isPin()); assertFalse(password.isPattern()); + assertFalse(password.hasInvalidChars()); } @Test @@ -91,6 +94,7 @@ public class LockscreenCredentialTest { assertFalse(pattern.isNone()); assertFalse(pattern.isPin()); assertFalse(pattern.isPassword()); + assertFalse(pattern.hasInvalidChars()); } // Constructing a LockscreenCredential with a too-short length, even 0, should not throw an @@ -131,6 +135,20 @@ public class LockscreenCredentialTest { LockscreenCredential.createPinOrNone("1357")); } + // Test that passwords containing invalid characters that were incorrectly allowed in + // Android 10–14 are still interpreted in the same way. + @Test + public void testPasswordWithInvalidChars() { + // ™ is U+2122, which was truncated to ASCII 0x22 which is double quote. + String[] passwords = new String[] { "foo™", "™™™™", "™foo" }; + String[] equivalentAsciiPasswords = new String[] { "foo\"", "\"\"\"\"", "\"foo" }; + for (int i = 0; i < passwords.length; i++) { + LockscreenCredential credential = LockscreenCredential.createPassword(passwords[i]); + assertTrue(credential.hasInvalidChars()); + assertArrayEquals(equivalentAsciiPasswords[i].getBytes(), credential.getCredential()); + } + } + @Test public void testSanitize() { LockscreenCredential password = LockscreenCredential.createPassword("password"); @@ -156,6 +174,10 @@ public class LockscreenCredentialTest { password.size(); fail("Sanitized credential still accessible"); } catch (IllegalStateException expected) { } + try { + password.hasInvalidChars(); + fail("Sanitized credential still accessible"); + } catch (IllegalStateException expected) { } try { password.getCredential(); fail("Sanitized credential still accessible"); @@ -171,32 +193,37 @@ public class LockscreenCredentialTest { LockscreenCredential.createPin("4321")); assertEquals(createPattern("1234"), createPattern("1234")); - assertNotSame(LockscreenCredential.createPassword("1234"), + assertNotEquals(LockscreenCredential.createPassword("1234"), LockscreenCredential.createNone()); - assertNotSame(LockscreenCredential.createPassword("1234"), + assertNotEquals(LockscreenCredential.createPassword("1234"), LockscreenCredential.createPassword("4321")); - assertNotSame(LockscreenCredential.createPassword("1234"), + assertNotEquals(LockscreenCredential.createPassword("1234"), createPattern("1234")); - assertNotSame(LockscreenCredential.createPassword("1234"), + assertNotEquals(LockscreenCredential.createPassword("1234"), LockscreenCredential.createPin("1234")); - assertNotSame(LockscreenCredential.createPin("1111"), + assertNotEquals(LockscreenCredential.createPin("1111"), LockscreenCredential.createNone()); - assertNotSame(LockscreenCredential.createPin("1111"), + assertNotEquals(LockscreenCredential.createPin("1111"), LockscreenCredential.createPin("2222")); - assertNotSame(LockscreenCredential.createPin("1111"), + assertNotEquals(LockscreenCredential.createPin("1111"), createPattern("1111")); - assertNotSame(LockscreenCredential.createPin("1111"), + assertNotEquals(LockscreenCredential.createPin("1111"), LockscreenCredential.createPassword("1111")); - assertNotSame(createPattern("5678"), + assertNotEquals(createPattern("5678"), LockscreenCredential.createNone()); - assertNotSame(createPattern("5678"), + assertNotEquals(createPattern("5678"), createPattern("1234")); - assertNotSame(createPattern("5678"), + assertNotEquals(createPattern("5678"), LockscreenCredential.createPassword("5678")); - assertNotSame(createPattern("5678"), + assertNotEquals(createPattern("5678"), LockscreenCredential.createPin("5678")); + + // Test that mHasInvalidChars is compared. To do this, compare two passwords that map to + // the same byte[] (due to the truncation bug) but different values of mHasInvalidChars. + assertNotEquals(LockscreenCredential.createPassword("™™™™"), + LockscreenCredential.createPassword("\"\"\"\"")); } @Test @@ -211,6 +238,10 @@ public class LockscreenCredentialTest { assertEquals(credential, credential.duplicate()); credential = createPattern("5678"); assertEquals(credential, credential.duplicate()); + + // Test that mHasInvalidChars is duplicated. + credential = LockscreenCredential.createPassword("™™™™"); + assertEquals(credential, credential.duplicate()); } @Test