From a4131c50d07c7b58c496bd82b9ab3389b6721654 Mon Sep 17 00:00:00 2001 From: Rubin Xu Date: Tue, 2 Mar 2021 15:27:51 +0000 Subject: 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 --- .../com/android/shell/BugreportProgressService.java | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) (limited to 'packages/Shell/src') diff --git a/packages/Shell/src/com/android/shell/BugreportProgressService.java b/packages/Shell/src/com/android/shell/BugreportProgressService.java index 0b8bd9784b7d..1ce4c64fd8b8 100644 --- a/packages/Shell/src/com/android/shell/BugreportProgressService.java +++ b/packages/Shell/src/com/android/shell/BugreportProgressService.java @@ -161,6 +161,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"; @@ -428,7 +429,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); @@ -441,7 +442,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); @@ -452,6 +453,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); @@ -628,11 +630,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 " @@ -2065,6 +2068,11 @@ public class BugreportProgressService extends Service { */ final int type; + /** + * Nonce of the bugreport + */ + final long nonce; + private final Object mLock = new Object(); /** @@ -2072,12 +2080,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")); } @@ -2317,6 +2326,7 @@ public class BugreportProgressService extends Service { screenshotCounter = in.readInt(); shareDescription = in.readString(); type = in.readInt(); + nonce = in.readLong(); } @Override @@ -2345,6 +2355,7 @@ public class BugreportProgressService extends Service { dest.writeInt(screenshotCounter); dest.writeString(shareDescription); dest.writeInt(type); + dest.writeLong(nonce); } @Override -- cgit v1.2.3-59-g8ed1b