From 828dad188ceb4c08ead7968924889e088b9b3dda Mon Sep 17 00:00:00 2001 From: junyulai Date: Wed, 27 Mar 2019 11:00:37 +0800 Subject: [PATCH] Block unpriviledged apps which create keepalives with null fd Currently, socketKeepalive implementation is accepting null fd due to backward compatibility with legacy packet keepalive API. However, due to lack of the fd, the service cannot guarantee the port is not reused by another app if the caller release the port for any reason. Thus, grant the null fd access only for priviledged apps. This commit also address some comments from aosp/918533. Bug: 126699232 Test: atest FrameworksNetTests Change-Id: I0baf582ff4ca8af6082c3754e8dfbcd867f39792 --- core/java/android/net/ConnectivityManager.java | 3 +++ .../server/connectivity/KeepaliveTracker.java | 17 +++++++++++++---- .../android/server/ConnectivityServiceTest.java | 17 +++++++++++------ 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/core/java/android/net/ConnectivityManager.java b/core/java/android/net/ConnectivityManager.java index ae93cf0197..4a64128f14 100644 --- a/core/java/android/net/ConnectivityManager.java +++ b/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 diff --git a/services/core/java/com/android/server/connectivity/KeepaliveTracker.java b/services/core/java/com/android/server/connectivity/KeepaliveTracker.java index 722c1a0d9a..d7a57b992e 100644 --- a/services/core/java/com/android/server/connectivity/KeepaliveTracker.java +++ b/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) { @@ -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; } diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 92a865a3dc..8740f079e1 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -4047,8 +4047,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"); @@ -4059,7 +4057,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"); @@ -4179,6 +4178,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); @@ -4186,7 +4186,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)) { @@ -4204,6 +4205,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; @@ -4292,7 +4297,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"); @@ -4311,7 +4315,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();