From 93401340261848ab0545fdf85dc79b16b39277da Mon Sep 17 00:00:00 2001
From: Lorenzo Colitti <lorenzo@google.com>
Date: Wed, 9 Dec 2020 18:30:52 +0900
Subject: [PATCH] Disallow NetworkAgents from changing the owner UID.

The current behaviour with regards to changing the owner UID is
bizarre and arguably incorrect. A NetworkAgent can change the
owner to whatever other app it wants, regardless of signatures,
at any time. This includes, for example, transferring ownership
to another UID and then recovering it.

Fortunately no existing NetworkAgent appears to do this:
- ClientModeImpl sets it to the UID of the app that created the
  configuration. It doesn't look like it can change while the
  network is connected.
- Vpn sets it to the UID of the VPN owner. That also can't change.
- Telephony does not appear to set it at all, it only sets the
  administrator UIDs (and updates them whenever it gets
  EVENT_CARRIER_PRIVILEGED_UIDS_CHANGED).

Disallow this now before code is written that depends on it.

Bug: 175188445
Test: modified tests in ConnectivityServiceTest
Change-Id: I638e29fda2481ec3bf4fff562ea66a73322881df
---
 .../android/server/ConnectivityService.java   |  5 ++++
 .../server/ConnectivityServiceTest.java       | 28 ++++---------------
 2 files changed, 10 insertions(+), 23 deletions(-)

diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java
index 65903af492..6b3e8a9718 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -6355,6 +6355,11 @@ public class ConnectivityService extends IConnectivityManager.Stub
      */
     private void processCapabilitiesFromAgent(NetworkAgentInfo nai, NetworkCapabilities nc) {
         nai.declaredMetered = !nc.hasCapability(NET_CAPABILITY_NOT_METERED);
+        if (nai.networkCapabilities.getOwnerUid() != nc.getOwnerUid()) {
+            Log.e(TAG, nai.toShortString() + ": ignoring attempt to change owner from "
+                    + nai.networkCapabilities.getOwnerUid() + " to " + nc.getOwnerUid());
+            nc.setOwnerUid(nai.networkCapabilities.getOwnerUid());
+        }
     }
 
     /** Modifies |caps| based on the capabilities of the specified underlying networks. */
diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java
index 2aade42476..a4b6e31182 100644
--- a/tests/net/java/com/android/server/ConnectivityServiceTest.java
+++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java
@@ -1958,7 +1958,7 @@ public class ConnectivityServiceTest {
     }
 
     @Test
-    public void testOwnerUidChangeBug() throws Exception {
+    public void testOwnerUidCannotChange() throws Exception {
         // Owner UIDs are not visible without location permission.
         setupLocationPermissions(Build.VERSION_CODES.Q, true, AppOpsManager.OPSTR_FINE_LOCATION,
                 Manifest.permission.ACCESS_FINE_LOCATION);
@@ -1973,37 +1973,19 @@ public class ConnectivityServiceTest {
         waitForIdle();
 
         // Send ConnectivityService an update to the mWiFiNetworkAgent's capabilities that changes
-        // its owner UID.
+        // the owner UID and an unrelated capability.
         NetworkCapabilities agentCapabilities = mWiFiNetworkAgent.getNetworkCapabilities();
         assertEquals(originalOwnerUid, agentCapabilities.getOwnerUid());
         agentCapabilities.setOwnerUid(42);
-        mWiFiNetworkAgent.setNetworkCapabilities(agentCapabilities, true);
-        waitForIdle();
-
-        // Check that the owner UID is updated.
-        // The owner UID is -1 because it is visible only to the UID that owns the network.
-        NetworkCapabilities nc = mCm.getNetworkCapabilities(mWiFiNetworkAgent.getNetwork());
-        assertEquals(-1, nc.getOwnerUid());
-
-        // Make an unrelated change to the capabilities and check it. The owner UID remains -1.
         assertFalse(agentCapabilities.hasCapability(NET_CAPABILITY_NOT_CONGESTED));
         agentCapabilities.addCapability(NET_CAPABILITY_NOT_CONGESTED);
         mWiFiNetworkAgent.setNetworkCapabilities(agentCapabilities, true);
         waitForIdle();
-        nc = mCm.getNetworkCapabilities(mWiFiNetworkAgent.getNetwork());
-        assertEquals(-1, nc.getOwnerUid());
-        assertTrue(nc.hasCapability(NET_CAPABILITY_NOT_CONGESTED));
 
-        // Set the owner back to originalOwnerUid, update the capabilities, and check that it is
-        // visible again.
-        // TODO: should this even be possible?
-        agentCapabilities.setOwnerUid(originalOwnerUid);
-        agentCapabilities.removeCapability(NET_CAPABILITY_NOT_CONGESTED);
-        mWiFiNetworkAgent.setNetworkCapabilities(agentCapabilities, true);
-        waitForIdle();
-
-        nc = mCm.getNetworkCapabilities(mWiFiNetworkAgent.getNetwork());
+        // Check that the capability change has been applied but the owner UID is not modified.
+        NetworkCapabilities nc = mCm.getNetworkCapabilities(mWiFiNetworkAgent.getNetwork());
         assertEquals(originalOwnerUid, nc.getOwnerUid());
+        assertTrue(nc.hasCapability(NET_CAPABILITY_NOT_CONGESTED));
     }
 
     @Test