From 3d79cd1bb1ce5bd0fba605a0f16790a855ee5bdf Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Thu, 7 Jul 2022 19:00:49 +0000 Subject: [PATCH 1/4] Fix misleading code in sendCredentialsOnUnlockIfRequired() lockScreenSecretAvailable() takes a @NonNull credential, so the code in sendCredentialsOnUnlockIfRequired() that conditionally passes a null credential is incorrect. This doesn't actually matter, since sendCredentialsOnUnlockIfRequired() is never called with CREDENTIAL_TYPE_NONE anyway. But make it do the more logical thing of skipping the call to lockScreenSecretAvailable() in this case. Test: atest com.android.server.locksettings Change-Id: I026f72ccaadef94a2f43cb6c2a70e8c08d21360c --- .../android/server/locksettings/LockSettingsService.java | 9 ++++++--- 1 file changed, 6 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 ee8a23c08b4a6..e9ef8a7bfde15 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsService.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsService.java @@ -1588,6 +1588,11 @@ public class LockSettingsService extends ILockSettings.Stub { return; } + // Don't send empty credentials on unlock. + if (credential.isNone()) { + return; + } + // A profile with a unified lock screen stores a randomly generated credential, so skip it. // Its parent will send credentials for the profile, as it stores the unified lock // credential. @@ -1595,12 +1600,10 @@ public class LockSettingsService extends ILockSettings.Stub { return; } - // RecoverableKeyStoreManager expects null for empty credential. - final byte[] secret = credential.isNone() ? null : credential.getCredential(); // Send credentials for the user and any child profiles that share its lock screen. for (int profileId : getProfilesWithSameLockScreen(userId)) { mRecoverableKeyStoreManager.lockScreenSecretAvailable( - credential.getType(), secret, profileId); + credential.getType(), credential.getCredential(), profileId); } } From 01a1f52fb4b5ca6e0ffffbed1f6fdd5b12dd32c1 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Thu, 7 Jul 2022 19:00:50 +0000 Subject: [PATCH 2/4] Remove LockSettingsStorage.CredentialHash LockSettingsStorage.CredentialHash is no longer used, so remove it. Fix the documentation and parameter naming in some RecoverableKeyStoreManager methods which claimed to be related to CredentialHash but really are not. Test: atest com.android.server.locksettings Change-Id: Ie2c27d37eba685a917df25dbb59ccf12bd3ad05c --- .../locksettings/LockSettingsStorage.java | 34 ------------------- .../recoverablekeystore/KeySyncTask.java | 2 +- .../RecoverableKeyStoreManager.java | 21 ++++++------ 3 files changed, 11 insertions(+), 46 deletions(-) diff --git a/services/core/java/com/android/server/locksettings/LockSettingsStorage.java b/services/core/java/com/android/server/locksettings/LockSettingsStorage.java index 8161503652ffd..b0fc71909bf24 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsStorage.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsStorage.java @@ -44,7 +44,6 @@ import com.android.internal.util.ArrayUtils; import com.android.internal.util.IndentingPrintWriter; import com.android.internal.util.Preconditions; import com.android.internal.widget.LockPatternUtils; -import com.android.internal.widget.LockPatternUtils.CredentialType; import com.android.server.LocalServices; import com.android.server.PersistentDataBlockManagerInternal; import com.android.server.utils.WatchableImpl; @@ -108,39 +107,6 @@ class LockSettingsStorage extends WatchableImpl { private PersistentDataBlockManagerInternal mPersistentDataBlockManagerInternal; - @VisibleForTesting - public static class CredentialHash { - - private CredentialHash(byte[] hash, @CredentialType int type) { - if (type != LockPatternUtils.CREDENTIAL_TYPE_NONE) { - if (hash == null) { - throw new IllegalArgumentException("Empty hash for CredentialHash"); - } - } else /* type == LockPatternUtils.CREDENTIAL_TYPE_NONE */ { - if (hash != null) { - throw new IllegalArgumentException( - "None type CredentialHash should not have hash"); - } - } - this.hash = hash; - this.type = type; - } - - static CredentialHash create(byte[] hash, int type) { - if (type == LockPatternUtils.CREDENTIAL_TYPE_NONE) { - throw new IllegalArgumentException("Bad type for CredentialHash"); - } - return new CredentialHash(hash, type); - } - - static CredentialHash createEmptyHash() { - return new CredentialHash(null, LockPatternUtils.CREDENTIAL_TYPE_NONE); - } - - byte[] hash; - @CredentialType int type; - } - public LockSettingsStorage(Context context) { mContext = context; mOpenHelper = new DatabaseHelper(context); diff --git a/services/core/java/com/android/server/locksettings/recoverablekeystore/KeySyncTask.java b/services/core/java/com/android/server/locksettings/recoverablekeystore/KeySyncTask.java index 2cb89b314b338..ec0d9856486db 100644 --- a/services/core/java/com/android/server/locksettings/recoverablekeystore/KeySyncTask.java +++ b/services/core/java/com/android/server/locksettings/recoverablekeystore/KeySyncTask.java @@ -122,7 +122,7 @@ public class KeySyncTask implements Runnable { * @param recoverableKeyStoreDb Database where the keys are stored. * @param userId The uid of the user whose profile has been unlocked. * @param credentialType The type of credential as defined in {@code LockPatternUtils} - * @param credential The credential, encoded as a {@link String}. + * @param credential The credential, encoded as a byte array * @param credentialUpdated signals weather credentials were updated. * @param platformKeyManager platform key manager * @param testOnlyInsecureCertificateHelper utility class used for end-to-end tests diff --git a/services/core/java/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManager.java b/services/core/java/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManager.java index b49bced4e567f..e620c80b8e285 100644 --- a/services/core/java/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManager.java +++ b/services/core/java/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManager.java @@ -900,14 +900,13 @@ public class RecoverableKeyStoreManager { /** * This function can only be used inside LockSettingsService. * - * @param storedHashType from {@code CredentialHash} - * @param credential - unencrypted byte array. Password length should be at most 16 symbols - * {@code mPasswordMaxLength} - * @param userId for user who just unlocked the device. + * @param credentialType the type of credential, as defined in {@code LockPatternUtils} + * @param credential the credential, encoded as a byte array + * @param userId the ID of the user to whom the credential belongs * @hide */ public void lockScreenSecretAvailable( - int storedHashType, @NonNull byte[] credential, int userId) { + int credentialType, @NonNull byte[] credential, int userId) { // So as not to block the critical path unlocking the phone, defer to another thread. try { mExecutorService.schedule(KeySyncTask.newInstance( @@ -916,7 +915,7 @@ public class RecoverableKeyStoreManager { mSnapshotStorage, mListenersStorage, userId, - storedHashType, + credentialType, credential, /*credentialUpdated=*/ false), SYNC_DELAY_MILLIS, @@ -934,13 +933,13 @@ public class RecoverableKeyStoreManager { /** * This function can only be used inside LockSettingsService. * - * @param storedHashType from {@code CredentialHash} - * @param credential - unencrypted byte array - * @param userId for the user whose lock screen credentials were changed. + * @param credentialType the type of the new credential, as defined in {@code LockPatternUtils} + * @param credential the new credential, encoded as a byte array + * @param userId the ID of the user whose credential was changed * @hide */ public void lockScreenSecretChanged( - int storedHashType, + int credentialType, @Nullable byte[] credential, int userId) { // So as not to block the critical path unlocking the phone, defer to another thread. @@ -951,7 +950,7 @@ public class RecoverableKeyStoreManager { mSnapshotStorage, mListenersStorage, userId, - storedHashType, + credentialType, credential, /*credentialUpdated=*/ true), SYNC_DELAY_MILLIS, From 2515cd5c161e7fd3686b16c9e04596892272cb1f Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Thu, 7 Jul 2022 19:00:51 +0000 Subject: [PATCH 3/4] Clean up the LockSettingsStorage file methods For file paths, consistently use File instead of String, and don't use ambiguous variable names like 'name' and 'key'. Remove unnecessary conversions to absolute paths, as the paths are all absolute already. Consistently use 'data' when referring to a file's data, instead of 'hash', 'stored', or 'value'. Don't catch exceptions from deleteFile(), as it doesn't throw exceptions. Remove the DEBUG field, as it's hardcoded to false, and it only enabled the printing of two messages which were redundant with each other. Test: atest com.android.server.locksettings Change-Id: If0dd16d8c16015d129bda75e93600e60a1341f82 --- .../locksettings/LockSettingsStorage.java | 141 ++++++++---------- .../LockSettingsStorageTestable.java | 31 ++-- .../LockSettingsStorageTests.java | 9 +- 3 files changed, 80 insertions(+), 101 deletions(-) diff --git a/services/core/java/com/android/server/locksettings/LockSettingsStorage.java b/services/core/java/com/android/server/locksettings/LockSettingsStorage.java index b0fc71909bf24..10831c7e8f4ca 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsStorage.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsStorage.java @@ -70,7 +70,6 @@ class LockSettingsStorage extends WatchableImpl { private static final String TAG = "LockSettingsStorage"; private static final String TABLE = "locksettings"; - private static final boolean DEBUG = false; private static final String COLUMN_KEY = "name"; private static final String COLUMN_USERID = "user"; @@ -83,11 +82,10 @@ class LockSettingsStorage extends WatchableImpl { COLUMN_KEY, COLUMN_VALUE }; - private static final String SYSTEM_DIRECTORY = "/system/"; private static final String CHILD_PROFILE_LOCK_FILE = "gatekeeper.profile.key"; private static final String REBOOT_ESCROW_FILE = "reboot.escrow.key"; - private static final String REBOOT_ESCROW_SERVER_BLOB = "reboot.escrow.server.blob.key"; + private static final String REBOOT_ESCROW_SERVER_BLOB_FILE = "reboot.escrow.server.blob.key"; private static final String SYNTHETIC_PASSWORD_DIRECTORY = "spblob/"; @@ -225,13 +223,7 @@ class LockSettingsStorage extends WatchableImpl { } public void removeChildProfileLock(int userId) { - if (DEBUG) - Slog.e(TAG, "Remove child profile lock for user: " + userId); - try { - deleteFile(getChildProfileLockFile(userId)); - } catch (Exception e) { - e.printStackTrace(); - } + deleteFile(getChildProfileLockFile(userId)); } public void writeChildProfileLock(int userId, byte[] lock) { @@ -263,39 +255,39 @@ class LockSettingsStorage extends WatchableImpl { } public void writeRebootEscrowServerBlob(byte[] serverBlob) { - writeFile(getRebootEscrowServerBlob(), serverBlob); + writeFile(getRebootEscrowServerBlobFile(), serverBlob); } public byte[] readRebootEscrowServerBlob() { - return readFile(getRebootEscrowServerBlob()); + return readFile(getRebootEscrowServerBlobFile()); } public boolean hasRebootEscrowServerBlob() { - return hasFile(getRebootEscrowServerBlob()); + return hasFile(getRebootEscrowServerBlobFile()); } public void removeRebootEscrowServerBlob() { - deleteFile(getRebootEscrowServerBlob()); + deleteFile(getRebootEscrowServerBlobFile()); } - private boolean hasFile(String name) { - byte[] contents = readFile(name); + private boolean hasFile(File path) { + byte[] contents = readFile(path); return contents != null && contents.length > 0; } - private byte[] readFile(String name) { + private byte[] readFile(File path) { int version; synchronized (mCache) { - if (mCache.hasFile(name)) { - return mCache.peekFile(name); + if (mCache.hasFile(path)) { + return mCache.peekFile(path); } version = mCache.getVersion(); } - byte[] stored = null; - try (RandomAccessFile raf = new RandomAccessFile(name, "r")) { - stored = new byte[(int) raf.length()]; - raf.readFully(stored, 0, stored.length); + byte[] data = null; + try (RandomAccessFile raf = new RandomAccessFile(path, "r")) { + data = new byte[(int) raf.length()]; + raf.readFully(data, 0, data.length); raf.close(); } catch (FileNotFoundException suppressed) { // readFile() is also called by hasFile() to check the existence of files, in this @@ -303,8 +295,8 @@ class LockSettingsStorage extends WatchableImpl { } catch (IOException e) { Slog.e(TAG, "Cannot read file " + e); } - mCache.putFileIfUnchanged(name, stored, version); - return stored; + mCache.putFileIfUnchanged(path, data, version); + return data; } private void fsyncDirectory(File directory) { @@ -318,23 +310,21 @@ class LockSettingsStorage extends WatchableImpl { } } - private void writeFile(String name, byte[] hash) { + private void writeFile(File path, byte[] data) { synchronized (mFileWriteLock) { RandomAccessFile raf = null; try { - // Write the hash to file, requiring each write to be synchronized to the + // Write the data to the file, requiring each write to be synchronized to the // underlying storage device immediately to avoid data loss in case of power loss. - // This also ensures future secdiscard operation on the file succeeds since the - // file would have been allocated on flash. - raf = new RandomAccessFile(name, "rws"); - // Truncate the file if pattern is null, to clear the lock - if (hash == null || hash.length == 0) { + raf = new RandomAccessFile(path, "rws"); + // Truncate the file if the data is empty. + if (data == null || data.length == 0) { raf.setLength(0); } else { - raf.write(hash, 0, hash.length); + raf.write(data, 0, data.length); } raf.close(); - fsyncDirectory((new File(name)).getAbsoluteFile().getParentFile()); + fsyncDirectory(path.getParentFile()); } catch (IOException e) { Slog.e(TAG, "Error writing to file " + e); } finally { @@ -346,70 +336,66 @@ class LockSettingsStorage extends WatchableImpl { } } } - mCache.putFile(name, hash); + mCache.putFile(path, data); dispatchChange(this); } } - private void deleteFile(String name) { - if (DEBUG) Slog.e(TAG, "Delete file " + name); + private void deleteFile(File path) { synchronized (mFileWriteLock) { - File file = new File(name); - if (file.exists()) { - file.delete(); - mCache.putFile(name, null); + if (path.exists()) { + path.delete(); + mCache.putFile(path, null); } dispatchChange(this); } } @VisibleForTesting - String getChildProfileLockFile(int userId) { - return getLockCredentialFilePathForUser(userId, CHILD_PROFILE_LOCK_FILE); + File getChildProfileLockFile(int userId) { + return getLockCredentialFileForUser(userId, CHILD_PROFILE_LOCK_FILE); } @VisibleForTesting - String getRebootEscrowFile(int userId) { - return getLockCredentialFilePathForUser(userId, REBOOT_ESCROW_FILE); + File getRebootEscrowFile(int userId) { + return getLockCredentialFileForUser(userId, REBOOT_ESCROW_FILE); } @VisibleForTesting - String getRebootEscrowServerBlob() { + File getRebootEscrowServerBlobFile() { // There is a single copy of server blob for all users. - return getLockCredentialFilePathForUser(UserHandle.USER_SYSTEM, REBOOT_ESCROW_SERVER_BLOB); + return getLockCredentialFileForUser(UserHandle.USER_SYSTEM, REBOOT_ESCROW_SERVER_BLOB_FILE); } - private String getLockCredentialFilePathForUser(int userId, String basename) { - String dataSystemDirectory = Environment.getDataDirectory().getAbsolutePath() + - SYSTEM_DIRECTORY; + private File getLockCredentialFileForUser(int userId, String fileName) { if (userId == 0) { - // Leave it in the same place for user 0 - return dataSystemDirectory + basename; + // The files for user 0 are stored directly in /data/system, since this is where they + // originally were, and they haven't been moved yet. + return new File(Environment.getDataSystemDirectory(), fileName); } else { - return new File(Environment.getUserSystemDirectory(userId), basename).getAbsolutePath(); + return new File(Environment.getUserSystemDirectory(userId), fileName); } } public void writeSyntheticPasswordState(int userId, long handle, String name, byte[] data) { ensureSyntheticPasswordDirectoryForUser(userId); - writeFile(getSynthenticPasswordStateFilePathForUser(userId, handle, name), data); + writeFile(getSyntheticPasswordStateFileForUser(userId, handle, name), data); } public byte[] readSyntheticPasswordState(int userId, long handle, String name) { - return readFile(getSynthenticPasswordStateFilePathForUser(userId, handle, name)); + return readFile(getSyntheticPasswordStateFileForUser(userId, handle, name)); } public void deleteSyntheticPasswordState(int userId, long handle, String name) { - String path = getSynthenticPasswordStateFilePathForUser(userId, handle, name); - File file = new File(path); - if (file.exists()) { + File path = getSyntheticPasswordStateFileForUser(userId, handle, name); + if (path.exists()) { try (RandomAccessFile raf = new RandomAccessFile(path, "rws")) { final int fileSize = (int) raf.length(); raf.write(new byte[fileSize]); } catch (Exception e) { Slog.w(TAG, "Failed to zeroize " + path, e); } finally { - file.delete(); + path.delete(); dispatchChange(this); } mCache.putFile(path, null); @@ -447,7 +433,7 @@ class LockSettingsStorage extends WatchableImpl { @VisibleForTesting protected File getSyntheticPasswordDirectoryForUser(int userId) { - return new File(Environment.getDataSystemDeDirectory(userId) ,SYNTHETIC_PASSWORD_DIRECTORY); + return new File(Environment.getDataSystemDeDirectory(userId), SYNTHETIC_PASSWORD_DIRECTORY); } /** Ensure per-user directory for synthetic password state exists */ @@ -458,12 +444,9 @@ class LockSettingsStorage extends WatchableImpl { } } - @VisibleForTesting - protected String getSynthenticPasswordStateFilePathForUser(int userId, long handle, - String name) { - final File baseDir = getSyntheticPasswordDirectoryForUser(userId); - final String baseName = formatSimple("%016x.%s", handle, name); - return new File(baseDir, baseName).getAbsolutePath(); + private File getSyntheticPasswordStateFileForUser(int userId, long handle, String name) { + String fileName = formatSimple("%016x.%s", handle, name); + return new File(getSyntheticPasswordDirectoryForUser(userId), fileName); } public void removeUser(int userId) { @@ -489,7 +472,7 @@ class LockSettingsStorage extends WatchableImpl { // The directory itself will be deleted as part of user deletion operation by the // framework, so only need to purge cache here. //TODO: (b/34600579) invoke secdiscardable - mCache.purgePath(spStateDir.getAbsolutePath()); + mCache.purgePath(spStateDir); } finally { db.endTransaction(); } @@ -670,7 +653,7 @@ class LockSettingsStorage extends WatchableImpl { final UserManager um = UserManager.get(mContext); for (UserInfo user : um.getUsers()) { File userPath = getSyntheticPasswordDirectoryForUser(user.id); - pw.println(String.format("User %d [%s]:", user.id, userPath.getAbsolutePath())); + pw.println(String.format("User %d [%s]:", user.id, userPath)); pw.increaseIndent(); File[] files = userPath.listFiles(); if (files != null) { @@ -783,20 +766,21 @@ class LockSettingsStorage extends WatchableImpl { remove(CacheKey.TYPE_KEY_VALUE, key, userId); } - byte[] peekFile(String fileName) { - return copyOf((byte[]) peek(CacheKey.TYPE_FILE, fileName, -1 /* userId */)); + byte[] peekFile(File path) { + return copyOf((byte[]) peek(CacheKey.TYPE_FILE, path.toString(), -1 /* userId */)); } - boolean hasFile(String fileName) { - return contains(CacheKey.TYPE_FILE, fileName, -1 /* userId */); + boolean hasFile(File path) { + return contains(CacheKey.TYPE_FILE, path.toString(), -1 /* userId */); } - void putFile(String key, byte[] value) { - put(CacheKey.TYPE_FILE, key, copyOf(value), -1 /* userId */); + void putFile(File path, byte[] data) { + put(CacheKey.TYPE_FILE, path.toString(), copyOf(data), -1 /* userId */); } - void putFileIfUnchanged(String key, byte[] value, int version) { - putIfUnchanged(CacheKey.TYPE_FILE, key, copyOf(value), -1 /* userId */, version); + void putFileIfUnchanged(File path, byte[] data, int version) { + putIfUnchanged(CacheKey.TYPE_FILE, path.toString(), copyOf(data), -1 /* userId */, + version); } void setFetched(int userId) { @@ -854,10 +838,11 @@ class LockSettingsStorage extends WatchableImpl { return data != null ? Arrays.copyOf(data, data.length) : null; } - synchronized void purgePath(String path) { + synchronized void purgePath(File path) { + final String pathStr = path.toString(); for (int i = mCache.size() - 1; i >= 0; i--) { CacheKey entry = mCache.keyAt(i); - if (entry.type == CacheKey.TYPE_FILE && entry.key.startsWith(path)) { + if (entry.type == CacheKey.TYPE_FILE && entry.key.startsWith(pathStr)) { mCache.removeAt(i); } } diff --git a/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTestable.java b/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTestable.java index c30af4cae59dc..3f3b8d770f618 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTestable.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTestable.java @@ -32,7 +32,7 @@ import java.util.Arrays; public class LockSettingsStorageTestable extends LockSettingsStorage { - public File mStorageDir; + public final File mStorageDir; public PersistentDataBlockManagerInternal mPersistentDataBlockManager; private byte[] mPersistentData; @@ -64,25 +64,23 @@ public class LockSettingsStorageTestable extends LockSettingsStorage { } @Override - String getChildProfileLockFile(int userId) { - return makeDirs(mStorageDir, - super.getChildProfileLockFile(userId)).getAbsolutePath(); + File getChildProfileLockFile(int userId) { + return remapToStorageDir(super.getChildProfileLockFile(userId)); } @Override - String getRebootEscrowServerBlob() { - return makeDirs(mStorageDir, super.getRebootEscrowServerBlob()).getAbsolutePath(); + File getRebootEscrowServerBlobFile() { + return remapToStorageDir(super.getRebootEscrowServerBlobFile()); } @Override - String getRebootEscrowFile(int userId) { - return makeDirs(mStorageDir, super.getRebootEscrowFile(userId)).getAbsolutePath(); + File getRebootEscrowFile(int userId) { + return remapToStorageDir(super.getRebootEscrowFile(userId)); } @Override protected File getSyntheticPasswordDirectoryForUser(int userId) { - return makeDirs(mStorageDir, super.getSyntheticPasswordDirectoryForUser( - userId).getAbsolutePath()); + return remapToStorageDir(super.getSyntheticPasswordDirectoryForUser(userId)); } @Override @@ -90,14 +88,9 @@ public class LockSettingsStorageTestable extends LockSettingsStorage { return mPersistentDataBlockManager; } - private File makeDirs(File baseDir, String filePath) { - File path = new File(filePath); - if (path.getParent() == null) { - return new File(baseDir, filePath); - } else { - File mappedDir = new File(baseDir, path.getParent()); - mappedDir.mkdirs(); - return new File(mappedDir, path.getName()); - } + private File remapToStorageDir(File origPath) { + File mappedPath = new File(mStorageDir, origPath.toString()); + mappedPath.getParentFile().mkdirs(); + return mappedPath; } } 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 c4df05149d4ba..a6638587bf221 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTests.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTests.java @@ -304,14 +304,15 @@ public class LockSettingsStorageTests { public void testFileLocation_Owner() { LockSettingsStorage storage = new LockSettingsStorage(InstrumentationRegistry.getContext()); - assertEquals("/data/system/gatekeeper.profile.key", storage.getChildProfileLockFile(0)); + assertEquals(new File("/data/system/gatekeeper.profile.key"), + storage.getChildProfileLockFile(0)); } @Test public void testFileLocation_SecondaryUser() { LockSettingsStorage storage = new LockSettingsStorage(InstrumentationRegistry.getContext()); - assertEquals("/data/system/users/1/gatekeeper.profile.key", + assertEquals(new File("/data/system/users/1/gatekeeper.profile.key"), storage.getChildProfileLockFile(1)); } @@ -319,7 +320,7 @@ public class LockSettingsStorageTests { public void testFileLocation_ProfileToSecondary() { LockSettingsStorage storage = new LockSettingsStorage(InstrumentationRegistry.getContext()); - assertEquals("/data/system/users/2/gatekeeper.profile.key", + assertEquals(new File("/data/system/users/2/gatekeeper.profile.key"), storage.getChildProfileLockFile(2)); } @@ -327,7 +328,7 @@ public class LockSettingsStorageTests { public void testFileLocation_ProfileToOwner() { LockSettingsStorage storage = new LockSettingsStorage(InstrumentationRegistry.getContext()); - assertEquals("/data/system/users/3/gatekeeper.profile.key", + assertEquals(new File("/data/system/users/3/gatekeeper.profile.key"), storage.getChildProfileLockFile(3)); } From b20792558e9d9d22fb04cb8994d2a4cc0ce9103e Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Thu, 7 Jul 2022 19:00:52 +0000 Subject: [PATCH 4/4] Fix deleteSyntheticPasswordState() to take mFileWriteLock The cache consistency model in LockSettingsStorage requires that mFileWriteLock be held during file writes and deletions. deleteSyntheticPasswordState() forgot to take this lock. Fix it by making it just call deleteFile(). Make deleteFile() zeroize the file before unlinking it, so that the zeroization that deleteSyntheticPasswordState() did isn't lost, and so that the zeroization is applied to the other locksettings files too. Found by code review; this bug isn't known to have been causing any real-world problems. Test: atest com.android.server.locksettings Change-Id: I8b4c8468a4ab533164b5750e109e2426f74c9110 --- .../locksettings/LockSettingsStorage.java | 24 ++++++++----------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/services/core/java/com/android/server/locksettings/LockSettingsStorage.java b/services/core/java/com/android/server/locksettings/LockSettingsStorage.java index 10831c7e8f4ca..ec66599ebe828 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsStorage.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsStorage.java @@ -344,10 +344,18 @@ class LockSettingsStorage extends WatchableImpl { private void deleteFile(File path) { synchronized (mFileWriteLock) { if (path.exists()) { + // Zeroize the file to try to make its contents unrecoverable. This is *not* + // guaranteed to be effective, and in fact it usually isn't, but it doesn't hurt. + try (RandomAccessFile raf = new RandomAccessFile(path, "rws")) { + final int fileSize = (int) raf.length(); + raf.write(new byte[fileSize]); + } catch (Exception e) { + Slog.w(TAG, "Failed to zeroize " + path, e); + } path.delete(); + dispatchChange(this); mCache.putFile(path, null); } - dispatchChange(this); } } @@ -387,19 +395,7 @@ class LockSettingsStorage extends WatchableImpl { } public void deleteSyntheticPasswordState(int userId, long handle, String name) { - File path = getSyntheticPasswordStateFileForUser(userId, handle, name); - if (path.exists()) { - try (RandomAccessFile raf = new RandomAccessFile(path, "rws")) { - final int fileSize = (int) raf.length(); - raf.write(new byte[fileSize]); - } catch (Exception e) { - Slog.w(TAG, "Failed to zeroize " + path, e); - } finally { - path.delete(); - dispatchChange(this); - } - mCache.putFile(path, null); - } + deleteFile(getSyntheticPasswordStateFileForUser(userId, handle, name)); } public Map> listSyntheticPasswordHandlesForAllUsers(String stateName) {