From cf5e437b6c1f23bbf3c65252fd9a30061bdd957c Mon Sep 17 00:00:00 2001 From: "Philip P. Moltmann" Date: Thu, 13 Jun 2019 15:49:08 -0700 Subject: [PATCH 1/4] Always use PermMgrSrv APIs to change permission So that the appropriate callbacks are called when a permission is changed. Test: cleared package state and saw PermissionPolicyService being run Bug: 132704705 Change-Id: Iea53b463fcc7d73dde89efbe91a7c5fa8b65bd7b --- .../server/pm/PackageManagerService.java | 126 +++++++++++++----- 1 file changed, 89 insertions(+), 37 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index 5f4d113601602..d2031b6c7e9cf 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -111,9 +111,6 @@ import static com.android.server.pm.PackageManagerServiceUtils.getCompressedFile import static com.android.server.pm.PackageManagerServiceUtils.getLastModifiedTime; import static com.android.server.pm.PackageManagerServiceUtils.logCriticalInfo; import static com.android.server.pm.PackageManagerServiceUtils.verifySignatures; -import static com.android.server.pm.permission.PermissionsState.PERMISSION_OPERATION_FAILURE; -import static com.android.server.pm.permission.PermissionsState.PERMISSION_OPERATION_SUCCESS; -import static com.android.server.pm.permission.PermissionsState.PERMISSION_OPERATION_SUCCESS_GIDS_CHANGED; import android.Manifest; import android.annotation.IntDef; @@ -294,6 +291,7 @@ import com.android.internal.util.ConcurrentUtils; import com.android.internal.util.DumpUtils; import com.android.internal.util.FastXmlSerializer; import com.android.internal.util.IndentingPrintWriter; +import com.android.internal.util.IntPair; import com.android.internal.util.Preconditions; import com.android.server.AttributeCache; import com.android.server.DeviceIdleController; @@ -19801,6 +19799,8 @@ public class PackageManagerService extends IPackageManager.Stub return; } + final String packageName = ps.pkg.packageName; + // These are flags that can change base on user actions. final int userSettableMask = FLAG_PERMISSION_USER_SET | FLAG_PERMISSION_USER_FIXED @@ -19810,8 +19810,59 @@ public class PackageManagerService extends IPackageManager.Stub final int policyOrSystemFlags = FLAG_PERMISSION_SYSTEM_FIXED | FLAG_PERMISSION_POLICY_FIXED; - boolean writeInstallPermissions = false; - boolean writeRuntimePermissions = false; + // Delay and combine non-async permission callbacks + final boolean[] permissionRemoved = new boolean[1]; + final ArraySet revokedPermissions = new ArraySet<>(); + final SparseBooleanArray updatedUsers = new SparseBooleanArray(); + + PermissionCallback delayingPermCallback = new PermissionCallback() { + public void onGidsChanged(int appId, int userId) { + mPermissionCallback.onGidsChanged(appId, userId); + } + + public void onPermissionChanged() { + mPermissionCallback.onPermissionChanged(); + } + + public void onPermissionGranted(int uid, int userId) { + mPermissionCallback.onPermissionGranted(uid, userId); + } + + public void onInstallPermissionGranted() { + mPermissionCallback.onInstallPermissionGranted(); + } + + public void onPermissionRevoked(int uid, int userId) { + revokedPermissions.add(IntPair.of(uid, userId)); + + updatedUsers.put(userId, true); + } + + public void onInstallPermissionRevoked() { + mPermissionCallback.onInstallPermissionRevoked(); + } + + public void onPermissionUpdated(int[] updatedUserIds, boolean sync) { + for (int userId : updatedUserIds) { + if (sync) { + updatedUsers.put(userId, true); + } else { + // Don't override sync=true by sync=false + if (!updatedUsers.get(userId)) { + updatedUsers.put(userId, false); + } + } + } + } + + public void onPermissionRemoved() { + permissionRemoved[0] = true; + } + + public void onInstallPermissionUpdated() { + mPermissionCallback.onInstallPermissionUpdated(); + } + }; final int permissionCount = ps.pkg.requestedPermissions.size(); for (int i = 0; i < permissionCount; i++) { @@ -19843,26 +19894,20 @@ public class PackageManagerService extends IPackageManager.Stub } } - final PermissionsState permissionsState = ps.getPermissionsState(); - - final int oldFlags = permissionsState.getPermissionFlags(permName, userId); + final int oldFlags = mPermissionManager.getPermissionFlags(permName, packageName, + Process.SYSTEM_UID, userId); // Always clear the user settable flags. - final boolean hasInstallState = - permissionsState.getInstallPermissionState(permName) != null; // If permission review is enabled and this is a legacy app, mark the // permission as requiring a review as this is the initial state. int flags = 0; if (ps.pkg.applicationInfo.targetSdkVersion < Build.VERSION_CODES.M && bp.isRuntime()) { flags |= FLAG_PERMISSION_REVIEW_REQUIRED | FLAG_PERMISSION_REVOKE_ON_UPGRADE; } - if (permissionsState.updatePermissionFlags(bp, userId, userSettableMask, flags)) { - if (hasInstallState) { - writeInstallPermissions = true; - } else { - writeRuntimePermissions = true; - } - } + + mPermissionManager.updatePermissionFlags(permName, packageName, + userSettableMask, flags, Process.SYSTEM_UID, userId, false, + delayingPermCallback); // Below is only runtime permission handling. if (!bp.isRuntime()) { @@ -19876,35 +19921,42 @@ public class PackageManagerService extends IPackageManager.Stub // If this permission was granted by default, make sure it is. if ((oldFlags & FLAG_PERMISSION_GRANTED_BY_DEFAULT) != 0) { - if (permissionsState.grantRuntimePermission(bp, userId) - != PERMISSION_OPERATION_FAILURE) { - writeRuntimePermissions = true; - } + mPermissionManager.grantRuntimePermission(permName, packageName, false, + Process.SYSTEM_UID, userId, delayingPermCallback); // If permission review is enabled the permissions for a legacy apps // are represented as constantly granted runtime ones, so don't revoke. } else if ((flags & FLAG_PERMISSION_REVIEW_REQUIRED) == 0) { // Otherwise, reset the permission. - final int revokeResult = permissionsState.revokeRuntimePermission(bp, userId); - switch (revokeResult) { - case PERMISSION_OPERATION_SUCCESS: - case PERMISSION_OPERATION_SUCCESS_GIDS_CHANGED: { - writeRuntimePermissions = true; - final int appId = ps.appId; - mHandler.post( - () -> killUid(appId, userId, KILL_APP_REASON_PERMISSIONS_REVOKED)); - } break; - } + mPermissionManager.revokeRuntimePermission(permName, packageName, false, userId, + delayingPermCallback); } } - // Synchronously write as we are taking permissions away. - if (writeRuntimePermissions) { - mSettings.writeRuntimePermissionsForUserLPr(userId, true); + // Execute delayed callbacks + if (permissionRemoved[0]) { + mPermissionCallback.onPermissionRemoved(); } - // Synchronously write as we are taking permissions away. - if (writeInstallPermissions) { - mSettings.writeLPr(); + // Slight variation on the code in mPermissionCallback.onPermissionRevoked() as we cannot + // kill uid while holding mPackages-lock + if (!revokedPermissions.isEmpty()) { + int numRevokedPermissions = revokedPermissions.size(); + for (int i = 0; i < numRevokedPermissions; i++) { + int revocationUID = IntPair.first(revokedPermissions.valueAt(i)); + int revocationUserId = IntPair.second(revokedPermissions.valueAt(i)); + + mOnPermissionChangeListeners.onPermissionsChanged(revocationUID); + + // Kill app later as we are holding mPackages + mHandler.post(() -> killUid(UserHandle.getAppId(revocationUID), revocationUserId, + KILL_APP_REASON_PERMISSIONS_REVOKED)); + } + } + + int numUpdatedUsers = updatedUsers.size(); + for (int i = 0; i < numUpdatedUsers; i++) { + mSettings.writeRuntimePermissionsForUserLPr(updatedUsers.keyAt(i), + updatedUsers.valueAt(i)); } } From 78c155b8a4fc0b79c76868d0ff0f5041e4f2f719 Mon Sep 17 00:00:00 2001 From: "Philip P. Moltmann" Date: Fri, 14 Jun 2019 09:02:24 -0700 Subject: [PATCH 2/4] Combine successive PermPolicySvc syncs As often we update many permissions and app-ops at once. Bug: 132704705 Test: RestrictedPermissionsTest Change-Id: I57176b3ec3e6f11b7dae8c14811b82094628d28a --- .../policy/PermissionPolicyService.java | 43 ++++++++++++++++--- 1 file changed, 36 insertions(+), 7 deletions(-) diff --git a/services/core/java/com/android/server/policy/PermissionPolicyService.java b/services/core/java/com/android/server/policy/PermissionPolicyService.java index 93b11beff239f..e01f42c642eb1 100644 --- a/services/core/java/com/android/server/policy/PermissionPolicyService.java +++ b/services/core/java/com/android/server/policy/PermissionPolicyService.java @@ -47,11 +47,14 @@ import android.permission.PermissionControllerManager; import android.permission.PermissionManagerInternal; import android.provider.Telephony; import android.telecom.TelecomManager; +import android.util.ArraySet; +import android.util.Pair; import android.util.Slog; import android.util.SparseBooleanArray; import android.util.SparseIntArray; import com.android.internal.annotations.GuardedBy; +import com.android.internal.util.function.pooled.PooledLambda; import com.android.server.FgThread; import com.android.server.LocalServices; import com.android.server.SystemService; @@ -76,6 +79,13 @@ public final class PermissionPolicyService extends SystemService { @GuardedBy("mLock") private final SparseBooleanArray mIsStarted = new SparseBooleanArray(); + /** + * Whether an async {@link #synchronizePackagePermissionsAndAppOpsForUser} is currently + * scheduled for a package/user. + */ + @GuardedBy("mLock") + private final ArraySet> mIsPackageSyncsScheduled = new ArraySet<>(); + public PermissionPolicyService(@NonNull Context context) { super(context); @@ -111,11 +121,26 @@ public final class PermissionPolicyService extends SystemService { }); permManagerInternal.addOnRuntimePermissionStateChangedListener( - (packageName, changedUserId) -> { - if (isStarted(changedUserId)) { - synchronizePackagePermissionsAndAppOpsForUser(packageName, changedUserId); + this::synchronizePackagePermissionsAndAppOpsAsyncForUser); + } + + private void synchronizePackagePermissionsAndAppOpsAsyncForUser(@NonNull String packageName, + @UserIdInt int changedUserId) { + if (isStarted(changedUserId)) { + synchronized (mLock) { + if (mIsPackageSyncsScheduled.add(new Pair<>(packageName, changedUserId))) { + FgThread.getHandler().sendMessage(PooledLambda.obtainMessage( + PermissionPolicyService + ::synchronizePackagePermissionsAndAppOpsForUser, + this, packageName, changedUserId)); + } else { + if (DEBUG) { + Slog.v(LOG_TAG, "sync for " + packageName + "/" + changedUserId + + " already scheduled"); } - }); + } + } + } } @Override @@ -230,10 +255,14 @@ public final class PermissionPolicyService extends SystemService { */ private void synchronizePackagePermissionsAndAppOpsForUser(@NonNull String packageName, @UserIdInt int userId) { + synchronized (mLock) { + mIsPackageSyncsScheduled.remove(new Pair<>(packageName, userId)); + } + if (DEBUG) { Slog.v(LOG_TAG, - "synchronizePackagePermissionsAndAppOpsForUser(" + packageName + ", " + userId - + ")"); + "synchronizePackagePermissionsAndAppOpsForUser(" + packageName + ", " + + userId + ")"); } final PackageManagerInternal packageManagerInternal = LocalServices.getService( @@ -578,7 +607,7 @@ public final class PermissionPolicyService extends SystemService { final int currentMode; try { currentMode = mAppOpsManager.unsafeCheckOpRaw(AppOpsManager - .opToPublicName(opCode), uid, packageName); + .opToPublicName(opCode), uid, packageName); } catch (SecurityException e) { // This might happen if the app was uninstalled in between the add and sync step. // In this case the package name cannot be resolved inside appops service and hence From fad1a8fb2f259cb9e699e9ac50c38f87d1921ba6 Mon Sep 17 00:00:00 2001 From: "Philip P. Moltmann" Date: Fri, 14 Jun 2019 09:02:24 -0700 Subject: [PATCH 3/4] Also trigger PermPolicySvc on app-ops changes PermissionPolicyService is supposed to keep permissions and app-ops in sync. Hence listen for app-ops changes too. Test: Reset app-ops and saw PermissionPolicyService triggered Fixes: 132704705 Change-Id: Ic208d9aebaab52c84893716777964c5b5cb254d2 --- .../permission/PermissionManagerService.java | 21 +++++ .../PermissionManagerServiceInternal.java | 4 + .../policy/PermissionPolicyService.java | 76 ++++++++++++++++--- .../SoftRestrictedPermissionPolicy.java | 36 ++++++--- 4 files changed, 115 insertions(+), 22 deletions(-) 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 e5f914d9bf75d..a7da3ec950f4d 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java +++ b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java @@ -3126,6 +3126,27 @@ public class PermissionManagerService { } } + @Override + public @NonNull ArrayList getAllPermissionWithProtectionLevel( + @PermissionInfo.Protection int protectionLevel) { + ArrayList matchingPermissions = new ArrayList<>(); + + synchronized (PermissionManagerService.this.mLock) { + int numTotalPermissions = mSettings.mPermissions.size(); + + for (int i = 0; i < numTotalPermissions; i++) { + BasePermission bp = mSettings.mPermissions.valueAt(i); + + if (bp.perm != null && bp.perm.info != null + && bp.protectionLevel == protectionLevel) { + matchingPermissions.add(bp.perm.info); + } + } + } + + return matchingPermissions; + } + @Override public @Nullable byte[] backupRuntimePermissions(@NonNull UserHandle user) { return PermissionManagerService.this.backupRuntimePermissions(user); 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 313de3daffa34..e5e9598bec577 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionManagerServiceInternal.java +++ b/services/core/java/com/android/server/pm/permission/PermissionManagerServiceInternal.java @@ -195,4 +195,8 @@ public abstract class PermissionManagerServiceInternal extends PermissionManager /** HACK HACK methods to allow for partial migration of data to the PermissionManager class */ public abstract @Nullable BasePermission getPermissionTEMP(@NonNull String permName); + + /** Get all permission that have a certain protection level */ + public abstract @NonNull ArrayList getAllPermissionWithProtectionLevel( + @PermissionInfo.Protection int protectionLevel); } diff --git a/services/core/java/com/android/server/policy/PermissionPolicyService.java b/services/core/java/com/android/server/policy/PermissionPolicyService.java index e01f42c642eb1..48f3d0b959967 100644 --- a/services/core/java/com/android/server/policy/PermissionPolicyService.java +++ b/services/core/java/com/android/server/policy/PermissionPolicyService.java @@ -41,10 +41,11 @@ import android.content.pm.PackageParser; import android.content.pm.PermissionInfo; import android.os.Build; import android.os.Process; +import android.os.RemoteException; +import android.os.ServiceManager; import android.os.UserHandle; import android.os.UserManagerInternal; import android.permission.PermissionControllerManager; -import android.permission.PermissionManagerInternal; import android.provider.Telephony; import android.telecom.TelecomManager; import android.util.ArraySet; @@ -54,10 +55,13 @@ import android.util.SparseBooleanArray; import android.util.SparseIntArray; import com.android.internal.annotations.GuardedBy; +import com.android.internal.app.IAppOpsCallback; +import com.android.internal.app.IAppOpsService; import com.android.internal.util.function.pooled.PooledLambda; import com.android.server.FgThread; import com.android.server.LocalServices; import com.android.server.SystemService; +import com.android.server.pm.permission.PermissionManagerServiceInternal; import java.util.ArrayList; import java.util.concurrent.CountDownLatch; @@ -96,8 +100,10 @@ public final class PermissionPolicyService extends SystemService { public void onStart() { final PackageManagerInternal packageManagerInternal = LocalServices.getService( PackageManagerInternal.class); - final PermissionManagerInternal permManagerInternal = LocalServices.getService( - PermissionManagerInternal.class); + final PermissionManagerServiceInternal permManagerInternal = LocalServices.getService( + PermissionManagerServiceInternal.class); + final IAppOpsService appOpsService = IAppOpsService.Stub.asInterface( + ServiceManager.getService(Context.APP_OPS_SERVICE)); packageManagerInternal.getPackageList(new PackageListObserver() { @Override @@ -122,6 +128,42 @@ public final class PermissionPolicyService extends SystemService { permManagerInternal.addOnRuntimePermissionStateChangedListener( this::synchronizePackagePermissionsAndAppOpsAsyncForUser); + + IAppOpsCallback appOpsListener = new IAppOpsCallback.Stub() { + public void opChanged(int op, int uid, String packageName) { + synchronizePackagePermissionsAndAppOpsAsyncForUser(packageName, + UserHandle.getUserId(uid)); + } + }; + + final ArrayList dangerousPerms = + permManagerInternal.getAllPermissionWithProtectionLevel( + PermissionInfo.PROTECTION_DANGEROUS); + + try { + int numDangerousPerms = dangerousPerms.size(); + for (int i = 0; i < numDangerousPerms; i++) { + PermissionInfo perm = dangerousPerms.get(i); + + if (perm.isHardRestricted() || perm.backgroundPermission != null) { + appOpsService.startWatchingMode(AppOpsManager.permissionToOpCode(perm.name), + null, appOpsListener); + } else if (perm.isSoftRestricted()) { + appOpsService.startWatchingMode(AppOpsManager.permissionToOpCode(perm.name), + null, appOpsListener); + + SoftRestrictedPermissionPolicy policy = + SoftRestrictedPermissionPolicy.forPermission(null, null, null, + perm.name); + if (policy.resolveAppOp() != OP_NONE) { + appOpsService.startWatchingMode(policy.resolveAppOp(), null, + appOpsListener); + } + } + } + } catch (RemoteException doesNotHappen) { + Slog.wtf(LOG_TAG, "Cannot set up app-ops listener"); + } } private void synchronizePackagePermissionsAndAppOpsAsyncForUser(@NonNull String packageName, @@ -380,7 +422,7 @@ public final class PermissionPolicyService extends SystemService { final int allowCount = mOpsToAllow.size(); for (int i = 0; i < allowCount; i++) { final OpToUnrestrict op = mOpsToAllow.get(i); - setUidModeAllowed(op.code, op.uid); + setUidModeAllowed(op.code, op.uid, op.packageName); } final int allowIfDefaultCount = mOpsToAllowIfDefault.size(); for (int i = 0; i < allowIfDefaultCount; i++) { @@ -390,12 +432,12 @@ public final class PermissionPolicyService extends SystemService { final int foregroundCount = mOpsToForeground.size(); for (int i = 0; i < foregroundCount; i++) { final OpToUnrestrict op = mOpsToForeground.get(i); - setUidModeForeground(op.code, op.uid); + setUidModeForeground(op.code, op.uid, op.packageName); } final int ignoreCount = mOpsToIgnore.size(); for (int i = 0; i < ignoreCount; i++) { final OpToUnrestrict op = mOpsToIgnore.get(i); - setUidModeIgnored(op.code, op.uid); + setUidModeIgnored(op.code, op.uid, op.packageName); } final int ignoreIfDefaultCount = mOpsToIgnoreIfDefault.size(); for (int i = 0; i < ignoreIfDefaultCount; i++) { @@ -586,20 +628,30 @@ public final class PermissionPolicyService extends SystemService { setUidModeIfDefault(opCode, uid, AppOpsManager.MODE_ALLOWED, packageName); } - private void setUidModeAllowed(int opCode, int uid) { - mAppOpsManager.setUidMode(opCode, uid, AppOpsManager.MODE_ALLOWED); + private void setUidModeAllowed(int opCode, int uid, @NonNull String packageName) { + setUidMode(opCode, uid, MODE_ALLOWED, packageName); } - private void setUidModeForeground(int opCode, int uid) { - mAppOpsManager.setUidMode(opCode, uid, AppOpsManager.MODE_FOREGROUND); + private void setUidModeForeground(int opCode, int uid, @NonNull String packageName) { + setUidMode(opCode, uid, MODE_FOREGROUND, packageName); } private void setUidModeIgnoredIfDefault(int opCode, int uid, @NonNull String packageName) { setUidModeIfDefault(opCode, uid, AppOpsManager.MODE_IGNORED, packageName); } - private void setUidModeIgnored(int opCode, int uid) { - mAppOpsManager.setUidMode(opCode, uid, MODE_IGNORED); + private void setUidModeIgnored(int opCode, int uid, @NonNull String packageName) { + setUidMode(opCode, uid, MODE_IGNORED, packageName); + } + + private void setUidMode(int opCode, int uid, int mode, + @NonNull String packageName) { + final int currentMode = mAppOpsManager.unsafeCheckOpRaw(AppOpsManager + .opToPublicName(opCode), uid, packageName); + + if (currentMode != mode) { + mAppOpsManager.setUidMode(opCode, uid, mode); + } } private void setUidModeIfDefault(int opCode, int uid, int mode, diff --git a/services/core/java/com/android/server/policy/SoftRestrictedPermissionPolicy.java b/services/core/java/com/android/server/policy/SoftRestrictedPermissionPolicy.java index 90e0dc4abd21e..1658833988976 100644 --- a/services/core/java/com/android/server/policy/SoftRestrictedPermissionPolicy.java +++ b/services/core/java/com/android/server/policy/SoftRestrictedPermissionPolicy.java @@ -29,6 +29,7 @@ import static android.content.pm.PackageManager.FLAG_PERMISSION_RESTRICTION_SYST import static android.content.pm.PackageManager.FLAG_PERMISSION_RESTRICTION_UPGRADE_EXEMPT; import android.annotation.NonNull; +import android.annotation.Nullable; import android.app.AppOpsManager; import android.content.Context; import android.content.pm.ApplicationInfo; @@ -75,14 +76,16 @@ public abstract class SoftRestrictedPermissionPolicy { * Get the policy for a soft restricted permission. * * @param context A context to use - * @param appInfo The application the permission belongs to - * @param user The user the app belongs to + * @param appInfo The application the permission belongs to. Can be {@code null}, but then + * only {@link #resolveAppOp} will work. + * @param user The user the app belongs to. Can be {@code null}, but then only + * {@link #resolveAppOp} will work. * @param permission The name of the permission * * @return The policy for this permission */ public static @NonNull SoftRestrictedPermissionPolicy forPermission(@NonNull Context context, - @NonNull ApplicationInfo appInfo, @NonNull UserHandle user, + @Nullable ApplicationInfo appInfo, @Nullable UserHandle user, @NonNull String permission) { switch (permission) { // Storage uses a special app op to decide the mount state and supports soft restriction @@ -90,13 +93,26 @@ public abstract class SoftRestrictedPermissionPolicy { // collections. case READ_EXTERNAL_STORAGE: case WRITE_EXTERNAL_STORAGE: { - int flags = context.getPackageManager().getPermissionFlags( - permission, appInfo.packageName, user); - boolean applyRestriction = (flags & FLAG_PERMISSION_APPLY_RESTRICTION) != 0; - boolean isWhiteListed = (flags & FLAGS_PERMISSION_RESTRICTION_ANY_EXEMPT) != 0; - boolean hasRequestedLegacyExternalStorage = - appInfo.hasRequestedLegacyExternalStorage(); - int targetSDK = appInfo.targetSdkVersion; + final int flags; + final boolean applyRestriction; + final boolean isWhiteListed; + final boolean hasRequestedLegacyExternalStorage; + final int targetSDK; + + if (appInfo != null) { + flags = context.getPackageManager().getPermissionFlags(permission, + appInfo.packageName, user); + applyRestriction = (flags & FLAG_PERMISSION_APPLY_RESTRICTION) != 0; + isWhiteListed = (flags & FLAGS_PERMISSION_RESTRICTION_ANY_EXEMPT) != 0; + hasRequestedLegacyExternalStorage = appInfo.hasRequestedLegacyExternalStorage(); + targetSDK = appInfo.targetSdkVersion; + } else { + flags = 0; + applyRestriction = false; + isWhiteListed = false; + hasRequestedLegacyExternalStorage = false; + targetSDK = 0; + } return new SoftRestrictedPermissionPolicy() { @Override From 4e369a79d018263c02cf7b46b4eb76c3efe67d5f Mon Sep 17 00:00:00 2001 From: "Philip P. Moltmann" Date: Fri, 14 Jun 2019 11:08:16 -0700 Subject: [PATCH 4/4] Avoid overiding bg perm app-op for pre-M apps For pre-M apps the only information about the permission state is encoded in the app-op. This state cannot be computed from the permission state. Hence be very careful to only override this state when sure that it is fine. E.g. one problem is that during the install the permissions are first not whitelisted and then whitelisted shortly after. At this time the permissions state might be lost. Fixes: 135288572, 135190563 Test: atest RestrictedPermissionsTest Change-Id: I8bf74730a6c632313d68f1558c9b67e273c5d7e1 --- .../policy/PermissionPolicyService.java | 97 ++++++++++++------- 1 file changed, 63 insertions(+), 34 deletions(-) diff --git a/services/core/java/com/android/server/policy/PermissionPolicyService.java b/services/core/java/com/android/server/policy/PermissionPolicyService.java index 48f3d0b959967..68feb4a8445eb 100644 --- a/services/core/java/com/android/server/policy/PermissionPolicyService.java +++ b/services/core/java/com/android/server/policy/PermissionPolicyService.java @@ -23,6 +23,7 @@ import static android.app.AppOpsManager.MODE_FOREGROUND; import static android.app.AppOpsManager.MODE_IGNORED; import static android.app.AppOpsManager.OP_NONE; import static android.content.pm.PackageManager.FLAG_PERMISSION_APPLY_RESTRICTION; +import static android.content.pm.PackageManager.FLAG_PERMISSION_REVIEW_REQUIRED; import static android.content.pm.PackageManager.GET_PERMISSIONS; import android.annotation.NonNull; @@ -407,6 +408,16 @@ public final class PermissionPolicyService extends SystemService { */ private final @NonNull ArrayList mOpsToForeground = new ArrayList<>(); + /** + * All ops that need to be flipped to foreground if allow. + * + * Currently, only used by the foreground/background permissions logic. + * + * @see #syncPackages + */ + private final @NonNull ArrayList mOpsToForegroundIfAllow = + new ArrayList<>(); + PermissionToOpSynchroniser(@NonNull Context context) { mContext = context; mPackageManager = context.getPackageManager(); @@ -429,8 +440,13 @@ public final class PermissionPolicyService extends SystemService { final OpToUnrestrict op = mOpsToAllowIfDefault.get(i); setUidModeAllowedIfDefault(op.code, op.uid, op.packageName); } - final int foregroundCount = mOpsToForeground.size(); + final int foregroundCount = mOpsToForegroundIfAllow.size(); for (int i = 0; i < foregroundCount; i++) { + final OpToUnrestrict op = mOpsToForegroundIfAllow.get(i); + setUidModeForegroundIfAllow(op.code, op.uid, op.packageName); + } + final int foregroundIfAllowCount = mOpsToForeground.size(); + for (int i = 0; i < foregroundIfAllowCount; i++) { final OpToUnrestrict op = mOpsToForeground.get(i); setUidModeForeground(op.code, op.uid, op.packageName); } @@ -530,6 +546,24 @@ public final class PermissionPolicyService extends SystemService { } } + private boolean isBgPermRestricted(@NonNull String pkg, @NonNull String perm, int uid) { + try { + final PermissionInfo bgPermInfo = mPackageManager.getPermissionInfo(perm, 0); + + if (bgPermInfo.isSoftRestricted()) { + Slog.wtf(LOG_TAG, "Support for soft restricted background permissions not " + + "implemented"); + } + + return bgPermInfo.isHardRestricted() && (mPackageManager.getPermissionFlags( + perm, pkg, UserHandle.getUserHandleForUid(uid)) + & FLAG_PERMISSION_APPLY_RESTRICTION) != 0; + } catch (NameNotFoundException e) { + Slog.w(LOG_TAG, "Cannot read permission state of " + perm, e); + return false; + } + } + /** * Add op that belong to a foreground permission for later processing in * {@link #syncPackages()}. @@ -552,26 +586,27 @@ public final class PermissionPolicyService extends SystemService { final String pkgName = pkg.packageName; final int uid = pkg.applicationInfo.uid; - if (mPackageManager.checkPermission(permission, pkgName) - == PackageManager.PERMISSION_GRANTED) { - boolean isBgHardRestricted = false; - try { - final PermissionInfo bgPermInfo = mPackageManager.getPermissionInfo( - bgPermissionName, 0); + // App does not support runtime permissions. Hence the state is encoded in the app-op. + // To not override unrecoverable state don't change app-op unless bg perm is reviewed. + if (pkg.applicationInfo.targetSdkVersion < Build.VERSION_CODES.M) { + // If the review is required for this permission, the grant state does not + // really matter. To have a stable state, don't change the app-op if review is still + // pending. + int flags = mPackageManager.getPermissionFlags(bgPermissionName, + pkg.packageName, UserHandle.getUserHandleForUid(uid)); - if (bgPermInfo.isSoftRestricted()) { - Slog.wtf(LOG_TAG, "Support for soft restricted background permissions not " - + "implemented"); - } - - isBgHardRestricted = - bgPermInfo.isHardRestricted() && (mPackageManager.getPermissionFlags( - bgPermissionName, pkgName, UserHandle.getUserHandleForUid(uid)) - & FLAG_PERMISSION_APPLY_RESTRICTION) != 0; - } catch (NameNotFoundException e) { - Slog.w(LOG_TAG, "Cannot read permission state of " + bgPermissionName, e); + if ((flags & FLAG_PERMISSION_REVIEW_REQUIRED) == 0 + && isBgPermRestricted(pkgName, bgPermissionName, uid)) { + mOpsToForegroundIfAllow.add(new OpToUnrestrict(uid, pkgName, opCode)); } + return; + } + + if (mPackageManager.checkPermission(permission, pkgName) + == PackageManager.PERMISSION_GRANTED) { + final boolean isBgHardRestricted = isBgPermRestricted(pkgName, bgPermissionName, + uid); final boolean isBgPermGranted = mPackageManager.checkPermission(bgPermissionName, pkgName) == PackageManager.PERMISSION_GRANTED; @@ -625,19 +660,23 @@ public final class PermissionPolicyService extends SystemService { } private void setUidModeAllowedIfDefault(int opCode, int uid, @NonNull String packageName) { - setUidModeIfDefault(opCode, uid, AppOpsManager.MODE_ALLOWED, packageName); + setUidModeIfMode(opCode, uid, MODE_DEFAULT, MODE_ALLOWED, packageName); } private void setUidModeAllowed(int opCode, int uid, @NonNull String packageName) { setUidMode(opCode, uid, MODE_ALLOWED, packageName); } + private void setUidModeForegroundIfAllow(int opCode, int uid, @NonNull String packageName) { + setUidModeIfMode(opCode, uid, MODE_ALLOWED, MODE_FOREGROUND, packageName); + } + private void setUidModeForeground(int opCode, int uid, @NonNull String packageName) { setUidMode(opCode, uid, MODE_FOREGROUND, packageName); } private void setUidModeIgnoredIfDefault(int opCode, int uid, @NonNull String packageName) { - setUidModeIfDefault(opCode, uid, AppOpsManager.MODE_IGNORED, packageName); + setUidModeIfMode(opCode, uid, MODE_DEFAULT, MODE_IGNORED, packageName); } private void setUidModeIgnored(int opCode, int uid, @NonNull String packageName) { @@ -654,23 +693,13 @@ public final class PermissionPolicyService extends SystemService { } } - private void setUidModeIfDefault(int opCode, int uid, int mode, + private void setUidModeIfMode(int opCode, int uid, int requiredModeBefore, int newMode, @NonNull String packageName) { - final int currentMode; - try { - currentMode = mAppOpsManager.unsafeCheckOpRaw(AppOpsManager + final int currentMode = mAppOpsManager.unsafeCheckOpRaw(AppOpsManager .opToPublicName(opCode), uid, packageName); - } catch (SecurityException e) { - // This might happen if the app was uninstalled in between the add and sync step. - // In this case the package name cannot be resolved inside appops service and hence - // the uid does not match. - Slog.w(LOG_TAG, "Cannot set mode of uid=" + uid + " op=" + opCode + " to " + mode, - e); - return; - } - if (currentMode == MODE_DEFAULT) { - mAppOpsManager.setUidMode(opCode, uid, mode); + if (currentMode == requiredModeBefore) { + mAppOpsManager.setUidMode(opCode, uid, newMode); } }