Revert "BG-FGS-start while-in-use permission restriction improve..."

Hui Yu · Android Build Coastguard Worker · commit a95cfc3ed8cf · 2021-08-27T22:04:20.000Z
Revert "Add test cases for background startForeground() improvement." Revert submission 15081873-cherrypick-security_backport_183147114-an8dvy98fv Reason for revert: https://b.corp.google.com/issues/197066403#comment15 After the app calls Service.startForeground() to put the app into foreground service mode, the service has mAllowWhileInUsePermissionInFgs set, if it is true, the app/service can access location/camera/micophone. Some apps may call Service.startForeground() again (for any reason, could be a app's unintentional redundant call, could be that app want to update the notification of the foreground service), but at this moment the app has went into background mode, although the foreground service is still running. The second or later Service.startForeground() call may set mAllowWhileInUsePermissionInFgs to false and the app may lose location/camera/micophone access. This is incorrect because the foreground service is still running and we expect location/camera/micophone access to continue. The Samsung Voice Recorder app has run into this situation. Reverted Changes: I0aca484e5:BG-FGS-start while-in-use permission restriction i... I4988dbba1:Add test cases for background startForeground() im... Bug: 183147114 Bug: 197066403 Change-Id: Iad32e4391fa15bc252e50f9b858fe2e5225edb19 Merged-In: Idc88f274c7a323d175d65bb47eca041772ae9bb7 (cherry picked from commit c83a949722d0b879fb7ac73f3740e45d08b8efa6)
diff --git a/services/core/java/com/android/server/am/ActiveServices.java b/services/core/java/com/android/server/am/ActiveServices.java
@@ -734,8 +734,11 @@ ComponentName startServiceLocked(IApplicationThread caller, Intent service, Stri
         }
         ComponentName cmp = startServiceInnerLocked(smap, service, r, callerFg, addToStarting);
 
-        setFgsRestrictionLocked(callingPackage, callingPid, callingUid, r,
-                allowBackgroundActivityStarts);
+        if (!r.mAllowWhileInUsePermissionInFgs) {
+            r.mAllowWhileInUsePermissionInFgs =
+                    shouldAllowWhileInUsePermissionInFgsLocked(callingPackage, callingPid,
+                            callingUid, service, r, allowBackgroundActivityStarts);
+        }
 
         return cmp;
     }
@@ -1397,6 +1400,14 @@ private void setServiceForegroundInnerLocked(final ServiceRecord r, int id,
                         +  String.format("0x%08X", manifestType)
                         + " in service element of manifest file");
                 }
+                // If the foreground service is not started from TOP process, do not allow it to
+                // have while-in-use location/camera/microphone access.
+                if (!r.mAllowWhileInUsePermissionInFgs) {
+                    Slog.w(TAG,
+                            "Foreground service started from background can not have "
+                                    + "location/camera/microphone access: service "
+                                    + r.shortInstanceName);
+                }
             }
             boolean alreadyStartedOp = false;
             boolean stopProcStatsOp = false;
@@ -1444,57 +1455,6 @@ && appRestrictedAnyInBackground(r.appInfo.uid, r.packageName)) {
                     ignoreForeground = true;
                 }
 
-                if (!ignoreForeground) {
-                    if (r.mStartForegroundCount == 0) {
-                        /*
-                        If the service was started with startService(), not
-                        startForegroundService(), and if startForeground() isn't called within
-                        mFgsStartForegroundTimeoutMs, then we check the state of the app
-                        (who owns the service, which is the app that called startForeground())
-                        again. If the app is in the foreground, or in any other cases where
-                        FGS-starts are allowed, then we still allow the FGS to be started.
-                        Otherwise, startForeground() would fail.
-
-                        If the service was started with startForegroundService(), then the service
-                        must call startForeground() within a timeout anyway, so we don't need this
-                        check.
-                        */
-                        if (!r.fgRequired) {
-                            final long delayMs = SystemClock.elapsedRealtime() - r.createRealTime;
-                            if (delayMs > mAm.mConstants.mFgsStartForegroundTimeoutMs) {
-                                resetFgsRestrictionLocked(r);
-                                setFgsRestrictionLocked(r.serviceInfo.packageName, r.app.pid,
-                                        r.appInfo.uid, r, false);
-                                EventLog.writeEvent(0x534e4554, "183147114",
-                                        r.appInfo.uid,
-                                        "call setFgsRestrictionLocked again due to "
-                                                + "startForegroundTimeout");
-                            }
-                        }
-                    } else if (r.mStartForegroundCount >= 1) {
-                        // The second or later time startForeground() is called after service is
-                        // started. Check for app state again.
-                        final long delayMs = SystemClock.elapsedRealtime() -
-                                r.mLastSetFgsRestrictionTime;
-                        if (delayMs > mAm.mConstants.mFgsStartForegroundTimeoutMs) {
-                            resetFgsRestrictionLocked(r);
-                            setFgsRestrictionLocked(r.serviceInfo.packageName, r.app.pid,
-                                    r.appInfo.uid, r, false);
-                            EventLog.writeEvent(0x534e4554, "183147114", r.appInfo.uid,
-                                    "call setFgsRestrictionLocked for "
-                                            + (r.mStartForegroundCount + 1) + "th startForeground");
-                        }
-                    }
-                    // If the foreground service is not started from TOP process, do not allow it to
-                    // have while-in-use location/camera/microphone access.
-                    if (!r.mAllowWhileInUsePermissionInFgs) {
-                        Slog.w(TAG,
-                                "Foreground service started from background can not have "
-                                        + "location/camera/microphone access: service "
-                                        + r.shortInstanceName);
-                    }
-                }
-
                 // Apps under strict background restrictions simply don't get to have foreground
                 // services, so now that we've enforced the startForegroundService() contract
                 // we only do the machinery of making the service foreground when the app
@@ -1530,7 +1490,6 @@ must call startForeground() within a timeout anyway, so we don't need this
                             active.mNumActive++;
                         }
                         r.isForeground = true;
-                        r.mStartForegroundCount++;
                         if (!stopProcStatsOp) {
                             ServiceState stracker = r.getTracker();
                             if (stracker != null) {
@@ -1589,7 +1548,6 @@ must call startForeground() within a timeout anyway, so we don't need this
                     decActiveForegroundAppLocked(smap, r);
                 }
                 r.isForeground = false;
-                resetFgsRestrictionLocked(r);
                 ServiceState stracker = r.getTracker();
                 if (stracker != null) {
                     stracker.setForeground(false, mAm.mProcessStats.getMemFactorLocked(),
@@ -2149,7 +2107,12 @@ public void run() {
                 }
             }
 
-            setFgsRestrictionLocked(callingPackage, callingPid, callingUid, s, false);
+            if (!s.mAllowWhileInUsePermissionInFgs) {
+                s.mAllowWhileInUsePermissionInFgs =
+                        shouldAllowWhileInUsePermissionInFgsLocked(callingPackage,
+                                callingPid, callingUid,
+                                service, s, false);
+            }
 
             if (s.app != null) {
                 if ((flags&Context.BIND_TREAT_LIKE_ACTIVITY) != 0) {
@@ -3445,7 +3408,7 @@ private final void bringDownServiceLocked(ServiceRecord r) {
         r.isForeground = false;
         r.foregroundId = 0;
         r.foregroundNoti = null;
-        resetFgsRestrictionLocked(r);
+        r.mAllowWhileInUsePermissionInFgs = false;
 
         // Clear start entries.
         r.clearDeliveredStartsLocked();
@@ -4926,7 +4889,7 @@ private void dumpService(String prefix, FileDescriptor fd, PrintWriter pw,
      * @return true if allow, false otherwise.
      */
     private boolean shouldAllowWhileInUsePermissionInFgsLocked(String callingPackage,
-            int callingPid, int callingUid, ServiceRecord r,
+            int callingPid, int callingUid, Intent intent, ServiceRecord r,
             boolean allowBackgroundActivityStarts) {
         // Is the background FGS start restriction turned on?
         if (!mAm.mConstants.mFlagBackgroundFgsStartRestrictionEnabled) {
@@ -4997,32 +4960,4 @@ private boolean shouldAllowWhileInUsePermissionInFgsLocked(String callingPackage
         }
         return false;
     }
-
-    boolean canAllowWhileInUsePermissionInFgsLocked(int callingPid, int callingUid,
-            String callingPackage) {
-        return shouldAllowWhileInUsePermissionInFgsLocked(
-                callingPackage, callingPid, callingUid, null, false);
-    }
-
-    /**
-     * In R, mAllowWhileInUsePermissionInFgs is to allow while-in-use permissions in foreground
-     *  service or not. while-in-use permissions in FGS started from background might be restricted.
-     * @param callingPackage caller app's package name.
-     * @param callingUid caller app's uid.
-     * @param r the service to start.
-     * @return true if allow, false otherwise.
-     */
-    private void setFgsRestrictionLocked(String callingPackage,
-            int callingPid, int callingUid, ServiceRecord r,
-            boolean allowBackgroundActivityStarts) {
-        r.mLastSetFgsRestrictionTime = SystemClock.elapsedRealtime();
-        if (!r.mAllowWhileInUsePermissionInFgs) {
-            r.mAllowWhileInUsePermissionInFgs = shouldAllowWhileInUsePermissionInFgsLocked(
-                    callingPackage, callingPid, callingUid, r, allowBackgroundActivityStarts);
-        }
-    }
-
-    private void resetFgsRestrictionLocked(ServiceRecord r) {
-        r.mAllowWhileInUsePermissionInFgs = false;
-    }
 }
diff --git a/services/core/java/com/android/server/am/ActivityManagerConstants.java b/services/core/java/com/android/server/am/ActivityManagerConstants.java
@@ -87,7 +87,6 @@ final class ActivityManagerConstants extends ContentObserver {
     static final String KEY_PROCESS_START_ASYNC = "process_start_async";
     static final String KEY_MEMORY_INFO_THROTTLE_TIME = "memory_info_throttle_time";
     static final String KEY_TOP_TO_FGS_GRACE_DURATION = "top_to_fgs_grace_duration";
-    static final String KEY_FGS_START_FOREGROUND_TIMEOUT = "fgs_start_foreground_timeout";
     static final String KEY_PENDINGINTENT_WARNING_THRESHOLD = "pendingintent_warning_threshold";
 
     private static final int DEFAULT_MAX_CACHED_PROCESSES = 32;
@@ -121,7 +120,6 @@ final class ActivityManagerConstants extends ContentObserver {
     private static final boolean DEFAULT_PROCESS_START_ASYNC = true;
     private static final long DEFAULT_MEMORY_INFO_THROTTLE_TIME = 5*60*1000;
     private static final long DEFAULT_TOP_TO_FGS_GRACE_DURATION = 15 * 1000;
-    private static final int DEFAULT_FGS_START_FOREGROUND_TIMEOUT_MS = 10 * 1000;
     private static final int DEFAULT_PENDINGINTENT_WARNING_THRESHOLD = 2000;
 
     // Flag stored in the DeviceConfig API.
@@ -274,12 +272,6 @@ final class ActivityManagerConstants extends ContentObserver {
     // this long.
     public long TOP_TO_FGS_GRACE_DURATION = DEFAULT_TOP_TO_FGS_GRACE_DURATION;
 
-    /**
-     * When service started from background, before the timeout it can be promoted to FGS by calling
-     * Service.startForeground().
-     */
-    volatile long mFgsStartForegroundTimeoutMs = DEFAULT_FGS_START_FOREGROUND_TIMEOUT_MS;
-
     // Indicates whether the activity starts logging is enabled.
     // Controlled by Settings.Global.ACTIVITY_STARTS_LOGGING_ENABLED
     volatile boolean mFlagActivityStartsLoggingEnabled;
@@ -423,9 +415,6 @@ public void onPropertiesChanged(Properties properties) {
                             case KEY_MIN_ASSOC_LOG_DURATION:
                                 updateMinAssocLogDuration();
                                 break;
-                            case KEY_FGS_START_FOREGROUND_TIMEOUT:
-                                updateFgsStartForegroundTimeout();
-                                break;
                             default:
                                 break;
                         }
@@ -698,13 +687,6 @@ private void updateMinAssocLogDuration() {
                 /* defaultValue */ DEFAULT_MIN_ASSOC_LOG_DURATION);
     }
 
-    private void updateFgsStartForegroundTimeout() {
-        mFgsStartForegroundTimeoutMs = DeviceConfig.getLong(
-                DeviceConfig.NAMESPACE_ACTIVITY_MANAGER,
-                KEY_FGS_START_FOREGROUND_TIMEOUT,
-                DEFAULT_FGS_START_FOREGROUND_TIMEOUT_MS);
-    }
-
     void dump(PrintWriter pw) {
         pw.println("ACTIVITY MANAGER SETTINGS (dumpsys activity settings) "
                 + Settings.Global.ACTIVITY_MANAGER_CONSTANTS + ":");
@@ -777,8 +759,6 @@ void dump(PrintWriter pw) {
         pw.println(Arrays.toString(IMPERCEPTIBLE_KILL_EXEMPT_PACKAGES.toArray()));
         pw.print("  "); pw.print(KEY_MIN_ASSOC_LOG_DURATION); pw.print("=");
         pw.println(MIN_ASSOC_LOG_DURATION);
-        pw.print("  "); pw.print(KEY_FGS_START_FOREGROUND_TIMEOUT); pw.print("=");
-        pw.println(mFgsStartForegroundTimeoutMs);
 
         pw.println();
         if (mOverrideMaxCachedProcesses >= 0) {
diff --git a/services/core/java/com/android/server/am/ServiceRecord.java b/services/core/java/com/android/server/am/ServiceRecord.java
@@ -138,10 +138,6 @@ final class ServiceRecord extends Binder implements ComponentName.WithComponentN
     // allow while-in-use permissions in foreground service or not.
     // while-in-use permissions in FGS started from background might be restricted.
     boolean mAllowWhileInUsePermissionInFgs;
-    // The number of times Service.startForeground() is called;
-    int mStartForegroundCount;
-    // Last time mAllowWhileInUsePermissionInFgs is set.
-    long mLastSetFgsRestrictionTime;
 
     // the most recent package that start/bind this service.
     String mRecentCallingPackage;
@@ -404,8 +400,6 @@ void dump(PrintWriter pw, String prefix) {
         }
         pw.print(prefix); pw.print("allowWhileInUsePermissionInFgs=");
                 pw.println(mAllowWhileInUsePermissionInFgs);
-        pw.print(prefix); pw.print("startForegroundCount=");
-        pw.println(mStartForegroundCount);
         pw.print(prefix); pw.print("recentCallingPackage=");
                 pw.println(mRecentCallingPackage);
         if (delayed) {
