From 752ff5a127dd837abc9fcad894c665bd3620a848 Mon Sep 17 00:00:00 2001 From: Zim Date: Fri, 7 Feb 2020 15:06:09 +0000 Subject: [PATCH 1/3] Introduce preserveLegacyExternalStorage manifest attribute This attribute will default to false and does nothing in the false state. In the true state, it will allow an updating app preserve any legacy storage access from the existing installed version on device. Test: atest RestrictedPermissionsTest Test: atest RestrictedStoragePermissionSharedUidTest Bug: 148944140 Change-Id: Id5b6d7aadd9d9844c8bd1164087a9e093481e234 --- api/current.txt | 1 + core/res/res/values/attrs_manifest.xml | 15 +++++++++++++++ core/res/res/values/public.xml | 1 + 3 files changed, 17 insertions(+) diff --git a/api/current.txt b/api/current.txt index 5e8974fb82d70..2a77fce06ddf8 100644 --- a/api/current.txt +++ b/api/current.txt @@ -1082,6 +1082,7 @@ package android { field public static final int preferenceScreenStyle = 16842891; // 0x101008b field public static final int preferenceStyle = 16842894; // 0x101008e field public static final int presentationTheme = 16843712; // 0x10103c0 + field public static final int preserveLegacyExternalStorage = 16844312; // 0x1010618 field public static final int previewImage = 16843482; // 0x10102da field public static final int primaryContentAlpha = 16844114; // 0x1010552 field public static final int priority = 16842780; // 0x101001c diff --git a/core/res/res/values/attrs_manifest.xml b/core/res/res/values/attrs_manifest.xml index e231c3a323fdd..16bed503c5599 100644 --- a/core/res/res/values/attrs_manifest.xml +++ b/core/res/res/values/attrs_manifest.xml @@ -1770,6 +1770,21 @@ --> + + + + From b14fba21d3a07cbea11b67e5df3f0389af8b532d Mon Sep 17 00:00:00 2001 From: Zim Date: Fri, 7 Feb 2020 15:11:40 +0000 Subject: [PATCH 2/3] Support preserveLegacyExternalStorage manifest attribute Parsed the manifest and included in the relevant PackageManager data structures. Test: atest RestrictedPermissionsTest Test: atest RestrictedStoragePermissionSharedUidTest Bug: 148944140 Change-Id: I785e707aa4c879ea163c1611c1693fea74f4ac82 --- .../android/content/pm/parsing/AndroidPackage.java | 2 ++ .../android/content/pm/parsing/ApkParseUtils.java | 4 ++++ .../java/android/content/pm/parsing/PackageImpl.java | 12 ++++++++++++ .../android/content/pm/parsing/ParsingPackage.java | 2 ++ 4 files changed, 20 insertions(+) diff --git a/core/java/android/content/pm/parsing/AndroidPackage.java b/core/java/android/content/pm/parsing/AndroidPackage.java index da17ff3cdefc9..b93c3a245a00a 100644 --- a/core/java/android/content/pm/parsing/AndroidPackage.java +++ b/core/java/android/content/pm/parsing/AndroidPackage.java @@ -397,6 +397,8 @@ public interface AndroidPackage extends Parcelable { boolean hasComponentClassName(String className); + boolean hasPreserveLegacyExternalStorage(); + // App Info boolean hasRequestedLegacyExternalStorage(); diff --git a/core/java/android/content/pm/parsing/ApkParseUtils.java b/core/java/android/content/pm/parsing/ApkParseUtils.java index a2671136ff7b4..905794bb24de1 100644 --- a/core/java/android/content/pm/parsing/ApkParseUtils.java +++ b/core/java/android/content/pm/parsing/ApkParseUtils.java @@ -2101,6 +2101,10 @@ public class ApkParseUtils { parsingPackage.setAllowNativeHeapPointerTagging(sa.getBoolean( R.styleable.AndroidManifestApplication_allowNativeHeapPointerTagging, true)); + parsingPackage.setPreserveLegacyExternalStorage(sa.getBoolean( + R.styleable.AndroidManifestApplication_preserveLegacyExternalStorage, + false)); + parsingPackage .setMaxAspectRatio( sa.getFloat(R.styleable.AndroidManifestApplication_maxAspectRatio, 0)) diff --git a/core/java/android/content/pm/parsing/PackageImpl.java b/core/java/android/content/pm/parsing/PackageImpl.java index 778d7b8b26b6a..180714a751430 100644 --- a/core/java/android/content/pm/parsing/PackageImpl.java +++ b/core/java/android/content/pm/parsing/PackageImpl.java @@ -289,6 +289,7 @@ public final class PackageImpl implements ParsingPackage, ParsedPackage, Android private String[] usesLibraryFiles; private List usesLibraryInfos; private String zygotePreloadName; + private boolean preserveLegacyExternalStorage; @VisibleForTesting public PackageImpl( @@ -1518,6 +1519,12 @@ public final class PackageImpl implements ParsingPackage, ParsedPackage, Android return this; } + @Override + public PackageImpl setPreserveLegacyExternalStorage(boolean preserveLegacyExternalStorage) { + this.preserveLegacyExternalStorage = preserveLegacyExternalStorage; + return this; + } + @Override public PackageImpl setUsesNonSdkApi(boolean usesNonSdkApi) { this.privateFlags = usesNonSdkApi @@ -2999,6 +3006,11 @@ public final class PackageImpl implements ParsingPackage, ParsedPackage, Android return queriesProviders; } + @Override + public boolean hasPreserveLegacyExternalStorage() { + return preserveLegacyExternalStorage; + } + private static void internStringArrayList(List list) { if (list != null) { final int N = list.size(); diff --git a/core/java/android/content/pm/parsing/ParsingPackage.java b/core/java/android/content/pm/parsing/ParsingPackage.java index 954d65c757308..74a2640446f04 100644 --- a/core/java/android/content/pm/parsing/ParsingPackage.java +++ b/core/java/android/content/pm/parsing/ParsingPackage.java @@ -192,6 +192,8 @@ public interface ParsingPackage extends AndroidPackage { ParsingPackage setRequestLegacyExternalStorage(boolean requestLegacyExternalStorage); ParsingPackage setAllowNativeHeapPointerTagging(boolean allowNativeHeapPointerTagging); + + ParsingPackage setPreserveLegacyExternalStorage(boolean preserveLegacyExternalStorage); ParsingPackage setRestoreAnyVersion(boolean restoreAnyVersion); From 3ce38499e47e5a3f3847b2b7d47dca63f22318a6 Mon Sep 17 00:00:00 2001 From: Zim Date: Fri, 7 Feb 2020 15:14:58 +0000 Subject: [PATCH 3/3] Enforce the preserveLegacyExternalStorage manifest attribute In Q, a manifest flag requestLegacyExternalStorage was introduced to grant legacy apps full access to external storage. This flag was sticky across app updates so an app already installed on a users device would typically always have the legacy_storage appop even if it received an update to stop requesting the manifest flag. With the R scoped storage model, we would like to break this stickiness for 2 reasons: 1. Increase the impact of the storage restrictions 2. Ease testing. Because of the stickyness it is difficult for apps targeting R to actually lose their legacy_storage appop Unfortunately, we have to still provide a path for apps who care about migirating their data on external storage from unreachable locations on R devices. The canonical example is an app with 3 versions: (1) Qcannot_migrate, (2) Qcan_migrate, (3) Rcannot_migrate An update from: 1 -> 2 -> 3 will be fine because (2) will migrate the apps data. However, apps can very well update from (1) - (3) thereby losing access to their data in unreachable storage locations on R devices. To facilitate this migration, apps (targeting R) can explicitly request the preserveLegacyExternalStorage manifest flag. This will ensure that if they get updated on a device with a previous version of their app with the legacy_storage appop, they'll keep legacy_storage status. Of course, fresh installs (of target R apps) will not have the legacy_storage appop even if they have requestLegacyExternalStorage and preserveLegacyExternalStorage flags. By default, this new flag will be false, legacy status will *not* be preserved (the legacy_storage appop will *not* be sticky). But apps that care about migrating data can set the flag to true and will preserve whatever legacy status they had on an existing install. Test: atest RestrictedPermissionsTest Test: atest RestrictedStoragePermissionSharedUidTest Bug: 148944140 Change-Id: Ifd3410ed1a60f4c0e8414fce904139b539e13ad8 --- .../os/storage/StorageManagerInternal.java | 6 ++ .../android/server/StorageManagerService.java | 77 ++++++++++++++++++- .../SoftRestrictedPermissionPolicy.java | 17 +++- 3 files changed, 97 insertions(+), 3 deletions(-) diff --git a/core/java/android/os/storage/StorageManagerInternal.java b/core/java/android/os/storage/StorageManagerInternal.java index 5cac5f580af10..e78aef5b7fbbf 100644 --- a/core/java/android/os/storage/StorageManagerInternal.java +++ b/core/java/android/os/storage/StorageManagerInternal.java @@ -118,4 +118,10 @@ public abstract class StorageManagerInternal { * @param userId the userId for which to reset storage */ public abstract void resetUser(int userId); + + /** + * Returns {@code true} if the immediate last installed version of an app with {@code uid} had + * legacy storage, {@code false} otherwise. + */ + public abstract boolean hasLegacyExternalStorage(int uid); } diff --git a/services/core/java/com/android/server/StorageManagerService.java b/services/core/java/com/android/server/StorageManagerService.java index b5b22f19d4262..45e8748620312 100644 --- a/services/core/java/com/android/server/StorageManagerService.java +++ b/services/core/java/com/android/server/StorageManagerService.java @@ -28,6 +28,10 @@ import static android.app.AppOpsManager.OP_MANAGE_EXTERNAL_STORAGE; import static android.app.AppOpsManager.OP_READ_EXTERNAL_STORAGE; import static android.app.AppOpsManager.OP_REQUEST_INSTALL_PACKAGES; import static android.app.AppOpsManager.OP_WRITE_EXTERNAL_STORAGE; +import static android.content.pm.PackageManager.MATCH_ANY_USER; +import static android.content.pm.PackageManager.MATCH_DIRECT_BOOT_AWARE; +import static android.content.pm.PackageManager.MATCH_DIRECT_BOOT_UNAWARE; +import static android.content.pm.PackageManager.MATCH_UNINSTALLED_PACKAGES; import static android.content.pm.PackageManager.PERMISSION_GRANTED; import static android.os.ParcelFileDescriptor.MODE_READ_WRITE; import static android.os.storage.OnObbStateChangeListener.ERROR_ALREADY_MOUNTED; @@ -136,6 +140,7 @@ import android.util.Xml; import com.android.internal.annotations.GuardedBy; import com.android.internal.app.IAppOpsCallback; import com.android.internal.app.IAppOpsService; +import com.android.internal.content.PackageMonitor; import com.android.internal.os.AppFuseMount; import com.android.internal.os.BackgroundThread; import com.android.internal.os.FuseUnavailableMountException; @@ -149,6 +154,7 @@ import com.android.internal.util.HexDump; import com.android.internal.util.IndentingPrintWriter; import com.android.internal.util.Preconditions; import com.android.internal.widget.LockPatternUtils; +import com.android.server.SystemService.TargetUser; import com.android.server.pm.Installer; import com.android.server.storage.AppFuseBridge; import com.android.server.storage.StorageSessionController; @@ -184,6 +190,7 @@ import java.util.Locale; import java.util.Map; import java.util.Map.Entry; import java.util.Objects; +import java.util.Set; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; @@ -270,6 +277,11 @@ class StorageManagerService extends IStorageManager.Stub public void onStopUser(int userHandle) { mStorageManagerService.onStopUser(userHandle); } + + @Override + public void onStartUser(TargetUser user) { + mStorageManagerService.snapshotAndMonitorLegacyStorageAppOp(user.getUserHandle()); + } } private static final boolean DEBUG_EVENTS = false; @@ -578,6 +590,12 @@ class StorageManagerService extends IStorageManager.Stub private final boolean mIsFuseEnabled; + @GuardedBy("mLock") + private final Set mUidsWithLegacyExternalStorage = new ArraySet<>(); + // Not guarded by lock, always used on the ActivityManager thread + private final Map mPackageMonitorsForUser = new ArrayMap<>(); + + class ObbState implements IBinder.DeathRecipient { public ObbState(String rawPath, String canonicalPath, int callingUid, IObbActionListener token, int nonce, String volId) { @@ -1145,6 +1163,10 @@ class StorageManagerService extends IStorageManager.Stub } catch (Exception e) { Slog.wtf(TAG, e); } + PackageMonitor monitor = mPackageMonitorsForUser.remove(userId); + if (monitor != null) { + monitor.unregister(); + } } private boolean supportsBlockCheckpoint() throws RemoteException { @@ -1836,6 +1858,49 @@ class StorageManagerService extends IStorageManager.Stub } } + private void updateLegacyStorageApps(String packageName, int uid, boolean hasLegacy) { + synchronized (mLock) { + if (hasLegacy) { + Slog.v(TAG, "Package " + packageName + " has legacy storage"); + mUidsWithLegacyExternalStorage.add(uid); + } else { + // TODO(b/149391976): Handle shared user id. Check if there's any other + // installed app with legacy external storage before removing + Slog.v(TAG, "Package " + packageName + " does not have legacy storage"); + mUidsWithLegacyExternalStorage.remove(uid); + } + } + } + + private void snapshotAndMonitorLegacyStorageAppOp(UserHandle user) { + int userId = user.getIdentifier(); + + // TODO(b/149391976): Use mIAppOpsService.getPackagesForOps instead of iterating below + // It should improve performance but the AppOps method doesn't return any app here :( + // This operation currently takes about ~20ms on a freshly flashed device + for (ApplicationInfo ai : mPmInternal.getInstalledApplications(MATCH_DIRECT_BOOT_AWARE + | MATCH_DIRECT_BOOT_UNAWARE | MATCH_UNINSTALLED_PACKAGES | MATCH_ANY_USER, + userId, Process.myUid())) { + try { + boolean hasLegacy = mIAppOpsService.checkOperation(OP_LEGACY_STORAGE, ai.uid, + ai.packageName) == MODE_ALLOWED; + updateLegacyStorageApps(ai.packageName, ai.uid, hasLegacy); + } catch (RemoteException e) { + Slog.e(TAG, "Failed to check legacy op for package " + ai.packageName, e); + } + } + + PackageMonitor monitor = new PackageMonitor() { + @Override + public void onPackageRemoved(String packageName, int uid) { + updateLegacyStorageApps(packageName, uid, false); + } + }; + // TODO(b/149391976): Use different handler? + monitor.register(mContext, user, true, mHandler); + mPackageMonitorsForUser.put(userId, monitor); + } + private static long getLastAccessTime(AppOpsManager manager, int uid, String packageName, int[] ops) { long maxTime = 0; @@ -4337,6 +4402,13 @@ class StorageManagerService extends IStorageManager.Stub mHandler.obtainMessage(H_RESET).sendToTarget(); } + @Override + public boolean hasLegacyExternalStorage(int uid) { + synchronized (mLock) { + return mUidsWithLegacyExternalStorage.contains(uid); + } + } + public boolean hasExternalStorage(int uid, String packageName) { // No need to check for system uid. This avoids a deadlock between // PackageManagerService and AppOpsService. @@ -4382,8 +4454,11 @@ class StorageManagerService extends IStorageManager.Stub // volumes, USB OTGs that are rarely mounted. The app will get the // external_storage gid on next organic restart. killAppForOpChange(code, uid, packageName); - return; } + return; + case OP_LEGACY_STORAGE: + updateLegacyStorageApps(packageName, uid, mode == MODE_ALLOWED); + return; } } diff --git a/services/core/java/com/android/server/policy/SoftRestrictedPermissionPolicy.java b/services/core/java/com/android/server/policy/SoftRestrictedPermissionPolicy.java index a83c58d9c5f47..740472e6b2111 100644 --- a/services/core/java/com/android/server/policy/SoftRestrictedPermissionPolicy.java +++ b/services/core/java/com/android/server/policy/SoftRestrictedPermissionPolicy.java @@ -35,14 +35,17 @@ import android.compat.annotation.EnabledAfter; import android.content.Context; import android.content.pm.ApplicationInfo; import android.content.pm.PackageManager; +import android.content.pm.PackageManagerInternal; import android.os.Binder; import android.os.IBinder; import android.os.RemoteException; import android.os.ServiceManager; import android.os.UserHandle; +import android.os.storage.StorageManagerInternal; import android.util.Log; import com.android.internal.compat.IPlatformCompat; +import com.android.server.LocalServices; /** * The behavior of soft restricted permissions is different for each permission. This class collects @@ -107,11 +110,16 @@ public abstract class SoftRestrictedPermissionPolicy { final boolean isWhiteListed; boolean shouldApplyRestriction; final boolean hasRequestedLegacyExternalStorage; + final boolean shouldPreserveLegacyExternalStorage; final boolean hasWriteMediaStorageGrantedForUid; final boolean isScopedStorageEnabled; if (appInfo != null) { PackageManager pm = context.getPackageManager(); + PackageManagerInternal pmInternal = + LocalServices.getService(PackageManagerInternal.class); + StorageManagerInternal smInternal = + LocalServices.getService(StorageManagerInternal.class); int flags = pm.getPermissionFlags(permission, appInfo.packageName, user); isWhiteListed = (flags & FLAGS_PERMISSION_RESTRICTION_ANY_EXEMPT) != 0; hasRequestedLegacyExternalStorage = hasUidRequestedLegacyExternalStorage( @@ -123,12 +131,16 @@ public abstract class SoftRestrictedPermissionPolicy { isScopedStorageEnabled = isChangeEnabledForUid(context, appInfo, user, ENABLE_SCOPED_STORAGE) || isScopedStorageRequired; + shouldPreserveLegacyExternalStorage = pmInternal.getPackage( + appInfo.packageName).hasPreserveLegacyExternalStorage() + && smInternal.hasLegacyExternalStorage(appInfo.uid); shouldApplyRestriction = (flags & FLAG_PERMISSION_APPLY_RESTRICTION) != 0 - || isScopedStorageRequired; + || (isScopedStorageRequired && !shouldPreserveLegacyExternalStorage); } else { isWhiteListed = false; shouldApplyRestriction = false; hasRequestedLegacyExternalStorage = false; + shouldPreserveLegacyExternalStorage = false; hasWriteMediaStorageGrantedForUid = false; isScopedStorageEnabled = false; } @@ -150,7 +162,8 @@ public abstract class SoftRestrictedPermissionPolicy { public boolean mayAllowExtraAppOp() { return !shouldApplyRestriction && (hasRequestedLegacyExternalStorage - || hasWriteMediaStorageGrantedForUid); + || hasWriteMediaStorageGrantedForUid + || shouldPreserveLegacyExternalStorage); } @Override public boolean mayDenyExtraAppOpIfGranted() {