diff options
author | Songchun Fan <schfan@google.com> | 2022-10-11 18:08:11 -0700 |
---|---|---|
committer | Android Build Coastguard Worker <android-build-coastguard-worker@google.com> | 2022-11-15 01:48:23 +0000 |
commit | 7b0d3908c718b60f3a0329fd9bf87c65f53aaa10 (patch) | |
tree | eb3dad410dd4b5ee440a9de28a93dd54f0d4613c | |
parent | b58e58dd0b695e17885a14899f0dc3ea959aa8e5 (diff) | |
download | base-7b0d3908c718b60f3a0329fd9bf87c65f53aaa10.tar.gz |
[RESTRICT AUTOMERGE][SettingsProvider] 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 f60e5a4b7ce47d21a6e707fdad7a865dc0b0f0a3)
Merged-In: Ic9e88c0cc3d7206c64ba5b5c7d15b50d1ffc9adc
-rw-r--r-- | packages/SettingsProvider/src/com/android/providers/settings/SettingsState.java | 40 | ||||
-rw-r--r-- | packages/SettingsProvider/test/src/com/android/providers/settings/SettingsStateTest.java | 94 |
2 files changed, 120 insertions, 14 deletions
diff --git a/packages/SettingsProvider/src/com/android/providers/settings/SettingsState.java b/packages/SettingsProvider/src/com/android/providers/settings/SettingsState.java index 528af2ec2528..cd667ca05f61 100644 --- a/packages/SettingsProvider/src/com/android/providers/settings/SettingsState.java +++ b/packages/SettingsProvider/src/com/android/providers/settings/SettingsState.java @@ -48,6 +48,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.internal.util.ArrayUtils; import com.android.internal.util.FrameworkStatsLog; @@ -376,8 +377,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); @@ -414,8 +415,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; @@ -559,8 +561,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); FrameworkStatsLog.write(FrameworkStatsLog.SETTING_CHANGED, name, /* value= */ "", /* newValue= */ "", oldState.value, /* tag */ "", false, getUserIdFromKey(mKey), @@ -583,15 +589,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()) { @@ -725,8 +732,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; } @@ -735,7 +742,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); } @@ -1577,4 +1584,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 66b809aeae30..f6d43292eafe 100644 --- a/packages/SettingsProvider/test/src/com/android/providers/settings/SettingsStateTest.java +++ b/packages/SettingsProvider/test/src/com/android/providers/settings/SettingsStateTest.java @@ -295,7 +295,7 @@ public class SettingsStateTest extends AndroidTestCase { settingsState.deleteSettingLocked(SETTING_NAME); // Should not throw if usage is under the cap - settingsState.insertSettingLocked(SETTING_NAME, Strings.repeat("A", 19999), + settingsState.insertSettingLocked(SETTING_NAME, Strings.repeat("A", 19975), null, false, "p1"); settingsState.deleteSettingLocked(SETTING_NAME); try { @@ -313,5 +313,97 @@ public class SettingsStateTest extends AndroidTestCase { assertTrue(ex.getMessage().contains("p1")); } assertTrue(settingsState.getSettingLocked(SETTING_NAME).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() { + SettingsState settingsState = new SettingsState(getContext(), mLock, mSettingsFile, 1, + SettingsState.MAX_BYTES_PER_APP_PACKAGE_LIMITED, Looper.getMainLooper()); + + // Test inserting one key with default + final String testKey1 = SETTING_NAME; + final String testValue1 = Strings.repeat("A", 100); + settingsState.insertSettingLocked(testKey1, testValue1, null, true, TEST_PACKAGE); + int expectedMemUsage = testKey1.length() + testValue1.length() + + testValue1.length() /* size for default */; + assertEquals(expectedMemUsage, settingsState.getMemoryUsage(TEST_PACKAGE)); + + // Test inserting another key + final String testKey2 = SETTING_NAME + "2"; + settingsState.insertSettingLocked(testKey2, testValue1, null, false, TEST_PACKAGE); + expectedMemUsage += testKey2.length() + testValue1.length(); + assertEquals(expectedMemUsage, settingsState.getMemoryUsage(TEST_PACKAGE)); + + // Test updating first key with new default + final String testValue2 = Strings.repeat("A", 300); + settingsState.insertSettingLocked(testKey1, testValue2, null, true, TEST_PACKAGE); + expectedMemUsage += (testValue2.length() - testValue1.length()) * 2; + assertEquals(expectedMemUsage, settingsState.getMemoryUsage(TEST_PACKAGE)); + + // Test updating first key without new default + final String testValue3 = Strings.repeat("A", 50); + settingsState.insertSettingLocked(testKey1, testValue3, null, false, TEST_PACKAGE); + expectedMemUsage -= testValue2.length() - testValue3.length(); + assertEquals(expectedMemUsage, settingsState.getMemoryUsage(TEST_PACKAGE)); + + // Test updating second key + settingsState.insertSettingLocked(testKey2, testValue2, null, false, TEST_PACKAGE); + expectedMemUsage -= testValue1.length() - testValue2.length(); + assertEquals(expectedMemUsage, settingsState.getMemoryUsage(TEST_PACKAGE)); + + // Test resetting key + settingsState.resetSettingLocked(testKey1); + expectedMemUsage += testValue2.length() - testValue3.length(); + assertEquals(expectedMemUsage, settingsState.getMemoryUsage(TEST_PACKAGE)); + + // Test resetting default value + settingsState.resetSettingDefaultValueLocked(testKey1); + expectedMemUsage -= testValue2.length(); + assertEquals(expectedMemUsage, settingsState.getMemoryUsage(TEST_PACKAGE)); + + // Test deletion + settingsState.deleteSettingLocked(testKey2); + expectedMemUsage -= testValue2.length() + testKey2.length() /* key is deleted too */; + assertEquals(expectedMemUsage, settingsState.getMemoryUsage(TEST_PACKAGE)); + + // Test another package with a different key + final String testPackage2 = TEST_PACKAGE + "2"; + final String testKey3 = SETTING_NAME + "3"; + settingsState.insertSettingLocked(testKey3, testValue1, null, true, testPackage2); + assertEquals(expectedMemUsage, settingsState.getMemoryUsage(TEST_PACKAGE)); + final int expectedMemUsage2 = testKey3.length() + testValue1.length() * 2; + assertEquals(expectedMemUsage2, settingsState.getMemoryUsage(testPackage2)); + + // Test system package + settingsState.insertSettingLocked(testKey1, testValue1, null, true, SYSTEM_PACKAGE); + assertEquals(expectedMemUsage, settingsState.getMemoryUsage(TEST_PACKAGE)); + assertEquals(expectedMemUsage2, settingsState.getMemoryUsage(testPackage2)); + assertEquals(0, settingsState.getMemoryUsage(SYSTEM_PACKAGE)); + + // Test invalid value + try { + settingsState.insertSettingLocked(testKey1, Strings.repeat("A", 20001), null, false, + TEST_PACKAGE); + 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(TEST_PACKAGE)); + + // Test invalid key + try { + settingsState.insertSettingLocked(Strings.repeat("A", 20001), "", null, false, + TEST_PACKAGE); + 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(TEST_PACKAGE)); } } |