From 0a35f11ba7a449712cd293e5075ec92a142bd6af Mon Sep 17 00:00:00 2001 From: Jackal Guo Date: Wed, 18 Aug 2021 14:31:35 +0800 Subject: [PATCH] Fix visibility was gone after updating System grants implicit visibility when the Uri permission is granted. But, all the visibility rules would be cleared when package updating. Since the implict visibility doesn't come from manifest, we cannot restore it via recompuating visibility. This solution is similar to how UriGrantsManagerService handles the granted Uri permissions when packagea updated. We keep them unchanged instead of clearing them. Bug: 161912313 Test: atest AppsFilterTest Test: atest AppEnumerationTests Test: atest UriGrantsManagerServiceTest Test: manually use test APKs on buganizer to verify Change-Id: I3026e06574f78c15dbf60c74db126679c677adb1 --- .../content/pm/PackageManagerInternal.java | 29 ++++++++++ .../com/android/server/pm/AppsFilter.java | 54 +++++++++++++++++-- .../server/pm/PackageManagerService.java | 15 ++++-- .../server/uri/UriGrantsManagerService.java | 11 ++-- .../com/android/server/pm/AppsFilterTest.java | 10 ++-- .../uri/UriGrantsManagerServiceTest.java | 4 +- 6 files changed, 104 insertions(+), 19 deletions(-) diff --git a/services/core/java/android/content/pm/PackageManagerInternal.java b/services/core/java/android/content/pm/PackageManagerInternal.java index e390ae28a0924..6529b11043323 100644 --- a/services/core/java/android/content/pm/PackageManagerInternal.java +++ b/services/core/java/android/content/pm/PackageManagerInternal.java @@ -479,6 +479,35 @@ public abstract class PackageManagerInternal implements PackageSettingsSnapshotP @AppIdInt int recipientAppId, int visibleUid, boolean direct); + /** + * Grants implicit access based on an interaction between two apps. This grants access to the + * from one application to the other's package metadata. + *

+ * When an application explicitly tries to interact with another application [via an + * activity, service or provider that is either declared in the caller's + * manifest via the {@code } tag or has been exposed via the target apps manifest using + * the {@code visibleToInstantApp} attribute], the target application must be able to see + * metadata about the calling app. If the calling application uses an implicit intent [ie + * action VIEW, category BROWSABLE], it remains hidden from the launched app. + *

+ * If an interaction is not explicit, the {@code direct} argument should be set to false as + * visibility should not be granted in some cases. This method handles that logic. + *

+ * @param userId the user + * @param intent the intent that triggered the grant + * @param recipientAppId The app ID of the application that is being given access to {@code + * visibleUid} + * @param visibleUid The uid of the application that is becoming accessible to {@code + * recipientAppId} + * @param direct true if the access is being made due to direct interaction between visibleUid + * and recipientAppId. + * @param retainOnUpdate true if the implicit access is retained across package update. + */ + public abstract void grantImplicitAccess( + @UserIdInt int userId, Intent intent, + @AppIdInt int recipientAppId, int visibleUid, + boolean direct, boolean retainOnUpdate); + public abstract boolean isInstantAppInstallerComponent(ComponentName component); /** * Prunes instant apps and state associated with uninstalled diff --git a/services/core/java/com/android/server/pm/AppsFilter.java b/services/core/java/com/android/server/pm/AppsFilter.java index 56b77b5944434..15e671cd7d035 100644 --- a/services/core/java/com/android/server/pm/AppsFilter.java +++ b/services/core/java/com/android/server/pm/AppsFilter.java @@ -96,6 +96,13 @@ public class AppsFilter implements Watchable, Snappable { */ private final SparseSetArray mImplicitlyQueryable = new SparseSetArray<>(); + /** + * This contains a list of app UIDs that are implicitly queryable because another app explicitly + * interacted with it, but could keep across package updates. For example, if application A + * grants persistable uri permission to application B; regardless of any manifest entries. + */ + private final SparseSetArray mRetainedImplicitlyQueryable = new SparseSetArray<>(); + /** * A mapping from the set of App IDs that query other App IDs via package name to the * list of packages that they can see. @@ -256,6 +263,7 @@ public class AppsFilter implements Watchable, Snappable { */ private AppsFilter(AppsFilter orig) { Snapshots.copy(mImplicitlyQueryable, orig.mImplicitlyQueryable); + Snapshots.copy(mRetainedImplicitlyQueryable, orig.mRetainedImplicitlyQueryable); Snapshots.copy(mQueriesViaPackage, orig.mQueriesViaPackage); Snapshots.copy(mQueriesViaComponent, orig.mQueriesViaComponent); Snapshots.copy(mQueryableViaUsesLibrary, orig.mQueryableViaUsesLibrary); @@ -633,15 +641,19 @@ public class AppsFilter implements Watchable, Snappable { * * @param recipientUid the uid gaining visibility of the {@code visibleUid}. * @param visibleUid the uid becoming visible to the {@recipientUid} + * @param retainOnUpdate if the implicit access retained across package updates. * @return {@code true} if implicit access was not already granted. */ - public boolean grantImplicitAccess(int recipientUid, int visibleUid) { + public boolean grantImplicitAccess(int recipientUid, int visibleUid, boolean retainOnUpdate) { if (recipientUid == visibleUid) { return false; } - final boolean changed = mImplicitlyQueryable.add(recipientUid, visibleUid); + final boolean changed = retainOnUpdate + ? mRetainedImplicitlyQueryable.add(recipientUid, visibleUid) + : mImplicitlyQueryable.add(recipientUid, visibleUid); if (changed && DEBUG_LOGGING) { - Slog.i(TAG, "implicit access granted: " + recipientUid + " -> " + visibleUid); + Slog.i(TAG, (retainOnUpdate ? "retained " : "") + "implicit access granted: " + + recipientUid + " -> " + visibleUid); } synchronized (mCacheLock) { if (mShouldFilterCache != null) { @@ -677,7 +689,7 @@ public class AppsFilter implements Watchable, Snappable { try { if (isReplace) { // let's first remove any prior rules for this package - removePackage(newPkgSetting); + removePackage(newPkgSetting, true /*isReplace*/); } mStateProvider.runWithState((settings, users) -> { ArraySet additionalChangedPackages = @@ -1078,8 +1090,9 @@ public class AppsFilter implements Watchable, Snappable { * Removes a package for consideration when filtering visibility between apps. * * @param setting the setting of the package being removed. + * @param isReplace if the package is being replaced. */ - public void removePackage(PackageSetting setting) { + public void removePackage(PackageSetting setting, boolean isReplace) { mStateProvider.runWithState((settings, users) -> { final int userCount = users.length; for (int u = 0; u < userCount; u++) { @@ -1089,6 +1102,16 @@ public class AppsFilter implements Watchable, Snappable { for (int i = mImplicitlyQueryable.size() - 1; i >= 0; i--) { mImplicitlyQueryable.remove(mImplicitlyQueryable.keyAt(i), removingUid); } + + if (isReplace) { + continue; + } + + mRetainedImplicitlyQueryable.remove(removingUid); + for (int i = mRetainedImplicitlyQueryable.size() - 1; i >= 0; i--) { + mRetainedImplicitlyQueryable.remove( + mRetainedImplicitlyQueryable.keyAt(i), removingUid); + } } if (!mQueriesViaComponentRequireRecompute) { @@ -1412,6 +1435,24 @@ public class AppsFilter implements Watchable, Snappable { } } + try { + if (DEBUG_TRACING) { + Trace.traceBegin(TRACE_TAG_PACKAGE_MANAGER, "mRetainedImplicitlyQueryable"); + } + final int targetUid = UserHandle.getUid(targetUserId, targetAppId); + if (mRetainedImplicitlyQueryable.contains(callingUid, targetUid)) { + if (DEBUG_LOGGING) { + log(callingSetting, targetPkgSetting, + "retained implicitly queryable for user"); + } + return false; + } + } finally { + if (DEBUG_TRACING) { + Trace.traceEnd(TRACE_TAG_PACKAGE_MANAGER); + } + } + try { if (DEBUG_TRACING) { Trace.traceBegin(TRACE_TAG_PACKAGE_MANAGER, "mOverlayReferenceMapper"); @@ -1550,6 +1591,9 @@ public class AppsFilter implements Watchable, Snappable { dumpQueriesMap(pw, filteringAppId == null ? null : UserHandle.getUid(user, filteringAppId), mImplicitlyQueryable, " ", expandPackages); + dumpQueriesMap(pw, + filteringAppId == null ? null : UserHandle.getUid(user, filteringAppId), + mRetainedImplicitlyQueryable, " ", expandPackages); } pw.println(" queryable via uses-library:"); dumpQueriesMap(pw, filteringAppId, mQueryableViaUsesLibrary, " ", expandPackages); diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index d16bfdb4cb689..c54ef27e84b0c 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -18050,7 +18050,8 @@ public class PackageManagerService extends IPackageManager.Stub synchronized (mLock) { mDomainVerificationManager.clearPackage(deletedPs.name); mSettings.getKeySetManagerService().removeAppKeySetDataLPw(packageName); - mAppsFilter.removePackage(getPackageSetting(packageName)); + mAppsFilter.removePackage(getPackageSetting(packageName), + false /* isReplace */); removedAppId = mSettings.removePackageLPw(packageName); if (outInfo != null) { outInfo.mRemovedAppId = removedAppId; @@ -23668,6 +23669,13 @@ public class PackageManagerService extends IPackageManager.Stub @Override public void grantImplicitAccess(int userId, Intent intent, int recipientAppId, int visibleUid, boolean direct) { + grantImplicitAccess(userId, intent, recipientAppId, visibleUid, direct, + false /* retainOnUpdate */); + } + + @Override + public void grantImplicitAccess(int userId, Intent intent, + int recipientAppId, int visibleUid, boolean direct, boolean retainOnUpdate) { synchronized (mLock) { final AndroidPackage visiblePackage = getPackage(visibleUid); final int recipientUid = UserHandle.getUid(userId, recipientAppId); @@ -23688,7 +23696,8 @@ public class PackageManagerService extends IPackageManager.Stub accessGranted = mInstantAppRegistry.grantInstantAccessLPw(userId, intent, recipientAppId, UserHandle.getAppId(visibleUid) /*instantAppId*/); } else { - accessGranted = mAppsFilter.grantImplicitAccess(recipientUid, visibleUid); + accessGranted = mAppsFilter.grantImplicitAccess(recipientUid, visibleUid, + retainOnUpdate); } if (accessGranted) { ApplicationPackageManager.invalidateGetPackagesForUidCache(); @@ -24628,7 +24637,7 @@ public class PackageManagerService extends IPackageManager.Stub } int visibleUid = providerInfo.applicationInfo.uid; mPmInternal.grantImplicitAccess(userId, null /*Intent*/, UserHandle.getAppId(recipientUid), - visibleUid, false); + visibleUid, false /*direct*/); } boolean canHaveOatDir(String packageName) { diff --git a/services/core/java/com/android/server/uri/UriGrantsManagerService.java b/services/core/java/com/android/server/uri/UriGrantsManagerService.java index fd1995ddec584..b17257a9db805 100644 --- a/services/core/java/com/android/server/uri/UriGrantsManagerService.java +++ b/services/core/java/com/android/server/uri/UriGrantsManagerService.java @@ -706,10 +706,10 @@ public class UriGrantsManagerService extends IUriGrantsManager.Stub { sourcePkg, targetPkg, targetUid, grantUri); perm.initPersistedModes(modeFlags, createdTime); mPmInternal.grantImplicitAccess( - targetUserId, null, + targetUserId, null /* intent */, UserHandle.getAppId(targetUid), pi.applicationInfo.uid, - false /* direct */); + false /* direct */, true /* retainOnUpdate */); } } else { Slog.w(TAG, "Persisted grant for " + uri + " had source " + sourcePkg @@ -773,8 +773,9 @@ public class UriGrantsManagerService extends IUriGrantsManager.Stub { perm = findOrCreateUriPermissionLocked(pi.packageName, targetPkg, targetUid, grantUri); } perm.grantModes(modeFlags, owner); - mPmInternal.grantImplicitAccess(UserHandle.getUserId(targetUid), null, - UserHandle.getAppId(targetUid), pi.applicationInfo.uid, false /*direct*/); + mPmInternal.grantImplicitAccess(UserHandle.getUserId(targetUid), null /*intent*/, + UserHandle.getAppId(targetUid), pi.applicationInfo.uid, false /*direct*/, + (modeFlags & Intent.FLAG_GRANT_PERSISTABLE_URI_PERMISSION) != 0); } /** Like grantUriPermissionUnchecked, but takes an Intent. */ @@ -1187,7 +1188,7 @@ public class UriGrantsManagerService extends IUriGrantsManager.Stub { // provider supports granting permissions mPmInternal.grantImplicitAccess( UserHandle.getUserId(targetUid), null, - UserHandle.getAppId(targetUid), pi.applicationInfo.uid, false); + UserHandle.getAppId(targetUid), pi.applicationInfo.uid, false /*direct*/); return -1; } diff --git a/services/tests/servicestests/src/com/android/server/pm/AppsFilterTest.java b/services/tests/servicestests/src/com/android/server/pm/AppsFilterTest.java index be942d6738af6..9bc20a7ba4e61 100644 --- a/services/tests/servicestests/src/com/android/server/pm/AppsFilterTest.java +++ b/services/tests/servicestests/src/com/android/server/pm/AppsFilterTest.java @@ -829,7 +829,7 @@ public class AppsFilterTest { assertTrue(appsFilter.shouldFilterApplication(DUMMY_OVERLAY_APPID, overlaySetting, actorSetting, SYSTEM_USER)); - appsFilter.removePackage(targetSetting); + appsFilter.removePackage(targetSetting, false /* isReplace */); // Actor loses visibility to the overlay via removal of the target assertTrue(appsFilter.shouldFilterApplication(DUMMY_ACTOR_APPID, actorSetting, @@ -1074,7 +1074,8 @@ public class AppsFilterTest { watcher.verifyNoChangeReported("getVisibility"); // provider read - appsFilter.grantImplicitAccess(hasProviderAppId, queriesProviderAppId); + appsFilter.grantImplicitAccess(hasProviderAppId, queriesProviderAppId, + false /* retainOnUpdate */); watcher.verifyChangeReported("grantImplicitAccess"); // ensure implicit access is included in the filter @@ -1143,7 +1144,8 @@ public class AppsFilterTest { watcher.verifyNoChangeReported("get"); // provider read - appsFilter.grantImplicitAccess(hasProviderAppId, queriesProviderAppId); + appsFilter.grantImplicitAccess( + hasProviderAppId, queriesProviderAppId, false /* retainOnUpdate */); watcher.verifyChangeReported("grantImplicitAccess"); // ensure implicit access is included in the filter @@ -1154,7 +1156,7 @@ public class AppsFilterTest { watcher.verifyNoChangeReported("get"); // remove a package - appsFilter.removePackage(seesNothing); + appsFilter.removePackage(seesNothing, false /* isReplace */); watcher.verifyChangeReported("removePackage"); } diff --git a/services/tests/servicestests/src/com/android/server/uri/UriGrantsManagerServiceTest.java b/services/tests/servicestests/src/com/android/server/uri/UriGrantsManagerServiceTest.java index 25b51dad460a8..e418d2f8d3280 100644 --- a/services/tests/servicestests/src/com/android/server/uri/UriGrantsManagerServiceTest.java +++ b/services/tests/servicestests/src/com/android/server/uri/UriGrantsManagerServiceTest.java @@ -73,8 +73,8 @@ public class UriGrantsManagerServiceTest { // we expect the following only during grant if a grant is expected private void verifyNoVisibilityGrant() { - verify(mContext.mPmInternal, never()) - .grantImplicitAccess(anyInt(), any(), anyInt(), anyInt(), anyBoolean()); + verify(mContext.mPmInternal, never()).grantImplicitAccess( + anyInt(), any(), anyInt(), anyInt(), anyBoolean(), anyBoolean()); } @Before