From 96ad5160e59719b870826e93fa7d17898a0e6dfc Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Tue, 10 Jan 2023 19:38:51 +0000 Subject: [PATCH 1/2] LockSettingsStorage: gracefully handle null keys in database Bug: 261860102 Test: com.android.server.locksettings Change-Id: I7606dad85826d82d663a701e9e7bccb9d908da98 --- .../server/locksettings/LockSettingsStorage.java | 8 ++++++-- .../locksettings/LockSettingsStorageTests.java | 14 ++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/services/core/java/com/android/server/locksettings/LockSettingsStorage.java b/services/core/java/com/android/server/locksettings/LockSettingsStorage.java index 2c28af1cc6183..434c0d7f955ae 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsStorage.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsStorage.java @@ -62,6 +62,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; import java.util.Map; +import java.util.Objects; /** * Storage for the lock settings service. @@ -886,12 +887,15 @@ class LockSettingsStorage { if (!(obj instanceof CacheKey)) return false; CacheKey o = (CacheKey) obj; - return userId == o.userId && type == o.type && key.equals(o.key); + return userId == o.userId && type == o.type && Objects.equals(key, o.key); } @Override public int hashCode() { - return key.hashCode() ^ userId ^ type; + int hashCode = Objects.hashCode(key); + hashCode = 31 * hashCode + userId; + hashCode = 31 * hashCode + type; + return hashCode; } } } 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 03d5b17d7fa85..05208441e3f2c 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTests.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTests.java @@ -265,6 +265,20 @@ public class LockSettingsStorageTests { assertEquals("Cached value didn't match stored value", storage, cached); } + @Test + public void testNullKey() { + mStorage.setString(null, "value", 0); + + // Verify that this doesn't throw an exception. + assertEquals("value", mStorage.readKeyValue(null, null, 0)); + + // The read that happens as part of prefetchUser shouldn't throw an exception either. + mStorage.clearCache(); + mStorage.prefetchUser(0); + + assertEquals("value", mStorage.readKeyValue(null, null, 0)); + } + @Test public void testRemoveUser() { mStorage.writeKeyValue("key", "value", 0); From f0f6d42bd537bfbb036d459cc19d84ab6db6857e Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Wed, 11 Jan 2023 22:54:51 +0000 Subject: [PATCH 2/2] LockSettingsService: reject null keys in setString() et al. Make the setBoolean(), setLong(), and setString() methods of ILockSettings throw an exception if the given key is null, rather than allowing it to be inserted into the locksettings database, where it previously would cause a crash upon preloading. This is a small cleanup only; we probably should go further and use an allowlist of key names. Still, the ACCESS_KEYGUARD_SECURE_STORAGE permission is required to call these methods, so there doesn't appear to be any security impact from the lax input validation. Bug: 261860102 Test: com.android.server.locksettings Change-Id: Ic8632068b92182425e78e2f28a1c4016b7af4a3b --- .../server/locksettings/LockSettingsService.java | 3 +++ .../locksettings/LockSettingsServiceTests.java | 15 +++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/services/core/java/com/android/server/locksettings/LockSettingsService.java b/services/core/java/com/android/server/locksettings/LockSettingsService.java index 121b7c87223bf..31b8ef28f2464 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsService.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsService.java @@ -1156,18 +1156,21 @@ public class LockSettingsService extends ILockSettings.Stub { @Override public void setBoolean(String key, boolean value, int userId) { checkWritePermission(); + Objects.requireNonNull(key); mStorage.setBoolean(key, value, userId); } @Override public void setLong(String key, long value, int userId) { checkWritePermission(); + Objects.requireNonNull(key); mStorage.setLong(key, value, userId); } @Override public void setString(String key, String value, int userId) { checkWritePermission(); + Objects.requireNonNull(key); mStorage.setString(key, value, userId); } 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 41e3a08be6c56..60a033fde4270 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsServiceTests.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsServiceTests.java @@ -423,6 +423,21 @@ public class LockSettingsServiceTests extends BaseLockSettingsServiceTests { checkPasswordHistoryLength(userId, 3); } + @Test(expected=NullPointerException.class) + public void testSetBooleanRejectsNullKey() { + mService.setBoolean(null, false, 0); + } + + @Test(expected=NullPointerException.class) + public void testSetLongRejectsNullKey() { + mService.setLong(null, 0, 0); + } + + @Test(expected=NullPointerException.class) + public void testSetStringRejectsNullKey() { + mService.setString(null, "value", 0); + } + private void checkPasswordHistoryLength(int userId, int expectedLen) { String history = mService.getString(LockPatternUtils.PASSWORD_HISTORY_KEY, "", userId); String[] hashes = TextUtils.split(history, LockPatternUtils.PASSWORD_HISTORY_DELIMITER);