From f0832139536abeb23c39f2b502278a348150c0e6 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Thu, 4 Aug 2022 19:03:03 +0000 Subject: [PATCH] Fix deadlock in updatePasswordHistory() One of the changes that commit bd3558749e71 ("Remove unused and insecure fallback to legacy password history hash") (http://ag/19331204) made was moving the call to updatePasswordHistory() into setLockCredentialWithSpLocked() so that the SyntheticPassword became available. Unfortunately, this causes a deadlock because the call to getRequestedPasswordHistoryLength() takes the DevicePolicyManagerService lock while the mSpManager lock is held, whereas normally these two locks are taken in the opposite order. Revert the problematic part of the change and go back to the original implementation where updatePasswordHistory() is called from onPostPasswordChanged(). Handle failure to compute the password hash (which should still never happen, but it becomes less obvious when the SP needs to be unwrapped) by logging an error message and not updating the password history. Test: atest LockscreenCredentialTest Test: atest com.android.server.locksettings Test: atest MixedDeviceOwnerTest#testSecurityLoggingWithSingleUser Bug: 241253969 Fixes: bd3558749e71 ("Remove unused and insecure fallback to legacy password history hash") Change-Id: I210407884a657bc2019d14f59ce56753b3cacd53 --- .../locksettings/LockSettingsService.java | 28 +++++++++++-------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/services/core/java/com/android/server/locksettings/LockSettingsService.java b/services/core/java/com/android/server/locksettings/LockSettingsService.java index b86fa7af8bd8d..f4ad750948e64 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsService.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsService.java @@ -1627,7 +1627,7 @@ public class LockSettingsService extends ILockSettings.Stub { } onSyntheticPasswordKnown(userId, sp); - setLockCredentialWithSpLocked(credential, sp, userId, isLockTiedToParent); + setLockCredentialWithSpLocked(credential, sp, userId); sendCredentialsOnChangeIfRequired(credential, userId, isLockTiedToParent); return true; } @@ -1641,15 +1641,18 @@ 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 new password in the password history list, if device policy enforces * a password history requirement. + * + * This must not be called while the mSpManager lock is held, as this calls into + * DevicePolicyManagerService to get the requested password history length. */ - private void updatePasswordHistory(SyntheticPassword sp, LockscreenCredential password, - int userHandle, boolean isLockTiedToParent) { + private void updatePasswordHistory(LockscreenCredential password, int userHandle) { if (password.isNone()) { return; } @@ -1657,10 +1660,6 @@ public class LockSettingsService extends ILockSettings.Stub { // Do not keep track of historical patterns return; } - 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); @@ -1671,9 +1670,16 @@ public class LockSettingsService extends ILockSettings.Stub { if (passwordHistoryLength == 0) { passwordHistory = ""; } else { - final byte[] hashFactor = sp.derivePasswordHashFactor(); + final byte[] hashFactor = getHashFactor(password, userHandle); final byte[] salt = getSalt(userHandle).getBytes(); String hash = password.passwordToHistoryHash(salt, hashFactor); + if (hash == null) { + // This should never happen, as all information needed to compute the hash should be + // available. In particular, unwrapping the SP in getHashFactor() should always + // succeed, as we're using the LSKF that was just set. + Slog.e(TAG, "Failed to compute password hash; password history won't be updated"); + return; + } if (TextUtils.isEmpty(passwordHistory)) { passwordHistory = hash; } else { @@ -2644,7 +2650,7 @@ public class LockSettingsService extends ILockSettings.Stub { */ @GuardedBy("mSpManager") private long setLockCredentialWithSpLocked(LockscreenCredential credential, - SyntheticPassword sp, int userId, boolean isLockTiedToParent) { + SyntheticPassword sp, int userId) { if (DEBUG) Slog.d(TAG, "setLockCredentialWithSpLocked: user=" + userId); final int savedCredentialType = getCredentialTypeInternal(userId); final long oldProtectorId = getCurrentLskfBasedProtectorId(userId); @@ -2682,7 +2688,6 @@ 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) { @@ -2928,8 +2933,7 @@ public class LockSettingsService extends ILockSettings.Stub { return false; } onSyntheticPasswordKnown(userId, result.syntheticPassword); - setLockCredentialWithSpLocked(credential, result.syntheticPassword, userId, - /* isLockTiedToParent= */ false); + setLockCredentialWithSpLocked(credential, result.syntheticPassword, userId); return true; }