From 874c974f73839da761177a4e0a53b7f4a7d29288 Mon Sep 17 00:00:00 2001 From: Christopher Tate Date: Mon, 3 Feb 2020 18:35:13 -0800 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 | 12 ++++++++- .../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 | 23 +++++++++------- 7 files changed, 53 insertions(+), 25 deletions(-) diff --git a/core/java/android/app/IActivityManager.aidl b/core/java/android/app/IActivityManager.aidl index 18117481b0eab..d29a332ae1971 100644 --- a/core/java/android/app/IActivityManager.aidl +++ b/core/java/android/app/IActivityManager.aidl @@ -266,7 +266,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 9d823a726afef..8e0bf6fada37d 100644 --- a/services/core/java/com/android/server/am/ActiveServices.java +++ b/services/core/java/com/android/server/am/ActiveServices.java @@ -653,6 +653,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(), @@ -3391,7 +3400,8 @@ public final class ActiveServices { void serviceForegroundCrash(ProcessRecord app) { mAm.crashApplication(app.uid, app.pid, app.info.packageName, app.userId, - "Context.startForegroundService() did not then call Service.startForeground()"); + "Context.startForegroundService() did not then call Service.startForeground()", + 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 b4c18b13947c8..0d9fc03208db8 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -5141,7 +5141,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=" @@ -5153,7 +5153,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 8488e526eba9b..9575fddf04f20 100644 --- a/services/core/java/com/android/server/am/ActivityManagerShellCommand.java +++ b/services/core/java/com/android/server/am/ActivityManagerShellCommand.java @@ -921,7 +921,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 a842724c31778..a7954bbd624d6 100644 --- a/services/core/java/com/android/server/am/AppErrors.java +++ b/services/core/java/com/android/server/am/AppErrors.java @@ -243,20 +243,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); } } @@ -270,7 +274,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 @@ -303,6 +307,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 16995e50fdbf9..f87ba716926e8 100644 --- a/services/core/java/com/android/server/am/ServiceRecord.java +++ b/services/core/java/com/android/server/am/ServiceRecord.java @@ -453,6 +453,7 @@ final class ServiceRecord extends Binder { 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( @@ -551,10 +552,8 @@ final class ServiceRecord extends Binder { 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 5df6083bb2233..797ce37af5609 100644 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -714,18 +714,23 @@ 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) { - Slog.d(TAG, "onNotification error pkg=" + pkg + " tag=" + tag + " id=" + id - + "; will crashApplication(uid=" + uid + ", pid=" + initialPid + ")"); + final boolean fgService; + synchronized (mNotificationLock) { + NotificationRecord r = findNotificationLocked(pkg, tag, id, userId); + fgService = r != null + && (r.getNotification().flags&Notification.FLAG_FOREGROUND_SERVICE) != 0; + } cancelNotification(callingUid, callingPid, pkg, tag, id, 0, 0, false, userId, REASON_ERROR, null); - long ident = Binder.clearCallingIdentity(); - try { - ActivityManager.getService().crashApplication(uid, initialPid, pkg, -1, - "Bad notification posted from package " + pkg - + ": " + message); - } catch (RemoteException e) { + 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 */)); } - Binder.restoreCallingIdentity(ident); } @Override