Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit a4131c50 authored by Rubin Xu's avatar Rubin Xu
Browse files

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
parent b7f5e21e

core/java/android/app/IActivityManager.aidl

+1 −1
Original line number Diff line number Diff line
@@ -449,7 +449,7 @@ interface IActivityManager { 


    void requestInteractiveBugReport();

    void requestFullBugReport();

    void requestRemoteBugReport();

    void requestRemoteBugReport(long nonce);

    boolean launchBugReportHandlerApp();

    List<String> getBugreportWhitelistedPackages();

core/java/android/app/admin/DevicePolicyManager.java

+8 −0
Original line number Diff line number Diff line
@@ -907,6 +907,14 @@ public class DevicePolicyManager { 
    public static final String EXTRA_REMOTE_BUGREPORT_HASH =

            "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.















     *

































packages/Shell/src/com/android/shell/BugreportProgressService.java



+15
−4




























Original line number
Diff line number
Diff line








@@ -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

































services/core/java/com/android/server/am/ActivityManagerService.java



+14
−3




























Original line number
Diff line number
Diff line








@@ -575,6 +575,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";





























    /**















     * It is now required for apps to explicitly set either










@@ -6978,7 +6979,7 @@ public class ActivityManagerService extends IActivityManager.Stub













     */















    @Override















    public void requestBugReport(@BugreportParams.BugreportMode int bugreportType) {













        requestBugReportWithDescription(null, null, bugreportType);













        requestBugReportWithDescription(null, null, bugreportType, 0L);















    }





























    /**









@@ -6988,6 +6989,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:










@@ -7038,6 +7048,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) {










@@ -7104,8 +7115,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);















    }





























    /**

































services/devicepolicy/java/com/android/server/devicepolicy/RemoteBugreportManager.java



+17
−1




























Original line number
Diff line number
Diff line








@@ -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();










@@ -232,6 +243,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();