From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Songchun Fan <schfan@google.com>
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 33b506468e11..c27c43d0cdc7 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.server.LocalServices;
 
@@ -358,8 +359,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);
@@ -379,8 +380,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;
@@ -420,8 +422,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);
 
         StatsLog.write(StatsLog.SETTING_CHANGED, name, /* value= */ "", /* newValue= */ "",
             oldState.value, /* tag */ "", false, getUserIdFromKey(mKey),
@@ -443,15 +449,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()) {
@@ -582,8 +589,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;
         }
@@ -592,7 +599,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);
     }
@@ -1216,4 +1223,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));
     }
 }
