From 5b7c195b0d4c18981f1d2c49bf12200cdc0240d6 Mon Sep 17 00:00:00 2001 From: Nick Moukhine Date: Tue, 22 Dec 2020 16:26:51 +0100 Subject: [PATCH] Follow-up fixes to MusicRecognitionManagerService Added a check to only allow the default MusicRecognitionService to be called, unless the caller is the same app that hosts the MusicRecognitionService, as is the case in tests. Refactored MusicRecognitionManagerPerUserService to not hold the client's callback as a field to prevent leaks and needing to null it out (then requiring null checks). Instead, it's always a local variable or a final field in ephemeral objects. Bug: 174744852 Bug: 168696277 Test: atest CtsMusicRecognitionTestCases Change-Id: I70ff3c38da7096c60f82fe75b47674b4b233e672 --- ...MusicRecognitionManagerPerUserService.java | 44 +++++++++----- .../MusicRecognitionManagerService.java | 60 ++++++++++++++++++- .../RemoteMusicRecognitionService.java | 11 +++- 3 files changed, 94 insertions(+), 21 deletions(-) diff --git a/services/musicrecognition/java/com/android/server/musicrecognition/MusicRecognitionManagerPerUserService.java b/services/musicrecognition/java/com/android/server/musicrecognition/MusicRecognitionManagerPerUserService.java index 3531512148233..0cb729d924b41 100644 --- a/services/musicrecognition/java/com/android/server/musicrecognition/MusicRecognitionManagerPerUserService.java +++ b/services/musicrecognition/java/com/android/server/musicrecognition/MusicRecognitionManagerPerUserService.java @@ -16,6 +16,7 @@ package com.android.server.musicrecognition; +import static android.media.musicrecognition.MusicRecognitionManager.RECOGNITION_FAILED_AUDIO_UNAVAILABLE; import static android.media.musicrecognition.MusicRecognitionManager.RECOGNITION_FAILED_SERVICE_KILLED; import static android.media.musicrecognition.MusicRecognitionManager.RECOGNITION_FAILED_SERVICE_UNAVAILABLE; import static android.media.musicrecognition.MusicRecognitionManager.RecognitionFailureCode; @@ -64,10 +65,6 @@ public final class MusicRecognitionManagerPerUserService extends @GuardedBy("mLock") private RemoteMusicRecognitionService mRemoteService; - private MusicRecognitionServiceCallback mRemoteServiceCallback = - new MusicRecognitionServiceCallback(); - private IMusicRecognitionManagerCallback mCallback; - MusicRecognitionManagerPerUserService( @NonNull MusicRecognitionManagerService primary, @NonNull Object lock, int userId) { @@ -100,7 +97,8 @@ public final class MusicRecognitionManagerPerUserService extends @GuardedBy("mLock") @Nullable - private RemoteMusicRecognitionService ensureRemoteServiceLocked() { + private RemoteMusicRecognitionService ensureRemoteServiceLocked( + IMusicRecognitionManagerCallback clientCallback) { if (mRemoteService == null) { final String serviceName = getComponentNameLocked(); if (serviceName == null) { @@ -113,7 +111,8 @@ public final class MusicRecognitionManagerPerUserService extends mRemoteService = new RemoteMusicRecognitionService(getContext(), serviceComponent, mUserId, this, - mRemoteServiceCallback, mMaster.isBindInstantServiceAllowed(), + new MusicRecognitionServiceCallback(clientCallback), + mMaster.isBindInstantServiceAllowed(), mMaster.verbose); } @@ -130,13 +129,14 @@ public final class MusicRecognitionManagerPerUserService extends @NonNull IBinder callback) { int maxAudioLengthSeconds = Math.min(recognitionRequest.getMaxAudioLengthSeconds(), MAX_STREAMING_SECONDS); - mCallback = IMusicRecognitionManagerCallback.Stub.asInterface(callback); + IMusicRecognitionManagerCallback clientCallback = + IMusicRecognitionManagerCallback.Stub.asInterface(callback); AudioRecord audioRecord = createAudioRecord(recognitionRequest, maxAudioLengthSeconds); - mRemoteService = ensureRemoteServiceLocked(); + mRemoteService = ensureRemoteServiceLocked(clientCallback); if (mRemoteService == null) { try { - mCallback.onRecognitionFailed( + clientCallback.onRecognitionFailed( RECOGNITION_FAILED_SERVICE_UNAVAILABLE); } catch (RemoteException e) { // Ignored. @@ -147,7 +147,8 @@ public final class MusicRecognitionManagerPerUserService extends Pair clientPipe = createPipe(); if (clientPipe == null) { try { - mCallback.onAudioStreamClosed(); + clientCallback.onRecognitionFailed( + RECOGNITION_FAILED_AUDIO_UNAVAILABLE); } catch (RemoteException ignored) { // Ignored. } @@ -192,11 +193,10 @@ public final class MusicRecognitionManagerPerUserService extends } finally { audioRecord.release(); try { - mCallback.onAudioStreamClosed(); + clientCallback.onAudioStreamClosed(); } catch (RemoteException ignored) { // Ignored. } - } }); // Send the pipe down to the lookup service while we write to it asynchronously. @@ -207,13 +207,20 @@ public final class MusicRecognitionManagerPerUserService extends * Callback invoked by {@link android.service.musicrecognition.MusicRecognitionService} to pass * back the music search result. */ - private final class MusicRecognitionServiceCallback extends + final class MusicRecognitionServiceCallback extends IMusicRecognitionServiceCallback.Stub { + + private final IMusicRecognitionManagerCallback mClientCallback; + + private MusicRecognitionServiceCallback(IMusicRecognitionManagerCallback clientCallback) { + mClientCallback = clientCallback; + } + @Override public void onRecognitionSucceeded(MediaMetadata result, Bundle extras) { try { sanitizeBundle(extras); - mCallback.onRecognitionSucceeded(result, extras); + mClientCallback.onRecognitionSucceeded(result, extras); } catch (RemoteException ignored) { // Ignored. } @@ -223,18 +230,23 @@ public final class MusicRecognitionManagerPerUserService extends @Override public void onRecognitionFailed(@RecognitionFailureCode int failureCode) { try { - mCallback.onRecognitionFailed(failureCode); + mClientCallback.onRecognitionFailed(failureCode); } catch (RemoteException ignored) { // Ignored. } destroyService(); } + + private IMusicRecognitionManagerCallback getClientCallback() { + return mClientCallback; + } } @Override public void onServiceDied(@NonNull RemoteMusicRecognitionService service) { try { - mCallback.onRecognitionFailed(RECOGNITION_FAILED_SERVICE_KILLED); + service.getServerCallback().getClientCallback().onRecognitionFailed( + RECOGNITION_FAILED_SERVICE_KILLED); } catch (RemoteException e) { // Ignored. } diff --git a/services/musicrecognition/java/com/android/server/musicrecognition/MusicRecognitionManagerService.java b/services/musicrecognition/java/com/android/server/musicrecognition/MusicRecognitionManagerService.java index 9123daf0378af..38f43138aee0e 100644 --- a/services/musicrecognition/java/com/android/server/musicrecognition/MusicRecognitionManagerService.java +++ b/services/musicrecognition/java/com/android/server/musicrecognition/MusicRecognitionManagerService.java @@ -22,7 +22,9 @@ import static android.media.musicrecognition.MusicRecognitionManager.RECOGNITION import android.annotation.NonNull; import android.annotation.Nullable; +import android.content.ComponentName; import android.content.Context; +import android.content.pm.PackageManager; import android.media.musicrecognition.IMusicRecognitionManager; import android.media.musicrecognition.IMusicRecognitionManagerCallback; import android.media.musicrecognition.RecognitionRequest; @@ -32,7 +34,9 @@ import android.os.RemoteException; import android.os.ResultReceiver; import android.os.ShellCallback; import android.os.UserHandle; +import android.util.Slog; +import com.android.internal.annotations.GuardedBy; import com.android.server.infra.AbstractMasterSystemService; import com.android.server.infra.FrameworkResourcesServiceNameResolver; @@ -113,9 +117,11 @@ public class MusicRecognitionManagerService extends enforceCaller("beginRecognition"); synchronized (mLock) { + int userId = UserHandle.getCallingUserId(); final MusicRecognitionManagerPerUserService service = getServiceForUserLocked( - UserHandle.getCallingUserId()); - if (service != null) { + userId); + if (service != null && (isDefaultServiceLocked(userId) + || isCalledByServiceAppLocked("beginRecognition"))) { service.beginRecognitionLocked(recognitionRequest, callback); } else { try { @@ -139,5 +145,55 @@ public class MusicRecognitionManagerService extends MusicRecognitionManagerService.this).exec(this, in, out, err, args, callback, resultReceiver); } + + /** True if the currently set handler service is not overridden by the shell. */ + @GuardedBy("mLock") + private boolean isDefaultServiceLocked(int userId) { + final String defaultServiceName = mServiceNameResolver.getDefaultServiceName(userId); + if (defaultServiceName == null) { + return false; + } + + final String currentServiceName = mServiceNameResolver.getServiceName(userId); + return defaultServiceName.equals(currentServiceName); + } + + /** True if the caller of the api is the same app which hosts the default service. */ + @GuardedBy("mLock") + private boolean isCalledByServiceAppLocked(@NonNull String methodName) { + final int userId = UserHandle.getCallingUserId(); + final int callingUid = Binder.getCallingUid(); + final String serviceName = mServiceNameResolver.getServiceName(userId); + if (serviceName == null) { + Slog.e(TAG, methodName + ": called by UID " + callingUid + + ", but there's no service set for user " + userId); + return false; + } + + final ComponentName serviceComponent = ComponentName.unflattenFromString(serviceName); + if (serviceComponent == null) { + Slog.w(TAG, methodName + ": invalid service name: " + serviceName); + return false; + } + + final String servicePackageName = serviceComponent.getPackageName(); + + final PackageManager pm = getContext().getPackageManager(); + final int serviceUid; + try { + serviceUid = pm.getPackageUidAsUser(servicePackageName, + UserHandle.getCallingUserId()); + } catch (PackageManager.NameNotFoundException e) { + Slog.w(TAG, methodName + ": could not verify UID for " + serviceName); + return false; + } + if (callingUid != serviceUid) { + Slog.e(TAG, methodName + ": called by UID " + callingUid + ", but service UID is " + + serviceUid); + return false; + } + + return true; + } } } diff --git a/services/musicrecognition/java/com/android/server/musicrecognition/RemoteMusicRecognitionService.java b/services/musicrecognition/java/com/android/server/musicrecognition/RemoteMusicRecognitionService.java index 4814a821d5258..6c7d673ffe11d 100644 --- a/services/musicrecognition/java/com/android/server/musicrecognition/RemoteMusicRecognitionService.java +++ b/services/musicrecognition/java/com/android/server/musicrecognition/RemoteMusicRecognitionService.java @@ -21,13 +21,13 @@ import android.content.ComponentName; import android.content.Context; import android.media.AudioFormat; import android.media.musicrecognition.IMusicRecognitionService; -import android.media.musicrecognition.IMusicRecognitionServiceCallback; import android.media.musicrecognition.MusicRecognitionService; import android.os.IBinder; import android.os.ParcelFileDescriptor; import android.text.format.DateUtils; import com.android.internal.infra.AbstractMultiplePendingRequestsRemoteService; +import com.android.server.musicrecognition.MusicRecognitionManagerPerUserService.MusicRecognitionServiceCallback; /** Remote connection to an instance of {@link MusicRecognitionService}. */ public class RemoteMusicRecognitionService extends @@ -39,11 +39,12 @@ public class RemoteMusicRecognitionService extends private static final long TIMEOUT_IDLE_BIND_MILLIS = 40 * DateUtils.SECOND_IN_MILLIS; // Allows the remote service to send back a result. - private final IMusicRecognitionServiceCallback mServerCallback; + private final MusicRecognitionServiceCallback + mServerCallback; public RemoteMusicRecognitionService(Context context, ComponentName serviceName, int userId, MusicRecognitionManagerPerUserService perUserService, - IMusicRecognitionServiceCallback callback, + MusicRecognitionServiceCallback callback, boolean bindInstantServiceAllowed, boolean verbose) { super(context, MusicRecognitionService.ACTION_MUSIC_SEARCH_LOOKUP, serviceName, userId, perUserService, @@ -66,6 +67,10 @@ public class RemoteMusicRecognitionService extends return TIMEOUT_IDLE_BIND_MILLIS; } + MusicRecognitionServiceCallback getServerCallback() { + return mServerCallback; + } + /** * Required, but empty since we don't need to notify the callback implementation of the request * results.