diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 63fd2fda1c..d22a5d27cc 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -27,7 +27,6 @@ import static android.net.ConnectivityManager.getNetworkTypeName; import static android.net.ConnectivityManager.isNetworkTypeValid; import static android.net.INetworkMonitor.NETWORK_TEST_RESULT_PARTIAL_CONNECTIVITY; import static android.net.INetworkMonitor.NETWORK_TEST_RESULT_VALID; -import static android.net.IpSecManager.INVALID_RESOURCE_ID; import static android.net.NetworkCapabilities.NET_CAPABILITY_CAPTIVE_PORTAL; import static android.net.NetworkCapabilities.NET_CAPABILITY_FOREGROUND; import static android.net.NetworkCapabilities.NET_CAPABILITY_INTERNET; @@ -6859,7 +6858,6 @@ public class ConnectivityService extends IConnectivityManager.Stub enforceKeepalivePermission(); mKeepaliveTracker.startNattKeepalive( getNetworkAgentInfoForNetwork(network), null /* fd */, - INVALID_RESOURCE_ID /* Unused */, intervalSeconds, cb, srcAddr, srcPort, dstAddr, NattSocketKeepalive.NATT_PORT); } diff --git a/services/core/java/com/android/server/connectivity/KeepaliveTracker.java b/services/core/java/com/android/server/connectivity/KeepaliveTracker.java index 84887bdb2d..6d6af5eb71 100644 --- a/services/core/java/com/android/server/connectivity/KeepaliveTracker.java +++ b/services/core/java/com/android/server/connectivity/KeepaliveTracker.java @@ -17,7 +17,6 @@ package com.android.server.connectivity; import static android.content.pm.PackageManager.PERMISSION_GRANTED; -import static android.net.IpSecManager.INVALID_RESOURCE_ID; import static android.net.NattSocketKeepalive.NATT_PORT; import static android.net.NetworkAgent.CMD_ADD_KEEPALIVE_PACKET_FILTER; import static android.net.NetworkAgent.CMD_REMOVE_KEEPALIVE_PACKET_FILTER; @@ -38,7 +37,6 @@ import static android.net.SocketKeepalive.SUCCESS; import android.annotation.NonNull; import android.annotation.Nullable; import android.content.Context; -import android.net.IIpSecService; import android.net.ISocketKeepaliveCallback; import android.net.KeepalivePacketData; import android.net.NattKeepalivePacketData; @@ -54,7 +52,6 @@ import android.os.IBinder; import android.os.Message; import android.os.Process; import android.os.RemoteException; -import android.os.ServiceManager; import android.system.ErrnoException; import android.system.Os; import android.util.Log; @@ -92,14 +89,11 @@ public class KeepaliveTracker { private final TcpKeepaliveController mTcpController; @NonNull private final Context mContext; - @NonNull - private final IIpSecService mIpSec; public KeepaliveTracker(Context context, Handler handler) { mConnectivityServiceHandler = handler; mTcpController = new TcpKeepaliveController(handler); mContext = context; - mIpSec = IIpSecService.Stub.asInterface(ServiceManager.getService(Context.IPSEC_SERVICE)); } /** @@ -118,10 +112,6 @@ public class KeepaliveTracker { private final int mType; private final FileDescriptor mFd; - private final int mEncapSocketResourceId; - // Stores the NATT keepalive resource ID returned by IpSecService. - private int mNattIpsecResourceId = INVALID_RESOURCE_ID; - public static final int TYPE_NATT = 1; public static final int TYPE_TCP = 2; @@ -150,8 +140,7 @@ public class KeepaliveTracker { @NonNull KeepalivePacketData packet, int interval, int type, - @Nullable FileDescriptor fd, - int encapSocketResourceId) throws InvalidSocketException { + @Nullable FileDescriptor fd) throws InvalidSocketException { mCallback = callback; mPid = Binder.getCallingPid(); mUid = Binder.getCallingUid(); @@ -162,9 +151,6 @@ public class KeepaliveTracker { mInterval = interval; mType = type; - mEncapSocketResourceId = encapSocketResourceId; - mNattIpsecResourceId = INVALID_RESOURCE_ID; - // For SocketKeepalive, a dup of fd is kept in mFd so the source port from which the // keepalives are sent cannot be reused by another app even if the fd gets closed by // the user. A null is acceptable here for backward compatibility of PacketKeepalive @@ -172,7 +158,7 @@ public class KeepaliveTracker { try { if (fd != null) { mFd = Os.dup(fd); - } else { + } else { Log.d(TAG, toString() + " calls with null fd"); if (!mPrivileged) { throw new SecurityException( @@ -220,8 +206,6 @@ public class KeepaliveTracker { + IpUtils.addressAndPortToString(mPacket.dstAddress, mPacket.dstPort) + " interval=" + mInterval + " uid=" + mUid + " pid=" + mPid + " privileged=" + mPrivileged - + " nattIpsecRId=" + mNattIpsecResourceId - + " encapSocketRId=" + mEncapSocketResourceId + " packetData=" + HexDump.toHexString(mPacket.getPacket()) + " ]"; } @@ -278,51 +262,6 @@ public class KeepaliveTracker { return SUCCESS; } - private int checkAndLockNattKeepaliveResource() { - // Check that apps should be either privileged or fill in an ipsec encapsulated socket - // resource id. - if (mEncapSocketResourceId == INVALID_RESOURCE_ID) { - if (mPrivileged) { - return SUCCESS; - } else { - // Invalid access. - return ERROR_INVALID_SOCKET; - } - } - - // Check if the ipsec encapsulated socket resource id is registered. - final HashMap networkKeepalives = mKeepalives.get(mNai); - if (networkKeepalives == null) { - return ERROR_INVALID_NETWORK; - } - for (KeepaliveInfo ki : networkKeepalives.values()) { - if (ki.mEncapSocketResourceId == mEncapSocketResourceId - && ki.mNattIpsecResourceId != INVALID_RESOURCE_ID) { - Log.d(TAG, "Registered resource found on keepalive " + mSlot - + " when verify NATT socket with uid=" + mUid + " rid=" - + mEncapSocketResourceId); - return ERROR_INVALID_SOCKET; - } - } - - // Ensure that the socket is created by IpSecService, and lock the resource that is - // preserved by IpSecService. If succeed, a resource id is stored to keep tracking - // the resource preserved by IpSecServce and must be released when stopping keepalive. - try { - mNattIpsecResourceId = - mIpSec.lockEncapSocketForNattKeepalive(mEncapSocketResourceId, mUid); - return SUCCESS; - } catch (IllegalArgumentException e) { - // The UID specified does not own the specified UDP encapsulation socket. - Log.d(TAG, "Failed to verify NATT socket with uid=" + mUid + " rid=" - + mEncapSocketResourceId + ": " + e); - } catch (RemoteException e) { - Log.wtf(TAG, "Error calling lockEncapSocketForNattKeepalive with " - + this.toString(), e); - } - return ERROR_INVALID_SOCKET; - } - private int isValid() { synchronized (mNai) { int error = checkInterval(); @@ -336,13 +275,6 @@ public class KeepaliveTracker { void start(int slot) { mSlot = slot; int error = isValid(); - - // Check and lock ipsec resource needed by natt kepalive. This should be only called - // once per keepalive. - if (error == SUCCESS && mType == TYPE_NATT) { - error = checkAndLockNattKeepaliveResource(); - } - if (error == SUCCESS) { Log.d(TAG, "Starting keepalive " + mSlot + " on " + mNai.name()); switch (mType) { @@ -418,20 +350,6 @@ public class KeepaliveTracker { } } - // Release the resource held by keepalive in IpSecService. - if (mNattIpsecResourceId != INVALID_RESOURCE_ID) { - if (mType != TYPE_NATT) { - Log.wtf(TAG, "natt ipsec resource held by incorrect keepalive " - + this.toString()); - } - try { - mIpSec.releaseNattKeepalive(mNattIpsecResourceId, mUid); - } catch (RemoteException e) { - Log.wtf(TAG, "error calling releaseNattKeepalive with " + this.toString(), e); - } - mNattIpsecResourceId = INVALID_RESOURCE_ID; - } - if (reason == SUCCESS) { try { mCallback.onStopped(); @@ -621,7 +539,6 @@ public class KeepaliveTracker { **/ public void startNattKeepalive(@Nullable NetworkAgentInfo nai, @Nullable FileDescriptor fd, - int encapSocketResourceId, int intervalSeconds, @NonNull ISocketKeepaliveCallback cb, @NonNull String srcAddrString, @@ -653,7 +570,7 @@ public class KeepaliveTracker { KeepaliveInfo ki = null; try { ki = new KeepaliveInfo(cb, nai, packet, intervalSeconds, - KeepaliveInfo.TYPE_NATT, fd, encapSocketResourceId); + KeepaliveInfo.TYPE_NATT, fd); } catch (InvalidSocketException | IllegalArgumentException | SecurityException e) { Log.e(TAG, "Fail to construct keepalive", e); notifyErrorCallback(cb, ERROR_INVALID_SOCKET); @@ -693,7 +610,7 @@ public class KeepaliveTracker { KeepaliveInfo ki = null; try { ki = new KeepaliveInfo(cb, nai, packet, intervalSeconds, - KeepaliveInfo.TYPE_TCP, fd, INVALID_RESOURCE_ID /* Unused */); + KeepaliveInfo.TYPE_TCP, fd); } catch (InvalidSocketException | IllegalArgumentException | SecurityException e) { Log.e(TAG, "Fail to construct keepalive e=" + e); notifyErrorCallback(cb, ERROR_INVALID_SOCKET); @@ -712,12 +629,17 @@ public class KeepaliveTracker { **/ public void startNattKeepalive(@Nullable NetworkAgentInfo nai, @Nullable FileDescriptor fd, - int encapSocketResourceId, + int resourceId, int intervalSeconds, @NonNull ISocketKeepaliveCallback cb, @NonNull String srcAddrString, @NonNull String dstAddrString, int dstPort) { + // Ensure that the socket is created by IpSecService. + if (!isNattKeepaliveSocketValid(fd, resourceId)) { + notifyErrorCallback(cb, ERROR_INVALID_SOCKET); + } + // Get src port to adopt old API. int srcPort = 0; try { @@ -728,8 +650,23 @@ public class KeepaliveTracker { } // Forward request to old API. - startNattKeepalive(nai, fd, encapSocketResourceId, intervalSeconds, cb, srcAddrString, - srcPort, dstAddrString, dstPort); + startNattKeepalive(nai, fd, intervalSeconds, cb, srcAddrString, srcPort, + dstAddrString, dstPort); + } + + /** + * Verify if the IPsec NAT-T file descriptor and resource Id hold for IPsec keepalive is valid. + **/ + public static boolean isNattKeepaliveSocketValid(@Nullable FileDescriptor fd, int resourceId) { + // TODO: 1. confirm whether the fd is called from system api or created by IpSecService. + // 2. If the fd is created from the system api, check that it's bounded. And + // call dup to keep the fd open. + // 3. If the fd is created from IpSecService, check if the resource ID is valid. And + // hold the resource needed in IpSecService. + if (null == fd) { + return false; + } + return true; } public void dump(IndentingPrintWriter pw) { diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 3c5bb6a0c6..3a28aca8aa 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -4269,25 +4269,6 @@ public class ConnectivityServiceTest { callback.expectStarted(); ka.stop(); callback.expectStopped(); - - // Check that the same NATT socket cannot be used by 2 keepalives. - try (SocketKeepalive ka2 = mCm.createSocketKeepalive( - myNet, testSocket, myIPv4, dstIPv4, executor, callback)) { - // Check that second keepalive cannot be started if the first one is running. - ka.start(validKaInterval); - callback.expectStarted(); - ka2.start(validKaInterval); - callback.expectError(SocketKeepalive.ERROR_INVALID_SOCKET); - ka.stop(); - callback.expectStopped(); - - // Check that second keepalive can be started/stopped normally if the first one is - // stopped. - ka2.start(validKaInterval); - callback.expectStarted(); - ka2.stop(); - callback.expectStopped(); - } } // Check that deleting the IP address stops the keepalive. @@ -4351,10 +4332,6 @@ public class ConnectivityServiceTest { testSocket.close(); testSocket2.close(); } - - // Check that the closed socket cannot be used to start keepalive. - ka.start(validKaInterval); - callback.expectError(SocketKeepalive.ERROR_INVALID_SOCKET); } // Check that there is no port leaked after all keepalives and sockets are closed. diff --git a/tests/net/java/com/android/server/IpSecServiceTest.java b/tests/net/java/com/android/server/IpSecServiceTest.java index 6b5a2203ce..4a35015044 100644 --- a/tests/net/java/com/android/server/IpSecServiceTest.java +++ b/tests/net/java/com/android/server/IpSecServiceTest.java @@ -118,7 +118,6 @@ public class IpSecServiceTest { INetd mMockNetd; IpSecService.IpSecServiceConfiguration mMockIpSecSrvConfig; IpSecService mIpSecService; - int mUid = Os.getuid(); @Before public void setUp() throws Exception { @@ -666,99 +665,4 @@ public class IpSecServiceTest { mIpSecService.releaseNetId(releasedNetId); assertEquals(releasedNetId, mIpSecService.reserveNetId()); } - - @Test - public void testLockEncapSocketForNattKeepalive() throws Exception { - IpSecUdpEncapResponse udpEncapResp = - mIpSecService.openUdpEncapsulationSocket(0, new Binder()); - assertNotNull(udpEncapResp); - assertEquals(IpSecManager.Status.OK, udpEncapResp.status); - - // Verify no NATT keepalive records upon startup - IpSecService.UserRecord userRecord = mIpSecService.mUserResourceTracker.getUserRecord(mUid); - assertEquals(0, userRecord.mNattKeepaliveRecords.size()); - - int nattKeepaliveResourceId = - mIpSecService.lockEncapSocketForNattKeepalive(udpEncapResp.resourceId, mUid); - - // Validate response, and record was added - assertNotEquals(IpSecManager.INVALID_RESOURCE_ID, nattKeepaliveResourceId); - assertEquals(1, userRecord.mNattKeepaliveRecords.size()); - - // Validate keepalive can be released and removed. - mIpSecService.releaseNattKeepalive(nattKeepaliveResourceId, mUid); - assertEquals(0, userRecord.mNattKeepaliveRecords.size()); - - mIpSecService.closeUdpEncapsulationSocket(udpEncapResp.resourceId); - } - - @Test - public void testLockEncapSocketForNattKeepaliveInvalidUid() throws Exception { - IpSecUdpEncapResponse udpEncapResp = - mIpSecService.openUdpEncapsulationSocket(0, new Binder()); - assertNotNull(udpEncapResp); - assertEquals(IpSecManager.Status.OK, udpEncapResp.status); - - // Verify no NATT keepalive records upon startup - IpSecService.UserRecord userRecord = mIpSecService.mUserResourceTracker.getUserRecord(mUid); - assertEquals(0, userRecord.mNattKeepaliveRecords.size()); - - try { - int nattKeepaliveResourceId = - mIpSecService.lockEncapSocketForNattKeepalive( - udpEncapResp.resourceId, mUid + 1); - fail("Expected SecurityException for invalid user"); - } catch (SecurityException expected) { - } - - // Validate keepalive was not added to lists - assertEquals(0, userRecord.mNattKeepaliveRecords.size()); - } - - @Test - public void testLockEncapSocketForNattKeepaliveInvalidResourceId() throws Exception { - // Verify no NATT keepalive records upon startup - IpSecService.UserRecord userRecord = mIpSecService.mUserResourceTracker.getUserRecord(mUid); - assertEquals(0, userRecord.mNattKeepaliveRecords.size()); - - try { - int nattKeepaliveResourceId = - mIpSecService.lockEncapSocketForNattKeepalive(12345, mUid); - fail("Expected IllegalArgumentException for invalid resource ID"); - } catch (IllegalArgumentException expected) { - } - - // Validate keepalive was not added to lists - assertEquals(0, userRecord.mNattKeepaliveRecords.size()); - } - - @Test - public void testEncapSocketReleasedBeforeKeepaliveReleased() throws Exception { - IpSecUdpEncapResponse udpEncapResp = - mIpSecService.openUdpEncapsulationSocket(0, new Binder()); - assertNotNull(udpEncapResp); - assertEquals(IpSecManager.Status.OK, udpEncapResp.status); - - // Get encap socket record, verify initial starting refcount. - IpSecService.UserRecord userRecord = mIpSecService.mUserResourceTracker.getUserRecord(mUid); - IpSecService.RefcountedResource encapSocketRefcountedRecord = - userRecord.mEncapSocketRecords.getRefcountedResourceOrThrow( - udpEncapResp.resourceId); - assertEquals(1, encapSocketRefcountedRecord.mRefCount); - - // Verify that the reference was added - int nattKeepaliveResourceId = - mIpSecService.lockEncapSocketForNattKeepalive(udpEncapResp.resourceId, mUid); - assertNotEquals(IpSecManager.INVALID_RESOURCE_ID, nattKeepaliveResourceId); - assertEquals(2, encapSocketRefcountedRecord.mRefCount); - - // Close UDP encap socket, but expect the refcountedRecord to still have a reference. - mIpSecService.closeUdpEncapsulationSocket(udpEncapResp.resourceId); - assertEquals(1, encapSocketRefcountedRecord.mRefCount); - - // Verify UDP encap socket cleaned up once reference is removed. Expect -1 if cleanup - // was properly completed. - mIpSecService.releaseNattKeepalive(nattKeepaliveResourceId, mUid); - assertEquals(-1, encapSocketRefcountedRecord.mRefCount); - } }