From 09f86191d6968ddc8c1f4a150bfee0f423b2f9d6 Mon Sep 17 00:00:00 2001 From: bkchoi Date: Mon, 11 Dec 2023 22:46:41 +0000 Subject: Check the calling user instead of the current user. In concurrent multi-user case, the calling user may be different from the current user, thus we cannot assume that it is always the current user to check if the calling user is an admin user. Instead, we use the calling user id from the binder, before the caller identity is cleared. Bug: 315382159 Test: atest android.bugreport.cts.BugreportManagerTest Change-Id: I003037d37b72c47dc4be7c85f634116f9bc4ca63 --- .../server/os/BugreportManagerServiceImpl.java | 71 ++++++++++--------- .../server/os/BugreportManagerServiceImplTest.java | 79 ++++++++++++++++++++-- 2 files changed, 107 insertions(+), 43 deletions(-) diff --git a/services/core/java/com/android/server/os/BugreportManagerServiceImpl.java b/services/core/java/com/android/server/os/BugreportManagerServiceImpl.java index 1660c3ef952a..e546f42c9c22 100644 --- a/services/core/java/com/android/server/os/BugreportManagerServiceImpl.java +++ b/services/core/java/com/android/server/os/BugreportManagerServiceImpl.java @@ -21,13 +21,11 @@ import static android.app.admin.flags.Flags.onboardingBugreportV2Enabled; import android.Manifest; import android.annotation.Nullable; import android.annotation.RequiresPermission; -import android.app.ActivityManager; import android.app.AppOpsManager; import android.app.admin.DevicePolicyManager; import android.app.role.RoleManager; import android.content.Context; import android.content.pm.PackageManager; -import android.content.pm.UserInfo; import android.os.Binder; import android.os.BugreportManager.BugreportCallback; import android.os.BugreportParams; @@ -39,6 +37,7 @@ import android.os.ServiceManager; import android.os.SystemClock; import android.os.SystemProperties; import android.os.UserHandle; +import android.os.UserManager; import android.telephony.TelephonyManager; import android.text.TextUtils; import android.util.ArrayMap; @@ -96,6 +95,7 @@ class BugreportManagerServiceImpl extends IDumpstate.Stub { private static final long DEFAULT_BUGREPORT_SERVICE_TIMEOUT_MILLIS = 30 * 1000; private final Object mLock = new Object(); + private final Injector mInjector; private final Context mContext; private final AppOpsManager mAppOps; private final TelephonyManager mTelephonyManager; @@ -346,6 +346,14 @@ class BugreportManagerServiceImpl extends IDumpstate.Stub { AtomicFile getMappingFile() { return mMappingFile; } + + UserManager getUserManager() { + return mContext.getSystemService(UserManager.class); + } + + DevicePolicyManager getDevicePolicyManager() { + return mContext.getSystemService(DevicePolicyManager.class); + } } BugreportManagerServiceImpl(Context context) { @@ -357,6 +365,7 @@ class BugreportManagerServiceImpl extends IDumpstate.Stub { @VisibleForTesting(visibility = VisibleForTesting.Visibility.PRIVATE) BugreportManagerServiceImpl(Injector injector) { + mInjector = injector; mContext = injector.getContext(); mAppOps = mContext.getSystemService(AppOpsManager.class); mTelephonyManager = mContext.getSystemService(TelephonyManager.class); @@ -389,12 +398,7 @@ class BugreportManagerServiceImpl extends IDumpstate.Stub { int callingUid = Binder.getCallingUid(); enforcePermission(callingPackage, callingUid, bugreportMode == BugreportParams.BUGREPORT_MODE_TELEPHONY /* checkCarrierPrivileges */); - final long identity = Binder.clearCallingIdentity(); - try { - ensureUserCanTakeBugReport(bugreportMode); - } finally { - Binder.restoreCallingIdentity(identity); - } + ensureUserCanTakeBugReport(bugreportMode); Slogf.i(TAG, "Starting bugreport for %s / %d", callingPackage, callingUid); synchronized (mLock) { @@ -433,7 +437,6 @@ class BugreportManagerServiceImpl extends IDumpstate.Stub { @RequiresPermission(value = Manifest.permission.DUMP, conditional = true) public void retrieveBugreport(int callingUidUnused, String callingPackage, int userId, FileDescriptor bugreportFd, String bugreportFile, - boolean keepBugreportOnRetrievalUnused, IDumpstateListener listener) { int callingUid = Binder.getCallingUid(); enforcePermission(callingPackage, callingUid, false); @@ -565,54 +568,48 @@ class BugreportManagerServiceImpl extends IDumpstate.Stub { } /** - * Validates that the current user is an admin user or, when bugreport is requested remotely - * that the current user is an affiliated user. + * Validates that the calling user is an admin user or, when bugreport is requested remotely + * that the user is an affiliated user. * - * @throws IllegalArgumentException if the current user is not an admin user + * @throws IllegalArgumentException if the calling user is not an admin user */ private void ensureUserCanTakeBugReport(int bugreportMode) { - UserInfo currentUser = null; + // Get the calling userId before clearing the caller identity. + int callingUserId = UserHandle.getUserId(Binder.getCallingUid()); + boolean isAdminUser = false; + final long identity = Binder.clearCallingIdentity(); try { - currentUser = ActivityManager.getService().getCurrentUser(); - } catch (RemoteException e) { - // Impossible to get RemoteException for an in-process call. - } - - if (currentUser == null) { - logAndThrow("There is no current user, so no bugreport can be requested."); + isAdminUser = mInjector.getUserManager().isUserAdmin(callingUserId); + } finally { + Binder.restoreCallingIdentity(identity); } - - if (!currentUser.isAdmin()) { + if (!isAdminUser) { if (bugreportMode == BugreportParams.BUGREPORT_MODE_REMOTE - && isCurrentUserAffiliated(currentUser.id)) { + && isUserAffiliated(callingUserId)) { return; } - logAndThrow(TextUtils.formatSimple("Current user %s is not an admin user." - + " Only admin users are allowed to take bugreport.", currentUser.id)); + logAndThrow(TextUtils.formatSimple("Calling user %s is not an admin user." + + " Only admin users are allowed to take bugreport.", callingUserId)); } } /** - * Returns {@code true} if the device has device owner and the current user is affiliated + * Returns {@code true} if the device has device owner and the specified user is affiliated * with the device owner. */ - private boolean isCurrentUserAffiliated(int currentUserId) { - DevicePolicyManager dpm = mContext.getSystemService(DevicePolicyManager.class); + private boolean isUserAffiliated(int userId) { + DevicePolicyManager dpm = mInjector.getDevicePolicyManager(); int deviceOwnerUid = dpm.getDeviceOwnerUserId(); if (deviceOwnerUid == UserHandle.USER_NULL) { return false; } - int callingUserId = UserHandle.getUserId(Binder.getCallingUid()); - - Slog.i(TAG, "callingUid: " + callingUserId + " deviceOwnerUid: " + deviceOwnerUid - + " currentUserId: " + currentUserId); - - if (callingUserId != deviceOwnerUid) { - logAndThrow("Caller is not device owner on provisioned device."); + if (DEBUG) { + Slog.d(TAG, "callingUid: " + userId + " deviceOwnerUid: " + deviceOwnerUid); } - if (!dpm.isAffiliatedUser(currentUserId)) { - logAndThrow("Current user is not affiliated to the device owner."); + + if (userId != deviceOwnerUid && !dpm.isAffiliatedUser(userId)) { + logAndThrow("User " + userId + " is not affiliated to the device owner."); } return true; } diff --git a/services/tests/servicestests/src/com/android/server/os/BugreportManagerServiceImplTest.java b/services/tests/servicestests/src/com/android/server/os/BugreportManagerServiceImplTest.java index dc1d2c5e54b6..21b8a94d17fb 100644 --- a/services/tests/servicestests/src/com/android/server/os/BugreportManagerServiceImplTest.java +++ b/services/tests/servicestests/src/com/android/server/os/BugreportManagerServiceImplTest.java @@ -16,23 +16,26 @@ package com.android.server.os; -import android.app.admin.flags.Flags; -import static android.app.admin.flags.Flags.onboardingBugreportV2Enabled; - import static com.android.compatibility.common.util.SystemUtil.runWithShellPermissionIdentity; import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertThrows; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.Mockito.when; +import android.app.admin.DevicePolicyManager; +import android.app.admin.flags.Flags; import android.app.role.RoleManager; import android.content.Context; import android.os.Binder; import android.os.BugreportManager.BugreportCallback; +import android.os.BugreportParams; import android.os.IBinder; import android.os.IDumpstateListener; import android.os.Process; import android.os.RemoteException; +import android.os.UserManager; import android.platform.test.annotations.RequiresFlagsEnabled; import android.platform.test.flag.junit.CheckFlagsRule; import android.platform.test.flag.junit.DeviceFlagsValueProvider; @@ -48,6 +51,8 @@ import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; import java.io.FileDescriptor; import java.util.concurrent.CompletableFuture; @@ -66,6 +71,11 @@ public class BugreportManagerServiceImplTest { private BugreportManagerServiceImpl mService; private BugreportManagerServiceImpl.BugreportFileManager mBugreportFileManager; + @Mock + private UserManager mMockUserManager; + @Mock + private DevicePolicyManager mMockDevicePolicyManager; + private int mCallingUid = 1234; private String mCallingPackage = "test.package"; private AtomicFile mMappingFile; @@ -75,14 +85,17 @@ public class BugreportManagerServiceImplTest { @Before public void setUp() { + MockitoAnnotations.initMocks(this); mContext = InstrumentationRegistry.getInstrumentation().getContext(); mMappingFile = new AtomicFile(mContext.getFilesDir(), "bugreport-mapping.xml"); ArraySet mAllowlistedPackages = new ArraySet<>(); mAllowlistedPackages.add(mContext.getPackageName()); mService = new BugreportManagerServiceImpl( - new BugreportManagerServiceImpl.Injector(mContext, mAllowlistedPackages, - mMappingFile)); + new TestInjector(mContext, mAllowlistedPackages, mMappingFile, + mMockUserManager, mMockDevicePolicyManager)); mBugreportFileManager = new BugreportManagerServiceImpl.BugreportFileManager(mMappingFile); + // The calling user is an admin user by default. + when(mMockUserManager.isUserAdmin(anyInt())).thenReturn(true); } @After @@ -164,6 +177,36 @@ public class BugreportManagerServiceImplTest { "test-file.zip", /* forceUpdateMapping= */ true)); } + @Test + public void testStartBugreport_throwsForNonAdminUser() throws Exception { + when(mMockUserManager.isUserAdmin(anyInt())).thenReturn(false); + + Exception thrown = assertThrows(Exception.class, + () -> mService.startBugreport(mCallingUid, mContext.getPackageName(), + new FileDescriptor(), /* screenshotFd= */ null, + BugreportParams.BUGREPORT_MODE_FULL, + /* flags= */ 0, new Listener(new CountDownLatch(1)), + /* isScreenshotRequested= */ false)); + + assertThat(thrown.getMessage()).contains("not an admin user"); + } + + @Test + public void testStartBugreport_throwsForNotAffiliatedUser() throws Exception { + when(mMockUserManager.isUserAdmin(anyInt())).thenReturn(false); + when(mMockDevicePolicyManager.getDeviceOwnerUserId()).thenReturn(-1); + when(mMockDevicePolicyManager.isAffiliatedUser(anyInt())).thenReturn(false); + + Exception thrown = assertThrows(Exception.class, + () -> mService.startBugreport(mCallingUid, mContext.getPackageName(), + new FileDescriptor(), /* screenshotFd= */ null, + BugreportParams.BUGREPORT_MODE_REMOTE, + /* flags= */ 0, new Listener(new CountDownLatch(1)), + /* isScreenshotRequested= */ false)); + + assertThat(thrown.getMessage()).contains("not affiliated to the device owner"); + } + @Test public void testRetrieveBugreportWithoutFilesForCaller() throws Exception { CountDownLatch latch = new CountDownLatch(1); @@ -207,7 +250,8 @@ public class BugreportManagerServiceImplTest { private void clearAllowlist() { mService = new BugreportManagerServiceImpl( - new BugreportManagerServiceImpl.Injector(mContext, new ArraySet<>(), mMappingFile)); + new TestInjector(mContext, new ArraySet<>(), mMappingFile, + mMockUserManager, mMockDevicePolicyManager)); } private static class Listener implements IDumpstateListener { @@ -258,4 +302,27 @@ public class BugreportManagerServiceImplTest { complete(successful); } } + + private static class TestInjector extends BugreportManagerServiceImpl.Injector { + + private final UserManager mUserManager; + private final DevicePolicyManager mDevicePolicyManager; + + TestInjector(Context context, ArraySet allowlistedPackages, AtomicFile mappingFile, + UserManager um, DevicePolicyManager dpm) { + super(context, allowlistedPackages, mappingFile); + mUserManager = um; + mDevicePolicyManager = dpm; + } + + @Override + public UserManager getUserManager() { + return mUserManager; + } + + @Override + public DevicePolicyManager getDevicePolicyManager() { + return mDevicePolicyManager; + } + } } -- cgit v1.2.3-59-g8ed1b