From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 From: Songchun Fan Date: Tue, 11 Oct 2022 18:08:11 -0700 Subject: [PATCH] key size limit for mutating settings Prior to targetSdk 22, apps could add random system settings keys which opens an opportunity for OOM attacks. This CL adds a key size limit. BUG: 239415997 Test: manual; will add cts test Merged-In: Ic9e88c0cc3d7206c64ba5b5c7d15b50d1ffc9adc Change-Id: Ic9e88c0cc3d7206c64ba5b5c7d15b50d1ffc9adc (cherry picked from commit 783bcba343c480f6ccedaaff41ba7171a1082e0c) (cherry picked from commit f1831c87122e56951c04e1f62f647ab156ca71e3) Merged-In: Ic9e88c0cc3d7206c64ba5b5c7d15b50d1ffc9adc --- .../providers/settings/SettingsState.java | 40 ++++--- .../providers/settings/SettingsStateTest.java | 102 +++++++++++++++++- 2 files changed, 126 insertions(+), 16 deletions(-) diff --git a/packages/SettingsProvider/src/com/android/providers/settings/SettingsState.java b/packages/SettingsProvider/src/com/android/providers/settings/SettingsState.java index 0e94ea85919b..11b4c9eb1c1d 100644 --- a/packages/SettingsProvider/src/com/android/providers/settings/SettingsState.java +++ b/packages/SettingsProvider/src/com/android/providers/settings/SettingsState.java @@ -46,6 +46,7 @@ import android.util.Xml; import android.util.proto.ProtoOutputStream; import com.android.internal.annotations.GuardedBy; +import com.android.internal.annotations.VisibleForTesting; import com.android.server.LocalServices; import libcore.io.IoUtils; @@ -306,8 +307,8 @@ final class SettingsState { Setting newSetting = new Setting(name, oldSetting.getValue(), null, oldSetting.getPackageName(), oldSetting.getTag(), false, oldSetting.getId()); - int newSize = getNewMemoryUsagePerPackageLocked(newSetting.getPackageName(), oldValue, - newSetting.getValue(), oldDefaultValue, newSetting.getDefaultValue()); + int newSize = getNewMemoryUsagePerPackageLocked(newSetting.getPackageName(), 0, + oldValue, newSetting.getValue(), oldDefaultValue, newSetting.getDefaultValue()); checkNewMemoryUsagePerPackageLocked(newSetting.getPackageName(), newSize); mSettings.put(name, newSetting); updateMemoryUsagePerPackageLocked(newSetting.getPackageName(), newSize); @@ -327,8 +328,9 @@ final class SettingsState { String oldDefaultValue = (oldState != null) ? oldState.defaultValue : null; String newDefaultValue = makeDefault ? value : oldDefaultValue; - int newSize = getNewMemoryUsagePerPackageLocked(packageName, oldValue, value, - oldDefaultValue, newDefaultValue); + int newSize = getNewMemoryUsagePerPackageLocked(packageName, + oldValue == null ? name.length() : 0 /* deltaKeySize */, + oldValue, value, oldDefaultValue, newDefaultValue); checkNewMemoryUsagePerPackageLocked(packageName, newSize); Setting newState; @@ -365,8 +367,12 @@ final class SettingsState { } Setting oldState = mSettings.remove(name); - int newSize = getNewMemoryUsagePerPackageLocked(oldState.packageName, oldState.value, - null, oldState.defaultValue, null); + if (oldState == null) { + return false; + } + int newSize = getNewMemoryUsagePerPackageLocked(oldState.packageName, + -name.length() /* deltaKeySize */, + oldState.value, null, oldState.defaultValue, null); updateMemoryUsagePerPackageLocked(oldState.packageName, newSize); @@ -384,15 +390,16 @@ final class SettingsState { } Setting setting = mSettings.get(name); + if (setting == null) { + return false; + } Setting oldSetting = new Setting(setting); String oldValue = setting.getValue(); String oldDefaultValue = setting.getDefaultValue(); - String newValue = oldDefaultValue; - String newDefaultValue = oldDefaultValue; - int newSize = getNewMemoryUsagePerPackageLocked(setting.packageName, oldValue, - newValue, oldDefaultValue, newDefaultValue); + int newSize = getNewMemoryUsagePerPackageLocked(setting.packageName, 0, oldValue, + oldDefaultValue, oldDefaultValue, oldDefaultValue); checkNewMemoryUsagePerPackageLocked(setting.packageName, newSize); if (!setting.reset()) { @@ -521,8 +528,8 @@ final class SettingsState { } @GuardedBy("mLock") - private int getNewMemoryUsagePerPackageLocked(String packageName, String oldValue, - String newValue, String oldDefaultValue, String newDefaultValue) { + private int getNewMemoryUsagePerPackageLocked(String packageName, int deltaKeySize, + String oldValue, String newValue, String oldDefaultValue, String newDefaultValue) { if (isExemptFromMemoryUsageCap(packageName)) { return 0; } @@ -531,7 +538,7 @@ final class SettingsState { final int newValueSize = (newValue != null) ? newValue.length() : 0; final int oldDefaultValueSize = (oldDefaultValue != null) ? oldDefaultValue.length() : 0; final int newDefaultValueSize = (newDefaultValue != null) ? newDefaultValue.length() : 0; - final int deltaSize = newValueSize + newDefaultValueSize + final int deltaSize = deltaKeySize + newValueSize + newDefaultValueSize - oldValueSize - oldDefaultValueSize; return Math.max((currentSize != null) ? currentSize + deltaSize : deltaSize, 0); } @@ -1140,4 +1147,11 @@ final class SettingsState { return false; } } + + @VisibleForTesting + public int getMemoryUsage(String packageName) { + synchronized (mLock) { + return mPackageToMemoryUsage.getOrDefault(packageName, 0); + } + } } diff --git a/packages/SettingsProvider/test/src/com/android/providers/settings/SettingsStateTest.java b/packages/SettingsProvider/test/src/com/android/providers/settings/SettingsStateTest.java index 6f45adef91f7..adb356726eec 100644 --- a/packages/SettingsProvider/test/src/com/android/providers/settings/SettingsStateTest.java +++ b/packages/SettingsProvider/test/src/com/android/providers/settings/SettingsStateTest.java @@ -186,8 +186,8 @@ public class SettingsStateTest extends AndroidTestCase { public void testInsertSetting_memoryUsage() { final Object lock = new Object(); - final File file = new File(getContext().getCacheDir(), "setting.xml"); - final String settingName = "test_setting"; + final File file = new File(getContext().getCacheDir(), "setting.xml"); + final String settingName = "test_setting"; SettingsState settingsState = new SettingsState(getContext(), lock, file, 1, SettingsState.MAX_BYTES_PER_APP_PACKAGE_UNLIMITED, Looper.getMainLooper()); @@ -204,7 +204,7 @@ public class SettingsStateTest extends AndroidTestCase { settingsState.deleteSettingLocked(settingName); // Should not throw if usage is under the cap - settingsState.insertSettingLocked(settingName, Strings.repeat("A", 19999), + settingsState.insertSettingLocked(settingName, Strings.repeat("A", 19975), null, false, "p1"); settingsState.deleteSettingLocked(settingName); try { @@ -222,5 +222,101 @@ public class SettingsStateTest extends AndroidTestCase { assertTrue(ex.getMessage().contains("p1")); } assertTrue(settingsState.getSettingLocked(settingName).isNull()); + try { + settingsState.insertSettingLocked(Strings.repeat("A", 20001), "", + null, false, "p1"); + fail("Should throw because it exceeded per package memory usage"); + } catch (IllegalStateException ex) { + assertTrue(ex.getMessage().contains("You are adding too many system settings")); + } + } + + public void testMemoryUsagePerPackage() { + final Object lock = new Object(); + final File file = new File(getContext().getCacheDir(), "setting.xml"); + final String testPackage = "package"; + SettingsState settingsState = new SettingsState(getContext(), lock, file, 1, + SettingsState.MAX_BYTES_PER_APP_PACKAGE_LIMITED, Looper.getMainLooper()); + + // Test inserting one key with default + final String settingName = "test_setting"; + final String testKey1 = settingName; + final String testValue1 = Strings.repeat("A", 100); + settingsState.insertSettingLocked(testKey1, testValue1, null, true, testPackage); + int expectedMemUsage = testKey1.length() + testValue1.length() + + testValue1.length() /* size for default */; + assertEquals(expectedMemUsage, settingsState.getMemoryUsage(testPackage)); + + // Test inserting another key + final String testKey2 = settingName + "2"; + settingsState.insertSettingLocked(testKey2, testValue1, null, false, testPackage); + expectedMemUsage += testKey2.length() + testValue1.length(); + assertEquals(expectedMemUsage, settingsState.getMemoryUsage(testPackage)); + + // Test updating first key with new default + final String testValue2 = Strings.repeat("A", 300); + settingsState.insertSettingLocked(testKey1, testValue2, null, true, testPackage); + expectedMemUsage += (testValue2.length() - testValue1.length()) * 2; + assertEquals(expectedMemUsage, settingsState.getMemoryUsage(testPackage)); + + // Test updating first key without new default + final String testValue3 = Strings.repeat("A", 50); + settingsState.insertSettingLocked(testKey1, testValue3, null, false, testPackage); + expectedMemUsage -= testValue2.length() - testValue3.length(); + assertEquals(expectedMemUsage, settingsState.getMemoryUsage(testPackage)); + + // Test updating second key + settingsState.insertSettingLocked(testKey2, testValue2, null, false, testPackage); + expectedMemUsage -= testValue1.length() - testValue2.length(); + assertEquals(expectedMemUsage, settingsState.getMemoryUsage(testPackage)); + + // Test resetting key + settingsState.resetSettingLocked(testKey1); + expectedMemUsage += testValue2.length() - testValue3.length(); + assertEquals(expectedMemUsage, settingsState.getMemoryUsage(testPackage)); + + // Test resetting default value + settingsState.resetSettingDefaultValueLocked(testKey1); + expectedMemUsage -= testValue2.length(); + assertEquals(expectedMemUsage, settingsState.getMemoryUsage(testPackage)); + + // Test deletion + settingsState.deleteSettingLocked(testKey2); + expectedMemUsage -= testValue2.length() + testKey2.length() /* key is deleted too */; + assertEquals(expectedMemUsage, settingsState.getMemoryUsage(testPackage)); + + // Test another package with a different key + final String testPackage2 = testPackage + "2"; + final String testKey3 = settingName + "3"; + settingsState.insertSettingLocked(testKey3, testValue1, null, true, testPackage2); + assertEquals(expectedMemUsage, settingsState.getMemoryUsage(testPackage)); + final int expectedMemUsage2 = testKey3.length() + testValue1.length() * 2; + assertEquals(expectedMemUsage2, settingsState.getMemoryUsage(testPackage2)); + + // Test system package + settingsState.insertSettingLocked(testKey1, testValue1, null, true, "android"); + assertEquals(expectedMemUsage, settingsState.getMemoryUsage(testPackage)); + assertEquals(expectedMemUsage2, settingsState.getMemoryUsage(testPackage2)); + assertEquals(0, settingsState.getMemoryUsage("android")); + + // Test invalid value + try { + settingsState.insertSettingLocked(testKey1, Strings.repeat("A", 20001), null, false, + testPackage); + fail("Should throw because it exceeded per package memory usage"); + } catch (IllegalStateException ex) { + assertTrue(ex.getMessage().contains("You are adding too many system settings")); + } + assertEquals(expectedMemUsage, settingsState.getMemoryUsage(testPackage)); + + // Test invalid key + try { + settingsState.insertSettingLocked(Strings.repeat("A", 20001), "", null, false, + testPackage); + fail("Should throw because it exceeded per package memory usage"); + } catch (IllegalStateException ex) { + assertTrue(ex.getMessage().contains("You are adding too many system settings")); + } + assertEquals(expectedMemUsage, settingsState.getMemoryUsage(testPackage)); } }