From ac2e8f354dd2cd155098a4609aba0a4fbd9e9c8e Mon Sep 17 00:00:00 2001 From: Chiachang Wang Date: Thu, 18 Jan 2024 12:55:11 +0000 Subject: [PATCH] Remove the UID range check for TCP sockets The original patch (aosp/2891011) incorrectly counted TCP sockets with keepalive enabled for VPNs. This issue was caused by the UID range verification. On bypassable VPNs, the socket mark is always correct. It's possible for an app that is not in the UID ranges to establish a socket on the VPN. But that socket should be counted for the purposes of keepalive. The old code would correctly count it and the new code will incorrectly not count it. Also, the UID range verification does not help on non-bypassable VPNs since the socket mark is always the mark of default network, not the socket mark of the VPN. Instead of reverting the entire patch, this commit removes the UID range mechanism. The code refactoring from the original commit (which used structured classes to read socket information) has been kept as it is a valuable improvement. Bug: 320842519 Test: atest FrameworksNetTests Change-Id: I00731796144c982b9fd4d9771f3d0db1f1529a89 --- .../android/server/ConnectivityService.java | 4 +-- .../AutomaticOnOffKeepaliveTracker.java | 33 +++++-------------- .../AutomaticOnOffKeepaliveTrackerTest.java | 31 ++++------------- 3 files changed, 16 insertions(+), 52 deletions(-) diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index 1264b0c967..8190def2b7 100755 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -6294,10 +6294,8 @@ public class ConnectivityService extends IConnectivityManager.Stub if (!networkFound) return; if (underpinnedNetworkFound) { - final NetworkCapabilities underpinnedNc = - getNetworkCapabilitiesInternal(underpinnedNetwork); mKeepaliveTracker.handleMonitorAutomaticKeepalive(ki, - underpinnedNetwork.netId, underpinnedNc.getUids()); + underpinnedNetwork.netId); } else { // If no underpinned network, then make sure the keepalive is running. mKeepaliveTracker.handleMaybeResumeKeepalive(ki); diff --git a/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java b/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java index 94ba9de498..31108fc869 100644 --- a/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java +++ b/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java @@ -50,7 +50,6 @@ import android.system.StructTimeval; import android.util.LocalLog; import android.util.Log; import android.util.Pair; -import android.util.Range; import android.util.SparseArray; import com.android.internal.annotations.VisibleForTesting; @@ -75,7 +74,6 @@ import java.nio.ByteBuffer; import java.util.ArrayList; import java.util.List; import java.util.Objects; -import java.util.Set; /** * Manages automatic on/off socket keepalive requests. @@ -373,27 +371,26 @@ public class AutomaticOnOffKeepaliveTracker { * Determine if any state transition is needed for the specific automatic keepalive. */ public void handleMonitorAutomaticKeepalive(@NonNull final AutomaticOnOffKeepalive ki, - final int vpnNetId, @NonNull Set> vpnUidRanges) { + final int vpnNetId) { // Might happen if the automatic keepalive was removed by the app just as the alarm fires. if (!mAutomaticOnOffKeepalives.contains(ki)) return; if (STATE_ALWAYS_ON == ki.mAutomaticOnOffState) { throw new IllegalStateException("Should not monitor non-auto keepalive"); } - handleMonitorTcpConnections(ki, vpnNetId, vpnUidRanges); + handleMonitorTcpConnections(ki, vpnNetId); } /** * Determine if disable or re-enable keepalive is needed or not based on TCP sockets status. */ - private void handleMonitorTcpConnections(@NonNull AutomaticOnOffKeepalive ki, int vpnNetId, - @NonNull Set> vpnUidRanges) { + private void handleMonitorTcpConnections(@NonNull AutomaticOnOffKeepalive ki, int vpnNetId) { // Might happen if the automatic keepalive was removed by the app just as the alarm fires. if (!mAutomaticOnOffKeepalives.contains(ki)) return; if (STATE_ALWAYS_ON == ki.mAutomaticOnOffState) { throw new IllegalStateException("Should not monitor non-auto keepalive"); } - if (!isAnyTcpSocketConnected(vpnNetId, vpnUidRanges)) { + if (!isAnyTcpSocketConnected(vpnNetId)) { // No TCP socket exists. Stop keepalive if ENABLED, and remain SUSPENDED if currently // SUSPENDED. if (ki.mAutomaticOnOffState == STATE_ENABLED) { @@ -745,7 +742,7 @@ public class AutomaticOnOffKeepaliveTracker { } @VisibleForTesting - boolean isAnyTcpSocketConnected(int netId, @NonNull Set> vpnUidRanges) { + boolean isAnyTcpSocketConnected(int netId) { FileDescriptor fd = null; try { @@ -758,8 +755,7 @@ public class AutomaticOnOffKeepaliveTracker { // Send request for each IP family for (final int family : ADDRESS_FAMILIES) { - if (isAnyTcpSocketConnectedForFamily( - fd, family, networkMark, networkMask, vpnUidRanges)) { + if (isAnyTcpSocketConnectedForFamily(fd, family, networkMark, networkMask)) { return true; } } @@ -773,7 +769,7 @@ public class AutomaticOnOffKeepaliveTracker { } private boolean isAnyTcpSocketConnectedForFamily(FileDescriptor fd, int family, int networkMark, - int networkMask, @NonNull Set> vpnUidRanges) + int networkMask) throws ErrnoException, InterruptedIOException { ensureRunningOnHandlerThread(); // Build SocketDiag messages and cache it. @@ -802,7 +798,7 @@ public class AutomaticOnOffKeepaliveTracker { } final InetDiagMessage diagMsg = (InetDiagMessage) nlMsg; - if (isTargetTcpSocket(diagMsg, networkMark, networkMask, vpnUidRanges)) { + if (isTargetTcpSocket(diagMsg, networkMark, networkMask)) { if (DBG) { Log.d(TAG, String.format("Found open TCP connection by uid %d to %s" + " cookie %d", @@ -828,19 +824,8 @@ public class AutomaticOnOffKeepaliveTracker { return false; } - private static boolean containsUid(Set> ranges, int uid) { - for (final Range range: ranges) { - if (range.contains(uid)) { - return true; - } - } - return false; - } - private boolean isTargetTcpSocket(@NonNull InetDiagMessage diagMsg, - int networkMark, int networkMask, @NonNull Set> vpnUidRanges) { - if (!containsUid(vpnUidRanges, diagMsg.inetDiagMsg.idiag_uid)) return false; - + int networkMark, int networkMask) { final int mark = readSocketDataAndReturnMark(diagMsg); return (mark & networkMask) == networkMark; } diff --git a/tests/unit/java/com/android/server/connectivity/AutomaticOnOffKeepaliveTrackerTest.java b/tests/unit/java/com/android/server/connectivity/AutomaticOnOffKeepaliveTrackerTest.java index 4fcf8a8c63..6cc301d933 100644 --- a/tests/unit/java/com/android/server/connectivity/AutomaticOnOffKeepaliveTrackerTest.java +++ b/tests/unit/java/com/android/server/connectivity/AutomaticOnOffKeepaliveTrackerTest.java @@ -72,9 +72,7 @@ import android.os.Message; import android.os.SystemClock; import android.telephony.SubscriptionManager; import android.test.suitebuilder.annotation.SmallTest; -import android.util.ArraySet; import android.util.Log; -import android.util.Range; import androidx.annotation.NonNull; import androidx.annotation.Nullable; @@ -104,9 +102,7 @@ import java.net.Socket; import java.nio.ByteBuffer; import java.nio.ByteOrder; import java.util.ArrayList; -import java.util.Arrays; import java.util.List; -import java.util.Set; @RunWith(DevSdkIgnoreRunner.class) @SmallTest @@ -236,9 +232,6 @@ public class AutomaticOnOffKeepaliveTrackerTest { private static final byte[] TEST_RESPONSE_BYTES = HexEncoding.decode(TEST_RESPONSE_HEX.toCharArray(), false); - private static final Set> TEST_UID_RANGES = - new ArraySet<>(Arrays.asList(new Range<>(10000, 99999))); - private static class TestKeepaliveInfo { private static List sOpenSockets = new ArrayList<>(); @@ -416,38 +409,28 @@ public class AutomaticOnOffKeepaliveTrackerTest { public void testIsAnyTcpSocketConnected_runOnNonHandlerThread() throws Exception { setupResponseWithSocketExisting(); assertThrows(IllegalStateException.class, - () -> mAOOKeepaliveTracker.isAnyTcpSocketConnected(TEST_NETID, TEST_UID_RANGES)); + () -> mAOOKeepaliveTracker.isAnyTcpSocketConnected(TEST_NETID)); } @Test public void testIsAnyTcpSocketConnected_withTargetNetId() throws Exception { setupResponseWithSocketExisting(); assertTrue(visibleOnHandlerThread(mTestHandler, - () -> mAOOKeepaliveTracker.isAnyTcpSocketConnected(TEST_NETID, TEST_UID_RANGES))); - } - - @Test - public void testIsAnyTcpSocketConnected_noTargetUidSocket() throws Exception { - setupResponseWithSocketExisting(); - // Configured uid(12345) is not in the VPN range. - assertFalse(visibleOnHandlerThread(mTestHandler, - () -> mAOOKeepaliveTracker.isAnyTcpSocketConnected( - TEST_NETID, - new ArraySet<>(Arrays.asList(new Range<>(99999, 99999)))))); + () -> mAOOKeepaliveTracker.isAnyTcpSocketConnected(TEST_NETID))); } @Test public void testIsAnyTcpSocketConnected_withIncorrectNetId() throws Exception { setupResponseWithSocketExisting(); assertFalse(visibleOnHandlerThread(mTestHandler, - () -> mAOOKeepaliveTracker.isAnyTcpSocketConnected(OTHER_NETID, TEST_UID_RANGES))); + () -> mAOOKeepaliveTracker.isAnyTcpSocketConnected(OTHER_NETID))); } @Test public void testIsAnyTcpSocketConnected_noSocketExists() throws Exception { setupResponseWithoutSocketExisting(); assertFalse(visibleOnHandlerThread(mTestHandler, - () -> mAOOKeepaliveTracker.isAnyTcpSocketConnected(TEST_NETID, TEST_UID_RANGES))); + () -> mAOOKeepaliveTracker.isAnyTcpSocketConnected(TEST_NETID))); } private void triggerEventKeepalive(int slot, int reason) { @@ -491,16 +474,14 @@ public class AutomaticOnOffKeepaliveTrackerTest { setupResponseWithoutSocketExisting(); visibleOnHandlerThread( mTestHandler, - () -> mAOOKeepaliveTracker.handleMonitorAutomaticKeepalive( - autoKi, TEST_NETID, TEST_UID_RANGES)); + () -> mAOOKeepaliveTracker.handleMonitorAutomaticKeepalive(autoKi, TEST_NETID)); } private void doResumeKeepalive(AutomaticOnOffKeepalive autoKi) throws Exception { setupResponseWithSocketExisting(); visibleOnHandlerThread( mTestHandler, - () -> mAOOKeepaliveTracker.handleMonitorAutomaticKeepalive( - autoKi, TEST_NETID, TEST_UID_RANGES)); + () -> mAOOKeepaliveTracker.handleMonitorAutomaticKeepalive(autoKi, TEST_NETID)); } private void doStopKeepalive(AutomaticOnOffKeepalive autoKi) throws Exception {