Merge changes Ibff278a6,If6d537a3

* changes:
  Revert "Add NATT keepalive resources and methods into IpSecService"
  Revert "[KA11] Verify fd ownership and allocate resource for NattKeepalive"

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);
     }

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,
+                @Nullable FileDescriptor fd) throws InvalidSocketException {
-                int encapSocketResourceId) 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
@@ -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<Integer, KeepaliveInfo> 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,
+        startNattKeepalive(nai, fd, intervalSeconds, cb, srcAddrString, srcPort,
-                srcPort, dstAddrString, dstPort);
+                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) {

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.

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);
-    }
 }