From 496ac000f680d5a7c216d293ae69ffc9a01176e5 Mon Sep 17 00:00:00 2001 From: Martijn Coenen Date: Wed, 8 Jan 2020 14:55:53 +0100 Subject: [PATCH] Only allow Download authority/MTP/installers to write in Android/. Previously when FUSE was enabled, we gave all apps requesting WRITE_MEDIA_STORAGE or install permissions a direct view to the lower filesystem. This was way too broad for a few reasons: 1) WRITE_MEDIA_STORAGE will be deprecated; holding that permission by itself shouldn't grant you any special privileges. 2) Installers should only be able to write OBBs The only other exceptions that are allowed to bypass scoped storage are the process hosting the DownloadProvider and the process implementing the MTP server; both of these have legit reasons for writing in Android/. The way this is currently implemented is by giving these apps the SDCARD_RW gid, which has write access in the default Android/ sdcardfs view. Installers will be further scoped down to be only able to access OBB in a follow-up CL. Bug: 134706060 Bug: 146490513 Test: DownloadProvider can download Play + OBBs work Writing in Android/ through MTP works Change-Id: Iff8681732d0c1124e24e5347f7dcb64b781c1e8c --- core/java/android/os/Process.java | 6 +++++ core/java/android/os/ZygoteProcess.java | 2 ++ core/java/com/android/internal/os/Zygote.java | 5 ++++ .../android/internal/os/ZygoteArguments.java | 2 ++ core/jni/com_android_internal_os_Zygote.cpp | 12 ++++----- .../android/server/StorageManagerService.java | 26 +++++++++++++++++++ .../com/android/server/am/ProcessList.java | 15 ++++++++--- 7 files changed, 57 insertions(+), 11 deletions(-) diff --git a/core/java/android/os/Process.java b/core/java/android/os/Process.java index 94623bc71346d..3ef86ed9f994e 100644 --- a/core/java/android/os/Process.java +++ b/core/java/android/os/Process.java @@ -88,6 +88,12 @@ public class Process { @UnsupportedAppUsage public static final int DRM_UID = 1019; + /** + * Defines the GID for the group that allows write access to the internal media storage. + * @hide + */ + public static final int SDCARD_RW_GID = 1015; + /** * Defines the UID/GID for the group that controls VPN services. * @hide diff --git a/core/java/android/os/ZygoteProcess.java b/core/java/android/os/ZygoteProcess.java index 62b8953b158ac..3846f894c4c30 100644 --- a/core/java/android/os/ZygoteProcess.java +++ b/core/java/android/os/ZygoteProcess.java @@ -594,6 +594,8 @@ public class ZygoteProcess { argsForZygote.add("--mount-external-legacy"); } else if (mountExternal == Zygote.MOUNT_EXTERNAL_PASS_THROUGH) { argsForZygote.add("--mount-external-pass-through"); + } else if (mountExternal == Zygote.MOUNT_EXTERNAL_ANDROID_WRITABLE) { + argsForZygote.add("--mount-external-android-writable"); } argsForZygote.add("--target-sdk-version=" + targetSdkVersion); diff --git a/core/java/com/android/internal/os/Zygote.java b/core/java/com/android/internal/os/Zygote.java index 2248b8853f8ca..f0a346ab25fdc 100644 --- a/core/java/com/android/internal/os/Zygote.java +++ b/core/java/com/android/internal/os/Zygote.java @@ -145,6 +145,11 @@ public final class Zygote { /** The lower file system should be bind mounted directly on external storage */ public static final int MOUNT_EXTERNAL_PASS_THROUGH = IVold.REMOUNT_MODE_PASS_THROUGH; + /** Use the regular scoped storage filesystem, but Android/ should be writable. + * Used to support the applications hosting DownloadManager and the MTP server. + */ + public static final int MOUNT_EXTERNAL_ANDROID_WRITABLE = IVold.REMOUNT_MODE_ANDROID_WRITABLE; + /** Number of bytes sent to the Zygote over USAP pipes or the pool event FD */ static final int USAP_MANAGEMENT_MESSAGE_BYTES = 8; diff --git a/core/java/com/android/internal/os/ZygoteArguments.java b/core/java/com/android/internal/os/ZygoteArguments.java index d3499541a3a31..37f570bba2385 100644 --- a/core/java/com/android/internal/os/ZygoteArguments.java +++ b/core/java/com/android/internal/os/ZygoteArguments.java @@ -376,6 +376,8 @@ class ZygoteArguments { mMountExternal = Zygote.MOUNT_EXTERNAL_LEGACY; } else if (arg.equals("--mount-external-pass-through")) { mMountExternal = Zygote.MOUNT_EXTERNAL_PASS_THROUGH; + } else if (arg.equals("--mount-external-android-writable")) { + mMountExternal = Zygote.MOUNT_EXTERNAL_ANDROID_WRITABLE; } else if (arg.equals("--query-abi-list")) { mAbiListQuery = true; } else if (arg.equals("--get-pid")) { diff --git a/core/jni/com_android_internal_os_Zygote.cpp b/core/jni/com_android_internal_os_Zygote.cpp index df5b02c22a2da..7e212e119786e 100644 --- a/core/jni/com_android_internal_os_Zygote.cpp +++ b/core/jni/com_android_internal_os_Zygote.cpp @@ -319,7 +319,8 @@ enum MountExternalKind { MOUNT_EXTERNAL_INSTALLER = 5, MOUNT_EXTERNAL_FULL = 6, MOUNT_EXTERNAL_PASS_THROUGH = 7, - MOUNT_EXTERNAL_COUNT = 8 + MOUNT_EXTERNAL_ANDROID_WRITABLE = 8, + MOUNT_EXTERNAL_COUNT = 9 }; // The order of entries here must be kept in sync with MountExternalKind enum values. @@ -331,6 +332,8 @@ static const std::array ExternalStorage "/mnt/runtime/write", // MOUNT_EXTERNAL_LEGACY "/mnt/runtime/write", // MOUNT_EXTERNAL_INSTALLER "/mnt/runtime/full", // MOUNT_EXTERNAL_FULL + "/mnt/runtime/full", // MOUNT_EXTERNAL_PASS_THROUGH (only used w/ FUSE) + "/mnt/runtime/full", // MOUNT_EXTERNAL_ANDROID_WRITABLE (only used w/ FUSE) }; // Must match values in com.android.internal.os.Zygote. @@ -749,12 +752,7 @@ static void MountEmulatedStorage(uid_t uid, jint mount_mode, PrepareDir(user_source, DEFAULT_DATA_DIR_PERMISSION, AID_ROOT, AID_ROOT, fail_fn); if (isFuse) { - if (mount_mode == MOUNT_EXTERNAL_PASS_THROUGH || mount_mode == - MOUNT_EXTERNAL_INSTALLER || mount_mode == MOUNT_EXTERNAL_FULL) { - // For now, MediaProvider, installers and "full" get the pass_through mount - // view, which is currently identical to the sdcardfs write view. - // - // TODO(b/146189163): scope down MOUNT_EXTERNAL_INSTALLER + if (mount_mode == MOUNT_EXTERNAL_PASS_THROUGH) { BindMount(pass_through_source, "/storage", fail_fn); } else { BindMount(user_source, "/storage", fail_fn); diff --git a/services/core/java/com/android/server/StorageManagerService.java b/services/core/java/com/android/server/StorageManagerService.java index 3d455ee1cf19b..db542145a750d 100644 --- a/services/core/java/com/android/server/StorageManagerService.java +++ b/services/core/java/com/android/server/StorageManagerService.java @@ -16,6 +16,7 @@ package com.android.server; +import static android.Manifest.permission.ACCESS_MTP; import static android.Manifest.permission.INSTALL_PACKAGES; import static android.Manifest.permission.READ_EXTERNAL_STORAGE; import static android.Manifest.permission.WRITE_EXTERNAL_STORAGE; @@ -111,6 +112,7 @@ import android.os.storage.StorageVolume; import android.os.storage.VolumeInfo; import android.os.storage.VolumeRecord; import android.provider.DeviceConfig; +import android.provider.Downloads; import android.provider.MediaStore; import android.provider.Settings; import android.sysprop.VoldProperties; @@ -367,6 +369,8 @@ class StorageManagerService extends IStorageManager.Stub private volatile int mMediaStoreAuthorityAppId = -1; + private volatile int mDownloadsAuthorityAppId = -1; + private volatile int mCurrentUserId = UserHandle.USER_SYSTEM; private final Installer mInstaller; @@ -1788,6 +1792,15 @@ class StorageManagerService extends IStorageManager.Stub mMediaStoreAuthorityAppId = UserHandle.getAppId(provider.applicationInfo.uid); } + provider = mPmInternal.resolveContentProvider( + Downloads.Impl.AUTHORITY, PackageManager.MATCH_DIRECT_BOOT_AWARE + | PackageManager.MATCH_DIRECT_BOOT_UNAWARE, + UserHandle.getUserId(UserHandle.USER_SYSTEM)); + + if (provider != null) { + mDownloadsAuthorityAppId = UserHandle.getAppId(provider.applicationInfo.uid); + } + try { mIAppOpsService.startWatchingMode(OP_REQUEST_INSTALL_PACKAGES, null, mAppOpsCallback); mIAppOpsService.startWatchingMode(OP_LEGACY_STORAGE, null, mAppOpsCallback); @@ -3881,6 +3894,19 @@ class StorageManagerService extends IStorageManager.Stub return Zygote.MOUNT_EXTERNAL_PASS_THROUGH; } + if (mIsFuseEnabled && mDownloadsAuthorityAppId == UserHandle.getAppId(uid)) { + // DownloadManager can write in app-private directories on behalf of apps; + // give it write access to Android/ + return Zygote.MOUNT_EXTERNAL_ANDROID_WRITABLE; + } + + final boolean hasMtp = mIPackageManager.checkUidPermission(ACCESS_MTP, uid) == + PERMISSION_GRANTED; + if (mIsFuseEnabled && hasMtp) { + // The process hosting the MTP server should be able to write in Android/ + return Zygote.MOUNT_EXTERNAL_ANDROID_WRITABLE; + } + // Determine if caller is holding runtime permission final boolean hasRead = StorageManager.checkPermissionAndCheckOp(mContext, false, 0, uid, packageName, READ_EXTERNAL_STORAGE, OP_READ_EXTERNAL_STORAGE); diff --git a/services/core/java/com/android/server/am/ProcessList.java b/services/core/java/com/android/server/am/ProcessList.java index a1e1f29016c2e..76f54cdf91be2 100644 --- a/services/core/java/com/android/server/am/ProcessList.java +++ b/services/core/java/com/android/server/am/ProcessList.java @@ -1555,20 +1555,27 @@ public final class ProcessList { } catch (RemoteException e) { throw e.rethrowAsRuntimeException(); } - + int numGids = 3; + if (mountExternal == Zygote.MOUNT_EXTERNAL_INSTALLER + || mountExternal == Zygote.MOUNT_EXTERNAL_ANDROID_WRITABLE) { + numGids++; + } /* * Add shared application and profile GIDs so applications can share some * resources like shared libraries and access user-wide resources */ if (ArrayUtils.isEmpty(permGids)) { - gids = new int[3]; + gids = new int[numGids]; } else { - gids = new int[permGids.length + 3]; - System.arraycopy(permGids, 0, gids, 3, permGids.length); + gids = new int[permGids.length + numGids]; + System.arraycopy(permGids, 0, gids, numGids, permGids.length); } gids[0] = UserHandle.getSharedAppGid(UserHandle.getAppId(uid)); gids[1] = UserHandle.getCacheAppGid(UserHandle.getAppId(uid)); gids[2] = UserHandle.getUserGid(UserHandle.getUserId(uid)); + if (numGids > 3) { + gids[3] = Process.SDCARD_RW_GID; + } // Replace any invalid GIDs if (gids[0] == UserHandle.ERR_GID) gids[0] = gids[2];