React to DELETE notifications only in INFORMATIONAL messages

and move the logic closer to the other INFORMATIONAL payloads.
Add some more sanity checks while we're at it.

ok patrick@

4 files changed, 191 insertions, 155 deletions

diff --git a/sbin/iked/iked.h b/sbin/iked/iked.h
index 4f9d44b4480..ecfd74cbb3f 100644
--- a/sbin/iked/iked.h
+++ b/sbin/iked/iked.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: iked.h,v 1.166 2020/09/23 14:25:55 tobhe Exp $	*/
+/*	$OpenBSD: iked.h,v 1.167 2020/10/03 20:23:08 tobhe Exp $	*/
 
 /*
  * Copyright (c) 2019 Tobias Heider <tobias.heider@stusta.de>
@@ -598,6 +598,10 @@ struct iked_message {
 	uint8_t			 msg_transform;
 	uint16_t		 msg_flags;
 	struct eap_msg		 msg_eap;
+	size_t			 msg_del_spisize;
+	size_t			 msg_del_cnt;
+	struct ibuf		*msg_del_buf;
+	int			 msg_del_protoid;
 
 	/* MOBIKE */
 	int			 msg_update_sa_addresses;
diff --git a/sbin/iked/ikev2.c b/sbin/iked/ikev2.c
index 005f268ba35..60311dbd30e 100644
--- a/sbin/iked/ikev2.c
+++ b/sbin/iked/ikev2.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ikev2.c,v 1.262 2020/10/02 20:02:03 tobhe Exp $	*/
+/*	$OpenBSD: ikev2.c,v 1.263 2020/10/03 20:23:08 tobhe Exp $	*/
 
 /*
  * Copyright (c) 2019 Tobias Heider <tobias.heider@stusta.de>
@@ -101,6 +101,8 @@ int	 ikev2_send_error(struct iked *, struct iked_sa *,
 int	 ikev2_send_init_error(struct iked *, struct iked_message *);
 
 int	 ikev2_handle_certreq(struct iked*, struct iked_message *);
+ssize_t	 ikev2_handle_delete(struct iked *, struct iked_message *,
+	    struct ibuf *, struct ikev2_payload **, uint8_t *);
 
 int	 ikev2_send_create_child_sa(struct iked *, struct iked_sa *,
 	    struct iked_spi *, uint8_t);
@@ -2451,6 +2453,11 @@ ikev2_resp_informational(struct iked *env, struct iked_sa *sa,
 
 	if ((buf = ibuf_static()) == NULL)
 		goto done;
+
+	if ((len = ikev2_handle_delete(env, msg, buf, &pld,
+	    &firstpayload)) == -1)
+		goto done;
+
 	/*
 	 * Include NAT_DETECTION notification on UPDATE_SA_ADDRESSES or if
 	 * the peer did include them, too (RFC 455, 3.8).
@@ -2645,6 +2652,157 @@ ikev2_resp_recv(struct iked *env, struct iked_message *msg,
 	}
 }
 
+ssize_t
+ikev2_handle_delete(struct iked *env, struct iked_message *msg,
+    struct ibuf *resp, struct ikev2_payload **pld, uint8_t *firstpayload)
+{
+	struct iked_childsa	**peersas = NULL;
+	struct iked_sa		*sa = msg->msg_sa;
+	struct ikev2_delete	*localdel;
+	struct ibuf		*spibuf = NULL;
+	uint64_t		*localspi = NULL;
+	uint64_t		 spi64, spi = 0;
+	uint32_t		 spi32;
+	uint8_t			*buf;
+	size_t			 found = 0, failed = 0;
+	int			 ret = -1;
+	size_t			 i, sz, cnt, len;
+
+	if (!msg->msg_del_protoid)
+		return (0);
+
+	sz = msg->msg_del_spisize;
+
+	switch (sz) {
+	case 4:
+	case 8:
+		break;
+	case 0:
+		if (msg->msg_del_protoid != IKEV2_SAPROTO_IKE) {
+			log_debug("%s: invalid SPI size", __func__);
+			goto done;
+		}
+		ikev2_ikesa_recv_delete(env, sa);
+		return (0);
+	default:
+		log_info("%s: error: invalid SPI size", __func__);
+		goto done;
+	}
+
+	cnt = msg->msg_del_cnt;
+	len = ibuf_length(msg->msg_del_buf);
+
+	if ((len / sz) != cnt) {
+		log_debug("%s: invalid payload length %zu/%zu != %zu",
+		    __func__, len, sz, cnt);
+		return (-1);
+	}
+
+	if (((peersas = calloc(cnt, sizeof(struct iked_childsa *))) == NULL ||
+	     (localspi = calloc(cnt, sizeof(uint64_t))) == NULL)) {
+		log_warn("%s", __func__);
+		goto done;
+	}
+
+	buf = ibuf_data(msg->msg_del_buf);
+	for (i = 0; i < cnt; i++) {
+		switch (sz) {
+		case 4:
+			memcpy(&spi32, buf + (i * sz), sizeof(spi32));
+			spi = betoh32(spi32);
+			break;
+		case 8:
+			memcpy(&spi64, buf + (i * sz), sizeof(spi64));
+			spi = betoh64(spi64);
+			break;
+		}
+
+		log_debug("%s: spi %s", __func__, print_spi(spi, sz));
+
+		if (peersas == NULL || sa == NULL)
+			continue;
+
+		if ((peersas[i] = childsa_lookup(sa, spi,
+		    msg->msg_del_protoid)) == NULL) {
+			log_warnx("%s: CHILD SA doesn't exist for spi %s",
+			    SPI_SA(sa, __func__),
+			    print_spi(spi, sz));
+			continue;
+		}
+
+		if (ikev2_childsa_delete(env, sa, msg->msg_del_protoid, spi,
+		    &localspi[i], 0) == -1)
+			failed++;
+		else {
+			found++;
+
+			/* append SPI to log buffer */
+			if (ibuf_strlen(spibuf))
+				ibuf_strcat(&spibuf, ", ");
+			ibuf_strcat(&spibuf, print_spi(spi, sz));
+		}
+
+		/*
+		 * Flows are left in the require mode so that it would be
+		 * possible to quickly negotiate a new Child SA
+		 */
+	}
+
+	if (resp == NULL) {
+		ret = 0;
+		goto done;
+	}
+
+	/* Response to the INFORMATIONAL with Delete payload */
+	if (found) {
+		if ((*pld = ikev2_add_payload(resp)) == NULL)
+			goto done;
+		*firstpayload = IKEV2_PAYLOAD_DELETE;
+
+		if ((localdel = ibuf_advance(resp, sizeof(*localdel))) == NULL)
+			goto done;
+
+		localdel->del_protoid = msg->msg_del_protoid;
+		localdel->del_spisize = sz;
+		localdel->del_nspi = htobe16(found);
+		ret = sizeof(*localdel);
+
+		for (i = 0; i < cnt; i++) {
+			if (localspi[i] == 0)	/* happens if found < cnt */
+				continue;
+			switch (sz) {
+			case 4:
+				spi32 = htobe32(localspi[i]);
+				if (ibuf_add(resp, &spi32, sizeof(spi32)) != 0)
+					goto done;
+				ret += sizeof(spi32);
+				break;
+			case 8:
+				spi64 = htobe64(localspi[i]);
+				if (ibuf_add(resp, &spi64, sizeof(spi64)) != 0)
+					goto done;
+				ret += sizeof(spi64);
+				break;
+			}
+		}
+		log_info("%sdeleted %zu SPI%s: %.*s",
+		    SPI_SA(sa, NULL), found,
+		    found == 1 ? "" : "s",
+		    spibuf ? ibuf_strlen(spibuf) : 0,
+		    spibuf ? (char *)ibuf_data(spibuf) : "");
+	} else {
+		/* XXX should we send an INVALID_SPI notification? */
+		ret = 0;
+	}
+
+ done:
+	free(localspi);
+	free(peersas);
+	ibuf_release(spibuf);
+
+	return (ret);
+}
+
 int
 ikev2_handle_notifies(struct iked *env, struct iked_message *msg)
 {
diff --git a/sbin/iked/ikev2_msg.c b/sbin/iked/ikev2_msg.c
index aa957e4f431..00a8220bc28 100644
--- a/sbin/iked/ikev2_msg.c
+++ b/sbin/iked/ikev2_msg.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ikev2_msg.c,v 1.72 2020/09/26 16:20:36 tobhe Exp $	*/
+/*	$OpenBSD: ikev2_msg.c,v 1.73 2020/10/03 20:23:08 tobhe Exp $	*/
 
 /*
  * Copyright (c) 2019 Tobias Heider <tobias.heider@stusta.de>
@@ -195,6 +195,7 @@ ikev2_msg_cleanup(struct iked *env, struct iked_message *msg)
 		ibuf_release(msg->msg_cert.id_buf);
 		ibuf_release(msg->msg_cookie);
 		ibuf_release(msg->msg_cookie2);
+		ibuf_release(msg->msg_del_buf);
 		free(msg->msg_eap.eam_user);
 
 		msg->msg_nonce = NULL;
@@ -204,6 +205,7 @@ ikev2_msg_cleanup(struct iked *env, struct iked_message *msg)
 		msg->msg_cert.id_buf = NULL;
 		msg->msg_cookie = NULL;
 		msg->msg_cookie2 = NULL;
+		msg->msg_del_buf = NULL;
 		msg->msg_eap.eam_user = NULL;
 
 		config_free_proposals(&msg->msg_proposals, 0);
diff --git a/sbin/iked/ikev2_pld.c b/sbin/iked/ikev2_pld.c
index 6c1113c9d1a..951a5397e7e 100644
--- a/sbin/iked/ikev2_pld.c
+++ b/sbin/iked/ikev2_pld.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ikev2_pld.c,v 1.100 2020/10/01 18:38:49 tobhe Exp $	*/
+/*	$OpenBSD: ikev2_pld.c,v 1.101 2020/10/03 20:23:08 tobhe Exp $	*/
 
 /*
  * Copyright (c) 2019 Tobias Heider <tobias.heider@stusta.de>
@@ -1337,6 +1337,11 @@ ikev2_validate_delete(struct iked_message *msg, size_t offset, size_t left,
 	}
 	memcpy(del, msgbuf + offset, sizeof(*del));
 
+	if (del->del_protoid == 0) {
+		log_info("%s: malformed payload: invalid protoid", __func__);
+		return (-1);
+	}
+
 	return (0);
 }
 
@@ -1344,17 +1349,9 @@ int
 ikev2_pld_delete(struct iked *env, struct ikev2_payload *pld,
     struct iked_message *msg, size_t offset, size_t left)
 {
-	struct iked_childsa	**peersas = NULL;
-	struct iked_sa		*sa = msg->msg_sa;
-	struct ikev2_delete	 del, *localdel;
-	struct ibuf		*resp = NULL;
-	struct ibuf		*spibuf = NULL;
-	uint64_t		*localspi = NULL;
-	uint64_t		 spi64, spi = 0;
-	uint32_t		 spi32;
+	struct ikev2_delete	 del;
 	uint8_t			*buf, *msgbuf = ibuf_data(msg->msg_data);
-	size_t			 found = 0, failed = 0;
-	int			 cnt, i, len, sz, ret = -1;
+	size_t			 cnt, sz, len;
 
 	if (ikev2_validate_delete(msg, offset, left, &del))
 		return (-1);
@@ -1367,160 +1364,35 @@ ikev2_pld_delete(struct iked *env, struct ikev2_payload *pld,
 	cnt = betoh16(del.del_nspi);
 	sz = del.del_spisize;
 
-	log_debug("%s: proto %s spisize %d nspi %d",
+	log_debug("%s: proto %s spisize %zu nspi %zu",
 	    __func__, print_map(del.del_protoid, ikev2_saproto_map),
 	    sz, cnt);
 
-	buf = msgbuf + offset + sizeof(del);
-	len = left - sizeof(del);
+	if (msg->msg_parent->msg_del_protoid) {
+		log_debug("%s: duplicate delete payload", __func__);
+		return (0);
+	}
 
-	print_hex(buf, 0, len);
+	msg->msg_parent->msg_del_protoid = del.del_protoid;
+	msg->msg_parent->msg_del_cnt = cnt;
+	msg->msg_parent->msg_del_spisize = sz;
 
-	switch (sz) {
-	case 4:
-	case 8:
-		break;
-	default:
-		if (del.del_protoid != IKEV2_SAPROTO_IKE) {
-			log_debug("%s: invalid SPI size", __func__);
-			return (-1);
-		}
-		if (ikev2_msg_frompeer(msg)) {
-			/* Send an empty informational response */
-			if ((resp = ibuf_static()) == NULL)
-				goto done;
-			ret = ikev2_send_ike_e(env, sa, resp,
-			    IKEV2_PAYLOAD_NONE,
-			    IKEV2_EXCHANGE_INFORMATIONAL, 1);
-			msg->msg_parent->msg_responded = 1;
-			ibuf_release(resp);
-			ikev2_ikesa_recv_delete(env, sa);
-		} else {
-			/*
-			 * We're sending a delete message. Upper layer
-			 * must deal with deletion of the IKE SA.
-			 */
-			ret = 0;
-		}
-		return (ret);
-	}
+	buf = msgbuf + offset + sizeof(del);
+	len = left - sizeof(del);
+	if (len == 0 || sz == 0 || cnt == 0)
+		return (0);
 
 	if ((len / sz) != cnt) {
-		log_debug("%s: invalid payload length %d/%d != %d",
+		log_debug("%s: invalid payload length %zu/%zu != %zu",
 		    __func__, len, sz, cnt);
 		return (-1);
 	}
 
-	if (ikev2_msg_frompeer(msg) &&
-	    ((peersas = calloc(cnt, sizeof(struct iked_childsa *))) == NULL ||
-	     (localspi = calloc(cnt, sizeof(uint64_t))) == NULL)) {
-		log_warn("%s", __func__);
-		goto done;
-	}
-
-	for (i = 0; i < cnt; i++) {
-		switch (sz) {
-		case 4:
-			memcpy(&spi32, buf + (i * sz), sizeof(spi32));
-			spi = betoh32(spi32);
-			break;
-		case 8:
-			memcpy(&spi64, buf + (i * sz), sizeof(spi64));
-			spi = betoh64(spi64);
-			break;
-		}
-
-		log_debug("%s: spi %s", __func__, print_spi(spi, sz));
-
-		if (peersas == NULL || sa == NULL)
-			continue;
-
-		if ((peersas[i] = childsa_lookup(sa, spi,
-		    del.del_protoid)) == NULL) {
-			log_warnx("%s: CHILD SA doesn't exist for spi %s",
-			    SPI_SA(sa, __func__),
-			    print_spi(spi, del.del_spisize));
-			continue;
-		}
-
-		if (ikev2_childsa_delete(env, sa, del.del_protoid, spi,
-		    &localspi[i], 0) == -1)
-			failed++;
-		else {
-			found++;
-
-			/* append SPI to log buffer */
-			if (ibuf_strlen(spibuf))
-				ibuf_strcat(&spibuf, ", ");
-			ibuf_strcat(&spibuf, print_spi(spi, sz));
-		}
-
-		/*
-		 * Flows are left in the require mode so that it would be
-		 * possible to quickly negotiate a new Child SA
-		 */
-	}
-
-	/* Parsed outgoing message? */
-	if (!ikev2_msg_frompeer(msg))
-		goto done;
-
-	if (msg->msg_parent->msg_response) {
-		ret = 0;
-		goto done;
-	}
-
-	/* Response to the INFORMATIONAL with Delete payload */
-
-	if ((resp = ibuf_static()) == NULL)
-		goto done;
-
-	if (found) {
-		if ((localdel = ibuf_advance(resp, sizeof(*localdel))) == NULL)
-			goto done;
-
-		localdel->del_protoid = del.del_protoid;
-		localdel->del_spisize = del.del_spisize;
-		localdel->del_nspi = htobe16(found);
-
-		for (i = 0; i < cnt; i++) {
-			if (localspi[i] == 0)	/* happens if found < cnt */
-				continue;
-			switch (sz) {
-			case 4:
-				spi32 = htobe32(localspi[i]);
-				if (ibuf_add(resp, &spi32, sizeof(spi32)) != 0)
-					goto done;
-				break;
-			case 8:
-				spi64 = htobe64(localspi[i]);
-				if (ibuf_add(resp, &spi64, sizeof(spi64)) != 0)
-					goto done;
-				break;
-			}
-		}
-		log_debug("%sdeleted %zu SPI%s: %.*s",
-		    SPI_SA(sa, NULL), found,
-		    found == 1 ? "" : "s",
-		    spibuf ? ibuf_strlen(spibuf) : 0,
-		    spibuf ? (char *)ibuf_data(spibuf) : "");
-	}
+	print_hex(buf, 0, len);
 
-	if (found) {
-		ret = ikev2_send_ike_e(env, sa, resp, IKEV2_PAYLOAD_DELETE,
-		    IKEV2_EXCHANGE_INFORMATIONAL, 1);
-		msg->msg_parent->msg_responded = 1;
-	} else {
-		/* XXX should we send an INVALID_SPI notification? */
-		ret = 0;
-	}
+	msg->msg_parent->msg_del_buf = ibuf_new(buf, len);
 
- done:
-	free(localspi);
-	free(peersas);
-	ibuf_release(spibuf);
-	ibuf_release(resp);
-	return (ret);
+	return (0);
 }
 
 int