From 65d46e9b05bccaf1bca91cf7528393f150568c41 Mon Sep 17 00:00:00 2001 From: Hai Zhang Date: Mon, 9 Nov 2020 16:38:28 -0800 Subject: [PATCH] Refactor getAppOpPermissionPackages() as API. - Remove IPermissionManager.getAppOpPermissionPackages() and its usage because we are not going to expose it as a new API on PermissionManager. - Make PermissionManagerServiceInternal.getAppOpPermissionPackages() unchecked because it's an internal API. Internal APIs should be by default unchecked and checks should be done manually when necessary. - The parameters and return value of PermissionManagerServiceInternal.getAppOpPermissionPackages() are also made non-null to be a good API, and EmptyArray.STRING is returned in the empty case so there won't be a performance penalty. IPackageManager.getAppOpPermissionPackages() will return an empty array for null permissionName before calling PermissionManagerServiceInternal.getAppOpPermissionPackages() for compatibility, and clients should handle returned empty arrays as good as null. - Use PermissionManagerServiceInternal.getAppOpPermissionPackages() only to support the @UnsupportedAppUsage of IPackageManager.getAppOpPermissionPackages() and perform checks there. Bug: 158736025 Test: presubmit Change-Id: I0f96e898daa4cf40706430f1b7fbd5737a1f97f8 --- .../permission/IPermissionManager.aidl | 2 - .../packageinstaller/InstallStart.java | 59 ++++++++----------- .../server/pm/PackageManagerService.java | 50 ++++++++-------- .../permission/PermissionManagerService.java | 31 ++++------ .../PermissionManagerServiceInternal.java | 12 +++- 5 files changed, 73 insertions(+), 81 deletions(-) diff --git a/core/java/android/permission/IPermissionManager.aidl b/core/java/android/permission/IPermissionManager.aidl index e23102113e9f1..1001668149754 100644 --- a/core/java/android/permission/IPermissionManager.aidl +++ b/core/java/android/permission/IPermissionManager.aidl @@ -29,8 +29,6 @@ import android.permission.IOnPermissionsChangeListener; * @hide */ interface IPermissionManager { - String[] getAppOpPermissionPackages(String permName); - ParceledListSlice getAllPermissionGroups(int flags); PermissionGroupInfo getPermissionGroupInfo(String groupName, int flags); diff --git a/packages/PackageInstaller/src/com/android/packageinstaller/InstallStart.java b/packages/PackageInstaller/src/com/android/packageinstaller/InstallStart.java index bbcc6a27babfa..af05078b4f3ad 100644 --- a/packages/PackageInstaller/src/com/android/packageinstaller/InstallStart.java +++ b/packages/PackageInstaller/src/com/android/packageinstaller/InstallStart.java @@ -22,24 +22,21 @@ import android.Manifest; import android.annotation.Nullable; import android.app.Activity; import android.app.ActivityManager; -import android.app.AppGlobals; import android.content.ContentResolver; import android.content.Intent; import android.content.pm.ApplicationInfo; -import android.content.pm.IPackageManager; +import android.content.pm.PackageInfo; import android.content.pm.PackageInstaller; import android.content.pm.PackageManager; import android.content.pm.ProviderInfo; -import android.content.pm.UserInfo; import android.net.Uri; import android.os.Build; import android.os.Bundle; import android.os.RemoteException; import android.os.UserManager; -import android.permission.IPermissionManager; import android.util.Log; -import java.util.List; +import java.util.Arrays; /** * Select which activity is the first visible activity of the installation and forward the intent to @@ -49,16 +46,14 @@ public class InstallStart extends Activity { private static final String LOG_TAG = InstallStart.class.getSimpleName(); private static final String DOWNLOADS_AUTHORITY = "downloads"; - private IPackageManager mIPackageManager; - private IPermissionManager mIPermissionManager; + private PackageManager mPackageManager; private UserManager mUserManager; private boolean mAbortInstall = false; @Override protected void onCreate(@Nullable Bundle savedInstanceState) { super.onCreate(savedInstanceState); - mIPackageManager = AppGlobals.getPackageManager(); - mIPermissionManager = AppGlobals.getPermissionManager(); + mPackageManager = getPackageManager(); mUserManager = getSystemService(UserManager.class); Intent intent = getIntent(); String callingPackage = getCallingPackage(); @@ -94,7 +89,7 @@ public class InstallStart extends Activity { Log.w(LOG_TAG, "Cannot get target sdk version for uid " + originatingUid); // Invalid originating uid supplied. Abort install. mAbortInstall = true; - } else if (targetSdkVersion >= Build.VERSION_CODES.O && !declaresAppOpPermission( + } else if (targetSdkVersion >= Build.VERSION_CODES.O && !isUidRequestingPermission( originatingUid, Manifest.permission.REQUEST_INSTALL_PACKAGES)) { Log.e(LOG_TAG, "Requesting uid " + originatingUid + " needs to declare permission " + Manifest.permission.REQUEST_INSTALL_PACKAGES); @@ -150,26 +145,24 @@ public class InstallStart extends Activity { finish(); } - private boolean declaresAppOpPermission(int uid, String permission) { - try { - final String[] packages = mIPermissionManager.getAppOpPermissionPackages(permission); - if (packages == null) { - return false; + private boolean isUidRequestingPermission(int uid, String permission) { + final String[] packageNames = mPackageManager.getPackagesForUid(uid); + if (packageNames == null) { + return false; + } + for (final String packageName : packageNames) { + final PackageInfo packageInfo; + try { + packageInfo = mPackageManager.getPackageInfo(packageName, + PackageManager.GET_PERMISSIONS); + } catch (PackageManager.NameNotFoundException e) { + // Ignore and try the next package + continue; } - final List users = mUserManager.getUsers(); - for (String packageName : packages) { - for (UserInfo user : users) { - try { - if (uid == getPackageManager().getPackageUidAsUser(packageName, user.id)) { - return true; - } - } catch (PackageManager.NameNotFoundException e) { - // Ignore and try the next package - } - } + if (packageInfo.requestedPermissions != null + && Arrays.asList(packageInfo.requestedPermissions).contains(permission)) { + return true; } - } catch (RemoteException rexc) { - // If remote package manager cannot be reached, install will likely fail anyway. } return false; } @@ -215,13 +208,9 @@ public class InstallStart extends Activity { return PackageInstaller.SessionParams.UID_UNKNOWN; } } - try { - if (mIPackageManager.checkUidPermission(Manifest.permission.MANAGE_DOCUMENTS, - callingUid) == PackageManager.PERMISSION_GRANTED) { - return uidFromIntent; - } - } catch (RemoteException rexc) { - // Ignore. Should not happen. + if (checkPermission(Manifest.permission.MANAGE_DOCUMENTS, -1, callingUid) + == PackageManager.PERMISSION_GRANTED) { + return uidFromIntent; } if (isSystemDownloadsProvider(callingUid)) { return uidFromIntent; diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index 7afd3dbb2c1ef..7409a9c332a81 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -6604,14 +6604,16 @@ public class PackageManagerService extends IPackageManager.Stub } // NOTE: Can't remove due to unsupported app usage + @NonNull @Override - public String[] getAppOpPermissionPackages(String permName) { - try { - // Because this is accessed via the package manager service AIDL, - // go through the permission manager service AIDL - return mPermissionManagerService.getAppOpPermissionPackages(permName); - } catch (RemoteException ignore) { } - return null; + public String[] getAppOpPermissionPackages(String permissionName) { + if (permissionName == null) { + return EmptyArray.STRING; + } + if (getInstantAppPackageName(getCallingUid()) != null) { + return EmptyArray.STRING; + } + return mPermissionManager.getAppOpPermissionPackages(permissionName); } @Override @@ -26118,27 +26120,29 @@ public class PackageManagerService extends IPackageManager.Stub throw new SecurityException( "Caller uid " + callingUid + " does not own package " + packageName); } - ApplicationInfo info = getApplicationInfo(packageName, flags, userId); - if (info == null) { - return false; - } - if (info.targetSdkVersion < Build.VERSION_CODES.O) { - return false; - } if (isInstantApp(packageName, userId)) { return false; } - String appOpPermission = Manifest.permission.REQUEST_INSTALL_PACKAGES; - String[] packagesDeclaringPermission = - mPermissionManager.getAppOpPermissionPackages(appOpPermission, callingUid); - if (!ArrayUtils.contains(packagesDeclaringPermission, packageName)) { - if (throwIfPermNotDeclared) { - throw new SecurityException("Need to declare " + appOpPermission - + " to call this api"); - } else { - Slog.e(TAG, "Need to declare " + appOpPermission + " to call this api"); + synchronized (mLock) { + final AndroidPackage pkg = mPackages.get(packageName); + if (pkg == null) { return false; } + if (pkg.getTargetSdkVersion() < Build.VERSION_CODES.O) { + return false; + } + if (!pkg.getRequestedPermissions().contains( + android.Manifest.permission.REQUEST_INSTALL_PACKAGES)) { + final String message = "Need to declare " + + android.Manifest.permission.REQUEST_INSTALL_PACKAGES + + " to call this api"; + if (throwIfPermNotDeclared) { + throw new SecurityException(message); + } else { + Slog.e(TAG, message); + return false; + } + } } if (mUserManager.hasUserRestriction(UserManager.DISALLOW_INSTALL_UNKNOWN_SOURCES, userId) || mUserManager.hasUserRestriction( 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 ff661a8860d09..01eee06306519 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java +++ b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java @@ -582,21 +582,15 @@ public class PermissionManagerService extends IPermissionManager.Stub { } } - @Override - public String[] getAppOpPermissionPackages(String permName) { - return getAppOpPermissionPackagesInternal(permName, getCallingUid()); - } - - private String[] getAppOpPermissionPackagesInternal(String permName, int callingUid) { - if (mPackageManagerInt.getInstantAppPackageName(callingUid) != null) { - return null; - } + @NonNull + private String[] getAppOpPermissionPackagesInternal(@NonNull String permissionName) { synchronized (mLock) { - final ArraySet pkgs = mRegistry.getAppOpPermissionPackages(permName); - if (pkgs == null) { - return null; + final ArraySet packageNames = mRegistry.getAppOpPermissionPackages( + permissionName); + if (packageNames == null) { + return EmptyArray.STRING; } - return pkgs.toArray(new String[pkgs.size()]); + return packageNames.toArray(new String[0]); } } @@ -5062,6 +5056,12 @@ public class PermissionManagerService extends IPermissionManager.Stub { public int[] getPermissionGids(@NonNull String permissionName, @UserIdInt int userId) { return PermissionManagerService.this.getPermissionGids(permissionName, userId); } + @NonNull + @Override + public String[] getAppOpPermissionPackages(@NonNull String permissionName) { + Objects.requireNonNull(permissionName, "permissionName"); + return PermissionManagerService.this.getAppOpPermissionPackagesInternal(permissionName); + } @Override public void grantRequestedRuntimePermissions(AndroidPackage pkg, int[] userIds, String[] grantedPermissions, int callingUid) { @@ -5107,11 +5107,6 @@ public class PermissionManagerService extends IPermissionManager.Stub { (AndroidPackage pkg) -> resetRuntimePermissionsInternal(pkg, userId)); } @Override - public String[] getAppOpPermissionPackages(String permName, int callingUid) { - return PermissionManagerService.this - .getAppOpPermissionPackagesInternal(permName, callingUid); - } - @Override public void enforceCrossUserPermission(int callingUid, int userId, boolean requireFullPermission, boolean checkShell, String message) { PermissionManagerService.this.enforceCrossUserPermission(callingUid, userId, 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 df81bac99a211..dcacf3d5ecd12 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionManagerServiceInternal.java +++ b/services/core/java/com/android/server/pm/permission/PermissionManagerServiceInternal.java @@ -338,9 +338,15 @@ public abstract class PermissionManagerServiceInternal extends PermissionManager @Nullable public abstract int[] getPermissionGids(@NonNull String permissionName, @UserIdInt int userId); - /** Retrieve the packages that have requested the given app op permission */ - public abstract @Nullable String[] getAppOpPermissionPackages( - @NonNull String permName, int callingUid); + /** + * Get the packages that have requested an app op permission. + * + * @param permissionName the name of the app op permission + * @return the names of the packages that have requested the app op permission + */ + //@SystemApi(client = SystemApi.Client.SYSTEM_SERVER) + @NonNull + public abstract String[] getAppOpPermissionPackages(@NonNull String permissionName); /** * Enforces the request is from the system or an app that has INTERACT_ACROSS_USERS