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

Commit 5fe72f93 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
Merged-In: I76f49fee440726a7c0714385564ddf0e3e8522b5
Change-Id: I9bf6fac218dccc092debe0904e08eb23cc4583c0
parent 859127b7

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

+16 −16
Original line number Diff line number Diff line
@@ -534,9 +534,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) {
@@ -809,9 +809,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();
@@ -843,9 +843,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();
@@ -881,9 +881,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
@@ -119,10 +119,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);

        }
@@ -136,10 +138,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);

        }