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

Commit 8098771b authored by Brian Delwiche's avatar Brian Delwiche
Browse files

Fix permission bypasses to multiple methods 

Researcher reports that some BT calls across Binder are validating only
BT's own permissions and not the calling app's permissions.  On
investigation this seems to be due to a missing null check in several BT
permissions checks, which allows a malicious app to pass in a null
AttributionSource and therefore produce a stub AttributionSource chain
which does not properly check for the caller's permissions.

Add null checks, and correct tests which assumed a null was a valid
input.

Bug: 242996380
Test: atest UtilsTest
Test: researcher POC
Tag: #security
Ignore-AOSP-First: Security
Change-Id: I76f49fee440726a7c0714385564ddf0e3e8522b5
parent 9fd7b197

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

+16 −16
Original line number Diff line number Diff line
@@ -598,9 +598,9 @@ public final class Utils { 
        }

        // STOPSHIP(b/188391719): enable this security enforcement

        // attributionSource.enforceCallingUid();

        AttributionSource currentAttribution = new AttributionSource

                .Builder(context.getAttributionSource())

                .setNext(attributionSource)

        AttributionSource currentAttribution =

                new AttributionSource.Builder(context.getAttributionSource())

                        .setNext(Objects.requireNonNull(attributionSource))

                        .build();

        PermissionManager pm = context.getSystemService(PermissionManager.class);

        if (pm == null) {
@@ -873,9 +873,9 @@ public final class Utils { 
            Log.e(TAG, "Permission denial: Location is off.");

            return false;

        }

        AttributionSource currentAttribution = new AttributionSource

                .Builder(context.getAttributionSource())

                .setNext(attributionSource)

        AttributionSource currentAttribution =

                new AttributionSource.Builder(context.getAttributionSource())

                        .setNext(Objects.requireNonNull(attributionSource))

                        .build();

        // STOPSHIP(b/188391719): enable this security enforcement

        // attributionSource.enforceCallingUid();
@@ -907,9 +907,9 @@ public final class Utils { 
            return false;

        }



        final AttributionSource currentAttribution = new AttributionSource

                .Builder(context.getAttributionSource())

                .setNext(attributionSource)

        final AttributionSource currentAttribution =

                new AttributionSource.Builder(context.getAttributionSource())

                        .setNext(Objects.requireNonNull(attributionSource))

                        .build();

        // STOPSHIP(b/188391719): enable this security enforcement

        // attributionSource.enforceCallingUid();
@@ -945,9 +945,9 @@ public final class Utils { 
            return false;

        }



        AttributionSource currentAttribution = new AttributionSource

                .Builder(context.getAttributionSource())

                .setNext(attributionSource)

        AttributionSource currentAttribution =

                new AttributionSource.Builder(context.getAttributionSource())

                        .setNext(Objects.requireNonNull(attributionSource))

                        .build();

        // STOPSHIP(b/188391719): enable this security enforcement

        // attributionSource.enforceCallingUid();

android/app/tests/unit/src/com/android/bluetooth/UtilsTest.java

+9 −4
Original line number Diff line number Diff line
@@ -120,10 +120,12 @@ public class UtilsTest { 
        boolean enabledStatus = locationManager.isLocationEnabledForUser(userHandle);



        locationManager.setLocationEnabledForUser(false, userHandle);

        assertThat(Utils.checkCallerHasCoarseLocation(context, null, userHandle)).isFalse();

        assertThat(Utils.checkCallerHasCoarseLocation(

                       context, context.getAttributionSource(), userHandle))

                .isFalse();



        locationManager.setLocationEnabledForUser(true, userHandle);

        Utils.checkCallerHasCoarseLocation(context, null, userHandle);

        Utils.checkCallerHasCoarseLocation(context, context.getAttributionSource(), userHandle);

        if (!enabledStatus) {

            locationManager.setLocationEnabledForUser(false, userHandle);

        }
@@ -137,10 +139,13 @@ public class UtilsTest { 
        boolean enabledStatus = locationManager.isLocationEnabledForUser(userHandle);



        locationManager.setLocationEnabledForUser(false, userHandle);

        assertThat(Utils.checkCallerHasCoarseOrFineLocation(context, null, userHandle)).isFalse();

        assertThat(Utils.checkCallerHasCoarseOrFineLocation(

                       context, context.getAttributionSource(), userHandle))

                .isFalse();



        locationManager.setLocationEnabledForUser(true, userHandle);

        Utils.checkCallerHasCoarseOrFineLocation(context, null, userHandle);

        Utils.checkCallerHasCoarseOrFineLocation(

                context, context.getAttributionSource(), userHandle);

        if (!enabledStatus) {

            locationManager.setLocationEnabledForUser(false, userHandle);

        }

service/src/com/android/server/bluetooth/BtPermissionUtils.java

+3 −1
Original line number Diff line number Diff line
@@ -86,7 +86,9 @@ class BtPermissionUtils { 
            String message) {

        final String permission = BLUETOOTH_CONNECT;

        AttributionSource currentSource =

                new AttributionSource.Builder(ctx.getAttributionSource()).setNext(source).build();

                new AttributionSource.Builder(ctx.getAttributionSource())

                        .setNext(Objects.requireNonNull(source))

                        .build();

        final int result =

                permissionManager.checkPermissionForDataDeliveryFromDataSource(

                        permission, currentSource, message);