Merge "[Attempt #2] Restrict {AM,WM}.closeSystemDialogs()"

7 files changed, 128 insertions, 65 deletions

diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java
index 21b6b557169c..9e62e63ec1b7 100644
--- a/services/core/java/com/android/server/am/ActivityManagerService.java
+++ b/services/core/java/com/android/server/am/ActivityManagerService.java
@@ -174,7 +174,6 @@ import android.app.ProfilerInfo;
 import android.app.WaitResult;
 import android.app.backup.BackupManager.OperationType;
 import android.app.backup.IBackupManager;
-import android.app.compat.CompatChanges;
 import android.app.usage.UsageEvents;
 import android.app.usage.UsageEvents.Event;
 import android.app.usage.UsageStatsManager;
@@ -13710,34 +13709,10 @@ public class ActivityManagerService extends IActivityManager.Stub
                             false, false, userId, "package unstartable");
                     break;
                 case Intent.ACTION_CLOSE_SYSTEM_DIALOGS:
-                    if (!canCloseSystemDialogs(callingPid, callingUid, callerApp)) {
-                        // The app can't close system dialogs, throw only if it targets S+
-                        if (CompatChanges.isChangeEnabled(
-                                ActivityManager.LOCK_DOWN_CLOSE_SYSTEM_DIALOGS, callingUid)) {
-                            throw new SecurityException(
-                                    "Permission Denial: " + Intent.ACTION_CLOSE_SYSTEM_DIALOGS
-                                            + " broadcast from " + callerPackage + " (pid="
-                                            + callingPid + ", uid=" + callingUid + ")"
-                                            + " requires "
-                                            + permission.BROADCAST_CLOSE_SYSTEM_DIALOGS + ".");
-                        } else if (CompatChanges.isChangeEnabled(
-                                ActivityManager.DROP_CLOSE_SYSTEM_DIALOGS, callingUid)) {
-                            Slog.w(TAG, "Permission Denial: " + intent.getAction()
-                                    + " broadcast from " + callerPackage + " (pid=" + callingPid
-                                    + ", uid=" + callingUid + ")"
-                                    + " requires "
-                                    + permission.BROADCAST_CLOSE_SYSTEM_DIALOGS
-                                    + ", dropping broadcast.");
-                            // Returning success seems to be the pattern here
-                            return ActivityManager.BROADCAST_SUCCESS;
-                        } else {
-                            Slog.w(TAG, intent.getAction()
-                                    + " broadcast from " + callerPackage + " (pid=" + callingPid
-                                    + ", uid=" + callingUid + ")"
-                                    + " will require  "
-                                    + permission.BROADCAST_CLOSE_SYSTEM_DIALOGS
-                                    + " in future builds.");
-                        }
+                    if (!mAtmInternal.checkCanCloseSystemDialogs(callingPid, callingUid,
+                            callerPackage)) {
+                        // Returning success seems to be the pattern here
+                        return ActivityManager.BROADCAST_SUCCESS;
                     }
                     break;
             }
@@ -14032,39 +14007,6 @@ public class ActivityManagerService extends IActivityManager.Stub
         return ActivityManager.BROADCAST_SUCCESS;
     }
 
-    private boolean canCloseSystemDialogs(int pid, int uid, @Nullable ProcessRecord callerApp) {
-        if (checkPermission(permission.BROADCAST_CLOSE_SYSTEM_DIALOGS, pid, uid)
-                == PERMISSION_GRANTED) {
-            return true;
-        }
-        if (callerApp == null) {
-            synchronized (mPidsSelfLocked) {
-                callerApp = mPidsSelfLocked.get(pid);
-            }
-        }
-
-        if (callerApp != null) {
-            // Check if the instrumentation of the process has the permission. This covers the usual
-            // test started from the shell (which has the permission) case. This is needed for apps
-            // targeting SDK level < S but we are also allowing for targetSdk S+ as a convenience to
-            // avoid breaking a bunch of existing tests and asking them to adopt shell permissions
-            // to do this.
-            ActiveInstrumentation instrumentation = callerApp.getActiveInstrumentation();
-            if (instrumentation != null && checkPermission(
-                    permission.BROADCAST_CLOSE_SYSTEM_DIALOGS, -1, instrumentation.mSourceUid)
-                    == PERMISSION_GRANTED) {
-                return true;
-            }
-            // This is the notification trampoline use-case for example, where apps use Intent.ACSD
-            // to close the shade prior to starting an activity.
-            WindowProcessController wmApp = callerApp.getWindowProcessController();
-            if (wmApp.canCloseSystemDialogsByToken()) {
-                return true;
-            }
-        }
-        return false;
-    }
-
     /**
      * @return uid from the extra field {@link Intent#EXTRA_UID} if present, Otherwise -1
      */
diff --git a/services/core/java/com/android/server/am/ProcessRecord.java b/services/core/java/com/android/server/am/ProcessRecord.java
index 90abc0cb6e03..181a5cb12b09 100644
--- a/services/core/java/com/android/server/am/ProcessRecord.java
+++ b/services/core/java/com/android/server/am/ProcessRecord.java
@@ -1431,7 +1431,9 @@ class ProcessRecord implements WindowProcessListener {
     void setActiveInstrumentation(ActiveInstrumentation instr) {
         mInstr = instr;
         boolean isInstrumenting = instr != null;
-        mWindowProcessController.setInstrumenting(isInstrumenting,
+        mWindowProcessController.setInstrumenting(
+                isInstrumenting,
+                isInstrumenting ? instr.mSourceUid : -1,
                 isInstrumenting && instr.mHasBackgroundActivityStartsPermission);
     }
 
diff --git a/services/core/java/com/android/server/wm/ActivityTaskManagerInternal.java b/services/core/java/com/android/server/wm/ActivityTaskManagerInternal.java
index 762e1f62f886..109ea0954be6 100644
--- a/services/core/java/com/android/server/wm/ActivityTaskManagerInternal.java
+++ b/services/core/java/com/android/server/wm/ActivityTaskManagerInternal.java
@@ -285,6 +285,14 @@ public abstract class ActivityTaskManagerInternal {
     public abstract void enforceCallerIsRecentsOrHasPermission(String permission, String func);
 
     /**
+     * Returns true if the app can close system dialogs. Otherwise it either throws a {@link
+     * SecurityException} or returns false with a logcat message depending on whether the app
+     * targets SDK level {@link android.os.Build.VERSION_CODES#S} or not.
+     */
+    public abstract boolean checkCanCloseSystemDialogs(int pid, int uid,
+            @Nullable String packageName);
+
+    /**
      * Called after the voice interaction service has changed.
      */
     public abstract void notifyActiveVoiceInteractionServiceChanged(ComponentName component);
diff --git a/services/core/java/com/android/server/wm/ActivityTaskManagerService.java b/services/core/java/com/android/server/wm/ActivityTaskManagerService.java
index 698013cc82ee..039f22da86d8 100644
--- a/services/core/java/com/android/server/wm/ActivityTaskManagerService.java
+++ b/services/core/java/com/android/server/wm/ActivityTaskManagerService.java
@@ -147,6 +147,7 @@ import android.app.WindowConfiguration;
 import android.app.admin.DevicePolicyCache;
 import android.app.assist.AssistContent;
 import android.app.assist.AssistStructure;
+import android.app.compat.CompatChanges;
 import android.app.usage.UsageStatsManagerInternal;
 import android.content.ActivityNotFoundException;
 import android.content.ComponentName;
@@ -2900,6 +2901,86 @@ public class ActivityTaskManagerService extends IActivityTaskManager.Stub {
         }
     }
 
+    /**
+     * Returns true if the app can close system dialogs. Otherwise it either throws a {@link
+     * SecurityException} or returns false with a logcat message depending on whether the app
+     * targets SDK level {@link android.os.Build.VERSION_CODES#S} or not.
+     */
+    private boolean checkCanCloseSystemDialogs(int pid, int uid, @Nullable String packageName) {
+        final WindowProcessController process;
+        synchronized (mGlobalLock) {
+            process = mProcessMap.getProcess(pid);
+        }
+        if (packageName == null && process != null) {
+            // WindowProcessController.mInfo is final, so after the synchronized memory barrier
+            // above, process.mInfo can't change. As for reading mInfo.packageName,
+            // WindowProcessController doesn't own the ApplicationInfo object referenced by mInfo.
+            // ProcessRecord for example also holds a reference to that object, so protecting access
+            // to packageName with the WM lock would not be enough as we'd also need to synchronize
+            // on the AM lock if we are worried about races, but we can't synchronize on AM lock
+            // here. Hence, since this is only used for logging, we don't synchronize here.
+            packageName = process.mInfo.packageName;
+        }
+        String caller = "(pid=" + pid + ", uid=" + uid + ")";
+        if (packageName != null) {
+            caller = packageName + " " + caller;
+        }
+        if (!canCloseSystemDialogs(pid, uid, process)) {
+            // The app can't close system dialogs, throw only if it targets S+
+            if (CompatChanges.isChangeEnabled(
+                    ActivityManager.LOCK_DOWN_CLOSE_SYSTEM_DIALOGS, uid)) {
+                throw new SecurityException(
+                        "Permission Denial: " + Intent.ACTION_CLOSE_SYSTEM_DIALOGS
+                                + " broadcast from " + caller + " requires "
+                                + Manifest.permission.BROADCAST_CLOSE_SYSTEM_DIALOGS + ".");
+            } else if (CompatChanges.isChangeEnabled(
+                    ActivityManager.DROP_CLOSE_SYSTEM_DIALOGS, uid)) {
+                Slog.e(TAG,
+                        "Permission Denial: " + Intent.ACTION_CLOSE_SYSTEM_DIALOGS
+                                + " broadcast from " + caller + " requires "
+                                + Manifest.permission.BROADCAST_CLOSE_SYSTEM_DIALOGS
+                                + ", dropping broadcast.");
+                return false;
+            } else {
+                Slog.w(TAG, Intent.ACTION_CLOSE_SYSTEM_DIALOGS
+                        + " broadcast from " + caller + " will require "
+                        + Manifest.permission.BROADCAST_CLOSE_SYSTEM_DIALOGS
+                        + " in future builds.");
+                return true;
+            }
+        }
+        return true;
+    }
+
+    private boolean canCloseSystemDialogs(int pid, int uid,
+            @Nullable WindowProcessController process) {
+        if (checkPermission(Manifest.permission.BROADCAST_CLOSE_SYSTEM_DIALOGS, pid, uid)
+                == PERMISSION_GRANTED) {
+            return true;
+        }
+        if (process != null) {
+            // Check if the instrumentation of the process has the permission. This covers the
+            // usual test started from the shell (which has the permission) case. This is needed
+            // for apps targeting SDK level < S but we are also allowing for targetSdk S+ as a
+            // convenience to avoid breaking a bunch of existing tests and asking them to adopt
+            // shell permissions to do this.
+            // Note that these getters all read from volatile fields in WindowProcessController, so
+            // no need to lock.
+            int sourceUid = process.getInstrumentationSourceUid();
+            if (process.isInstrumenting() && sourceUid != -1 && checkPermission(
+                    Manifest.permission.BROADCAST_CLOSE_SYSTEM_DIALOGS, -1, sourceUid)
+                    == PERMISSION_GRANTED) {
+                return true;
+            }
+            // This is the notification trampoline use-case for example, where apps use Intent.ACSD
+            // to close the shade prior to starting an activity.
+            if (process.canCloseSystemDialogsByToken()) {
+                return true;
+            }
+        }
+        return false;
+    }
+
     static void enforceTaskPermission(String func) {
         if (checkCallingPermission(MANAGE_ACTIVITY_TASKS) == PackageManager.PERMISSION_GRANTED) {
             return;
@@ -5150,6 +5231,12 @@ public class ActivityTaskManagerService extends IActivityTaskManager.Stub {
         }
 
         @Override
+        public boolean checkCanCloseSystemDialogs(int pid, int uid, @Nullable String packageName) {
+            return ActivityTaskManagerService.this.checkCanCloseSystemDialogs(pid, uid,
+                    packageName);
+        }
+
+        @Override
         public void notifyActiveVoiceInteractionServiceChanged(ComponentName component) {
             synchronized (mGlobalLock) {
                 mActiveVoiceInteractionServiceComponent = component;
@@ -5649,9 +5736,12 @@ public class ActivityTaskManagerService extends IActivityTaskManager.Stub {
         @Override
         public void closeSystemDialogs(String reason) {
             enforceNotIsolatedCaller("closeSystemDialogs");
-
             final int pid = Binder.getCallingPid();
             final int uid = Binder.getCallingUid();
+            if (!checkCanCloseSystemDialogs(pid, uid, null)) {
+                return;
+            }
+
             final long origId = Binder.clearCallingIdentity();
             try {
                 synchronized (mGlobalLock) {
diff --git a/services/core/java/com/android/server/wm/WindowManagerService.java b/services/core/java/com/android/server/wm/WindowManagerService.java
index 4eeae6c0710c..a034bac993e0 100644
--- a/services/core/java/com/android/server/wm/WindowManagerService.java
+++ b/services/core/java/com/android/server/wm/WindowManagerService.java
@@ -3238,6 +3238,11 @@ public class WindowManagerService extends IWindowManager.Stub
 
     @Override
     public void closeSystemDialogs(String reason) {
+        int callingPid = Binder.getCallingPid();
+        int callingUid = Binder.getCallingUid();
+        if (!mAtmInternal.checkCanCloseSystemDialogs(callingPid, callingUid, null)) {
+            return;
+        }
         synchronized (mGlobalLock) {
             mRoot.closeSystemDialogs(reason);
         }
diff --git a/services/core/java/com/android/server/wm/WindowProcessController.java b/services/core/java/com/android/server/wm/WindowProcessController.java
index 8aa154bb9dd1..663d91ecc82a 100644
--- a/services/core/java/com/android/server/wm/WindowProcessController.java
+++ b/services/core/java/com/android/server/wm/WindowProcessController.java
@@ -23,6 +23,7 @@ import static android.os.IInputConstants.DEFAULT_DISPATCHING_TIMEOUT_MILLIS;
 import static android.view.Display.INVALID_DISPLAY;
 
 import static com.android.internal.protolog.ProtoLogGroup.WM_DEBUG_CONFIGURATION;
+import static com.android.internal.util.Preconditions.checkArgument;
 import static com.android.server.am.ActivityManagerService.MY_PID;
 import static com.android.server.wm.ActivityTaskManagerDebugConfig.DEBUG_ACTIVITY_STARTS;
 import static com.android.server.wm.ActivityTaskManagerDebugConfig.DEBUG_RELEASE;
@@ -161,6 +162,8 @@ public class WindowProcessController extends ConfigurationContainer<Configuratio
     private volatile boolean mDebugging;
     // Active instrumentation running in process?
     private volatile boolean mInstrumenting;
+    // If there is active instrumentation, this is the source
+    private volatile int mInstrumentationSourceUid = -1;
     // Active instrumentation with background activity starts privilege running in process?
     private volatile boolean mInstrumentingWithBackgroundActivityStartPrivileges;
     // This process it perceptible by the user.
@@ -623,9 +626,16 @@ public class WindowProcessController extends ConfigurationContainer<Configuratio
         mBoundClientUids = boundClientUids;
     }
 
-    public void setInstrumenting(boolean instrumenting,
+    /**
+     * Set instrumentation-related info.
+     *
+     * If {@code instrumenting} is {@code false}, {@code sourceUid} has to be -1.
+     */
+    public void setInstrumenting(boolean instrumenting, int sourceUid,
             boolean hasBackgroundActivityStartPrivileges) {
+        checkArgument(instrumenting || sourceUid == -1);
         mInstrumenting = instrumenting;
+        mInstrumentationSourceUid = sourceUid;
         mInstrumentingWithBackgroundActivityStartPrivileges = hasBackgroundActivityStartPrivileges;
     }
 
@@ -633,6 +643,11 @@ public class WindowProcessController extends ConfigurationContainer<Configuratio
         return mInstrumenting;
     }
 
+    /** Returns the uid of the active instrumentation source if there is one, otherwise -1. */
+    int getInstrumentationSourceUid() {
+        return mInstrumentationSourceUid;
+    }
+
     public void setPerceptible(boolean perceptible) {
         mPerceptible = perceptible;
     }
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 e8045e06d9ff..fee848b1a10d 100644
--- a/services/tests/wmtests/src/com/android/server/wm/ActivityStarterTests.java
+++ b/services/tests/wmtests/src/com/android/server/wm/ActivityStarterTests.java
@@ -720,6 +720,7 @@ public class ActivityStarterTests extends WindowTestsBase {
         }
         // caller is instrumenting with background activity starts privileges
         callerApp.setInstrumenting(callerIsInstrumentingWithBackgroundActivityStartPrivileges,
+                callerIsInstrumentingWithBackgroundActivityStartPrivileges ? Process.SHELL_UID : -1,
                 callerIsInstrumentingWithBackgroundActivityStartPrivileges);
         // callingUid is the device owner
         doReturn(isCallingUidDeviceOwner).when(mAtm).isDeviceOwner(callingUid);