From 9ec7f95b3c1d1db6299d3c159fad5d7a491b5e57 Mon Sep 17 00:00:00 2001 From: Hai Zhang Date: Tue, 19 Jan 2021 12:01:42 -0800 Subject: [PATCH] Store all permissions per-user. By putting install permissions into all users when reading the old USER_ALL state, and writing them back as how we have been writing per-user permissions. We are allowing roles to control signature permissions, however roles are per-user while previously only one install permission state is stored for all users, so we should make all permissions stored per-user. This change keeps reusing the old persistence mechanism, and the alternative would be creating a new persistence inside PermissionManagerService. However, since permission isn't getting modularized in S, that would mean yet another form of persistence we need to migrate from and another system API to support it, so I don't think it's a good trade-off. Meanwhile, even if we manage to create such a new persistence for per-user permission state, we are still stuck with the legacy persistence for permission definitions inside package (writeLegacyPermissionsTEMP) and thus still won't be in a totally clean state. And to migrate that as well, we need yet another storage class, system API and future migration path, without much benefits from doing it besides legacy code looking better, so I believe this change is the better approach to address our problem. Once we modularized permission, we will be able to get rid of the writeLegacyPermissionStateTEMP() methods. Since we are no longer writing install permissions with package, we no longer need to write permission state back to PackageSetting before Settings.writeLPr(). The dumpsys output is maintained to be the same as before, where the install permissions section will print the install permission state for user 0. Bug: 158736025 Test: presubmit Change-Id: I8ebfb9acd354ad906490f3bb2fb9c962bc1c16eb --- .../server/pm/PackageManagerService.java | 7 +- .../com/android/server/pm/PackageSetting.java | 2 +- .../java/com/android/server/pm/Settings.java | 108 +++++++++--------- .../pm/permission/LegacyPermissionState.java | 52 +++------ .../permission/PermissionManagerService.java | 21 +--- .../server/pm/permission/PermissionState.java | 9 -- 6 files changed, 79 insertions(+), 120 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index e7a04ef2b449d..bdff19bbd7bcd 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -21423,7 +21423,7 @@ public class PackageManagerService extends IPackageManager.Stub mPermissionManager.onPackageInstalled(pkg, PermissionManagerServiceInternal.PackageInstalledParams.DEFAULT, userId); if (applyUserRestrictions) { - mSettings.writeRuntimePermissionsForUserLPr(userId, false); + mSettings.writePermissionStateForUserLPr(userId, false); } } @@ -27429,7 +27429,7 @@ public class PackageManagerService extends IPackageManager.Stub public void writePermissionSettings(int[] userIds, boolean async) { synchronized (mLock) { for (int userId : userIds) { - mSettings.writeRuntimePermissionsForUserLPr(userId, !async); + mSettings.writePermissionStateForUserLPr(userId, !async); } } } @@ -28069,13 +28069,12 @@ public class PackageManagerService extends IPackageManager.Stub /** * Temporary method that wraps mSettings.writeLPr() and calls mPermissionManager's - * writeLegacyPermissionsTEMP() and writeLegacyPermissionStateTEMP() beforehand. + * writeLegacyPermissionsTEMP() beforehand. * * TODO(zhanghai): This should be removed once we finish migration of permission storage. */ private void writeSettingsLPrTEMP() { mPermissionManager.writeLegacyPermissionsTEMP(mSettings.mPermissions); - mPermissionManager.writeLegacyPermissionStateTEMP(); mSettings.writeLPr(); } diff --git a/services/core/java/com/android/server/pm/PackageSetting.java b/services/core/java/com/android/server/pm/PackageSetting.java index 83f6c52858740..ade087be30c98 100644 --- a/services/core/java/com/android/server/pm/PackageSetting.java +++ b/services/core/java/com/android/server/pm/PackageSetting.java @@ -356,7 +356,7 @@ public class PackageSetting extends PackageSettingBase { proto.write(PackageProto.UserPermissionsProto.ID, user.id); runtimePermissionStates = dataProvider.getLegacyPermissionState(appId) - .getRuntimePermissionStates(user.id); + .getPermissionStates(user.id); for (LegacyPermissionState.PermissionState permission : runtimePermissionStates) { if (permission.isGranted()) { proto.write(PackageProto.UserPermissionsProto.GRANTED_PERMISSIONS, diff --git a/services/core/java/com/android/server/pm/Settings.java b/services/core/java/com/android/server/pm/Settings.java index 50c1065a60002..34161473a18f4 100644 --- a/services/core/java/com/android/server/pm/Settings.java +++ b/services/core/java/com/android/server/pm/Settings.java @@ -2313,7 +2313,8 @@ public final class Settings implements Watchable, Snappable { } void readInstallPermissionsLPr(TypedXmlPullParser parser, - LegacyPermissionState permissionsState) throws IOException, XmlPullParserException { + LegacyPermissionState permissionsState, List users) + throws IOException, XmlPullParserException { int outerDepth = parser.getDepth(); int type; while ((type=parser.next()) != XmlPullParser.END_DOCUMENT @@ -2328,8 +2329,10 @@ public final class Settings implements Watchable, Snappable { String name = parser.getAttributeValue(null, ATTR_NAME); final boolean granted = parser.getAttributeBoolean(null, ATTR_GRANTED, true); final int flags = parser.getAttributeIntHex(null, ATTR_FLAGS, 0); - permissionsState.putInstallPermissionState(new PermissionState(name, granted, - flags)); + for (final UserInfo user : users) { + permissionsState.putPermissionState(new PermissionState(name, false, granted, + flags), user.id); + } } else { Slog.w(PackageManagerService.TAG, "Unknown element under : " + parser.getName()); @@ -2338,25 +2341,6 @@ public final class Settings implements Watchable, Snappable { } } - void writePermissionsLPr(TypedXmlSerializer serializer, Collection permissionStates) - throws IOException { - if (permissionStates.isEmpty()) { - return; - } - - serializer.startTag(null, TAG_PERMISSIONS); - - for (PermissionState permissionState : permissionStates) { - serializer.startTag(null, TAG_ITEM); - serializer.attributeInterned(null, ATTR_NAME, permissionState.getName()); - serializer.attributeBoolean(null, ATTR_GRANTED, permissionState.isGranted()); - serializer.attributeIntHex(null, ATTR_FLAGS, permissionState.getFlags()); - serializer.endTag(null, TAG_ITEM); - } - - serializer.endTag(null, TAG_PERMISSIONS); - } - void readUsesStaticLibLPw(TypedXmlPullParser parser, PackageSetting outPs) throws IOException, XmlPullParserException { String libName = parser.getAttributeValue(null, ATTR_NAME); @@ -2572,8 +2556,6 @@ public final class Settings implements Watchable, Snappable { serializer.attribute(null, ATTR_NAME, usr.name); serializer.attributeInt(null, "userId", usr.userId); usr.signatures.writeXml(serializer, "sigs", mPastSignatures); - writePermissionsLPr(serializer, usr.getLegacyPermissionState() - .getInstallPermissionStates()); serializer.endTag(null, "shared-user"); } @@ -2898,12 +2880,6 @@ public final class Settings implements Watchable, Snappable { writeUsesStaticLibLPw(serializer, pkg.usesStaticLibraries, pkg.usesStaticLibrariesVersions); - // If this is a shared user, the permissions will be written there. - if (pkg.sharedUser == null) { - writePermissionsLPr(serializer, pkg.getLegacyPermissionState() - .getInstallPermissionStates()); - } - serializer.endTag(null, "updated-package"); } @@ -2991,9 +2967,6 @@ public final class Settings implements Watchable, Snappable { serializer, "install-initiator-sigs", mPastSignatures); } - writePermissionsLPr(serializer, - pkg.getLegacyPermissionState().getInstallPermissionStates()); - writeSigningKeySetLPr(serializer, pkg.keySetData); writeUpgradeKeySetsLPr(serializer, pkg.keySetData); writeKeySetAliasesLPr(serializer, pkg.keySetData); @@ -3097,13 +3070,13 @@ public final class Settings implements Watchable, Snappable { String tagName = parser.getName(); if (tagName.equals("package")) { - readPackageLPw(parser); + readPackageLPw(parser, users); } else if (tagName.equals("permissions")) { mPermissions.readPermissions(parser); } else if (tagName.equals("permission-trees")) { mPermissions.readPermissionTrees(parser); } else if (tagName.equals("shared-user")) { - readSharedUserLPw(parser); + readSharedUserLPw(parser, users); } else if (tagName.equals("preferred-packages")) { // no longer used. } else if (tagName.equals("preferred-activities")) { @@ -3121,7 +3094,7 @@ public final class Settings implements Watchable, Snappable { } else if (tagName.equals(TAG_DEFAULT_BROWSER)) { readDefaultAppsLPw(parser, 0); } else if (tagName.equals("updated-package")) { - readDisabledSysPackageLPw(parser); + readDisabledSysPackageLPw(parser, users); } else if (tagName.equals("renamed-package")) { String nname = parser.getAttributeValue(null, "new"); String oname = parser.getAttributeValue(null, "old"); @@ -3627,7 +3600,7 @@ public final class Settings implements Watchable, Snappable { } } - private void readDisabledSysPackageLPw(TypedXmlPullParser parser) + private void readDisabledSysPackageLPw(TypedXmlPullParser parser, List users) throws XmlPullParserException, IOException { String name = parser.getAttributeValue(null, ATTR_NAME); String realName = parser.getAttributeValue(null, "realName"); @@ -3676,7 +3649,7 @@ public final class Settings implements Watchable, Snappable { } if (parser.getName().equals(TAG_PERMISSIONS)) { - readInstallPermissionsLPr(parser, ps.getLegacyPermissionState()); + readInstallPermissionsLPr(parser, ps.getLegacyPermissionState(), users); } else if (parser.getName().equals(TAG_USES_STATIC_LIB)) { readUsesStaticLibLPw(parser, ps); } else { @@ -3693,7 +3666,7 @@ public final class Settings implements Watchable, Snappable { private static int PRE_M_APP_INFO_FLAG_CANT_SAVE_STATE = 1<<28; private static int PRE_M_APP_INFO_FLAG_PRIVILEGED = 1<<30; - private void readPackageLPw(TypedXmlPullParser parser) + private void readPackageLPw(TypedXmlPullParser parser, List users) throws XmlPullParserException, IOException { String name = null; String realName = null; @@ -3935,7 +3908,7 @@ public final class Settings implements Watchable, Snappable { packageSetting.signatures.readXml(parser, mPastSignatures); } else if (tagName.equals(TAG_PERMISSIONS)) { readInstallPermissionsLPr(parser, - packageSetting.getLegacyPermissionState()); + packageSetting.getLegacyPermissionState(), users); packageSetting.installPermissionsFixed = true; } else if (tagName.equals("proper-signing-keyset")) { long id = parser.getAttributeLong(null, "identifier"); @@ -4112,7 +4085,7 @@ public final class Settings implements Watchable, Snappable { } } - private void readSharedUserLPw(TypedXmlPullParser parser) + private void readSharedUserLPw(TypedXmlPullParser parser, List users) throws XmlPullParserException, IOException { String name = null; int pkgFlags = 0; @@ -4156,7 +4129,7 @@ public final class Settings implements Watchable, Snappable { if (tagName.equals("sigs")) { su.signatures.readXml(parser, mPastSignatures); } else if (tagName.equals("perms")) { - readInstallPermissionsLPr(parser, su.getLegacyPermissionState()); + readInstallPermissionsLPr(parser, su.getLegacyPermissionState(), users); } else { PackageManagerService.reportSettingsProblem(Log.WARN, "Unknown element under : " + parser.getName()); @@ -4979,7 +4952,7 @@ public final class Settings implements Watchable, Snappable { dumpGidsLPr(pw, prefix + " ", mPermissionDataProvider.getGidsForUid( UserHandle.getUid(user.id, ps.appId))); dumpRuntimePermissionsLPr(pw, prefix + " ", permissionNames, permissionsState - .getRuntimePermissionStates(user.id), dumpAll); + .getPermissionStates(user.id), dumpAll); } String harmfulAppWarning = ps.getHarmfulAppWarning(user.id); @@ -5154,7 +5127,7 @@ public final class Settings implements Watchable, Snappable { final int[] gids = mPermissionDataProvider.getGidsForUid(UserHandle.getUid( userId, su.userId)); final Collection permissions = - permissionsState.getRuntimePermissionStates(userId); + permissionsState.getPermissionStates(userId); if (!ArrayUtils.isEmpty(gids) || !permissions.isEmpty()) { pw.print(prefix); pw.print("User "); pw.print(userId); pw.println(": "); dumpGidsLPr(pw, prefix + " ", gids); @@ -5215,9 +5188,19 @@ public final class Settings implements Watchable, Snappable { void dumpRuntimePermissionsLPr(PrintWriter pw, String prefix, ArraySet permissionNames, Collection permissionStates, boolean dumpAll) { - if (!permissionStates.isEmpty() || dumpAll) { + boolean hasRuntimePermissions = false; + for (PermissionState permissionState : permissionStates) { + if (permissionState.isRuntime()) { + hasRuntimePermissions = true; + break; + } + } + if (hasRuntimePermissions || dumpAll) { pw.print(prefix); pw.println("runtime permissions:"); for (PermissionState permissionState : permissionStates) { + if (!permissionState.isRuntime()) { + continue; + } if (permissionNames != null && !permissionNames.contains(permissionState.getName())) { continue; @@ -5256,11 +5239,21 @@ public final class Settings implements Watchable, Snappable { void dumpInstallPermissionsLPr(PrintWriter pw, String prefix, ArraySet permissionNames, LegacyPermissionState permissionsState) { - Collection permissionStates = - permissionsState.getInstallPermissionStates(); - if (!permissionStates.isEmpty()) { + Collection permissionStates = permissionsState.getPermissionStates( + UserHandle.USER_SYSTEM); + boolean hasInstallPermissions = false; + for (PermissionState permissionState : permissionStates) { + if (!permissionState.isRuntime()) { + hasInstallPermissions = true; + break; + } + } + if (hasInstallPermissions) { pw.print(prefix); pw.println("install permissions:"); for (PermissionState permissionState : permissionStates) { + if (permissionState.isRuntime()) { + continue; + } if (permissionNames != null && !permissionNames.contains(permissionState.getName())) { continue; @@ -5295,7 +5288,7 @@ public final class Settings implements Watchable, Snappable { } } - public void writeRuntimePermissionsForUserLPr(int userId, boolean sync) { + public void writePermissionStateForUserLPr(int userId, boolean sync) { if (sync) { mRuntimePermissionsPersistence.writeStateForUserSyncLPr(userId); } else { @@ -5530,7 +5523,7 @@ public final class Settings implements Watchable, Snappable { private List getPermissionsFromPermissionsState( @NonNull LegacyPermissionState permissionsState, @UserIdInt int userId) { Collection permissionStates = - permissionsState.getRuntimePermissionStates(userId); + permissionsState.getPermissionStates(userId); List permissions = new ArrayList<>(); for (PermissionState permissionState : permissionStates) { RuntimePermissionsState.PermissionState permission = @@ -5590,6 +5583,7 @@ public final class Settings implements Watchable, Snappable { if (permissions != null) { readPermissionsStateLpr(permissions, packageSetting.getLegacyPermissionState(), userId); + packageSetting.installPermissionsFixed = true; } else if (packageSetting.sharedUser == null && !isUpgradeToR) { Slog.w(TAG, "Missing permission state for package: " + packageName); packageSetting.getLegacyPermissionState().setMissing(true, userId); @@ -5624,7 +5618,7 @@ public final class Settings implements Watchable, Snappable { String name = permission.getName(); boolean granted = permission.isGranted(); int flags = permission.getFlags(); - permissionsState.putRuntimePermissionState(new PermissionState(name, granted, + permissionsState.putPermissionState(new PermissionState(name, true, granted, flags), userId); } } @@ -5646,7 +5640,7 @@ public final class Settings implements Watchable, Snappable { try { final TypedXmlPullParser parser = Xml.resolvePullParser(in); - parseRuntimePermissionsLPr(parser, userId); + parseLegacyRuntimePermissionsLPr(parser, userId); } catch (XmlPullParserException | IOException e) { throw new IllegalStateException("Failed parsing permissions file: " @@ -5659,7 +5653,7 @@ public final class Settings implements Watchable, Snappable { // Private internals @GuardedBy("Settings.this.mLock") - private void parseRuntimePermissionsLPr(TypedXmlPullParser parser, int userId) + private void parseLegacyRuntimePermissionsLPr(TypedXmlPullParser parser, int userId) throws IOException, XmlPullParserException { final int outerDepth = parser.getDepth(); int type; @@ -5687,7 +5681,7 @@ public final class Settings implements Watchable, Snappable { XmlUtils.skipCurrentTag(parser); continue; } - parsePermissionsLPr(parser, ps.getLegacyPermissionState(), userId); + parseLegacyPermissionsLPr(parser, ps.getLegacyPermissionState(), userId); } break; case TAG_SHARED_USER: { @@ -5698,13 +5692,13 @@ public final class Settings implements Watchable, Snappable { XmlUtils.skipCurrentTag(parser); continue; } - parsePermissionsLPr(parser, sus.getLegacyPermissionState(), userId); + parseLegacyPermissionsLPr(parser, sus.getLegacyPermissionState(), userId); } break; } } } - private void parsePermissionsLPr(TypedXmlPullParser parser, + private void parseLegacyPermissionsLPr(TypedXmlPullParser parser, LegacyPermissionState permissionsState, int userId) throws IOException, XmlPullParserException { final int outerDepth = parser.getDepth(); @@ -5722,7 +5716,7 @@ public final class Settings implements Watchable, Snappable { parser.getAttributeBoolean(null, ATTR_GRANTED, true); final int flags = parser.getAttributeIntHex(null, ATTR_FLAGS, 0); - permissionsState.putRuntimePermissionState(new PermissionState(name, + permissionsState.putPermissionState(new PermissionState(name, true, granted, flags), userId); } break; diff --git a/services/core/java/com/android/server/pm/permission/LegacyPermissionState.java b/services/core/java/com/android/server/pm/permission/LegacyPermissionState.java index 72d76285fe405..92f22a4a7b8e0 100644 --- a/services/core/java/com/android/server/pm/permission/LegacyPermissionState.java +++ b/services/core/java/com/android/server/pm/permission/LegacyPermissionState.java @@ -19,7 +19,6 @@ package com.android.server.pm.permission; import android.annotation.NonNull; import android.annotation.Nullable; import android.annotation.UserIdInt; -import android.os.UserHandle; import android.util.ArrayMap; import android.util.SparseArray; import android.util.SparseBooleanArray; @@ -105,28 +104,14 @@ public final class LegacyPermissionState { } /** - * Put a install permission state. - * - * @param permissionState the permission state - */ - public void putInstallPermissionState(@NonNull PermissionState permissionState) { - putPermissionState(permissionState, UserHandle.USER_ALL); - } - - /** - * Put a runtime permission state for a user. + * Put a permission state for a user. * * @param permissionState the permission state * @param userId the user ID */ - public void putRuntimePermissionState(@NonNull PermissionState permissionState, + public void putPermissionState(@NonNull PermissionState permissionState, @UserIdInt int userId) { checkUserId(userId); - putPermissionState(permissionState, userId); - } - - private void putPermissionState(@NonNull PermissionState permissionState, - @UserIdInt int userId) { UserState userState = mUserStates.get(userId); if (userState == null) { userState = new UserState(); @@ -156,16 +141,6 @@ public final class LegacyPermissionState { return false; } - /** - * Get all the install permission states. - * - * @return the install permission states - */ - @NonNull - public Collection getInstallPermissionStates() { - return getPermissionStates(UserHandle.USER_ALL); - } - /** * Get all the runtime permission states for a user. * @@ -173,13 +148,8 @@ public final class LegacyPermissionState { * @return the runtime permission states */ @NonNull - public Collection getRuntimePermissionStates(@UserIdInt int userId) { + public Collection getPermissionStates(@UserIdInt int userId) { checkUserId(userId); - return getPermissionStates(userId); - } - - @NonNull - private Collection getPermissionStates(@UserIdInt int userId) { final UserState userState = mUserStates.get(userId); if (userState == null) { return Collections.emptyList(); @@ -265,6 +235,8 @@ public final class LegacyPermissionState { @NonNull private final String mName; + private final boolean mRuntime; + private final boolean mGranted; private final int mFlags; @@ -273,17 +245,20 @@ public final class LegacyPermissionState { * Create a new instance of this class. * * @param name the name of the permission + * @param runtime whether the permission is runtime * @param granted whether the permission is granted * @param flags the permission flags */ - public PermissionState(@NonNull String name, boolean granted, int flags) { + public PermissionState(@NonNull String name, boolean runtime, boolean granted, int flags) { mName = name; + mRuntime = runtime; mGranted = granted; mFlags = flags; } private PermissionState(@NonNull PermissionState other) { mName = other.mName; + mRuntime = other.mRuntime; mGranted = other.mGranted; mFlags = other.mFlags; } @@ -298,6 +273,15 @@ public final class LegacyPermissionState { return mName; } + /** + * Get whether the permission is a runtime permission. + * + * @return whether the permission is a runtime permission. + */ + public boolean isRuntime() { + return mRuntime; + } + /** * Get whether the permission is granted. * diff --git a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java index 8f422890c9734..2a646b5575471 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java +++ b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java @@ -3965,8 +3965,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { if (!usedPermissions.contains(permissionState.getName())) { Permission bp = mRegistry.getPermission(permissionState.getName()); if (bp != null) { - if (uidState.removePermissionState(bp.getName()) - && permissionState.isRuntime()) { + if (uidState.removePermissionState(bp.getName()) && bp.isRuntime()) { runtimePermissionChanged = true; } } @@ -4573,9 +4572,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { uidState.reset(); uidState.setMissing(legacyState.isMissing(userId)); readLegacyPermissionStatesLocked(uidState, - legacyState.getInstallPermissionStates()); - readLegacyPermissionStatesLocked(uidState, - legacyState.getRuntimePermissionStates(userId)); + legacyState.getPermissionStates(userId)); } } }); @@ -4634,12 +4631,9 @@ public class PermissionManagerService extends IPermissionManager.Stub { final LegacyPermissionState.PermissionState legacyPermissionState = new LegacyPermissionState.PermissionState(permissionState.getName(), + permissionState.getPermission().isRuntime(), permissionState.isGranted(), permissionState.getFlags()); - if (permissionState.isRuntime()) { - legacyState.putRuntimePermissionState(legacyPermissionState, userId); - } else { - legacyState.putInstallPermissionState(legacyPermissionState); - } + legacyState.putPermissionState(legacyPermissionState, userId); } } } @@ -4904,12 +4898,9 @@ public class PermissionManagerService extends IPermissionManager.Stub { final LegacyPermissionState.PermissionState legacyPermissionState = new LegacyPermissionState.PermissionState(permissionState.getName(), + permissionState.getPermission().isRuntime(), permissionState.isGranted(), permissionState.getFlags()); - if (permissionState.isRuntime()) { - legacyState.putRuntimePermissionState(legacyPermissionState, userId); - } else if (userId == UserHandle.USER_SYSTEM) { - legacyState.putInstallPermissionState(legacyPermissionState); - } + legacyState.putPermissionState(legacyPermissionState, userId); } } } diff --git a/services/core/java/com/android/server/pm/permission/PermissionState.java b/services/core/java/com/android/server/pm/permission/PermissionState.java index 12f29d0911347..9ad8a05c945e6 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionState.java +++ b/services/core/java/com/android/server/pm/permission/PermissionState.java @@ -31,9 +31,6 @@ public final class PermissionState { private final Object mLock = new Object(); - @GuardedBy("mLock") - private boolean mRuntime; - @GuardedBy("mLock") private boolean mGranted; @@ -66,12 +63,6 @@ public final class PermissionState { return mPermission.computeGids(userId); } - public boolean isRuntime() { - synchronized (mLock) { - return mPermission.isRuntime(); - } - } - public boolean isGranted() { synchronized (mLock) { return mGranted;