From bd3558749e71f323e656ad8fedb5ee99155af15b Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 22 Jul 2022 19:01:03 +0000 Subject: [PATCH] Remove unused and insecure fallback to legacy password history hash Since users with an LSKF now always have a synthetic password, the hashFactor needed by passwordToHistoryHash() is always available. Therefore, new hashes in the password history always use passwordToHistoryHash(), and the fallback to legacyPasswordToHash() is unused. Also, since the legacy algorithm can be easily bruteforced, falling back to it would be a security vulnerability. Therefore, remove this dangerous and unnecessary code. To make it clear that hashFactor is always available, also move the call to updatePasswordHistory() into setLockCredentialWithSpLocked(), where the SP is available. This makes it so that the SP doesn't need to be unwrapped by updatePasswordHistory(). This shouldn't have failed anyway, but this avoids needing to consider this case at all. For now, legacyPasswordToHash() itself is still needed for checking the password history on devices that have legacy hashes in their database. However, remove one of its two overloads that is no longer needed. Finally, add a couple unit tests, as the password history functionality didn't have any unit tests. Test: atest com.android.server.locksettings Test: atest LockscreenCredentialTest Change-Id: Ib48f05fba2e63397a89da2c323b60a4641852827 --- .../internal/widget/LockscreenCredential.java | 24 +++------- .../widget/LockscreenCredentialTest.java | 6 --- .../locksettings/LockSettingsService.java | 29 +++++------ .../LockSettingsServiceTests.java | 48 ++++++++++++++++++- 4 files changed, 69 insertions(+), 38 deletions(-) diff --git a/core/java/com/android/internal/widget/LockscreenCredential.java b/core/java/com/android/internal/widget/LockscreenCredential.java index 40164a45516e6..c6125fb217a92 100644 --- a/core/java/com/android/internal/widget/LockscreenCredential.java +++ b/core/java/com/android/internal/widget/LockscreenCredential.java @@ -290,25 +290,15 @@ public class LockscreenCredential implements Parcelable, AutoCloseable { } /** - * Generate a hash for the given password. To avoid brute force attacks, we use a salted hash. - * Not the most secure, but it is at least a second level of protection. First level is that - * the file is in a location only readable by the system process. + * Hash the given password for the password history, using the legacy algorithm. * - * @return the hash of the pattern in a byte array. - */ - public String legacyPasswordToHash(byte[] salt) { - return legacyPasswordToHash(mCredential, salt); - } - - /** - * Generate a hash for the given password. To avoid brute force attacks, we use a salted hash. - * Not the most secure, but it is at least a second level of protection. First level is that - * the file is in a location only readable by the system process. - * - * @param password the gesture pattern. - * - * @return the hash of the pattern in a byte array. + * @deprecated This algorithm is insecure because the password can be easily bruteforced, given + * the hash and salt. Use {@link #passwordToHistoryHash(byte[], byte[], byte[])} + * instead, which incorporates an SP-derived secret into the hash. + * + * @return the legacy password hash */ + @Deprecated public static String legacyPasswordToHash(byte[] password, byte[] salt) { if (password == null || password.length == 0 || salt == null) { return null; 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 9d77d16b4a1df..a47868d0a5243 100644 --- a/core/tests/coretests/src/com/android/internal/widget/LockscreenCredentialTest.java +++ b/core/tests/coretests/src/com/android/internal/widget/LockscreenCredentialTest.java @@ -223,14 +223,10 @@ public class LockscreenCredentialTest extends AndroidTestCase { public void testLegacyPasswordToHash() { String password = "1234"; - LockscreenCredential credential = LockscreenCredential.createPassword(password); String salt = "6d5331dd120077a0"; String expectedHash = "2DD04348ADBF8F4CABD7F722DC2E2887FAD4B6020A0C3E02C831E09946F0554FDC13B155"; - assertThat( - credential.legacyPasswordToHash(salt.getBytes())) - .isEqualTo(expectedHash); assertThat( LockscreenCredential.legacyPasswordToHash( password.getBytes(), salt.getBytes())) @@ -239,10 +235,8 @@ public class LockscreenCredentialTest extends AndroidTestCase { public void testLegacyPasswordToHashInvalidInput() { String password = "1234"; - LockscreenCredential credential = LockscreenCredential.createPassword(password); String salt = "6d5331dd120077a0"; - assertThat(credential.legacyPasswordToHash(/* salt= */ null)).isNull(); assertThat(LockscreenCredential.legacyPasswordToHash( password.getBytes(), /* salt= */ null)).isNull(); diff --git a/services/core/java/com/android/server/locksettings/LockSettingsService.java b/services/core/java/com/android/server/locksettings/LockSettingsService.java index 2f6fd005fb824..9b6d0e7c09e49 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsService.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsService.java @@ -1626,7 +1626,7 @@ public class LockSettingsService extends ILockSettings.Stub { } onSyntheticPasswordKnown(userId, sp); - setLockCredentialWithSpLocked(credential, sp, userId); + setLockCredentialWithSpLocked(credential, sp, userId, isLockTiedToParent); sendCredentialsOnChangeIfRequired(credential, userId, isLockTiedToParent); return true; } @@ -1640,15 +1640,15 @@ public class LockSettingsService extends ILockSettings.Stub { if (newCredential.isPattern()) { setBoolean(LockPatternUtils.PATTERN_EVER_CHOSEN_KEY, true, userHandle); } - updatePasswordHistory(newCredential, userHandle); mContext.getSystemService(TrustManager.class).reportEnabledTrustAgentsChanged(userHandle); } /** - * Store the hash of the *current* password in the password history list, if device policy - * enforces password history requirement. + * Store the hash of the new password in the password history list, if device policy enforces + * a password history requirement. */ - private void updatePasswordHistory(LockscreenCredential password, int userHandle) { + private void updatePasswordHistory(SyntheticPassword sp, LockscreenCredential password, + int userHandle, boolean isLockTiedToParent) { if (password.isNone()) { return; } @@ -1656,8 +1656,11 @@ public class LockSettingsService extends ILockSettings.Stub { // Do not keep track of historical patterns return; } - // Add the password to the password history. We assume all - // password hashes have the same length for simplicity of implementation. + if (isLockTiedToParent) { + // Do not keep track of historical auto-generated profile passwords + return; + } + // Add the password to the password history. String passwordHistory = getString( LockPatternUtils.PASSWORD_HISTORY_KEY, /* defaultValue= */ null, userHandle); if (passwordHistory == null) { @@ -1667,13 +1670,9 @@ public class LockSettingsService extends ILockSettings.Stub { if (passwordHistoryLength == 0) { passwordHistory = ""; } else { - final byte[] hashFactor = getHashFactor(password, userHandle); + final byte[] hashFactor = sp.derivePasswordHashFactor(); final byte[] salt = getSalt(userHandle).getBytes(); String hash = password.passwordToHistoryHash(salt, hashFactor); - if (hash == null) { - Slog.e(TAG, "Compute new style password hash failed, fallback to legacy style"); - hash = password.legacyPasswordToHash(salt); - } if (TextUtils.isEmpty(passwordHistory)) { passwordHistory = hash; } else { @@ -2651,7 +2650,7 @@ public class LockSettingsService extends ILockSettings.Stub { */ @GuardedBy("mSpManager") private long setLockCredentialWithSpLocked(LockscreenCredential credential, - SyntheticPassword sp, int userId) { + SyntheticPassword sp, int userId, boolean isLockTiedToParent) { if (DEBUG) Slog.d(TAG, "setLockCredentialWithSpLocked: user=" + userId); final int savedCredentialType = getCredentialTypeInternal(userId); final long oldProtectorId = getCurrentLskfBasedProtectorId(userId); @@ -2689,6 +2688,7 @@ public class LockSettingsService extends ILockSettings.Stub { LockPatternUtils.invalidateCredentialTypeCache(); synchronizeUnifiedWorkChallengeForProfiles(userId, profilePasswords); + updatePasswordHistory(sp, credential, userId, isLockTiedToParent); setUserPasswordMetrics(credential, userId); mManagedProfilePasswordCache.removePassword(userId); if (savedCredentialType != CREDENTIAL_TYPE_NONE) { @@ -2934,7 +2934,8 @@ public class LockSettingsService extends ILockSettings.Stub { return false; } onSyntheticPasswordKnown(userId, result.syntheticPassword); - setLockCredentialWithSpLocked(credential, result.syntheticPassword, userId); + setLockCredentialWithSpLocked(credential, result.syntheticPassword, userId, + /* isLockTiedToParent= */ false); return true; } 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 9458b9bd5be08..a4cccb320ff17 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsServiceTests.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsServiceTests.java @@ -33,15 +33,18 @@ import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.never; import static org.mockito.Mockito.reset; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; import android.app.PropertyInvalidatedCache; import android.os.RemoteException; import android.platform.test.annotations.Presubmit; import android.service.gatekeeper.GateKeeperResponse; +import android.text.TextUtils; import androidx.test.filters.SmallTest; import androidx.test.runner.AndroidJUnit4; +import com.android.internal.widget.LockPatternUtils; import com.android.internal.widget.LockscreenCredential; import com.android.internal.widget.VerifyCredentialResponse; @@ -450,6 +453,45 @@ public class LockSettingsServiceTests extends BaseLockSettingsServiceTests { PRIMARY_USER_ID)); } + @Test + public void testPasswordHistoryDisabledByDefault() throws Exception { + final int userId = PRIMARY_USER_ID; + checkPasswordHistoryLength(userId, 0); + initializeStorageWithCredential(userId, nonePassword()); + checkPasswordHistoryLength(userId, 0); + + assertTrue(mService.setLockCredential(newPassword("1234"), nonePassword(), userId)); + checkPasswordHistoryLength(userId, 0); + } + + @Test + public void testPasswordHistoryLengthHonored() throws Exception { + final int userId = PRIMARY_USER_ID; + when(mDevicePolicyManager.getPasswordHistoryLength(any(), eq(userId))).thenReturn(3); + checkPasswordHistoryLength(userId, 0); + initializeStorageWithCredential(userId, nonePassword()); + checkPasswordHistoryLength(userId, 0); + + assertTrue(mService.setLockCredential(newPassword("pass1"), nonePassword(), userId)); + checkPasswordHistoryLength(userId, 1); + + assertTrue(mService.setLockCredential(newPassword("pass2"), newPassword("pass1"), userId)); + checkPasswordHistoryLength(userId, 2); + + assertTrue(mService.setLockCredential(newPassword("pass3"), newPassword("pass2"), userId)); + checkPasswordHistoryLength(userId, 3); + + // maximum length should have been reached + assertTrue(mService.setLockCredential(newPassword("pass4"), newPassword("pass3"), userId)); + checkPasswordHistoryLength(userId, 3); + } + + private void checkPasswordHistoryLength(int userId, int expectedLen) { + String history = mService.getString(LockPatternUtils.PASSWORD_HISTORY_KEY, "", userId); + String[] hashes = TextUtils.split(history, LockPatternUtils.PASSWORD_HISTORY_DELIMITER); + assertEquals(expectedLen, hashes.length); + } + private void testCreateCredential(int userId, LockscreenCredential credential) throws RemoteException { assertTrue(mService.setLockCredential(credential, nonePassword(), userId)); @@ -510,6 +552,10 @@ public class LockSettingsServiceTests extends BaseLockSettingsServiceTests { synchronized (mService.mSpManager) { mService.initializeSyntheticPasswordLocked(credential, userId); } - assertNotEquals(0, mGateKeeperService.getSecureUserId(userId)); + if (credential.isNone()) { + assertEquals(0, mGateKeeperService.getSecureUserId(userId)); + } else { + assertNotEquals(0, mGateKeeperService.getSecureUserId(userId)); + } } }