diff --git a/core/java/android/app/KeyguardManager.java b/core/java/android/app/KeyguardManager.java index e7a2b61ecaf9e..f467200fab85f 100644 --- a/core/java/android/app/KeyguardManager.java +++ b/core/java/android/app/KeyguardManager.java @@ -883,17 +883,18 @@ public class KeyguardManager { if (!checkInitialLockMethodUsage()) { return false; } + Objects.requireNonNull(password, "Password cannot be null."); complexity = PasswordMetrics.sanitizeComplexityLevel(complexity); // TODO: b/131755827 add devicePolicyManager support for Auto DevicePolicyManager devicePolicyManager = (DevicePolicyManager) mContext.getSystemService(Context.DEVICE_POLICY_SERVICE); PasswordMetrics adminMetrics = devicePolicyManager.getPasswordMinimumMetrics(mContext.getUserId()); - // Check if the password fits the mold of a pin or pattern. - boolean isPinOrPattern = lockType != PASSWORD; - return PasswordMetrics.validatePassword( - adminMetrics, complexity, isPinOrPattern, password).size() == 0; + try (LockscreenCredential credential = createLockscreenCredential(lockType, password)) { + return PasswordMetrics.validateCredential(adminMetrics, complexity, + credential).size() == 0; + } } /** diff --git a/core/java/android/app/admin/PasswordMetrics.java b/core/java/android/app/admin/PasswordMetrics.java index ab48791d43eff..251c5d83fbb9c 100644 --- a/core/java/android/app/admin/PasswordMetrics.java +++ b/core/java/android/app/admin/PasswordMetrics.java @@ -513,16 +513,45 @@ public final class PasswordMetrics implements Parcelable { } /** - * Validates password against minimum metrics and complexity. + * Validates a proposed lockscreen credential against minimum metrics and complexity. * - * @param adminMetrics - minimum metrics to satisfy admin requirements. - * @param minComplexity - minimum complexity imposed by the requester. - * @param isPin - whether it is PIN that should be only digits - * @param password - password to validate. - * @return a list of password validation errors. An empty list means the password is OK. + * @param adminMetrics minimum metrics to satisfy admin requirements + * @param minComplexity minimum complexity imposed by the requester + * @param credential the proposed lockscreen credential + * + * @return a list of validation errors. An empty list means the credential is OK. * * TODO: move to PasswordPolicy */ + public static List validateCredential( + PasswordMetrics adminMetrics, int minComplexity, LockscreenCredential credential) { + if (credential.hasInvalidChars()) { + return Collections.singletonList( + new PasswordValidationError(CONTAINS_INVALID_CHARACTERS, 0)); + } + if (credential.isPassword() || credential.isPin()) { + return validatePassword(adminMetrics, minComplexity, credential.isPin(), + credential.getCredential()); + } else { + return validatePasswordMetrics(adminMetrics, minComplexity, + new PasswordMetrics(credential.getType())); + } + } + + /** + * Validates a proposed lockscreen credential against minimum metrics and complexity. + * + * @param adminMetrics minimum metrics to satisfy admin requirements + * @param minComplexity minimum complexity imposed by the requester + * @param isPin whether to validate as a PIN (true) or password (false) + * @param password the proposed lockscreen credential as a byte[]. Must be the value from + * {@link LockscreenCredential#getCredential()}. + * + * @return a list of validation errors. An empty list means the credential is OK. + * + * TODO: merge this into validateCredential() and remove the redundant hasInvalidCharacters(), + * once all external callers are removed + */ public static List validatePassword( PasswordMetrics adminMetrics, int minComplexity, boolean isPin, byte[] password) { diff --git a/core/java/com/android/internal/widget/LockPatternUtils.java b/core/java/com/android/internal/widget/LockPatternUtils.java index 12fd6c5531497..fe5850a59903f 100644 --- a/core/java/com/android/internal/widget/LockPatternUtils.java +++ b/core/java/com/android/internal/widget/LockPatternUtils.java @@ -97,7 +97,7 @@ public class LockPatternUtils { public static final int MIN_LOCK_PATTERN_SIZE = 4; /** - * The minimum size of a valid password. + * The minimum size of a valid password or PIN. */ public static final int MIN_LOCK_PASSWORD_SIZE = 4; @@ -780,7 +780,6 @@ public class LockPatternUtils { * and return false if the given credential is wrong. * @throws RuntimeException if password change encountered an unrecoverable error. * @throws UnsupportedOperationException secure lockscreen is not supported on this device. - * @throws IllegalArgumentException if new credential is too short. */ public boolean setLockCredential(@NonNull LockscreenCredential newCredential, @NonNull LockscreenCredential savedCredential, int userHandle) { @@ -788,7 +787,6 @@ public class LockPatternUtils { throw new UnsupportedOperationException( "This operation requires the lock screen feature."); } - newCredential.checkLength(); try { if (!getLockSettings().setLockCredential(newCredential, savedCredential, userHandle)) { @@ -1553,7 +1551,6 @@ public class LockPatternUtils { throw new UnsupportedOperationException( "This operation requires the lock screen feature."); } - credential.checkLength(); LockSettingsInternal localService = getLockSettingsInternal(); return localService.setLockCredentialWithToken(credential, tokenHandle, token, userHandle); diff --git a/core/java/com/android/internal/widget/LockscreenCredential.java b/core/java/com/android/internal/widget/LockscreenCredential.java index 03e7fd1c74035..65e5e2965527e 100644 --- a/core/java/com/android/internal/widget/LockscreenCredential.java +++ b/core/java/com/android/internal/widget/LockscreenCredential.java @@ -60,10 +60,24 @@ import java.util.Objects; public class LockscreenCredential implements Parcelable, AutoCloseable { private final int mType; - // Stores raw credential bytes, or null if credential has been zeroized. An empty password + // Stores raw credential bytes, or null if credential has been zeroized. A none credential // 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); @@ -80,17 +94,29 @@ public class LockscreenCredential implements Parcelable, AutoCloseable { Preconditions.checkArgument(type == CREDENTIAL_TYPE_PIN || type == CREDENTIAL_TYPE_PASSWORD || type == CREDENTIAL_TYPE_PATTERN); - Preconditions.checkArgument(credential.length > 0); + // Do not validate credential.length yet. All non-none credentials have a minimum + // length requirement; however, one of the uses of LockscreenCredential is to represent + // a proposed credential that might be too short. For example, a LockscreenCredential + // with type CREDENTIAL_TYPE_PIN and length 0 represents an attempt to set an empty PIN. + // This differs from an actual attempt to set a none credential. We have to allow the + // 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 empty password. + * 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); } /** @@ -98,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); } /** @@ -117,20 +142,19 @@ 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); } /** * Creates a LockscreenCredential object representing the given alphabetic password. - * If the supplied password is empty, create an empty credential object. + * If the supplied password is empty, create a none credential object. */ public static LockscreenCredential createPasswordOrNone(@Nullable CharSequence password) { if (TextUtils.isEmpty(password)) { @@ -142,7 +166,7 @@ public class LockscreenCredential implements Parcelable, AutoCloseable { /** * Creates a LockscreenCredential object representing the given numeric PIN. - * If the supplied password is empty, create an empty credential object. + * If the supplied password is empty, create a none credential object. */ public static LockscreenCredential createPinOrNone(@Nullable CharSequence pin) { if (TextUtils.isEmpty(pin)) { @@ -175,7 +199,7 @@ public class LockscreenCredential implements Parcelable, AutoCloseable { return mCredential; } - /** Returns whether this is an empty credential */ + /** Returns whether this is a none credential */ public boolean isNone() { ensureNotZeroized(); return mType == CREDENTIAL_TYPE_NONE; @@ -205,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); } /** @@ -222,27 +253,37 @@ public class LockscreenCredential implements Parcelable, AutoCloseable { } /** - * Check if the credential meets minimal length requirement. + * Checks whether the credential meets basic requirements for setting it as a new credential. * - * @throws IllegalArgumentException if the credential is too short. + * This is redundant if {@link android.app.admin.PasswordMetrics#validateCredential()}, which + * does more comprehensive checks, is correctly called first (which it should be). + * + * @throws IllegalArgumentException if the credential contains invalid characters or is too + * short */ - public void checkLength() { - if (isNone()) { - return; - } - if (isPattern()) { - if (size() < LockPatternUtils.MIN_LOCK_PATTERN_SIZE) { - throw new IllegalArgumentException("pattern must not be null and at least " - + LockPatternUtils.MIN_LOCK_PATTERN_SIZE + " dots long."); - } - return; - } - if (isPassword() || isPin()) { - if (size() < LockPatternUtils.MIN_LOCK_PASSWORD_SIZE) { - throw new IllegalArgumentException("password must not be null and at least " - + "of length " + LockPatternUtils.MIN_LOCK_PASSWORD_SIZE); - } - return; + public void validateBasicRequirements() { + switch (getType()) { + case CREDENTIAL_TYPE_PATTERN: + if (size() < LockPatternUtils.MIN_LOCK_PATTERN_SIZE) { + throw new IllegalArgumentException("pattern must be at least " + + LockPatternUtils.MIN_LOCK_PATTERN_SIZE + " dots long."); + } + break; + case CREDENTIAL_TYPE_PIN: + if (size() < LockPatternUtils.MIN_LOCK_PASSWORD_SIZE) { + throw new IllegalArgumentException("PIN must be at least " + + LockPatternUtils.MIN_LOCK_PASSWORD_SIZE + " digits long."); + } + break; + case CREDENTIAL_TYPE_PASSWORD: + if (mHasInvalidChars) { + throw new IllegalArgumentException("password contains invalid characters"); + } + if (size() < LockPatternUtils.MIN_LOCK_PASSWORD_SIZE) { + throw new IllegalArgumentException("password must be at least " + + LockPatternUtils.MIN_LOCK_PASSWORD_SIZE + " characters long."); + } + break; } } @@ -317,6 +358,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 = @@ -324,7 +366,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 @@ -346,7 +389,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 @@ -354,20 +397,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/android/app/admin/PasswordMetricsTest.java b/core/tests/coretests/src/android/app/admin/PasswordMetricsTest.java index c9e02f8a998d4..84c54006fc67b 100644 --- a/core/tests/coretests/src/android/app/admin/PasswordMetricsTest.java +++ b/core/tests/coretests/src/android/app/admin/PasswordMetricsTest.java @@ -25,6 +25,7 @@ import static android.app.admin.DevicePolicyManager.PASSWORD_QUALITY_SOMETHING; import static android.app.admin.DevicePolicyManager.PASSWORD_QUALITY_UNSPECIFIED; import static android.app.admin.PasswordMetrics.complexityLevelToMinQuality; import static android.app.admin.PasswordMetrics.sanitizeComplexityLevel; +import static android.app.admin.PasswordMetrics.validateCredential; import static android.app.admin.PasswordMetrics.validatePasswordMetrics; import static com.android.internal.widget.LockPatternUtils.CREDENTIAL_TYPE_NONE; @@ -41,6 +42,7 @@ import android.platform.test.annotations.Presubmit; import androidx.test.ext.junit.runners.AndroidJUnit4; import androidx.test.filters.SmallTest; +import com.android.internal.widget.LockscreenCredential; import com.android.internal.widget.PasswordValidationError; import org.junit.Test; @@ -374,8 +376,51 @@ public class PasswordMetricsTest { PasswordValidationError.NOT_ENOUGH_NON_DIGITS, 1); } + @Test + public void testValidateCredential_none() { + PasswordMetrics adminMetrics; + LockscreenCredential none = LockscreenCredential.createNone(); + + adminMetrics = new PasswordMetrics(CREDENTIAL_TYPE_NONE); + assertValidationErrors( + validateCredential(adminMetrics, PASSWORD_COMPLEXITY_NONE, none)); + + adminMetrics = new PasswordMetrics(CREDENTIAL_TYPE_PIN); + assertValidationErrors( + validateCredential(adminMetrics, PASSWORD_COMPLEXITY_NONE, none), + PasswordValidationError.WEAK_CREDENTIAL_TYPE, 0); + } + + @Test + public void testValidateCredential_password() { + PasswordMetrics adminMetrics; + LockscreenCredential password; + + adminMetrics = new PasswordMetrics(CREDENTIAL_TYPE_NONE); + password = LockscreenCredential.createPassword("password"); + assertValidationErrors( + validateCredential(adminMetrics, PASSWORD_COMPLEXITY_LOW, password)); + + // Test that validateCredential() checks LockscreenCredential#hasInvalidChars(). + adminMetrics = new PasswordMetrics(CREDENTIAL_TYPE_NONE); + password = LockscreenCredential.createPassword("™™™™"); + assertTrue(password.hasInvalidChars()); + assertValidationErrors( + validateCredential(adminMetrics, PASSWORD_COMPLEXITY_LOW, password), + PasswordValidationError.CONTAINS_INVALID_CHARACTERS, 0); + + // Test one more case where validateCredential() should reject the password. Beyond this, + // the unit tests for the lower-level method validatePasswordMetrics() should be sufficient. + adminMetrics = new PasswordMetrics(CREDENTIAL_TYPE_NONE); + adminMetrics.length = 6; + password = LockscreenCredential.createPassword("pass"); + assertValidationErrors( + validateCredential(adminMetrics, PASSWORD_COMPLEXITY_LOW, password), + PasswordValidationError.TOO_SHORT, 6); + } + /** - * @param expected sequense of validation error codes followed by requirement values, must have + * @param expected sequence of validation error codes followed by requirement values, must have * even number of elements. Empty means no errors. */ private void assertValidationErrors( 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 a47868d0a5243..12abaa90b9dd8 100644 --- a/core/tests/coretests/src/com/android/internal/widget/LockscreenCredentialTest.java +++ b/core/tests/coretests/src/com/android/internal/widget/LockscreenCredentialTest.java @@ -16,52 +16,71 @@ package com.android.internal.widget; - import static com.google.common.truth.Truth.assertThat; -import android.test.AndroidTestCase; +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import androidx.test.ext.junit.runners.AndroidJUnit4; + +import org.junit.Test; +import org.junit.runner.RunWith; import java.util.Arrays; +@RunWith(AndroidJUnit4.class) +public class LockscreenCredentialTest { -public class LockscreenCredentialTest extends AndroidTestCase { + @Test + public void testNoneCredential() { + LockscreenCredential none = LockscreenCredential.createNone(); - public void testEmptyCredential() { - LockscreenCredential empty = LockscreenCredential.createNone(); + assertTrue(none.isNone()); + assertEquals(0, none.size()); + assertArrayEquals(new byte[0], none.getCredential()); - assertTrue(empty.isNone()); - assertEquals(0, empty.size()); - assertNotNull(empty.getCredential()); - - assertFalse(empty.isPin()); - assertFalse(empty.isPassword()); - assertFalse(empty.isPattern()); + assertFalse(none.isPin()); + assertFalse(none.isPassword()); + assertFalse(none.isPattern()); + assertFalse(none.hasInvalidChars()); + none.validateBasicRequirements(); } + @Test public void testPinCredential() { LockscreenCredential pin = LockscreenCredential.createPin("3456"); assertTrue(pin.isPin()); assertEquals(4, pin.size()); - assertTrue(Arrays.equals("3456".getBytes(), pin.getCredential())); + assertArrayEquals("3456".getBytes(), pin.getCredential()); assertFalse(pin.isNone()); assertFalse(pin.isPassword()); assertFalse(pin.isPattern()); + assertFalse(pin.hasInvalidChars()); + pin.validateBasicRequirements(); } + @Test public void testPasswordCredential() { LockscreenCredential password = LockscreenCredential.createPassword("password"); assertTrue(password.isPassword()); assertEquals(8, password.size()); - assertTrue(Arrays.equals("password".getBytes(), password.getCredential())); + assertArrayEquals("password".getBytes(), password.getCredential()); assertFalse(password.isNone()); assertFalse(password.isPin()); assertFalse(password.isPattern()); + assertFalse(password.hasInvalidChars()); + password.validateBasicRequirements(); } + @Test public void testPatternCredential() { LockscreenCredential pattern = LockscreenCredential.createPattern(Arrays.asList( LockPatternView.Cell.of(0, 0), @@ -73,13 +92,34 @@ public class LockscreenCredentialTest extends AndroidTestCase { assertTrue(pattern.isPattern()); assertEquals(5, pattern.size()); - assertTrue(Arrays.equals("12369".getBytes(), pattern.getCredential())); + assertArrayEquals("12369".getBytes(), pattern.getCredential()); assertFalse(pattern.isNone()); assertFalse(pattern.isPin()); assertFalse(pattern.isPassword()); + assertFalse(pattern.hasInvalidChars()); + pattern.validateBasicRequirements(); } + // Constructing a LockscreenCredential with a too-short length, even 0, should not throw an + // exception. This is because LockscreenCredential needs to be able to represent a request to + // set a credential that is too short. + @Test + public void testZeroLengthCredential() { + LockscreenCredential credential = LockscreenCredential.createPin(""); + assertTrue(credential.isPin()); + assertEquals(0, credential.size()); + + credential = createPattern(""); + assertTrue(credential.isPattern()); + assertEquals(0, credential.size()); + + credential = LockscreenCredential.createPassword(""); + assertTrue(credential.isPassword()); + assertEquals(0, credential.size()); + } + + @Test public void testPasswordOrNoneCredential() { assertEquals(LockscreenCredential.createNone(), LockscreenCredential.createPasswordOrNone(null)); @@ -89,6 +129,7 @@ public class LockscreenCredentialTest extends AndroidTestCase { LockscreenCredential.createPasswordOrNone("abcd")); } + @Test public void testPinOrNoneCredential() { assertEquals(LockscreenCredential.createNone(), LockscreenCredential.createPinOrNone(null)); @@ -98,6 +139,25 @@ public class LockscreenCredentialTest extends AndroidTestCase { LockscreenCredential.createPinOrNone("1357")); } + // Test that passwords containing invalid characters that were incorrectly allowed in + // Android 10–14 are still interpreted in the same way, but are not allowed for new passwords. + @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()); + try { + credential.validateBasicRequirements(); + fail("should not be able to set password with invalid chars"); + } catch (IllegalArgumentException expected) { } + } + } + + @Test public void testSanitize() { LockscreenCredential password = LockscreenCredential.createPassword("password"); password.zeroize(); @@ -122,12 +182,17 @@ public class LockscreenCredentialTest extends AndroidTestCase { 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"); } catch (IllegalStateException expected) { } } + @Test public void testEquals() { assertEquals(LockscreenCredential.createNone(), LockscreenCredential.createNone()); assertEquals(LockscreenCredential.createPassword("1234"), @@ -136,34 +201,40 @@ public class LockscreenCredentialTest extends AndroidTestCase { 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 public void testDuplicate() { LockscreenCredential credential; @@ -175,8 +246,13 @@ public class LockscreenCredentialTest extends AndroidTestCase { assertEquals(credential, credential.duplicate()); credential = createPattern("5678"); assertEquals(credential, credential.duplicate()); + + // Test that mHasInvalidChars is duplicated. + credential = LockscreenCredential.createPassword("™™™™"); + assertEquals(credential, credential.duplicate()); } + @Test public void testPasswordToHistoryHash() { String password = "1234"; LockscreenCredential credential = LockscreenCredential.createPassword(password); @@ -193,6 +269,7 @@ public class LockscreenCredentialTest extends AndroidTestCase { .isEqualTo(expectedHash); } + @Test public void testPasswordToHistoryHashInvalidInput() { String password = "1234"; LockscreenCredential credential = LockscreenCredential.createPassword(password); @@ -221,6 +298,7 @@ public class LockscreenCredentialTest extends AndroidTestCase { .isNull(); } + @Test public void testLegacyPasswordToHash() { String password = "1234"; String salt = "6d5331dd120077a0"; @@ -233,6 +311,7 @@ public class LockscreenCredentialTest extends AndroidTestCase { .isEqualTo(expectedHash); } + @Test public void testLegacyPasswordToHashInvalidInput() { String password = "1234"; String salt = "6d5331dd120077a0"; diff --git a/services/core/java/com/android/server/locksettings/LockSettingsService.java b/services/core/java/com/android/server/locksettings/LockSettingsService.java index 7bb0489467b40..97dc062006694 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsService.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsService.java @@ -1634,6 +1634,7 @@ public class LockSettingsService extends ILockSettings.Stub { + PERMISSION); } } + credential.validateBasicRequirements(); final long identity = Binder.clearCallingIdentity(); try { @@ -3069,6 +3070,7 @@ public class LockSettingsService extends ILockSettings.Stub { private boolean setLockCredentialWithToken(LockscreenCredential credential, long tokenHandle, byte[] token, int userId) { boolean result; + credential.validateBasicRequirements(); synchronized (mSpManager) { if (!mSpManager.hasEscrowData(userId)) { throw new SecurityException("Escrow token is disabled on the current user"); diff --git a/services/core/java/com/android/server/locksettings/LockSettingsShellCommand.java b/services/core/java/com/android/server/locksettings/LockSettingsShellCommand.java index f107d0bf99327..df95c69e7271c 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsShellCommand.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsShellCommand.java @@ -16,8 +16,6 @@ package com.android.server.locksettings; -import static com.android.internal.widget.LockPatternUtils.CREDENTIAL_TYPE_NONE; -import static com.android.internal.widget.LockPatternUtils.CREDENTIAL_TYPE_PATTERN; import static com.android.internal.widget.LockPatternUtils.StrongAuthTracker.STRONG_AUTH_REQUIRED_AFTER_USER_LOCKDOWN; import android.app.ActivityManager; @@ -313,16 +311,8 @@ class LockSettingsShellCommand extends ShellCommand { mLockPatternUtils.getRequestedPasswordMetrics(mCurrentUserId); final int requiredComplexity = mLockPatternUtils.getRequestedPasswordComplexity(mCurrentUserId); - final List errors; - if (credential.isPassword() || credential.isPin()) { - errors = PasswordMetrics.validatePassword(requiredMetrics, requiredComplexity, - credential.isPin(), credential.getCredential()); - } else { - PasswordMetrics metrics = new PasswordMetrics( - credential.isPattern() ? CREDENTIAL_TYPE_PATTERN : CREDENTIAL_TYPE_NONE); - errors = PasswordMetrics.validatePasswordMetrics( - requiredMetrics, requiredComplexity, metrics); - } + final List errors = + PasswordMetrics.validateCredential(requiredMetrics, requiredComplexity, credential); if (!errors.isEmpty()) { getOutPrintWriter().println( "New credential doesn't satisfy admin policies: " + errors.get(0)); diff --git a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java index a452328599e34..6ea71e382a716 100644 --- a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java +++ b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java @@ -5728,20 +5728,17 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { final int callingUid = caller.getUid(); final int userHandle = UserHandle.getUserId(callingUid); final boolean isPin = PasswordMetrics.isNumericOnly(password); + final LockscreenCredential newCredential; + if (isPin) { + newCredential = LockscreenCredential.createPin(password); + } else { + newCredential = LockscreenCredential.createPasswordOrNone(password); + } synchronized (getLockObject()) { final PasswordMetrics minMetrics = getPasswordMinimumMetricsUnchecked(userHandle); - final List validationErrors; final int complexity = getAggregatedPasswordComplexityLocked(userHandle); - // TODO: Consider changing validation API to take LockscreenCredential. - if (password.isEmpty()) { - validationErrors = PasswordMetrics.validatePasswordMetrics( - minMetrics, complexity, new PasswordMetrics(CREDENTIAL_TYPE_NONE)); - } else { - // TODO(b/120484642): remove getBytes() below - validationErrors = PasswordMetrics.validatePassword( - minMetrics, complexity, isPin, password.getBytes()); - } - + final List validationErrors = + PasswordMetrics.validateCredential(minMetrics, complexity, newCredential); if (!validationErrors.isEmpty()) { Slogf.w(LOG_TAG, "Failed to reset password due to constraint violation: %s", validationErrors.get(0)); @@ -5765,12 +5762,6 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { // Don't do this with the lock held, because it is going to call // back in to the service. final long ident = mInjector.binderClearCallingIdentity(); - final LockscreenCredential newCredential; - if (isPin) { - newCredential = LockscreenCredential.createPin(password); - } else { - newCredential = LockscreenCredential.createPasswordOrNone(password); - } try { if (tokenHandle == 0 || token == null) { if (!mLockPatternUtils.setLockCredential(newCredential, diff --git a/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsServiceTests.java b/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsServiceTests.java index 60a033fde4270..5a62d92e8e127 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsServiceTests.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsServiceTests.java @@ -77,6 +77,26 @@ public class LockSettingsServiceTests extends BaseLockSettingsServiceTests { testSetCredentialFailsWithoutLockScreen(PRIMARY_USER_ID, newPassword("password")); } + @Test(expected = IllegalArgumentException.class) + public void testSetTooShortPatternFails() throws RemoteException { + mService.setLockCredential(newPattern("123"), nonePassword(), PRIMARY_USER_ID); + } + + @Test(expected = IllegalArgumentException.class) + public void testSetTooShortPinFails() throws RemoteException { + mService.setLockCredential(newPin("123"), nonePassword(), PRIMARY_USER_ID); + } + + @Test(expected = IllegalArgumentException.class) + public void testSetTooShortPassword() throws RemoteException { + mService.setLockCredential(newPassword("123"), nonePassword(), PRIMARY_USER_ID); + } + + @Test(expected = IllegalArgumentException.class) + public void testSetPasswordWithInvalidChars() throws RemoteException { + mService.setLockCredential(newPassword("§µ¿¶¥£"), nonePassword(), PRIMARY_USER_ID); + } + @Test public void testSetPatternPrimaryUser() throws RemoteException { setAndVerifyCredential(PRIMARY_USER_ID, newPattern("123456789")); @@ -94,7 +114,7 @@ public class LockSettingsServiceTests extends BaseLockSettingsServiceTests { @Test public void testChangePatternPrimaryUser() throws RemoteException { - testChangeCredential(PRIMARY_USER_ID, newPassword("!£$%^&*(())"), newPattern("1596321")); + testChangeCredential(PRIMARY_USER_ID, newPassword("password"), newPattern("1596321")); } @Test @@ -185,7 +205,7 @@ public class LockSettingsServiceTests extends BaseLockSettingsServiceTests { assertNotNull(mGateKeeperService.getAuthToken(MANAGED_PROFILE_USER_ID)); assertEquals(profileSid, mGateKeeperService.getSecureUserId(MANAGED_PROFILE_USER_ID)); - setCredential(PRIMARY_USER_ID, newPassword("pwd"), primaryPassword); + setCredential(PRIMARY_USER_ID, newPassword("password"), primaryPassword); assertEquals(VerifyCredentialResponse.RESPONSE_OK, mService.verifyCredential( profilePassword, MANAGED_PROFILE_USER_ID, 0 /* flags */) .getResponseCode()); diff --git a/services/tests/servicestests/src/com/android/server/locksettings/SyntheticPasswordTests.java b/services/tests/servicestests/src/com/android/server/locksettings/SyntheticPasswordTests.java index ce0347dbe4acf..dee77806e4f37 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/SyntheticPasswordTests.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/SyntheticPasswordTests.java @@ -215,12 +215,12 @@ public class SyntheticPasswordTests extends BaseLockSettingsServiceTests { @Test public void testChangeCredentialKeepsAuthSecret() throws RemoteException { LockscreenCredential password = newPassword("password"); - LockscreenCredential badPassword = newPassword("new"); + LockscreenCredential newPassword = newPassword("newPassword"); initSpAndSetCredential(PRIMARY_USER_ID, password); - mService.setLockCredential(badPassword, password, PRIMARY_USER_ID); + mService.setLockCredential(newPassword, password, PRIMARY_USER_ID); assertEquals(VerifyCredentialResponse.RESPONSE_OK, mService.verifyCredential( - badPassword, PRIMARY_USER_ID, 0 /* flags */).getResponseCode()); + newPassword, PRIMARY_USER_ID, 0 /* flags */).getResponseCode()); // Check the same secret was passed each time ArgumentCaptor secret = ArgumentCaptor.forClass(byte[].class);