From 6466c1cc5efc4ff05fabdd670cf78a6a7ca45afb Mon Sep 17 00:00:00 2001 From: Dianne Hackborn Date: Tue, 13 Jun 2017 10:33:19 -0700 Subject: [PATCH] Fix issue #62390590: SecurityException in JobIntentService$... ...JobServiceEngineImpl$WrapperWorkItem.complete When a job is in the process of stopping a job, we should no longer allow new work to be dispatched for it. Also there was an issue with how we determine wether a caller is valid for the current job -- this was only based on the uid of the currently running job, but of course that context could be re-used for a new job of the same uid. Instead, we now create a different callback binder for each client, so we can identify the exact client each time it calls back. (This also allows us to hang information about why the job last stopped on that client state, so we can always report it.) Finally make a bunch of classes final that should have been. Test: bit CtsJobSchedulerTestCases:* Change-Id: I1b00dd2da710414dd2898c4d39a5c528d54b95ea --- .../server/job/GrantedUriPermissions.java | 2 +- .../server/job/JobSchedulerShellCommand.java | 2 +- .../android/server/job/JobServiceContext.java | 101 ++++++++++++------ .../java/com/android/server/job/JobStore.java | 8 +- .../job/controllers/AppIdleController.java | 4 +- .../job/controllers/BatteryController.java | 4 +- .../controllers/ConnectivityController.java | 2 +- .../ContentObserverController.java | 2 +- .../controllers/DeviceIdleJobsController.java | 2 +- .../job/controllers/IdleController.java | 4 +- .../job/controllers/StorageController.java | 4 +- .../job/controllers/TimeController.java | 2 +- 12 files changed, 86 insertions(+), 51 deletions(-) diff --git a/services/core/java/com/android/server/job/GrantedUriPermissions.java b/services/core/java/com/android/server/job/GrantedUriPermissions.java index e413d8d1d8c08..c23b109bb9dec 100644 --- a/services/core/java/com/android/server/job/GrantedUriPermissions.java +++ b/services/core/java/com/android/server/job/GrantedUriPermissions.java @@ -29,7 +29,7 @@ import android.util.Slog; import java.io.PrintWriter; import java.util.ArrayList; -public class GrantedUriPermissions { +public final class GrantedUriPermissions { private final int mGrantFlags; private final int mSourceUserId; private final String mTag; diff --git a/services/core/java/com/android/server/job/JobSchedulerShellCommand.java b/services/core/java/com/android/server/job/JobSchedulerShellCommand.java index 2d2f61f0288dd..a53c0885f334d 100644 --- a/services/core/java/com/android/server/job/JobSchedulerShellCommand.java +++ b/services/core/java/com/android/server/job/JobSchedulerShellCommand.java @@ -26,7 +26,7 @@ import android.os.UserHandle; import java.io.PrintWriter; -public class JobSchedulerShellCommand extends ShellCommand { +public final class JobSchedulerShellCommand extends ShellCommand { public static final int CMD_ERR_NO_PACKAGE = -1000; public static final int CMD_ERR_NO_JOB = -1001; public static final int CMD_ERR_CONSTRAINTS = -1002; diff --git a/services/core/java/com/android/server/job/JobServiceContext.java b/services/core/java/com/android/server/job/JobServiceContext.java index 637db11af6d8e..5d3f6f7b804b6 100644 --- a/services/core/java/com/android/server/job/JobServiceContext.java +++ b/services/core/java/com/android/server/job/JobServiceContext.java @@ -55,13 +55,13 @@ import com.android.server.job.controllers.JobStatus; * job lands, and again when it is complete. * - Cancelling is trickier, because there are also interactions from the client. It's possible * the {@link com.android.server.job.JobServiceContext.JobServiceHandler} tries to process a - * {@link #doCancelLocked(int)} after the client has already finished. This is handled by having + * {@link #doCancelLocked} after the client has already finished. This is handled by having * {@link com.android.server.job.JobServiceContext.JobServiceHandler#handleCancelLocked} check whether * the context is still valid. * To mitigate this, we avoid sending duplicate onStopJob() * calls to the client after they've specified jobFinished(). */ -public class JobServiceContext extends IJobCallback.Stub implements ServiceConnection { +public final class JobServiceContext implements ServiceConnection { private static final boolean DEBUG = JobSchedulerService.DEBUG; private static final String TAG = "JobServiceContext"; /** Amount of time a job is allowed to execute for before being considered timed-out. */ @@ -112,6 +112,7 @@ public class JobServiceContext extends IJobCallback.Stub implements ServiceConne * Writes can only be done from the handler thread, or {@link #executeRunnableJob(JobStatus)}. */ private JobStatus mRunningJob; + private JobCallback mRunningCallback; /** Used to store next job to run when current job is to be preempted. */ private int mPreferredUid; IJobService service; @@ -133,6 +134,36 @@ public class JobServiceContext extends IJobCallback.Stub implements ServiceConne // Debugging: time this job was last stopped. public long mStoppedTime; + final class JobCallback extends IJobCallback.Stub { + public String mStoppedReason; + public long mStoppedTime; + + @Override + public void acknowledgeStartMessage(int jobId, boolean ongoing) { + doAcknowledgeStartMessage(this, jobId, ongoing); + } + + @Override + public void acknowledgeStopMessage(int jobId, boolean reschedule) { + doAcknowledgeStopMessage(this, jobId, reschedule); + } + + @Override + public JobWorkItem dequeueWork(int jobId) { + return doDequeueWork(this, jobId); + } + + @Override + public boolean completeWork(int jobId, int workId) { + return doCompleteWork(this, jobId, workId); + } + + @Override + public void jobFinished(int jobId, boolean reschedule) { + doJobFinished(this, jobId, reschedule); + } + } + JobServiceContext(JobSchedulerService service, IBatteryStats batteryStats, JobPackageTracker tracker, Looper looper) { this(service.getContext(), service.getLock(), batteryStats, tracker, service, looper); @@ -168,6 +199,7 @@ public class JobServiceContext extends IJobCallback.Stub implements ServiceConne mPreferredUid = NO_PREFERRED_UID; mRunningJob = job; + mRunningCallback = new JobCallback(); final boolean isDeadlineExpired = job.hasDeadlineConstraint() && (job.getLatestRunTimeElapsed() < SystemClock.elapsedRealtime()); @@ -182,7 +214,7 @@ public class JobServiceContext extends IJobCallback.Stub implements ServiceConne job.changedAuthorities.toArray(triggeredAuthorities); } final JobInfo ji = job.getJob(); - mParams = new JobParameters(this, job.getJobId(), ji.getExtras(), + mParams = new JobParameters(mRunningCallback, job.getJobId(), ji.getExtras(), ji.getTransientExtras(), ji.getClipData(), ji.getClipGrantFlags(), isDeadlineExpired, triggeredUris, triggeredAuthorities); mExecutionStartTimeElapsed = SystemClock.elapsedRealtime(); @@ -198,6 +230,7 @@ public class JobServiceContext extends IJobCallback.Stub implements ServiceConne Slog.d(TAG, job.getServiceComponent().getShortClassName() + " unavailable."); } mRunningJob = null; + mRunningCallback = null; mParams = null; mExecutionStartTimeElapsed = 0L; mVerb = VERB_FINISHED; @@ -263,28 +296,29 @@ public class JobServiceContext extends IJobCallback.Stub implements ServiceConne return false; } - @Override - public void jobFinished(int jobId, boolean reschedule) { - doCallback(reschedule, "app called jobFinished"); + void doJobFinished(JobCallback cb, int jobId, boolean reschedule) { + doCallback(cb, reschedule, "app called jobFinished"); } - @Override - public void acknowledgeStopMessage(int jobId, boolean reschedule) { - doCallback(reschedule, null); + void doAcknowledgeStopMessage(JobCallback cb, int jobId, boolean reschedule) { + doCallback(cb, reschedule, null); } - @Override - public void acknowledgeStartMessage(int jobId, boolean ongoing) { - doCallback(ongoing, "finished start"); + void doAcknowledgeStartMessage(JobCallback cb, int jobId, boolean ongoing) { + doCallback(cb, ongoing, "finished start"); } - @Override - public JobWorkItem dequeueWork(int jobId) { - final int callingUid = Binder.getCallingUid(); + JobWorkItem doDequeueWork(JobCallback cb, int jobId) { final long ident = Binder.clearCallingIdentity(); try { synchronized (mLock) { - assertCallingUidLocked(callingUid); + assertCallerLocked(cb); + if (mVerb == VERB_STOPPING || mVerb == VERB_FINISHED) { + // This job is either all done, or on its way out. Either way, it + // should not dispatch any more work. We will pick up any remaining + // work the next time we start the job again. + return null; + } final JobWorkItem work = mRunningJob.dequeueWorkLocked(); if (work == null && !mRunningJob.hasExecutingWorkLocked()) { // This will finish the job. @@ -297,13 +331,11 @@ public class JobServiceContext extends IJobCallback.Stub implements ServiceConne } } - @Override - public boolean completeWork(int jobId, int workId) { - final int callingUid = Binder.getCallingUid(); + boolean doCompleteWork(JobCallback cb, int jobId, int workId) { final long ident = Binder.clearCallingIdentity(); try { synchronized (mLock) { - assertCallingUidLocked(callingUid); + assertCallerLocked(cb); return mRunningJob.completeWorkLocked(ActivityManager.getService(), workId); } } finally { @@ -369,8 +401,8 @@ public class JobServiceContext extends IJobCallback.Stub implements ServiceConne * whether the client exercising the callback is the client we expect. * @return True if the binder calling is coming from the client we expect. */ - private boolean verifyCallingUidLocked(final int callingUid) { - if (mRunningJob == null || callingUid != mRunningJob.getUid()) { + private boolean verifyCallerLocked(JobCallback cb) { + if (mRunningCallback != cb) { if (DEBUG) { Slog.d(TAG, "Stale callback received, ignoring."); } @@ -379,16 +411,15 @@ public class JobServiceContext extends IJobCallback.Stub implements ServiceConne return true; } - private void assertCallingUidLocked(final int callingUid) { - if (!verifyCallingUidLocked(callingUid)) { + private void assertCallerLocked(JobCallback cb) { + if (!verifyCallerLocked(cb)) { StringBuilder sb = new StringBuilder(128); - sb.append("Bad calling uid "); - sb.append(callingUid); - if (mStoppedReason != null) { + sb.append("Caller no longer running"); + if (cb.mStoppedReason != null) { sb.append(", last stopped "); - TimeUtils.formatDuration(SystemClock.elapsedRealtime() - mStoppedTime, sb); + TimeUtils.formatDuration(SystemClock.elapsedRealtime() - cb.mStoppedTime, sb); sb.append(" because: "); - sb.append(mStoppedReason); + sb.append(cb.mStoppedReason); } throw new SecurityException(sb.toString()); } @@ -421,12 +452,11 @@ public class JobServiceContext extends IJobCallback.Stub implements ServiceConne handleServiceBoundLocked(); } - void doCallback(boolean reschedule, String reason) { - final int callingUid = Binder.getCallingUid(); + void doCallback(JobCallback cb, boolean reschedule, String reason) { final long ident = Binder.clearCallingIdentity(); try { synchronized (mLock) { - if (!verifyCallingUidLocked(callingUid)) { + if (!verifyCallerLocked(cb)) { return; } doCallbackLocked(reschedule, reason); @@ -559,7 +589,7 @@ public class JobServiceContext extends IJobCallback.Stub implements ServiceConne * VERB_BINDING -> Cancelled before bind completed. Mark as cancelled and wait for * {@link #onServiceConnected(android.content.ComponentName, android.os.IBinder)} * _STARTING -> Mark as cancelled and wait for - * {@link JobServiceContext#acknowledgeStartMessage(int, boolean)} + * {@link JobServiceContext#doAcknowledgeStartMessage} * _EXECUTING -> call {@link #sendStopMessageLocked}}, but only if there are no callbacks * in the message queue. * _ENDING -> No point in doing anything here, so we ignore. @@ -671,6 +701,7 @@ public class JobServiceContext extends IJobCallback.Stub implements ServiceConne mContext.unbindService(JobServiceContext.this); mWakeLock = null; mRunningJob = null; + mRunningCallback = null; mParams = null; mVerb = VERB_FINISHED; mCancelled = false; @@ -684,6 +715,10 @@ public class JobServiceContext extends IJobCallback.Stub implements ServiceConne if (reason != null && mStoppedReason == null) { mStoppedReason = reason; mStoppedTime = SystemClock.elapsedRealtime(); + if (mRunningCallback != null) { + mRunningCallback.mStoppedReason = mStoppedReason; + mRunningCallback.mStoppedTime = mStoppedTime; + } } } diff --git a/services/core/java/com/android/server/job/JobStore.java b/services/core/java/com/android/server/job/JobStore.java index 22eed3b042504..f0cd8a8569e07 100644 --- a/services/core/java/com/android/server/job/JobStore.java +++ b/services/core/java/com/android/server/job/JobStore.java @@ -66,7 +66,7 @@ import org.xmlpull.v1.XmlSerializer; * and {@link com.android.server.job.JobStore.ReadJobMapFromDiskRunnable} lock on that * object. */ -public class JobStore { +public final class JobStore { private static final String TAG = "JobStore"; private static final boolean DEBUG = JobSchedulerService.DEBUG; @@ -263,7 +263,7 @@ public class JobStore { * Runnable that writes {@link #mJobSet} out to xml. * NOTE: This Runnable locks on mLock */ - private class WriteJobsMapToDiskRunnable implements Runnable { + private final class WriteJobsMapToDiskRunnable implements Runnable { @Override public void run() { final long startElapsed = SystemClock.elapsedRealtime(); @@ -444,7 +444,7 @@ public class JobStore { * Runnable that reads list of persisted job from xml. This is run once at start up, so doesn't * need to go through {@link JobStore#add(com.android.server.job.controllers.JobStatus)}. */ - private class ReadJobMapFromDiskRunnable implements Runnable { + private final class ReadJobMapFromDiskRunnable implements Runnable { private final JobSet jobSet; /** @@ -796,7 +796,7 @@ public class JobStore { } } - static class JobSet { + static final class JobSet { // Key is the getUid() originator of the jobs in each sheaf private SparseArray> mJobs; diff --git a/services/core/java/com/android/server/job/controllers/AppIdleController.java b/services/core/java/com/android/server/job/controllers/AppIdleController.java index 68dd00ff00c13..39f2a96b30e3a 100644 --- a/services/core/java/com/android/server/job/controllers/AppIdleController.java +++ b/services/core/java/com/android/server/job/controllers/AppIdleController.java @@ -33,7 +33,7 @@ import java.io.PrintWriter; * for a certain amount of time (maybe hours or days) are considered idle. When the app comes * out of idle state, it will be allowed to run scheduled jobs. */ -public class AppIdleController extends StateController { +public final class AppIdleController extends StateController { private static final String LOG_TAG = "AppIdleController"; private static final boolean DEBUG = false; @@ -171,7 +171,7 @@ public class AppIdleController extends StateController { } } - private class AppIdleStateChangeListener + private final class AppIdleStateChangeListener extends UsageStatsManagerInternal.AppIdleStateChangeListener { @Override public void onAppIdleStateChanged(String packageName, int userId, boolean idle) { diff --git a/services/core/java/com/android/server/job/controllers/BatteryController.java b/services/core/java/com/android/server/job/controllers/BatteryController.java index d275bd94a6f4e..91119690617a6 100644 --- a/services/core/java/com/android/server/job/controllers/BatteryController.java +++ b/services/core/java/com/android/server/job/controllers/BatteryController.java @@ -39,7 +39,7 @@ import java.io.PrintWriter; * be charging when it's been plugged in for more than two minutes, and the system has broadcast * ACTION_BATTERY_OK. */ -public class BatteryController extends StateController { +public final class BatteryController extends StateController { private static final String TAG = "JobScheduler.Batt"; private static final Object sCreationLock = new Object(); @@ -121,7 +121,7 @@ public class BatteryController extends StateController { } } - public class ChargingTracker extends BroadcastReceiver { + public final class ChargingTracker extends BroadcastReceiver { /** * Track whether we're "charging", where charging means that we're ready to commit to * doing work. diff --git a/services/core/java/com/android/server/job/controllers/ConnectivityController.java b/services/core/java/com/android/server/job/controllers/ConnectivityController.java index f4268185aa409..17c89282f2807 100644 --- a/services/core/java/com/android/server/job/controllers/ConnectivityController.java +++ b/services/core/java/com/android/server/job/controllers/ConnectivityController.java @@ -43,7 +43,7 @@ import java.io.PrintWriter; * status due to user-requested network policies, so we need to check * constraints on a per-UID basis. */ -public class ConnectivityController extends StateController implements +public final class ConnectivityController extends StateController implements ConnectivityManager.OnNetworkActiveListener { private static final String TAG = "JobScheduler.Conn"; private static final boolean DEBUG = false; diff --git a/services/core/java/com/android/server/job/controllers/ContentObserverController.java b/services/core/java/com/android/server/job/controllers/ContentObserverController.java index cfafc38428f38..ff807eccee7b8 100644 --- a/services/core/java/com/android/server/job/controllers/ContentObserverController.java +++ b/services/core/java/com/android/server/job/controllers/ContentObserverController.java @@ -39,7 +39,7 @@ import java.util.ArrayList; /** * Controller for monitoring changes to content URIs through a ContentObserver. */ -public class ContentObserverController extends StateController { +public final class ContentObserverController extends StateController { private static final String TAG = "JobScheduler.Content"; private static final boolean DEBUG = false; diff --git a/services/core/java/com/android/server/job/controllers/DeviceIdleJobsController.java b/services/core/java/com/android/server/job/controllers/DeviceIdleJobsController.java index 5ccf812882553..85993b900dc9b 100644 --- a/services/core/java/com/android/server/job/controllers/DeviceIdleJobsController.java +++ b/services/core/java/com/android/server/job/controllers/DeviceIdleJobsController.java @@ -37,7 +37,7 @@ import java.util.Arrays; * When device is dozing, set constraint for all jobs, except whitelisted apps, as not satisfied. * When device is not dozing, set constraint for all jobs as satisfied. */ -public class DeviceIdleJobsController extends StateController { +public final class DeviceIdleJobsController extends StateController { private static final String LOG_TAG = "DeviceIdleJobsController"; private static final boolean LOG_DEBUG = false; diff --git a/services/core/java/com/android/server/job/controllers/IdleController.java b/services/core/java/com/android/server/job/controllers/IdleController.java index 7e922930e6e86..9eda046fa1e43 100644 --- a/services/core/java/com/android/server/job/controllers/IdleController.java +++ b/services/core/java/com/android/server/job/controllers/IdleController.java @@ -33,7 +33,7 @@ import com.android.server.am.ActivityManagerService; import com.android.server.job.JobSchedulerService; import com.android.server.job.StateChangedListener; -public class IdleController extends StateController { +public final class IdleController extends StateController { private static final String TAG = "IdleController"; // Policy: we decide that we're "idle" if the device has been unused / @@ -107,7 +107,7 @@ public class IdleController extends StateController { mIdleTracker.startTracking(); } - class IdlenessTracker extends BroadcastReceiver { + final class IdlenessTracker extends BroadcastReceiver { private AlarmManager mAlarm; private PendingIntent mIdleTriggerIntent; boolean mIdle; diff --git a/services/core/java/com/android/server/job/controllers/StorageController.java b/services/core/java/com/android/server/job/controllers/StorageController.java index 4fe8eca54a6e1..c24e563948d45 100644 --- a/services/core/java/com/android/server/job/controllers/StorageController.java +++ b/services/core/java/com/android/server/job/controllers/StorageController.java @@ -35,7 +35,7 @@ import java.io.PrintWriter; /** * Simple controller that tracks the status of the device's storage. */ -public class StorageController extends StateController { +public final class StorageController extends StateController { private static final String TAG = "JobScheduler.Stor"; private static final Object sCreationLock = new Object(); @@ -112,7 +112,7 @@ public class StorageController extends StateController { } } - public class StorageTracker extends BroadcastReceiver { + public final class StorageTracker extends BroadcastReceiver { /** * Track whether storage is low. */ diff --git a/services/core/java/com/android/server/job/controllers/TimeController.java b/services/core/java/com/android/server/job/controllers/TimeController.java index 01c841e2083cc..d90699a61928b 100644 --- a/services/core/java/com/android/server/job/controllers/TimeController.java +++ b/services/core/java/com/android/server/job/controllers/TimeController.java @@ -38,7 +38,7 @@ import java.util.ListIterator; * This class sets an alarm for the next expiring job, and determines whether a job's minimum * delay has been satisfied. */ -public class TimeController extends StateController { +public final class TimeController extends StateController { private static final String TAG = "JobScheduler.Time"; /** Deadline alarm tag for logging purposes */