From 401e447e10f0026ca0a529c15a33e85fc05e4362 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Wed, 26 Jan 2022 01:59:14 +0000 Subject: [PATCH 1/6] Remove HardwareAuthToken support from FakeStorageManager There is no longer any need for FakeStorageManager to keep track of hardware auth tokens, since they aren't used for real anymore. Test: atest com.android.server.locksettings Bug: 184723544 Change-Id: Ida3a989ecea974fe79568e381cf0e6ff3fe1f1eb (cherry picked from commit 2e10d6394a32c5ebaa114569932ab8a255636673) Merged-In: Ida3a989ecea974fe79568e381cf0e6ff3fe1f1eb --- .../BaseLockSettingsServiceTests.java | 2 - .../locksettings/FakeStorageManager.java | 39 +++++++++---------- 2 files changed, 19 insertions(+), 22 deletions(-) diff --git a/services/tests/servicestests/src/com/android/server/locksettings/BaseLockSettingsServiceTests.java b/services/tests/servicestests/src/com/android/server/locksettings/BaseLockSettingsServiceTests.java index d62f83c005fb7..dad50bd8a9d14 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/BaseLockSettingsServiceTests.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/BaseLockSettingsServiceTests.java @@ -221,7 +221,6 @@ public abstract class BaseLockSettingsServiceTests { Object[] args = invocation.getArguments(); mStorageManager.addUserKeyAuth((int) args[0] /* userId */, (int) args[1] /* serialNumber */, - (byte[]) args[2] /* token */, (byte[]) args[3] /* secret */); return null; } @@ -233,7 +232,6 @@ public abstract class BaseLockSettingsServiceTests { Object[] args = invocation.getArguments(); mStorageManager.clearUserKeyAuth((int) args[0] /* userId */, (int) args[1] /* serialNumber */, - (byte[]) args[2] /* token */, (byte[]) args[3] /* secret */); return null; } diff --git a/services/tests/servicestests/src/com/android/server/locksettings/FakeStorageManager.java b/services/tests/servicestests/src/com/android/server/locksettings/FakeStorageManager.java index 102bac111adac..619ef7078c246 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/FakeStorageManager.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/FakeStorageManager.java @@ -19,7 +19,6 @@ package com.android.server.locksettings; import android.os.IProgressListener; import android.os.RemoteException; import android.util.ArrayMap; -import android.util.Pair; import junit.framework.AssertionFailedError; @@ -29,56 +28,56 @@ import java.util.Arrays; public class FakeStorageManager { - private ArrayMap>> mAuth = new ArrayMap<>(); + private ArrayMap> mAuth = new ArrayMap<>(); private boolean mIgnoreBadUnlock; - public void addUserKeyAuth(int userId, int serialNumber, byte[] token, byte[] secret) { - getUserAuth(userId).add(new Pair<>(token, secret)); + public void addUserKeyAuth(int userId, int serialNumber, byte[] secret) { + getUserAuth(userId).add(secret); } - public void clearUserKeyAuth(int userId, int serialNumber, byte[] token, byte[] secret) { - ArrayList> auths = getUserAuth(userId); - if (token == null && secret == null) { + public void clearUserKeyAuth(int userId, int serialNumber, byte[] secret) { + ArrayList auths = getUserAuth(userId); + if (secret == null) { return; } - auths.remove(new Pair<>(token, secret)); - auths.add(new Pair<>(null, null)); + auths.remove(secret); + auths.add(null); } public void fixateNewestUserKeyAuth(int userId) { - ArrayList> auths = mAuth.get(userId); - Pair latest = auths.get(auths.size() - 1); + ArrayList auths = mAuth.get(userId); + byte[] latest = auths.get(auths.size() - 1); auths.clear(); auths.add(latest); } - private ArrayList> getUserAuth(int userId) { + private ArrayList getUserAuth(int userId) { if (!mAuth.containsKey(userId)) { - ArrayList> auths = new ArrayList>(); - auths.add(new Pair(null, null)); - mAuth.put(userId, auths); + ArrayList auths = new ArrayList<>(); + auths.add(null); + mAuth.put(userId, auths); } return mAuth.get(userId); } public byte[] getUserUnlockToken(int userId) { - ArrayList> auths = getUserAuth(userId); + ArrayList auths = getUserAuth(userId); if (auths.size() != 1) { throw new AssertionFailedError("More than one secret exists"); } - return auths.get(0).second; + return auths.get(0); } public void unlockUser(int userId, byte[] secret, IProgressListener listener) throws RemoteException { listener.onStarted(userId, null); listener.onFinished(userId, null); - ArrayList> auths = getUserAuth(userId); + ArrayList auths = getUserAuth(userId); if (auths.size() > 1) { throw new AssertionFailedError("More than one secret exists"); } - Pair auth = auths.get(0); - if (!Arrays.equals(secret, auth.second)) { + byte[] auth = auths.get(0); + if (!Arrays.equals(secret, auth)) { if (!mIgnoreBadUnlock) { throw new AssertionFailedError("Invalid secret to unlock user " + userId); } From 0566abd58cc1b18a6e072d033b9aa25c989bda3f Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Wed, 26 Jan 2022 01:59:14 +0000 Subject: [PATCH 2/6] Remove non-SP based setLockCredentialInternal() This code is no longer used, since the migration to synthetic passwords is always enabled. Test: atest com.android.server.locksettings Bug: 184723544 Change-Id: Ieefffb2641f5c12dfcd7556f529830328e8ba292 (cherry picked from commit 2dd97def291b392e85080ba9ab84734dd2bed8af) Merged-In: Ieefffb2641f5c12dfcd7556f529830328e8ba292 --- .../locksettings/LockSettingsService.java | 81 +------------------ 1 file changed, 3 insertions(+), 78 deletions(-) diff --git a/services/core/java/com/android/server/locksettings/LockSettingsService.java b/services/core/java/com/android/server/locksettings/LockSettingsService.java index 45f85edeff7e6..14597a743a56f 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsService.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsService.java @@ -1722,42 +1722,10 @@ public class LockSettingsService extends ILockSettings.Stub { } } synchronized (mSpManager) { - if (shouldMigrateToSyntheticPasswordLocked(userId)) { - initializeSyntheticPasswordLocked(currentHandle.hash, savedCredential, userId); - return spBasedSetLockCredentialInternalLocked(credential, savedCredential, userId, - isLockTiedToParent); - } + initializeSyntheticPasswordLocked(currentHandle.hash, savedCredential, userId); + return spBasedSetLockCredentialInternalLocked(credential, savedCredential, userId, + isLockTiedToParent); } - if (DEBUG) Slog.d(TAG, "setLockCredentialInternal: user=" + userId); - byte[] enrolledHandle = enrollCredential(currentHandle.hash, - savedCredential.getCredential(), credential.getCredential(), userId); - if (enrolledHandle == null) { - Slog.w(TAG, String.format("Failed to enroll %s: incorrect credential", - credential.isPattern() ? "pattern" : "password")); - return false; - } - CredentialHash willStore = CredentialHash.create(enrolledHandle, credential.getType()); - mStorage.writeCredentialHash(willStore, userId); - // Still update PASSWORD_TYPE_KEY if we are running in pre-synthetic password code path, - // since it forms part of the state that determines the credential type - // @see getCredentialTypeInternal - setKeyguardStoredQuality( - LockPatternUtils.credentialTypeToPasswordQuality(credential.getType()), userId); - // push new secret and auth token to vold - GateKeeperResponse gkResponse; - try { - gkResponse = getGateKeeperService().verifyChallenge(userId, 0, willStore.hash, - credential.getCredential()); - } catch (RemoteException e) { - throw new IllegalStateException("Failed to verify current credential", e); - } - setUserKeyProtection(userId, credential, convertResponse(gkResponse)); - fixateNewestUserKeyAuth(userId); - // Refresh the auth token - doVerifyCredential(credential, userId, null /* progressCallback */, 0 /* flags */); - synchronizeUnifiedWorkChallengeForProfiles(userId, null); - sendCredentialsOnChangeIfRequired(credential, userId, isLockTiedToParent); - return true; } private void onPostPasswordChanged(LockscreenCredential newCredential, int userHandle) { @@ -1923,54 +1891,11 @@ public class LockSettingsService extends ILockSettings.Stub { mStorage.writeChildProfileLock(userId, outputStream.toByteArray()); } - private byte[] enrollCredential(byte[] enrolledHandle, - byte[] enrolledCredential, byte[] toEnroll, int userId) { - checkWritePermission(userId); - GateKeeperResponse response; - try { - response = getGateKeeperService().enroll(userId, enrolledHandle, - enrolledCredential, toEnroll); - } catch (RemoteException e) { - Slog.e(TAG, "Failed to enroll credential", e); - return null; - } - - if (response == null) { - return null; - } - - byte[] hash = response.getPayload(); - if (hash != null) { - setKeystorePassword(toEnroll, userId); - } else { - // Should not happen - Slog.e(TAG, "Throttled while enrolling a password"); - } - return hash; - } - private void setAuthlessUserKeyProtection(int userId, byte[] key) { if (DEBUG) Slog.d(TAG, "setAuthlessUserKeyProtectiond: user=" + userId); addUserKeyAuth(userId, null, key); } - private void setUserKeyProtection(int userId, LockscreenCredential credential, - VerifyCredentialResponse vcr) { - if (DEBUG) Slog.d(TAG, "setUserKeyProtection: user=" + userId); - if (vcr == null) { - throw new IllegalArgumentException("Null response verifying a credential we just set"); - } - if (vcr.getResponseCode() != VerifyCredentialResponse.RESPONSE_OK) { - throw new IllegalArgumentException("Non-OK response verifying a credential we just set " - + vcr.getResponseCode()); - } - byte[] token = vcr.getGatekeeperHAT(); - if (token == null) { - throw new IllegalArgumentException("Empty payload verifying a credential we just set"); - } - addUserKeyAuth(userId, token, secretFromCredential(credential)); - } - private void clearUserKeyProtection(int userId, byte[] secret) { if (DEBUG) Slog.d(TAG, "clearUserKeyProtection user=" + userId); final UserInfo userInfo = mUserManager.getUserInfo(userId); From 665b5f9228c482c5d199731f9d891b0b62efa580 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Wed, 26 Jan 2022 01:59:15 +0000 Subject: [PATCH 3/6] Don't pass HardwareAuthToken to unlockUser() in non-SP verifyCredential While at first glance it looks like this is still needed, actually the support for HardwareAuthTokens was already removed from vold in Android 12, so this cannot actually be doing anything. Test: atest com.android.server.locksettings Bug: 184723544 Change-Id: I3c176ba282f4c7901dd09fe3d66cfd380794fb48 (cherry picked from commit 3654b097b179289fe41a7647dadf8404e72bbb87) Merged-In: I3c176ba282f4c7901dd09fe3d66cfd380794fb48 --- .../com/android/server/locksettings/LockSettingsService.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/services/core/java/com/android/server/locksettings/LockSettingsService.java b/services/core/java/com/android/server/locksettings/LockSettingsService.java index 14597a743a56f..a1626696c7bec 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsService.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsService.java @@ -2216,9 +2216,8 @@ public class LockSettingsService extends ILockSettings.Stub { setUserPasswordMetrics(credential, userId); unlockKeystore(credential.getCredential(), userId); - Slog.i(TAG, "Unlocking user " + userId + " with token length " - + response.getGatekeeperHAT().length); - unlockUser(userId, response.getGatekeeperHAT(), secretFromCredential(credential)); + Slog.i(TAG, "Unlocking user " + userId); + unlockUser(userId, null, secretFromCredential(credential)); if (isManagedProfileWithSeparatedLock(userId)) { setDeviceUnlockedForUser(userId); From 5eb0659999abf740f1a0077a7c703347debb84c9 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Wed, 26 Jan 2022 01:59:16 +0000 Subject: [PATCH 4/6] Remove HardwareAuthToken parameter from addUserKeyAuth Due to the migration to synthetic passwords, the 'token' parameter to addUserKeyAuth() is no longer needed. Remove it. Test: atest com.android.server.locksettings Bug: 184723544 Change-Id: I06e7c36787cc7f384acb7742737c3b1cfa50f0ae (cherry picked from commit 6b220a95e9bcb25e103bc0cb3dc4f4bc18c3e137) Merged-In: I06e7c36787cc7f384acb7742737c3b1cfa50f0ae --- core/java/android/os/storage/IStorageManager.aidl | 2 +- .../com/android/server/StorageManagerService.java | 15 ++++++++------- .../server/locksettings/LockSettingsService.java | 14 +++++++------- .../BaseLockSettingsServiceTests.java | 4 ++-- 4 files changed, 18 insertions(+), 17 deletions(-) diff --git a/core/java/android/os/storage/IStorageManager.aidl b/core/java/android/os/storage/IStorageManager.aidl index 6c0a1f99e112f..c86221c26fa33 100644 --- a/core/java/android/os/storage/IStorageManager.aidl +++ b/core/java/android/os/storage/IStorageManager.aidl @@ -179,7 +179,7 @@ interface IStorageManager { void prepareUserStorage(in String volumeUuid, int userId, int serialNumber, int flags) = 66; void destroyUserStorage(in String volumeUuid, int userId, int flags) = 67; boolean isConvertibleToFBE() = 68; - void addUserKeyAuth(int userId, int serialNumber, in byte[] token, in byte[] secret) = 70; + void addUserKeyAuth(int userId, int serialNumber, in byte[] secret) = 70; void fixateNewestUserKeyAuth(int userId) = 71; void fstrim(int flags, IVoldTaskListener listener) = 72; AppFuseMount mountProxyFileDescriptorBridge() = 73; diff --git a/services/core/java/com/android/server/StorageManagerService.java b/services/core/java/com/android/server/StorageManagerService.java index 8a83130f50fa1..bfa310f2b0edf 100644 --- a/services/core/java/com/android/server/StorageManagerService.java +++ b/services/core/java/com/android/server/StorageManagerService.java @@ -3408,18 +3408,19 @@ class StorageManagerService extends IStorageManager.Stub } /* - * Add this token/secret pair to the set of ways we can recover a disk encryption key. - * Changing the token/secret for a disk encryption key is done in two phases: first, adding - * a new token/secret pair with this call, then delting all other pairs with - * fixateNewestUserKeyAuth. This allows other places where a credential is used, such as - * Gatekeeper, to be updated between the two calls. + * Add this secret to the set of ways we can recover a user's disk + * encryption key. Changing the secret for a disk encryption key is done in + * two phases. First, this method is called to add the new secret binding. + * Second, fixateNewestUserKeyAuth is called to delete all other bindings. + * This allows other places where a credential is used, such as Gatekeeper, + * to be updated between the two calls. */ @Override - public void addUserKeyAuth(int userId, int serialNumber, byte[] token, byte[] secret) { + public void addUserKeyAuth(int userId, int serialNumber, byte[] secret) { enforcePermission(android.Manifest.permission.STORAGE_INTERNAL); try { - mVold.addUserKeyAuth(userId, serialNumber, encodeBytes(token), encodeBytes(secret)); + mVold.addUserKeyAuth(userId, serialNumber, encodeBytes(secret)); } catch (Exception e) { Slog.wtf(TAG, e); } diff --git a/services/core/java/com/android/server/locksettings/LockSettingsService.java b/services/core/java/com/android/server/locksettings/LockSettingsService.java index a1626696c7bec..31083601b15c4 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsService.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsService.java @@ -1891,9 +1891,9 @@ public class LockSettingsService extends ILockSettings.Stub { mStorage.writeChildProfileLock(userId, outputStream.toByteArray()); } - private void setAuthlessUserKeyProtection(int userId, byte[] key) { - if (DEBUG) Slog.d(TAG, "setAuthlessUserKeyProtectiond: user=" + userId); - addUserKeyAuth(userId, null, key); + private void setUserKeyProtection(int userId, byte[] key) { + if (DEBUG) Slog.d(TAG, "setUserKeyProtection: user=" + userId); + addUserKeyAuth(userId, key); } private void clearUserKeyProtection(int userId, byte[] secret) { @@ -1944,11 +1944,11 @@ public class LockSettingsService extends ILockSettings.Stub { } } - private void addUserKeyAuth(int userId, byte[] token, byte[] secret) { + private void addUserKeyAuth(int userId, byte[] secret) { final UserInfo userInfo = mUserManager.getUserInfo(userId); final long callingId = Binder.clearCallingIdentity(); try { - mStorageManager.addUserKeyAuth(userId, userInfo.serialNumber, token, secret); + mStorageManager.addUserKeyAuth(userId, userInfo.serialNumber, secret); } catch (RemoteException e) { throw new IllegalStateException("Failed to add new key to vold " + userId, e); } finally { @@ -2725,7 +2725,7 @@ public class LockSettingsService extends ILockSettings.Stub { mSpManager.newSidForUser(getGateKeeperService(), auth, userId); } mSpManager.verifyChallenge(getGateKeeperService(), auth, 0L, userId); - setAuthlessUserKeyProtection(userId, auth.deriveDiskEncryptionKey()); + setUserKeyProtection(userId, auth.deriveDiskEncryptionKey()); setKeystorePassword(auth.deriveKeyStorePassword(), userId); } else { clearUserKeyProtection(userId, null); @@ -2927,7 +2927,7 @@ public class LockSettingsService extends ILockSettings.Stub { // a new SID, and re-add keys to vold and keystore. mSpManager.newSidForUser(getGateKeeperService(), auth, userId); mSpManager.verifyChallenge(getGateKeeperService(), auth, 0L, userId); - setAuthlessUserKeyProtection(userId, auth.deriveDiskEncryptionKey()); + setUserKeyProtection(userId, auth.deriveDiskEncryptionKey()); fixateNewestUserKeyAuth(userId); setKeystorePassword(auth.deriveKeyStorePassword(), userId); } diff --git a/services/tests/servicestests/src/com/android/server/locksettings/BaseLockSettingsServiceTests.java b/services/tests/servicestests/src/com/android/server/locksettings/BaseLockSettingsServiceTests.java index dad50bd8a9d14..2bd42fa26d659 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/BaseLockSettingsServiceTests.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/BaseLockSettingsServiceTests.java @@ -221,10 +221,10 @@ public abstract class BaseLockSettingsServiceTests { Object[] args = invocation.getArguments(); mStorageManager.addUserKeyAuth((int) args[0] /* userId */, (int) args[1] /* serialNumber */, - (byte[]) args[3] /* secret */); + (byte[]) args[2] /* secret */); return null; } - }).when(sm).addUserKeyAuth(anyInt(), anyInt(), any(), any()); + }).when(sm).addUserKeyAuth(anyInt(), anyInt(), any()); doAnswer(new Answer() { @Override From d04de5ce97e56555edc6fbbd1d9292a959071488 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Wed, 26 Jan 2022 01:59:17 +0000 Subject: [PATCH 5/6] Remove HardwareAuthToken parameter from clearUserKeyAuth Due to the migration to synthetic passwords, the 'token' parameter to clearUserKeyAuth() is no longer needed. Remove it. Test: atest com.android.server.locksettings Bug: 184723544 Change-Id: I739b519b0e91293acbf018020891d68b3090c175 (cherry picked from commit 2a8ab4778297852738f94ea5dd3f1e6ff9ab9416) Merged-In: I739b519b0e91293acbf018020891d68b3090c175 --- .../android/os/storage/IStorageManager.aidl | 2 +- .../android/server/StorageManagerService.java | 17 +++++++++-------- .../locksettings/LockSettingsService.java | 2 +- .../BaseLockSettingsServiceTests.java | 4 ++-- 4 files changed, 13 insertions(+), 12 deletions(-) diff --git a/core/java/android/os/storage/IStorageManager.aidl b/core/java/android/os/storage/IStorageManager.aidl index c86221c26fa33..09bdf198315cd 100644 --- a/core/java/android/os/storage/IStorageManager.aidl +++ b/core/java/android/os/storage/IStorageManager.aidl @@ -195,7 +195,7 @@ interface IStorageManager { void startCheckpoint(int numTries) = 85; boolean needsCheckpoint() = 86; void abortChanges(in String message, boolean retry) = 87; - void clearUserKeyAuth(int userId, int serialNumber, in byte[] token, in byte[] secret) = 88; + void clearUserKeyAuth(int userId, int serialNumber, in byte[] secret) = 88; void fixupAppDir(in String path) = 89; void disableAppDataIsolation(in String pkgName, int pid, int userId) = 90; void notifyAppIoBlocked(in String volumeUuid, int uid, int tid, int reason) = 91; diff --git a/services/core/java/com/android/server/StorageManagerService.java b/services/core/java/com/android/server/StorageManagerService.java index bfa310f2b0edf..eb2721da5e8ab 100644 --- a/services/core/java/com/android/server/StorageManagerService.java +++ b/services/core/java/com/android/server/StorageManagerService.java @@ -3427,25 +3427,26 @@ class StorageManagerService extends IStorageManager.Stub } /* - * Clear disk encryption key bound to the associated token / secret pair. Removing the user - * binding of the Disk encryption key is done in two phases: first, this call will retrieve - * the disk encryption key using the provided token / secret pair and store it by - * encrypting it with a keymaster key not bound to the user, then fixateNewestUserKeyAuth - * is called to delete all other bindings of the disk encryption key. + * Store a user's disk encryption key without secret binding. Removing the + * secret for a disk encryption key is done in two phases. First, this + * method is called to retrieve the key using the provided secret and store + * it encrypted with a keystore key not bound to the user. Second, + * fixateNewestUserKeyAuth is called to delete the key's other bindings. */ @Override - public void clearUserKeyAuth(int userId, int serialNumber, byte[] token, byte[] secret) { + public void clearUserKeyAuth(int userId, int serialNumber, byte[] secret) { enforcePermission(android.Manifest.permission.STORAGE_INTERNAL); try { - mVold.clearUserKeyAuth(userId, serialNumber, encodeBytes(token), encodeBytes(secret)); + mVold.clearUserKeyAuth(userId, serialNumber, encodeBytes(secret)); } catch (Exception e) { Slog.wtf(TAG, e); } } /* - * Delete all disk encryption token/secret pairs except the most recently added one + * Delete all bindings of a user's disk encryption key except the most + * recently added one. */ @Override public void fixateNewestUserKeyAuth(int userId) { diff --git a/services/core/java/com/android/server/locksettings/LockSettingsService.java b/services/core/java/com/android/server/locksettings/LockSettingsService.java index 31083601b15c4..56078d5c72d63 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsService.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsService.java @@ -1901,7 +1901,7 @@ public class LockSettingsService extends ILockSettings.Stub { final UserInfo userInfo = mUserManager.getUserInfo(userId); final long callingId = Binder.clearCallingIdentity(); try { - mStorageManager.clearUserKeyAuth(userId, userInfo.serialNumber, null, secret); + mStorageManager.clearUserKeyAuth(userId, userInfo.serialNumber, secret); } catch (RemoteException e) { throw new IllegalStateException("clearUserKeyAuth failed user=" + userId); } finally { diff --git a/services/tests/servicestests/src/com/android/server/locksettings/BaseLockSettingsServiceTests.java b/services/tests/servicestests/src/com/android/server/locksettings/BaseLockSettingsServiceTests.java index 2bd42fa26d659..e220841a38160 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/BaseLockSettingsServiceTests.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/BaseLockSettingsServiceTests.java @@ -232,10 +232,10 @@ public abstract class BaseLockSettingsServiceTests { Object[] args = invocation.getArguments(); mStorageManager.clearUserKeyAuth((int) args[0] /* userId */, (int) args[1] /* serialNumber */, - (byte[]) args[3] /* secret */); + (byte[]) args[2] /* secret */); return null; } - }).when(sm).clearUserKeyAuth(anyInt(), anyInt(), any(), any()); + }).when(sm).clearUserKeyAuth(anyInt(), anyInt(), any()); doAnswer( new Answer() { From 95792478f197ae363a7e86d2c3139a7ade048818 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Wed, 26 Jan 2022 01:59:17 +0000 Subject: [PATCH 6/6] Remove HardwareAuthToken parameter from unlockUserKey Due to the migration to synthetic passwords, the 'token' parameter to unlockUserKey() is no longer needed. Remove it. Note: I didn't change unlockUser() in IActivityManager because it is marked with UnsupportedAppUsage, so it might not be safe to change the method signature. It now just ignores the 'token' parameter rather than passing it down the stack. Test: atest com.android.server.locksettings Bug: 184723544 Change-Id: I35ce09412f47f2f2a17a371d518a0a518b70bfb6 (cherry picked from commit b1bcec9c7d3aa97e39f71cc3ac821656d8b0b981) Merged-In: I35ce09412f47f2f2a17a371d518a0a518b70bfb6 --- .../android/os/storage/IStorageManager.aidl | 2 +- .../android/os/storage/StorageManager.java | 4 +-- .../android/server/StorageManagerService.java | 9 ++--- .../server/am/ActivityManagerService.java | 18 ++++++++-- .../com/android/server/am/UserController.java | 33 +++++++------------ .../java/com/android/server/am/UserState.java | 4 +-- .../locksettings/LockSettingsService.java | 16 ++++----- .../android/server/am/UserControllerTest.java | 6 ++-- 8 files changed, 45 insertions(+), 47 deletions(-) diff --git a/core/java/android/os/storage/IStorageManager.aidl b/core/java/android/os/storage/IStorageManager.aidl index 09bdf198315cd..2f3f3189d70f9 100644 --- a/core/java/android/os/storage/IStorageManager.aidl +++ b/core/java/android/os/storage/IStorageManager.aidl @@ -173,7 +173,7 @@ interface IStorageManager { void setDebugFlags(int flags, int mask) = 60; void createUserKey(int userId, int serialNumber, boolean ephemeral) = 61; void destroyUserKey(int userId) = 62; - void unlockUserKey(int userId, int serialNumber, in byte[] token, in byte[] secret) = 63; + void unlockUserKey(int userId, int serialNumber, in byte[] secret) = 63; void lockUserKey(int userId) = 64; boolean isUserKeyUnlocked(int userId) = 65; void prepareUserStorage(in String volumeUuid, int userId, int serialNumber, int flags) = 66; diff --git a/core/java/android/os/storage/StorageManager.java b/core/java/android/os/storage/StorageManager.java index db724d2de30bd..2b2b4cfe85383 100644 --- a/core/java/android/os/storage/StorageManager.java +++ b/core/java/android/os/storage/StorageManager.java @@ -1528,9 +1528,9 @@ public class StorageManager { } /** {@hide} */ - public void unlockUserKey(int userId, int serialNumber, byte[] token, byte[] secret) { + public void unlockUserKey(int userId, int serialNumber, byte[] secret) { try { - mStorageManager.unlockUserKey(userId, serialNumber, token, secret); + mStorageManager.unlockUserKey(userId, serialNumber, secret); } catch (RemoteException e) { throw e.rethrowFromSystemServer(); } diff --git a/services/core/java/com/android/server/StorageManagerService.java b/services/core/java/com/android/server/StorageManagerService.java index eb2721da5e8ab..2883e0d09d633 100644 --- a/services/core/java/com/android/server/StorageManagerService.java +++ b/services/core/java/com/android/server/StorageManagerService.java @@ -1120,8 +1120,7 @@ class StorageManagerService extends IStorageManager.Stub if (initLocked) { mVold.lockUserKey(user.id); } else { - mVold.unlockUserKey(user.id, user.serialNumber, encodeBytes(null), - encodeBytes(null)); + mVold.unlockUserKey(user.id, user.serialNumber, encodeBytes(null)); } } catch (Exception e) { Slog.wtf(TAG, e); @@ -3460,11 +3459,10 @@ class StorageManagerService extends IStorageManager.Stub } @Override - public void unlockUserKey(int userId, int serialNumber, byte[] token, byte[] secret) { + public void unlockUserKey(int userId, int serialNumber, byte[] secret) { boolean isFsEncrypted = StorageManager.isFileEncryptedNativeOrEmulated(); Slog.d(TAG, "unlockUserKey: " + userId + " isFileEncryptedNativeOrEmulated: " + isFsEncrypted - + " hasToken: " + (token != null) + " hasSecret: " + (secret != null)); enforcePermission(android.Manifest.permission.STORAGE_INTERNAL); @@ -3484,8 +3482,7 @@ class StorageManagerService extends IStorageManager.Stub return; } try { - mVold.unlockUserKey(userId, serialNumber, encodeBytes(token), - encodeBytes(secret)); + mVold.unlockUserKey(userId, serialNumber, encodeBytes(secret)); } catch (Exception e) { Slog.wtf(TAG, e); return; diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index 9f59a5fc7253f..f978b2b68e481 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -15108,9 +15108,23 @@ public class ActivityManagerService extends IActivityManager.Stub return mUserController.startUser(userId, /* foreground */ true, unlockListener); } + /** + * Unlocks the given user. + * + * @param userId The ID of the user to unlock. + * @param token No longer used. (This parameter cannot be removed because + * this method is marked with UnsupportedAppUsage, so its + * signature might not be safe to change.) + * @param secret The secret needed to unlock the user's credential-encrypted + * storage, or null if no secret is needed. + * @param listener An optional progress listener. + * + * @return true if the user was successfully unlocked, otherwise false. + */ @Override - public boolean unlockUser(int userId, byte[] token, byte[] secret, IProgressListener listener) { - return mUserController.unlockUser(userId, token, secret, listener); + public boolean unlockUser(int userId, @Nullable byte[] token, @Nullable byte[] secret, + @Nullable IProgressListener listener) { + return mUserController.unlockUser(userId, secret, listener); } @Override diff --git a/services/core/java/com/android/server/am/UserController.java b/services/core/java/com/android/server/am/UserController.java index b28b1a66cd97e..5a43f4d6c3dcc 100644 --- a/services/core/java/com/android/server/am/UserController.java +++ b/services/core/java/com/android/server/am/UserController.java @@ -714,15 +714,9 @@ class UserController implements Handler.Callback { if (!Objects.equals(info.lastLoggedInFingerprint, Build.FINGERPRINT) || SystemProperties.getBoolean("persist.pm.mock-upgrade", false)) { // Suppress double notifications for managed profiles that - // were unlocked automatically as part of their parent user - // being unlocked. - final boolean quiet; - if (info.isManagedProfile()) { - quiet = !uss.tokenProvided - || !mLockPatternUtils.isSeparateProfileChallengeEnabled(userId); - } else { - quiet = false; - } + // were unlocked automatically as part of their parent user being + // unlocked. TODO(b/217442918): this code doesn't work correctly. + final boolean quiet = info.isManagedProfile(); mInjector.sendPreBootBroadcast(userId, quiet, () -> finishUserUnlockedCompleted(uss)); } else { @@ -1658,27 +1652,25 @@ class UserController implements Handler.Callback { } } - boolean unlockUser(final @UserIdInt int userId, byte[] token, byte[] secret, - IProgressListener listener) { + boolean unlockUser(final @UserIdInt int userId, byte[] secret, IProgressListener listener) { checkCallingPermission(INTERACT_ACROSS_USERS_FULL, "unlockUser"); EventLog.writeEvent(EventLogTags.UC_UNLOCK_USER, userId); final long binderToken = Binder.clearCallingIdentity(); try { - return unlockUserCleared(userId, token, secret, listener); + return unlockUserCleared(userId, secret, listener); } finally { Binder.restoreCallingIdentity(binderToken); } } /** - * Attempt to unlock user without a credential token. This typically - * succeeds when the device doesn't have credential-encrypted storage, or - * when the credential-encrypted storage isn't tied to a user-provided - * PIN or pattern. + * Attempt to unlock user without a secret. This typically succeeds when the + * device doesn't have credential-encrypted storage, or when the + * credential-encrypted storage isn't tied to a user-provided PIN or + * pattern. */ private boolean maybeUnlockUser(final @UserIdInt int userId) { - // Try unlocking storage using empty token - return unlockUserCleared(userId, null, null, null); + return unlockUserCleared(userId, null, null); } private static void notifyFinished(@UserIdInt int userId, IProgressListener listener) { @@ -1689,7 +1681,7 @@ class UserController implements Handler.Callback { } } - private boolean unlockUserCleared(final @UserIdInt int userId, byte[] token, byte[] secret, + private boolean unlockUserCleared(final @UserIdInt int userId, byte[] secret, IProgressListener listener) { UserState uss; if (!StorageManager.isUserKeyUnlocked(userId)) { @@ -1697,7 +1689,7 @@ class UserController implements Handler.Callback { final IStorageManager storageManager = mInjector.getStorageManager(); try { // We always want to unlock user storage, even user is not started yet - storageManager.unlockUserKey(userId, userInfo.serialNumber, token, secret); + storageManager.unlockUserKey(userId, userInfo.serialNumber, secret); } catch (RemoteException | RuntimeException e) { Slogf.w(TAG, "Failed to unlock: " + e.getMessage()); } @@ -1707,7 +1699,6 @@ class UserController implements Handler.Callback { uss = mStartedUsers.get(userId); if (uss != null) { uss.mUnlockProgress.addListener(listener); - uss.tokenProvided = (token != null); } } // Bail if user isn't actually running diff --git a/services/core/java/com/android/server/am/UserState.java b/services/core/java/com/android/server/am/UserState.java index 40fc3066e2bb5..71a551113f125 100644 --- a/services/core/java/com/android/server/am/UserState.java +++ b/services/core/java/com/android/server/am/UserState.java @@ -56,7 +56,6 @@ public final class UserState { public int state = STATE_BOOTING; public int lastState = STATE_BOOTING; public boolean switching; - public boolean tokenProvided; /** Callback for key eviction. */ public interface KeyEvictedCallback { @@ -149,7 +148,6 @@ public final class UserState { @Override public String toString() { return "[UserState: id=" + mHandle.getIdentifier() + ", state=" + stateToString(state) - + ", lastState=" + stateToString(lastState) + ", switching=" + switching - + ", tokenProvided=" + tokenProvided + "]"; + + ", lastState=" + stateToString(lastState) + ", switching=" + switching + "]"; } } diff --git a/services/core/java/com/android/server/locksettings/LockSettingsService.java b/services/core/java/com/android/server/locksettings/LockSettingsService.java index 56078d5c72d63..2e7c5ff61eb36 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsService.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsService.java @@ -1370,7 +1370,7 @@ public class LockSettingsService extends ILockSettings.Stub { * can end up calling into other system services to process user unlock request (via * {@link com.android.server.SystemServiceManager#unlockUser} */ - private void unlockUser(int userId, byte[] token, byte[] secret) { + private void unlockUser(int userId, byte[] secret) { Slog.i(TAG, "Unlocking user " + userId + " with secret only, length " + (secret != null ? secret.length : 0)); // TODO: make this method fully async so we can update UI with progress strings @@ -1395,7 +1395,7 @@ public class LockSettingsService extends ILockSettings.Stub { }; try { - mActivityManager.unlockUser(userId, token, secret, listener); + mActivityManager.unlockUser(userId, null, secret, listener); } catch (RemoteException e) { throw e.rethrowAsRuntimeException(); } @@ -1934,10 +1934,10 @@ public class LockSettingsService extends ILockSettings.Stub { } /** Unlock disk encryption */ - private void unlockUserKey(int userId, byte[] token, byte[] secret) { + private void unlockUserKey(int userId, byte[] secret) { final UserInfo userInfo = mUserManager.getUserInfo(userId); try { - mStorageManager.unlockUserKey(userId, userInfo.serialNumber, token, secret); + mStorageManager.unlockUserKey(userId, userInfo.serialNumber, secret); } catch (RemoteException e) { throw new IllegalStateException("Failed to unlock user key " + userId, e); @@ -2217,7 +2217,7 @@ public class LockSettingsService extends ILockSettings.Stub { unlockKeystore(credential.getCredential(), userId); Slog.i(TAG, "Unlocking user " + userId); - unlockUser(userId, null, secretFromCredential(credential)); + unlockUser(userId, secretFromCredential(credential)); if (isManagedProfileWithSeparatedLock(userId)) { setDeviceUnlockedForUser(userId); @@ -2877,7 +2877,7 @@ public class LockSettingsService extends ILockSettings.Stub { { final byte[] secret = authToken.deriveDiskEncryptionKey(); - unlockUser(userId, null, secret); + unlockUser(userId, secret); Arrays.fill(secret, (byte) 0); } activateEscrowTokens(authToken, userId); @@ -2942,7 +2942,7 @@ public class LockSettingsService extends ILockSettings.Stub { // Clear key from vold so ActivityManager can just unlock the user with empty secret // during boot. Vold storage needs to be unlocked before manipulation of the keys can // succeed. - unlockUserKey(userId, null, auth.deriveDiskEncryptionKey()); + unlockUserKey(userId, auth.deriveDiskEncryptionKey()); clearUserKeyProtection(userId, auth.deriveDiskEncryptionKey()); fixateNewestUserKeyAuth(userId); unlockKeystore(auth.deriveKeyStorePassword(), userId); @@ -3212,7 +3212,7 @@ public class LockSettingsService extends ILockSettings.Stub { // If clearing credential, unlock the user manually in order to progress user start // Call unlockUser() on a handler thread so no lock is held (either by LSS or by // the caller like DPMS), otherwise it can lead to deadlock. - mHandler.post(() -> unlockUser(userId, null, null)); + mHandler.post(() -> unlockUser(userId, null)); } notifyPasswordChanged(userId); notifySeparateProfileChallengeChanged(userId); diff --git a/services/tests/servicestests/src/com/android/server/am/UserControllerTest.java b/services/tests/servicestests/src/com/android/server/am/UserControllerTest.java index 9ffb50176f0ee..5562308e55c03 100644 --- a/services/tests/servicestests/src/com/android/server/am/UserControllerTest.java +++ b/services/tests/servicestests/src/com/android/server/am/UserControllerTest.java @@ -589,8 +589,7 @@ public class UserControllerTest { setUpUser(userId, 0); mUserController.startUser(userId, /* foreground= */ false); verify(mInjector.mStorageManagerMock, times(1)) - .unlockUserKey(userId, /* serialNumber= */ 0, /* token= */ null, /* secret= */ - null); + .unlockUserKey(userId, /* serialNumber= */ 0, /* secret= */ null); mUserStates.put(userId, mUserController.getStartedUserState(userId)); } @@ -599,8 +598,7 @@ public class UserControllerTest { assertThat(mUserController.startProfile(userId)).isTrue(); verify(mInjector.mStorageManagerMock, times(1)) - .unlockUserKey(userId, /* serialNumber= */ 0, /* token= */ null, /* secret= */ - null); + .unlockUserKey(userId, /* serialNumber= */ 0, /* secret= */ null); mUserStates.put(userId, mUserController.getStartedUserState(userId)); }