From e72f2c18a6615c8becef647d319471bf9d0f0d36 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 2 Dec 2022 22:53:37 +0000 Subject: [PATCH 1/2] Convert locksettings unit tests to use FakeSettingsProvider To mock out settings values in the locksettings unit tests, use com.android.internal.util.test.FakeSettingsProvider instead of the custom solution that was used before. This makes mocked settings accessible to SyntheticPasswordManager, via the Context using the standard methods. The previous solution only made mocked settings accessible to LockSettingsService itself, and only when the special accessor methods in LockSettingsService.Injector were used. Test: atest com.android.server.locksettings Bug: 254378141 Change-Id: I16cfc1fc25cc55e66121fc9512fec01915de4885 --- .../locksettings/LockSettingsService.java | 16 +---- .../BaseLockSettingsServiceTests.java | 40 ++++++++++--- .../server/locksettings/FakeSettings.java | 60 ------------------- .../LockSettingsServiceTestable.java | 25 ++------ .../LockSettingsServiceTests.java | 8 +-- .../LockSettingsStorageTests.java | 19 ++++-- .../locksettings/LockscreenFrpTest.java | 4 +- .../locksettings/MockLockSettingsContext.java | 37 +++++++----- 8 files changed, 80 insertions(+), 129 deletions(-) delete mode 100644 services/tests/servicestests/src/com/android/server/locksettings/FakeSettings.java diff --git a/services/core/java/com/android/server/locksettings/LockSettingsService.java b/services/core/java/com/android/server/locksettings/LockSettingsService.java index 25e71e8ceca17..d19ea790821bb 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsService.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsService.java @@ -550,16 +550,6 @@ public class LockSettingsService extends ILockSettings.Stub { return (BiometricManager) mContext.getSystemService(Context.BIOMETRIC_SERVICE); } - public int settingsGlobalGetInt(ContentResolver contentResolver, String keyName, - int defaultValue) { - return Settings.Global.getInt(contentResolver, keyName, defaultValue); - } - - public int settingsSecureGetInt(ContentResolver contentResolver, String keyName, - int defaultValue, int userId) { - return Settings.Secure.getIntForUser(contentResolver, keyName, defaultValue, userId); - } - public java.security.KeyStore getJavaKeyStore() { try { java.security.KeyStore ks = java.security.KeyStore.getInstance( @@ -1027,9 +1017,9 @@ public class LockSettingsService extends ILockSettings.Stub { private void enforceFrpResolved() { final ContentResolver cr = mContext.getContentResolver(); - final boolean inSetupWizard = mInjector.settingsSecureGetInt(cr, + final boolean inSetupWizard = Settings.Secure.getIntForUser(cr, Settings.Secure.USER_SETUP_COMPLETE, 0, UserHandle.USER_SYSTEM) == 0; - final boolean secureFrp = mInjector.settingsSecureGetInt(cr, + final boolean secureFrp = Settings.Secure.getIntForUser(cr, Settings.Secure.SECURE_FRP_MODE, 0, UserHandle.USER_SYSTEM) == 1; if (inSetupWizard && secureFrp) { throw new SecurityException("Cannot change credential in SUW while factory reset" @@ -2155,7 +2145,7 @@ public class LockSettingsService extends ILockSettings.Stub { if (credential == null || credential.isNone()) { throw new IllegalArgumentException("Credential can't be null or empty"); } - if (userId == USER_FRP && mInjector.settingsGlobalGetInt(mContext.getContentResolver(), + if (userId == USER_FRP && Settings.Global.getInt(mContext.getContentResolver(), Settings.Global.DEVICE_PROVISIONED, 0) != 0) { Slog.e(TAG, "FRP credential can only be verified prior to provisioning."); return VerifyCredentialResponse.ERROR; 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 c934e653564eb..55ab4a0383338 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/BaseLockSettingsServiceTests.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/BaseLockSettingsServiceTests.java @@ -33,6 +33,7 @@ import android.app.admin.DevicePolicyManagerInternal; import android.app.admin.DeviceStateCache; import android.app.trust.TrustManager; import android.content.ComponentName; +import android.content.Context; import android.content.pm.PackageManager; import android.content.pm.UserInfo; import android.hardware.authsecret.V1_0.IAuthSecret; @@ -41,14 +42,18 @@ import android.hardware.fingerprint.FingerprintManager; import android.os.FileUtils; import android.os.IProgressListener; import android.os.RemoteException; +import android.os.UserHandle; import android.os.UserManager; import android.os.storage.IStorageManager; import android.os.storage.StorageManager; +import android.provider.Settings; import android.security.KeyStore; import androidx.test.InstrumentationRegistry; import androidx.test.runner.AndroidJUnit4; +import com.android.internal.util.test.FakeSettingsProvider; +import com.android.internal.util.test.FakeSettingsProviderRule; import com.android.internal.widget.LockPatternUtils; import com.android.internal.widget.LockSettingsInternal; import com.android.internal.widget.LockscreenCredential; @@ -59,6 +64,7 @@ import com.android.server.wm.WindowManagerInternal; import org.junit.After; import org.junit.Before; +import org.junit.Rule; import org.junit.runner.RunWith; import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; @@ -106,7 +112,8 @@ public abstract class BaseLockSettingsServiceTests { FingerprintManager mFingerprintManager; FaceManager mFaceManager; PackageManager mPackageManager; - FakeSettings mSettings; + @Rule + public FakeSettingsProviderRule mSettingsRule = FakeSettingsProvider.rule(); @Before public void setUp_baseServices() throws Exception { @@ -126,7 +133,6 @@ public abstract class BaseLockSettingsServiceTests { mFingerprintManager = mock(FingerprintManager.class); mFaceManager = mock(FaceManager.class); mPackageManager = mock(PackageManager.class); - mSettings = new FakeSettings(); LocalServices.removeServiceForTest(LockSettingsInternal.class); LocalServices.removeServiceForTest(DevicePolicyManagerInternal.class); @@ -134,12 +140,13 @@ public abstract class BaseLockSettingsServiceTests { LocalServices.addService(DevicePolicyManagerInternal.class, mDevicePolicyManagerInternal); LocalServices.addService(WindowManagerInternal.class, mMockWindowManager); - mContext = new MockLockSettingsContext(InstrumentationRegistry.getContext(), mUserManager, - mNotificationManager, mDevicePolicyManager, mock(StorageManager.class), - mock(TrustManager.class), mock(KeyguardManager.class), mFingerprintManager, - mFaceManager, mPackageManager); + final Context origContext = InstrumentationRegistry.getContext(); + mContext = new MockLockSettingsContext(origContext, + mSettingsRule.mockContentResolver(origContext), mUserManager, mNotificationManager, + mDevicePolicyManager, mock(StorageManager.class), mock(TrustManager.class), + mock(KeyguardManager.class), mFingerprintManager, mFaceManager, mPackageManager); mStorage = new LockSettingsStorageTestable(mContext, - new File(InstrumentationRegistry.getContext().getFilesDir(), "locksettings")); + new File(origContext.getFilesDir(), "locksettings")); File storageDir = mStorage.mStorageDir; if (storageDir.exists()) { FileUtils.deleteContents(storageDir); @@ -153,7 +160,7 @@ public abstract class BaseLockSettingsServiceTests { mService = new LockSettingsServiceTestable(mContext, mStorage, mGateKeeperService, mKeyStore, setUpStorageManagerMock(), mActivityManager, mSpManager, mAuthSecretService, mGsiService, mRecoverableKeyStoreManager, - mUserManagerInternal, mDeviceStateCache, mSettings); + mUserManagerInternal, mDeviceStateCache); mService.mHasSecureLockScreen = true; when(mUserManager.getUserInfo(eq(PRIMARY_USER_ID))).thenReturn(PRIMARY_USER_INFO); mPrimaryUserProfiles.add(PRIMARY_USER_INFO); @@ -186,10 +193,25 @@ public abstract class BaseLockSettingsServiceTests { mockBiometricsHardwareFingerprintsAndTemplates(PRIMARY_USER_ID); mockBiometricsHardwareFingerprintsAndTemplates(MANAGED_PROFILE_USER_ID); - mSettings.setDeviceProvisioned(true); + setDeviceProvisioned(true); mLocalService = LocalServices.getService(LockSettingsInternal.class); } + protected void setDeviceProvisioned(boolean provisioned) { + Settings.Global.putInt(mContext.getContentResolver(), + Settings.Global.DEVICE_PROVISIONED, provisioned ? 1 : 0); + } + + protected void setUserSetupComplete(boolean complete) { + Settings.Secure.putIntForUser(mContext.getContentResolver(), + Settings.Secure.USER_SETUP_COMPLETE, complete ? 1 : 0, UserHandle.USER_SYSTEM); + } + + protected void setSecureFrpMode(boolean secure) { + Settings.Secure.putIntForUser(mContext.getContentResolver(), + Settings.Secure.SECURE_FRP_MODE, secure ? 1 : 0, UserHandle.USER_SYSTEM); + } + private UserInfo installChildProfile(int profileId) { final UserInfo userInfo = new UserInfo( profileId, null, null, UserInfo.FLAG_INITIALIZED | UserInfo.FLAG_MANAGED_PROFILE); diff --git a/services/tests/servicestests/src/com/android/server/locksettings/FakeSettings.java b/services/tests/servicestests/src/com/android/server/locksettings/FakeSettings.java deleted file mode 100644 index 2bcd653a5476b..0000000000000 --- a/services/tests/servicestests/src/com/android/server/locksettings/FakeSettings.java +++ /dev/null @@ -1,60 +0,0 @@ -/* - * Copyright (C) 2019 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package com.android.server.locksettings; - -import android.content.ContentResolver; -import android.os.UserHandle; -import android.provider.Settings; - -public class FakeSettings { - - private int mDeviceProvisioned; - private int mSecureFrpMode; - private int mUserSetupComplete; - - public void setDeviceProvisioned(boolean provisioned) { - mDeviceProvisioned = provisioned ? 1 : 0; - } - - public void setSecureFrpMode(boolean secure) { - mSecureFrpMode = secure ? 1 : 0; - } - - public void setUserSetupComplete(boolean complete) { - mUserSetupComplete = complete ? 1 : 0; - } - - public int globalGetInt(String keyName) { - switch (keyName) { - case Settings.Global.DEVICE_PROVISIONED: - return mDeviceProvisioned; - default: - throw new IllegalArgumentException("Unhandled global settings: " + keyName); - } - } - - public int secureGetInt(ContentResolver contentResolver, String keyName, int defaultValue, - int userId) { - if (Settings.Secure.SECURE_FRP_MODE.equals(keyName) && userId == UserHandle.USER_SYSTEM) { - return mSecureFrpMode; - } - if (Settings.Secure.USER_SETUP_COMPLETE.equals(keyName) - && userId == UserHandle.USER_SYSTEM) { - return mUserSetupComplete; - } - return defaultValue; - } -} diff --git a/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsServiceTestable.java b/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsServiceTestable.java index 85db23c6b7171..eccfa06fde5e1 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsServiceTestable.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsServiceTestable.java @@ -20,7 +20,6 @@ import static org.mockito.Mockito.mock; import android.app.IActivityManager; import android.app.admin.DeviceStateCache; -import android.content.ContentResolver; import android.content.Context; import android.content.pm.UserInfo; import android.hardware.authsecret.V1_0.IAuthSecret; @@ -52,14 +51,12 @@ public class LockSettingsServiceTestable extends LockSettingsService { private RecoverableKeyStoreManager mRecoverableKeyStoreManager; private UserManagerInternal mUserManagerInternal; private DeviceStateCache mDeviceStateCache; - private FakeSettings mSettings; public MockInjector(Context context, LockSettingsStorage storage, KeyStore keyStore, IActivityManager activityManager, IStorageManager storageManager, SyntheticPasswordManager spManager, FakeGsiService gsiService, RecoverableKeyStoreManager recoverableKeyStoreManager, - UserManagerInternal userManagerInternal, DeviceStateCache deviceStateCache, - FakeSettings settings) { + UserManagerInternal userManagerInternal, DeviceStateCache deviceStateCache) { super(context); mLockSettingsStorage = storage; mKeyStore = keyStore; @@ -70,7 +67,6 @@ public class LockSettingsServiceTestable extends LockSettingsService { mRecoverableKeyStoreManager = recoverableKeyStoreManager; mUserManagerInternal = userManagerInternal; mDeviceStateCache = deviceStateCache; - mSettings = settings; } @Override @@ -118,18 +114,6 @@ public class LockSettingsServiceTestable extends LockSettingsService { return mSpManager; } - @Override - public int settingsGlobalGetInt(ContentResolver contentResolver, String keyName, - int defaultValue) { - return mSettings.globalGetInt(keyName); - } - - @Override - public int settingsSecureGetInt(ContentResolver contentResolver, String keyName, - int defaultValue, int userId) { - return mSettings.secureGetInt(contentResolver, keyName, defaultValue, userId); - } - @Override public UserManagerInternal getUserManagerInternal() { return mUserManagerInternal; @@ -165,11 +149,10 @@ public class LockSettingsServiceTestable extends LockSettingsService { IStorageManager storageManager, IActivityManager mActivityManager, SyntheticPasswordManager spManager, IAuthSecret authSecretService, FakeGsiService gsiService, RecoverableKeyStoreManager recoverableKeyStoreManager, - UserManagerInternal userManagerInternal, DeviceStateCache deviceStateCache, - FakeSettings settings) { + UserManagerInternal userManagerInternal, DeviceStateCache deviceStateCache) { super(new MockInjector(context, storage, keystore, mActivityManager, - storageManager, spManager, gsiService, - recoverableKeyStoreManager, userManagerInternal, deviceStateCache, settings)); + storageManager, spManager, gsiService, recoverableKeyStoreManager, + userManagerInternal, deviceStateCache)); mGateKeeperService = gatekeeper; mAuthSecretService = authSecretService; } diff --git a/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsServiceTests.java b/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsServiceTests.java index 3f259e343ee61..196226a220a70 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsServiceTests.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsServiceTests.java @@ -424,8 +424,8 @@ public class LockSettingsServiceTests extends BaseLockSettingsServiceTests { @Test public void testCredentialChangeNotPossibleInSecureFrpModeDuringSuw() { - mSettings.setUserSetupComplete(false); - mSettings.setSecureFrpMode(true); + setUserSetupComplete(false); + setSecureFrpMode(true); try { mService.setLockCredential(newPassword("1234"), nonePassword(), PRIMARY_USER_ID); fail("Password shouldn't be changeable before FRP unlock"); @@ -434,8 +434,8 @@ public class LockSettingsServiceTests extends BaseLockSettingsServiceTests { @Test public void testCredentialChangePossibleInSecureFrpModeAfterSuw() { - mSettings.setUserSetupComplete(true); - mSettings.setSecureFrpMode(true); + setUserSetupComplete(true); + setSecureFrpMode(true); assertTrue(mService.setLockCredential(newPassword("1234"), nonePassword(), PRIMARY_USER_ID)); } diff --git a/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTests.java b/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTests.java index a6638587bf221..95d0e15bb2a88 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTests.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTests.java @@ -31,6 +31,7 @@ import android.app.KeyguardManager; import android.app.NotificationManager; import android.app.admin.DevicePolicyManager; import android.app.trust.TrustManager; +import android.content.Context; import android.content.pm.PackageManager; import android.content.pm.UserInfo; import android.database.sqlite.SQLiteDatabase; @@ -49,11 +50,14 @@ import androidx.test.InstrumentationRegistry; import androidx.test.filters.SmallTest; import androidx.test.runner.AndroidJUnit4; +import com.android.internal.util.test.FakeSettingsProvider; +import com.android.internal.util.test.FakeSettingsProviderRule; import com.android.server.PersistentDataBlockManagerInternal; import com.android.server.locksettings.LockSettingsStorage.PersistentData; import org.junit.After; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; @@ -78,14 +82,17 @@ public class LockSettingsStorageTests { public static final byte[] PAYLOAD = new byte[] {1, 2, -1, -2, 33}; - LockSettingsStorageTestable mStorage; - File mStorageDir; - + private LockSettingsStorageTestable mStorage; + private File mStorageDir; private File mDb; + @Rule + public FakeSettingsProviderRule mSettingsRule = FakeSettingsProvider.rule(); @Before public void setUp() throws Exception { - mStorageDir = new File(InstrumentationRegistry.getContext().getFilesDir(), "locksettings"); + final Context origContext = InstrumentationRegistry.getContext(); + + mStorageDir = new File(origContext.getFilesDir(), "locksettings"); mDb = InstrumentationRegistry.getContext().getDatabasePath("locksettings.db"); assertTrue(mStorageDir.exists() || mStorageDir.mkdirs()); @@ -98,8 +105,8 @@ public class LockSettingsStorageTests { // User 3 is a profile of user 0. when(mockUserManager.getProfileParent(eq(3))).thenReturn(new UserInfo(0, "name", 0)); - MockLockSettingsContext context = new MockLockSettingsContext( - InstrumentationRegistry.getContext(), mockUserManager, + MockLockSettingsContext context = new MockLockSettingsContext(origContext, + mSettingsRule.mockContentResolver(origContext), mockUserManager, mock(NotificationManager.class), mock(DevicePolicyManager.class), mock(StorageManager.class), mock(TrustManager.class), mock(KeyguardManager.class), mock(FingerprintManager.class), mock(FaceManager.class), diff --git a/services/tests/servicestests/src/com/android/server/locksettings/LockscreenFrpTest.java b/services/tests/servicestests/src/com/android/server/locksettings/LockscreenFrpTest.java index c2f94e202ca3e..d05af76f8f5ad 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/LockscreenFrpTest.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/LockscreenFrpTest.java @@ -44,7 +44,7 @@ public class LockscreenFrpTest extends BaseLockSettingsServiceTests { @Before public void setDeviceNotProvisioned() throws Exception { // FRP credential can only be verified prior to provisioning - mSettings.setDeviceProvisioned(false); + setDeviceProvisioned(false); } @Before @@ -106,7 +106,7 @@ public class LockscreenFrpTest extends BaseLockSettingsServiceTests { public void testFrpCredential_cannotVerifyAfterProvsioning() { mService.setLockCredential(newPin("1234"), nonePassword(), PRIMARY_USER_ID); - mSettings.setDeviceProvisioned(true); + setDeviceProvisioned(true); assertEquals(VerifyCredentialResponse.RESPONSE_ERROR, mService.verifyCredential(newPin("1234"), USER_FRP, 0 /* flags */) .getResponseCode()); diff --git a/services/tests/servicestests/src/com/android/server/locksettings/MockLockSettingsContext.java b/services/tests/servicestests/src/com/android/server/locksettings/MockLockSettingsContext.java index efa1b044f8f94..21c367b3a6e2f 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/MockLockSettingsContext.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/MockLockSettingsContext.java @@ -21,6 +21,7 @@ import android.app.NotificationManager; import android.app.admin.DevicePolicyManager; import android.app.trust.TrustManager; import android.content.BroadcastReceiver; +import android.content.ContentResolver; import android.content.Context; import android.content.ContextWrapper; import android.content.Intent; @@ -35,22 +36,25 @@ import android.os.storage.StorageManager; public class MockLockSettingsContext extends ContextWrapper { - private UserManager mUserManager; - private NotificationManager mNotificationManager; - private DevicePolicyManager mDevicePolicyManager; - private StorageManager mStorageManager; - private TrustManager mTrustManager; - private KeyguardManager mKeyguardManager; - private FingerprintManager mFingerprintManager; - private FaceManager mFaceManager; - private PackageManager mPackageManager; + private final ContentResolver mContentResolver; + private final UserManager mUserManager; + private final NotificationManager mNotificationManager; + private final DevicePolicyManager mDevicePolicyManager; + private final StorageManager mStorageManager; + private final TrustManager mTrustManager; + private final KeyguardManager mKeyguardManager; + private final FingerprintManager mFingerprintManager; + private final FaceManager mFaceManager; + private final PackageManager mPackageManager; - public MockLockSettingsContext(Context base, UserManager userManager, - NotificationManager notificationManager, DevicePolicyManager devicePolicyManager, - StorageManager storageManager, TrustManager trustManager, - KeyguardManager keyguardManager, FingerprintManager fingerprintManager, - FaceManager faceManager, PackageManager packageManager) { + public MockLockSettingsContext(Context base, ContentResolver contentResolver, + UserManager userManager, NotificationManager notificationManager, + DevicePolicyManager devicePolicyManager, StorageManager storageManager, + TrustManager trustManager, KeyguardManager keyguardManager, + FingerprintManager fingerprintManager, FaceManager faceManager, + PackageManager packageManager) { super(base); + mContentResolver = contentResolver; mUserManager = userManager; mNotificationManager = notificationManager; mDevicePolicyManager = devicePolicyManager; @@ -62,6 +66,11 @@ public class MockLockSettingsContext extends ContextWrapper { mPackageManager = packageManager; } + @Override + public ContentResolver getContentResolver() { + return mContentResolver; + } + @Override public Object getSystemService(String name) { if (USER_SERVICE.equals(name)) { From 745555592daa8a7ed8955c0a9c8a04fe7b826f7f Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 2 Dec 2022 22:53:38 +0000 Subject: [PATCH 2/2] Fix FRP credential overwritten before it is verified When initializing the synthetic password of the user that will own the FRP credential, don't clear the FRP credential if the device is not yet provisioned. In addition, prevent the Weaver slot used by the FRP credential from being overwritten while the device is not yet provisioned. This fixes commit 78e245a21a11 ("Give all users SP-based credentials"). Test: atest com.android.server.locksettings Test: Manually tested FRP Bug: 254378141 Change-Id: I55d916d43fc30e8c76c3d85a09fad14b7946b53d --- .../locksettings/LockSettingsService.java | 1 + .../locksettings/LockSettingsStorage.java | 2 +- .../SyntheticPasswordManager.java | 73 ++++++++++++++++--- .../locksettings/LockscreenFrpTest.java | 1 + .../locksettings/SyntheticPasswordTests.java | 37 ++++++++++ .../WeaverBasedSyntheticPasswordTests.java | 38 ++++++++++ 6 files changed, 139 insertions(+), 13 deletions(-) diff --git a/services/core/java/com/android/server/locksettings/LockSettingsService.java b/services/core/java/com/android/server/locksettings/LockSettingsService.java index d19ea790821bb..0ae3a02fb80a2 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsService.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsService.java @@ -3272,6 +3272,7 @@ public class LockSettingsService extends ILockSettings.Stub { for (UserInfo user : users) { if (userOwnsFrpCredential(mContext, user)) { if (!isUserSecure(user.id)) { + Slogf.d(TAG, "Clearing FRP credential tied to user %d", user.id); mStorage.writePersistentDataBlock(PersistentData.TYPE_NONE, user.id, 0, null); } diff --git a/services/core/java/com/android/server/locksettings/LockSettingsStorage.java b/services/core/java/com/android/server/locksettings/LockSettingsStorage.java index 807ba3cf4463b..473c4b6e0e759 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsStorage.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsStorage.java @@ -550,7 +550,7 @@ class LockSettingsStorage { mCache.clear(); } - @Nullable @VisibleForTesting + @Nullable PersistentDataBlockManagerInternal getPersistentDataBlockManager() { if (mPersistentDataBlockManagerInternal == null) { mPersistentDataBlockManagerInternal = diff --git a/services/core/java/com/android/server/locksettings/SyntheticPasswordManager.java b/services/core/java/com/android/server/locksettings/SyntheticPasswordManager.java index 73a16fdbc7c96..acd7cc1f0ddba 100644 --- a/services/core/java/com/android/server/locksettings/SyntheticPasswordManager.java +++ b/services/core/java/com/android/server/locksettings/SyntheticPasswordManager.java @@ -32,6 +32,7 @@ import android.hardware.weaver.V1_0.WeaverStatus; import android.os.RemoteCallbackList; import android.os.RemoteException; import android.os.UserManager; +import android.provider.Settings; import android.security.GateKeeper; import android.security.Scrypt; import android.service.gatekeeper.GateKeeperResponse; @@ -457,6 +458,11 @@ public class SyntheticPasswordManager { mPasswordSlotManager = passwordSlotManager; } + private boolean isDeviceProvisioned() { + return Settings.Global.getInt(mContext.getContentResolver(), + Settings.Global.DEVICE_PROVISIONED, 0) != 0; + } + @VisibleForTesting protected IWeaver getWeaverService() throws RemoteException { try { @@ -770,6 +776,17 @@ public class SyntheticPasswordManager { private int getNextAvailableWeaverSlot() { Set usedSlots = getUsedWeaverSlots(); usedSlots.addAll(mPasswordSlotManager.getUsedSlots()); + // If the device is not yet provisioned, then the Weaver slot used by the FRP credential may + // be still needed and must not be reused yet. (This *should* instead check "has FRP been + // resolved yet?", which would allow reusing the slot a bit earlier. However, the + // SECURE_FRP_MODE setting gets set to 1 too late for it to be used here.) + if (!isDeviceProvisioned()) { + PersistentData persistentData = mStorage.readPersistentDataBlock(); + if (persistentData != null && persistentData.type == PersistentData.TYPE_SP_WEAVER) { + int slot = persistentData.userId; // Note: field name is misleading + usedSlots.add(slot); + } + } for (int i = 0; i < mWeaverConfig.slots; i++) { if (!usedSlots.contains(i)) { return i; @@ -814,9 +831,14 @@ public class SyntheticPasswordManager { protectorSecret = transformUnderWeaverSecret(stretchedLskf, weaverSecret); } else { - // Weaver is unavailable, so make the protector use Gatekeeper to verify the LSKF - // instead. However, skip Gatekeeper when the LSKF is empty, since it wouldn't give any - // benefit in that case as Gatekeeper isn't expected to provide secure deletion. + // Weaver is unavailable, so make the protector use Gatekeeper (GK) to verify the LSKF. + // + // However, skip GK when the LSKF is empty. There are two reasons for this, one + // performance and one correctness. The performance reason is that GK wouldn't give any + // benefit with an empty LSKF anyway, since GK isn't expected to provide secure + // deletion. The correctness reason is that it is unsafe to enroll a password in the + // 'fakeUserId' GK range on an FRP-protected device that is in the setup wizard with FRP + // not passed yet, as that may overwrite the enrollment used by the FRP credential. if (!credential.isNone()) { // In case GK enrollment leaves persistent state around (in RPMB), this will nuke // them to prevent them from accumulating and causing problems. @@ -908,12 +930,40 @@ public class SyntheticPasswordManager { } } + private static boolean isNoneCredential(PasswordData pwd) { + return pwd == null || pwd.credentialType == LockPatternUtils.CREDENTIAL_TYPE_NONE; + } + + private boolean shouldSynchronizeFrpCredential(@Nullable PasswordData pwd, int userId) { + if (mStorage.getPersistentDataBlockManager() == null) { + return false; + } + UserInfo userInfo = mUserManager.getUserInfo(userId); + if (!LockPatternUtils.userOwnsFrpCredential(mContext, userInfo)) { + return false; + } + // When initializing the synthetic password of the user that will own the FRP credential, + // the FRP data block must not be cleared if the device isn't provisioned yet, since in this + // case the old value of the block may still be needed for the FRP authentication step. The + // FRP data block will instead be cleared later, by + // LockSettingsService.DeviceProvisionedObserver.clearFrpCredentialIfOwnerNotSecure(). + // + // Don't check the SECURE_FRP_MODE setting here, as it gets set to 1 too late. + // + // Don't delay anything for a nonempty credential. A nonempty credential can be set before + // the device has been provisioned, but it's guaranteed to be after FRP was resolved. + if (isNoneCredential(pwd) && !isDeviceProvisioned()) { + Slog.d(TAG, "Not clearing FRP credential yet because device is not yet provisioned"); + return false; + } + return true; + } + private void synchronizeFrpPassword(@Nullable PasswordData pwd, int requestedQuality, int userId) { - if (mStorage.getPersistentDataBlockManager() != null - && LockPatternUtils.userOwnsFrpCredential(mContext, - mUserManager.getUserInfo(userId))) { - if (pwd != null && pwd.credentialType != LockPatternUtils.CREDENTIAL_TYPE_NONE) { + if (shouldSynchronizeFrpCredential(pwd, userId)) { + Slogf.d(TAG, "Syncing Gatekeeper-based FRP credential tied to user %d", userId); + if (!isNoneCredential(pwd)) { mStorage.writePersistentDataBlock(PersistentData.TYPE_SP, userId, requestedQuality, pwd.toBytes()); } else { @@ -924,10 +974,9 @@ public class SyntheticPasswordManager { private void synchronizeWeaverFrpPassword(@Nullable PasswordData pwd, int requestedQuality, int userId, int weaverSlot) { - if (mStorage.getPersistentDataBlockManager() != null - && LockPatternUtils.userOwnsFrpCredential(mContext, - mUserManager.getUserInfo(userId))) { - if (pwd != null && pwd.credentialType != LockPatternUtils.CREDENTIAL_TYPE_NONE) { + if (shouldSynchronizeFrpCredential(pwd, userId)) { + Slogf.d(TAG, "Syncing Weaver-based FRP credential tied to user %d", userId); + if (!isNoneCredential(pwd)) { mStorage.writePersistentDataBlock(PersistentData.TYPE_SP_WEAVER, weaverSlot, requestedQuality, pwd.toBytes()); } else { @@ -1058,7 +1107,7 @@ public class SyntheticPasswordManager { AuthenticationResult result = new AuthenticationResult(); if (protectorId == SyntheticPasswordManager.NULL_PROTECTOR_ID) { - // This should never happen, due to the migration done in LSS.bootCompleted(). + // This should never happen, due to the migration done in LSS.onThirdPartyAppsStarted(). Slogf.wtf(TAG, "Synthetic password not found for user %d", userId); result.gkResponse = VerifyCredentialResponse.ERROR; return result; diff --git a/services/tests/servicestests/src/com/android/server/locksettings/LockscreenFrpTest.java b/services/tests/servicestests/src/com/android/server/locksettings/LockscreenFrpTest.java index d05af76f8f5ad..fc0ca7eda2438 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/LockscreenFrpTest.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/LockscreenFrpTest.java @@ -98,6 +98,7 @@ public class LockscreenFrpTest extends BaseLockSettingsServiceTests { mService.setLockCredential(newPassword("1234"), nonePassword(), PRIMARY_USER_ID); assertEquals(CREDENTIAL_TYPE_PASSWORD, mService.getCredentialType(USER_FRP)); + setDeviceProvisioned(true); mService.setLockCredential(nonePassword(), newPassword("1234"), PRIMARY_USER_ID); assertEquals(CREDENTIAL_TYPE_NONE, mService.getCredentialType(USER_FRP)); } diff --git a/services/tests/servicestests/src/com/android/server/locksettings/SyntheticPasswordTests.java b/services/tests/servicestests/src/com/android/server/locksettings/SyntheticPasswordTests.java index 3f4bec6530914..b9cafa4e24239 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/SyntheticPasswordTests.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/SyntheticPasswordTests.java @@ -136,6 +136,43 @@ public class SyntheticPasswordTests extends BaseLockSettingsServiceTests { assertTrue(mService.isSyntheticPasswordBasedCredential(userId)); } + protected void initializeSyntheticPassword(int userId) { + synchronized (mService.mSpManager) { + mService.initializeSyntheticPasswordLocked(userId); + } + } + + // Tests that the FRP credential is updated when an LSKF-based protector is created for the user + // that owns the FRP credential, if the device is already provisioned. + @Test + public void testFrpCredentialSyncedIfDeviceProvisioned() throws RemoteException { + setDeviceProvisioned(true); + initializeSyntheticPassword(PRIMARY_USER_ID); + verify(mStorage.mPersistentDataBlockManager).setFrpCredentialHandle(any()); + } + + // Tests that the FRP credential is not updated when an LSKF-based protector is created for the + // user that owns the FRP credential, if the new credential is empty and the device is not yet + // provisioned. + @Test + public void testEmptyFrpCredentialNotSyncedIfDeviceNotProvisioned() throws RemoteException { + setDeviceProvisioned(false); + initializeSyntheticPassword(PRIMARY_USER_ID); + verify(mStorage.mPersistentDataBlockManager, never()).setFrpCredentialHandle(any()); + } + + // Tests that the FRP credential is updated when an LSKF-based protector is created for the user + // that owns the FRP credential, if the new credential is nonempty and the device is not yet + // provisioned. + @Test + public void testNonEmptyFrpCredentialSyncedIfDeviceNotProvisioned() throws RemoteException { + setDeviceProvisioned(false); + initializeSyntheticPassword(PRIMARY_USER_ID); + verify(mStorage.mPersistentDataBlockManager, never()).setFrpCredentialHandle(any()); + mService.setLockCredential(newPassword("password"), nonePassword(), PRIMARY_USER_ID); + verify(mStorage.mPersistentDataBlockManager).setFrpCredentialHandle(any()); + } + @Test public void testChangeCredential() throws RemoteException { final LockscreenCredential password = newPassword("password"); diff --git a/services/tests/servicestests/src/com/android/server/locksettings/WeaverBasedSyntheticPasswordTests.java b/services/tests/servicestests/src/com/android/server/locksettings/WeaverBasedSyntheticPasswordTests.java index a3ac5153a03d5..6c13a6fe04a04 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/WeaverBasedSyntheticPasswordTests.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/WeaverBasedSyntheticPasswordTests.java @@ -1,11 +1,17 @@ package com.android.server.locksettings; +import static org.junit.Assert.assertEquals; + import android.platform.test.annotations.Presubmit; import androidx.test.filters.SmallTest; import androidx.test.runner.AndroidJUnit4; +import com.android.server.locksettings.LockSettingsStorage.PersistentData; +import com.google.android.collect.Sets; + import org.junit.Before; +import org.junit.Test; import org.junit.runner.RunWith; @SmallTest @@ -17,4 +23,36 @@ public class WeaverBasedSyntheticPasswordTests extends SyntheticPasswordTests { public void enableWeaver() throws Exception { mSpManager.enableWeaver(); } + + // Tests that if the device is not yet provisioned and the FRP credential uses Weaver, then the + // Weaver slot of the FRP credential is not reused. Assumes that Weaver slots are allocated + // sequentially, starting at slot 0. + @Test + public void testFrpWeaverSlotNotReused() { + final int userId = 10; + final int frpWeaverSlot = 0; + + setDeviceProvisioned(false); + assertEquals(Sets.newHashSet(), mPasswordSlotManager.getUsedSlots()); + mStorage.writePersistentDataBlock(PersistentData.TYPE_SP_WEAVER, frpWeaverSlot, 0, + new byte[1]); + initializeSyntheticPassword(userId); // This should allocate a Weaver slot. + assertEquals(Sets.newHashSet(1), mPasswordSlotManager.getUsedSlots()); + } + + // Tests that if the device is already provisioned and the FRP credential uses Weaver, then the + // Weaver slot of the FRP credential is reused. This is not a very interesting test by itself; + // it's here as a control for testFrpWeaverSlotNotReused(). + @Test + public void testFrpWeaverSlotReused() { + final int userId = 10; + final int frpWeaverSlot = 0; + + setDeviceProvisioned(true); + assertEquals(Sets.newHashSet(), mPasswordSlotManager.getUsedSlots()); + mStorage.writePersistentDataBlock(PersistentData.TYPE_SP_WEAVER, frpWeaverSlot, 0, + new byte[1]); + initializeSyntheticPassword(userId); // This should allocate a Weaver slot. + assertEquals(Sets.newHashSet(0), mPasswordSlotManager.getUsedSlots()); + } }