Make LockSettingsService enforce basic requirements for new credentials

Currently all LSKF requirements are enforced by
PasswordMetrics#validateCredential().  The standard minimum length of 4
is also checked again in LockPatternUtils#setLockCredential().

These are both at the caller's option, though.  These requirements could
be circumvented by calling ILockSettings#setLockCredential() directly.

Therefore, to provide higher assurance that at least the standard
requirements are met, this CL moves the standard length check into
LockSettingsService and also adds the invalid chars check alongside it.

Bug: 219511761
Bug: 232900169
Bug: 243881358
Test: atest LockscreenCredentialTest
Test: atest com.android.server.locksettings
Change-Id: Icc48a0d6caac0884bf3e3a9181828e8dfffff7e4
Merged-In: Icc48a0d6caac0884bf3e3a9181828e8dfffff7e4
(cherry picked from commit fe59a023)

core/java/com/android/internal/widget/LockPatternUtils.java

@@ -96,7 +96,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;
 
@@ -771,7 +771,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) {
@@ -779,7 +778,6 @@ public class LockPatternUtils {
             throw new UnsupportedOperationException(
                     "This operation requires the lock screen feature.");
         }
-        newCredential.checkLength();
 
         try {
             if (!getLockSettings().setLockCredential(newCredential, savedCredential, userHandle)) {
@@ -1543,7 +1541,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);

core/java/com/android/internal/widget/LockscreenCredential.java

@@ -253,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;
         }
     }
 

core/tests/coretests/src/com/android/internal/widget/LockscreenCredentialTest.java

@@ -47,6 +47,7 @@ public class LockscreenCredentialTest {
         assertFalse(none.isPassword());
         assertFalse(none.isPattern());
         assertFalse(none.hasInvalidChars());
+        none.validateBasicRequirements();
     }
 
     @Test
@@ -61,6 +62,7 @@ public class LockscreenCredentialTest {
         assertFalse(pin.isPassword());
         assertFalse(pin.isPattern());
         assertFalse(pin.hasInvalidChars());
+        pin.validateBasicRequirements();
     }
 
     @Test
@@ -75,6 +77,7 @@ public class LockscreenCredentialTest {
         assertFalse(password.isPin());
         assertFalse(password.isPattern());
         assertFalse(password.hasInvalidChars());
+        password.validateBasicRequirements();
     }
 
     @Test
@@ -95,6 +98,7 @@ public class LockscreenCredentialTest {
         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
@@ -136,7 +140,7 @@ public class LockscreenCredentialTest {
     }
 
     // Test that passwords containing invalid characters that were incorrectly allowed in
-    // Android 10–14 are still interpreted in the same way.
+    // 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.
@@ -146,6 +150,10 @@ public class LockscreenCredentialTest {
             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) { }
         }
     }
 

services/core/java/com/android/server/locksettings/LockSettingsService.java

@@ -1676,6 +1676,7 @@ public class LockSettingsService extends ILockSettings.Stub {
                                 + PERMISSION);
             }
         }
+        credential.validateBasicRequirements();
 
         final long identity = Binder.clearCallingIdentity();
         try {
@@ -3105,6 +3106,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");

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

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<byte[]> secret = ArgumentCaptor.forClass(byte[].class);