From 683441d87a7cff3ed09418f05492b7e1c9819ae6 Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Wed, 25 Jul 2018 18:46:19 -0700 Subject: [PATCH 1/3] Disable attempted updating of SA marks SA marks are never updated during the UPDSA call. This change disables the attempts to update the specified SAs, ensuring that the config stored in IpSecService matches that of the allocated kernel resources. Bug: 111854872 Test: Unit, CTS tests passing Change-Id: Ic1fb862c8021ffa260c3e262ec698d8af0a826d9 --- core/java/android/net/IpSecConfig.java | 14 ++++++++++++- .../java/com/android/server/IpSecService.java | 20 ++++++++++++++----- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/core/java/android/net/IpSecConfig.java b/core/java/android/net/IpSecConfig.java index 8599f47c62..c7eb2dd7a3 100644 --- a/core/java/android/net/IpSecConfig.java +++ b/core/java/android/net/IpSecConfig.java @@ -65,7 +65,7 @@ public final class IpSecConfig implements Parcelable { // An interval, in seconds between the NattKeepalive packets private int mNattKeepaliveInterval; - // XFRM mark and mask + // XFRM mark and mask; defaults to 0 (no mark/mask) private int mMarkValue; private int mMarkMask; @@ -125,10 +125,22 @@ public final class IpSecConfig implements Parcelable { mNattKeepaliveInterval = interval; } + /** + * Sets the mark value + * + *

Internal (System server) use only. Marks passed in by users will be overwritten or + * ignored. + */ public void setMarkValue(int mark) { mMarkValue = mark; } + /** + * Sets the mark mask + * + *

Internal (System server) use only. Marks passed in by users will be overwritten or + * ignored. + */ public void setMarkMask(int mask) { mMarkMask = mask; } diff --git a/services/core/java/com/android/server/IpSecService.java b/services/core/java/com/android/server/IpSecService.java index 8c25917c74..c4b749011a 100644 --- a/services/core/java/com/android/server/IpSecService.java +++ b/services/core/java/com/android/server/IpSecService.java @@ -24,6 +24,7 @@ import static android.system.OsConstants.AF_UNSPEC; import static android.system.OsConstants.EINVAL; import static android.system.OsConstants.IPPROTO_UDP; import static android.system.OsConstants.SOCK_DGRAM; + import static com.android.internal.util.Preconditions.checkNotNull; import android.annotation.NonNull; @@ -62,6 +63,8 @@ import com.android.internal.annotations.GuardedBy; import com.android.internal.annotations.VisibleForTesting; import com.android.internal.util.Preconditions; +import libcore.io.IoUtils; + import java.io.FileDescriptor; import java.io.IOException; import java.io.PrintWriter; @@ -73,8 +76,6 @@ import java.net.UnknownHostException; import java.util.ArrayList; import java.util.List; -import libcore.io.IoUtils; - /** * A service to manage multiple clients that want to access the IpSec API. The service is * responsible for maintaining a list of clients and managing the resources (and related quotas) @@ -1523,6 +1524,9 @@ public class IpSecService extends IIpSecService.Stub { throw new IllegalArgumentException( "Invalid IpSecTransform.mode: " + config.getMode()); } + + config.setMarkValue(0); + config.setMarkMask(0); } private static final String TUNNEL_OP = AppOpsManager.OPSTR_MANAGE_IPSEC_TUNNELS; @@ -1740,8 +1744,14 @@ public class IpSecService extends IIpSecService.Stub { : tunnelInterfaceInfo.getIkey(); try { - c.setMarkValue(mark); - c.setMarkMask(0xffffffff); + // TODO: enable this when UPDSA supports updating marks. Adding kernel support upstream + // (and backporting) would allow us to narrow the mark space, and ensure that the SA + // and SPs have matching marks (as VTI are meant to be built). + // Currently update does nothing with marks. Leave empty (defaulting to 0) to ensure the + // config matches the actual allocated resources in the kernel. + // + // c.setMarkValue(mark); + // c.setMarkMask(0xffffffff); if (direction == IpSecManager.DIRECTION_OUT) { // Set output mark via underlying network (output only) @@ -1758,7 +1768,7 @@ public class IpSecService extends IIpSecService.Stub { tunnelInterfaceInfo.getLocalAddress(), tunnelInterfaceInfo.getRemoteAddress(), transformInfo.getSpiRecord().getSpi(), - mark, + mark, // Must always set policy mark; ikey/okey for VTIs 0xffffffff); } } From 5d74984c2489b3e898767c831932cad8d75e2f3a Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Thu, 6 Sep 2018 11:31:25 -0700 Subject: [PATCH 2/3] Add XFRM-I support to IpSecService This change adds support for XFRM-I to all IpSecService netd calls. Fallback logic is in netd, and thus both VTI and XFRM-I parameters are always passed down to IpSecService. Bug: 78589502 Test: All java, CTS tests passing Change-Id: Ie4186f0ad7e50763b21831f6fa411b5ee436de78 --- core/java/android/net/IpSecConfig.java | 22 +++++- .../java/com/android/server/IpSecService.java | 76 +++++++++++++------ 2 files changed, 70 insertions(+), 28 deletions(-) diff --git a/core/java/android/net/IpSecConfig.java b/core/java/android/net/IpSecConfig.java index c7eb2dd7a3..3552655983 100644 --- a/core/java/android/net/IpSecConfig.java +++ b/core/java/android/net/IpSecConfig.java @@ -69,6 +69,9 @@ public final class IpSecConfig implements Parcelable { private int mMarkValue; private int mMarkMask; + // XFRM interface id + private int mXfrmInterfaceId; + /** Set the mode for this IPsec transform */ public void setMode(int mode) { mMode = mode; @@ -145,6 +148,10 @@ public final class IpSecConfig implements Parcelable { mMarkMask = mask; } + public void setXfrmInterfaceId(int xfrmInterfaceId) { + mXfrmInterfaceId = xfrmInterfaceId; + } + // Transport or Tunnel public int getMode() { return mMode; @@ -202,6 +209,10 @@ public final class IpSecConfig implements Parcelable { return mMarkMask; } + public int getXfrmInterfaceId() { + return mXfrmInterfaceId; + } + // Parcelable Methods @Override @@ -225,6 +236,7 @@ public final class IpSecConfig implements Parcelable { out.writeInt(mNattKeepaliveInterval); out.writeInt(mMarkValue); out.writeInt(mMarkMask); + out.writeInt(mXfrmInterfaceId); } @VisibleForTesting @@ -247,6 +259,7 @@ public final class IpSecConfig implements Parcelable { mNattKeepaliveInterval = c.mNattKeepaliveInterval; mMarkValue = c.mMarkValue; mMarkMask = c.mMarkMask; + mXfrmInterfaceId = c.mXfrmInterfaceId; } private IpSecConfig(Parcel in) { @@ -267,6 +280,7 @@ public final class IpSecConfig implements Parcelable { mNattKeepaliveInterval = in.readInt(); mMarkValue = in.readInt(); mMarkMask = in.readInt(); + mXfrmInterfaceId = in.readInt(); } @Override @@ -301,6 +315,8 @@ public final class IpSecConfig implements Parcelable { .append(mMarkValue) .append(", mMarkMask=") .append(mMarkMask) + .append(", mXfrmInterfaceId=") + .append(mXfrmInterfaceId) .append("}"); return strBuilder.toString(); @@ -332,10 +348,10 @@ public final class IpSecConfig implements Parcelable { && lhs.mNattKeepaliveInterval == rhs.mNattKeepaliveInterval && lhs.mSpiResourceId == rhs.mSpiResourceId && IpSecAlgorithm.equals(lhs.mEncryption, rhs.mEncryption) - && IpSecAlgorithm.equals( - lhs.mAuthenticatedEncryption, rhs.mAuthenticatedEncryption) + && IpSecAlgorithm.equals(lhs.mAuthenticatedEncryption, rhs.mAuthenticatedEncryption) && IpSecAlgorithm.equals(lhs.mAuthentication, rhs.mAuthentication) && lhs.mMarkValue == rhs.mMarkValue - && lhs.mMarkMask == rhs.mMarkMask); + && lhs.mMarkMask == rhs.mMarkMask + && lhs.mXfrmInterfaceId == rhs.mXfrmInterfaceId); } } diff --git a/services/core/java/com/android/server/IpSecService.java b/services/core/java/com/android/server/IpSecService.java index c4b749011a..71a7a2b5ab 100644 --- a/services/core/java/com/android/server/IpSecService.java +++ b/services/core/java/com/android/server/IpSecService.java @@ -622,7 +622,8 @@ public class IpSecService extends IIpSecService.Stub { mConfig.getDestinationAddress(), spi, mConfig.getMarkValue(), - mConfig.getMarkMask()); + mConfig.getMarkMask(), + mConfig.getXfrmInterfaceId()); } catch (RemoteException | ServiceSpecificException e) { Log.e(TAG, "Failed to delete SA with ID: " + mResourceId, e); } @@ -684,7 +685,8 @@ public class IpSecService extends IIpSecService.Stub { mSrvConfig .getNetdInstance() .ipSecDeleteSecurityAssociation( - uid, mSourceAddress, mDestinationAddress, mSpi, 0, 0); + uid, mSourceAddress, mDestinationAddress, mSpi, 0 /* mark */, + 0 /* mask */, 0 /* if_id */); } } catch (ServiceSpecificException | RemoteException e) { Log.e(TAG, "Failed to delete SPI reservation with ID: " + mResourceId, e); @@ -796,6 +798,8 @@ public class IpSecService extends IIpSecService.Stub { private final int mIkey; private final int mOkey; + private final int mIfId; + TunnelInterfaceRecord( int resourceId, String interfaceName, @@ -803,7 +807,8 @@ public class IpSecService extends IIpSecService.Stub { String localAddr, String remoteAddr, int ikey, - int okey) { + int okey, + int intfId) { super(resourceId); mInterfaceName = interfaceName; @@ -812,6 +817,7 @@ public class IpSecService extends IIpSecService.Stub { mRemoteAddress = remoteAddr; mIkey = ikey; mOkey = okey; + mIfId = intfId; } /** always guarded by IpSecService#this */ @@ -822,7 +828,7 @@ public class IpSecService extends IIpSecService.Stub { // Delete global policies try { final INetd netd = mSrvConfig.getNetdInstance(); - netd.removeVirtualTunnelInterface(mInterfaceName); + netd.ipSecRemoveTunnelInterface(mInterfaceName); for (int selAddrFamily : ADDRESS_FAMILIES) { netd.ipSecDeleteSecurityPolicy( @@ -830,13 +836,15 @@ public class IpSecService extends IIpSecService.Stub { selAddrFamily, IpSecManager.DIRECTION_OUT, mOkey, - 0xffffffff); + 0xffffffff, + mIfId); netd.ipSecDeleteSecurityPolicy( uid, selAddrFamily, IpSecManager.DIRECTION_IN, mIkey, - 0xffffffff); + 0xffffffff, + mIfId); } } catch (ServiceSpecificException | RemoteException e) { Log.e( @@ -878,6 +886,10 @@ public class IpSecService extends IIpSecService.Stub { return mOkey; } + public int getIfId() { + return mIfId; + } + @Override protected ResourceTracker getResourceTracker() { return getUserRecord().mTunnelQuotaTracker; @@ -1287,7 +1299,7 @@ public class IpSecService extends IIpSecService.Stub { // Add inbound/outbound global policies // (use reqid = 0) final INetd netd = mSrvConfig.getNetdInstance(); - netd.addVirtualTunnelInterface(intfName, localAddr, remoteAddr, ikey, okey); + netd.ipSecAddTunnelInterface(intfName, localAddr, remoteAddr, ikey, okey, resourceId); for (int selAddrFamily : ADDRESS_FAMILIES) { // Always send down correct local/remote addresses for template. @@ -1299,7 +1311,8 @@ public class IpSecService extends IIpSecService.Stub { remoteAddr, 0, okey, - 0xffffffff); + 0xffffffff, + resourceId); netd.ipSecAddSecurityPolicy( callerUid, selAddrFamily, @@ -1308,7 +1321,8 @@ public class IpSecService extends IIpSecService.Stub { localAddr, 0, ikey, - 0xffffffff); + 0xffffffff, + resourceId); } userRecord.mTunnelInterfaceRecords.put( @@ -1321,7 +1335,8 @@ public class IpSecService extends IIpSecService.Stub { localAddr, remoteAddr, ikey, - okey), + okey, + resourceId), binder)); return new IpSecTunnelInterfaceResponse(IpSecManager.Status.OK, resourceId, intfName); } catch (RemoteException e) { @@ -1588,7 +1603,8 @@ public class IpSecService extends IIpSecService.Stub { (authCrypt != null) ? authCrypt.getTruncationLengthBits() : 0, encapType, encapLocalPort, - encapRemotePort); + encapRemotePort, + c.getXfrmInterfaceId()); } /** @@ -1744,6 +1760,11 @@ public class IpSecService extends IIpSecService.Stub { : tunnelInterfaceInfo.getIkey(); try { + // Default to using the invalid SPI of 0 for inbound SAs. This allows policies to skip + // SPI matching as part of the template resolution. + int spi = IpSecManager.INVALID_SECURITY_PARAMETER_INDEX; + c.setXfrmInterfaceId(tunnelInterfaceInfo.getIfId()); + // TODO: enable this when UPDSA supports updating marks. Adding kernel support upstream // (and backporting) would allow us to narrow the mark space, and ensure that the SA // and SPs have matching marks (as VTI are meant to be built). @@ -1757,20 +1778,25 @@ public class IpSecService extends IIpSecService.Stub { // Set output mark via underlying network (output only) c.setNetwork(tunnelInterfaceInfo.getUnderlyingNetwork()); - // If outbound, also add SPI to the policy. - for (int selAddrFamily : ADDRESS_FAMILIES) { - mSrvConfig - .getNetdInstance() - .ipSecUpdateSecurityPolicy( - callingUid, - selAddrFamily, - direction, - tunnelInterfaceInfo.getLocalAddress(), - tunnelInterfaceInfo.getRemoteAddress(), - transformInfo.getSpiRecord().getSpi(), - mark, // Must always set policy mark; ikey/okey for VTIs - 0xffffffff); - } + // Set outbound SPI only. We want inbound to use any valid SA (old, new) on rekeys, + // but want to guarantee outbound packets are sent over the new SA. + spi = transformInfo.getSpiRecord().getSpi(); + } + + // Always update the policy with the relevant XFRM_IF_ID + for (int selAddrFamily : ADDRESS_FAMILIES) { + mSrvConfig + .getNetdInstance() + .ipSecUpdateSecurityPolicy( + callingUid, + selAddrFamily, + direction, + transformInfo.getConfig().getSourceAddress(), + transformInfo.getConfig().getDestinationAddress(), + spi, // If outbound, also add SPI to the policy. + mark, // Must always set policy mark; ikey/okey for VTIs + 0xffffffff, + c.getXfrmInterfaceId()); } // Update SA with tunnel mark (ikey or okey based on direction) From c6fcedd7790fd703f3b49de5bd54abe50812c99d Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Wed, 21 Nov 2018 21:24:55 -0800 Subject: [PATCH 3/3] Cleanup and update comments SA mark disabling comments This commit cleans up and upates comments with regard to changes in aosp/721999, clarifying the restrictions and potential pitfalls we would see with regards to IPsec tunnel mode without updatable SAs. Bug: 111854872 Test: Compiles, comment-only change Change-Id: I07b0063987463c1a3cf42e112839a31739947c80 --- services/core/java/com/android/server/IpSecService.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/services/core/java/com/android/server/IpSecService.java b/services/core/java/com/android/server/IpSecService.java index 71a7a2b5ab..7ee3d3b3bd 100644 --- a/services/core/java/com/android/server/IpSecService.java +++ b/services/core/java/com/android/server/IpSecService.java @@ -1770,6 +1770,11 @@ public class IpSecService extends IIpSecService.Stub { // and SPs have matching marks (as VTI are meant to be built). // Currently update does nothing with marks. Leave empty (defaulting to 0) to ensure the // config matches the actual allocated resources in the kernel. + // All SAs will have zero marks (from creation time), and any policy that matches the + // same src/dst could match these SAs. Non-IpSecService governed processes that + // establish floating policies with the same src/dst may result in undefined + // behavior. This is generally limited to vendor code due to the permissions + // (CAP_NET_ADMIN) required. // // c.setMarkValue(mark); // c.setMarkMask(0xffffffff);