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

Commit daee2b30 authored by Jeff Sharkey's avatar Jeff Sharkey
Browse files

Refinement of AttributionSource handling. 

Previous CLs had started passing AttributionSource values across
Binder calls inside BluetoothDevice instances, but this can cause
confuse the permission check logic in the future; we should instead
always aim to use the AttributionSource closest to the app making
the call, instead of parceling it.

This change also improves logging to highlight when we're quietly
treating a permission as denied, and when a UID is mismatched.

Bug: 186106084
Test: atest BluetoothInstrumentationTests
Change-Id: I83f4c7349baefc9c642dec120708ed50bfcb3f12
parent 746c55ed

android/app/src/com/android/bluetooth/Utils.java

+46 −39
Original line number Diff line number Diff line
@@ -345,6 +345,8 @@ public final class Utils { 
        try {

            int packageUid = context.getPackageManager().getPackageUid(callingPackage, 0);

            if (packageUid != callingUid) {

                Log.e(TAG, "isPackageNameAccurate: App with package name " + callingPackage

                        + " is UID " + packageUid + " but caller is " + callingUid);

                return false;

            }

        } catch (PackageManager.NameNotFoundException e) {
@@ -390,6 +392,41 @@ public final class Utils { 
                "Need DUMP permission");

    }



    private static boolean checkPermissionForPreflight(Context context, String permission) {

        final int result = PermissionChecker.checkCallingOrSelfPermissionForPreflight(

                context, permission);

        if (result == PERMISSION_GRANTED) {

            return true;

        }



        final String msg = "Need " + permission + " permission";

        if (result == PERMISSION_HARD_DENIED) {

            throw new SecurityException(msg);

        } else {

            Log.w(TAG, msg);

            return false;

        }

    }



    private static boolean checkPermissionForDataDelivery(Context context, String permission,

            AttributionSource attributionSource, String message) {

        final int result = PermissionChecker.checkPermissionForDataDeliveryFromDataSource(

                context, permission, PID_UNKNOWN,

                new AttributionSource(context.getAttributionSource(), attributionSource), message);

        if (result == PERMISSION_GRANTED) {

            return true;

        }



        final String msg = "Need " + permission + " permission for " + attributionSource + ": "

                + message;

        if (result == PERMISSION_HARD_DENIED) {

            throw new SecurityException(msg);

        } else {

            Log.w(TAG, msg);

            return false;

        }

    }



    /**

     * Returns true if the BLUETOOTH_CONNECT permission is granted for the calling app. Returns

     * false if the result is a soft denial. Throws SecurityException if the result is a hard
@@ -399,12 +436,7 @@ public final class Utils { 
     */

    @RequiresPermission(android.Manifest.permission.BLUETOOTH_CONNECT)

    public static boolean checkConnectPermissionForPreflight(Context context) {

        int permissionCheckResult = PermissionChecker.checkCallingOrSelfPermissionForPreflight(

                context, BLUETOOTH_CONNECT);

        if (permissionCheckResult == PERMISSION_HARD_DENIED) {

            throw new SecurityException("Need BLUETOOTH_CONNECT permission");

        }

        return permissionCheckResult == PERMISSION_GRANTED;

        return checkPermissionForPreflight(context, BLUETOOTH_CONNECT);

    }



    /**
@@ -418,13 +450,8 @@ public final class Utils { 
    @RequiresPermission(android.Manifest.permission.BLUETOOTH_CONNECT)

    public static boolean checkConnectPermissionForDataDelivery(

            Context context, AttributionSource attributionSource, String message) {

        int permissionCheckResult = PermissionChecker.checkPermissionForDataDeliveryFromDataSource(

                context, BLUETOOTH_CONNECT, PID_UNKNOWN,

                new AttributionSource(context.getAttributionSource(), attributionSource), message);

        if (permissionCheckResult == PERMISSION_HARD_DENIED) {

            throw new SecurityException("Need BLUETOOTH_CONNECT permission");

        }

        return permissionCheckResult == PERMISSION_GRANTED;

        return checkPermissionForDataDelivery(context, BLUETOOTH_CONNECT,

                attributionSource, message);

    }



    /**
@@ -435,12 +462,7 @@ public final class Utils { 
     */

    @RequiresPermission(android.Manifest.permission.BLUETOOTH_SCAN)

    public static boolean checkScanPermissionForPreflight(Context context) {

        int permissionCheckResult = PermissionChecker.checkCallingOrSelfPermissionForPreflight(

                context, BLUETOOTH_SCAN);

        if (permissionCheckResult == PERMISSION_HARD_DENIED) {

            throw new SecurityException("Need BLUETOOTH_SCAN permission");

        }

        return permissionCheckResult == PERMISSION_GRANTED;

        return checkPermissionForPreflight(context, BLUETOOTH_SCAN);

    }



    /**
@@ -453,13 +475,8 @@ public final class Utils { 
    @RequiresPermission(android.Manifest.permission.BLUETOOTH_SCAN)

    public static boolean checkScanPermissionForDataDelivery(

            Context context, AttributionSource attributionSource, String message) {

        int permissionCheckResult = PermissionChecker.checkPermissionForDataDeliveryFromDataSource(

                context, BLUETOOTH_SCAN, PID_UNKNOWN,

                new AttributionSource(context.getAttributionSource(), attributionSource), message);

        if (permissionCheckResult == PERMISSION_HARD_DENIED) {

            throw new SecurityException("Need BLUETOOTH_SCAN permission");

        }

        return permissionCheckResult == PERMISSION_GRANTED;

        return checkPermissionForDataDelivery(context, BLUETOOTH_SCAN,

                attributionSource, message);

    }



    /**
@@ -471,12 +488,7 @@ public final class Utils { 
     */

    @RequiresPermission(android.Manifest.permission.BLUETOOTH_ADVERTISE)

    public static boolean checkAdvertisePermissionForPreflight(Context context) {

        int permissionCheckResult = PermissionChecker.checkCallingOrSelfPermissionForPreflight(

                context, BLUETOOTH_ADVERTISE);

        if (permissionCheckResult == PERMISSION_HARD_DENIED) {

            throw new SecurityException("Need BLUETOOTH_ADVERTISE permission");

        }

        return permissionCheckResult == PERMISSION_GRANTED;

        return checkPermissionForPreflight(context, BLUETOOTH_ADVERTISE);

    }



    /**
@@ -490,13 +502,8 @@ public final class Utils { 
    @RequiresPermission(android.Manifest.permission.BLUETOOTH_ADVERTISE)

    public static boolean checkAdvertisePermissionForDataDelivery(

            Context context, AttributionSource attributionSource, String message) {

        int permissionCheckResult = PermissionChecker.checkPermissionForDataDeliveryFromDataSource(

                context, BLUETOOTH_ADVERTISE, PID_UNKNOWN,

                new AttributionSource(context.getAttributionSource(), attributionSource), message);

        if (permissionCheckResult == PERMISSION_HARD_DENIED) {

            throw new SecurityException("Need BLUETOOTH_ADVERTISE permission");

        }

        return permissionCheckResult == PERMISSION_GRANTED;

        return checkPermissionForDataDelivery(context, BLUETOOTH_ADVERTISE,

                attributionSource, message);

    }



    /**

android/app/src/com/android/bluetooth/btservice/AdapterService.java

+1 −1
Original line number Diff line number Diff line
@@ -1515,7 +1515,7 @@ public class AdapterService extends Service { 
                return new ArrayList<>();

            }



            return service.mDatabaseManager.getMostRecentlyConnectedDevices(attributionSource);

            return service.mDatabaseManager.getMostRecentlyConnectedDevices();

        }



        @Override

android/app/src/com/android/bluetooth/btservice/storage/DatabaseManager.java

+3 −4
Original line number Diff line number Diff line
@@ -613,16 +613,15 @@ public class DatabaseManager { 
     * @return a {@link List} of {@link BluetoothDevice} representing connected bluetooth devices

     * in order of most recently connected

     */

    public List<BluetoothDevice> getMostRecentlyConnectedDevices(

            AttributionSource attributionSource) {

    public List<BluetoothDevice> getMostRecentlyConnectedDevices() {

        List<BluetoothDevice> mostRecentlyConnectedDevices = new ArrayList<>();

        synchronized (mMetadataCache) {

            List<Metadata> sortedMetadata = new ArrayList<>(mMetadataCache.values());

            sortedMetadata.sort((o1, o2) -> Long.compare(o2.last_active_time, o1.last_active_time));

            for (Metadata metadata : sortedMetadata) {

                try {

                    mostRecentlyConnectedDevices.add(

                            new BluetoothDevice(metadata.getAddress(), attributionSource));

                    mostRecentlyConnectedDevices.add(BluetoothAdapter.getDefaultAdapter()

                            .getRemoteDevice(metadata.getAddress()));

                } catch (IllegalArgumentException ex) {

                    Log.d(TAG, "getBondedDevicesOrdered: Invalid address for "

                            + "device " + metadata.getAddress());

android/app/tests/unit/src/com/android/bluetooth/btservice/storage/DatabaseManagerTest.java

+13 −16
Original line number Diff line number Diff line
@@ -17,9 +17,9 @@ 
package com.android.bluetooth.btservice.storage;



import static org.junit.Assert.assertThat;

import static org.mockito.Mockito.any;

import static org.mockito.Mockito.anyInt;

import static org.mockito.Mockito.anyString;

import static org.mockito.ArgumentMatchers.any;

import static org.mockito.ArgumentMatchers.anyInt;

import static org.mockito.ArgumentMatchers.anyString;

import static org.mockito.Mockito.doNothing;

import static org.mockito.Mockito.doReturn;

import static org.mockito.Mockito.times;
@@ -30,7 +30,6 @@ import android.bluetooth.BluetoothA2dp; 
import android.bluetooth.BluetoothAdapter;

import android.bluetooth.BluetoothDevice;

import android.bluetooth.BluetoothProfile;

import android.content.AttributionSource;

import android.content.ContentValues;

import android.database.Cursor;

import android.database.sqlite.SQLiteDatabase;
@@ -70,7 +69,6 @@ public final class DatabaseManagerTest { 
    private BluetoothDevice mTestDevice;

    private BluetoothDevice mTestDevice2;

    private BluetoothDevice mTestDevice3;

    private AttributionSource mAttributionSource;



    private static final String LOCAL_STORAGE = "LocalStorage";

    private static final String TEST_BT_ADDR = "11:22:33:44:55:66";
@@ -105,7 +103,6 @@ public final class DatabaseManagerTest { 
        when(mAdapterService.getPackageManager()).thenReturn(

                InstrumentationRegistry.getTargetContext().getPackageManager());

        mDatabaseManager = new DatabaseManager(mAdapterService);

        mAttributionSource = InstrumentationRegistry.getTargetContext().getAttributionSource();



        BluetoothDevice[] bondedDevices = {mTestDevice};

        doReturn(bondedDevices).when(mAdapterService).getBondedDevices();
@@ -455,7 +452,7 @@ public final class DatabaseManagerTest { 
        Assert.assertTrue(mDatabaseManager

                .mMetadataCache.get(mTestDevice.getAddress()).is_active_a2dp_device);

        List<BluetoothDevice> mostRecentlyConnectedDevicesOrdered =

                mDatabaseManager.getMostRecentlyConnectedDevices(mAttributionSource);

                mDatabaseManager.getMostRecentlyConnectedDevices();

        Assert.assertEquals(mTestDevice, mDatabaseManager.getMostRecentlyConnectedA2dpDevice());

        Assert.assertEquals(1, mostRecentlyConnectedDevicesOrdered.size());

        Assert.assertEquals(mTestDevice, mostRecentlyConnectedDevicesOrdered.get(0));
@@ -470,7 +467,7 @@ public final class DatabaseManagerTest { 
                .mMetadataCache.get(mTestDevice2.getAddress()).is_active_a2dp_device);

        Assert.assertEquals(mTestDevice2, mDatabaseManager.getMostRecentlyConnectedA2dpDevice());

        mostRecentlyConnectedDevicesOrdered =

                mDatabaseManager.getMostRecentlyConnectedDevices(mAttributionSource);

                mDatabaseManager.getMostRecentlyConnectedDevices();

        Assert.assertEquals(2, mostRecentlyConnectedDevicesOrdered.size());

        Assert.assertEquals(mTestDevice2, mostRecentlyConnectedDevicesOrdered.get(0));

        Assert.assertEquals(mTestDevice, mostRecentlyConnectedDevicesOrdered.get(1));
@@ -485,7 +482,7 @@ public final class DatabaseManagerTest { 
                .mMetadataCache.get(mTestDevice2.getAddress()).is_active_a2dp_device);

        Assert.assertEquals(mTestDevice, mDatabaseManager.getMostRecentlyConnectedA2dpDevice());

        mostRecentlyConnectedDevicesOrdered =

                mDatabaseManager.getMostRecentlyConnectedDevices(mAttributionSource);

                mDatabaseManager.getMostRecentlyConnectedDevices();

        Assert.assertEquals(2, mostRecentlyConnectedDevicesOrdered.size());

        Assert.assertEquals(mTestDevice, mostRecentlyConnectedDevicesOrdered.get(0));

        Assert.assertEquals(mTestDevice2, mostRecentlyConnectedDevicesOrdered.get(1));
@@ -500,7 +497,7 @@ public final class DatabaseManagerTest { 
                .mMetadataCache.get(mTestDevice2.getAddress()).is_active_a2dp_device);

        Assert.assertNull(mDatabaseManager.getMostRecentlyConnectedA2dpDevice());

        mostRecentlyConnectedDevicesOrdered =

                mDatabaseManager.getMostRecentlyConnectedDevices(mAttributionSource);

                mDatabaseManager.getMostRecentlyConnectedDevices();

        Assert.assertEquals(2, mostRecentlyConnectedDevicesOrdered.size());

        Assert.assertEquals(mTestDevice, mostRecentlyConnectedDevicesOrdered.get(0));

        Assert.assertEquals(mTestDevice2, mostRecentlyConnectedDevicesOrdered.get(1));
@@ -517,7 +514,7 @@ public final class DatabaseManagerTest { 
                .mMetadataCache.get(mTestDevice3.getAddress()).is_active_a2dp_device);

        Assert.assertNull(mDatabaseManager.getMostRecentlyConnectedA2dpDevice());

        mostRecentlyConnectedDevicesOrdered =

                mDatabaseManager.getMostRecentlyConnectedDevices(mAttributionSource);

                mDatabaseManager.getMostRecentlyConnectedDevices();

        Assert.assertEquals(3, mostRecentlyConnectedDevicesOrdered.size());

        Assert.assertEquals(mTestDevice3, mostRecentlyConnectedDevicesOrdered.get(0));

        Assert.assertEquals(mTestDevice, mostRecentlyConnectedDevicesOrdered.get(1));
@@ -535,7 +532,7 @@ public final class DatabaseManagerTest { 
                .mMetadataCache.get(mTestDevice3.getAddress()).is_active_a2dp_device);

        Assert.assertEquals(mTestDevice, mDatabaseManager.getMostRecentlyConnectedA2dpDevice());

        mostRecentlyConnectedDevicesOrdered =

                mDatabaseManager.getMostRecentlyConnectedDevices(mAttributionSource);

                mDatabaseManager.getMostRecentlyConnectedDevices();

        Assert.assertEquals(3, mostRecentlyConnectedDevicesOrdered.size());

        Assert.assertEquals(mTestDevice, mostRecentlyConnectedDevicesOrdered.get(0));

        Assert.assertEquals(mTestDevice3, mostRecentlyConnectedDevicesOrdered.get(1));
@@ -553,7 +550,7 @@ public final class DatabaseManagerTest { 
                .mMetadataCache.get(mTestDevice3.getAddress()).is_active_a2dp_device);

        Assert.assertEquals(mTestDevice, mDatabaseManager.getMostRecentlyConnectedA2dpDevice());

        mostRecentlyConnectedDevicesOrdered =

                mDatabaseManager.getMostRecentlyConnectedDevices(mAttributionSource);

                mDatabaseManager.getMostRecentlyConnectedDevices();

        Assert.assertEquals(3, mostRecentlyConnectedDevicesOrdered.size());

        Assert.assertEquals(mTestDevice3, mostRecentlyConnectedDevicesOrdered.get(0));

        Assert.assertEquals(mTestDevice, mostRecentlyConnectedDevicesOrdered.get(1));
@@ -571,7 +568,7 @@ public final class DatabaseManagerTest { 
                .mMetadataCache.get(mTestDevice3.getAddress()).is_active_a2dp_device);

        Assert.assertEquals(mTestDevice, mDatabaseManager.getMostRecentlyConnectedA2dpDevice());

        mostRecentlyConnectedDevicesOrdered =

                mDatabaseManager.getMostRecentlyConnectedDevices(mAttributionSource);

                mDatabaseManager.getMostRecentlyConnectedDevices();

        Assert.assertEquals(3, mostRecentlyConnectedDevicesOrdered.size());

        Assert.assertEquals(mTestDevice3, mostRecentlyConnectedDevicesOrdered.get(0));

        Assert.assertEquals(mTestDevice, mostRecentlyConnectedDevicesOrdered.get(1));
@@ -589,7 +586,7 @@ public final class DatabaseManagerTest { 
                .mMetadataCache.get(mTestDevice3.getAddress()).is_active_a2dp_device);

        Assert.assertNull(mDatabaseManager.getMostRecentlyConnectedA2dpDevice());

        mostRecentlyConnectedDevicesOrdered =

                mDatabaseManager.getMostRecentlyConnectedDevices(mAttributionSource);

                mDatabaseManager.getMostRecentlyConnectedDevices();

        Assert.assertEquals(3, mostRecentlyConnectedDevicesOrdered.size());

        Assert.assertEquals(mTestDevice3, mostRecentlyConnectedDevicesOrdered.get(0));

        Assert.assertEquals(mTestDevice, mostRecentlyConnectedDevicesOrdered.get(1));
@@ -607,7 +604,7 @@ public final class DatabaseManagerTest { 
                .mMetadataCache.get(mTestDevice3.getAddress()).is_active_a2dp_device);

        Assert.assertNull(mDatabaseManager.getMostRecentlyConnectedA2dpDevice());

        mostRecentlyConnectedDevicesOrdered =

                mDatabaseManager.getMostRecentlyConnectedDevices(mAttributionSource);

                mDatabaseManager.getMostRecentlyConnectedDevices();

        Assert.assertEquals(3, mostRecentlyConnectedDevicesOrdered.size());

        Assert.assertEquals(mTestDevice3, mostRecentlyConnectedDevicesOrdered.get(0));

        Assert.assertEquals(mTestDevice, mostRecentlyConnectedDevicesOrdered.get(1));