Merge "[KA11] Verify fd ownership and allocate resource for NattKeepalive"
am: fb36d44462
Change-Id: I2f5e0e11494866b054668c9224a8732cd8c035f9
diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java
index ed6ca99d..94ab1ac 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -27,6 +27,7 @@
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;
@@ -6842,6 +6843,7 @@
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 77a18e2..bde430c 100644
--- a/services/core/java/com/android/server/connectivity/KeepaliveTracker.java
+++ b/services/core/java/com/android/server/connectivity/KeepaliveTracker.java
@@ -17,6 +17,7 @@
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;
@@ -37,6 +38,7 @@
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;
@@ -52,6 +54,7 @@
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;
@@ -89,11 +92,14 @@
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));
}
/**
@@ -112,6 +118,10 @@
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;
@@ -140,7 +150,8 @@
@NonNull KeepalivePacketData packet,
int interval,
int type,
- @Nullable FileDescriptor fd) throws InvalidSocketException {
+ @Nullable FileDescriptor fd,
+ int encapSocketResourceId) throws InvalidSocketException {
mCallback = callback;
mPid = Binder.getCallingPid();
mUid = Binder.getCallingUid();
@@ -151,6 +162,9 @@
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
@@ -158,7 +172,7 @@
try {
if (fd != null) {
mFd = Os.dup(fd);
- } else {
+ } else {
Log.d(TAG, toString() + " calls with null fd");
if (!mPrivileged) {
throw new SecurityException(
@@ -206,6 +220,8 @@
+ IpUtils.addressAndPortToString(mPacket.dstAddress, mPacket.dstPort)
+ " interval=" + mInterval
+ " uid=" + mUid + " pid=" + mPid + " privileged=" + mPrivileged
+ + " nattIpsecRId=" + mNattIpsecResourceId
+ + " encapSocketRId=" + mEncapSocketResourceId
+ " packetData=" + HexDump.toHexString(mPacket.getPacket())
+ " ]";
}
@@ -262,6 +278,51 @@
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();
@@ -275,6 +336,13 @@
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) {
@@ -350,6 +418,20 @@
}
}
+ // 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();
@@ -538,6 +620,7 @@
**/
public void startNattKeepalive(@Nullable NetworkAgentInfo nai,
@Nullable FileDescriptor fd,
+ int encapSocketResourceId,
int intervalSeconds,
@NonNull ISocketKeepaliveCallback cb,
@NonNull String srcAddrString,
@@ -569,7 +652,7 @@
KeepaliveInfo ki = null;
try {
ki = new KeepaliveInfo(cb, nai, packet, intervalSeconds,
- KeepaliveInfo.TYPE_NATT, fd);
+ KeepaliveInfo.TYPE_NATT, fd, encapSocketResourceId);
} catch (InvalidSocketException | IllegalArgumentException | SecurityException e) {
Log.e(TAG, "Fail to construct keepalive", e);
notifyErrorCallback(cb, ERROR_INVALID_SOCKET);
@@ -609,7 +692,7 @@
KeepaliveInfo ki = null;
try {
ki = new KeepaliveInfo(cb, nai, packet, intervalSeconds,
- KeepaliveInfo.TYPE_TCP, fd);
+ KeepaliveInfo.TYPE_TCP, fd, INVALID_RESOURCE_ID /* Unused */);
} catch (InvalidSocketException | IllegalArgumentException | SecurityException e) {
Log.e(TAG, "Fail to construct keepalive e=" + e);
notifyErrorCallback(cb, ERROR_INVALID_SOCKET);
@@ -628,17 +711,12 @@
**/
public void startNattKeepalive(@Nullable NetworkAgentInfo nai,
@Nullable FileDescriptor fd,
- int resourceId,
+ int encapSocketResourceId,
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 {
@@ -649,23 +727,8 @@
}
// Forward request to old API.
- 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;
+ startNattKeepalive(nai, fd, encapSocketResourceId, intervalSeconds, cb, srcAddrString,
+ srcPort, dstAddrString, dstPort);
}
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 e3c6c41..64672bd8a 100644
--- a/tests/net/java/com/android/server/ConnectivityServiceTest.java
+++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java
@@ -4228,6 +4228,25 @@
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.
@@ -4291,6 +4310,10 @@
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.