From 7cf4509c31f3dc1c32f89c26867a50c4ed0d5618 Mon Sep 17 00:00:00 2001 From: Rubin Xu Date: Mon, 28 Aug 2017 11:47:35 +0100 Subject: [PATCH] Fix resetPasswordWithToken before user unlock 1. Fix system server crash when resetPasswordWithToken is called before use unlock, due to DPMS enforces user is unlocked when calculating password sufficiency. 2. Propogate new password metric from LockSettingsService to DPMS after a password reset with token, and fix a bug where stale quality was used. Bug: 64923343 Bug: 64928518 Bug: 65286643 Test: cts-tradefed run cts-dev -m CtsDevicePolicyManagerTestCases -t com.android.cts.devicepolicy.ManagedProfileTest#testResetPasswordWithTokenBeforeUnlock Test: cts-tradefed run cts-dev -m CtsDevicePolicyManagerTestCases -t com.android.cts.devicepolicy.MixedManagedProfileOwnerTest#testResetPasswordWithToken Test: runtest frameworks-services -p com.android.server.locksettings Test: cts-tradefed run cts-dev -m CtsDevicePolicyManagerTestCases -t com.android.cts.devicepolicy.DeviceAdminHostSideTestApi24#testRunDeviceOwnerPasswordTest Test: runtest frameworks-core -c android.app.admin.PasswordMetricsTest Test: runtest frameworks-services -c com.android.server.devicepolicy.DevicePolicyManagerTest Change-Id: Ibb3736547b3b36da4a8a67af711e08a38427aa56 --- .../android/app/admin/DevicePolicyManager.java | 2 +- core/java/android/app/admin/PasswordMetrics.java | 20 +++++++-- .../android/internal/widget/LockPatternUtils.java | 49 +++++++++++++++++----- .../src/android/app/admin/PasswordMetricsTest.java | 45 ++++++++++++++++++++ .../server/locksettings/LockSettingsService.java | 17 ++++++-- .../devicepolicy/DevicePolicyManagerService.java | 13 ++++-- .../devicepolicy/DevicePolicyManagerTest.java | 3 +- .../locksettings/LockSettingsServiceTestable.java | 3 +- .../locksettings/SyntheticPasswordTests.java | 15 +++++-- 9 files changed, 141 insertions(+), 26 deletions(-) diff --git a/core/java/android/app/admin/DevicePolicyManager.java b/core/java/android/app/admin/DevicePolicyManager.java index 56123a72d00e..121b58a2b104 100644 --- a/core/java/android/app/admin/DevicePolicyManager.java +++ b/core/java/android/app/admin/DevicePolicyManager.java @@ -2531,7 +2531,7 @@ public class DevicePolicyManager { * @return Returns true if the password meets the current requirements, else false. * @throws SecurityException if the calling application does not own an active administrator * that uses {@link DeviceAdminInfo#USES_POLICY_LIMIT_PASSWORD} - * @throws InvalidStateException if the user is not unlocked. + * @throws IllegalStateException if the user is not unlocked. */ public boolean isActivePasswordSufficient() { if (mService != null) { diff --git a/core/java/android/app/admin/PasswordMetrics.java b/core/java/android/app/admin/PasswordMetrics.java index ea3f560d02db..4658a47444f9 100644 --- a/core/java/android/app/admin/PasswordMetrics.java +++ b/core/java/android/app/admin/PasswordMetrics.java @@ -18,13 +18,11 @@ package android.app.admin; import android.annotation.IntDef; import android.annotation.NonNull; -import android.app.admin.DevicePolicyManager; -import android.os.Parcelable; import android.os.Parcel; +import android.os.Parcelable; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; -import java.io.IOException; /** * A class that represents the metrics of a password that are used to decide whether or not a @@ -159,6 +157,22 @@ public class PasswordMetrics implements Parcelable { quality, length, letters, upperCase, lowerCase, numeric, symbols, nonLetter); } + @Override + public boolean equals(Object other) { + if (!(other instanceof PasswordMetrics)) { + return false; + } + PasswordMetrics o = (PasswordMetrics) other; + return this.quality == o.quality + && this.length == o.length + && this.letters == o.letters + && this.upperCase == o.upperCase + && this.lowerCase == o.lowerCase + && this.numeric == o.numeric + && this.symbols == o.symbols + && this.nonLetter == o.nonLetter; + } + /* * Returns the maximum length of a sequential characters. A sequence is defined as * monotonically increasing characters with a constant interval or the same character repeated. diff --git a/core/java/com/android/internal/widget/LockPatternUtils.java b/core/java/com/android/internal/widget/LockPatternUtils.java index 0d1575805ee9..f85333eb9588 100644 --- a/core/java/com/android/internal/widget/LockPatternUtils.java +++ b/core/java/com/android/internal/widget/LockPatternUtils.java @@ -803,12 +803,14 @@ public class LockPatternUtils { + "of length " + MIN_LOCK_PASSWORD_SIZE); } - final int computedQuality = PasswordMetrics.computeForPassword(password).quality; - setLong(PASSWORD_TYPE_KEY, Math.max(requestedQuality, computedQuality), userHandle); + setLong(PASSWORD_TYPE_KEY, + computePasswordQuality(CREDENTIAL_TYPE_PASSWORD, password, requestedQuality), + userHandle); getLockSettings().setLockCredential(password, CREDENTIAL_TYPE_PASSWORD, savedPassword, requestedQuality, userHandle); - updateEncryptionPasswordIfNeeded(password, computedQuality, userHandle); + updateEncryptionPasswordIfNeeded(password, + PasswordMetrics.computeForPassword(password).quality, userHandle); updatePasswordHistory(password, userHandle); } catch (RemoteException re) { // Cant do much @@ -898,6 +900,24 @@ public class LockPatternUtils { } /** + * Returns the password quality of the given credential, promoting it to a higher level + * if DevicePolicyManager has a stronger quality requirement. This value will be written + * to PASSWORD_TYPE_KEY. + */ + private int computePasswordQuality(int type, String credential, int requestedQuality) { + final int quality; + if (type == CREDENTIAL_TYPE_PASSWORD) { + int computedQuality = PasswordMetrics.computeForPassword(credential).quality; + quality = Math.max(requestedQuality, computedQuality); + } else if (type == CREDENTIAL_TYPE_PATTERN) { + quality = DevicePolicyManager.PASSWORD_QUALITY_SOMETHING; + } else /* if (type == CREDENTIAL_TYPE_NONE) */ { + quality = DevicePolicyManager.PASSWORD_QUALITY_UNSPECIFIED; + } + return quality; + } + + /** * Enables/disables the Separate Profile Challenge for this {@param userHandle}. This is a no-op * for user handles that do not belong to a managed profile. * @@ -1505,25 +1525,34 @@ public class LockPatternUtils { } } - public boolean setLockCredentialWithToken(String credential, int type, long tokenHandle, - byte[] token, int userId) { + /** + * Change a user's lock credential with a pre-configured escrow token. + * + * @param credential The new credential to be set + * @param type Credential type: password / pattern / none. + * @param requestedQuality the requested password quality by DevicePolicyManager. + * See {@link DevicePolicyManager#getPasswordQuality(android.content.ComponentName)} + * @param tokenHandle Handle of the escrow token + * @param token Escrow token + * @param userId The user who's lock credential to be changed + * @return {@code true} if the operation is successful. + */ + public boolean setLockCredentialWithToken(String credential, int type, int requestedQuality, + long tokenHandle, byte[] token, int userId) { try { if (type != CREDENTIAL_TYPE_NONE) { if (TextUtils.isEmpty(credential) || credential.length() < MIN_LOCK_PASSWORD_SIZE) { throw new IllegalArgumentException("password must not be null and at least " + "of length " + MIN_LOCK_PASSWORD_SIZE); } - - final int computedQuality = PasswordMetrics.computeForPassword(credential).quality; - int quality = Math.max(DevicePolicyManager.PASSWORD_QUALITY_NUMERIC, - computedQuality); + final int quality = computePasswordQuality(type, credential, requestedQuality); if (!getLockSettings().setLockCredentialWithToken(credential, type, tokenHandle, token, quality, userId)) { return false; } setLong(PASSWORD_TYPE_KEY, quality, userId); - updateEncryptionPasswordIfNeeded(credential, computedQuality, userId); + updateEncryptionPasswordIfNeeded(credential, quality, userId); updatePasswordHistory(credential, userId); } else { if (!TextUtils.isEmpty(credential)) { diff --git a/core/tests/coretests/src/android/app/admin/PasswordMetricsTest.java b/core/tests/coretests/src/android/app/admin/PasswordMetricsTest.java index 2470e87666a9..d289f1f5defc 100644 --- a/core/tests/coretests/src/android/app/admin/PasswordMetricsTest.java +++ b/core/tests/coretests/src/android/app/admin/PasswordMetricsTest.java @@ -17,6 +17,7 @@ package android.app.admin; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; import android.os.Parcel; import android.support.test.filters.SmallTest; @@ -119,4 +120,48 @@ public class PasswordMetricsTest { // ordered, but not composed of alphas or digits assertEquals(1, PasswordMetrics.maxLengthSequence(":;<=>")); } + + @Test + public void testEquals() { + PasswordMetrics metrics0 = new PasswordMetrics(); + PasswordMetrics metrics1 = new PasswordMetrics(); + assertNotEquals(metrics0, null); + assertNotEquals(metrics0, new Object()); + assertEquals(metrics0, metrics0); + assertEquals(metrics0, metrics1); + + assertEquals(new PasswordMetrics(DevicePolicyManager.PASSWORD_QUALITY_SOMETHING, 4), + new PasswordMetrics(DevicePolicyManager.PASSWORD_QUALITY_SOMETHING, 4)); + + assertNotEquals(new PasswordMetrics(DevicePolicyManager.PASSWORD_QUALITY_SOMETHING, 4), + new PasswordMetrics(DevicePolicyManager.PASSWORD_QUALITY_SOMETHING, 5)); + + assertNotEquals(new PasswordMetrics(DevicePolicyManager.PASSWORD_QUALITY_SOMETHING, 4), + new PasswordMetrics(DevicePolicyManager.PASSWORD_QUALITY_COMPLEX, 4)); + + metrics0 = PasswordMetrics.computeForPassword("1234abcd,./"); + metrics1 = PasswordMetrics.computeForPassword("1234abcd,./"); + assertEquals(metrics0, metrics1); + metrics1.letters++; + assertNotEquals(metrics0, metrics1); + metrics1.letters--; + metrics1.upperCase++; + assertNotEquals(metrics0, metrics1); + metrics1.upperCase--; + metrics1.lowerCase++; + assertNotEquals(metrics0, metrics1); + metrics1.lowerCase--; + metrics1.numeric++; + assertNotEquals(metrics0, metrics1); + metrics1.numeric--; + metrics1.symbols++; + assertNotEquals(metrics0, metrics1); + metrics1.symbols--; + metrics1.nonLetter++; + assertNotEquals(metrics0, metrics1); + metrics1.nonLetter--; + assertEquals(metrics0, metrics1); + + + } } diff --git a/services/core/java/com/android/server/locksettings/LockSettingsService.java b/services/core/java/com/android/server/locksettings/LockSettingsService.java index a1b84568943f..018b5fa45a5a 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsService.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsService.java @@ -154,7 +154,8 @@ public class LockSettingsService extends ILockSettings.Stub { private final Injector mInjector; private final Context mContext; - private final Handler mHandler; + @VisibleForTesting + protected final Handler mHandler; @VisibleForTesting protected final LockSettingsStorage mStorage; private final LockSettingsStrongAuth mStrongAuth; @@ -1736,6 +1737,10 @@ public class LockSettingsService extends ILockSettings.Stub { return response; } + /** + * Call this method to notify DPMS regarding the latest password metric. This should be called + * when the user is authenticating or when a new password is being set. + */ private void notifyActivePasswordMetricsAvailable(String password, @UserIdInt int userId) { final PasswordMetrics metrics; if (password == null) { @@ -2197,6 +2202,8 @@ public class LockSettingsService extends ILockSettings.Stub { } setLong(SYNTHETIC_PASSWORD_HANDLE_KEY, newHandle, userId); synchronizeUnifiedWorkChallengeForProfiles(userId, profilePasswords); + + notifyActivePasswordMetricsAvailable(credential, userId); return newHandle; } @@ -2246,13 +2253,13 @@ public class LockSettingsService extends ILockSettings.Stub { userId); synchronizeUnifiedWorkChallengeForProfiles(userId, null); mSpManager.destroyPasswordBasedSyntheticPassword(handle, userId); + + notifyActivePasswordMetricsAvailable(credential, userId); } else /* response == null || responseCode == VerifyCredentialResponse.RESPONSE_RETRY */ { Slog.w(TAG, "spBasedSetLockCredentialInternalLocked: " + (response != null ? "rate limit exceeded" : "failed")); return; } - notifyActivePasswordMetricsAvailable(credential, userId); - } @Override @@ -2358,6 +2365,10 @@ public class LockSettingsService extends ILockSettings.Stub { Slog.w(TAG, "Invalid escrow token supplied"); return false; } + // Update PASSWORD_TYPE_KEY since it's needed by notifyActivePasswordMetricsAvailable() + // called by setLockCredentialWithAuthTokenLocked(). + // TODO: refactor usage of PASSWORD_TYPE_KEY b/65239740 + setLong(LockPatternUtils.PASSWORD_TYPE_KEY, requestedQuality, userId); long oldHandle = getSyntheticPasswordHandleLocked(userId); setLockCredentialWithAuthTokenLocked(credential, type, result.authToken, requestedQuality, userId); diff --git a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java index 312327e552e9..0b967b31f4b5 100644 --- a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java +++ b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java @@ -4077,6 +4077,7 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { return true; } enforceFullCrossUsersPermission(userHandle); + enforceUserUnlocked(userHandle, parent); synchronized (this) { // This API can only be called by an active device admin, @@ -4096,7 +4097,8 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { enforceManagedProfile(userHandle, "call APIs refering to the parent profile"); synchronized (this) { - int targetUser = getProfileParentId(userHandle); + final int targetUser = getProfileParentId(userHandle); + enforceUserUnlocked(targetUser, false); DevicePolicyData policy = getUserDataUnchecked(getCredentialOwner(userHandle, false)); return isActivePasswordSufficientForUserLocked(policy, targetUser, false); } @@ -4104,8 +4106,6 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { private boolean isActivePasswordSufficientForUserLocked( DevicePolicyData policy, int userHandle, boolean parent) { - enforceUserUnlocked(userHandle, parent); - if (!mInjector.storageManagerIsFileBasedEncryptionEnabled() && !policy.mPasswordStateHasBeenSetSinceBoot) { // Before user enters their password for the first time after a reboot, return the @@ -4438,7 +4438,7 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { result = mLockPatternUtils.setLockCredentialWithToken(password, TextUtils.isEmpty(password) ? LockPatternUtils.CREDENTIAL_TYPE_NONE : LockPatternUtils.CREDENTIAL_TYPE_PASSWORD, - tokenHandle, token, userHandle); + quality, tokenHandle, token, userHandle); } boolean requireEntry = (flags & DevicePolicyManager.RESET_PASSWORD_REQUIRE_ENTRY) != 0; if (requireEntry) { @@ -5442,6 +5442,11 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { } } + /** + * Notify DPMS regarding the metric of the current password. This happens when the user changes + * the password, but also when the user just unlocks the keyguard. In comparison, + * reportPasswordChanged() is only called when the user changes the password. + */ @Override public void setActivePasswordState(PasswordMetrics metrics, int userHandle) { if (!mHasFeature) { diff --git a/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerTest.java b/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerTest.java index e3faa5280859..96d9605d3a3d 100644 --- a/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerTest.java +++ b/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerTest.java @@ -3653,7 +3653,8 @@ public class DevicePolicyManagerTest extends DpmTestBase { // test reset password with token when(getServices().lockPatternUtils.setLockCredentialWithToken(eq(password), - eq(LockPatternUtils.CREDENTIAL_TYPE_PASSWORD), eq(handle), eq(token), + eq(LockPatternUtils.CREDENTIAL_TYPE_PASSWORD), + eq(DevicePolicyManager.PASSWORD_QUALITY_UNSPECIFIED), eq(handle), eq(token), eq(UserHandle.USER_SYSTEM))) .thenReturn(true); assertTrue(dpm.resetPasswordWithToken(admin1, password, token, 0)); diff --git a/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsServiceTestable.java b/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsServiceTestable.java index 0df834f0469e..0916a335b51e 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsServiceTestable.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsServiceTestable.java @@ -21,6 +21,7 @@ import static org.mockito.Mockito.mock; import android.app.IActivityManager; import android.content.Context; import android.os.Handler; +import android.os.Looper; import android.os.Process; import android.os.RemoteException; import android.os.storage.IStorageManager; @@ -56,7 +57,7 @@ public class LockSettingsServiceTestable extends LockSettingsService { @Override public Handler getHandler() { - return mock(Handler.class); + return new Handler(Looper.getMainLooper()); } @Override 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 fd77de344aa6..2c9aa9d6a245 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/SyntheticPasswordTests.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/SyntheticPasswordTests.java @@ -23,8 +23,9 @@ import static android.app.admin.DevicePolicyManager.PASSWORD_QUALITY_UNSPECIFIED import static com.android.internal.widget.LockPatternUtils.CREDENTIAL_TYPE_PASSWORD; import static com.android.internal.widget.LockPatternUtils.SYNTHETIC_PASSWORD_ENABLED_KEY; import static com.android.internal.widget.LockPatternUtils.SYNTHETIC_PASSWORD_HANDLE_KEY; +import static org.mockito.Mockito.verify; -import android.app.admin.DevicePolicyManager; +import android.app.admin.PasswordMetrics; import android.os.RemoteException; import android.os.UserHandle; @@ -272,14 +273,22 @@ public class SyntheticPasswordTests extends BaseLockSettingsServiceTests { long handle = mService.addEscrowToken(TOKEN.getBytes(), PRIMARY_USER_ID); assertFalse(mService.isEscrowTokenActive(handle, PRIMARY_USER_ID)); - mService.verifyCredential(PASSWORD, LockPatternUtils.CREDENTIAL_TYPE_PASSWORD, 0, PRIMARY_USER_ID).getResponseCode(); + mService.verifyCredential(PASSWORD, LockPatternUtils.CREDENTIAL_TYPE_PASSWORD, 0, + PRIMARY_USER_ID).getResponseCode(); assertTrue(mService.isEscrowTokenActive(handle, PRIMARY_USER_ID)); mService.setLockCredentialWithToken(PATTERN, LockPatternUtils.CREDENTIAL_TYPE_PATTERN, handle, TOKEN.getBytes(), PASSWORD_QUALITY_SOMETHING, PRIMARY_USER_ID); + // Verify DPM gets notified about new device lock + mService.mHandler.runWithScissors(() -> {}, 0 /*now*/); // Flush runnables on handler + PasswordMetrics metric = PasswordMetrics.computeForPassword(PATTERN); + metric.quality = PASSWORD_QUALITY_SOMETHING; + verify(mDevicePolicyManager).setActivePasswordState(metric, PRIMARY_USER_ID); + assertEquals(VerifyCredentialResponse.RESPONSE_OK, - mService.verifyCredential(PATTERN, LockPatternUtils.CREDENTIAL_TYPE_PATTERN, 0, PRIMARY_USER_ID).getResponseCode()); + mService.verifyCredential(PATTERN, LockPatternUtils.CREDENTIAL_TYPE_PATTERN, 0, + PRIMARY_USER_ID).getResponseCode()); assertArrayEquals(storageKey, mStorageManager.getUserUnlockToken(PRIMARY_USER_ID)); } -- 2.11.0