Fix deadlock in updatePasswordHistory()

One of the changes that commit bd3558749e ("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: bd3558749e ("Remove unused and insecure fallback to legacy password history hash")
Change-Id: I210407884a657bc2019d14f59ce56753b3cacd53
This commit is contained in:
Eric Biggers
2022-08-04 19:03:03 +00:00
parent 051989e657
commit f083213953

View File

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