From 9dbc4348a97db2076e6841669525d733bbacc287 Mon Sep 17 00:00:00 2001 From: evitayan Date: Thu, 14 May 2020 22:22:20 -0700 Subject: Allow resp proposal to omit transform if transform ID NONE is proposed This commit fixes the interoperability issue in SA negotiation to allow the following two cases: - When the request proposal has a NONE value transform, response proposal is still valid if it does not have this transform - When the request proposal does not have this transform, the response is still valid if it has a NONE value of this transform. Bug: 156757683 Test: CtsIkeTestCases, FrameworksIkeTests(new tests added) Change-Id: I1cc17b92903912a03bdb27729ab0b9db093c94b7 --- src/java/android/net/ipsec/ike/SaProposal.java | 55 ++++++++++++++++++++-- .../java/android/net/ipsec/ike/SaProposalTest.java | 33 +++++++++++++ 2 files changed, 85 insertions(+), 3 deletions(-) diff --git a/src/java/android/net/ipsec/ike/SaProposal.java b/src/java/android/net/ipsec/ike/SaProposal.java index fa6fbf17..d5eb8c81 100644 --- a/src/java/android/net/ipsec/ike/SaProposal.java +++ b/src/java/android/net/ipsec/ike/SaProposal.java @@ -228,6 +228,11 @@ public abstract class SaProposal { * Check if the current SaProposal from the SA responder is consistent with the selected * reqProposal from the SA initiator. * + *

As per RFC 7296, The accepted cryptographic suite MUST contain exactly one transform of + * each type included in the proposal. But for interoperability reason, IKE library allows + * exceptions when the accepted suite or the request proposal has a NONE value transform. + * Currently only IntegrityTransform and DhGroupTransform have NONE value transform ID defined. + * * @param reqProposal selected SaProposal from SA initiator * @return if current SaProposal from SA responder is consistent with the selected reqProposal * from SA initiator. @@ -236,11 +241,16 @@ public abstract class SaProposal { public boolean isNegotiatedFrom(SaProposal reqProposal) { return this.mProtocolId == reqProposal.mProtocolId && isTransformSelectedFrom(mEncryptionAlgorithms, reqProposal.mEncryptionAlgorithms) - && isTransformSelectedFrom(mIntegrityAlgorithms, reqProposal.mIntegrityAlgorithms) - && isTransformSelectedFrom(mDhGroups, reqProposal.mDhGroups); + && isIntegrityTransformSelectedFrom( + mIntegrityAlgorithms, reqProposal.mIntegrityAlgorithms) + && isDhGroupTransformSelectedFrom(mDhGroups, reqProposal.mDhGroups); } - /** Package private */ + /** + * Check if the response transform can be selected from the request transforms + * + *

Package private + */ static boolean isTransformSelectedFrom(Transform[] selected, Transform[] selectFrom) { // If the selected proposal has multiple transforms with the same type, the responder MUST // choose a single one. @@ -253,6 +263,45 @@ public abstract class SaProposal { return Arrays.asList(selectFrom).contains(selected[0]); } + /** + * Check if the response integrity transform can be selected from the request integrity + * transforms. + * + *

For interoperability reason, it is allowed to do not include integrity transform in the + * response proposal when the request proposal has a NONE value integrity transform; and it is + * also allowed to have a NONE value integrity transform when the request proposal does not have + * integrity transforms. + */ + private static boolean isIntegrityTransformSelectedFrom( + IntegrityTransform[] selected, IntegrityTransform[] selectFrom) { + if (selected.length == 0) { + selected = new IntegrityTransform[] {new IntegrityTransform(INTEGRITY_ALGORITHM_NONE)}; + } + if (selectFrom.length == 0) { + selectFrom = + new IntegrityTransform[] {new IntegrityTransform(INTEGRITY_ALGORITHM_NONE)}; + } + return isTransformSelectedFrom(selected, selectFrom); + } + + /** + * Check if the response DH group can be selected from the request DH groups + * + *

For interoperability reason, it is allowed to do not include DH group in the response + * proposal when the request proposal has a NONE value DH group; and it is also allowed to have + * a NONE value DH group when the request proposal does not have DH groups. + */ + private static boolean isDhGroupTransformSelectedFrom( + DhGroupTransform[] selected, DhGroupTransform[] selectFrom) { + if (selected.length == 0) { + selected = new DhGroupTransform[] {new DhGroupTransform(DH_GROUP_NONE)}; + } + if (selectFrom.length == 0) { + selectFrom = new DhGroupTransform[] {new DhGroupTransform(DH_GROUP_NONE)}; + } + return isTransformSelectedFrom(selected, selectFrom); + } + /** @hide */ @IkePayload.ProtocolId public int getProtocolId() { diff --git a/tests/iketests/src/java/android/net/ipsec/ike/SaProposalTest.java b/tests/iketests/src/java/android/net/ipsec/ike/SaProposalTest.java index d4efb0c3..0c98bfa0 100644 --- a/tests/iketests/src/java/android/net/ipsec/ike/SaProposalTest.java +++ b/tests/iketests/src/java/android/net/ipsec/ike/SaProposalTest.java @@ -16,8 +16,15 @@ package android.net.ipsec.ike; +import static android.net.ipsec.ike.SaProposal.DH_GROUP_1024_BIT_MODP; +import static android.net.ipsec.ike.SaProposal.DH_GROUP_2048_BIT_MODP; +import static android.net.ipsec.ike.SaProposal.ENCRYPTION_ALGORITHM_AES_GCM_12; +import static android.net.ipsec.ike.SaProposal.ENCRYPTION_ALGORITHM_AES_GCM_8; +import static android.net.ipsec.ike.SaProposal.INTEGRITY_ALGORITHM_NONE; import static android.net.ipsec.ike.SaProposal.KEY_LEN_AES_128; import static android.net.ipsec.ike.SaProposal.KEY_LEN_UNUSED; +import static android.net.ipsec.ike.SaProposal.PSEUDORANDOM_FUNCTION_AES128_XCBC; +import static android.net.ipsec.ike.SaProposal.PSEUDORANDOM_FUNCTION_SHA2_256; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; @@ -318,4 +325,30 @@ public final class SaProposalTest { new Transform[] {mIntegrityNoneTransform}, new Transform[] {mIntegrityHmacSha1Transform})); } + + @Test + public void testIsNegotiatedFromProposalWithIntegrityNone() throws Exception { + SaProposal respProposal = + new IkeSaProposal.Builder() + .addEncryptionAlgorithm( + ENCRYPTION_ALGORITHM_AES_GCM_12, SaProposal.KEY_LEN_AES_128) + .addDhGroup(DH_GROUP_2048_BIT_MODP) + .addPseudorandomFunction(PSEUDORANDOM_FUNCTION_AES128_XCBC) + .build(); + + SaProposal reqProposal = + new IkeSaProposal.Builder() + .addEncryptionAlgorithm( + ENCRYPTION_ALGORITHM_AES_GCM_12, SaProposal.KEY_LEN_AES_128) + .addEncryptionAlgorithm( + ENCRYPTION_ALGORITHM_AES_GCM_8, SaProposal.KEY_LEN_AES_128) + .addIntegrityAlgorithm(INTEGRITY_ALGORITHM_NONE) + .addDhGroup(DH_GROUP_1024_BIT_MODP) + .addDhGroup(DH_GROUP_2048_BIT_MODP) + .addPseudorandomFunction(PSEUDORANDOM_FUNCTION_AES128_XCBC) + .addPseudorandomFunction(PSEUDORANDOM_FUNCTION_SHA2_256) + .build(); + + assertTrue(respProposal.isNegotiatedFrom(reqProposal)); + } } -- cgit v1.2.3