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

Commit a3deece0 authored by Chris Li's avatar Chris Li
Browse files

Fix security bug for TaskFragment creation 

1. Check Task#effectiveUid instead of rootActivity Uid for TF creation
2. Pass in the caller uid/pid for activity starter instead of using
   Binder.getCallingUid() after clearCallingIdentity()

Bug: 205996115
Test: atest WmTests:TaskFragmentOrganizerControllerTest
Change-Id: I1b1470a95d55820dd6d69658c570c3e5640ebe55
parent 4d94c48a

services/core/java/com/android/server/wm/ActivityStartController.java

+5 −3
Original line number Diff line number Diff line
@@ -496,11 +496,13 @@ public class ActivityStartController { 
     * @param activityIntent intent to start the activity.

     * @param activityOptions ActivityOptions to start the activity with.

     * @param resultTo the caller activity

     * @param callingUid the caller uid

     * @param callingPid the caller pid

     * @return the start result.

     */

    int startActivityInTaskFragment(@NonNull TaskFragment taskFragment,

            @NonNull Intent activityIntent, @Nullable Bundle activityOptions,

            @Nullable IBinder resultTo) {

            @Nullable IBinder resultTo, int callingUid, int callingPid) {

        final ActivityRecord caller =

                resultTo != null ? ActivityRecord.forTokenLocked(resultTo) : null;

        return obtainStarter(activityIntent, "startActivityInTaskFragment")
@@ -508,8 +510,8 @@ public class ActivityStartController { 
                .setInTaskFragment(taskFragment)

                .setResultTo(resultTo)

                .setRequestCode(-1)

                .setCallingUid(Binder.getCallingUid())

                .setCallingPid(Binder.getCallingPid())

                .setCallingUid(callingUid)

                .setCallingPid(callingPid)

                .setUserId(caller != null ? caller.mUserId : mService.getCurrentUserId())

                .execute();

    }

services/core/java/com/android/server/wm/AppTransitionController.java

+1 −1
Original line number Diff line number Diff line
@@ -590,7 +590,7 @@ public class AppTransitionController { 
            }

            // We don't want the organizer to handle transition of non-embedded activity of other

            // app.

            if (r.getUid() != rootActivity.getUid() && !r.isEmbedded()) {

            if (r.getUid() != task.effectiveUid && !r.isEmbedded()) {

                leafTask = null;

                break;

            }

services/core/java/com/android/server/wm/WindowOrganizerController.java

+6 −6
Original line number Diff line number Diff line
@@ -588,7 +588,7 @@ class WindowOrganizerController extends IWindowOrganizerController.Stub 
            case HIERARCHY_OP_TYPE_CREATE_TASK_FRAGMENT: {

                final TaskFragmentCreationParams taskFragmentCreationOptions =

                        hop.getTaskFragmentCreationOptions();

                createTaskFragment(taskFragmentCreationOptions, errorCallbackToken);

                createTaskFragment(taskFragmentCreationOptions, errorCallbackToken, caller);

                break;

            }

            case HIERARCHY_OP_TYPE_DELETE_TASK_FRAGMENT: {
@@ -631,7 +631,7 @@ class WindowOrganizerController extends IWindowOrganizerController.Stub 
                final TaskFragment tf = mLaunchTaskFragments.get(fragmentToken);

                final int result = mService.getActivityStartController()

                        .startActivityInTaskFragment(tf, activityIntent, activityOptions,

                                hop.getCallingActivity());

                                hop.getCallingActivity(), caller.mUid, caller.mPid);

                if (!isStartResultSuccessful(result)) {

                    sendTaskFragmentOperationFailure(tf.getTaskFragmentOrganizer(),

                            errorCallbackToken,
@@ -1200,7 +1200,7 @@ class WindowOrganizerController extends IWindowOrganizerController.Stub 
    }



    void createTaskFragment(@NonNull TaskFragmentCreationParams creationParams,

            @Nullable IBinder errorCallbackToken) {

            @Nullable IBinder errorCallbackToken, @NonNull CallerInfo caller) {

        final ActivityRecord ownerActivity =

                ActivityRecord.forTokenLocked(creationParams.getOwnerToken());

        final ITaskFragmentOrganizer organizer = ITaskFragmentOrganizer.Stub.asInterface(
@@ -1218,9 +1218,9 @@ class WindowOrganizerController extends IWindowOrganizerController.Stub 
            sendTaskFragmentOperationFailure(organizer, errorCallbackToken, exception);

            return;

        }

        // The ownerActivity has to belong to the same app as the root Activity of the target Task.

        final ActivityRecord rootActivity = ownerActivity.getTask().getRootActivity();

        if (rootActivity.getUid() != ownerActivity.getUid()) {

        // The ownerActivity has to belong to the same app as the target Task.

        if (ownerActivity.getTask().effectiveUid != ownerActivity.getUid()

                || ownerActivity.getTask().effectiveUid != caller.mUid) {

            final Throwable exception =

                    new IllegalArgumentException("Not allowed to operate with the ownerToken while "

                            + "the root activity of the target task belong to the different app");

services/tests/wmtests/src/com/android/server/wm/AppTransitionControllerTest.java

+2 −0
Original line number Diff line number Diff line
@@ -803,6 +803,7 @@ public class AppTransitionControllerTest extends WindowTestsBase { 
        final TaskFragment taskFragment = createTaskFragmentWithEmbeddedActivity(task, organizer);

        final ActivityRecord openingActivity = taskFragment.getTopMostActivity();

        openingActivity.allDrawn = true;

        task.effectiveUid = openingActivity.getUid();

        spyOn(mDisplayContent.mAppTransition);



        // Prepare a transition.
@@ -879,6 +880,7 @@ public class AppTransitionControllerTest extends WindowTestsBase { 
        final ActivityRecord closingActivity = taskFragment.getTopMostActivity();

        closingActivity.allDrawn = true;

        closingActivity.info.applicationInfo.uid = 12345;

        task.effectiveUid = closingActivity.getUid();

        // Opening non-embedded activity with different UID.

        final ActivityRecord openingActivity = createActivityRecord(task);

        openingActivity.info.applicationInfo.uid = 54321;

services/tests/wmtests/src/com/android/server/wm/TaskFragmentOrganizerControllerTest.java

+52 −13
Original line number Diff line number Diff line
@@ -21,6 +21,7 @@ import static com.android.dx.mockito.inline.extended.ExtendedMockito.spyOn; 
import static com.android.server.wm.testing.Assert.assertThrows;



import static org.junit.Assert.assertEquals;

import static org.junit.Assert.assertNotNull;

import static org.junit.Assert.assertNull;

import static org.junit.Assert.fail;

import static org.mockito.ArgumentMatchers.any;
@@ -352,28 +353,66 @@ public class TaskFragmentOrganizerControllerTest extends WindowTestsBase { 
    }



    @Test

    public void testApplyTransaction_enforceHierarchyChange_createTaskFragment() {

    public void testApplyTransaction_enforceHierarchyChange_createTaskFragment()

            throws RemoteException {

        mController.registerOrganizer(mIOrganizer);

        final ActivityRecord activity = createActivityRecord(mDisplayContent);

        final int uid = Binder.getCallingUid();

        activity.info.applicationInfo.uid = uid;

        activity.getTask().effectiveUid = uid;

        final IBinder fragmentToken = new Binder();

        final TaskFragmentCreationParams params = new TaskFragmentCreationParams.Builder(

                mOrganizerToken, fragmentToken, activity.token).build();

        mOrganizer.applyTransaction(mTransaction);



        // Allow organizer to create TaskFragment and start/reparent activity to TaskFragment.

        final TaskFragmentCreationParams mockParams = mock(TaskFragmentCreationParams.class);

        doReturn(mOrganizerToken).when(mockParams).getOrganizer();

        mTransaction.createTaskFragment(mockParams);

        mTransaction.createTaskFragment(params);

        mTransaction.startActivityInTaskFragment(

                mFragmentToken, null /* callerToken */, new Intent(), null /* activityOptions */);

        mTransaction.reparentActivityToTaskFragment(mFragmentToken, mock(IBinder.class));

        mTransaction.setAdjacentTaskFragments(mFragmentToken, mock(IBinder.class),

                null /* options */);



        // It is expected to fail for the mock TaskFragmentCreationParams. It is ok as we are

        // testing the security check here.

        assertThrows(IllegalArgumentException.class, () -> {

            try {

        mAtm.getWindowOrganizerController().applyTransaction(mTransaction);

            } catch (RemoteException e) {

                fail();



        // Successfully created a TaskFragment.

        final TaskFragment taskFragment = mAtm.mWindowOrganizerController

                .getTaskFragment(fragmentToken);

        assertNotNull(taskFragment);

        assertEquals(activity.getTask(), taskFragment.getTask());

    }

        });



    @Test

    public void testApplyTransaction_createTaskFragment_failForDifferentUid()

            throws RemoteException {

        mController.registerOrganizer(mIOrganizer);

        final ActivityRecord activity = createActivityRecord(mDisplayContent);

        final int uid = Binder.getCallingUid();

        final IBinder fragmentToken = new Binder();

        final TaskFragmentCreationParams params = new TaskFragmentCreationParams.Builder(

                mOrganizerToken, fragmentToken, activity.token).build();

        mOrganizer.applyTransaction(mTransaction);

        mTransaction.createTaskFragment(params);



        // Fail to create TaskFragment when the task uid is different from caller.

        activity.info.applicationInfo.uid = uid;

        activity.getTask().effectiveUid = uid + 1;

        mAtm.getWindowOrganizerController().applyTransaction(mTransaction);



        assertNull(mAtm.mWindowOrganizerController.getTaskFragment(fragmentToken));



        // Fail to create TaskFragment when the task uid is different from owner activity.

        activity.info.applicationInfo.uid = uid + 1;

        activity.getTask().effectiveUid = uid;

        mAtm.getWindowOrganizerController().applyTransaction(mTransaction);



        assertNull(mAtm.mWindowOrganizerController.getTaskFragment(fragmentToken));



        // Successfully created a TaskFragment for same uid.

        activity.info.applicationInfo.uid = uid;

        activity.getTask().effectiveUid = uid;

        mAtm.getWindowOrganizerController().applyTransaction(mTransaction);



        assertNotNull(mAtm.mWindowOrganizerController.getTaskFragment(fragmentToken));

    }



    @Test