Secure REMOTE_BUGREPORT_DISPATCH

In remote bugreport collection, Shell sends REMOTE_BUGREPORT_DISPATCH to
DevicePolicyManagerService which in turn notifies Device Owners that a
bug report is ready for collection. There existed a threat where a
malicous user could spoof the REMOTE_BUGREPORT_DISPATCH broadcast via
ADB to send a crafted bugreport to the Device Owner. Securing
REMOTE_BUGREPORT_DISPATCH is not as easy as it appears: putting a
permission on REMOTE_BUGREPORT_DISPATCH does not work since both the
legitimate sender and the malicious user are UID_SHELL. Instead, we
introduces a nonce which was sent from DPMS to Shell when bugreport is
triggered, and DPM will only accept REMOTE_BUGREPORT_DISPATCH when
a matching nonce is seen.

Ignore-AOSP-First: security fix

Bug: 171495100
Test: atest DeviceOwnerTest#testRemoteBugreportWithTwoUsers
Test: atest DeviceOwnerTest#testAdminActionBookkeeping
Test: atest BugreportManagerTest
Change-Id: I7649b4f22b74647d152d76bb46d5ca70bfa3617d
Merged-In: I7649b4f22b74647d152d76bb46d5ca70bfa3617d
(cherry picked from commit a4131c50d07c7b58c496bd82b9ab3389b6721654)

5 files changed, 55 insertions, 9 deletions

diff --git a/core/java/android/app/IActivityManager.aidl b/core/java/android/app/IActivityManager.aidl
index 64a9c441c57f..9e23b5fa692b 100644
--- a/core/java/android/app/IActivityManager.aidl
+++ b/core/java/android/app/IActivityManager.aidl
@@ -438,7 +438,7 @@ interface IActivityManager {
 
     void requestInteractiveBugReport();
     void requestFullBugReport();
-    void requestRemoteBugReport();
+    void requestRemoteBugReport(long nonce);
     boolean launchBugReportHandlerApp();
     List<String> getBugreportWhitelistedPackages();
 
diff --git a/core/java/android/app/admin/DevicePolicyManager.java b/core/java/android/app/admin/DevicePolicyManager.java
index a37979a2417f..ff072c268449 100644
--- a/core/java/android/app/admin/DevicePolicyManager.java
+++ b/core/java/android/app/admin/DevicePolicyManager.java
@@ -497,6 +497,14 @@ public class DevicePolicyManager {
             "android.intent.extra.REMOTE_BUGREPORT_HASH";
 
     /**
+     * Extra for shared bugreport's nonce in long integer type.
+     *
+     * @hide
+     */
+    public static final String EXTRA_REMOTE_BUGREPORT_NONCE =
+            "android.intent.extra.REMOTE_BUGREPORT_NONCE";
+
+    /**
      * Extra for remote bugreport notification shown type.
      *
      * @hide
diff --git a/packages/Shell/src/com/android/shell/BugreportProgressService.java b/packages/Shell/src/com/android/shell/BugreportProgressService.java
index 8c7011253c8a..4183c0885033 100644
--- a/packages/Shell/src/com/android/shell/BugreportProgressService.java
+++ b/packages/Shell/src/com/android/shell/BugreportProgressService.java
@@ -157,6 +157,7 @@ public class BugreportProgressService extends Service {
 
     static final String EXTRA_BUGREPORT = "android.intent.extra.BUGREPORT";
     static final String EXTRA_BUGREPORT_TYPE = "android.intent.extra.BUGREPORT_TYPE";
+    static final String EXTRA_BUGREPORT_NONCE = "android.intent.extra.BUGREPORT_NONCE";
     static final String EXTRA_SCREENSHOT = "android.intent.extra.SCREENSHOT";
     static final String EXTRA_ID = "android.intent.extra.ID";
     static final String EXTRA_NAME = "android.intent.extra.NAME";
@@ -415,7 +416,7 @@ public class BugreportProgressService extends Service {
             final String bugreportFilePath = mInfo.bugreportFile.getAbsolutePath();
             if (mInfo.type == BugreportParams.BUGREPORT_MODE_REMOTE) {
                 sendRemoteBugreportFinishedBroadcast(mContext, bugreportFilePath,
-                        mInfo.bugreportFile);
+                        mInfo.bugreportFile, mInfo.nonce);
             } else {
                 cleanupOldFiles(MIN_KEEP_COUNT, MIN_KEEP_AGE, mBugreportsDir);
                 final Intent intent = new Intent(INTENT_BUGREPORT_FINISHED);
@@ -428,7 +429,7 @@ public class BugreportProgressService extends Service {
     }
 
     private static void sendRemoteBugreportFinishedBroadcast(Context context,
-            String bugreportFileName, File bugreportFile) {
+            String bugreportFileName, File bugreportFile, long nonce) {
         cleanupOldFiles(REMOTE_BUGREPORT_FILES_AMOUNT, REMOTE_MIN_KEEP_AGE,
                 bugreportFile.getParentFile());
         final Intent intent = new Intent(DevicePolicyManager.ACTION_REMOTE_BUGREPORT_DISPATCH);
@@ -439,6 +440,7 @@ public class BugreportProgressService extends Service {
         }
         intent.setDataAndType(bugreportUri, BUGREPORT_MIMETYPE);
         intent.putExtra(DevicePolicyManager.EXTRA_REMOTE_BUGREPORT_HASH, bugreportHash);
+        intent.putExtra(DevicePolicyManager.EXTRA_REMOTE_BUGREPORT_NONCE, nonce);
         intent.putExtra(EXTRA_BUGREPORT, bugreportFileName);
         context.sendBroadcastAsUser(intent, UserHandle.SYSTEM,
                 android.Manifest.permission.DUMP);
@@ -613,11 +615,12 @@ public class BugreportProgressService extends Service {
         String shareDescription = intent.getStringExtra(EXTRA_DESCRIPTION);
         int bugreportType = intent.getIntExtra(EXTRA_BUGREPORT_TYPE,
                 BugreportParams.BUGREPORT_MODE_INTERACTIVE);
+        long nonce = intent.getLongExtra(EXTRA_BUGREPORT_NONCE, 0);
         String baseName = getBugreportBaseName(bugreportType);
         String name = new SimpleDateFormat("yyyy-MM-dd-HH-mm-ss").format(new Date());
 
         BugreportInfo info = new BugreportInfo(mContext, baseName, name,
-                shareTitle, shareDescription, bugreportType, mBugreportsDir);
+                shareTitle, shareDescription, bugreportType, mBugreportsDir, nonce);
         synchronized (mLock) {
             if (info.bugreportFile.exists()) {
                 Log.e(TAG, "Failed to start bugreport generation, the requested bugreport file "
@@ -1915,6 +1918,11 @@ public class BugreportProgressService extends Service {
          */
         final int type;
 
+        /**
+         * Nonce of the bugreport
+         */
+        final long nonce;
+
         private final Object mLock = new Object();
 
         /**
@@ -1922,12 +1930,13 @@ public class BugreportProgressService extends Service {
          */
         BugreportInfo(Context context, String baseName, String name,
                 @Nullable String shareTitle, @Nullable String shareDescription,
-                @BugreportParams.BugreportMode int type, File bugreportsDir) {
+                @BugreportParams.BugreportMode int type, File bugreportsDir, long nonce) {
             this.context = context;
             this.name = this.initialName = name;
             this.shareTitle = shareTitle == null ? "" : shareTitle;
             this.shareDescription = shareDescription == null ? "" : shareDescription;
             this.type = type;
+            this.nonce = nonce;
             this.baseName = baseName;
             this.bugreportFile = new File(bugreportsDir, getFileName(this, ".zip"));
         }
@@ -2167,6 +2176,7 @@ public class BugreportProgressService extends Service {
             screenshotCounter = in.readInt();
             shareDescription = in.readString();
             type = in.readInt();
+            nonce = in.readLong();
         }
 
         @Override
@@ -2195,6 +2205,7 @@ public class BugreportProgressService extends Service {
             dest.writeInt(screenshotCounter);
             dest.writeString(shareDescription);
             dest.writeInt(type);
+            dest.writeLong(nonce);
         }
 
         @Override
diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java
index ed7448140581..f5103df80d6a 100644
--- a/services/core/java/com/android/server/am/ActivityManagerService.java
+++ b/services/core/java/com/android/server/am/ActivityManagerService.java
@@ -544,6 +544,7 @@ public class ActivityManagerService extends IActivityManager.Stub
     static final String EXTRA_TITLE = "android.intent.extra.TITLE";
     static final String EXTRA_DESCRIPTION = "android.intent.extra.DESCRIPTION";
     static final String EXTRA_BUGREPORT_TYPE = "android.intent.extra.BUGREPORT_TYPE";
+    static final String EXTRA_BUGREPORT_NONCE = "android.intent.extra.BUGREPORT_NONCE";
 
     /**
      * The maximum number of bytes that {@link #setProcessStateSummary} accepts.
@@ -6591,7 +6592,7 @@ public class ActivityManagerService extends IActivityManager.Stub
      */
     @Override
     public void requestBugReport(@BugreportParams.BugreportMode int bugreportType) {
-        requestBugReportWithDescription(null, null, bugreportType);
+        requestBugReportWithDescription(null, null, bugreportType, 0L);
     }
 
     /**
@@ -6601,6 +6602,15 @@ public class ActivityManagerService extends IActivityManager.Stub
     @Override
     public void requestBugReportWithDescription(@Nullable String shareTitle,
             @Nullable String shareDescription, int bugreportType) {
+        requestBugReportWithDescription(shareTitle, shareDescription, bugreportType, /*nonce*/ 0L);
+    }
+
+    /**
+     * Takes a bugreport using bug report API ({@code BugreportManager}) which gets
+     * triggered by sending a broadcast to Shell.
+     */
+    public void requestBugReportWithDescription(@Nullable String shareTitle,
+            @Nullable String shareDescription, int bugreportType, long nonce) {
         String type = null;
         switch (bugreportType) {
             case BugreportParams.BUGREPORT_MODE_FULL:
@@ -6651,6 +6661,7 @@ public class ActivityManagerService extends IActivityManager.Stub
         triggerShellBugreport.setAction(INTENT_BUGREPORT_REQUESTED);
         triggerShellBugreport.setPackage(SHELL_APP_PACKAGE);
         triggerShellBugreport.putExtra(EXTRA_BUGREPORT_TYPE, bugreportType);
+        triggerShellBugreport.putExtra(EXTRA_BUGREPORT_NONCE, nonce);
         triggerShellBugreport.addFlags(Intent.FLAG_RECEIVER_FOREGROUND);
         triggerShellBugreport.addFlags(Intent.FLAG_RECEIVER_INCLUDE_BACKGROUND);
         if (shareTitle != null) {
@@ -6717,8 +6728,8 @@ public class ActivityManagerService extends IActivityManager.Stub
      * Takes a bugreport remotely
      */
     @Override
-    public void requestRemoteBugReport() {
-        requestBugReportWithDescription(null, null, BugreportParams.BUGREPORT_MODE_REMOTE);
+    public void requestRemoteBugReport(long nonce) {
+        requestBugReportWithDescription(null, null, BugreportParams.BUGREPORT_MODE_REMOTE, nonce);
     }
 
     /**
diff --git a/services/devicepolicy/java/com/android/server/devicepolicy/RemoteBugreportManager.java b/services/devicepolicy/java/com/android/server/devicepolicy/RemoteBugreportManager.java
index d7cbd9b17ff3..346e859f9275 100644
--- a/services/devicepolicy/java/com/android/server/devicepolicy/RemoteBugreportManager.java
+++ b/services/devicepolicy/java/com/android/server/devicepolicy/RemoteBugreportManager.java
@@ -31,6 +31,7 @@ import android.annotation.IntDef;
 import android.app.Notification;
 import android.app.PendingIntent;
 import android.app.admin.DeviceAdminReceiver;
+import android.app.admin.DevicePolicyManager;
 import android.content.BroadcastReceiver;
 import android.content.Context;
 import android.content.Intent;
@@ -54,7 +55,9 @@ import com.android.server.utils.Slogf;
 import java.io.FileNotFoundException;
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
+import java.security.SecureRandom;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicLong;
 
 /**
  * Class managing bugreport collection upon device owner's request.
@@ -78,6 +81,9 @@ public class RemoteBugreportManager {
     private final DevicePolicyManagerService mService;
     private final DevicePolicyManagerService.Injector mInjector;
 
+    private final SecureRandom mRng = new SecureRandom();
+
+    private final AtomicLong mRemoteBugreportNonce = new AtomicLong();
     private final AtomicBoolean mRemoteBugreportServiceIsActive = new AtomicBoolean();
     private final AtomicBoolean mRemoteBugreportSharingAccepted = new AtomicBoolean();
     private final Context mContext;
@@ -197,8 +203,13 @@ public class RemoteBugreportManager {
 
         final long callingIdentity = mInjector.binderClearCallingIdentity();
         try {
-            mInjector.getIActivityManager().requestRemoteBugReport();
+            long nonce;
+            do {
+                nonce = mRng.nextLong();
+            } while (nonce == 0);
+            mInjector.getIActivityManager().requestRemoteBugReport(nonce);
 
+            mRemoteBugreportNonce.set(nonce);
             mRemoteBugreportServiceIsActive.set(true);
             mRemoteBugreportSharingAccepted.set(false);
             registerRemoteBugreportReceivers();
@@ -231,6 +242,11 @@ public class RemoteBugreportManager {
     }
 
     private void onBugreportFinished(Intent intent) {
+        long nonce = intent.getLongExtra(DevicePolicyManager.EXTRA_REMOTE_BUGREPORT_NONCE, 0);
+        if (nonce == 0 || mRemoteBugreportNonce.get() != nonce) {
+            Slogf.w(LOG_TAG, "Invalid nonce provided, ignoring " + nonce);
+            return;
+        }
         mHandler.removeCallbacks(mRemoteBugreportTimeoutRunnable);
         mRemoteBugreportServiceIsActive.set(false);
         final Uri bugreportUri = intent.getData();