From 3ec33a7ef6de5ca373f6904e49a873a350df653e Mon Sep 17 00:00:00 2001 From: Roshan Pius Date: Thu, 1 Apr 2021 11:16:31 -0700 Subject: [PATCH] UwbService: Enforce UWB_RANGING runtime permission i. Use the provided AttributionSource for runtime permission checks. ii. Check for preflight permissions when the callbacks are registered during openRanging. iii. Check for data delivery permission when the callback is providing a new ranging result. Bug: 183904955 Test: Compiles Change-Id: I8f3b42d865ac0d33d579ad4558b3aad6470bba94 --- core/java/android/uwb/IUwbAdapter.aidl | 5 +- core/java/android/uwb/RangingManager.java | 3 +- .../src/android/uwb/RangingManagerTest.java | 30 ++++++--- .../server/uwb/UwbServiceImplTest.java | 67 ++++++++++++++++--- .../com/android/server/uwb/UwbInjector.java | 35 ++++++++++ .../android/server/uwb/UwbServiceImpl.java | 25 +++++-- 6 files changed, 137 insertions(+), 28 deletions(-) diff --git a/core/java/android/uwb/IUwbAdapter.aidl b/core/java/android/uwb/IUwbAdapter.aidl index 30da248e9e87c..62f1c0c3fa0d9 100644 --- a/core/java/android/uwb/IUwbAdapter.aidl +++ b/core/java/android/uwb/IUwbAdapter.aidl @@ -16,6 +16,7 @@ package android.uwb; +import android.content.AttributionSource; import android.os.PersistableBundle; import android.uwb.IUwbAdapterStateCallbacks; import android.uwb.IUwbRangingCallbacks; @@ -77,11 +78,13 @@ interface IUwbAdapter { * If the provided sessionHandle is already open for the calling client, then * #onRangingOpenFailed must be called and the new session must not be opened. * + * @param attributionSource AttributionSource to use for permission enforcement. * @param sessionHandle the session handle to open ranging for * @param rangingCallbacks the callbacks used to deliver ranging information * @param parameters the configuration to use for ranging */ - void openRanging(in SessionHandle sessionHandle, + void openRanging(in AttributionSource attributionSource, + in SessionHandle sessionHandle, in IUwbRangingCallbacks rangingCallbacks, in PersistableBundle parameters); diff --git a/core/java/android/uwb/RangingManager.java b/core/java/android/uwb/RangingManager.java index ff8b91207166a..6bba796005989 100644 --- a/core/java/android/uwb/RangingManager.java +++ b/core/java/android/uwb/RangingManager.java @@ -63,8 +63,7 @@ public class RangingManager extends android.uwb.IUwbRangingCallbacks.Stub { new RangingSession(executor, callbacks, mAdapter, sessionHandle); mRangingSessionTable.put(sessionHandle, session); try { - // TODO: Pass in the attributionSource to the service. - mAdapter.openRanging(sessionHandle, this, params); + mAdapter.openRanging(attributionSource, sessionHandle, this, params); } catch (RemoteException e) { throw e.rethrowFromSystemServer(); } diff --git a/core/tests/uwbtests/src/android/uwb/RangingManagerTest.java b/core/tests/uwbtests/src/android/uwb/RangingManagerTest.java index 5de6d4208bafd..24267e46e9e21 100644 --- a/core/tests/uwbtests/src/android/uwb/RangingManagerTest.java +++ b/core/tests/uwbtests/src/android/uwb/RangingManagerTest.java @@ -57,7 +57,8 @@ public class RangingManagerTest { RangingManager rangingManager = new RangingManager(adapter); RangingSession.Callback callback = mock(RangingSession.Callback.class); rangingManager.openSession(ATTRIBUTION_SOURCE, PARAMS, EXECUTOR, callback); - verify(adapter, times(1)).openRanging(any(), eq(rangingManager), eq(PARAMS)); + verify(adapter, times(1)).openRanging( + eq(ATTRIBUTION_SOURCE), any(), any(), any()); } @Test @@ -80,11 +81,13 @@ public class RangingManagerTest { RangingManager rangingManager = new RangingManager(adapter); rangingManager.openSession(ATTRIBUTION_SOURCE, PARAMS, EXECUTOR, callback1); - verify(adapter, times(1)).openRanging(sessionHandleCaptor.capture(), any(), any()); + verify(adapter, times(1)).openRanging( + eq(ATTRIBUTION_SOURCE), sessionHandleCaptor.capture(), any(), any()); SessionHandle sessionHandle1 = sessionHandleCaptor.getValue(); rangingManager.openSession(ATTRIBUTION_SOURCE, PARAMS, EXECUTOR, callback2); - verify(adapter, times(2)).openRanging(sessionHandleCaptor.capture(), any(), any()); + verify(adapter, times(2)).openRanging( + eq(ATTRIBUTION_SOURCE), sessionHandleCaptor.capture(), any(), any()); SessionHandle sessionHandle2 = sessionHandleCaptor.getValue(); rangingManager.onRangingOpened(sessionHandle1); @@ -106,7 +109,8 @@ public class RangingManagerTest { ArgumentCaptor.forClass(SessionHandle.class); rangingManager.openSession(ATTRIBUTION_SOURCE, PARAMS, EXECUTOR, callback); - verify(adapter, times(1)).openRanging(sessionHandleCaptor.capture(), any(), any()); + verify(adapter, times(1)).openRanging( + eq(ATTRIBUTION_SOURCE), sessionHandleCaptor.capture(), any(), any()); SessionHandle handle = sessionHandleCaptor.getValue(); rangingManager.onRangingOpened(handle); @@ -151,11 +155,13 @@ public class RangingManagerTest { ArgumentCaptor.forClass(SessionHandle.class); rangingManager.openSession(ATTRIBUTION_SOURCE, PARAMS, EXECUTOR, callback1); - verify(adapter, times(1)).openRanging(sessionHandleCaptor.capture(), any(), any()); + verify(adapter, times(1)).openRanging( + eq(ATTRIBUTION_SOURCE), sessionHandleCaptor.capture(), any(), any()); SessionHandle sessionHandle1 = sessionHandleCaptor.getValue(); rangingManager.openSession(ATTRIBUTION_SOURCE, PARAMS, EXECUTOR, callback2); - verify(adapter, times(2)).openRanging(sessionHandleCaptor.capture(), any(), any()); + verify(adapter, times(2)).openRanging( + eq(ATTRIBUTION_SOURCE), sessionHandleCaptor.capture(), any(), any()); SessionHandle sessionHandle2 = sessionHandleCaptor.getValue(); rangingManager.onRangingClosed(sessionHandle1, REASON, PARAMS); @@ -178,12 +184,14 @@ public class RangingManagerTest { RangingManager rangingManager = new RangingManager(adapter); rangingManager.openSession(ATTRIBUTION_SOURCE, PARAMS, EXECUTOR, callback1); - verify(adapter, times(1)).openRanging(sessionHandleCaptor.capture(), any(), any()); + verify(adapter, times(1)).openRanging( + eq(ATTRIBUTION_SOURCE), sessionHandleCaptor.capture(), any(), any()); SessionHandle sessionHandle1 = sessionHandleCaptor.getValue(); rangingManager.onRangingStarted(sessionHandle1, PARAMS); rangingManager.openSession(ATTRIBUTION_SOURCE, PARAMS, EXECUTOR, callback2); - verify(adapter, times(2)).openRanging(sessionHandleCaptor.capture(), any(), any()); + verify(adapter, times(2)).openRanging( + eq(ATTRIBUTION_SOURCE), sessionHandleCaptor.capture(), any(), any()); SessionHandle sessionHandle2 = sessionHandleCaptor.getValue(); rangingManager.onRangingStarted(sessionHandle2, PARAMS); @@ -230,7 +238,8 @@ public class RangingManagerTest { ArgumentCaptor.forClass(SessionHandle.class); rangingManager.openSession(ATTRIBUTION_SOURCE, PARAMS, EXECUTOR, callback); - verify(adapter, times(1)).openRanging(sessionHandleCaptor.capture(), any(), any()); + verify(adapter, times(1)).openRanging( + eq(ATTRIBUTION_SOURCE), sessionHandleCaptor.capture(), any(), any()); SessionHandle handle = sessionHandleCaptor.getValue(); rangingManager.onRangingOpenFailed(handle, reasonIn, PARAMS); @@ -238,7 +247,8 @@ public class RangingManagerTest { // Open a new session rangingManager.openSession(ATTRIBUTION_SOURCE, PARAMS, EXECUTOR, callback); - verify(adapter, times(2)).openRanging(sessionHandleCaptor.capture(), any(), any()); + verify(adapter, times(2)).openRanging( + eq(ATTRIBUTION_SOURCE), sessionHandleCaptor.capture(), any(), any()); handle = sessionHandleCaptor.getValue(); rangingManager.onRangingOpened(handle); diff --git a/services/tests/servicestests/src/com/android/server/uwb/UwbServiceImplTest.java b/services/tests/servicestests/src/com/android/server/uwb/UwbServiceImplTest.java index 37e3060d6a790..11554c7a7dc55 100644 --- a/services/tests/servicestests/src/com/android/server/uwb/UwbServiceImplTest.java +++ b/services/tests/servicestests/src/com/android/server/uwb/UwbServiceImplTest.java @@ -32,6 +32,7 @@ import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import android.content.AttributionSource; import android.content.Context; import android.os.IBinder; import android.os.PersistableBundle; @@ -61,6 +62,11 @@ import org.mockito.MockitoAnnotations; @SmallTest @Presubmit public class UwbServiceImplTest { + private static final int UID = 343453; + private static final String PACKAGE_NAME = "com.uwb.test"; + private static final AttributionSource ATTRIBUTION_SOURCE = + new AttributionSource.Builder(UID).setPackageName(PACKAGE_NAME).build(); + @Mock private IUwbAdapter mVendorService; @Mock private IBinder mVendorServiceBinder; @Mock private Context mContext; @@ -75,6 +81,7 @@ public class UwbServiceImplTest { public void setUp() throws Exception { MockitoAnnotations.initMocks(this); when(mUwbInjector.getVendorService()).thenReturn(mVendorService); + when(mUwbInjector.checkUwbRangingPermissionForDataDelivery(any(), any())).thenReturn(true); when(mVendorService.asBinder()).thenReturn(mVendorServiceBinder); mUwbServiceImpl = new UwbServiceImpl(mContext, mUwbInjector); } @@ -132,10 +139,11 @@ public class UwbServiceImplTest { final IBinder cbBinder = mock(IBinder.class); when(cb.asBinder()).thenReturn(cbBinder); - mUwbServiceImpl.openRanging(sessionHandle, cb, parameters); + mUwbServiceImpl.openRanging(ATTRIBUTION_SOURCE, sessionHandle, cb, parameters); verify(mVendorService).openRanging( - eq(sessionHandle), mRangingCbCaptor.capture(), eq(parameters)); + eq(ATTRIBUTION_SOURCE), eq(sessionHandle), mRangingCbCaptor.capture(), + eq(parameters)); assertThat(mRangingCbCaptor.getValue()).isNotNull(); } @@ -185,10 +193,11 @@ public class UwbServiceImplTest { final IBinder cbBinder = mock(IBinder.class); when(cb.asBinder()).thenReturn(cbBinder); - mUwbServiceImpl.openRanging(sessionHandle, cb, parameters); + mUwbServiceImpl.openRanging(ATTRIBUTION_SOURCE, sessionHandle, cb, parameters); verify(mVendorService).openRanging( - eq(sessionHandle), mRangingCbCaptor.capture(), eq(parameters)); + eq(ATTRIBUTION_SOURCE), eq(sessionHandle), mRangingCbCaptor.capture(), + eq(parameters)); assertThat(mRangingCbCaptor.getValue()).isNotNull(); // Invoke vendor service callbacks and ensure that the corresponding app callback is @@ -245,10 +254,11 @@ public class UwbServiceImplTest { final IBinder cbBinder = mock(IBinder.class); when(cb.asBinder()).thenReturn(cbBinder); - mUwbServiceImpl.openRanging(sessionHandle, cb, parameters); + mUwbServiceImpl.openRanging(ATTRIBUTION_SOURCE, sessionHandle, cb, parameters); verify(mVendorService).openRanging( - eq(sessionHandle), mRangingCbCaptor.capture(), eq(parameters)); + eq(ATTRIBUTION_SOURCE), eq(sessionHandle), mRangingCbCaptor.capture(), + eq(parameters)); assertThat(mRangingCbCaptor.getValue()).isNotNull(); verify(cbBinder).linkToDeath(mClientDeathCaptor.capture(), anyInt()); @@ -278,13 +288,14 @@ public class UwbServiceImplTest { final IBinder cbBinder = mock(IBinder.class); when(cb.asBinder()).thenReturn(cbBinder); - mUwbServiceImpl.openRanging(sessionHandle, cb, parameters); + mUwbServiceImpl.openRanging(ATTRIBUTION_SOURCE, sessionHandle, cb, parameters); verify(mVendorServiceBinder).linkToDeath(mVendorServiceDeathCaptor.capture(), anyInt()); assertThat(mVendorServiceDeathCaptor.getValue()).isNotNull(); verify(mVendorService).openRanging( - eq(sessionHandle), mRangingCbCaptor.capture(), eq(parameters)); + eq(ATTRIBUTION_SOURCE), eq(sessionHandle), mRangingCbCaptor.capture(), + eq(parameters)); assertThat(mRangingCbCaptor.getValue()).isNotNull(); clearInvocations(cb); @@ -311,4 +322,44 @@ public class UwbServiceImplTest { fail(); } catch (SecurityException e) { /* pass */ } } + + @Test + public void testThrowSecurityExceptionWhenOpenRangingCalledWithoutUwbRangingPermission() + throws Exception { + doThrow(new SecurityException()).when(mUwbInjector).enforceUwbRangingPermissionForPreflight( + any()); + + final SessionHandle sessionHandle = new SessionHandle(5); + final IUwbRangingCallbacks cb = mock(IUwbRangingCallbacks.class); + final PersistableBundle parameters = new PersistableBundle(); + final IBinder cbBinder = mock(IBinder.class); + when(cb.asBinder()).thenReturn(cbBinder); + try { + mUwbServiceImpl.openRanging(ATTRIBUTION_SOURCE, sessionHandle, cb, parameters); + fail(); + } catch (SecurityException e) { /* pass */ } + } + + @Test + public void testOnRangingResultCallbackNotSentWithoutUwbRangingPermission() throws Exception { + final SessionHandle sessionHandle = new SessionHandle(5); + final IUwbRangingCallbacks cb = mock(IUwbRangingCallbacks.class); + final PersistableBundle parameters = new PersistableBundle(); + final IBinder cbBinder = mock(IBinder.class); + when(cb.asBinder()).thenReturn(cbBinder); + + mUwbServiceImpl.openRanging(ATTRIBUTION_SOURCE, sessionHandle, cb, parameters); + + verify(mVendorService).openRanging( + eq(ATTRIBUTION_SOURCE), eq(sessionHandle), mRangingCbCaptor.capture(), + eq(parameters)); + assertThat(mRangingCbCaptor.getValue()).isNotNull(); + + when(mUwbInjector.checkUwbRangingPermissionForDataDelivery(any(), any())).thenReturn(false); + + // Ensure the ranging cb is not delivered to the client. + final RangingReport rangingReport = new RangingReport.Builder().build(); + mRangingCbCaptor.getValue().onRangingResult(sessionHandle, rangingReport); + verify(cb, never()).onRangingResult(sessionHandle, rangingReport); + } } diff --git a/services/uwb/java/com/android/server/uwb/UwbInjector.java b/services/uwb/java/com/android/server/uwb/UwbInjector.java index 00c0acabcb3bc..64f1da1c8e16a 100644 --- a/services/uwb/java/com/android/server/uwb/UwbInjector.java +++ b/services/uwb/java/com/android/server/uwb/UwbInjector.java @@ -16,8 +16,13 @@ package com.android.server.uwb; +import static android.Manifest.permission.UWB_RANGING; +import static android.content.PermissionChecker.PERMISSION_GRANTED; + import android.annotation.NonNull; +import android.content.AttributionSource; import android.content.Context; +import android.content.PermissionChecker; import android.os.IBinder; import android.os.ServiceManager; import android.uwb.IUwbAdapter; @@ -45,4 +50,34 @@ public class UwbInjector { if (b == null) return null; return IUwbAdapter.Stub.asInterface(b); } + + /** + * Throws security exception if the UWB_RANGING permission is not granted for the calling app. + * + *

Should be used in situations where the app op should not be noted. + */ + public void enforceUwbRangingPermissionForPreflight( + @NonNull AttributionSource attributionSource) { + if (!attributionSource.checkCallingUid()) { + throw new SecurityException("Invalid attribution source " + attributionSource); + } + int permissionCheckResult = PermissionChecker.checkPermissionForPreflight( + mContext, UWB_RANGING, attributionSource); + if (permissionCheckResult != PERMISSION_GRANTED) { + throw new SecurityException("Caller does not hold UWB_RANGING permission"); + } + } + + /** + * Returns true if the UWB_RANGING permission is granted for the calling app. + * + *

Should be used in situations where data will be delivered and hence the app op should + * be noted. + */ + public boolean checkUwbRangingPermissionForDataDelivery( + @NonNull AttributionSource attributionSource, @NonNull String message) { + int permissionCheckResult = PermissionChecker.checkPermissionForDataDelivery( + mContext, UWB_RANGING, -1, attributionSource, message); + return permissionCheckResult == PERMISSION_GRANTED; + } } diff --git a/services/uwb/java/com/android/server/uwb/UwbServiceImpl.java b/services/uwb/java/com/android/server/uwb/UwbServiceImpl.java index 6c2c0cb994817..b0661fc86dafc 100644 --- a/services/uwb/java/com/android/server/uwb/UwbServiceImpl.java +++ b/services/uwb/java/com/android/server/uwb/UwbServiceImpl.java @@ -17,6 +17,7 @@ package com.android.server.uwb; import android.annotation.NonNull; +import android.content.AttributionSource; import android.content.Context; import android.os.IBinder; import android.os.PersistableBundle; @@ -60,13 +61,16 @@ public class UwbServiceImpl extends IUwbAdapter.Stub implements IBinder.DeathRec * Access to these callbacks are synchronized. */ private class UwbRangingCallbacksWrapper extends IUwbRangingCallbacks.Stub - implements IBinder.DeathRecipient{ + implements IBinder.DeathRecipient { + private final AttributionSource mAttributionSource; private final SessionHandle mSessionHandle; private final IUwbRangingCallbacks mExternalCb; private boolean mIsValid; - UwbRangingCallbacksWrapper(@NonNull SessionHandle sessionHandle, + UwbRangingCallbacksWrapper(@NonNull AttributionSource attributionSource, + @NonNull SessionHandle sessionHandle, @NonNull IUwbRangingCallbacks externalCb) { + mAttributionSource = attributionSource; mSessionHandle = sessionHandle; mExternalCb = externalCb; mIsValid = true; @@ -167,7 +171,12 @@ public class UwbServiceImpl extends IUwbAdapter.Stub implements IBinder.DeathRec RangingReport rangingReport) throws RemoteException { if (!mIsValid) return; - // TODO: Perform runtime permission checks and noteOp. + if (!mUwbInjector.checkUwbRangingPermissionForDataDelivery( + mAttributionSource, "uwb ranging result")) { + Log.e(TAG, "Not delivering ranging result because of permission denial" + + mSessionHandle); + return; + } mExternalCb.onRangingResult(sessionHandle, rangingReport); } @@ -261,22 +270,24 @@ public class UwbServiceImpl extends IUwbAdapter.Stub implements IBinder.DeathRec } @Override - public void openRanging(SessionHandle sessionHandle, IUwbRangingCallbacks rangingCallbacks, + public void openRanging(AttributionSource attributionSource, + SessionHandle sessionHandle, IUwbRangingCallbacks rangingCallbacks, PersistableBundle parameters) throws RemoteException { enforceUwbPrivilegedPermission(); + mUwbInjector.enforceUwbRangingPermissionForPreflight(attributionSource); + UwbRangingCallbacksWrapper wrapperCb = - new UwbRangingCallbacksWrapper(sessionHandle, rangingCallbacks); + new UwbRangingCallbacksWrapper(attributionSource, sessionHandle, rangingCallbacks); synchronized (mCallbacksMap) { mCallbacksMap.put(sessionHandle, wrapperCb); } - getVendorUwbAdapter().openRanging(sessionHandle, wrapperCb, parameters); + getVendorUwbAdapter().openRanging(attributionSource, sessionHandle, wrapperCb, parameters); } @Override public void startRanging(SessionHandle sessionHandle, PersistableBundle parameters) throws RemoteException { enforceUwbPrivilegedPermission(); - // TODO: Perform runtime apermission checks. getVendorUwbAdapter().startRanging(sessionHandle, parameters); }