Merge "Fix security bug for TaskFragment creation" into sc-v2-dev am: 5b9b66e85d

Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/base/+/16280766

Change-Id: Ic8d56fd36296c031381e3726b5b8915c98ed5e7d

5 files changed, 66 insertions, 23 deletions

diff --git a/services/core/java/com/android/server/wm/ActivityStartController.java b/services/core/java/com/android/server/wm/ActivityStartController.java
index 6ad2f7cad3c2..bb5d962760e7 100644
--- a/services/core/java/com/android/server/wm/ActivityStartController.java
+++ b/services/core/java/com/android/server/wm/ActivityStartController.java
@@ -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();
     }
diff --git a/services/core/java/com/android/server/wm/AppTransitionController.java b/services/core/java/com/android/server/wm/AppTransitionController.java
index df9a6d21113e..878822522d08 100644
--- a/services/core/java/com/android/server/wm/AppTransitionController.java
+++ b/services/core/java/com/android/server/wm/AppTransitionController.java
@@ -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;
             }
diff --git a/services/core/java/com/android/server/wm/WindowOrganizerController.java b/services/core/java/com/android/server/wm/WindowOrganizerController.java
index bd8d1164ebef..877965e5bfde 100644
--- a/services/core/java/com/android/server/wm/WindowOrganizerController.java
+++ b/services/core/java/com/android/server/wm/WindowOrganizerController.java
@@ -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");
diff --git a/services/tests/wmtests/src/com/android/server/wm/AppTransitionControllerTest.java b/services/tests/wmtests/src/com/android/server/wm/AppTransitionControllerTest.java
index 5d0e34a80f3f..6fa306b004a2 100644
--- a/services/tests/wmtests/src/com/android/server/wm/AppTransitionControllerTest.java
+++ b/services/tests/wmtests/src/com/android/server/wm/AppTransitionControllerTest.java
@@ -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;
diff --git a/services/tests/wmtests/src/com/android/server/wm/TaskFragmentOrganizerControllerTest.java b/services/tests/wmtests/src/com/android/server/wm/TaskFragmentOrganizerControllerTest.java
index b8e12d15c5d2..7014851ee210 100644
--- a/services/tests/wmtests/src/com/android/server/wm/TaskFragmentOrganizerControllerTest.java
+++ b/services/tests/wmtests/src/com/android/server/wm/TaskFragmentOrganizerControllerTest.java
@@ -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 */);
+        mAtm.getWindowOrganizerController().applyTransaction(mTransaction);
 
-        // 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