diff options
3 files changed, 138 insertions, 12 deletions
diff --git a/core/java/android/os/BugreportManager.java b/core/java/android/os/BugreportManager.java index 46ad7b880a37..33736d3bf9f3 100644 --- a/core/java/android/os/BugreportManager.java +++ b/core/java/android/os/BugreportManager.java @@ -26,7 +26,6 @@ import android.annotation.SystemApi; import android.annotation.SystemService; import android.app.ActivityManager; import android.content.Context; -import android.os.Handler; import android.util.Log; import android.widget.Toast; @@ -189,13 +188,18 @@ public final class BugreportManager { } } - /* - * Cancels a currently running bugreport. + /** + * Cancels the currently running bugreport. + * + * <p>Apps are only able to cancel their own bugreports. App A cannot cancel a bugreport started + * by app B. + * + * @throws SecurityException if trying to cancel another app's bugreport in progress */ @RequiresPermission(android.Manifest.permission.DUMP) public void cancelBugreport() { try { - mBinder.cancelBugreport(); + mBinder.cancelBugreport(-1 /* callingUid */, mContext.getOpPackageName()); } catch (RemoteException e) { throw e.rethrowFromSystemServer(); } diff --git a/core/tests/bugreports/src/com/android/os/bugreports/tests/BugreportManagerTest.java b/core/tests/bugreports/src/com/android/os/bugreports/tests/BugreportManagerTest.java index a5ef2b49b65b..d8ed805919f7 100644 --- a/core/tests/bugreports/src/com/android/os/bugreports/tests/BugreportManagerTest.java +++ b/core/tests/bugreports/src/com/android/os/bugreports/tests/BugreportManagerTest.java @@ -23,7 +23,10 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import android.Manifest; +import android.content.BroadcastReceiver; import android.content.Context; +import android.content.Intent; +import android.content.IntentFilter; import android.os.BugreportManager; import android.os.BugreportManager.BugreportCallback; import android.os.BugreportParams; @@ -31,7 +34,9 @@ import android.os.FileUtils; import android.os.Handler; import android.os.HandlerThread; import android.os.ParcelFileDescriptor; +import android.os.Process; import android.os.StrictMode; +import android.text.TextUtils; import android.util.Log; import androidx.annotation.NonNull; @@ -53,10 +58,11 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; import java.io.File; +import java.io.IOException; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.Executor; import java.util.concurrent.TimeUnit; - /** * Tests for BugreportManager API. */ @@ -67,8 +73,16 @@ public class BugreportManagerTest { private static final String TAG = "BugreportManagerTest"; private static final long BUGREPORT_TIMEOUT_MS = TimeUnit.MINUTES.toMillis(10); + private static final long DUMPSTATE_STARTUP_TIMEOUT_MS = TimeUnit.SECONDS.toMillis(10); private static final long UIAUTOMATOR_TIMEOUT_MS = TimeUnit.SECONDS.toMillis(10); + // Sent by Shell when its bugreport finishes (contains final bugreport/screenshot file name + // associated with the bugreport). + private static final String INTENT_BUGREPORT_FINISHED = + "com.android.internal.intent.action.BUGREPORT_FINISHED"; + private static final String EXTRA_BUGREPORT = "android.intent.extra.BUGREPORT"; + private static final String EXTRA_SCREENSHOT = "android.intent.extra.SCREENSHOT"; + private Handler mHandler; private Executor mExecutor; private BugreportManager mBrm; @@ -212,6 +226,48 @@ public class BugreportManagerTest { } @Test + public void cancelBugreport_noReportStarted() throws Exception { + // Without the native DumpstateService running, we don't get a SecurityException. + mBrm.cancelBugreport(); + } + + @LargeTest + @Test + public void cancelBugreport_fromDifferentUid() throws Exception { + assertThat(Process.myUid()).isNotEqualTo(Process.SHELL_UID); + + // Start a bugreport through ActivityManager's shell command - this starts a BR from the + // shell UID rather than our own. + BugreportBroadcastReceiver br = new BugreportBroadcastReceiver(); + InstrumentationRegistry.getContext() + .registerReceiver(br, new IntentFilter(INTENT_BUGREPORT_FINISHED)); + UiDevice.getInstance(InstrumentationRegistry.getInstrumentation()) + .executeShellCommand("am bug-report"); + + // The command triggers the report through a broadcast, so wait until dumpstate actually + // starts up, which may take a bit. + waitTillDumpstateRunningOrTimeout(); + + try { + mBrm.cancelBugreport(); + fail("Expected cancelBugreport to throw SecurityException when report started by " + + "different UID"); + } catch (SecurityException expected) { + } finally { + // Do this in the finally block so that even if this test case fails, we don't break + // other test cases unexpectedly due to the still-running shell report. + try { + // The shell's BR is still running and should complete successfully. + br.waitForBugreportFinished(); + } finally { + // The latch may fail for a number of reasons but we still need to unregister the + // BroadcastReceiver. + InstrumentationRegistry.getContext().unregisterReceiver(br); + } + } + } + + @Test public void insufficientPermissions_throwsException() throws Exception { dropPermissions(); @@ -347,6 +403,28 @@ public class BugreportManagerTest { .adoptShellPermissionIdentity(Manifest.permission.DUMP); } + private static boolean isDumpstateRunning() { + String[] output; + try { + output = + UiDevice.getInstance(InstrumentationRegistry.getInstrumentation()) + .executeShellCommand("ps -A -o NAME | grep dumpstate") + .trim() + .split("\n"); + } catch (IOException e) { + Log.w(TAG, "Failed to check if dumpstate is running", e); + return false; + } + for (String line : output) { + // Check for an exact match since there may be other things that contain "dumpstate" as + // a substring (e.g. the dumpstate HAL). + if (TextUtils.equals("dumpstate", line)) { + return true; + } + } + return false; + } + private static void assertFdIsClosed(ParcelFileDescriptor pfd) { try { int fd = pfd.getFd(); @@ -365,18 +443,25 @@ public class BugreportManagerTest { return System.currentTimeMillis(); } - private static boolean shouldTimeout(long startTimeMs) { - return now() - startTimeMs >= BUGREPORT_TIMEOUT_MS; + private static void waitTillDumpstateRunningOrTimeout() throws Exception { + long startTimeMs = now(); + while (!isDumpstateRunning()) { + Thread.sleep(500 /* .5s */); + if (now() - startTimeMs >= DUMPSTATE_STARTUP_TIMEOUT_MS) { + break; + } + Log.d(TAG, "Waited " + (now() - startTimeMs) + "ms for dumpstate to start"); + } } private static void waitTillDoneOrTimeout(BugreportCallbackImpl callback) throws Exception { long startTimeMs = now(); while (!callback.isDone()) { Thread.sleep(1000 /* 1s */); - if (shouldTimeout(startTimeMs)) { + if (now() - startTimeMs >= BUGREPORT_TIMEOUT_MS) { break; } - Log.d(TAG, "Waited " + (now() - startTimeMs) + "ms"); + Log.d(TAG, "Waited " + (now() - startTimeMs) + "ms for bugreport to finish"); } } @@ -451,6 +536,36 @@ public class BugreportManagerTest { assertTrue(device.wait(Until.gone(consentTitleObj), UIAUTOMATOR_TIMEOUT_MS)); } + private class BugreportBroadcastReceiver extends BroadcastReceiver { + Intent mBugreportFinishedIntent = null; + final CountDownLatch mLatch; + + BugreportBroadcastReceiver() { + mLatch = new CountDownLatch(1); + } + + @Override + public void onReceive(Context context, Intent intent) { + setBugreportFinishedIntent(intent); + mLatch.countDown(); + } + + private void setBugreportFinishedIntent(Intent intent) { + mBugreportFinishedIntent = intent; + } + + public Intent getBugreportFinishedIntent() { + return mBugreportFinishedIntent; + } + + public void waitForBugreportFinished() throws Exception { + if (!mLatch.await(BUGREPORT_TIMEOUT_MS, TimeUnit.MILLISECONDS)) { + throw new Exception("Failed to receive BUGREPORT_FINISHED in " + + BUGREPORT_TIMEOUT_MS + " ms."); + } + } + } + /** * A rule to change strict mode vm policy temporarily till test method finished. * diff --git a/services/core/java/com/android/server/os/BugreportManagerServiceImpl.java b/services/core/java/com/android/server/os/BugreportManagerServiceImpl.java index dd507a3b1f81..01eeb31dff2b 100644 --- a/services/core/java/com/android/server/os/BugreportManagerServiceImpl.java +++ b/services/core/java/com/android/server/os/BugreportManagerServiceImpl.java @@ -94,9 +94,12 @@ class BugreportManagerServiceImpl extends IDumpstate.Stub { @Override @RequiresPermission(android.Manifest.permission.DUMP) - public void cancelBugreport() { + public void cancelBugreport(int callingUidUnused, String callingPackage) { mContext.enforceCallingOrSelfPermission(android.Manifest.permission.DUMP, "cancelBugreport"); + int callingUid = Binder.getCallingUid(); + mAppOps.checkPackage(callingUid, callingPackage); + synchronized (mLock) { IDumpstate ds = getDumpstateBinderServiceLocked(); if (ds == null) { @@ -104,7 +107,11 @@ class BugreportManagerServiceImpl extends IDumpstate.Stub { return; } try { - ds.cancelBugreport(); + // Note: this may throw SecurityException back out to the caller if they aren't + // allowed to cancel the report, in which case we should NOT be setting ctl.stop, + // since that would unintentionally kill some other app's bugreport, which we + // specifically disallow. + ds.cancelBugreport(callingUid, callingPackage); } catch (RemoteException e) { Slog.e(TAG, "RemoteException in cancelBugreport", e); } @@ -182,7 +189,7 @@ class BugreportManagerServiceImpl extends IDumpstate.Stub { // lifecycle correctly. If we don't subsequent callers will get // BUGREPORT_ERROR_ANOTHER_REPORT_IN_PROGRESS error. // Note that listener will be notified by the death recipient below. - cancelBugreport(); + cancelBugreport(callingUid, callingPackage); } } |