Merge changes I0baf582f,I4bba01ba

* changes:
  Block unpriviledged apps which create keepalives with null fd
  Fix keepalive don't get removed when lower layer error

core/java/android/net/ConnectivityManager.java

@@ -1934,6 +1934,8 @@ public class ConnectivityManager {
             @NonNull Callback callback) {
         ParcelFileDescriptor dup;
         try {
+            // Dup is needed here as the pfd inside the socket is owned by the IpSecService,
+            // which cannot be obtained by the app process.
             dup = ParcelFileDescriptor.dup(socket.getFileDescriptor());
         } catch (IOException ignored) {
             // Construct an invalid fd, so that if the user later calls start(), it will fail with
@@ -1975,6 +1977,7 @@ public class ConnectivityManager {
             @NonNull Callback callback) {
         ParcelFileDescriptor dup;
         try {
+            // TODO: Consider remove unnecessary dup.
             dup = pfd.dup();
         } catch (IOException ignored) {
             // Construct an invalid fd, so that if the user later calls start(), it will fail with

services/core/java/com/android/server/connectivity/KeepaliveTracker.java

@@ -154,12 +154,19 @@ public class KeepaliveTracker {
             // 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
             // API.
-            // TODO: don't accept null fd after legacy packetKeepalive API is removed.
             try {
                 if (fd != null) {
                     mFd = Os.dup(fd);
                 }  else {
-                    Log.d(TAG, "uid/pid " + mUid + "/" + mPid + " calls with null fd");
+                    Log.d(TAG, toString() + " calls with null fd");
+                    if (!mPrivileged) {
+                        throw new SecurityException(
+                                "null fd is not allowed for unprivileged access.");
+                    }
+                    if (mType == TYPE_TCP) {
+                        throw new IllegalArgumentException(
+                                "null fd is not allowed for tcp socket keepalives.");
+                    }
                     mFd = null;
                 }
             } catch (ErrnoException e) {
@@ -480,7 +487,6 @@ public class KeepaliveTracker {
             }
         } else {
             // Keepalive successfully stopped, or error.
-            ki.mStartedState = KeepaliveInfo.NOT_STARTED;
             if (reason == SUCCESS) {
                 // The message indicated success stopping : don't call handleStopKeepalive.
                 if (DBG) Log.d(TAG, "Successfully stopped keepalive " + slot + " on " + nai.name());
@@ -490,6 +496,7 @@ public class KeepaliveTracker {
                 handleStopKeepalive(nai, slot, reason);
                 if (DBG) Log.d(TAG, "Keepalive " + slot + " on " + nai.name() + " error " + reason);
             }
+            ki.mStartedState = KeepaliveInfo.NOT_STARTED;
         }
     }
 
@@ -531,7 +538,8 @@ public class KeepaliveTracker {
         try {
             ki = new KeepaliveInfo(cb, nai, packet, intervalSeconds,
                     KeepaliveInfo.TYPE_NATT, fd);
-        } catch (InvalidSocketException e) {
+        } catch (InvalidSocketException | IllegalArgumentException | SecurityException e) {
+            Log.e(TAG, "Fail to construct keepalive", e);
             notifyErrorCallback(cb, ERROR_INVALID_SOCKET);
             return;
         }
@@ -570,7 +578,8 @@ public class KeepaliveTracker {
         try {
             ki = new KeepaliveInfo(cb, nai, packet, intervalSeconds,
                     KeepaliveInfo.TYPE_TCP, fd);
-        } catch (InvalidSocketException e) {
+        } catch (InvalidSocketException | IllegalArgumentException | SecurityException e) {
+            Log.e(TAG, "Fail to construct keepalive e=" + e);
             notifyErrorCallback(cb, ERROR_INVALID_SOCKET);
             return;
         }

tests/net/java/com/android/server/ConnectivityServiceTest.java

@@ -4066,8 +4066,6 @@ public class ConnectivityServiceTest {
         // TODO: 1. Move this outside of ConnectivityServiceTest.
         //       2. Make test to verify that Nat-T keepalive socket is created by IpSecService.
         //       3. Mock ipsec service.
-        //       4. Find a free port instead of a fixed port.
-        final int srcPort = 12345;
         final InetAddress myIPv4 = InetAddress.getByName("192.0.2.129");
         final InetAddress notMyIPv4 = InetAddress.getByName("192.0.2.35");
         final InetAddress myIPv6 = InetAddress.getByName("2001:db8::1");
@@ -4078,7 +4076,8 @@ public class ConnectivityServiceTest {
         final int invalidKaInterval = 9;
 
         final IpSecManager mIpSec = (IpSecManager) mContext.getSystemService(Context.IPSEC_SERVICE);
-        final UdpEncapsulationSocket testSocket = mIpSec.openUdpEncapsulationSocket(srcPort);
+        final UdpEncapsulationSocket testSocket = mIpSec.openUdpEncapsulationSocket();
+        final int srcPort = testSocket.getPort();
 
         LinkProperties lp = new LinkProperties();
         lp.setInterfaceName("wlan12");
@@ -4198,6 +4197,7 @@ public class ConnectivityServiceTest {
 
         // Check that keepalive slots start from 1 and increment. The first one gets slot 1.
         mWiFiNetworkAgent.setExpectedKeepaliveSlot(1);
+        int srcPort2 = 0;
         try (SocketKeepalive ka = mCm.createSocketKeepalive(
                 myNet, testSocket, myIPv4, dstIPv4, executor, callback)) {
             ka.start(validKaInterval);
@@ -4205,7 +4205,8 @@ public class ConnectivityServiceTest {
 
             // The second one gets slot 2.
             mWiFiNetworkAgent.setExpectedKeepaliveSlot(2);
-            final UdpEncapsulationSocket testSocket2 = mIpSec.openUdpEncapsulationSocket(6789);
+            final UdpEncapsulationSocket testSocket2 = mIpSec.openUdpEncapsulationSocket();
+            srcPort2 = testSocket2.getPort();
             TestSocketKeepaliveCallback callback2 = new TestSocketKeepaliveCallback(executor);
             try (SocketKeepalive ka2 = mCm.createSocketKeepalive(
                     myNet, testSocket2, myIPv4, dstIPv4, executor, callback2)) {
@@ -4223,6 +4224,10 @@ public class ConnectivityServiceTest {
             }
         }
 
+        // Check that there is no port leaked after all keepalives and sockets are closed.
+        assertFalse(isUdpPortInUse(srcPort));
+        assertFalse(isUdpPortInUse(srcPort2));
+
         mWiFiNetworkAgent.disconnect();
         waitFor(mWiFiNetworkAgent.getDisconnectedCV());
         mWiFiNetworkAgent = null;
@@ -4305,7 +4310,6 @@ public class ConnectivityServiceTest {
     }
 
     private void doTestNattSocketKeepalivesFdWithExecutor(Executor executor) throws Exception {
-        final int srcPort = 12345;
         final InetAddress myIPv4 = InetAddress.getByName("192.0.2.129");
         final InetAddress anyIPv4 = InetAddress.getByName("0.0.0.0");
         final InetAddress dstIPv4 = InetAddress.getByName("8.8.8.8");
@@ -4324,7 +4328,8 @@ public class ConnectivityServiceTest {
 
         // Prepare the target file descriptor, keep only one instance.
         final IpSecManager mIpSec = (IpSecManager) mContext.getSystemService(Context.IPSEC_SERVICE);
-        final UdpEncapsulationSocket testSocket = mIpSec.openUdpEncapsulationSocket(srcPort);
+        final UdpEncapsulationSocket testSocket = mIpSec.openUdpEncapsulationSocket();
+        final int srcPort = testSocket.getPort();
         final ParcelFileDescriptor testPfd =
                 ParcelFileDescriptor.dup(testSocket.getFileDescriptor());
         testSocket.close();