From ab80e1fb303e0e28b2638212b9038f049ddf2ba2 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 8599f47c62454..c7eb2dd7a3754 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 8c25917c74362..c4b749011a941 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 781dae6306b4d2b48f398f687e1c9d276b7a87ca 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 +++++++++++++------ .../net/java/android/net/IpSecConfigTest.java | 2 + .../server/IpSecServiceParameterizedTest.java | 54 ++++++++++++- 4 files changed, 122 insertions(+), 32 deletions(-) diff --git a/core/java/android/net/IpSecConfig.java b/core/java/android/net/IpSecConfig.java index c7eb2dd7a3754..355265598365a 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 c4b749011a941..71a7a2b5abd36 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) diff --git a/tests/net/java/android/net/IpSecConfigTest.java b/tests/net/java/android/net/IpSecConfigTest.java index 771faaf4955d2..be1a45501bb19 100644 --- a/tests/net/java/android/net/IpSecConfigTest.java +++ b/tests/net/java/android/net/IpSecConfigTest.java @@ -47,6 +47,7 @@ public class IpSecConfigTest { assertNull(c.getEncryption()); assertNull(c.getAuthentication()); assertEquals(IpSecManager.INVALID_RESOURCE_ID, c.getSpiResourceId()); + assertEquals(0, c.getXfrmInterfaceId()); } private IpSecConfig getSampleConfig() { @@ -77,6 +78,7 @@ public class IpSecConfigTest { c.setNattKeepaliveInterval(42); c.setMarkValue(12); c.setMarkMask(23); + c.setXfrmInterfaceId(34); return c; } diff --git a/tests/net/java/com/android/server/IpSecServiceParameterizedTest.java b/tests/net/java/com/android/server/IpSecServiceParameterizedTest.java index 9b919abfa41d5..4dc0341c8e2bf 100644 --- a/tests/net/java/com/android/server/IpSecServiceParameterizedTest.java +++ b/tests/net/java/com/android/server/IpSecServiceParameterizedTest.java @@ -71,6 +71,9 @@ public class IpSecServiceParameterizedTest { private final LinkAddress mLocalInnerAddress; private final int mFamily; + private static final int[] ADDRESS_FAMILIES = + new int[] {AF_INET, AF_INET6}; + @Parameterized.Parameters public static Collection ipSecConfigs() { return Arrays.asList( @@ -196,6 +199,7 @@ public class IpSecServiceParameterizedTest { anyString(), eq(TEST_SPI), anyInt(), + anyInt(), anyInt()); // Verify quota and RefcountedResource objects cleaned up @@ -231,6 +235,7 @@ public class IpSecServiceParameterizedTest { anyString(), eq(TEST_SPI), anyInt(), + anyInt(), anyInt()); // Verify quota and RefcountedResource objects cleaned up @@ -304,7 +309,8 @@ public class IpSecServiceParameterizedTest { eq((authCrypt != null) ? authCrypt.getTruncationLengthBits() : 0), eq(config.getEncapType()), eq(encapSocketPort), - eq(config.getEncapRemotePort())); + eq(config.getEncapRemotePort()), + eq(config.getXfrmInterfaceId())); } @Test @@ -430,6 +436,7 @@ public class IpSecServiceParameterizedTest { anyString(), eq(TEST_SPI), anyInt(), + anyInt(), anyInt()); // quota is not released until the SPI is released by the Transform assertEquals(1, userRecord.mSpiQuotaTracker.mCurrent); @@ -452,6 +459,7 @@ public class IpSecServiceParameterizedTest { anyString(), eq(TEST_SPI), anyInt(), + anyInt(), anyInt()); // Verify quota and RefcountedResource objects cleaned up @@ -469,6 +477,7 @@ public class IpSecServiceParameterizedTest { anyString(), anyInt(), anyInt(), + anyInt(), anyInt()); assertEquals(0, userRecord.mSpiQuotaTracker.mCurrent); @@ -504,6 +513,7 @@ public class IpSecServiceParameterizedTest { anyString(), eq(TEST_SPI), anyInt(), + anyInt(), anyInt()); // Verify quota and RefcountedResource objects cleaned up @@ -572,11 +582,12 @@ public class IpSecServiceParameterizedTest { assertEquals(1, userRecord.mTunnelQuotaTracker.mCurrent); verify(mMockNetd) - .addVirtualTunnelInterface( + .ipSecAddTunnelInterface( eq(createTunnelResp.interfaceName), eq(mSourceAddr), eq(mDestinationAddr), anyInt(), + anyInt(), anyInt()); } @@ -591,7 +602,7 @@ public class IpSecServiceParameterizedTest { // Verify quota and RefcountedResource objects cleaned up assertEquals(0, userRecord.mTunnelQuotaTracker.mCurrent); - verify(mMockNetd).removeVirtualTunnelInterface(eq(createTunnelResp.interfaceName)); + verify(mMockNetd).ipSecRemoveTunnelInterface(eq(createTunnelResp.interfaceName)); try { userRecord.mTunnelInterfaceRecords.getRefcountedResourceOrThrow( createTunnelResp.resourceId); @@ -614,7 +625,7 @@ public class IpSecServiceParameterizedTest { // Verify quota and RefcountedResource objects cleaned up assertEquals(0, userRecord.mTunnelQuotaTracker.mCurrent); - verify(mMockNetd).removeVirtualTunnelInterface(eq(createTunnelResp.interfaceName)); + verify(mMockNetd).ipSecRemoveTunnelInterface(eq(createTunnelResp.interfaceName)); try { userRecord.mTunnelInterfaceRecords.getRefcountedResourceOrThrow( createTunnelResp.resourceId); @@ -623,6 +634,41 @@ public class IpSecServiceParameterizedTest { } } + @Test + public void testApplyTunnelModeTransform() throws Exception { + IpSecConfig ipSecConfig = new IpSecConfig(); + ipSecConfig.setMode(IpSecTransform.MODE_TUNNEL); + addDefaultSpisAndRemoteAddrToIpSecConfig(ipSecConfig); + addAuthAndCryptToIpSecConfig(ipSecConfig); + + IpSecTransformResponse createTransformResp = + mIpSecService.createTransform(ipSecConfig, new Binder(), "blessedPackage"); + IpSecTunnelInterfaceResponse createTunnelResp = + createAndValidateTunnel(mSourceAddr, mDestinationAddr, "blessedPackage"); + + int transformResourceId = createTransformResp.resourceId; + int tunnelResourceId = createTunnelResp.resourceId; + mIpSecService.applyTunnelModeTransform(tunnelResourceId, IpSecManager.DIRECTION_OUT, + transformResourceId, "blessedPackage"); + + for (int selAddrFamily : ADDRESS_FAMILIES) { + verify(mMockNetd) + .ipSecUpdateSecurityPolicy( + eq(mUid), + eq(selAddrFamily), + eq(IpSecManager.DIRECTION_OUT), + anyString(), + anyString(), + eq(TEST_SPI), + anyInt(), // iKey/oKey + anyInt(), // mask + eq(tunnelResourceId)); + } + + ipSecConfig.setXfrmInterfaceId(tunnelResourceId); + verifyTransformNetdCalledForCreatingSA(ipSecConfig, createTransformResp); + } + @Test public void testAddRemoveAddressFromTunnelInterface() throws Exception { for (String pkgName : new String[]{"blessedPackage", "systemPackage"}) { From b29205728283e260893f48fcef6263369f2f0949 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 71a7a2b5abd36..7ee3d3b3bdc7d 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);