From a79b6ba5c59dc6aaa8adbe1ffa3ee4b761f45e7f Mon Sep 17 00:00:00 2001 From: Christopher Tate Date: Mon, 19 Aug 2019 16:16:20 -0700 Subject: [PATCH] DO NOT MERGE - Kill apps outright for API contract violations ...rather than relying on in-app code to perform the shutdown. Backport of security fix. Bug: 128649910 Bug: 140108616 Test: manual Test: atest OsHostTests#testForegroundServiceBadNotification Change-Id: I94d9de50bb03c33666471e3dbd9c721e9278f7cb Merged-In: I94d9de50bb03c33666471e3dbd9c721e9278f7cb --- core/java/android/app/IActivityManager.aidl | 3 ++- .../com/android/server/am/ActiveServices.java | 11 +++++++- .../server/am/ActivityManagerService.java | 5 ++-- .../am/ActivityManagerShellCommand.java | 2 +- .../java/com/android/server/am/AppErrors.java | 26 ++++++++++++++----- .../com/android/server/am/ServiceRecord.java | 7 +++-- .../NotificationManagerService.java | 14 ++++++++++ 7 files changed, 52 insertions(+), 16 deletions(-) diff --git a/core/java/android/app/IActivityManager.aidl b/core/java/android/app/IActivityManager.aidl index b192021f821b9..3f7f3b73c4171 100644 --- a/core/java/android/app/IActivityManager.aidl +++ b/core/java/android/app/IActivityManager.aidl @@ -278,7 +278,8 @@ interface IActivityManager { boolean isImmersive(in IBinder token); void setImmersive(in IBinder token, boolean immersive); boolean isTopActivityImmersive(); - void crashApplication(int uid, int initialPid, in String packageName, int userId, in String message); + void crashApplication(int uid, int initialPid, in String packageName, int userId, + in String message, boolean force); String getProviderMimeType(in Uri uri, int userId); IBinder newUriPermissionOwner(in String name); void grantUriPermissionFromOwner(in IBinder owner, int fromUid, in String targetPkg, diff --git a/services/core/java/com/android/server/am/ActiveServices.java b/services/core/java/com/android/server/am/ActiveServices.java index ca715b51a328b..b49d07f3e46cb 100644 --- a/services/core/java/com/android/server/am/ActiveServices.java +++ b/services/core/java/com/android/server/am/ActiveServices.java @@ -787,6 +787,15 @@ public final class ActiveServices { } } + void killMisbehavingService(ServiceRecord r, + int appUid, int appPid, String localPackageName) { + synchronized (mAm) { + stopServiceLocked(r); + mAm.crashApplication(appUid, appPid, localPackageName, -1, + "Bad notification for startForeground", true /*force*/); + } + } + IBinder peekServiceLocked(Intent service, String resolvedType, String callingPackage) { ServiceLookupResult r = retrieveServiceLocked(service, resolvedType, callingPackage, Binder.getCallingPid(), Binder.getCallingUid(), @@ -3655,7 +3664,7 @@ public final class ActiveServices { void serviceForegroundCrash(ProcessRecord app, CharSequence serviceRecord) { mAm.crashApplication(app.uid, app.pid, app.info.packageName, app.userId, "Context.startForegroundService() did not then call Service.startForeground(): " - + serviceRecord); + + serviceRecord, false /*force*/); } void scheduleServiceTimeoutLocked(ProcessRecord proc) { diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index 7c57c43533afd..3dea717a99821 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -5773,7 +5773,7 @@ public class ActivityManagerService extends IActivityManager.Stub @Override public void crashApplication(int uid, int initialPid, String packageName, int userId, - String message) { + String message, boolean force) { if (checkCallingPermission(android.Manifest.permission.FORCE_STOP_PACKAGES) != PackageManager.PERMISSION_GRANTED) { String msg = "Permission Denial: crashApplication() from pid=" @@ -5785,7 +5785,8 @@ public class ActivityManagerService extends IActivityManager.Stub } synchronized(this) { - mAppErrors.scheduleAppCrashLocked(uid, initialPid, packageName, userId, message); + mAppErrors.scheduleAppCrashLocked(uid, initialPid, packageName, userId, + message, force); } } diff --git a/services/core/java/com/android/server/am/ActivityManagerShellCommand.java b/services/core/java/com/android/server/am/ActivityManagerShellCommand.java index ccbc8ed32a9c0..adb7056408ba9 100644 --- a/services/core/java/com/android/server/am/ActivityManagerShellCommand.java +++ b/services/core/java/com/android/server/am/ActivityManagerShellCommand.java @@ -987,7 +987,7 @@ final class ActivityManagerShellCommand extends ShellCommand { } catch (NumberFormatException e) { packageName = arg; } - mInterface.crashApplication(-1, pid, packageName, userId, "shell-induced crash"); + mInterface.crashApplication(-1, pid, packageName, userId, "shell-induced crash", false); return 0; } diff --git a/services/core/java/com/android/server/am/AppErrors.java b/services/core/java/com/android/server/am/AppErrors.java index f0e2876b47610..0b8fca9d8e712 100644 --- a/services/core/java/com/android/server/am/AppErrors.java +++ b/services/core/java/com/android/server/am/AppErrors.java @@ -314,20 +314,24 @@ class AppErrors { } void killAppAtUserRequestLocked(ProcessRecord app, Dialog fromDialog) { - app.crashing = false; - app.crashingReport = null; - app.notResponding = false; - app.notRespondingReport = null; if (app.anrDialog == fromDialog) { app.anrDialog = null; } if (app.waitDialog == fromDialog) { app.waitDialog = null; } + killAppImmediateLocked(app, "user-terminated", "user request after error"); + } + + private void killAppImmediateLocked(ProcessRecord app, String reason, String killReason) { + app.crashing = false; + app.crashingReport = null; + app.notResponding = false; + app.notRespondingReport = null; if (app.pid > 0 && app.pid != MY_PID) { - handleAppCrashLocked(app, "user-terminated" /*reason*/, + handleAppCrashLocked(app, reason, null /*shortMsg*/, null /*longMsg*/, null /*stackTrace*/, null /*data*/); - app.kill("user request after error", true); + app.kill(killReason, true); } } @@ -341,7 +345,7 @@ class AppErrors { * @param message */ void scheduleAppCrashLocked(int uid, int initialPid, String packageName, int userId, - String message) { + String message, boolean force) { ProcessRecord proc = null; // Figure out which process to kill. We don't trust that initialPid @@ -374,6 +378,14 @@ class AppErrors { } proc.scheduleCrash(message); + if (force) { + // If the app is responsive, the scheduled crash will happen as expected + // and then the delayed summary kill will be a no-op. + final ProcessRecord p = proc; + mService.mHandler.postDelayed( + () -> killAppImmediateLocked(p, "forced", "killed for invalid state"), + 5000L); + } } /** diff --git a/services/core/java/com/android/server/am/ServiceRecord.java b/services/core/java/com/android/server/am/ServiceRecord.java index 4d89d015b6694..925a4d97fa324 100644 --- a/services/core/java/com/android/server/am/ServiceRecord.java +++ b/services/core/java/com/android/server/am/ServiceRecord.java @@ -584,6 +584,7 @@ final class ServiceRecord extends Binder implements ComponentName.WithComponentN final String localPackageName = packageName; final int localForegroundId = foregroundId; final Notification _foregroundNoti = foregroundNoti; + final ServiceRecord record = this; ams.mHandler.post(new Runnable() { public void run() { NotificationManagerInternal nm = LocalServices.getService( @@ -682,10 +683,8 @@ final class ServiceRecord extends Binder implements ComponentName.WithComponentN Slog.w(TAG, "Error showing notification for service", e); // If it gave us a garbage notification, it doesn't // get to be foreground. - ams.setServiceForeground(name, ServiceRecord.this, - 0, null, 0); - ams.crashApplication(appUid, appPid, localPackageName, -1, - "Bad notification for startForeground: " + e); + ams.mServices.killMisbehavingService(record, + appUid, appPid, localPackageName); } } }); diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index b1ea47c6f3b09..c4647096f61c1 100644 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -798,8 +798,22 @@ public class NotificationManagerService extends SystemService { @Override public void onNotificationError(int callingUid, int callingPid, String pkg, String tag, int id, int uid, int initialPid, String message, int userId) { + final boolean fgService; + synchronized (mNotificationLock) { + NotificationRecord r = findNotificationLocked(pkg, tag, id, userId); + fgService = r != null && (r.getNotification().flags & FLAG_FOREGROUND_SERVICE) != 0; + } cancelNotification(callingUid, callingPid, pkg, tag, id, 0, 0, false, userId, REASON_ERROR, null); + if (fgService) { + // Still crash for foreground services, preventing the not-crash behaviour abused + // by apps to give us a garbage notification and silently start a fg service. + Binder.withCleanCallingIdentity( + () -> mAm.crashApplication(uid, initialPid, pkg, -1, + "Bad notification(tag=" + tag + ", id=" + id + ") posted from package " + + pkg + ", crashing app(uid=" + uid + ", pid=" + initialPid + "): " + + message, true /* force */)); + } } @Override