From 22e66597aa50f3462af6c7509b8b5fb39c7d0b94 Mon Sep 17 00:00:00 2001 From: Yuri Lin Date: Mon, 1 Nov 2021 20:34:55 +0000 Subject: [PATCH] Revert^2 for adding permission enforcement for matchesCallFilter. This change limits callers of matchesCallFilter to those who have listener access, contacts read permission, or are system/SysUI/phone. The previous attempt at this change broke phone functionality because the original permissions of this method (system or systemUI or phone) were not necessarily covered by "listener or contacts access". Revert submission 16150992-revert-16077951-yl-mcfperm-TORWDGEVSS Reason for revert: Trying to re-submit changes but with fixing the tests/functionality that they broke Reverted Changes: Ib5c6814de:Revert "Enforce that callers to matchesCallFilter ... Ie93693688:Revert "Test permissions for matchesCallFilter in ... Bug: 204523343 Bug: 192592755 Test: atest IncomingCallTest#testRingOnIncomingCall; NotificationManagerTest Change-Id: Ic250d912d7f9b3351442927d822e9cd2ec632346 --- .../java/android/app/NotificationManager.java | 4 + .../NotificationManagerService.java | 44 +++++++++++ .../NotificationListenersTest.java | 21 +++++- .../NotificationManagerServiceTest.java | 75 +++++++++++++++++++ 4 files changed, 141 insertions(+), 3 deletions(-) diff --git a/core/java/android/app/NotificationManager.java b/core/java/android/app/NotificationManager.java index 9be4adcbec753..717e289dcaead 100644 --- a/core/java/android/app/NotificationManager.java +++ b/core/java/android/app/NotificationManager.java @@ -2579,6 +2579,10 @@ public class NotificationManager { * for more information. *

*

+ * Callers of this method must have notification listener access, permission to read contacts, + * or have system permissions. + *

+ *

* NOTE: This method calls into Contacts, which may take some time, and should not be called * on the main thread. *

diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index f701c2abe0c64..10bd0c4b6e138 100755 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -5083,6 +5083,33 @@ public class NotificationManagerService extends SystemService { @Override public boolean matchesCallFilter(Bundle extras) { + // Because matchesCallFilter may use contact data to filter calls, the callers of this + // method need to either have notification listener access or permission to read + // contacts. + boolean systemAccess = false; + try { + enforceSystemOrSystemUI("INotificationManager.matchesCallFilter"); + systemAccess = true; + } catch (SecurityException e) { + } + + boolean listenerAccess = false; + try { + String[] pkgNames = mPackageManager.getPackagesForUid(Binder.getCallingUid()); + for (int i = 0; i < pkgNames.length; i++) { + // in most cases there should only be one package here + listenerAccess |= mListeners.hasAllowedListener(pkgNames[i], + Binder.getCallingUserHandle().getIdentifier()); + } + } catch (RemoteException e) { + } finally { + if (!systemAccess && !listenerAccess) { + getContext().enforceCallingPermission(Manifest.permission.READ_CONTACTS, + "matchesCallFilter requires listener permission, contacts read access," + + " or system level access"); + } + } + return mZenModeHelper.matchesCallFilter( Binder.getCallingUserHandle(), extras, @@ -10954,6 +10981,23 @@ public class NotificationManagerService extends SystemService { } return false; } + + // Returns whether there is a component with listener access granted that is associated + // with the given package name / user ID. + boolean hasAllowedListener(String packageName, int userId) { + if (packageName == null) { + return false; + } + + // Loop through allowed components to compare package names + List allowedComponents = getAllowedComponents(userId); + for (int i = 0; i < allowedComponents.size(); i++) { + if (allowedComponents.get(i).getPackageName().equals(packageName)) { + return true; + } + } + return false; + } } // TODO (b/194833441): remove when we've fully migrated to a permission diff --git a/services/tests/uiservicestests/src/com/android/server/notification/NotificationListenersTest.java b/services/tests/uiservicestests/src/com/android/server/notification/NotificationListenersTest.java index 50ebffc31035f..d4420bd86fc14 100644 --- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationListenersTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationListenersTest.java @@ -24,8 +24,9 @@ import static com.android.server.notification.NotificationManagerService.Notific import static com.google.common.truth.Truth.assertThat; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyInt; +import static junit.framework.Assert.assertFalse; +import static junit.framework.Assert.assertTrue; + import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.when; @@ -41,7 +42,6 @@ import android.service.notification.NotificationListenerFilter; import android.service.notification.NotificationListenerService; import android.util.ArraySet; import android.util.Pair; -import android.util.Slog; import android.util.TypedXmlPullParser; import android.util.TypedXmlSerializer; import android.util.Xml; @@ -355,4 +355,19 @@ public class NotificationListenersTest extends UiServiceTestCase { .getDisallowedPackages()).isEmpty(); } + @Test + public void testHasAllowedListener() { + final int uid1 = 1, uid2 = 2; + // enable mCn1 but not mCn2 for uid1 + mListeners.addApprovedList(mCn1.flattenToString(), uid1, true); + + // verify that: + // the package for mCn1 has an allowed listener for uid1 and not uid2 + assertTrue(mListeners.hasAllowedListener(mCn1.getPackageName(), uid1)); + assertFalse(mListeners.hasAllowedListener(mCn1.getPackageName(), uid2)); + + // and that mCn2 has no allowed listeners for either user id + assertFalse(mListeners.hasAllowedListener(mCn2.getPackageName(), uid1)); + assertFalse(mListeners.hasAllowedListener(mCn2.getPackageName(), uid2)); + } } diff --git a/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java b/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java index 5694e599edc38..3a9aa1ec36cfa 100755 --- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java @@ -8356,4 +8356,79 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { verify(mPermissionHelper, never()).hasPermission(anyInt()); verify(mPreferencesHelper, never()).getNotificationChannelsBypassingDnd(PKG, mUid); } + + @Test + public void testMatchesCallFilter_noPermissionShouldThrow() throws Exception { + // set the testable NMS to not system uid + mService.isSystemUid = false; + + // make sure a caller without listener access or read_contacts permission can't call + // matchesCallFilter. + when(mListeners.hasAllowedListener(anyString(), anyInt())).thenReturn(false); + doThrow(new SecurityException()).when(mContext).enforceCallingPermission( + eq("android.permission.READ_CONTACTS"), anyString()); + + try { + // shouldn't matter what we're passing in, if we get past this line fail immediately + ((INotificationManager) mService.mService).matchesCallFilter(null); + fail("call to matchesCallFilter with no permissions should fail"); + } catch (SecurityException e) { + // pass + } + } + + @Test + public void testMatchesCallFilter_hasSystemPermission() throws Exception { + // set the testable NMS to system uid + mService.isSystemUid = true; + + // make sure caller doesn't have listener access or read_contacts permission + when(mListeners.hasAllowedListener(anyString(), anyInt())).thenReturn(false); + doThrow(new SecurityException()).when(mContext).enforceCallingPermission( + eq("android.permission.READ_CONTACTS"), anyString()); + + try { + ((INotificationManager) mService.mService).matchesCallFilter(null); + // pass, but check that we actually checked for system permissions + assertTrue(mService.countSystemChecks > 0); + } catch (SecurityException e) { + fail("call to matchesCallFilter with just system permissions should work"); + } + } + + @Test + public void testMatchesCallFilter_hasListenerPermission() throws Exception { + mService.isSystemUid = false; + + // make sure a caller with only listener access and not read_contacts permission can call + // matchesCallFilter. + when(mListeners.hasAllowedListener(anyString(), anyInt())).thenReturn(true); + doThrow(new SecurityException()).when(mContext).enforceCallingPermission( + eq("android.permission.READ_CONTACTS"), anyString()); + + try { + ((INotificationManager) mService.mService).matchesCallFilter(null); + // pass, this is not a functionality test + } catch (SecurityException e) { + fail("call to matchesCallFilter with listener permissions should work"); + } + } + + @Test + public void testMatchesCallFilter_hasContactsPermission() throws Exception { + mService.isSystemUid = false; + + // make sure a caller with only read_contacts permission and not listener access can call + // matchesCallFilter. + when(mListeners.hasAllowedListener(anyString(), anyInt())).thenReturn(false); + doNothing().when(mContext).enforceCallingPermission( + eq("android.permission.READ_CONTACTS"), anyString()); + + try { + ((INotificationManager) mService.mService).matchesCallFilter(null); + // pass, this is not a functionality test + } catch (SecurityException e) { + fail("call to matchesCallFilter with listener permissions should work"); + } + } }