From b0bcbce79c5db1571789fac107dfa356e0375c0f Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Tue, 31 Jan 2023 05:47:42 +0000 Subject: [PATCH] Lock down the ability to read from the locksettings database Currently the getString(), getLong(), and getBoolean() methods of ILockSettings don't require any permission by default. There are some specific database keys that they enforce ACCESS_KEYGUARD_SECURE_STORAGE for, and some other keys that they enforce READ_CONTACTS for. This is much too lenient, since it means that anything new that gets added to the database is automatically readable without any permission. I've searched through the users of these methods as much as I can. Virtually all their callers are from SystemUI / Keyguard or Settings, or from system_server itself with a cleared calling identity, and therefore would be fine with ACCESS_KEYGUARD_SECURE_STORAGE. The only potential exception I found is the public API android.provider.Settings, which intentionally allows apps targeted to an old API level to read three LOCK_PATTERN_* settings. I've kept these three settings unprotected. Regarding potential unsupported app usage, the non-SDK dashboards show no hits for any of the following, which should cover all relevant @UnsupportedAppUsage entry points: * ILockSettings.getBoolean() * ILockSettings.getLong() * ILockSettings.getString() * LockPatternUtils.getOwnerInfo() * LockPatternUtils.getString() * LockPatternUtils.isLockScreenDisabled() * LockPatternUtils.isVisiblePatternEnabled() LOCK_SCREEN_OWNER_INFO and LOCK_SCREEN_OWNER_INFO_ENABLED are an interesting case in that they were protected by READ_CONTACTS. However, looking at the change that added that code (commit 158fe19ff88e, http://ag/298629) and its associated bug, it seems that READ_CONTACTS was just used because of the nature of the bug report: it was a security vulnerability report that was presented as apps being able to get personal user information without the READ_CONTACTS permission. That doesn't mean that READ_CONTACTS is the right permission, however. The "owner info" is somewhat misnamed in that it isn't really owner info, but rather just the text configured in `Settings -> Display -> Lock screen -> Add text on lock screen`. So it can be any sort of free-form text the user puts there. It's only Settings and System UI / Keyguard that access this text. It's also totally separate from the emergency information that can be configured via `Settings -> Safety & emergency`. Thus, switching to ACCESS_KEYGUARD_SECURE_STORAGE (a "system-only" permission, instead of a permission that apps can get) for the owner info seems like the right choice. All in all, it's likely that this change is safe to make. If an issue arises, we can always relax the permission check for specific keys. Bug: 156606120 Bug: 256170784 Test: Tested setting PIN, rebooting, unlocking, changing PIN Test: Tested that "Add text on lock screen" still works Test: atest com.android.server.locksettings Change-Id: I574d6a3f29b6fc76d964ddd5e9e0f8e4c680084f --- .../locksettings/LockSettingsService.java | 56 ++++++------------- 1 file changed, 17 insertions(+), 39 deletions(-) diff --git a/services/core/java/com/android/server/locksettings/LockSettingsService.java b/services/core/java/com/android/server/locksettings/LockSettingsService.java index ebc18bcd2369a..fe6d5c8688faf 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsService.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsService.java @@ -18,7 +18,6 @@ package com.android.server.locksettings; import static android.Manifest.permission.ACCESS_KEYGUARD_SECURE_STORAGE; import static android.Manifest.permission.MANAGE_BIOMETRIC; -import static android.Manifest.permission.READ_CONTACTS; import static android.Manifest.permission.SET_AND_VERIFY_LOCKSCREEN_CREDENTIALS; import static android.Manifest.permission.SET_INITIAL_LOCK; import static android.app.admin.DevicePolicyManager.DEPRECATE_USERMANAGERINTERNAL_DEVICEPOLICY_DEFAULT; @@ -96,7 +95,6 @@ import android.os.storage.IStorageManager; import android.os.storage.StorageManager; import android.provider.DeviceConfig; import android.provider.Settings; -import android.provider.Settings.Secure; import android.security.AndroidKeyStoreMaintenance; import android.security.Authorization; import android.security.KeyStore; @@ -1048,27 +1046,21 @@ public class LockSettingsService extends ILockSettings.Stub { mContext.enforceCallingOrSelfPermission(PERMISSION, "LockSettingsHave"); } - private final void checkReadPermission(String requestedKey, int userId) { - final int callingUid = Binder.getCallingUid(); + private static final String[] UNPROTECTED_SETTINGS = { + // These three LOCK_PATTERN_* settings have traditionally been readable via the public API + // android.provider.Settings.{System,Secure}.getString() without any permission. + Settings.Secure.LOCK_PATTERN_ENABLED, + Settings.Secure.LOCK_PATTERN_VISIBLE, + Settings.Secure.LOCK_PATTERN_TACTILE_FEEDBACK_ENABLED, + }; - for (int i = 0; i < READ_CONTACTS_PROTECTED_SETTINGS.length; i++) { - String key = READ_CONTACTS_PROTECTED_SETTINGS[i]; - if (key.equals(requestedKey) && mContext.checkCallingOrSelfPermission(READ_CONTACTS) - != PackageManager.PERMISSION_GRANTED) { - throw new SecurityException("uid=" + callingUid - + " needs permission " + READ_CONTACTS + " to read " - + requestedKey + " for user " + userId); - } + private final void checkDatabaseReadPermission(String requestedKey, int userId) { + if (ArrayUtils.contains(UNPROTECTED_SETTINGS, requestedKey)) { + return; } - - for (int i = 0; i < READ_PASSWORD_PROTECTED_SETTINGS.length; i++) { - String key = READ_PASSWORD_PROTECTED_SETTINGS[i]; - if (key.equals(requestedKey) && mContext.checkCallingOrSelfPermission(PERMISSION) - != PackageManager.PERMISSION_GRANTED) { - throw new SecurityException("uid=" + callingUid - + " needs permission " + PERMISSION + " to read " - + requestedKey + " for user " + userId); - } + if (!hasPermission(PERMISSION)) { + throw new SecurityException("uid=" + getCallingUid() + " needs permission " + + PERMISSION + " to read " + requestedKey + " for user " + userId); } } @@ -1097,7 +1089,7 @@ public class LockSettingsService extends ILockSettings.Stub { @Override public boolean getSeparateProfileChallengeEnabled(int userId) { - checkReadPermission(SEPARATE_PROFILE_CHALLENGE_KEY, userId); + checkDatabaseReadPermission(SEPARATE_PROFILE_CHALLENGE_KEY, userId); return getSeparateProfileChallengeEnabledInternal(userId); } @@ -1178,7 +1170,7 @@ public class LockSettingsService extends ILockSettings.Stub { @Override public boolean getBoolean(String key, boolean defaultValue, int userId) { - checkReadPermission(key, userId); + checkDatabaseReadPermission(key, userId); if (Settings.Secure.LOCK_PATTERN_ENABLED.equals(key)) { return getCredentialTypeInternal(userId) == CREDENTIAL_TYPE_PATTERN; } @@ -1187,13 +1179,13 @@ public class LockSettingsService extends ILockSettings.Stub { @Override public long getLong(String key, long defaultValue, int userId) { - checkReadPermission(key, userId); + checkDatabaseReadPermission(key, userId); return mStorage.getLong(key, defaultValue, userId); } @Override public String getString(String key, String defaultValue, int userId) { - checkReadPermission(key, userId); + checkDatabaseReadPermission(key, userId); return mStorage.getString(key, defaultValue, userId); } @@ -2534,20 +2526,6 @@ public class LockSettingsService extends ILockSettings.Stub { mRecoverableKeyStoreManager.validateRemoteLockscreen(encryptedCredential); } - // Reading these settings needs the contacts permission - private static final String[] READ_CONTACTS_PROTECTED_SETTINGS = new String[] { - Secure.LOCK_SCREEN_OWNER_INFO_ENABLED, - Secure.LOCK_SCREEN_OWNER_INFO - }; - - // Reading these settings needs the same permission as checking the password - private static final String[] READ_PASSWORD_PROTECTED_SETTINGS = new String[] { - LockPatternUtils.LOCK_PASSWORD_SALT_KEY, - LockPatternUtils.PASSWORD_HISTORY_KEY, - LockPatternUtils.PASSWORD_TYPE_KEY, - SEPARATE_PROFILE_CHALLENGE_KEY - }; - private class GateKeeperDiedRecipient implements IBinder.DeathRecipient { @Override public void binderDied() {