From 65c2dc24528d42611ffac65ab5d5421b427ca1b4 Mon Sep 17 00:00:00 2001 From: Hassan Ali Date: Mon, 21 Nov 2022 14:51:45 +0000 Subject: [PATCH 1/2] Move enforceReadPermission to Setting.config As part of moving DeviceConfig.java to packages/modules/ConfigInfrastructure. Need to move activity thread dependency to setting.config as the new module will not have access to hidden apis (ActivityThread). For security purpose we cannot expose enforceReadPermission so we need to move it to Settings.Config Bug: 258220607 Test: m Change-Id: Ibdd7c0489eb4f5238272705fd45e15b2b146a656 --- core/java/android/provider/DeviceConfig.java | 17 +---------------- core/java/android/provider/Settings.java | 17 ++++++++++++++++- .../internal/jank/InteractionJankMonitor.java | 3 ++- .../providers/settings/SettingsProvider.java | 4 ++-- .../systemui/util/DeviceConfigProxy.java | 7 ------- .../systemui/util/DeviceConfigProxyFake.java | 5 ----- 6 files changed, 21 insertions(+), 32 deletions(-) diff --git a/core/java/android/provider/DeviceConfig.java b/core/java/android/provider/DeviceConfig.java index ab878202cfdaa..be110ebb92ea2 100644 --- a/core/java/android/provider/DeviceConfig.java +++ b/core/java/android/provider/DeviceConfig.java @@ -25,7 +25,6 @@ import android.annotation.Nullable; import android.annotation.RequiresPermission; import android.annotation.SuppressLint; import android.annotation.SystemApi; -import android.content.pm.PackageManager; import android.database.ContentObserver; import android.net.Uri; import android.provider.Settings.Config.SyncDisabledMode; @@ -1173,7 +1172,7 @@ public final class DeviceConfig { @NonNull String namespace, @NonNull @CallbackExecutor Executor executor, @NonNull OnPropertiesChangedListener onPropertiesChangedListener) { - enforceReadPermission(namespace); + Settings.Config.enforceReadPermission(namespace); synchronized (sLock) { Pair oldNamespace = sListeners.get(onPropertiesChangedListener); if (oldNamespace == null) { @@ -1295,20 +1294,6 @@ public final class DeviceConfig { } } - /** - * Enforces READ_DEVICE_CONFIG permission if namespace is not one of public namespaces. - * @hide - */ - public static void enforceReadPermission(@NonNull String namespace) { - if (Settings.Config.checkCallingOrSelfPermission(READ_DEVICE_CONFIG) - != PackageManager.PERMISSION_GRANTED) { - if (!PUBLIC_NAMESPACES.contains(namespace)) { - throw new SecurityException("Permission denial: reading from settings requires:" - + READ_DEVICE_CONFIG); - } - } - } - /** * Returns list of namespaces that can be read without READ_DEVICE_CONFIG_PERMISSION; * @hide diff --git a/core/java/android/provider/Settings.java b/core/java/android/provider/Settings.java index 4ca5a93a28022..0b933fc64ec72 100644 --- a/core/java/android/provider/Settings.java +++ b/core/java/android/provider/Settings.java @@ -3366,7 +3366,7 @@ public final class Settings { public ArrayMap getStringsForPrefix(ContentResolver cr, String prefix, List names) { String namespace = prefix.substring(0, prefix.length() - 1); - DeviceConfig.enforceReadPermission(namespace); + Config.enforceReadPermission(namespace); ArrayMap keyValues = new ArrayMap<>(); int currentGeneration = -1; @@ -18355,6 +18355,21 @@ public final class Settings { .getApplicationContext().checkCallingOrSelfPermission(permission); } + /** + * Enforces READ_DEVICE_CONFIG permission if namespace is not one of public namespaces. + * @hide + */ + public static void enforceReadPermission(String namespace) { + if (ActivityThread.currentApplication().getApplicationContext() + .checkCallingOrSelfPermission(Manifest.permission.READ_DEVICE_CONFIG) + != PackageManager.PERMISSION_GRANTED) { + if (!DeviceConfig.getPublicNamespaces().contains(namespace)) { + throw new SecurityException("Permission denial: reading from settings requires:" + + Manifest.permission.READ_DEVICE_CONFIG); + } + } + } + private static void registerMonitorCallbackAsUser( @NonNull ContentResolver resolver, @UserIdInt int userHandle, @NonNull RemoteCallback callback) { diff --git a/core/java/com/android/internal/jank/InteractionJankMonitor.java b/core/java/com/android/internal/jank/InteractionJankMonitor.java index 614f96255acb0..75f0bf5749476 100644 --- a/core/java/com/android/internal/jank/InteractionJankMonitor.java +++ b/core/java/com/android/internal/jank/InteractionJankMonitor.java @@ -97,6 +97,7 @@ import android.os.Handler; import android.os.HandlerExecutor; import android.os.HandlerThread; import android.provider.DeviceConfig; +import android.provider.Settings; import android.text.TextUtils; import android.util.Log; import android.util.SparseArray; @@ -415,7 +416,7 @@ public class InteractionJankMonitor { @VisibleForTesting public InteractionJankMonitor(@NonNull HandlerThread worker) { // Check permission early. - DeviceConfig.enforceReadPermission( + Settings.Config.enforceReadPermission( DeviceConfig.NAMESPACE_INTERACTION_JANK_MONITOR); mRunningTrackers = new SparseArray<>(); diff --git a/packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java b/packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java index 503859b8dc38b..9192086de98a3 100644 --- a/packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java +++ b/packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java @@ -1144,7 +1144,7 @@ public class SettingsProvider extends ContentProvider { Slog.v(LOG_TAG, "getConfigSetting(" + name + ")"); } - DeviceConfig.enforceReadPermission(/*namespace=*/name.split("/")[0]); + Settings.Config.enforceReadPermission(/*namespace=*/name.split("/")[0]); // Get the value. synchronized (mLock) { @@ -1317,7 +1317,7 @@ public class SettingsProvider extends ContentProvider { Slog.v(LOG_TAG, "getAllConfigFlags() for " + prefix); } - DeviceConfig.enforceReadPermission( + Settings.Config.enforceReadPermission( prefix != null ? prefix.split("/")[0] : null); synchronized (mLock) { diff --git a/packages/SystemUI/src/com/android/systemui/util/DeviceConfigProxy.java b/packages/SystemUI/src/com/android/systemui/util/DeviceConfigProxy.java index 0f3eddf2eb7c1..bff6132d5e23a 100644 --- a/packages/SystemUI/src/com/android/systemui/util/DeviceConfigProxy.java +++ b/packages/SystemUI/src/com/android/systemui/util/DeviceConfigProxy.java @@ -49,13 +49,6 @@ public class DeviceConfigProxy { namespace, executor, onPropertiesChangedListener); } - /** - * Wrapped version of {@link DeviceConfig#enforceReadPermission}. - */ - public void enforceReadPermission(String namespace) { - DeviceConfig.enforceReadPermission(namespace); - } - /** * Wrapped version of {@link DeviceConfig#getBoolean}. */ diff --git a/packages/SystemUI/tests/utils/src/com/android/systemui/util/DeviceConfigProxyFake.java b/packages/SystemUI/tests/utils/src/com/android/systemui/util/DeviceConfigProxyFake.java index 21e16a1e7be48..8a10bf064910e 100644 --- a/packages/SystemUI/tests/utils/src/com/android/systemui/util/DeviceConfigProxyFake.java +++ b/packages/SystemUI/tests/utils/src/com/android/systemui/util/DeviceConfigProxyFake.java @@ -81,11 +81,6 @@ public class DeviceConfigProxyFake extends DeviceConfigProxy { properties.get(namespace).put(name, value); } - @Override - public void enforceReadPermission(String namespace) { - // no-op - } - private Properties propsForNamespaceAndName(String namespace, String name) { if (mProperties.containsKey(namespace) && mProperties.get(namespace).containsKey(name)) { return new Properties.Builder(namespace) From ab384c5b7210d35ebf88105859d353c7dad56d99 Mon Sep 17 00:00:00 2001 From: Hassan Ali Date: Mon, 28 Nov 2022 12:44:52 +0000 Subject: [PATCH 2/2] Stop calling Enforcereadpermission from DeviceConf Stop calling enforcereadpermission from DeviceConfig.java because It's not safe to call Enforcereadpermission from the client peocess side. Test: m Bug: 258220607 Change-Id: I9e83866f41ebe174888722336c45cf0986359e91 --- core/api/system-current.txt | 2 +- core/java/android/provider/DeviceConfig.java | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/core/api/system-current.txt b/core/api/system-current.txt index 5e86546b56db6..01ecc51c8e2e9 100644 --- a/core/api/system-current.txt +++ b/core/api/system-current.txt @@ -10529,7 +10529,7 @@ package android.provider { } public final class DeviceConfig { - method @RequiresPermission(android.Manifest.permission.READ_DEVICE_CONFIG) public static void addOnPropertiesChangedListener(@NonNull String, @NonNull java.util.concurrent.Executor, @NonNull android.provider.DeviceConfig.OnPropertiesChangedListener); + method public static void addOnPropertiesChangedListener(@NonNull String, @NonNull java.util.concurrent.Executor, @NonNull android.provider.DeviceConfig.OnPropertiesChangedListener); method @RequiresPermission(android.Manifest.permission.WRITE_DEVICE_CONFIG) public static boolean deleteProperty(@NonNull String, @NonNull String); method @RequiresPermission(android.Manifest.permission.READ_DEVICE_CONFIG) public static boolean getBoolean(@NonNull String, @NonNull String, boolean); method @RequiresPermission(android.Manifest.permission.READ_DEVICE_CONFIG) public static float getFloat(@NonNull String, @NonNull String, float); diff --git a/core/java/android/provider/DeviceConfig.java b/core/java/android/provider/DeviceConfig.java index be110ebb92ea2..7df9290274ce9 100644 --- a/core/java/android/provider/DeviceConfig.java +++ b/core/java/android/provider/DeviceConfig.java @@ -1167,12 +1167,10 @@ public final class DeviceConfig { * @see #removeOnPropertiesChangedListener(OnPropertiesChangedListener) */ @SystemApi - @RequiresPermission(READ_DEVICE_CONFIG) public static void addOnPropertiesChangedListener( @NonNull String namespace, @NonNull @CallbackExecutor Executor executor, @NonNull OnPropertiesChangedListener onPropertiesChangedListener) { - Settings.Config.enforceReadPermission(namespace); synchronized (sLock) { Pair oldNamespace = sListeners.get(onPropertiesChangedListener); if (oldNamespace == null) {