Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 661e6a10 authored by Marvin Ramin's avatar Marvin Ramin
Browse files

Improve permissions for display proxy methods 

Addresses a number of issues with the two methods:

- Checking for CREATE_VIRTUAL_DEVICE permission is redundant as the
  implementations validates that the caller owns the provided displayId
  in a VirtualDevice. Creating a VirtualDevice via VirtualDeviceManager is
  only possible if the caller holds CREATE_VIRTUAL_DEVICE.
- AccessibilityManager#unregisterDisplayProxy did not verify that the
  caller owns the display, letting different VirtualDevice owners
  unregister each others A11yProxies.
- Move the displayBelongsToCaller function to the
  AccessibilitySecurityPolicy as that seems like a better place for the
  function.

Bug: 399840253
Test: atest AccessibilityManagerServiceTest
Flag: EXEMPT refactor
Change-Id: I6ac25bf0bee91a2b8da47a8eefe4797f6c1dd4fc
parent 90f4380b

core/java/android/view/accessibility/IAccessibilityManager.aidl

+4 −4
Original line number Diff line number Diff line
@@ -147,12 +147,12 @@ interface IAccessibilityManager { 
    @RequiresNoPermission

    oneway void setAccessibilityWindowAttributes(int displayId, int windowId, int userId, in AccessibilityWindowAttributes attributes);



    // Requires CREATE_VIRTUAL_DEVICE permission. Also requires either a11y permission or role.

    @EnforcePermission("CREATE_VIRTUAL_DEVICE")

    // Requires the caller to either hold the MANAGE_ACCESSIBILITY permission or own the provided

    // displayId via VirtualDeviceManager.

    boolean registerProxyForDisplay(IAccessibilityServiceClient proxy, int displayId);



    // Requires CREATE_VIRTUAL_DEVICE permission. Also requires either a11y permission or role.

    @EnforcePermission("CREATE_VIRTUAL_DEVICE")

    // Requires the caller to either hold the MANAGE_ACCESSIBILITY permission or own the provided

    // displayId via VirtualDeviceManager.

    boolean unregisterProxyForDisplay(int displayId);



    // Used by UiAutomation for tests on the InputFilter

services/accessibility/java/com/android/server/accessibility/AccessibilityManagerService.java

+2 −11
Original line number Diff line number Diff line
@@ -16,7 +16,6 @@ 


package com.android.server.accessibility;



import static android.Manifest.permission.CREATE_VIRTUAL_DEVICE;

import static android.Manifest.permission.INJECT_EVENTS;

import static android.Manifest.permission.INTERNAL_SYSTEM_WINDOW;

import static android.Manifest.permission.MANAGE_ACCESSIBILITY;
@@ -4964,11 +4963,9 @@ public class AccessibilityManagerService extends IAccessibilityManager.Stub 
    }



    @Override

    @EnforcePermission(CREATE_VIRTUAL_DEVICE)

    public boolean registerProxyForDisplay(IAccessibilityServiceClient client, int displayId)

            throws RemoteException {

        registerProxyForDisplay_enforcePermission();

        mSecurityPolicy.checkForAccessibilityPermissionOrRole();

        mSecurityPolicy.checkForAccessibilityPermissionOrDisplayOwnership(displayId);

        enforceCurrentUserIfVisibleBackgroundEnabled();

        if (client == null) {

            return false;
@@ -4984,10 +4981,6 @@ public class AccessibilityManagerService extends IAccessibilityManager.Stub 
            throw new IllegalArgumentException("The display " + displayId + " is already being"

                    + " proxy-ed");

        }

        if (!mProxyManager.displayBelongsToCaller(Binder.getCallingUid(), displayId)) {

            throw new SecurityException("The display " + displayId + " does not belong to"

                    + " the caller.");

        }



        final long identity = Binder.clearCallingIdentity();

        try {
@@ -5004,10 +4997,8 @@ public class AccessibilityManagerService extends IAccessibilityManager.Stub 
    }



    @Override

    @EnforcePermission(CREATE_VIRTUAL_DEVICE)

    public boolean unregisterProxyForDisplay(int displayId) {

        unregisterProxyForDisplay_enforcePermission();

        mSecurityPolicy.checkForAccessibilityPermissionOrRole();

        mSecurityPolicy.checkForAccessibilityPermissionOrDisplayOwnership(displayId);

        final long identity = Binder.clearCallingIdentity();

        try {

            return mProxyManager.unregisterProxy(displayId);

services/accessibility/java/com/android/server/accessibility/AccessibilitySecurityPolicy.java

+18 −26
Original line number Diff line number Diff line
@@ -18,7 +18,6 @@ package com.android.server.accessibility; 


import static android.accessibilityservice.AccessibilityService.SoftKeyboardController.ENABLE_IME_FAIL_BY_ADMIN;

import static android.accessibilityservice.AccessibilityService.SoftKeyboardController.ENABLE_IME_SUCCESS;

import static android.companion.AssociationRequest.DEVICE_PROFILE_APP_STREAMING;



import android.Manifest;

import android.accessibilityservice.AccessibilityService;
@@ -26,7 +25,6 @@ import android.accessibilityservice.AccessibilityServiceInfo; 
import android.annotation.NonNull;

import android.annotation.Nullable;

import android.app.AppOpsManager;

import android.app.role.RoleManager;

import android.appwidget.AppWidgetManagerInternal;

import android.content.ComponentName;

import android.content.Context;
@@ -47,6 +45,8 @@ import android.view.accessibility.AccessibilityEvent; 
import android.view.inputmethod.InputMethodInfo;



import com.android.internal.util.ArrayUtils;

import com.android.server.LocalServices;

import com.android.server.companion.virtual.VirtualDeviceManagerInternal;

import com.android.server.inputmethod.InputMethodManagerInternal;



import libcore.util.EmptyArray;
@@ -675,39 +675,31 @@ public class AccessibilitySecurityPolicy { 
        }

    }



    /** Returns true if the display belongs to one of the caller's virtual devices. */

    private boolean displayBelongsToCaller(int callingUid, int proxyDisplayId) {

        final VirtualDeviceManagerInternal localVdm = LocalServices.getService(

                VirtualDeviceManagerInternal.class);

        if (localVdm == null) {

            return false;

        }

        int deviceId = localVdm.getDeviceIdForDisplayId(proxyDisplayId);

        return callingUid == localVdm.getDeviceOwnerUid(deviceId);

    }



    /**

     * Throws a SecurityException if the caller has neither the MANAGE_ACCESSIBILITY permission nor

     * the COMPANION_DEVICE_APP_STREAMING role.

     * owns the provided displayId in any VirtualDevice.

     */

    public void checkForAccessibilityPermissionOrRole() {

    public void checkForAccessibilityPermissionOrDisplayOwnership(int displayId) {

        final boolean canManageAccessibility =

                mContext.checkCallingOrSelfPermission(Manifest.permission.MANAGE_ACCESSIBILITY)

                        == PackageManager.PERMISSION_GRANTED;

        if (canManageAccessibility) {

            return;

        }

        final int callingUid = Binder.getCallingUid();

        final long identity = Binder.clearCallingIdentity();

        try {

            final RoleManager roleManager = mContext.getSystemService(RoleManager.class);

            if (roleManager != null) {

                final List<String> holders = roleManager.getRoleHoldersAsUser(

                        DEVICE_PROFILE_APP_STREAMING, UserHandle.getUserHandleForUid(callingUid));

                final String[] packageNames = mPackageManager.getPackagesForUid(callingUid);

                if (packageNames != null) {

                    for (String packageName : packageNames) {

                        if (holders.contains(packageName)) {

                            return;

                        }

                    }

                }

            }

            throw new SecurityException(

                    "Cannot register a proxy for a device without the "

                            + "android.app.role.COMPANION_DEVICE_APP_STREAMING role or the"

                            + " MANAGE_ACCESSIBILITY permission.");

        } finally {

            Binder.restoreCallingIdentity(identity);

        if (!displayBelongsToCaller(Binder.getCallingUid(), displayId)) {

            throw new SecurityException("The display " + displayId + " does not belong to"

                    + " the caller.");

        }

    }

services/accessibility/java/com/android/server/accessibility/ProxyManager.java

+0 −10
Original line number Diff line number Diff line
@@ -375,16 +375,6 @@ public class ProxyManager { 
        return isTrackingDeviceId;

    }



    /** Returns true if the display belongs to one of the caller's virtual devices. */

    public boolean displayBelongsToCaller(int callingUid, int proxyDisplayId) {

        final VirtualDeviceManagerInternal localVdm = getLocalVdm();

        if (localVdm == null) {

            return false;

        }

        int deviceId = localVdm.getDeviceIdForDisplayId(proxyDisplayId);

        return callingUid == localVdm.getDeviceOwnerUid(deviceId);

    }



    /**

     * Sends AccessibilityEvents to a proxy given the event's displayId.

     */

services/tests/servicestests/src/com/android/server/accessibility/AccessibilityManagerServiceTest.java

+5 −28
Original line number Diff line number Diff line
@@ -432,8 +432,6 @@ public class AccessibilityManagerServiceTest { 
    @SmallTest

    @Test

    public void testRegisterProxy() throws Exception {

        mFakePermissionEnforcer.grant(Manifest.permission.CREATE_VIRTUAL_DEVICE);

        when(mProxyManager.displayBelongsToCaller(anyInt(), anyInt())).thenReturn(true);

        mA11yms.registerProxyForDisplay(mMockServiceClient, TEST_DISPLAY);

        verify(mProxyManager).registerProxy(eq(mMockServiceClient), eq(TEST_DISPLAY), anyInt(),

                eq(mMockSecurityPolicy),
@@ -444,9 +442,8 @@ public class AccessibilityManagerServiceTest { 
    @SmallTest

    @Test

    public void testRegisterProxyWithoutA11yPermissionOrRole() throws Exception {

        mFakePermissionEnforcer.grant(Manifest.permission.CREATE_VIRTUAL_DEVICE);

        doThrow(SecurityException.class).when(mMockSecurityPolicy)

                .checkForAccessibilityPermissionOrRole();

                .checkForAccessibilityPermissionOrDisplayOwnership(TEST_DISPLAY);



        assertThrows(SecurityException.class,

                () -> mA11yms.registerProxyForDisplay(mMockServiceClient, TEST_DISPLAY));
@@ -454,19 +451,11 @@ public class AccessibilityManagerServiceTest { 
                any(), any(), any());

    }



    @SmallTest

    @Test

    public void testRegisterProxyWithoutDevicePermission() throws Exception {

        mFakePermissionEnforcer.revoke(Manifest.permission.CREATE_VIRTUAL_DEVICE);

        assertThrows(SecurityException.class,

                () -> mA11yms.registerProxyForDisplay(mMockServiceClient, TEST_DISPLAY));

        verify(mProxyManager, never()).registerProxy(any(), anyInt(), anyInt(), any(),

                any(), any(), any());

    }



    @SmallTest

    @Test

    public void testRegisterProxyForDefaultDisplay() throws Exception {

        doThrow(SecurityException.class).when(mMockSecurityPolicy)

                .checkForAccessibilityPermissionOrDisplayOwnership(Display.DEFAULT_DISPLAY);

        assertThrows(SecurityException.class,

                () -> mA11yms.registerProxyForDisplay(mMockServiceClient, Display.DEFAULT_DISPLAY));

        verify(mProxyManager, never()).registerProxy(any(), anyInt(), anyInt(), any(),
@@ -476,7 +465,6 @@ public class AccessibilityManagerServiceTest { 
    @SmallTest

    @Test

    public void testRegisterProxyForInvalidDisplay() throws Exception {

        mFakePermissionEnforcer.grant(Manifest.permission.CREATE_VIRTUAL_DEVICE);

        assertThrows(IllegalArgumentException.class,

                () -> mA11yms.registerProxyForDisplay(mMockServiceClient, Display.INVALID_DISPLAY));

        verify(mProxyManager, never()).registerProxy(any(), anyInt(), anyInt(), any(),
@@ -486,8 +474,6 @@ public class AccessibilityManagerServiceTest { 
    @SmallTest

    @Test

    public void testUnRegisterProxyWithPermission() throws Exception {

        mFakePermissionEnforcer.grant(Manifest.permission.CREATE_VIRTUAL_DEVICE);

        when(mProxyManager.displayBelongsToCaller(anyInt(), anyInt())).thenReturn(true);

        mA11yms.registerProxyForDisplay(mMockServiceClient, TEST_DISPLAY);

        mA11yms.unregisterProxyForDisplay(TEST_DISPLAY);
@@ -496,19 +482,10 @@ public class AccessibilityManagerServiceTest { 


    @SmallTest

    @Test

    public void testUnRegisterProxyWithoutA11yPermissionOrRole() {

    public void testUnRegisterProxyWithoutA11yPermissionOrDisplayOwnership() {

        doThrow(SecurityException.class).when(mMockSecurityPolicy)

                .checkForAccessibilityPermissionOrRole();



        assertThrows(SecurityException.class,

                () -> mA11yms.unregisterProxyForDisplay(TEST_DISPLAY));

        verify(mProxyManager, never()).unregisterProxy(TEST_DISPLAY);

    }

                .checkForAccessibilityPermissionOrDisplayOwnership(TEST_DISPLAY);



    @SmallTest

    @Test

    public void testUnRegisterProxyWithoutDevicePermission() {

        mFakePermissionEnforcer.revoke(Manifest.permission.CREATE_VIRTUAL_DEVICE);

        assertThrows(SecurityException.class,

                () -> mA11yms.unregisterProxyForDisplay(TEST_DISPLAY));

        verify(mProxyManager, never()).unregisterProxy(TEST_DISPLAY);