From 95792478f197ae363a7e86d2c3139a7ade048818 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Wed, 26 Jan 2022 01:59:17 +0000 Subject: 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 --- core/java/android/os/storage/IStorageManager.aidl | 2 +- core/java/android/os/storage/StorageManager.java | 4 +-- .../com/android/server/StorageManagerService.java | 9 ++---- .../android/server/am/ActivityManagerService.java | 18 ++++++++++-- .../java/com/android/server/am/UserController.java | 33 ++++++++-------------- .../core/java/com/android/server/am/UserState.java | 4 +-- .../server/locksettings/LockSettingsService.java | 16 +++++------ .../com/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 09bdf198315c..2f3f3189d70f 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 db724d2de30b..2b2b4cfe8538 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 eb2721da5e8a..2883e0d09d63 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 9f59a5fc7253..f978b2b68e48 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 b28b1a66cd97..5a43f4d6c3dc 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 40fc3066e2bb..71a551113f12 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 56078d5c72d6..2e7c5ff61eb3 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 9ffb50176f0e..5562308e55c0 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)); } -- cgit v1.2.3-59-g8ed1b