From a60f62668082c812f344ac31ff5687e4e18855dc Mon Sep 17 00:00:00 2001 From: tobhe Date: Tue, 9 Feb 2021 21:35:48 +0000 Subject: [PATCH] Add optional 'group none' transform for child SAs and fix handling of 'group none'. We currently send no transform of type DH by default, which should be equivalent to explicitly sending a single DH transform of type 'none'. However, the proposal matching logic had a bug where these two would not match, effectively breaking the ability to negotiate optional PFS. This commit fixes the bug but continues to send no DH proposal by default to remain backwards compatible with older versions. ok patrick@ --- sbin/iked/ikev2.c | 30 +++++++++++++++++++++++++----- sbin/iked/parse.y | 12 +++++++++++- sbin/iked/policy.c | 5 +++-- 3 files changed, 39 insertions(+), 8 deletions(-) diff --git a/sbin/iked/ikev2.c b/sbin/iked/ikev2.c index 2ea7e7b23ac..2f37cd8edf3 100644 --- a/sbin/iked/ikev2.c +++ b/sbin/iked/ikev2.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ikev2.c,v 1.303 2021/02/04 20:38:26 tobhe Exp $ */ +/* $OpenBSD: ikev2.c,v 1.304 2021/02/09 21:35:48 tobhe Exp $ */ /* * Copyright (c) 2019 Tobias Heider @@ -2435,12 +2435,15 @@ ikev2_add_proposals(struct iked *env, struct iked_sa *sa, struct ibuf *buf, uint64_t spi64; uint32_t spi32, spi = 0; unsigned int i, xfi, nxforms; + int prop_skipdh; TAILQ_FOREACH(prop, proposals, prop_entry) { if ((protoid && prop->prop_protoid != protoid) || (!protoid && prop->prop_protoid == IKEV2_SAPROTO_IKE)) continue; + prop_skipdh = skipdh; + if (protoid != IKEV2_SAPROTO_IKE && initiator) { if (spi == 0) { bzero(&csa, sizeof(csa)); @@ -2471,12 +2474,28 @@ ikev2_add_proposals(struct iked *env, struct iked_sa *sa, struct ibuf *buf, prop->prop_localspi.spi_protoid = IKEV2_SAPROTO_IKE; } + /* + * A single DH transform of type NONE is equivalent with + * not sending a DH transform at all. + * Prefer the latter for downwards compatibility. + */ + if (protoid != IKEV2_SAPROTO_IKE) { + for (i = 0; i < prop->prop_nxforms; i++) { + xform = prop->prop_xforms + i; + if (xform->xform_type == IKEV2_XFORMTYPE_DH && + xform->xform_id != IKEV2_XFORMDH_NONE) + break; + } + if (i == prop->prop_nxforms) + prop_skipdh = 1; + } + /* * RFC 7296: 1.2. The Initial Exchanges * IKE_AUTH messages do not contain KE/N payloads, thus * SA payloads cannot contain groups. */ - if (skipdh) { + if (prop_skipdh) { nxforms = 0; for (i = 0; i < prop->prop_nxforms; i++) { xform = prop->prop_xforms + i; @@ -2514,7 +2533,7 @@ ikev2_add_proposals(struct iked *env, struct iked_sa *sa, struct ibuf *buf, for (i = 0, xfi = 0; i < prop->prop_nxforms; i++) { xform = prop->prop_xforms + i; - if (skipdh && xform->xform_type == IKEV2_XFORMTYPE_DH) + if (prop_skipdh && xform->xform_type == IKEV2_XFORMTYPE_DH) continue; if ((xflen = ikev2_add_transform(buf, @@ -3826,6 +3845,7 @@ ikev2_send_create_child_sa(struct iked *env, struct iked_sa *sa, { struct iked_policy *pol = sa->sa_policy; struct iked_childsa *csa = NULL, *csb = NULL; + struct iked_transform *xform; struct ikev2_notify *n; struct ikev2_payload *pld = NULL; struct ikev2_keyexchange *ke; @@ -3918,8 +3938,8 @@ ikev2_send_create_child_sa(struct iked *env, struct iked_sa *sa, goto done; len = ibuf_size(nonce); - if (config_findtransform(&pol->pol_proposals, IKEV2_XFORMTYPE_DH, - protoid)) { + if ((xform = config_findtransform(&pol->pol_proposals, IKEV2_XFORMTYPE_DH, + protoid)) && group_get(xform->xform_id) != IKEV2_XFORMDH_NONE) { log_debug("%s: enable PFS", __func__); ikev2_sa_cleanup_dh(sa); if (proposed_group) { diff --git a/sbin/iked/parse.y b/sbin/iked/parse.y index ff6fa16f094..e9a57c97667 100644 --- a/sbin/iked/parse.y +++ b/sbin/iked/parse.y @@ -1,4 +1,4 @@ -/* $OpenBSD: parse.y,v 1.126 2021/02/07 00:51:53 tobhe Exp $ */ +/* $OpenBSD: parse.y,v 1.127 2021/02/09 21:35:48 tobhe Exp $ */ /* * Copyright (c) 2019 Tobias Heider @@ -192,6 +192,7 @@ struct iked_transform ikev2_default_esp_transforms[] = { { IKEV2_XFORMTYPE_INTEGR, IKEV2_XFORMAUTH_HMAC_SHA2_384_192 }, { IKEV2_XFORMTYPE_INTEGR, IKEV2_XFORMAUTH_HMAC_SHA2_512_256 }, { IKEV2_XFORMTYPE_INTEGR, IKEV2_XFORMAUTH_HMAC_SHA1_96 }, + { IKEV2_XFORMTYPE_DH, IKEV2_XFORMDH_NONE }, { IKEV2_XFORMTYPE_ESN, IKEV2_XFORMESN_ESN }, { IKEV2_XFORMTYPE_ESN, IKEV2_XFORMESN_NONE }, { 0 } @@ -202,6 +203,7 @@ size_t ikev2_default_nesp_transforms = ((sizeof(ikev2_default_esp_transforms) / struct iked_transform ikev2_default_esp_transforms_noauth[] = { { IKEV2_XFORMTYPE_ENCR, IKEV2_XFORMENCR_AES_GCM_16, 128 }, { IKEV2_XFORMTYPE_ENCR, IKEV2_XFORMENCR_AES_GCM_16, 256 }, + { IKEV2_XFORMTYPE_DH, IKEV2_XFORMDH_NONE }, { IKEV2_XFORMTYPE_ESN, IKEV2_XFORMESN_ESN }, { IKEV2_XFORMTYPE_ESN, IKEV2_XFORMESN_NONE }, { 0 } @@ -267,6 +269,7 @@ const struct ipsec_xf ipsecencxfs[] = { }; const struct ipsec_xf groupxfs[] = { + { "none", IKEV2_XFORMDH_NONE }, { "modp768", IKEV2_XFORMDH_MODP_768 }, { "grp1", IKEV2_XFORMDH_MODP_768 }, { "modp1024", IKEV2_XFORMDH_MODP_1024 }, @@ -2840,6 +2843,13 @@ create_ike(char *name, int af, uint8_t ipproto, else auth++; } + for (j = 0; j < ike_sa->xfs[i]->ngroupxf; j++) { + if (ike_sa->xfs[i]->groupxf[j]->id + == IKEV2_XFORMDH_NONE) { + yyerror("IKE group can not be \"none\"."); + goto done; + } + } if (ike_sa->xfs[i]->nauthxf) auth++; diff --git a/sbin/iked/policy.c b/sbin/iked/policy.c index df7f2676dd1..d312980c392 100644 --- a/sbin/iked/policy.c +++ b/sbin/iked/policy.c @@ -1,4 +1,4 @@ -/* $OpenBSD: policy.c,v 1.75 2021/02/01 16:37:48 tobhe Exp $ */ +/* $OpenBSD: policy.c,v 1.76 2021/02/09 21:35:48 tobhe Exp $ */ /* * Copyright (c) 2010-2013 Reyk Floeter @@ -1009,7 +1009,8 @@ proposals_match(struct iked_proposal *local, struct iked_proposal *peer, */ if (rekey && requiredh == 0 && protoid == IKEV2_SAPROTO_ESP && - tlocal->xform_type == IKEV2_XFORMTYPE_DH) + tlocal->xform_type == IKEV2_XFORMTYPE_DH && + tlocal->xform_id != IKEV2_XFORMDH_NONE) requiredh = 1; /* Compare peer and local proposals */ -- 2.20.1