From ea789fd6b1dd8f36059fcb930d10b861dcd19131 Mon Sep 17 00:00:00 2001 From: Hai Zhang Date: Mon, 30 Nov 2020 04:29:59 -0800 Subject: [PATCH] Refactor and rename onPackageStateRemoved() to onPackageUninstalled(). Under what conditions to notify the permission subsystem that a package is considered uninstalled can be tricky/confusing, because there are so many different code paths and flavors for deleting a package, deleting a package can mean different things, and it's critical to get it right for permissions due to security concerns. If a package is being deleted as a upgrade step, we should not consider it an uninstallation (but a later package installation with replaced == true), and handle the change later in that callback. This is also true for uninstalling an updated system package and reinstalling the factory version later, but that's a different code path. For system packages, we normally can't just completely remove their permission state upon uninstallation, because they can be marked as installed later, and permission pregrants needs to stick for them. So in this case we just clear the user modifiable permission state instead of dropping the whole state. However, we cannot assume that we should never drop the permission state for a system package, because there are package uninstallations during the package reconciliation phase of system boot that we do need to remove the permission state for invalid (e.g. missing) system packages. DELETE_KEEP_DATA as a flag for deleting a package actually means two different things when marking package as uninstalled v.s. removing package for all users. When marking a package as uninstalled, this flag only affects whether the data directory is deleted (as what is described in its javadoc), and all other states (including permission state) is always deleted because it's an uninstallation for that user. However when removing a package for all users, specifying this flag actually means keeping all package states and only remove the APK, which is also used when an external SD card is ejected (or a package is upgraded, or uninstalling an updated system package which has a lower version than its factory version due to OTA). In other words, uninstalling a package for all users with DELETE_KEEP_DATA actually keeps the package installed and all its states (including PackageSetting) intact, so it shouldn't be considred uninstalled for permission; whereas uninstalling for a subset of users with DELETE_KEEP_DATA marks the package uninstalled and removes most package state for those users except for the data directory, so it should be considered uninstalled for permission. Another subtle aspect of DELETE_KEEP_DATA is that it is always set when deleting an updated system app whose version is lower than its factory version due to OTA, but never set when the factory version is older, i.e. a downgrade, which is more often the case. In this case we are deleting most of the package states, however we do need to keep the permission state mostly intact, as to user it's only a replacement so user preference should be carried over, and the downgraded app cannot be missing pregranted permissions anyway. So the current implementation is that we need to clear the no longer requested permission states if the app has a shared user ID, or leave it as-is if it doesn't as permissions not requested won't be reported as granted. According to the discussion above, we are no longer calling the onPackageUninstalled() callback when a updated system package is being uninstalled for later installation of its factory version, because it should be considered as a package replacement instead of an uninstallation that should remove/reset its permission state. This is covered by the call to updatePermissions() inside installPackageFromSystemLIF() called by deleteSystemPackageLIF(), which is also moved a bit later to allow correct installed state to be set first. The method updatePermissions() should be later refactored into onPackageInstalled(). The call to updatePermissions() in the old onPackageStateRemoved() is actually necessary because it removes permission definitions according. On the contrary, removeAllPermissions() called in onPackageRemoved() only removes the PermissionInfo object in Permission, instead of the Permission itself, because it may be called upon SD card ejection. Bug: 158736025 Test: presubmit Change-Id: I06321ba21ee9e13a9bfbb1c990bbd50a68503b88 --- .../server/pm/PackageManagerService.java | 43 ++++++++++----- .../permission/PermissionManagerService.java | 55 +++++++++++-------- .../PermissionManagerServiceInternal.java | 19 +++++-- 3 files changed, 76 insertions(+), 41 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index 479a26b0f10d7..2b0f4abe1c113 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -19497,13 +19497,20 @@ public class PackageManagerService extends IPackageManager.Stub if (outInfo != null) { outInfo.removedAppId = removedAppId; } - final SharedUserSetting sus = deletedPs.getSharedUser(); - List sharedUserPkgs = sus != null ? sus.getPackages() : null; - if (sharedUserPkgs == null) { - sharedUserPkgs = Collections.emptyList(); + if (!mSettings.isDisabledSystemPackageLPr(packageName)) { + // If we don't have a disabled system package to reinstall, the package is + // really gone and its permission state should be removed. + final SharedUserSetting sus = deletedPs.getSharedUser(); + List sharedUserPkgs = sus != null ? sus.getPackages() + : null; + if (sharedUserPkgs == null) { + sharedUserPkgs = Collections.emptyList(); + } + for (final int userId : allUserHandles) { + mPermissionManager.onPackageUninstalled(packageName, deletedPs.appId, + deletedPs.pkg, sharedUserPkgs, userId); + } } - mPermissionManager.onPackageStateRemoved(packageName, deletedPs.appId, - deletedPs.pkg, sharedUserPkgs); clearPackagePreferredActivitiesLPw( deletedPs.name, changedUsers, UserHandle.USER_ALL); } @@ -19692,10 +19699,6 @@ public class PackageManagerService extends IPackageManager.Stub synchronized (mLock) { PackageSetting ps = mSettings.mPackages.get(pkg.getPackageName()); - // The update permissions method below will take care of removing obsolete permissions - // and granting install permissions. - mPermissionManager.updatePermissions(pkg.getPackageName(), pkg); - final boolean applyUserRestrictions = origUserHandles != null; if (applyUserRestrictions) { boolean installedStateChanged = false; @@ -19714,8 +19717,6 @@ public class PackageManagerService extends IPackageManager.Stub if (installed) { ps.setUninstallReason(UNINSTALL_REASON_UNKNOWN, userId); } - - mSettings.writeRuntimePermissionsForUserLPr(userId, false); } // Regardless of writeSettings we need to ensure that this restriction // state propagation is persisted @@ -19724,6 +19725,16 @@ public class PackageManagerService extends IPackageManager.Stub mSettings.writeKernelMappingLPr(ps); } } + + // The update permissions method below will take care of removing obsolete permissions + // and granting install permissions. + mPermissionManager.updatePermissions(pkg.getPackageName(), pkg); + if (applyUserRestrictions) { + for (int userId : allUserHandles) { + mSettings.writeRuntimePermissionsForUserLPr(userId, false); + } + } + // can downgrade to reader here if (writeSettings) { writeSettingsLPrTEMP(); @@ -20022,6 +20033,11 @@ public class PackageManagerService extends IPackageManager.Stub destroyAppProfilesLIF(pkg); + final SharedUserSetting sus = ps.getSharedUser(); + List sharedUserPkgs = sus != null ? sus.getPackages() : null; + if (sharedUserPkgs == null) { + sharedUserPkgs = Collections.emptyList(); + } final int[] userIds = (userId == UserHandle.USER_ALL) ? mUserManager.getUserIds() : new int[] {userId}; for (int nextUserId : userIds) { @@ -20036,7 +20052,8 @@ public class PackageManagerService extends IPackageManager.Stub clearDefaultBrowserIfNeededForUser(ps.name, nextUserId); removeKeystoreDataIfNeeded(mInjector.getUserManagerInternal(), nextUserId, ps.appId); clearPackagePreferredActivities(ps.name, nextUserId); - mPermissionManager.resetRuntimePermissions(pkg, nextUserId); + mPermissionManager.onPackageUninstalled(ps.name, ps.appId, pkg, sharedUserPkgs, + nextUserId); } if (outInfo != null) { 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 d2c9bf0733d3d..c42ee49e58f6f 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java +++ b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java @@ -4664,13 +4664,13 @@ public class PermissionManagerService extends IPermissionManager.Stub { return userState.getUidState(appId); } - private void removeAppIdState(@AppIdInt int appId) { + private void removeUidState(@AppIdInt int appId, @UserIdInt int userId) { synchronized (mLock) { - final int[] userIds = mState.getUserIds(); - for (final int userId : userIds) { - final UserPermissionState userState = mState.getUserState(userId); - userState.removeUidState(appId); + final UserPermissionState userState = mState.getUserState(userId); + if (userState == null) { + return; } + userState.removeUidState(appId); } } @@ -4929,25 +4929,34 @@ public class PermissionManagerService extends IPermissionManager.Stub { removeAllPermissionsInternal(pkg); } - private void onPackageStateRemovedInternal(@NonNull String packageName, int appId, - @Nullable AndroidPackage pkg, @NonNull List sharedUserPkgs) { - if (sharedUserPkgs.isEmpty() - && mPackageManagerInt.getDisabledSystemPackage(packageName) == null) { - removeAppIdState(appId); + private void onPackageUninstalledInternal(@NonNull String packageName, int appId, + @Nullable AndroidPackage pkg, @NonNull List sharedUserPkgs, + @UserIdInt int userId) { + // TODO: Move these checks to check PackageState to be more reliable. + // System packages should always have an available APK. + if (pkg != null && pkg.isSystem() + // We may be fully removing invalid system packages during boot, and in that case we + // do want to remove their permission state. So make sure that the package is only + // being marked as uninstalled instead of fully removed. + && mPackageManagerInt.getPackage(packageName) != null) { + // If we are only marking a system package as uninstalled, we need to keep its + // pregranted permission state so that it still works once it gets reinstalled, thus + // only reset the user modifications to its permission state. + resetRuntimePermissionsInternal(pkg, userId); + return; } updatePermissions(packageName, null, mDefaultPermissionCallback); - if (!sharedUserPkgs.isEmpty()) { + if (sharedUserPkgs.isEmpty()) { + removeUidState(appId, userId); + } else { // Remove permissions associated with package. Since runtime // permissions are per user we have to kill the removed package // or packages running under the shared user of the removed // package if revoking the permissions requested only by the removed // package is successful and this causes a change in gids. - boolean shouldKill = false; - for (int userId : UserManagerService.getInstance().getUserIds()) { - final int userIdToKill = revokeSharedUserPermissionsForDeletedPackageInternal(pkg, - sharedUserPkgs, userId); - shouldKill |= userIdToKill != UserHandle.USER_NULL; - } + final int userIdToKill = revokeSharedUserPermissionsForDeletedPackageInternal(pkg, + sharedUserPkgs, userId); + final boolean shouldKill = userIdToKill != UserHandle.USER_NULL; // If gids changed, kill all affected packages. if (shouldKill) { mHandler.post(() -> { @@ -5381,11 +5390,13 @@ public class PermissionManagerService extends IPermissionManager.Stub { } @Override - public void onPackageStateRemoved(@NonNull String packageName, int appId, - @Nullable AndroidPackage pkg, @NonNull List sharedUserPkgs) { - Objects.requireNonNull(packageName); - Objects.requireNonNull(sharedUserPkgs); - onPackageStateRemovedInternal(packageName, appId, pkg, sharedUserPkgs); + public void onPackageUninstalled(@NonNull String packageName, int appId, + @Nullable AndroidPackage pkg, @NonNull List sharedUserPkgs, + @UserIdInt int userId) { + Objects.requireNonNull(packageName, "packageName"); + Objects.requireNonNull(sharedUserPkgs, "sharedUserPkgs"); + Preconditions.checkArgumentNonNegative(userId, "userId"); + onPackageUninstalledInternal(packageName, appId, pkg, sharedUserPkgs, userId); } @Override diff --git a/services/core/java/com/android/server/pm/permission/PermissionManagerServiceInternal.java b/services/core/java/com/android/server/pm/permission/PermissionManagerServiceInternal.java index 457fe36ca2b88..8ed7fb43fb819 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionManagerServiceInternal.java +++ b/services/core/java/com/android/server/pm/permission/PermissionManagerServiceInternal.java @@ -512,16 +512,23 @@ public abstract class PermissionManagerServiceInternal extends PermissionManager public abstract void onPackageRemoved(@NonNull AndroidPackage pkg); /** - * Callback when the state for a package has been removed. + * Callback when a package has been uninstalled. + *

+ * The package may have been fully removed from the system, or only marked as uninstalled for + * this user but still instlaled for other users. * - * @param packageName the name of the removed package - * @param appId the app ID of the removed package - * @param pkg the removed package, or {@code null} if unavailable + * TODO: Pass PackageState instead. + * + * @param packageName the name of the uninstalled package + * @param appId the app ID of the uninstalled package + * @param pkg the uninstalled package, or {@code null} if unavailable * @param sharedUserPkgs the packages that are in the same shared user + * @param userId the user ID the package is uninstalled for */ //@SystemApi(client = SystemApi.Client.SYSTEM_SERVER) - public abstract void onPackageStateRemoved(@NonNull String packageName, int appId, - @Nullable AndroidPackage pkg, @NonNull List sharedUserPkgs); + public abstract void onPackageUninstalled(@NonNull String packageName, int appId, + @Nullable AndroidPackage pkg, @NonNull List sharedUserPkgs, + @UserIdInt int userId); /** * Check whether a permission can be propagated to instant app.