From fbbbef4e213fe8855bfcfafabc98874fe7b0675b Mon Sep 17 00:00:00 2001 From: Hani Kazmi Date: Mon, 13 Feb 2023 14:06:06 +0000 Subject: [PATCH] Update ASM rules based on feedback. This change updates the rules based on droidfood findings. 1. BAL_ALLOW_SAW_PERMISSION now allows ASM bypass for new tasks. Some apps (i.e, alarm apps) use display over other window permissions to show the alarm activity even if the alarm task is not in the foreground. As users must explicitely grant this permission, we do not consider it a risk. 2. BAL_ALLOW_VISIBLE_WINDOW now only allows ASM bypass for new tasks. If an app requires a bypass within a task, it can request the new flag we are introducing in ag/20882611. 3. Do not clear activities from top of task if launcher or launched activity matches top activity. This is relaxed rule from before, where only the launched was considered. We allow this to allow pending intents to be passed from A to B in the case below, and still fired from A without creating unpredictable back behaviour. As A passes the intent to B (and launched B), we assume there is a level of trust between A and B. B A ---- Bottom Bug: 263368846 Test: atest BackgroundActivityLaunchTest ActivitySecurityModelTest Change-Id: Ic698d27b35023063f3c7214414bee2b9298f4fb3 --- .../android/server/wm/ActivityStarter.java | 107 ++++++++++-------- .../server/wm/ActivityStarterTests.java | 3 +- .../DisplayWindowPolicyControllerTests.java | 6 +- 3 files changed, 64 insertions(+), 52 deletions(-) diff --git a/services/core/java/com/android/server/wm/ActivityStarter.java b/services/core/java/com/android/server/wm/ActivityStarter.java index ad5ca9598f42f..1d5fcd76b9909 100644 --- a/services/core/java/com/android/server/wm/ActivityStarter.java +++ b/services/core/java/com/android/server/wm/ActivityStarter.java @@ -80,6 +80,7 @@ import static com.android.server.wm.BackgroundActivityStartController.BAL_ALLOW_ import static com.android.server.wm.BackgroundActivityStartController.BAL_ALLOW_DEFAULT; import static com.android.server.wm.BackgroundActivityStartController.BAL_ALLOW_PENDING_INTENT; import static com.android.server.wm.BackgroundActivityStartController.BAL_ALLOW_PERMISSION; +import static com.android.server.wm.BackgroundActivityStartController.BAL_ALLOW_SAW_PERMISSION; import static com.android.server.wm.BackgroundActivityStartController.BAL_ALLOW_VISIBLE_WINDOW; import static com.android.server.wm.BackgroundActivityStartController.BAL_BLOCK; import static com.android.server.wm.LaunchParamsController.LaunchParamsModifier.PHASE_BOUNDS; @@ -148,6 +149,7 @@ import com.android.server.wm.TaskFragment.EmbeddingCheckResult; import java.io.PrintWriter; import java.text.DateFormat; import java.util.Date; +import java.util.function.Predicate; /** * Controller for interpreting how and then launching an activity. @@ -189,6 +191,7 @@ class ActivityStarter { ActivityRecord mStartActivity; private Intent mIntent; private int mCallingUid; + private int mRealCallingUid; private ActivityOptions mOptions; // If it is BAL_BLOCK, background activity can only be started in an existing task that contains @@ -611,6 +614,7 @@ class ActivityStarter { mStartActivity = starter.mStartActivity; mIntent = starter.mIntent; mCallingUid = starter.mCallingUid; + mRealCallingUid = starter.mRealCallingUid; mOptions = starter.mOptions; mBalCode = starter.mBalCode; @@ -1272,7 +1276,7 @@ class ActivityStarter { mLastStartActivityResult = startActivityUnchecked(r, sourceRecord, voiceSession, request.voiceInteractor, startFlags, checkedOptions, - inTask, inTaskFragment, balCode, intentGrants); + inTask, inTaskFragment, balCode, intentGrants, realCallingUid); if (request.outActivity != null) { request.outActivity[0] = mLastStartActivityRecord; @@ -1423,7 +1427,7 @@ class ActivityStarter { IVoiceInteractionSession voiceSession, IVoiceInteractor voiceInteractor, int startFlags, ActivityOptions options, Task inTask, TaskFragment inTaskFragment, @BalCode int balCode, - NeededUriGrants intentGrants) { + NeededUriGrants intentGrants, int realCallingUid) { int result = START_CANCELED; final Task startedActivityRootTask; @@ -1443,7 +1447,7 @@ class ActivityStarter { Trace.traceBegin(Trace.TRACE_TAG_WINDOW_MANAGER, "startActivityInner"); result = startActivityInner(r, sourceRecord, voiceSession, voiceInteractor, startFlags, options, inTask, inTaskFragment, balCode, - intentGrants); + intentGrants, realCallingUid); } finally { Trace.traceEnd(Trace.TRACE_TAG_WINDOW_MANAGER); startedActivityRootTask = handleStartResult(r, options, result, newTransition, @@ -1590,9 +1594,9 @@ class ActivityStarter { IVoiceInteractionSession voiceSession, IVoiceInteractor voiceInteractor, int startFlags, ActivityOptions options, Task inTask, TaskFragment inTaskFragment, @BalCode int balCode, - NeededUriGrants intentGrants) { + NeededUriGrants intentGrants, int realCallingUid) { setInitialState(r, options, inTask, inTaskFragment, startFlags, sourceRecord, - voiceSession, voiceInteractor, balCode); + voiceSession, voiceInteractor, balCode, realCallingUid); computeLaunchingTaskFlags(); mIntent.setFlags(mLaunchFlags); @@ -1938,7 +1942,9 @@ class ActivityStarter { if (taskToFront) { if (mBalCode == BAL_ALLOW_ALLOWLISTED_COMPONENT || mBalCode == BAL_ALLOW_PERMISSION - || mBalCode == BAL_ALLOW_PENDING_INTENT) { + || mBalCode == BAL_ALLOW_PENDING_INTENT + || mBalCode == BAL_ALLOW_SAW_PERMISSION + || mBalCode == BAL_ALLOW_VISIBLE_WINDOW) { return true; } } @@ -1948,12 +1954,6 @@ class ActivityStarter { return true; } - // TODO(b/230590090): Revisit this - ideally we would not rely on visibility, but rather - // have an explicit api for activities to opt-out of ASM protection if they need to. - if (mBalCode == BAL_ALLOW_VISIBLE_WINDOW) { - return true; - } - // ASM rules have failed. Log why ActivityRecord targetTopActivity = targetTask == null ? null : targetTask.getActivity(ar -> @@ -1988,7 +1988,7 @@ class ActivityStarter { /* action */ action, /* version */ - 3, + 4, /* multi_window - we have our source not in the target task, but both are visible */ targetTask != null && mSourceRecord != null && !targetTask.equals(mSourceRecord.getTask()) && targetTask.isVisible(), @@ -2106,7 +2106,8 @@ class ActivityStarter { reusedTask != null ? reusedTask.getTopNonFinishingActivity() : null, intentGrants); if (mAddingToTask) { - clearTopIfNeeded(targetTask, mCallingUid, mStartActivity.getUid(), mLaunchFlags); + clearTopIfNeeded(targetTask, mCallingUid, mRealCallingUid, mStartActivity.getUid(), + mLaunchFlags); return START_SUCCESS; } @@ -2139,52 +2140,58 @@ class ActivityStarter { } /** - * If the top activity uid does not match the launched activity, and the launch was not - * requested from the top uid, we want to clear out all non matching activities to prevent the - * top activity being sandwiched. + * If the top activity uid does not match the launching or launched activity, and the launch was + * not requested from the top uid, we want to clear out all non matching activities to prevent + * the top activity being sandwiched. + * + * Both creator and sender UID are considered for the launching activity. */ - private void clearTopIfNeeded(@NonNull Task targetTask, int callingUid, int startingUid, - int launchFlags) { + private void clearTopIfNeeded(@NonNull Task targetTask, int callingUid, int realCallingUid, + int startingUid, int launchFlags) { if ((launchFlags & FLAG_ACTIVITY_NEW_TASK) != FLAG_ACTIVITY_NEW_TASK) { // Launch is from the same task, so must be a top or privileged UID return; } - ActivityRecord targetTaskTop = targetTask.getTopNonFinishingActivity(); - if (targetTaskTop != null && targetTaskTop.getUid() != startingUid) { - boolean shouldBlockActivityStart = ActivitySecurityModelFeatureFlags - .shouldRestrictActivitySwitch(callingUid); - int[] finishCount = new int[0]; - if (shouldBlockActivityStart) { - ActivityRecord activity = targetTask.getActivity( - ar -> !ar.finishing && ar.isUid(startingUid)); + Predicate isLaunchingOrLaunched = ar -> !ar.finishing && (ar.isUid( + startingUid) || ar.isUid(callingUid) || ar.isUid(realCallingUid)); - if (activity == null) { - // mStartActivity is not in task, so clear everything - activity = mStartActivity; - } + // Return early if we know for sure we won't need to clear any activities by just checking + // the top activity. + ActivityRecord targetTaskTop = targetTask.getTopMostActivity(); + if (targetTaskTop == null || isLaunchingOrLaunched.test(targetTaskTop)) { + return; + } - finishCount = new int[1]; - if (activity != null) { - targetTask.performClearTop(activity, launchFlags, finishCount); - } - - if (finishCount[0] > 0) { - Slog.w(TAG, "Clearing top n: " + finishCount[0] + " activities from task t: " - + targetTask + " not matching top uid: " + callingUid); - } + // Find the first activity which matches a safe UID and is not finishing. Clear everything + // above it + boolean shouldBlockActivityStart = ActivitySecurityModelFeatureFlags + .shouldRestrictActivitySwitch(callingUid); + int[] finishCount = new int[0]; + if (shouldBlockActivityStart) { + ActivityRecord activity = targetTask.getActivity(isLaunchingOrLaunched); + if (activity == null) { + // mStartActivity is not in task, so clear everything + activity = mStartActivity; } - if (ActivitySecurityModelFeatureFlags.shouldShowToast(callingUid) - && (!shouldBlockActivityStart || finishCount[0] > 0)) { - UiThread.getHandler().post(() -> Toast.makeText(mService.mContext, - (shouldBlockActivityStart - ? "Top activities cleared by " - : "Top activities would be cleared by ") - + ActivitySecurityModelFeatureFlags.DOC_LINK, - Toast.LENGTH_SHORT).show()); + finishCount = new int[1]; + targetTask.performClearTop(activity, launchFlags, finishCount); + if (finishCount[0] > 0) { + Slog.w(TAG, "Cleared top n: " + finishCount[0] + " activities from task t: " + + targetTask + " not matching top uid: " + callingUid); } } + + if (ActivitySecurityModelFeatureFlags.shouldShowToast(callingUid) + && (!shouldBlockActivityStart || finishCount[0] > 0)) { + UiThread.getHandler().post(() -> Toast.makeText(mService.mContext, + (shouldBlockActivityStart + ? "Top activities cleared by " + : "Top activities would be cleared by ") + + ActivitySecurityModelFeatureFlags.DOC_LINK, + Toast.LENGTH_SHORT).show()); + } } /** @@ -2376,6 +2383,7 @@ class ActivityStarter { mStartActivity = null; mIntent = null; mCallingUid = -1; + mRealCallingUid = -1; mOptions = null; mBalCode = BAL_ALLOW_DEFAULT; @@ -2422,13 +2430,14 @@ class ActivityStarter { private void setInitialState(ActivityRecord r, ActivityOptions options, Task inTask, TaskFragment inTaskFragment, int startFlags, ActivityRecord sourceRecord, IVoiceInteractionSession voiceSession, - IVoiceInteractor voiceInteractor, @BalCode int balCode) { + IVoiceInteractor voiceInteractor, @BalCode int balCode, int realCallingUid) { reset(false /* clearRequest */); mStartActivity = r; mIntent = r.intent; mOptions = options; mCallingUid = r.launchedFromUid; + mRealCallingUid = realCallingUid; mSourceRecord = sourceRecord; mSourceRootTask = mSourceRecord != null ? mSourceRecord.getRootTask() : null; mVoiceSession = voiceSession; diff --git a/services/tests/wmtests/src/com/android/server/wm/ActivityStarterTests.java b/services/tests/wmtests/src/com/android/server/wm/ActivityStarterTests.java index 7b19e1f4db425..1981ac9f87cb9 100644 --- a/services/tests/wmtests/src/com/android/server/wm/ActivityStarterTests.java +++ b/services/tests/wmtests/src/com/android/server/wm/ActivityStarterTests.java @@ -1781,6 +1781,7 @@ public class ActivityStarterTests extends WindowTestsBase { starter.startActivityInner(target, source, null /* voiceSession */, null /* voiceInteractor */, 0 /* startFlags */, options, inTask, inTaskFragment, - BackgroundActivityStartController.BAL_ALLOW_DEFAULT, null /* intentGrants */); + BackgroundActivityStartController.BAL_ALLOW_DEFAULT, null /* intentGrants */, + -1 /* realCallingUid */); } } diff --git a/services/tests/wmtests/src/com/android/server/wm/DisplayWindowPolicyControllerTests.java b/services/tests/wmtests/src/com/android/server/wm/DisplayWindowPolicyControllerTests.java index 19d1d83f5e737..5dfb901a7c1e7 100644 --- a/services/tests/wmtests/src/com/android/server/wm/DisplayWindowPolicyControllerTests.java +++ b/services/tests/wmtests/src/com/android/server/wm/DisplayWindowPolicyControllerTests.java @@ -189,7 +189,8 @@ public class DisplayWindowPolicyControllerTests extends WindowTestsBase { /* inTask */null, /* inTaskFragment */ null, /* balCode */ BackgroundActivityStartController.BAL_ALLOW_DEFAULT, - /* intentGrants */null); + /* intentGrants */null, + /* realCaiingUid */ -1); assertEquals(result, START_ABORTED); } @@ -213,7 +214,8 @@ public class DisplayWindowPolicyControllerTests extends WindowTestsBase { /* inTask= */null, /* inTaskFragment= */ null, /* balCode= */ BackgroundActivityStartController.BAL_ALLOW_DEFAULT, - /* intentGrants= */null); + /* intentGrants= */null, + /* realCaiingUid */ -1); assertEquals(result, START_ABORTED); }