From 6ffbb9568c09e77bd934a03f5b0fbc21d520ce1a Mon Sep 17 00:00:00 2001 From: Hyundo Moon Date: Thu, 15 Apr 2021 17:20:30 +0900 Subject: [PATCH 1/2] Change MODIFY_AUDIO_ROUTING permission to MEDIA_CONTENT_CONTROL As changing the permission for MediaRouter2 system APIs, this CL changes the permission in shell accordingly. The permission was added by ag/13959439. Bug: 183428114 Test: Passed CTS tests Change-Id: I3aee256a11db730dd786e69821f3bb8bd590074f --- data/etc/privapp-permissions-platform.xml | 2 +- packages/Shell/AndroidManifest.xml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/data/etc/privapp-permissions-platform.xml b/data/etc/privapp-permissions-platform.xml index 2fe8b285ed888..8108eb91188bd 100644 --- a/data/etc/privapp-permissions-platform.xml +++ b/data/etc/privapp-permissions-platform.xml @@ -503,7 +503,7 @@ applications that come with the platform - + diff --git a/packages/Shell/AndroidManifest.xml b/packages/Shell/AndroidManifest.xml index bef6423f3baa1..9c2e8c97f1f40 100644 --- a/packages/Shell/AndroidManifest.xml +++ b/packages/Shell/AndroidManifest.xml @@ -534,7 +534,7 @@ - + From 057b4b8d466e9bde0e4b9261fea408a83c5c7a13 Mon Sep 17 00:00:00 2001 From: Hyundo Moon Date: Wed, 14 Apr 2021 13:39:55 +0900 Subject: [PATCH 2/2] System MediaRouter2: Resolve API review This CL resolves API reviews on system APIs on MediaRouter2. 1) It checks permission when registering the MediaRoute2Manager in system server. Therefore, app cannot bypass the permission checks via reflection. The system api calls will be no-op without permission. 2) It adds @RequiresPermission annotations to related APIs. Also, this CL changes the required permission from MODIFY_AUDIO_ROUTING to MEDIA_CONTENT_CONTROL, as the media routing can be applied to video, not only to audio. Note that MediaRouter2#getInstance(Context, String) does not throw SecurityException anymore. Bug: 183428114 Test: atest android.media.cts.MediaRouter2Test atest android.media.cts.SystemMediaRouterTest (MediaRouter2ManagerTest is already broken, and that will be handled out of this CL.) Change-Id: I55013a5b4b954ca1c8e2adb586f49f01103563db --- core/api/current.txt | 1 + core/api/system-current.txt | 11 +- .../android/media/IMediaRouterService.aidl | 2 +- media/java/android/media/MediaRouter2.java | 117 +++++++++--------- .../android/media/MediaRouter2Manager.java | 2 + .../MediaRouter2ManagerTest.java | 7 ++ .../server/media/MediaRouter2ServiceImpl.java | 12 +- .../server/media/MediaRouterService.java | 4 +- 8 files changed, 82 insertions(+), 74 deletions(-) diff --git a/core/api/current.txt b/core/api/current.txt index ddb76bb748b26..efed718cb6c41 100644 --- a/core/api/current.txt +++ b/core/api/current.txt @@ -23322,6 +23322,7 @@ package android.media { } public final class MediaRouter2 { + method @Nullable public android.media.MediaRouter2.RoutingController getController(@NonNull String); method @NonNull public java.util.List getControllers(); method @NonNull public static android.media.MediaRouter2 getInstance(@NonNull android.content.Context); method @NonNull public java.util.List getRoutes(); diff --git a/core/api/system-current.txt b/core/api/system-current.txt index 8d03d17f13088..ab2a3f65a916b 100644 --- a/core/api/system-current.txt +++ b/core/api/system-current.txt @@ -5301,12 +5301,11 @@ package android.media { public final class MediaRouter2 { method @NonNull public java.util.List getAllRoutes(); method @Nullable public String getClientPackageName(); - method @Nullable public android.media.MediaRouter2.RoutingController getController(@NonNull String); - method @Nullable @RequiresPermission(android.Manifest.permission.MODIFY_AUDIO_ROUTING) public static android.media.MediaRouter2 getInstance(@NonNull android.content.Context, @NonNull String); - method public void setRouteVolume(@NonNull android.media.MediaRoute2Info, int); - method public void startScan(); - method public void stopScan(); - method public void transfer(@NonNull android.media.MediaRouter2.RoutingController, @NonNull android.media.MediaRoute2Info); + method @Nullable @RequiresPermission(android.Manifest.permission.MEDIA_CONTENT_CONTROL) public static android.media.MediaRouter2 getInstance(@NonNull android.content.Context, @NonNull String); + method @RequiresPermission(android.Manifest.permission.MEDIA_CONTENT_CONTROL) public void setRouteVolume(@NonNull android.media.MediaRoute2Info, int); + method @RequiresPermission(android.Manifest.permission.MEDIA_CONTENT_CONTROL) public void startScan(); + method @RequiresPermission(android.Manifest.permission.MEDIA_CONTENT_CONTROL) public void stopScan(); + method @RequiresPermission(android.Manifest.permission.MEDIA_CONTENT_CONTROL) public void transfer(@NonNull android.media.MediaRouter2.RoutingController, @NonNull android.media.MediaRoute2Info); } public abstract static class MediaRouter2.RouteCallback { diff --git a/media/java/android/media/IMediaRouterService.aidl b/media/java/android/media/IMediaRouterService.aidl index f817a3c3c3530..48289ecde9e0e 100644 --- a/media/java/android/media/IMediaRouterService.aidl +++ b/media/java/android/media/IMediaRouterService.aidl @@ -48,7 +48,7 @@ interface IMediaRouterService { // MediaRouterService.java for readability. // Methods for MediaRouter2 - void checkModifyAudioRoutingPermission(); + void enforceMediaContentControlPermission(); List getSystemRoutes(); RoutingSessionInfo getSystemSessionInfo(); diff --git a/media/java/android/media/MediaRouter2.java b/media/java/android/media/MediaRouter2.java index 90fa9a52f5cf2..232de0b7c57f5 100644 --- a/media/java/android/media/MediaRouter2.java +++ b/media/java/android/media/MediaRouter2.java @@ -18,6 +18,7 @@ package android.media; import static com.android.internal.util.function.pooled.PooledLambda.obtainMessage; +import android.Manifest; import android.annotation.CallbackExecutor; import android.annotation.NonNull; import android.annotation.Nullable; @@ -93,7 +94,7 @@ public final class MediaRouter2 { // TODO: Specify the fields that are only used (or not used) by system media router. private final String mClientPackageName; - private final ManagerCallback mManagerCallback; + final ManagerCallback mManagerCallback; private final String mPackageName; @@ -164,13 +165,24 @@ public final class MediaRouter2 { * @hide */ @SystemApi - @RequiresPermission(android.Manifest.permission.MODIFY_AUDIO_ROUTING) + @RequiresPermission(Manifest.permission.MEDIA_CONTENT_CONTROL) @Nullable public static MediaRouter2 getInstance(@NonNull Context context, @NonNull String clientPackageName) { Objects.requireNonNull(context, "context must not be null"); Objects.requireNonNull(clientPackageName, "clientPackageName must not be null"); + // Note: Even though this check could be somehow bypassed, the other permission checks + // in system server will not allow MediaRouter2Manager to be registered. + IMediaRouterService serviceBinder = IMediaRouterService.Stub.asInterface( + ServiceManager.getService(Context.MEDIA_ROUTER_SERVICE)); + try { + // SecurityException will be thrown if there's no permission. + serviceBinder.enforceMediaContentControlPermission(); + } catch (RemoteException e) { + Log.e(TAG, "Unable to check MEDIA_CONTENT_CONTROL permission."); + } + PackageManager pm = context.getPackageManager(); try { pm.getPackageInfo(clientPackageName, 0); @@ -183,20 +195,13 @@ public final class MediaRouter2 { MediaRouter2 instance = sSystemMediaRouter2Map.get(clientPackageName); if (instance == null) { if (sManager == null) { - IMediaRouterService serviceBinder = IMediaRouterService.Stub.asInterface( - ServiceManager.getService(Context.MEDIA_ROUTER_SERVICE)); - try { - // MediaRouterService will throw a SecurityException if the caller - // doesn't have MODIFY_AUDIO_ROUTING permission. - serviceBinder.checkModifyAudioRoutingPermission(); - } catch (RemoteException e) { - e.rethrowAsRuntimeException(); - } sManager = MediaRouter2Manager.getInstance(context.getApplicationContext()); } instance = new MediaRouter2(context, clientPackageName); sSystemMediaRouter2Map.put(clientPackageName, instance); - instance.registerManagerCallbackForSystemRouter(); + // Using direct executor here, since MediaRouter2Manager also posts + // to the main handler. + sManager.registerCallback(Runnable::run, instance.mManagerCallback); } return instance; } @@ -213,11 +218,15 @@ public final class MediaRouter2 { * Use {@link RouteCallback} to get the route related events. *

* Note that calling start/stopScan is applied to all system routers in the same process. + *

+ * This will be no-op for non-system media routers. * * @see #stopScan() + * @see #getInstance(Context, String) * @hide */ @SystemApi + @RequiresPermission(Manifest.permission.MEDIA_CONTENT_CONTROL) public void startScan() { if (isSystemRouter()) { sManager.startScan(); @@ -236,11 +245,15 @@ public final class MediaRouter2 { * Use {@link RouteCallback} to get the route related events. *

* Note that calling start/stopScan is applied to all system routers in the same process. + *

+ * This will be no-op for non-system media routers. * * @see #startScan() + * @see #getInstance(Context, String) * @hide */ @SystemApi + @RequiresPermission(Manifest.permission.MEDIA_CONTENT_CONTROL) public void stopScan() { if (isSystemRouter()) { sManager.stopScan(); @@ -314,7 +327,8 @@ public final class MediaRouter2 { /** * Gets the client package name of the app which this media router controls. - * This is only non-null when the router instance is created with the client package name. + *

+ * This will return null for non-system media routers. * * @see #getInstance(Context, String) * @hide @@ -573,9 +587,25 @@ public final class MediaRouter2 { return; } - Objects.requireNonNull(route, "route must not be null"); Log.v(TAG, "Transferring to route: " + route); - transfer(getCurrentController(), route); + + boolean routeFound; + synchronized (mLock) { + // TODO: Check thread-safety + routeFound = mRoutes.containsKey(route.getId()); + } + if (!routeFound) { + notifyTransferFailure(route); + return; + } + + RoutingController controller = getCurrentController(); + if (controller.getRoutingSessionInfo().getTransferableRoutes().contains(route.getId())) { + controller.transferToRoute(route); + return; + } + + requestCreateController(controller, route, MANAGER_REQUEST_ID_NONE); } /** @@ -594,36 +624,20 @@ public final class MediaRouter2 { /** * Transfers the media of a routing controller to the given route. + *

+ * This will be no-op for non-system media routers. + * * @param controller a routing controller controlling media routing. * @param route the route you want to transfer the media to. * @hide */ @SystemApi + @RequiresPermission(Manifest.permission.MEDIA_CONTENT_CONTROL) public void transfer(@NonNull RoutingController controller, @NonNull MediaRoute2Info route) { if (isSystemRouter()) { sManager.transfer(controller.getRoutingSessionInfo(), route); return; } - - Objects.requireNonNull(controller, "controller must not be null"); - Objects.requireNonNull(route, "route must not be null"); - - boolean routeFound; - synchronized (mLock) { - // TODO: Check thread-safety - routeFound = mRoutes.containsKey(route.getId()); - } - if (!routeFound) { - notifyTransferFailure(route); - return; - } - - if (controller.getRoutingSessionInfo().getTransferableRoutes().contains(route.getId())) { - controller.transferToRoute(route); - return; - } - - requestCreateController(controller, route, MANAGER_REQUEST_ID_NONE); } void requestCreateController(@NonNull RoutingController controller, @@ -687,9 +701,7 @@ public final class MediaRouter2 { /** * Gets a {@link RoutingController} whose ID is equal to the given ID. * Returns {@code null} if there is no matching controller. - * @hide */ - @SystemApi @Nullable public RoutingController getController(@NonNull String id) { Objects.requireNonNull(id, "id must not be null"); @@ -739,14 +751,16 @@ public final class MediaRouter2 { /** * Requests a volume change for the route asynchronously. - *

* It may have no effect if the route is currently not selected. - *

+ *

+ * This will be no-op for non-system media routers. * * @param volume The new volume value between 0 and {@link MediaRoute2Info#getVolumeMax}. + * @see #getInstance(Context, String) * @hide */ @SystemApi + @RequiresPermission(Manifest.permission.MEDIA_CONTENT_CONTROL) public void setRouteVolume(@NonNull MediaRoute2Info route, int volume) { Objects.requireNonNull(route, "route must not be null"); @@ -754,18 +768,7 @@ public final class MediaRouter2 { sManager.setRouteVolume(route, volume); return; } - - MediaRouter2Stub stub; - synchronized (mLock) { - stub = mStub; - } - if (stub != null) { - try { - mMediaRouterService.setRouteVolumeWithRouter2(stub, route, volume); - } catch (RemoteException ex) { - Log.e(TAG, "Unable to set route volume.", ex); - } - } + // If this API needs to be public, use IMediaRouterService#setRouteVolumeWithRouter2() } void syncRoutesOnHandler(List currentRoutes, @@ -1039,15 +1042,6 @@ public final class MediaRouter2 { return mClientPackageName != null; } - /** - * Registers {@link MediaRouter2Manager.Callback} for getting events. - * Should only used for system media routers. - */ - private void registerManagerCallbackForSystemRouter() { - // Using direct executor here, since MediaRouter2Manager also posts to the main handler. - sManager.registerCallback(Runnable::run, mManagerCallback); - } - /** * Returns a {@link RoutingSessionInfo} which has the client package name. * The client package name is set only when the given sessionInfo doesn't have it. @@ -1073,6 +1067,9 @@ public final class MediaRouter2 { } private void updateAllRoutesFromManager() { + if (!isSystemRouter()) { + return; + } synchronized (mLock) { mRoutes.clear(); for (MediaRoute2Info route : sManager.getAllRoutes()) { diff --git a/media/java/android/media/MediaRouter2Manager.java b/media/java/android/media/MediaRouter2Manager.java index 758a8130a62e8..915cb1272040f 100644 --- a/media/java/android/media/MediaRouter2Manager.java +++ b/media/java/android/media/MediaRouter2Manager.java @@ -49,6 +49,8 @@ import java.util.stream.Collectors; /** * A class that monitors and controls media routing of other apps. + * {@link android.Manifest.permission#MEDIA_CONTENT_CONTROL} is required to use this class, + * or {@link SecurityException} will be thrown. * @hide */ public final class MediaRouter2Manager { diff --git a/media/tests/MediaRouter/src/com/android/mediaroutertest/MediaRouter2ManagerTest.java b/media/tests/MediaRouter/src/com/android/mediaroutertest/MediaRouter2ManagerTest.java index 4b0062b90e961..eaa4f03ba81ac 100644 --- a/media/tests/MediaRouter/src/com/android/mediaroutertest/MediaRouter2ManagerTest.java +++ b/media/tests/MediaRouter/src/com/android/mediaroutertest/MediaRouter2ManagerTest.java @@ -42,6 +42,8 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; +import android.Manifest; +import android.app.UiAutomation; import android.content.Context; import android.media.MediaRoute2Info; import android.media.MediaRouter2; @@ -87,6 +89,7 @@ public class MediaRouter2ManagerTest { private static final String TEST_NAME_UNKNOWN = "unknown"; private Context mContext; + private UiAutomation mUiAutomation; private MediaRouter2Manager mManager; private MediaRouter2 mRouter2; private Executor mExecutor; @@ -110,6 +113,8 @@ public class MediaRouter2ManagerTest { @Before public void setUp() throws Exception { mContext = InstrumentationRegistry.getTargetContext(); + mUiAutomation = InstrumentationRegistry.getInstrumentation().getUiAutomation(); + mUiAutomation.adoptShellPermissionIdentity(Manifest.permission.MEDIA_CONTENT_CONTROL); mManager = MediaRouter2Manager.getInstance(mContext); mRouter2 = MediaRouter2.getInstance(mContext); // If we need to support thread pool executors, change this to thread pool executor. @@ -129,6 +134,8 @@ public class MediaRouter2ManagerTest { instance.setProxy(null); instance.setSpy(null); } + + mUiAutomation.dropShellPermissionIdentity(); } @Test diff --git a/services/core/java/com/android/server/media/MediaRouter2ServiceImpl.java b/services/core/java/com/android/server/media/MediaRouter2ServiceImpl.java index ba1e23ce022fe..1dbc8a926996b 100644 --- a/services/core/java/com/android/server/media/MediaRouter2ServiceImpl.java +++ b/services/core/java/com/android/server/media/MediaRouter2ServiceImpl.java @@ -25,6 +25,7 @@ import static android.media.MediaRouter2Utils.getProviderId; import static com.android.internal.util.function.pooled.PooledLambda.obtainMessage; +import android.Manifest; import android.annotation.NonNull; import android.annotation.Nullable; import android.app.ActivityManager; @@ -142,16 +143,14 @@ class MediaRouter2ServiceImpl { //////////////////////////////////////////////////////////////// @NonNull - public void checkModifyAudioRoutingPermission() { + public void enforceMediaContentControlPermission() { final int pid = Binder.getCallingPid(); final int uid = Binder.getCallingUid(); final long token = Binder.clearCallingIdentity(); try { - if (mContext.checkPermission(android.Manifest.permission.MODIFY_AUDIO_ROUTING, pid, uid) - != PackageManager.PERMISSION_GRANTED) { - throw new SecurityException("Must hold the MODIFY_AUDIO_ROUTING permission."); - } + mContext.enforcePermission(Manifest.permission.MEDIA_CONTENT_CONTROL, pid, uid, + "Must hold MEDIA_CONTENT_CONTROL permission."); } finally { Binder.restoreCallingIdentity(token); } @@ -890,6 +889,9 @@ class MediaRouter2ServiceImpl { return; } + mContext.enforcePermission(Manifest.permission.MEDIA_CONTENT_CONTROL, pid, uid, + "Must hold MEDIA_CONTENT_CONTROL permission."); + UserRecord userRecord = getOrCreateUserRecordLocked(userId); managerRecord = new ManagerRecord(userRecord, manager, uid, pid, packageName); try { diff --git a/services/core/java/com/android/server/media/MediaRouterService.java b/services/core/java/com/android/server/media/MediaRouterService.java index 3d19b70ca95fd..384bc99cb1151 100644 --- a/services/core/java/com/android/server/media/MediaRouterService.java +++ b/services/core/java/com/android/server/media/MediaRouterService.java @@ -438,8 +438,8 @@ public final class MediaRouterService extends IMediaRouterService.Stub // Binder call @Override - public void checkModifyAudioRoutingPermission() { - mService2.checkModifyAudioRoutingPermission(); + public void enforceMediaContentControlPermission() { + mService2.enforceMediaContentControlPermission(); } // Binder call