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
diff --git a/services/core/java/com/android/server/wm/ActivityStartController.java b/services/core/java/com/android/server/wm/ActivityStartController.java
index 6ad2f7c..bb5d962 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 @@
* @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 @@
.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 df9a6d2..8788225 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 @@
}
// 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 bd8d116..877965e 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 @@
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 @@
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 @@
}
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 @@
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 5d0e34a..6fa306b 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 @@
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 @@
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 b8e12d1..7014851 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.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 @@
}
@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