From 66acc13aefde36973305c469e429a814566807ed Mon Sep 17 00:00:00 2001 From: Bohdan Petrivskyy Date: Thu, 7 Nov 2019 11:48:20 +0000 Subject: [PATCH] Adds DeviceConfig flag configuration banning to SettingsProvider. When resetToDefault() is called on DeviceConfig before reset existing flag configuration is going to be banned. Next time setProperties() is called with some flag configuration a check will be done to verify if the configuration is not banned only if passed the write will be allowed to go through. Test: atest FrameworksCoreTests:DeviceConfigTest Bug: 136134408 Change-Id: I341f51632b131c1cfef85402906b7a0b6e18075b --- core/java/android/provider/Settings.java | 18 ++- .../android/provider/DeviceConfigTest.java | 76 ++++++++++++ .../providers/settings/SettingsProvider.java | 50 +++++++- .../providers/settings/SettingsState.java | 109 +++++++++++++++++- 4 files changed, 242 insertions(+), 11 deletions(-) diff --git a/core/java/android/provider/Settings.java b/core/java/android/provider/Settings.java index 462627e08520d..b350806358c57 100644 --- a/core/java/android/provider/Settings.java +++ b/core/java/android/provider/Settings.java @@ -252,6 +252,9 @@ public final class Settings { /** @hide */ public static final String EXTRA_NETWORK_TEMPLATE = "network_template"; + /** @hide */ + public static final String KEY_CONFIG_SET_RETURN = "config_set_return"; + /** * An int extra specifying a subscription ID. * @@ -2435,13 +2438,14 @@ public final class Settings { args.putString(CALL_METHOD_PREFIX_KEY, prefix); args.putSerializable(CALL_METHOD_FLAGS_KEY, keyValues); IContentProvider cp = mProviderHolder.getProvider(cr); - cp.call(cr.getPackageName(), cr.getFeatureId(), mProviderHolder.mUri.getAuthority(), + Bundle bundle = cp.call(cr.getPackageName(), cr.getFeatureId(), + mProviderHolder.mUri.getAuthority(), mCallSetAllCommand, null, args); + return bundle.getBoolean(KEY_CONFIG_SET_RETURN); } catch (RemoteException e) { // Not supported by the remote side return false; } - return true; } @UnsupportedAppUsage @@ -13893,14 +13897,18 @@ public final class Settings { */ @RequiresPermission(Manifest.permission.WRITE_DEVICE_CONFIG) static boolean setStrings(@NonNull ContentResolver resolver, @NonNull String namespace, - @NonNull Map keyValues) { + @NonNull Map keyValues) throws DeviceConfig.BadConfigException { HashMap compositeKeyValueMap = new HashMap<>(keyValues.keySet().size()); for (Map.Entry entry : keyValues.entrySet()) { compositeKeyValueMap.put( createCompositeName(namespace, entry.getKey()), entry.getValue()); } - return sNameValueCache.setStringsForPrefix(resolver, createPrefix(namespace), - compositeKeyValueMap); + // If can't set given configuration that means it's bad + if (!sNameValueCache.setStringsForPrefix(resolver, createPrefix(namespace), + compositeKeyValueMap)) { + throw new DeviceConfig.BadConfigException(); + } + return true; } /** diff --git a/core/tests/coretests/src/android/provider/DeviceConfigTest.java b/core/tests/coretests/src/android/provider/DeviceConfigTest.java index ae835e4a1c73f..84c42dbc14e76 100644 --- a/core/tests/coretests/src/android/provider/DeviceConfigTest.java +++ b/core/tests/coretests/src/android/provider/DeviceConfigTest.java @@ -20,6 +20,8 @@ import static android.provider.DeviceConfig.Properties; import static com.google.common.truth.Truth.assertThat; +import static org.testng.Assert.assertThrows; + import android.content.ContentResolver; import android.os.Bundle; import android.platform.test.annotations.Presubmit; @@ -44,9 +46,11 @@ public class DeviceConfigTest { private static final String KEY = "key1"; private static final String KEY2 = "key2"; private static final String KEY3 = "key3"; + private static final String KEY4 = "key4"; private static final String VALUE = "value1"; private static final String VALUE2 = "value2"; private static final String VALUE3 = "value3"; + private static final String NULL_VALUE = "null"; @After public void cleanUp() { @@ -561,6 +565,78 @@ public class DeviceConfigTest { assertThat(properties.getFloat("key5", 0f)).isEqualTo(floatValue); } + @Test + public void banNamespaceProperties() throws DeviceConfig.BadConfigException { + // Given namespace will be permanently banned, thus it needs to be different every time + final String namespaceToBan = NAMESPACE + System.currentTimeMillis(); + Properties properties = new Properties.Builder(namespaceToBan).setString(KEY, VALUE) + .setString(KEY4, NULL_VALUE).build(); + // Set namespace properties + DeviceConfig.setProperties(properties); + // Ban namespace with related properties + DeviceConfig.resetToDefaults(Settings.RESET_MODE_PACKAGE_DEFAULTS, namespaceToBan); + // Verify given namespace properties are banned + assertThrows(DeviceConfig.BadConfigException.class, + () -> DeviceConfig.setProperties(properties)); + // Modify properties and verify we can set them + Properties modifiedProperties = new Properties.Builder(namespaceToBan).setString(KEY, VALUE) + .setString(KEY4, NULL_VALUE).setString(KEY2, VALUE2).build(); + DeviceConfig.setProperties(modifiedProperties); + modifiedProperties = DeviceConfig.getProperties(namespaceToBan); + assertThat(modifiedProperties.getKeyset()).containsExactly(KEY, KEY2, KEY4); + assertThat(modifiedProperties.getString(KEY, DEFAULT_VALUE)).isEqualTo(VALUE); + assertThat(modifiedProperties.getString(KEY2, DEFAULT_VALUE)).isEqualTo(VALUE2); + // Since value is null DEFAULT_VALUE should be returned + assertThat(modifiedProperties.getString(KEY4, DEFAULT_VALUE)).isEqualTo(DEFAULT_VALUE); + } + + @Test + public void banEntireDeviceConfig() throws DeviceConfig.BadConfigException { + // Given namespaces will be permanently banned, thus they need to be different every time + final String namespaceToBan1 = NAMESPACE + System.currentTimeMillis(); + final String namespaceToBan2 = NAMESPACE + System.currentTimeMillis() + 1; + + // Set namespaces properties + Properties properties1 = new Properties.Builder(namespaceToBan1).setString(KEY, VALUE) + .setString(KEY4, NULL_VALUE).build(); + DeviceConfig.setProperties(properties1); + Properties properties2 = new Properties.Builder(namespaceToBan2).setString(KEY2, VALUE2) + .setString(KEY4, NULL_VALUE).build(); + DeviceConfig.setProperties(properties2); + + // Ban entire DeviceConfig + DeviceConfig.resetToDefaults(Settings.RESET_MODE_PACKAGE_DEFAULTS, null); + + // Verify given namespace properties are banned + assertThrows(DeviceConfig.BadConfigException.class, + () -> DeviceConfig.setProperties(properties1)); + assertThrows(DeviceConfig.BadConfigException.class, + () -> DeviceConfig.setProperties(properties2)); + + // Modify properties and verify we can set them + Properties modifiedProperties1 = new Properties.Builder(namespaceToBan1).setString(KEY, + VALUE) + .setString(KEY4, NULL_VALUE).setString(KEY2, VALUE2).build(); + DeviceConfig.setProperties(modifiedProperties1); + modifiedProperties1 = DeviceConfig.getProperties(namespaceToBan1); + assertThat(modifiedProperties1.getKeyset()).containsExactly(KEY, KEY2, KEY4); + assertThat(modifiedProperties1.getString(KEY, DEFAULT_VALUE)).isEqualTo(VALUE); + assertThat(modifiedProperties1.getString(KEY2, DEFAULT_VALUE)).isEqualTo(VALUE2); + // Since value is null DEFAULT_VALUE should be returned + assertThat(modifiedProperties1.getString(KEY4, DEFAULT_VALUE)).isEqualTo(DEFAULT_VALUE); + + Properties modifiedProperties2 = new Properties.Builder(namespaceToBan2).setString(KEY, + VALUE) + .setString(KEY4, NULL_VALUE).setString(KEY2, VALUE2).build(); + DeviceConfig.setProperties(modifiedProperties1); + modifiedProperties2 = DeviceConfig.getProperties(namespaceToBan1); + assertThat(modifiedProperties2.getKeyset()).containsExactly(KEY, KEY2, KEY4); + assertThat(modifiedProperties2.getString(KEY, DEFAULT_VALUE)).isEqualTo(VALUE); + assertThat(modifiedProperties2.getString(KEY2, DEFAULT_VALUE)).isEqualTo(VALUE2); + // Since value is null DEFAULT_VALUE should be returned + assertThat(modifiedProperties2.getString(KEY4, DEFAULT_VALUE)).isEqualTo(DEFAULT_VALUE); + } + // TODO(mpape): resolve b/142727848 and re-enable listener tests // @Test // public void onPropertiesChangedListener_setPropertyCallback() throws InterruptedException { diff --git a/packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java b/packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java index 4309c8030dbd5..1e0c1d877f67d 100644 --- a/packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java +++ b/packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java @@ -387,8 +387,10 @@ public class SettingsProvider extends ContentProvider { case Settings.CALL_METHOD_SET_ALL_CONFIG: { String prefix = getSettingPrefix(args); Map flags = getSettingFlags(args); - setAllConfigSettings(prefix, flags); - break; + Bundle result = new Bundle(); + result.putBoolean(Settings.KEY_CONFIG_SET_RETURN, + setAllConfigSettings(prefix, flags)); + return result; } case Settings.CALL_METHOD_RESET_CONFIG: { @@ -2666,20 +2668,28 @@ public class SettingsProvider extends ContentProvider { return success; } + /** + * Set Settings using consumed keyValues, returns true if the keyValues can be set, false + * otherwise. + */ public boolean setSettingsLocked(int type, int userId, String prefix, Map keyValues, String packageName) { final int key = makeKey(type, userId); SettingsState settingsState = peekSettingsStateLocked(key); if (settingsState != null) { + if (SETTINGS_TYPE_CONFIG == type && settingsState.isNewConfigBannedLocked(prefix, + keyValues)) { + return false; + } List changedSettings = settingsState.setSettingsLocked(prefix, keyValues, packageName); if (!changedSettings.isEmpty()) { notifyForConfigSettingsChangeLocked(key, prefix, changedSettings); } } - - return settingsState != null; + // keyValues aren't banned and can be set + return true; } public boolean deleteSettingLocked(int type, int userId, String name, boolean forceNotify, @@ -2739,7 +2749,8 @@ public class SettingsProvider extends ContentProvider { public void resetSettingsLocked(int type, int userId, String packageName, int mode, String tag) { - resetSettingsLocked(type, userId, packageName, mode, tag, null); + resetSettingsLocked(type, userId, packageName, mode, tag, /*prefix=*/ + null); } public void resetSettingsLocked(int type, int userId, String packageName, int mode, @@ -2750,6 +2761,7 @@ public class SettingsProvider extends ContentProvider { return; } + banConfigurationIfNecessary(type, prefix, settingsState); switch (mode) { case Settings.RESET_MODE_PACKAGE_DEFAULTS: { for (String name : settingsState.getSettingNamesLocked()) { @@ -3173,6 +3185,34 @@ public class SettingsProvider extends ContentProvider { return getTypeFromKey(key) == SETTINGS_TYPE_SSAID; } + private boolean shouldBan(int type) { + if (SETTINGS_TYPE_CONFIG != type) { + return false; + } + final int callingUid = Binder.getCallingUid(); + final int appId = UserHandle.getAppId(callingUid); + + // Only non-shell resets should result in namespace banning + return appId != SHELL_UID; + } + + private void banConfigurationIfNecessary(int type, @Nullable String prefix, + SettingsState settingsState) { + // Banning should be performed only for Settings.Config and for non-shell reset calls + if (!shouldBan(type)) { + return; + } + if (prefix != null) { + settingsState.banConfigurationLocked(prefix, getAllConfigFlags(prefix)); + } else { + Set configPrefixes = settingsState.getAllConfigPrefixesLocked(); + for (String configPrefix : configPrefixes) { + settingsState.banConfigurationLocked(configPrefix, + getAllConfigFlags(configPrefix)); + } + } + } + private File getSettingsFile(int key) { if (isConfigSettingsKey(key)) { final int userId = getUserIdFromKey(key); diff --git a/packages/SettingsProvider/src/com/android/providers/settings/SettingsState.java b/packages/SettingsProvider/src/com/android/providers/settings/SettingsState.java index 086b20facb469..5b1b5305865eb 100644 --- a/packages/SettingsProvider/src/com/android/providers/settings/SettingsState.java +++ b/packages/SettingsProvider/src/com/android/providers/settings/SettingsState.java @@ -65,9 +65,12 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; +import java.util.HashSet; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; /** * This class contains the state for one type of settings. It is responsible @@ -109,6 +112,11 @@ final class SettingsState { private static final String ATTR_ID = "id"; private static final String ATTR_NAME = "name"; + private static final String TAG_NAMESPACE_HASHES = "namespaceHashes"; + private static final String TAG_NAMESPACE_HASH = "namespaceHash"; + private static final String ATTR_NAMESPACE = "namespace"; + private static final String ATTR_BANNED_HASH = "bannedHash"; + /** * Non-binary value will be written in this attributes. */ @@ -158,6 +166,9 @@ final class SettingsState { @GuardedBy("mLock") private final ArrayMap mSettings = new ArrayMap<>(); + @GuardedBy("mLock") + private final ArrayMap mNamespaceBannedHashes = new ArrayMap<>(); + @GuardedBy("mLock") private final ArrayMap mPackageToMemoryUsage; @@ -418,6 +429,41 @@ final class SettingsState { return true; } + @GuardedBy("mLock") + public boolean isNewConfigBannedLocked(String prefix, Map keyValues) { + // Replaces old style "null" String values with actual null's. This is done to simulate + // what will happen to String "null" values when they are written to Settings. This needs to + // be done here make sure that config hash computed during is banned check matches the + // one computed during banning when values are already stored. + keyValues = removeNullValueOldStyle(keyValues); + String bannedHash = mNamespaceBannedHashes.get(prefix); + if (bannedHash == null) { + return false; + } + return bannedHash.equals(hashCode(keyValues)); + } + + @GuardedBy("mLock") + public void banConfigurationLocked(String prefix, Map keyValues) { + if (prefix == null || keyValues.isEmpty()) { + return; + } + // The write is intentionally not scheduled here, banned hashes should and will be written + // when the related setting changes are written + mNamespaceBannedHashes.put(prefix, hashCode(keyValues)); + } + + @GuardedBy("mLock") + public Set getAllConfigPrefixesLocked() { + Set prefixSet = new HashSet<>(); + final int settingsCount = mSettings.size(); + for (int i = 0; i < settingsCount; i++) { + String name = mSettings.keyAt(i); + prefixSet.add(name.split("/")[0] + "/"); + } + return prefixSet; + } + // The settings provider must hold its lock when calling here. // Returns the list of keys which changed (added, updated, or deleted). @GuardedBy("mLock") @@ -710,10 +756,12 @@ final class SettingsState { boolean wroteState = false; final int version; final ArrayMap settings; + final ArrayMap namespaceBannedHashes; synchronized (mLock) { version = mVersion; settings = new ArrayMap<>(mSettings); + namespaceBannedHashes = new ArrayMap<>(mNamespaceBannedHashes); mDirty = false; mWriteScheduled = false; } @@ -756,8 +804,19 @@ final class SettingsState { + setting.getValue()); } } - serializer.endTag(null, TAG_SETTINGS); + + serializer.startTag(null, TAG_NAMESPACE_HASHES); + for (int i = 0; i < namespaceBannedHashes.size(); i++) { + String namespace = namespaceBannedHashes.keyAt(i); + String bannedHash = namespaceBannedHashes.get(namespace); + writeSingleNamespaceHash(serializer, namespace, bannedHash); + if (DEBUG_PERSISTENCE) { + Slog.i(LOG_TAG, "[PERSISTED] namespace=" + namespace + + ", bannedHash=" + bannedHash); + } + } + serializer.endTag(null, TAG_NAMESPACE_HASHES); serializer.endDocument(); destination.finishWrite(out); @@ -869,6 +928,21 @@ final class SettingsState { } } + private static void writeSingleNamespaceHash(XmlSerializer serializer, String namespace, + String bannedHashCode) throws IOException { + if (namespace == null || bannedHashCode == null) { + return; + } + serializer.startTag(null, TAG_NAMESPACE_HASH); + serializer.attribute(null, ATTR_NAMESPACE, namespace); + serializer.attribute(null, ATTR_BANNED_HASH, bannedHashCode); + serializer.endTag(null, TAG_NAMESPACE_HASH); + } + + private static String hashCode(Map keyValues) { + return Integer.toString(keyValues.hashCode()); + } + private String getValueAttribute(XmlPullParser parser, String attr, String base64Attr) { if (mVersion >= SETTINGS_VERSION_NEW_ENCODING) { final String value = parser.getAttributeValue(null, attr); @@ -939,6 +1013,8 @@ final class SettingsState { String tagName = parser.getName(); if (tagName.equals(TAG_SETTINGS)) { parseSettingsLocked(parser); + } else if (tagName.equals(TAG_NAMESPACE_HASHES)) { + parseNamespaceHash(parser); } } } @@ -982,6 +1058,37 @@ final class SettingsState { } } + @GuardedBy("mLock") + private void parseNamespaceHash(XmlPullParser parser) + throws IOException, XmlPullParserException { + + final int outerDepth = parser.getDepth(); + int type; + while ((type = parser.next()) != XmlPullParser.END_DOCUMENT + && (type != XmlPullParser.END_TAG || parser.getDepth() > outerDepth)) { + if (type == XmlPullParser.END_TAG || type == XmlPullParser.TEXT) { + continue; + } + + if (parser.getName().equals(TAG_NAMESPACE_HASH)) { + String namespace = parser.getAttributeValue(null, ATTR_NAMESPACE); + String bannedHashCode = parser.getAttributeValue(null, ATTR_BANNED_HASH); + mNamespaceBannedHashes.put(namespace, bannedHashCode); + } + } + } + + private static Map removeNullValueOldStyle(Map keyValues) { + Iterator> it = keyValues.entrySet().iterator(); + while (it.hasNext()) { + Map.Entry keyValueEntry = it.next(); + if (NULL_VALUE_OLD_STYLE.equals(keyValueEntry.getValue())) { + keyValueEntry.setValue(null); + } + } + return keyValues; + } + private final class MyHandler extends Handler { public static final int MSG_PERSIST_SETTINGS = 1;