From a830439b375d228259fbad70f690cb29954ad5cf Mon Sep 17 00:00:00 2001
From: Cody Kesting <ckesting@google.com>
Date: Fri, 7 Feb 2020 14:49:34 -0800
Subject: [PATCH] Protect ConnectivityService from SecurityException in
 permission check.

ConnectivityService currently calls
LocationPermissionChecker#checkLocationPermission. This method call
throws a SecurityException if the given package name and UID do not
match. This permission check is made from the ConnectivityService
Thread, so any Exception being thrown will crash the SystemServer. This
is not acceptable, so surround the permission check in a try-catch in
case any SecurityExceptions are thrown.

Bug: 149119324
Test: atest ConnectivityServiceTest
Change-Id: Ibe2874f2a5249432270aa1b9eb7d004bbba35ac2
---
 .../android/server/ConnectivityService.java   | 11 ++++++++--
 .../server/ConnectivityServiceTest.java       | 20 +++++++++++++++++++
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java
index 6211769882..26b67a9e1e 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -7917,8 +7917,15 @@ public class ConnectivityService extends IConnectivityManager.Stub
             return true;
         }
 
-        if (!mLocationPermissionChecker.checkLocationPermission(
-                callbackPackageName, null /* featureId */, callbackUid, null /* message */)) {
+        // LocationPermissionChecker#checkLocationPermission can throw SecurityException if the uid
+        // and package name don't match. Throwing on the CS thread is not acceptable, so wrap the
+        // call in a try-catch.
+        try {
+            if (!mLocationPermissionChecker.checkLocationPermission(
+                    callbackPackageName, null /* featureId */, callbackUid, null /* message */)) {
+                return false;
+            }
+        } catch (SecurityException e) {
             return false;
         }
 
diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java
index c1999dba69..90ed77b330 100644
--- a/tests/net/java/com/android/server/ConnectivityServiceTest.java
+++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java
@@ -6761,6 +6761,26 @@ public class ConnectivityServiceTest {
                         mContext.getOpPackageName()));
     }
 
+    @Test
+    public void testCheckConnectivityDiagnosticsPermissionsWrongUidPackageName() throws Exception {
+        final NetworkAgentInfo naiWithoutUid =
+                new NetworkAgentInfo(
+                        null, null, null, null, null, new NetworkCapabilities(), 0,
+                        mServiceContext, null, null, mService, null, null, null, 0);
+
+        mServiceContext.setPermission(android.Manifest.permission.NETWORK_STACK, PERMISSION_DENIED);
+
+        try {
+            assertFalse(
+                    "Mismatched uid/package name should not pass the location permission check",
+                    mService.checkConnectivityDiagnosticsPermissions(
+                            Process.myPid() + 1, Process.myUid() + 1, naiWithoutUid,
+                            mContext.getOpPackageName()));
+        } catch (SecurityException e) {
+            fail("checkConnectivityDiagnosticsPermissions shouldn't surface a SecurityException");
+        }
+    }
+
     @Test
     public void testCheckConnectivityDiagnosticsPermissionsNoLocationPermission() throws Exception {
         final NetworkAgentInfo naiWithoutUid =