From 3c499ea9ef299c2b3966ad61bab0361c10d983ac Mon Sep 17 00:00:00 2001 From: Svet Ganov Date: Fri, 26 Jul 2019 17:45:56 -0700 Subject: [PATCH] Don't drop restricted permissions on upgrade Restricted permissions cannot be held until whitelisted. In a P -> Q upgrade we grandfather all restricted permissions. However, the whitelisting code runs after the internal update of permission happens for the first time resulting in a revocation of the restricted permissions we were about to grandfather. The fix is to not deal with restricted permission when updating the permissions state until the permission controller has run the grandfathering logic and once the latter happens we do run the permission update logic again to properly handle the restricted permissions. Bug: 138263882 Test: atest CtsPermissionTestCases atest CtsPermission2TestCases atest CtsAppSecurityHostTestCases:android.appsecurity.cts.PermissionsHostTest P -> Q upgrade preserves grandfathered restricted permissions P -> Bad Q build -> Q fixes up broken fixed restricted permissions Change-Id: Iaef80426bf50181df93d1380af1d0855340def8e (cherry picked from commit 0b41c8940a44a9eff4b277ce019a1ffdb3a44b7e) --- .../server/pm/PackageManagerService.java | 12 +++ .../DefaultPermissionGrantPolicy.java | 16 +++- .../permission/PermissionManagerService.java | 83 ++++++++++++------- .../policy/PermissionPolicyInternal.java | 27 ++++++ .../policy/PermissionPolicyService.java | 25 ++++++ .../java/com/android/server/SystemServer.java | 10 +-- 6 files changed, 135 insertions(+), 38 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index 0d1b9c68a2147..22b1f250b498f 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -313,6 +313,8 @@ import com.android.server.pm.permission.BasePermission; import com.android.server.pm.permission.PermissionManagerService; import com.android.server.pm.permission.PermissionManagerServiceInternal; import com.android.server.pm.permission.PermissionsState; +import com.android.server.policy.PermissionPolicyInternal; +import com.android.server.policy.PermissionPolicyInternal.OnInitializedCallback; import com.android.server.security.VerityUtils; import com.android.server.storage.DeviceStorageMonitorInternal; import com.android.server.utils.TimingsTraceAndSlog; @@ -20273,6 +20275,16 @@ public class PackageManagerService extends IPackageManager.Stub // try optimizing this. synchronized (mLock) { mPermissionManager.updateAllPermissions(StorageManager.UUID_PRIVATE_INTERNAL, false); + + final PermissionPolicyInternal permissionPolicyInternal = + LocalServices.getService(PermissionPolicyInternal.class); + permissionPolicyInternal.setOnInitializedCallback(userId -> { + // The SDK updated case is already handled when we run during the ctor. + synchronized (mPackages) { + mPermissionManager.updateAllPermissions( + StorageManager.UUID_PRIVATE_INTERNAL, false); + } + }); } // Watch for external volumes that come and go over time diff --git a/services/core/java/com/android/server/pm/permission/DefaultPermissionGrantPolicy.java b/services/core/java/com/android/server/pm/permission/DefaultPermissionGrantPolicy.java index fe0b3a3966f94..3e655edf5db09 100644 --- a/services/core/java/com/android/server/pm/permission/DefaultPermissionGrantPolicy.java +++ b/services/core/java/com/android/server/pm/permission/DefaultPermissionGrantPolicy.java @@ -1170,6 +1170,11 @@ public final class DefaultPermissionGrantPolicy { final int flags = mContext.getPackageManager().getPermissionFlags( permission, pkg.packageName, user); + // If we are trying to grant as system fixed and already system fixed + // then the system can change the system fixed grant state. + final boolean changingGrantForSystemFixed = systemFixed + && (flags & PackageManager.FLAG_PERMISSION_SYSTEM_FIXED) != 0; + // Certain flags imply that the permission's current state by the system or // device/profile owner or the user. In these cases we do not want to clobber the // current state. @@ -1177,7 +1182,8 @@ public final class DefaultPermissionGrantPolicy { // Unless the caller wants to override user choices. The override is // to make sure we can grant the needed permission to the default // sms and phone apps after the user chooses this in the UI. - if (!isFixedOrUserSet(flags) || ignoreSystemPackage) { + if (!isFixedOrUserSet(flags) || ignoreSystemPackage + || changingGrantForSystemFixed) { // Never clobber policy fixed permissions. // We must allow the grant of a system-fixed permission because // system-fixed is sticky, but the permission itself may be revoked. @@ -1196,6 +1202,14 @@ public final class DefaultPermissionGrantPolicy { PackageManager.FLAG_PERMISSION_RESTRICTION_SYSTEM_EXEMPT, user); } + // If the system tries to change a system fixed permission from one fixed + // state to another we need to drop the fixed flag to allow the grant. + if (changingGrantForSystemFixed) { + mContext.getPackageManager().updatePermissionFlags(permission, + pkg.packageName, flags, + flags & ~PackageManager.FLAG_PERMISSION_SYSTEM_FIXED, user); + } + if (pm.checkPermission(permission, pkg.packageName) != PackageManager.PERMISSION_GRANTED) { mContext.getPackageManager() 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 674e8921ff05e..210a7afb11a2a 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java +++ b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java @@ -125,6 +125,7 @@ import com.android.server.pm.permission.PermissionManagerServiceInternal.Default import com.android.server.pm.permission.PermissionManagerServiceInternal.DefaultHomeProvider; import com.android.server.pm.permission.PermissionManagerServiceInternal.PermissionCallback; import com.android.server.pm.permission.PermissionsState.PermissionState; +import com.android.server.policy.PermissionPolicyInternal; import com.android.server.policy.SoftRestrictedPermissionPolicy; import libcore.util.EmptyArray; @@ -227,6 +228,9 @@ public class PermissionManagerService extends IPermissionManager.Stub { @GuardedBy("mLock") private boolean mSystemReady; + @GuardedBy("mLock") + private PermissionPolicyInternal mPermissionPolicyInternal; + /** * For each foreground/background permission the mapping: * Background permission -> foreground permissions @@ -2443,6 +2447,13 @@ public class PermissionManagerService extends IPermissionManager.Stub { boolean softRestricted = bp.isSoftRestricted(); for (int userId : currentUserIds) { + // If permission policy is not ready we don't deal with restricted + // permissions as the policy may whitelist some permissions. Once + // the policy is initialized we would re-evaluate permissions. + final boolean permissionPolicyInitialized = + mPermissionPolicyInternal != null + && mPermissionPolicyInternal.isInitialized(userId); + PermissionState permState = origPermissions .getRuntimePermissionState(perm, userId); int flags = permState != null ? permState.getFlags() : 0; @@ -2457,7 +2468,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { if (appSupportsRuntimePermissions) { // If hard restricted we don't allow holding it - if (hardRestricted) { + if (permissionPolicyInitialized && hardRestricted) { if (!restrictionExempt) { if (permState != null && permState.isGranted() && permissionsState.revokeRuntimePermission( @@ -2470,7 +2481,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { } } // If soft restricted we allow holding in a restricted form - } else if (softRestricted) { + } else if (permissionPolicyInitialized && softRestricted) { // Regardless if granted set the restriction flag as it // may affect app treatment based on this permission. if (!restrictionExempt && !restrictionApplied) { @@ -2489,7 +2500,8 @@ public class PermissionManagerService extends IPermissionManager.Stub { flags &= ~FLAG_PERMISSION_REVOKE_ON_UPGRADE; wasChanged = true; // Hard restricted permissions cannot be held. - } else if (!hardRestricted || restrictionExempt) { + } else if (!permissionPolicyInitialized + || (!hardRestricted || restrictionExempt)) { if (permState != null && permState.isGranted()) { if (permissionsState.grantRuntimePermission(bp, userId) == PERMISSION_OPERATION_FAILURE) { @@ -2518,33 +2530,28 @@ public class PermissionManagerService extends IPermissionManager.Stub { // If legacy app always grant the permission but if restricted // and not exempt take a note a restriction should be applied. - if ((hardRestricted || softRestricted) - && !restrictionExempt && !restrictionApplied) { + if (permissionPolicyInitialized + && (hardRestricted || softRestricted) + && !restrictionExempt && !restrictionApplied) { flags |= FLAG_PERMISSION_APPLY_RESTRICTION; wasChanged = true; } } // If unrestricted or restriction exempt, don't apply restriction. - if (!(hardRestricted || softRestricted) || restrictionExempt) { - if (restrictionApplied) { - flags &= ~FLAG_PERMISSION_APPLY_RESTRICTION; - // Dropping restriction on a legacy app requires a review. - if (!appSupportsRuntimePermissions) { - flags |= FLAG_PERMISSION_REVIEW_REQUIRED; + if (permissionPolicyInitialized) { + if (!(hardRestricted || softRestricted) || restrictionExempt) { + if (restrictionApplied) { + flags &= ~FLAG_PERMISSION_APPLY_RESTRICTION; + // Dropping restriction on a legacy app implies a review + if (!appSupportsRuntimePermissions) { + flags |= FLAG_PERMISSION_REVIEW_REQUIRED; + } + wasChanged = true; } - wasChanged = true; } } - if (hardRestricted && !restrictionExempt - && (flags & FLAG_PERMISSION_SYSTEM_FIXED) != 0) { - // Applying a hard restriction implies revoking it. This might - // lead to a system-fixed, revoked permission. - flags &= ~FLAG_PERMISSION_SYSTEM_FIXED; - wasChanged = true; - } - if (wasChanged) { updatedUserIds = ArrayUtils.appendInt(updatedUserIds, userId); } @@ -2579,6 +2586,13 @@ public class PermissionManagerService extends IPermissionManager.Stub { boolean softRestricted = bp.isSoftRestricted(); for (int userId : currentUserIds) { + // If permission policy is not ready we don't deal with restricted + // permissions as the policy may whitelist some permissions. Once + // the policy is initialized we would re-evaluate permissions. + final boolean permissionPolicyInitialized = + mPermissionPolicyInternal != null + && mPermissionPolicyInternal.isInitialized(userId); + boolean wasChanged = false; boolean restrictionExempt = @@ -2589,7 +2603,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { if (appSupportsRuntimePermissions) { // If hard restricted we don't allow holding it - if (hardRestricted) { + if (permissionPolicyInitialized && hardRestricted) { if (!restrictionExempt) { if (permState != null && permState.isGranted() && permissionsState.revokeRuntimePermission( @@ -2602,7 +2616,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { } } // If soft restricted we allow holding in a restricted form - } else if (softRestricted) { + } else if (permissionPolicyInitialized && softRestricted) { // Regardless if granted set the restriction flag as it // may affect app treatment based on this permission. if (!restrictionExempt && !restrictionApplied) { @@ -2621,7 +2635,8 @@ public class PermissionManagerService extends IPermissionManager.Stub { flags &= ~FLAG_PERMISSION_REVOKE_ON_UPGRADE; wasChanged = true; // Hard restricted permissions cannot be held. - } else if (!hardRestricted || restrictionExempt) { + } else if (!permissionPolicyInitialized || + (!hardRestricted || restrictionExempt)) { if (permissionsState.grantRuntimePermission(bp, userId) != PERMISSION_OPERATION_FAILURE) { wasChanged = true; @@ -2637,22 +2652,25 @@ public class PermissionManagerService extends IPermissionManager.Stub { // If legacy app always grant the permission but if restricted // and not exempt take a note a restriction should be applied. - if ((hardRestricted || softRestricted) - && !restrictionExempt && !restrictionApplied) { + if (permissionPolicyInitialized + && (hardRestricted || softRestricted) + && !restrictionExempt && !restrictionApplied) { flags |= FLAG_PERMISSION_APPLY_RESTRICTION; wasChanged = true; } } // If unrestricted or restriction exempt, don't apply restriction. - if (!(hardRestricted || softRestricted) || restrictionExempt) { - if (restrictionApplied) { - flags &= ~FLAG_PERMISSION_APPLY_RESTRICTION; - // Dropping restriction on a legacy app requires a review. - if (!appSupportsRuntimePermissions) { - flags |= FLAG_PERMISSION_REVIEW_REQUIRED; + if (permissionPolicyInitialized) { + if (!(hardRestricted || softRestricted) || restrictionExempt) { + if (restrictionApplied) { + flags &= ~FLAG_PERMISSION_APPLY_RESTRICTION; + // Dropping restriction on a legacy app implies a review + if (!appSupportsRuntimePermissions) { + flags |= FLAG_PERMISSION_REVIEW_REQUIRED; + } + wasChanged = true; } - wasChanged = true; } } @@ -3959,6 +3977,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { } mPermissionControllerManager = mContext.getSystemService(PermissionControllerManager.class); + mPermissionPolicyInternal = LocalServices.getService(PermissionPolicyInternal.class); int[] grantPermissionsUserIds = EMPTY_INT_ARRAY; for (int userId : UserManagerService.getInstance().getUserIds()) { diff --git a/services/core/java/com/android/server/policy/PermissionPolicyInternal.java b/services/core/java/com/android/server/policy/PermissionPolicyInternal.java index 7760c1edd9e6f..6084c6718577a 100644 --- a/services/core/java/com/android/server/policy/PermissionPolicyInternal.java +++ b/services/core/java/com/android/server/policy/PermissionPolicyInternal.java @@ -18,6 +18,7 @@ package com.android.server.policy; import android.annotation.NonNull; import android.annotation.Nullable; +import android.annotation.UserIdInt; import android.content.Intent; /** @@ -25,6 +26,19 @@ import android.content.Intent; */ public abstract class PermissionPolicyInternal { + /** + * Callback for initializing the permission policy service. + */ + public interface OnInitializedCallback { + + /** + * Called when initialized for the given user. + * + * @param userId The initialized user. + */ + void onInitialized(@UserIdInt int userId); + } + /** * Check whether an activity should be started. * @@ -36,4 +50,17 @@ public abstract class PermissionPolicyInternal { */ public abstract boolean checkStartActivity(@NonNull Intent intent, int callingUid, @Nullable String callingPackage); + + /** + * @return Whether the policy is initialized for a user. + */ + public abstract boolean isInitialized(@UserIdInt int userId); + + /** + * Set a callback for users being initialized. If the user is already + * initialized the callback will not be invoked. + * + * @param callback The callback to register. + */ + public abstract void setOnInitializedCallback(@NonNull OnInitializedCallback callback); } diff --git a/services/core/java/com/android/server/policy/PermissionPolicyService.java b/services/core/java/com/android/server/policy/PermissionPolicyService.java index 8da7f7bb68985..f68a06bb191a6 100644 --- a/services/core/java/com/android/server/policy/PermissionPolicyService.java +++ b/services/core/java/com/android/server/policy/PermissionPolicyService.java @@ -66,6 +66,7 @@ import com.android.server.LocalServices; import com.android.server.SystemService; import com.android.server.pm.permission.PermissionManagerServiceInternal; +import com.android.server.policy.PermissionPolicyInternal.OnInitializedCallback; import java.util.ArrayList; import java.util.concurrent.CountDownLatch; @@ -86,6 +87,10 @@ public final class PermissionPolicyService extends SystemService { @GuardedBy("mLock") private final SparseBooleanArray mIsStarted = new SparseBooleanArray(); + /** Callbacks for when a user is initialized */ + @GuardedBy("mLock") + private OnInitializedCallback mOnInitializedCallback; + /** * Whether an async {@link #synchronizePackagePermissionsAndAppOpsForUser} is currently * scheduled for a package/user. @@ -240,12 +245,20 @@ public final class PermissionPolicyService extends SystemService { grantOrUpgradeDefaultRuntimePermissionsIfNeeded(userId); + final OnInitializedCallback callback; + synchronized (mLock) { mIsStarted.put(userId, true); + callback = mOnInitializedCallback; } // Force synchronization as permissions might have changed synchronizePermissionsAndAppOpsForUser(userId); + + // Tell observers we are initialized for this user. + if (callback != null) { + callback.onInitialized(userId); + } } @Override @@ -809,6 +822,18 @@ public final class PermissionPolicyService extends SystemService { return true; } + @Override + public boolean isInitialized(int userId) { + return isStarted(userId); + } + + @Override + public void setOnInitializedCallback(@NonNull OnInitializedCallback callback) { + synchronized (mLock) { + mOnInitializedCallback = callback; + } + } + /** * Check if the intent action is removed for the calling package (often based on target SDK * version). If the action is removed, we'll silently cancel the activity launch. diff --git a/services/java/com/android/server/SystemServer.java b/services/java/com/android/server/SystemServer.java index 9fe44dc7b2f06..e198c7945bf0e 100644 --- a/services/java/com/android/server/SystemServer.java +++ b/services/java/com/android/server/SystemServer.java @@ -1980,6 +1980,11 @@ public final class SystemServer { } t.traceEnd(); + // Permission policy service + t.traceBegin("StartPermissionPolicyService"); + mSystemServiceManager.startService(PermissionPolicyService.class); + t.traceEnd(); + t.traceBegin("MakePackageManagerServiceReady"); mPackageManagerService.systemReady(); t.traceEnd(); @@ -2014,11 +2019,6 @@ public final class SystemServer { mSystemServiceManager.startBootPhase(t, SystemService.PHASE_DEVICE_SPECIFIC_SERVICES_READY); t.traceEnd(); - // Permission policy service - t.traceBegin("StartPermissionPolicyService"); - mSystemServiceManager.startService(PermissionPolicyService.class); - t.traceEnd(); - // These are needed to propagate to the runnable below. final NetworkManagementService networkManagementF = networkManagement; final NetworkStatsService networkStatsF = networkStats;