From c5106f3cfc98c3931809c63a4b338c307a8a511b Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Wed, 14 Mar 2018 19:01:14 -0700 Subject: [PATCH 1/5] Add documentation for TCP interactions with transforms Updates API documentation to mention that TCP sockets where transforms are deactivated will not send FIN packets. Bug: 74851550 Test: API updates only Merged-In: I8169f221c8c747538a8bddfbf02dcc73c9337189 Change-Id: I8169f221c8c747538a8bddfbf02dcc73c9337189 (cherry picked from commit 7d31a2f3579eff80c3cef07feadf77dbfcbfcd17) --- core/java/android/net/IpSecManager.java | 26 +++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/core/java/android/net/IpSecManager.java b/core/java/android/net/IpSecManager.java index 972b9c0746..a88fe0428a 100644 --- a/core/java/android/net/IpSecManager.java +++ b/core/java/android/net/IpSecManager.java @@ -305,6 +305,19 @@ public final class IpSecManager { * will throw IOException if the user deactivates the transform (by calling {@link * IpSecTransform#close()}) without calling {@link #removeTransportModeTransforms}. * + *

Note that when applied to TCP sockets, calling {@link IpSecTransform#close()} on an + * applied transform before completion of graceful shutdown may result in the shutdown sequence + * failing to complete. As such, applications requiring graceful shutdown MUST close the socket + * prior to deactivating the applied transform. Socket closure may be performed asynchronously + * (in batches), so the returning of a close function does not guarantee shutdown of a socket. + * Setting an SO_LINGER timeout results in socket closure being performed synchronously, and is + * sufficient to ensure shutdown. + * + * Specifically, if the transform is deactivated (by calling {@link IpSecTransform#close()}), + * prior to the socket being closed, the standard [FIN - FIN/ACK - ACK], or the reset [RST] + * packets are dropped due to the lack of a valid Transform. Similarly, if a socket without the + * SO_LINGER option set is closed, the delayed/batched FIN packets may be dropped. + * *

Rekey Procedure

* *

When applying a new tranform to a socket in the outbound direction, the previous transform @@ -373,6 +386,19 @@ public final class IpSecManager { * will throw IOException if the user deactivates the transform (by calling {@link * IpSecTransform#close()}) without calling {@link #removeTransportModeTransforms}. * + *

Note that when applied to TCP sockets, calling {@link IpSecTransform#close()} on an + * applied transform before completion of graceful shutdown may result in the shutdown sequence + * failing to complete. As such, applications requiring graceful shutdown MUST close the socket + * prior to deactivating the applied transform. Socket closure may be performed asynchronously + * (in batches), so the returning of a close function does not guarantee shutdown of a socket. + * Setting an SO_LINGER timeout results in socket closure being performed synchronously, and is + * sufficient to ensure shutdown. + * + * Specifically, if the transform is deactivated (by calling {@link IpSecTransform#close()}), + * prior to the socket being closed, the standard [FIN - FIN/ACK - ACK], or the reset [RST] + * packets are dropped due to the lack of a valid Transform. Similarly, if a socket without the + * SO_LINGER option set is closed, the delayed/batched FIN packets may be dropped. + * *

Rekey Procedure

* *

When applying a new tranform to a socket in the outbound direction, the previous transform From 9968ec4f144332a432de132561fd22432af3604b Mon Sep 17 00:00:00 2001 From: Remi NGUYEN VAN Date: Mon, 5 Mar 2018 18:14:56 +0900 Subject: [PATCH 2/5] Allow null subscriberId in NetworkStatsManager. Use a MATCH_MOBILE_WILDCARD template to avoid filtering by subscriberId when querying statistics from NetworkStatsService. Bug: 74038898 Change-Id: I8296220472a9ba37044dd1a5ede9bdb45d3ed339 Fixes: 74038898 Test: runtest frameworks-net, CTS tests pass Merged-In: I1e4e283c6eaecf33d12488e41e0c524f6ff83954 Merged-In: Ia84d2c7cc63bf8b8bf30f133e0382fd7103bf490 (cherry-picked from aosp I4b39e7031416cb33b23d89aa36ff0f774eaa942f) --- core/java/android/app/usage/NetworkStats.java | 6 +-- .../app/usage/NetworkStatsManager.java | 40 ++++++++++++------- 2 files changed, 27 insertions(+), 19 deletions(-) diff --git a/core/java/android/app/usage/NetworkStats.java b/core/java/android/app/usage/NetworkStats.java index da36157d85..e315e9115f 100644 --- a/core/java/android/app/usage/NetworkStats.java +++ b/core/java/android/app/usage/NetworkStats.java @@ -24,7 +24,6 @@ import android.net.NetworkStatsHistory; import android.net.NetworkTemplate; import android.net.TrafficStats; import android.os.RemoteException; -import android.os.ServiceManager; import android.util.IntArray; import android.util.Log; @@ -98,9 +97,8 @@ public final class NetworkStats implements AutoCloseable { /** @hide */ NetworkStats(Context context, NetworkTemplate template, int flags, long startTimestamp, - long endTimestamp) throws RemoteException, SecurityException { - final INetworkStatsService statsService = INetworkStatsService.Stub.asInterface( - ServiceManager.getService(Context.NETWORK_STATS_SERVICE)); + long endTimestamp, INetworkStatsService statsService) + throws RemoteException, SecurityException { // Open network stats session mSession = statsService.openSessionForUsageStats(flags, context.getOpPackageName()); mCloseGuard.open("close"); diff --git a/core/java/android/app/usage/NetworkStatsManager.java b/core/java/android/app/usage/NetworkStatsManager.java index 5576e86edd..2357637b72 100644 --- a/core/java/android/app/usage/NetworkStatsManager.java +++ b/core/java/android/app/usage/NetworkStatsManager.java @@ -37,6 +37,8 @@ import android.os.ServiceManager; import android.os.ServiceManager.ServiceNotFoundException; import android.util.Log; +import com.android.internal.annotations.VisibleForTesting; + /** * Provides access to network usage history and statistics. Usage data is collected in * discrete bins of time called 'Buckets'. See {@link NetworkStats.Bucket} for details. @@ -107,9 +109,15 @@ public class NetworkStatsManager { * {@hide} */ public NetworkStatsManager(Context context) throws ServiceNotFoundException { + this(context, INetworkStatsService.Stub.asInterface( + ServiceManager.getServiceOrThrow(Context.NETWORK_STATS_SERVICE))); + } + + /** @hide */ + @VisibleForTesting + public NetworkStatsManager(Context context, INetworkStatsService service) { mContext = context; - mService = INetworkStatsService.Stub.asInterface( - ServiceManager.getServiceOrThrow(Context.NETWORK_STATS_SERVICE)); + mService = service; setPollOnOpen(true); } @@ -135,7 +143,8 @@ public class NetworkStatsManager { public Bucket querySummaryForDevice(NetworkTemplate template, long startTime, long endTime) throws SecurityException, RemoteException { Bucket bucket = null; - NetworkStats stats = new NetworkStats(mContext, template, mFlags, startTime, endTime); + NetworkStats stats = new NetworkStats(mContext, template, mFlags, startTime, endTime, + mService); bucket = stats.getDeviceSummaryForNetwork(); stats.close(); @@ -208,7 +217,7 @@ public class NetworkStatsManager { } NetworkStats stats; - stats = new NetworkStats(mContext, template, mFlags, startTime, endTime); + stats = new NetworkStats(mContext, template, mFlags, startTime, endTime, mService); stats.startSummaryEnumeration(); stats.close(); @@ -245,7 +254,7 @@ public class NetworkStatsManager { } NetworkStats result; - result = new NetworkStats(mContext, template, mFlags, startTime, endTime); + result = new NetworkStats(mContext, template, mFlags, startTime, endTime, mService); result.startSummaryEnumeration(); return result; @@ -295,7 +304,7 @@ public class NetworkStatsManager { NetworkStats result; try { - result = new NetworkStats(mContext, template, mFlags, startTime, endTime); + result = new NetworkStats(mContext, template, mFlags, startTime, endTime, mService); result.startHistoryEnumeration(uid, tag); } catch (RemoteException e) { Log.e(TAG, "Error while querying stats for uid=" + uid + " tag=" + tag, e); @@ -341,7 +350,7 @@ public class NetworkStatsManager { } NetworkStats result; - result = new NetworkStats(mContext, template, mFlags, startTime, endTime); + result = new NetworkStats(mContext, template, mFlags, startTime, endTime, mService); result.startUserUidEnumeration(); return result; } @@ -451,19 +460,20 @@ public class NetworkStatsManager { } private static NetworkTemplate createTemplate(int networkType, String subscriberId) { - NetworkTemplate template = null; + final NetworkTemplate template; switch (networkType) { - case ConnectivityManager.TYPE_MOBILE: { - template = NetworkTemplate.buildTemplateMobileAll(subscriberId); - } break; - case ConnectivityManager.TYPE_WIFI: { + case ConnectivityManager.TYPE_MOBILE: + template = subscriberId == null + ? NetworkTemplate.buildTemplateMobileWildcard() + : NetworkTemplate.buildTemplateMobileAll(subscriberId); + break; + case ConnectivityManager.TYPE_WIFI: template = NetworkTemplate.buildTemplateWifiWildcard(); - } break; - default: { + break; + default: throw new IllegalArgumentException("Cannot create template for network type " + networkType + ", subscriberId '" + NetworkIdentity.scrubSubscriberId(subscriberId) + "'."); - } } return template; } From a386e37eab50cd2bced31b8fc121e5d6416ed24c Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Tue, 27 Mar 2018 16:55:48 -0700 Subject: [PATCH 3/5] Clarify UDP encapsulation socket API This change updates the getSocket() methods for IPsec to improve clarity of the return types, both for public APIs, and internal-only methods. Bug: 72473753 Test: APIs updated, CTS + unit tests ran. Merged-In: I0afebd432c5d04c47c93daa1ce616d712aa323d7 Change-Id: I0afebd432c5d04c47c93daa1ce616d712aa323d7 (cherry picked from commit 4c987ebade580d4abc8a3d549e0df90baab33140) --- core/java/android/net/IpSecManager.java | 6 +++--- services/core/java/com/android/server/IpSecService.java | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/core/java/android/net/IpSecManager.java b/core/java/android/net/IpSecManager.java index a88fe0428a..c7234e3165 100644 --- a/core/java/android/net/IpSecManager.java +++ b/core/java/android/net/IpSecManager.java @@ -502,7 +502,7 @@ public final class IpSecManager { * signalling and UDP encapsulated IPsec traffic. Instances can be obtained by calling {@link * IpSecManager#openUdpEncapsulationSocket}. The provided socket cannot be re-bound by the * caller. The caller should not close the {@code FileDescriptor} returned by {@link - * #getSocket}, but should use {@link #close} instead. + * #getFileDescriptor}, but should use {@link #close} instead. * *

Allowing the user to close or unbind a UDP encapsulation socket could impact the traffic * of the next user who binds to that port. To prevent this scenario, these sockets are held @@ -541,8 +541,8 @@ public final class IpSecManager { mCloseGuard.open("constructor"); } - /** Get the wrapped socket. */ - public FileDescriptor getSocket() { + /** Get the encapsulation socket's file descriptor. */ + public FileDescriptor getFileDescriptor() { if (mPfd == null) { return null; } diff --git a/services/core/java/com/android/server/IpSecService.java b/services/core/java/com/android/server/IpSecService.java index d09a161d1e..06c10564ab 100644 --- a/services/core/java/com/android/server/IpSecService.java +++ b/services/core/java/com/android/server/IpSecService.java @@ -931,7 +931,7 @@ public class IpSecService extends IIpSecService.Stub { return mPort; } - public FileDescriptor getSocket() { + public FileDescriptor getFileDescriptor() { return mSocket; } From ad2615cae598c04f04342dcab6f944ac9e5aeaf8 Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Wed, 28 Mar 2018 13:10:40 -0700 Subject: [PATCH 4/5] Require explicitly supplied truncation length Instead of providing default truncation lengths (based on RFC or otherwise), this change imposes a restriction that the truncation length must be supplied for all auth or aead algorithms. Bug: 77204048 Test: Updated tests, ran on walleye Merged-In: I4a0e2e71aa97259e56f44e7c8a2ce53135708d97 Change-Id: I4a0e2e71aa97259e56f44e7c8a2ce53135708d97 (cherry picked from commit bb7f2820f5bcccf8618078c2cbe4ea9836797e3b) --- core/java/android/net/IpSecAlgorithm.java | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/core/java/android/net/IpSecAlgorithm.java b/core/java/android/net/IpSecAlgorithm.java index 57f05884ce..8034bb62c9 100644 --- a/core/java/android/net/IpSecAlgorithm.java +++ b/core/java/android/net/IpSecAlgorithm.java @@ -56,7 +56,8 @@ public final class IpSecAlgorithm implements Parcelable { * new applications and is provided for legacy compatibility with 3gpp infrastructure. * *

Keys for this algorithm must be 128 bits in length. - *

Valid truncation lengths are multiples of 8 bits from 96 to (default) 128. + * + *

Valid truncation lengths are multiples of 8 bits from 96 to 128. */ public static final String AUTH_HMAC_MD5 = "hmac(md5)"; @@ -65,7 +66,8 @@ public final class IpSecAlgorithm implements Parcelable { * new applications and is provided for legacy compatibility with 3gpp infrastructure. * *

Keys for this algorithm must be 160 bits in length. - *

Valid truncation lengths are multiples of 8 bits from 96 to (default) 160. + * + *

Valid truncation lengths are multiples of 8 bits from 96 to 160. */ public static final String AUTH_HMAC_SHA1 = "hmac(sha1)"; @@ -73,7 +75,8 @@ public final class IpSecAlgorithm implements Parcelable { * SHA256 HMAC Authentication/Integrity Algorithm. * *

Keys for this algorithm must be 256 bits in length. - *

Valid truncation lengths are multiples of 8 bits from 96 to (default) 256. + * + *

Valid truncation lengths are multiples of 8 bits from 96 to 256. */ public static final String AUTH_HMAC_SHA256 = "hmac(sha256)"; @@ -81,7 +84,8 @@ public final class IpSecAlgorithm implements Parcelable { * SHA384 HMAC Authentication/Integrity Algorithm. * *

Keys for this algorithm must be 384 bits in length. - *

Valid truncation lengths are multiples of 8 bits from 192 to (default) 384. + * + *

Valid truncation lengths are multiples of 8 bits from 192 to 384. */ public static final String AUTH_HMAC_SHA384 = "hmac(sha384)"; @@ -89,7 +93,8 @@ public final class IpSecAlgorithm implements Parcelable { * SHA512 HMAC Authentication/Integrity Algorithm. * *

Keys for this algorithm must be 512 bits in length. - *

Valid truncation lengths are multiples of 8 bits from 256 to (default) 512. + * + *

Valid truncation lengths are multiples of 8 bits from 256 to 512. */ public static final String AUTH_HMAC_SHA512 = "hmac(sha512)"; @@ -112,6 +117,7 @@ public final class IpSecAlgorithm implements Parcelable { AUTH_HMAC_MD5, AUTH_HMAC_SHA1, AUTH_HMAC_SHA256, + AUTH_HMAC_SHA384, AUTH_HMAC_SHA512, AUTH_CRYPT_AES_GCM }) @@ -126,11 +132,14 @@ public final class IpSecAlgorithm implements Parcelable { * Creates an IpSecAlgorithm of one of the supported types. Supported algorithm names are * defined as constants in this class. * + *

For algorithms that produce an integrity check value, the truncation length is a required + * parameter. See {@link #IpSecAlgorithm(String algorithm, byte[] key, int truncLenBits)} + * * @param algorithm name of the algorithm. * @param key key padded to a multiple of 8 bits. */ public IpSecAlgorithm(@NonNull @AlgorithmName String algorithm, @NonNull byte[] key) { - this(algorithm, key, key.length * 8); + this(algorithm, key, 0); } /** @@ -228,6 +237,7 @@ public final class IpSecAlgorithm implements Parcelable { case AUTH_CRYPT_AES_GCM: // The keying material for GCM is a key plus a 32-bit salt isValidLen = keyLen == 128 + 32 || keyLen == 192 + 32 || keyLen == 256 + 32; + isValidTruncLen = truncLen == 64 || truncLen == 96 || truncLen == 128; break; default: throw new IllegalArgumentException("Couldn't find an algorithm: " + name); From 1b88f0e6ab410767079bf4763b679ec176974401 Mon Sep 17 00:00:00 2001 From: Nathan Harold Date: Wed, 28 Mar 2018 08:52:51 -0700 Subject: [PATCH 5/5] Disallow Reserved SPI Allocation Disallow the allocation of SPIs in the range reserved for future use by RFC 4303. Bug: 77205120 Test: runtest frameworks-net Merged-In: I05e26ed34b5871f1a07d5bd7b58b79a64cd74b67 Change-Id: I05e26ed34b5871f1a07d5bd7b58b79a64cd74b67 (cherry picked from commit 7f606ee8e57d9d8b7c5d0cb2a78421aa02efb385) --- core/java/android/net/IpSecManager.java | 3 ++- services/core/java/com/android/server/IpSecService.java | 5 ++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/core/java/android/net/IpSecManager.java b/core/java/android/net/IpSecManager.java index c7234e3165..1525508326 100644 --- a/core/java/android/net/IpSecManager.java +++ b/core/java/android/net/IpSecManager.java @@ -274,7 +274,8 @@ public final class IpSecManager { * * @param destinationAddress the destination address for traffic bearing the requested SPI. * For inbound traffic, the destination should be an address currently assigned on-device. - * @param requestedSpi the requested SPI, or '0' to allocate a random SPI + * @param requestedSpi the requested SPI, or '0' to allocate a random SPI. The range 1-255 is + * reserved and may not be used. See RFC 4303 Section 2.1. * @return the reserved SecurityParameterIndex * @throws {@link #ResourceUnavailableException} indicating that too many SPIs are * currently allocated for this user diff --git a/services/core/java/com/android/server/IpSecService.java b/services/core/java/com/android/server/IpSecService.java index 06c10564ab..bde6bd8db6 100644 --- a/services/core/java/com/android/server/IpSecService.java +++ b/services/core/java/com/android/server/IpSecService.java @@ -1065,7 +1065,10 @@ public class IpSecService extends IIpSecService.Stub { public synchronized IpSecSpiResponse allocateSecurityParameterIndex( String destinationAddress, int requestedSpi, IBinder binder) throws RemoteException { checkInetAddress(destinationAddress); - /* requestedSpi can be anything in the int range, so no check is needed. */ + // RFC 4303 Section 2.1 - 0=local, 1-255=reserved. + if (requestedSpi > 0 && requestedSpi < 256) { + throw new IllegalArgumentException("ESP SPI must not be in the range of 0-255."); + } checkNotNull(binder, "Null Binder passed to allocateSecurityParameterIndex"); UserRecord userRecord = mUserResourceTracker.getUserRecord(Binder.getCallingUid());