From bd0bc157d3c552741056c5ba9ef85a6170075d98 Mon Sep 17 00:00:00 2001 From: Tianjie Date: Sat, 17 Apr 2021 14:19:03 -0700 Subject: [PATCH] Revisit the lifetime of the RebootEscrowProvider Currently the RebootEscrowProvider is created as part of the boot process, when device tries to restore the escrow data. This will fail the first RoR boot attempt after we switch from HAL based -> server based. So revisit the logic to reduce the time for RebootEscrowManager to hold the provider in RAM. Now we attempt to create a new provider object when 1. new RoR preparation request 2. load / restore escrow data (after reboot) 3. clear rebootescrow, as we want to clear provider's internal data 4. lskf capture We reuse the old provider in memory when 1. arm reboot escrow, right before reboot (we don't switch the provider after lskf is captured) we clear the provider in memory when 1. after clear rebootescrow, so new RoR request can create a new one 2. after we finish restoring escrow data Bug: 184676743 Test: unittest, cts test Change-Id: I2109cbe69f0ebba766aabf30feb141053496354f --- .../locksettings/RebootEscrowManager.java | 47 +++++++++++++------ .../RebootEscrowManagerTests.java | 42 +++++++++++------ 2 files changed, 59 insertions(+), 30 deletions(-) diff --git a/services/core/java/com/android/server/locksettings/RebootEscrowManager.java b/services/core/java/com/android/server/locksettings/RebootEscrowManager.java index c01523a4bc404..90694d0a5f64c 100644 --- a/services/core/java/com/android/server/locksettings/RebootEscrowManager.java +++ b/services/core/java/com/android/server/locksettings/RebootEscrowManager.java @@ -205,6 +205,7 @@ class RebootEscrowManager { Slog.i(TAG, "Using server based resume on reboot"); rebootEscrowProvider = new RebootEscrowProviderServerBasedImpl(mContext, mStorage); } else { + Slog.i(TAG, "Using HAL based resume on reboot"); rebootEscrowProvider = new RebootEscrowProviderHalImpl(); } @@ -239,7 +240,7 @@ class RebootEscrowManager { return mKeyStoreManager; } - public RebootEscrowProviderInterface getRebootEscrowProvider() { + public RebootEscrowProviderInterface createRebootEscrowProviderIfNeeded() { // Initialize for the provider lazily. Because the device_config and service // implementation apps may change when system server is running. if (mRebootEscrowProvider == null) { @@ -249,6 +250,14 @@ class RebootEscrowManager { return mRebootEscrowProvider; } + public RebootEscrowProviderInterface getRebootEscrowProvider() { + return mRebootEscrowProvider; + } + + public void clearRebootEscrowProvider() { + mRebootEscrowProvider = null; + } + public int getBootCount() { return Settings.Global.getInt(mContext.getContentResolver(), Settings.Global.BOOT_COUNT, 0); @@ -308,8 +317,6 @@ class RebootEscrowManager { mStorage.removeRebootEscrow(user.id); } - // Clear the old key in keystore. - mKeyStoreManager.clearKeyStoreEncryptionKey(); onEscrowRestoreComplete(false, attemptCount); } @@ -395,9 +402,6 @@ class RebootEscrowManager { allUsersUnlocked &= restoreRebootEscrowForUser(user.id, escrowKey, kk); } - // Clear the old key in keystore. A new key will be generated by new RoR requests. - mKeyStoreManager.clearKeyStoreEncryptionKey(); - if (!allUsersUnlocked && mLoadEscrowDataErrorCode == ERROR_NONE) { mLoadEscrowDataErrorCode = ERROR_UNLOCK_ALL_USERS; } @@ -473,11 +477,17 @@ class RebootEscrowManager { if (success || (previousBootCount != -1 && bootCountDelta <= BOOT_COUNT_TOLERANCE)) { reportMetricOnRestoreComplete(success, attemptCount); } + + // Clear the old key in keystore. A new key will be generated by new RoR requests. + mKeyStoreManager.clearKeyStoreEncryptionKey(); + // Clear the saved reboot escrow provider + mInjector.clearRebootEscrowProvider(); clearMetricsStorage(); } private RebootEscrowKey getAndClearRebootEscrowKey(SecretKey kk) throws IOException { - RebootEscrowProviderInterface rebootEscrowProvider = mInjector.getRebootEscrowProvider(); + RebootEscrowProviderInterface rebootEscrowProvider = + mInjector.createRebootEscrowProviderIfNeeded(); if (rebootEscrowProvider == null) { Slog.w(TAG, "Had reboot escrow data for users, but RebootEscrowProvider is unavailable"); @@ -529,9 +539,8 @@ class RebootEscrowManager { return; } - if (mInjector.getRebootEscrowProvider() == null) { - Slog.w(TAG, - "Had reboot escrow data for users, but RebootEscrowProvider is unavailable"); + if (mInjector.createRebootEscrowProviderIfNeeded() == null) { + Slog.w(TAG, "Not storing escrow data, RebootEscrowProvider is unavailable"); return; } @@ -586,13 +595,17 @@ class RebootEscrowManager { mRebootEscrowWanted = false; setRebootEscrowReady(false); - RebootEscrowProviderInterface rebootEscrowProvider = mInjector.getRebootEscrowProvider(); + // We want to clear the internal data inside the provider, so always try to create the + // provider. + RebootEscrowProviderInterface rebootEscrowProvider = + mInjector.createRebootEscrowProviderIfNeeded(); if (rebootEscrowProvider == null) { Slog.w(TAG, "RebootEscrowProvider is unavailable for clear request"); } else { rebootEscrowProvider.clearRebootEscrowKey(); } + mInjector.clearRebootEscrowProvider(); clearMetricsStorage(); List users = mUserManager.getUsers(); @@ -610,8 +623,7 @@ class RebootEscrowManager { RebootEscrowProviderInterface rebootEscrowProvider = mInjector.getRebootEscrowProvider(); if (rebootEscrowProvider == null) { - Slog.w(TAG, - "Had reboot escrow data for users, but RebootEscrowProvider is unavailable"); + Slog.w(TAG, "Not storing escrow key, RebootEscrowProvider is unavailable"); clearRebootEscrowIfNeeded(); return ARM_REBOOT_ERROR_NO_PROVIDER; } @@ -677,11 +689,12 @@ class RebootEscrowManager { } boolean prepareRebootEscrow() { - if (mInjector.getRebootEscrowProvider() == null) { + clearRebootEscrowIfNeeded(); + if (mInjector.createRebootEscrowProviderIfNeeded() == null) { + Slog.w(TAG, "No reboot escrow provider, skipping resume on reboot preparation."); return false; } - clearRebootEscrowIfNeeded(); mRebootEscrowWanted = true; mEventLog.addEntry(RebootEscrowEvent.REQUESTED_LSKF); return true; @@ -807,6 +820,10 @@ class RebootEscrowManager { pw.print("mPendingRebootEscrowKey is "); pw.println(keySet ? "set" : "not set"); + RebootEscrowProviderInterface provider = mInjector.getRebootEscrowProvider(); + String providerType = provider == null ? "null" : String.valueOf(provider.getType()); + pw.print("RebootEscrowProvider type is " + providerType); + pw.println(); pw.println("Event log:"); pw.increaseIndent(); diff --git a/services/tests/servicestests/src/com/android/server/locksettings/RebootEscrowManagerTests.java b/services/tests/servicestests/src/com/android/server/locksettings/RebootEscrowManagerTests.java index 49a54ec1354b1..aecc7942b2660 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/RebootEscrowManagerTests.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/RebootEscrowManagerTests.java @@ -112,14 +112,13 @@ public class RebootEscrowManagerTests { private MockableRebootEscrowInjected mInjected; private RebootEscrowManager mService; private SecretKey mAesKey; + private MockInjector mMockInjector; public interface MockableRebootEscrowInjected { int getBootCount(); long getCurrentTimeMillis(); - boolean forceServerBased(); - void reportMetric(boolean success, int errorCode, int serviceType, int attemptCount, int escrowDurationInSeconds, int vbmetaDigestStatus, int durationSinceBootComplete); } @@ -127,11 +126,12 @@ public class RebootEscrowManagerTests { static class MockInjector extends RebootEscrowManager.Injector { private final IRebootEscrow mRebootEscrow; private final ResumeOnRebootServiceConnection mServiceConnection; - private final RebootEscrowProviderInterface mRebootEscrowProvider; + private final RebootEscrowProviderInterface mDefaultRebootEscrowProvider; private final UserManager mUserManager; private final MockableRebootEscrowInjected mInjected; private final RebootEscrowKeyStoreManager mKeyStoreManager; - private final boolean mServerBased; + private boolean mServerBased; + private RebootEscrowProviderInterface mRebootEscrowProviderInUse; MockInjector(Context context, UserManager userManager, IRebootEscrow rebootEscrow, @@ -149,7 +149,7 @@ public class RebootEscrowManagerTests { return mRebootEscrow; } }; - mRebootEscrowProvider = new RebootEscrowProviderHalImpl(halInjector); + mDefaultRebootEscrowProvider = new RebootEscrowProviderHalImpl(halInjector); mUserManager = userManager; mKeyStoreManager = keyStoreManager; mInjected = injected; @@ -166,7 +166,8 @@ public class RebootEscrowManagerTests { mServerBased = true; RebootEscrowProviderServerBasedImpl.Injector injector = new RebootEscrowProviderServerBasedImpl.Injector(serviceConnection); - mRebootEscrowProvider = new RebootEscrowProviderServerBasedImpl(storage, injector); + mDefaultRebootEscrowProvider = new RebootEscrowProviderServerBasedImpl( + storage, injector); mUserManager = userManager; mKeyStoreManager = keyStoreManager; mInjected = injected; @@ -184,15 +185,23 @@ public class RebootEscrowManagerTests { @Override public boolean serverBasedResumeOnReboot() { - if (mInjected.forceServerBased()) { - return true; - } return mServerBased; } + @Override + public RebootEscrowProviderInterface createRebootEscrowProviderIfNeeded() { + mRebootEscrowProviderInUse = mDefaultRebootEscrowProvider; + return mRebootEscrowProviderInUse; + } + @Override public RebootEscrowProviderInterface getRebootEscrowProvider() { - return mRebootEscrowProvider; + return mRebootEscrowProviderInUse; + } + + @Override + public void clearRebootEscrowProvider() { + mRebootEscrowProviderInUse = null; } @Override @@ -264,13 +273,15 @@ public class RebootEscrowManagerTests { when(mCallbacks.isUserSecure(NONSECURE_SECONDARY_USER_ID)).thenReturn(false); when(mCallbacks.isUserSecure(SECURE_SECONDARY_USER_ID)).thenReturn(true); mInjected = mock(MockableRebootEscrowInjected.class); - mService = new RebootEscrowManager(new MockInjector(mContext, mUserManager, mRebootEscrow, - mKeyStoreManager, mStorage, mInjected), mCallbacks, mStorage); + mMockInjector = new MockInjector(mContext, mUserManager, mRebootEscrow, + mKeyStoreManager, mStorage, mInjected); + mService = new RebootEscrowManager(mMockInjector, mCallbacks, mStorage); } private void setServerBasedRebootEscrowProvider() throws Exception { - mService = new RebootEscrowManager(new MockInjector(mContext, mUserManager, - mServiceConnection, mKeyStoreManager, mStorage, mInjected), mCallbacks, mStorage); + mMockInjector = new MockInjector(mContext, mUserManager, mServiceConnection, + mKeyStoreManager, mStorage, mInjected); + mService = new RebootEscrowManager(mMockInjector, mCallbacks, mStorage); } @Test @@ -317,6 +328,7 @@ public class RebootEscrowManagerTests { doThrow(ServiceSpecificException.class).when(mRebootEscrow).storeKey(any()); mService.clearRebootEscrow(); verify(mRebootEscrow).storeKey(eq(new byte[32])); + assertNull(mMockInjector.getRebootEscrowProvider()); } @Test @@ -785,7 +797,7 @@ public class RebootEscrowManagerTests { assertNull( mStorage.getString(RebootEscrowManager.REBOOT_ESCROW_ARMED_KEY, null, USER_SYSTEM)); // Change the provider to server based, expect the reboot to fail - when(mInjected.forceServerBased()).thenReturn(true); + mMockInjector.mServerBased = true; assertEquals(ARM_REBOOT_ERROR_PROVIDER_MISMATCH, mService.armRebootEscrowIfNeeded()); assertNull( mStorage.getString(RebootEscrowManager.REBOOT_ESCROW_ARMED_KEY, null, USER_SYSTEM));