Don't cache the DH group in the policy

When tearing IKE SA down, the DH group referred by it is destroyed,
however it remains cached in the policy.  With the introduction of
IKE SA rekeying we have extended the life of this dangling pointer
by reusing it on new SAs.  So instead of caching the pointer in the
policy we can store the DH group ID and create a DH group on demand
using this parameter if it's specified.

With and OK reyk

5 files changed, 40 insertions, 25 deletions

diff --git a/sbin/iked/dh.c b/sbin/iked/dh.c
index b9268fb2ddb..29cba9addf0 100644
--- a/sbin/iked/dh.c
+++ b/sbin/iked/dh.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: dh.c,v 1.18 2017/01/04 12:31:01 mikeb Exp $	*/
+/*	$OpenBSD: dh.c,v 1.19 2017/03/27 17:17:49 mikeb Exp $	*/
 
 /*
  * Copyright (c) 2010-2014 Reyk Floeter <reyk@openbsd.org>
@@ -63,7 +63,7 @@ extern int crypto_scalarmult_curve25519(unsigned char a[CURVE25519_SIZE],
 	__attribute__((__bounded__(__minbytes__, 2, CURVE25519_SIZE)))
 	__attribute__((__bounded__(__minbytes__, 3, CURVE25519_SIZE)));
 
-struct group_id ike_groups[] = {
+const struct group_id ike_groups[] = {
 	{ GROUP_MODP, 1, 768,
 	    "FFFFFFFFFFFFFFFFC90FDAA22168C234C4C6628B80DC1CD1"
 	    "29024E088A67CC74020BBEA63B139B22514A08798E3404DD"
@@ -274,18 +274,10 @@ group_free(struct group *group)
 struct group *
 group_get(uint32_t id)
 {
-	struct group_id	*p = NULL;
-	struct group	*group;
-	unsigned int	 i, items;
+	const struct group_id	*p;
+	struct group		*group;
 
-	items = sizeof(ike_groups) / sizeof(ike_groups[0]);
-	for (i = 0; i < items; i++) {
-		if (id == ike_groups[i].id) {
-			p = &ike_groups[i];
-			break;
-		}
-	}
-	if (p == NULL)
+	if ((p = group_getid(id)) == NULL)
 		return (NULL);
 
 	if ((group = calloc(1, sizeof(*group))) == NULL)
@@ -327,6 +319,22 @@ group_get(uint32_t id)
 	return (group);
 }
 
+const struct group_id *
+group_getid(uint32_t id)
+{
+	const struct group_id	*p = NULL;
+	unsigned int		 i, items;
+
+	items = sizeof(ike_groups) / sizeof(ike_groups[0]);
+	for (i = 0; i < items; i++) {
+		if (id == ike_groups[i].id) {
+			p = &ike_groups[i];
+			break;
+		}
+	}
+	return (p);
+}
+
 int
 dh_init(struct group *group)
 {
diff --git a/sbin/iked/dh.h b/sbin/iked/dh.h
index e5b825cb64e..77bb4b5ef16 100644
--- a/sbin/iked/dh.h
+++ b/sbin/iked/dh.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: dh.h,v 1.9 2015/08/21 11:59:27 reyk Exp $	*/
+/*	$OpenBSD: dh.h,v 1.10 2017/03/27 17:17:49 mikeb Exp $	*/
 
 /*
  * Copyright (c) 2010-2013 Reyk Floeter <reyk@openbsd.org>
@@ -37,7 +37,8 @@ struct group_id {
 
 struct group {
 	int		 id;
-	struct group_id	*spec;
+	const struct group_id
+			*spec;
 
 	void		*dh;
 	void		*ec;
@@ -54,6 +55,8 @@ struct group {
 void		 group_init(void);
 void		 group_free(struct group *);
 struct group	*group_get(uint32_t);
+const struct group_id
+		*group_getid(uint32_t);
 
 int		 dh_getlen(struct group *);
 int		 dh_create_exchange(struct group *, uint8_t *);
diff --git a/sbin/iked/iked.h b/sbin/iked/iked.h
index ed4d0f257be..25e81ea34a8 100644
--- a/sbin/iked/iked.h
+++ b/sbin/iked/iked.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: iked.h,v 1.112 2017/03/27 10:43:53 mikeb Exp $	*/
+/*	$OpenBSD: iked.h,v 1.113 2017/03/27 17:17:49 mikeb Exp $	*/
 
 /*
  * Copyright (c) 2010-2013 Reyk Floeter <reyk@openbsd.org>
@@ -258,8 +258,8 @@ struct iked_policy {
 	unsigned int			 pol_ipproto;
 
 	struct iked_addr		 pol_peer;
-	struct group			*pol_peerdh;
 	struct iked_static_id		 pol_peerid;
+	uint32_t			 pol_peerdh;
 
 	struct iked_addr		 pol_local;
 	struct iked_static_id		 pol_localid;
diff --git a/sbin/iked/ikev2.c b/sbin/iked/ikev2.c
index d8c15be8cc2..631fbd3a645 100644
--- a/sbin/iked/ikev2.c
+++ b/sbin/iked/ikev2.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ikev2.c,v 1.148 2017/03/27 10:43:53 mikeb Exp $	*/
+/*	$OpenBSD: ikev2.c,v 1.149 2017/03/27 17:17:49 mikeb Exp $	*/
 
 /*
  * Copyright (c) 2010-2013 Reyk Floeter <reyk@openbsd.org>
@@ -900,8 +900,12 @@ ikev2_init_ike_sa_peer(struct iked *env, struct iked_policy *pol,
 		return (-1);
 
 	/* Pick peer's DH group if asked */
-	/* XXX free old sa_dhgroup ? */
-	sa->sa_dhgroup = pol->pol_peerdh;
+	if (pol->pol_peerdh > 0 && sa->sa_dhgroup == NULL &&
+	    (sa->sa_dhgroup = group_get(pol->pol_peerdh)) == NULL) {
+		log_warnx("%s: invalid peer DH group %u", __func__,
+		    pol->pol_peerdh);
+		return (-1);
+	}
 	sa->sa_reqid = 0;
 
 	if (ikev2_sa_initiator(env, sa, NULL, NULL) == -1)
diff --git a/sbin/iked/ikev2_pld.c b/sbin/iked/ikev2_pld.c
index 9aeb61eb987..86086e56ca4 100644
--- a/sbin/iked/ikev2_pld.c
+++ b/sbin/iked/ikev2_pld.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ikev2_pld.c,v 1.60 2017/03/27 10:21:19 reyk Exp $	*/
+/*	$OpenBSD: ikev2_pld.c,v 1.61 2017/03/27 17:17:49 mikeb Exp $	*/
 
 /*
  * Copyright (c) 2010-2013 Reyk Floeter <reyk@openbsd.org>
@@ -1202,13 +1202,13 @@ ikev2_pld_notify(struct iked *env, struct ikev2_payload *pld,
 		}
 		memcpy(&group, buf, len);
 		group = betoh16(group);
-		if ((msg->msg_policy->pol_peerdh = group_get(group))
-		    == NULL) {
-			log_debug("%s: unable to select DH group %d", __func__,
+		if (group_getid(group) == NULL) {
+			log_debug("%s: unable to select DH group %u", __func__,
 			    group);
 			return (-1);
 		}
-		log_debug("%s: responder selected DH group %d", __func__,
+		msg->msg_policy->pol_peerdh = group;
+		log_debug("%s: responder selected DH group %u", __func__,
 		    group);
 		sa_state(env, msg->msg_sa, IKEV2_STATE_CLOSED);
 		msg->msg_sa = NULL;