From 6e264ad04996a3121af527d964c0bf6c575af7e9 Mon Sep 17 00:00:00 2001 From: tobhe Date: Mon, 14 Mar 2022 12:58:55 +0000 Subject: [PATCH] Improve retransmission of message fragments. RFC 7383 states that loss of a single fragment results in a retransmit of all fragments belonging to the same message. Instead of treating each fragment as message with seperate retransmit timer, keep only a single timer for all fragments of a message and retransmit all fragments in order on timeout. Improves reliability in case of packet loss when fragmentation is enabled. Found by and diff from Daniel Herzinger ok patrick@ --- sbin/iked/iked.h | 30 +++--- sbin/iked/ikev2.c | 48 ++++++---- sbin/iked/ikev2_msg.c | 211 +++++++++++++++++++++++------------------- sbin/iked/ikev2_pld.c | 8 +- 4 files changed, 165 insertions(+), 132 deletions(-) diff --git a/sbin/iked/iked.h b/sbin/iked/iked.h index 527ab171085..ff6e66cd6ef 100644 --- a/sbin/iked/iked.h +++ b/sbin/iked/iked.h @@ -1,4 +1,4 @@ -/* $OpenBSD: iked.h,v 1.203 2021/12/09 13:49:45 tobhe Exp $ */ +/* $OpenBSD: iked.h,v 1.204 2022/03/14 12:58:55 tobhe Exp $ */ /* * Copyright (c) 2019 Tobias Heider @@ -364,7 +364,8 @@ struct iked_id { "\20\01CERT\02CERTVALID\03CERTREQ\04AUTH\05AUTHVALID\06SA\07EAPVALID" \ "\10CHILDSA\11INF" -TAILQ_HEAD(iked_msgqueue, iked_message); +TAILQ_HEAD(iked_msgqueue, iked_msg_retransmit); +TAILQ_HEAD(iked_msg_fragqueue, iked_message); struct iked_sahdr { uint64_t sh_ispi; /* Initiator SPI */ @@ -628,10 +629,15 @@ struct iked_message { uint16_t msg_attrlength; /* Retransmit queue */ - struct iked_timer msg_timer; TAILQ_ENTRY(iked_message) msg_entry; - int msg_tries; /* retransmits sent */ +}; + +struct iked_msg_retransmit { + struct iked_msg_fragqueue mrt_frags; + TAILQ_ENTRY(iked_msg_retransmit) mrt_entry; + struct iked_timer mrt_timer; + int mrt_tries; #define IKED_RETRANSMIT_TRIES 5 /* try 5 times */ }; @@ -1071,22 +1077,18 @@ int ikev2_msg_integr(struct iked *, struct iked_sa *, struct ibuf *); int ikev2_msg_frompeer(struct iked_message *); struct iked_socket * ikev2_msg_getsocket(struct iked *, int, int); +int ikev2_msg_enqueue(struct iked *, struct iked_msgqueue *, + struct iked_message *, int); int ikev2_msg_retransmit_response(struct iked *, struct iked_sa *, - struct iked_message *); + struct iked_message *, uint8_t); void ikev2_msg_prevail(struct iked *, struct iked_msgqueue *, struct iked_message *); void ikev2_msg_dispose(struct iked *, struct iked_msgqueue *, - struct iked_message *); + struct iked_msg_retransmit *); void ikev2_msg_flushqueue(struct iked *, struct iked_msgqueue *); -struct iked_message * +struct iked_msg_retransmit * ikev2_msg_lookup(struct iked *, struct iked_msgqueue *, - struct iked_message *, struct ike_header *); -void ikev2_msg_lookup_dispose_all(struct iked *env, - struct iked_msgqueue *queue, struct iked_message *msg, - struct ike_header *hdr); -int ikev2_msg_lookup_retransmit_all(struct iked *env, - struct iked_msgqueue *queue, struct iked_message *msg, - struct ike_header *hdr, struct iked_sa *sa); + struct iked_message *, uint8_t); /* ikev2_pld.c */ int ikev2_pld_parse(struct iked *, struct ike_header *, diff --git a/sbin/iked/ikev2.c b/sbin/iked/ikev2.c index b646c7f3fed..3107b0652e3 100644 --- a/sbin/iked/ikev2.c +++ b/sbin/iked/ikev2.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ikev2.c,v 1.345 2022/02/13 12:26:54 mbuhl Exp $ */ +/* $OpenBSD: ikev2.c,v 1.346 2022/03/14 12:58:55 tobhe Exp $ */ /* * Copyright (c) 2019 Tobias Heider @@ -583,6 +583,7 @@ ikev2_recv(struct iked *env, struct iked_message *msg) { struct ike_header *hdr; struct iked_sa *sa; + struct iked_msg_retransmit *mr; unsigned int initiator, flag = 0; int r; @@ -635,9 +636,10 @@ ikev2_recv(struct iked *env, struct iked_message *msg) if (msg->msg_response) { if (msg->msg_msgid > sa->sa_reqid) return; + mr = ikev2_msg_lookup(env, &sa->sa_requests, msg, + hdr->ike_exchange); if (hdr->ike_exchange != IKEV2_EXCHANGE_INFORMATIONAL && - !ikev2_msg_lookup(env, &sa->sa_requests, msg, hdr) && - sa->sa_fragments.frag_count == 0) + mr == NULL && sa->sa_fragments.frag_count == 0) return; if (flag) { if ((sa->sa_stateflags & flag) == 0) @@ -651,7 +653,8 @@ ikev2_recv(struct iked *env, struct iked_message *msg) /* * There's no need to keep the request (fragments) around */ - ikev2_msg_lookup_dispose_all(env, &sa->sa_requests, msg, hdr); + if (mr != NULL && hdr->ike_nextpayload != IKEV2_PAYLOAD_SKF) + ikev2_msg_dispose(env, &sa->sa_requests, mr); } else { /* * IKE_SA_INIT is special since it always uses the message id 0. @@ -678,8 +681,8 @@ ikev2_recv(struct iked *env, struct iked_message *msg) /* * See if we have responded to this request before */ - if ((r = ikev2_msg_lookup_retransmit_all(env, &sa->sa_responses, - msg, hdr, sa)) != 0) { + if ((r = ikev2_msg_retransmit_response(env, sa, msg, + hdr->ike_exchange)) != 0) { if (r == -1) { log_warn("%s: failed to retransmit a " "response", __func__); @@ -7176,9 +7179,10 @@ ikev2_cp_fixflow(struct iked_sa *sa, struct iked_flow *flow, int ikev2_update_sa_addresses(struct iked *env, struct iked_sa *sa) { - struct iked_childsa *csa, *ipcomp; - struct iked_flow *flow, *oflow; - struct iked_message *msg; + struct iked_childsa *csa, *ipcomp; + struct iked_flow *flow, *oflow; + struct iked_message *msg; + struct iked_msg_retransmit *mr; if (!sa_stateok(sa, IKEV2_STATE_ESTABLISHED)) return -1; @@ -7221,17 +7225,21 @@ ikev2_update_sa_addresses(struct iked *env, struct iked_sa *sa) } /* update pending requests and responses */ - TAILQ_FOREACH(msg, &sa->sa_requests, msg_entry) { - msg->msg_local = sa->sa_local.addr; - msg->msg_locallen = sa->sa_local.addr.ss_len; - msg->msg_peer = sa->sa_peer.addr; - msg->msg_peerlen = sa->sa_peer.addr.ss_len; - } - TAILQ_FOREACH(msg, &sa->sa_responses, msg_entry) { - msg->msg_local = sa->sa_local.addr; - msg->msg_locallen = sa->sa_local.addr.ss_len; - msg->msg_peer = sa->sa_peer.addr; - msg->msg_peerlen = sa->sa_peer.addr.ss_len; + TAILQ_FOREACH(mr, &sa->sa_requests, mrt_entry) { + TAILQ_FOREACH(msg, &mr->mrt_frags, msg_entry) { + msg->msg_local = sa->sa_local.addr; + msg->msg_locallen = sa->sa_local.addr.ss_len; + msg->msg_peer = sa->sa_peer.addr; + msg->msg_peerlen = sa->sa_peer.addr.ss_len; + } + } + TAILQ_FOREACH(mr, &sa->sa_responses, mrt_entry) { + TAILQ_FOREACH(msg, &mr->mrt_frags, msg_entry) { + msg->msg_local = sa->sa_local.addr; + msg->msg_locallen = sa->sa_local.addr.ss_len; + msg->msg_peer = sa->sa_peer.addr; + msg->msg_peerlen = sa->sa_peer.addr.ss_len; + } } /* Update sa_peer_loaded, to match in-kernel information */ diff --git a/sbin/iked/ikev2_msg.c b/sbin/iked/ikev2_msg.c index 0f47eb41cfa..feab4aa7730 100644 --- a/sbin/iked/ikev2_msg.c +++ b/sbin/iked/ikev2_msg.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ikev2_msg.c,v 1.84 2021/12/01 16:42:13 deraadt Exp $ */ +/* $OpenBSD: ikev2_msg.c,v 1.85 2022/03/14 12:58:55 tobhe Exp $ */ /* * Copyright (c) 2019 Tobias Heider @@ -319,13 +319,19 @@ ikev2_msg_send(struct iked *env, struct iked_message *msg) m->msg_exchange = exchange; if (flags & IKEV2_FLAG_RESPONSE) { - TAILQ_INSERT_TAIL(&sa->sa_responses, m, msg_entry); - timer_set(env, &m->msg_timer, ikev2_msg_response_timeout, m); - timer_add(env, &m->msg_timer, IKED_RESPONSE_TIMEOUT); + if (ikev2_msg_enqueue(env, &sa->sa_responses, m, + IKED_RESPONSE_TIMEOUT) != 0) { + ikev2_msg_cleanup(env, m); + free(m); + return (-1); + } } else { - TAILQ_INSERT_TAIL(&sa->sa_requests, m, msg_entry); - timer_set(env, &m->msg_timer, ikev2_msg_retransmit_timeout, m); - timer_add(env, &m->msg_timer, IKED_RETRANSMIT_TIMEOUT); + if (ikev2_msg_enqueue(env, &sa->sa_requests, m, + IKED_RETRANSMIT_TIMEOUT) != 0) { + ikev2_msg_cleanup(env, m); + free(m); + return (-1); + } } return (0); @@ -1165,145 +1171,156 @@ ikev2_msg_getsocket(struct iked *env, int af, int natt) return (NULL); } +int +ikev2_msg_enqueue(struct iked *env, struct iked_msgqueue *queue, + struct iked_message *msg, int timeout) +{ + struct iked_msg_retransmit *mr; + + if ((mr = ikev2_msg_lookup(env, queue, msg, msg->msg_exchange)) == + NULL) { + if ((mr = calloc(1, sizeof(*mr))) == NULL) + return (-1); + TAILQ_INIT(&mr->mrt_frags); + mr->mrt_tries = 0; + + timer_set(env, &mr->mrt_timer, msg->msg_response ? + ikev2_msg_response_timeout : ikev2_msg_retransmit_timeout, + mr); + timer_add(env, &mr->mrt_timer, timeout); + + TAILQ_INSERT_TAIL(queue, mr, mrt_entry); + } + + TAILQ_INSERT_TAIL(&mr->mrt_frags, msg, msg_entry); + + return 0; +} + void ikev2_msg_prevail(struct iked *env, struct iked_msgqueue *queue, struct iked_message *msg) { - struct iked_message *m, *mtmp; + struct iked_msg_retransmit *mr, *mrtmp; - TAILQ_FOREACH_SAFE(m, queue, msg_entry, mtmp) { - if (m->msg_msgid < msg->msg_msgid) - ikev2_msg_dispose(env, queue, m); + TAILQ_FOREACH_SAFE(mr, queue, mrt_entry, mrtmp) { + if (TAILQ_FIRST(&mr->mrt_frags)->msg_msgid < msg->msg_msgid) + ikev2_msg_dispose(env, queue, mr); } } void ikev2_msg_dispose(struct iked *env, struct iked_msgqueue *queue, - struct iked_message *msg) + struct iked_msg_retransmit *mr) { - TAILQ_REMOVE(queue, msg, msg_entry); - timer_del(env, &msg->msg_timer); - ikev2_msg_cleanup(env, msg); - free(msg); + struct iked_message *m; + + while ((m = TAILQ_FIRST(&mr->mrt_frags)) != NULL) { + TAILQ_REMOVE(&mr->mrt_frags, m, msg_entry); + ikev2_msg_cleanup(env, m); + free(m); + } + + timer_del(env, &mr->mrt_timer); + TAILQ_REMOVE(queue, mr, mrt_entry); + free(mr); } void ikev2_msg_flushqueue(struct iked *env, struct iked_msgqueue *queue) { - struct iked_message *m = NULL; + struct iked_msg_retransmit *mr = NULL; - while ((m = TAILQ_FIRST(queue)) != NULL) - ikev2_msg_dispose(env, queue, m); + while ((mr = TAILQ_FIRST(queue)) != NULL) + ikev2_msg_dispose(env, queue, mr); } -struct iked_message * +struct iked_msg_retransmit * ikev2_msg_lookup(struct iked *env, struct iked_msgqueue *queue, - struct iked_message *msg, struct ike_header *hdr) + struct iked_message *msg, uint8_t exchange) { - struct iked_message *m = NULL; + struct iked_msg_retransmit *mr = NULL; - TAILQ_FOREACH(m, queue, msg_entry) { - if (m->msg_msgid == msg->msg_msgid && - m->msg_exchange == hdr->ike_exchange) + TAILQ_FOREACH(mr, queue, mrt_entry) { + if (TAILQ_FIRST(&mr->mrt_frags)->msg_msgid == + msg->msg_msgid && + TAILQ_FIRST(&mr->mrt_frags)->msg_exchange == exchange) break; } - return (m); + return (mr); } -void -ikev2_msg_lookup_dispose_all(struct iked *env, struct iked_msgqueue *queue, - struct iked_message *msg, struct ike_header *hdr) +int +ikev2_msg_retransmit_response(struct iked *env, struct iked_sa *sa, + struct iked_message *msg, uint8_t exchange) { - struct iked_message *m = NULL, *tmp = NULL; + struct iked_msg_retransmit *mr = NULL; + struct iked_message *m = NULL; - TAILQ_FOREACH_SAFE(m, queue, msg_entry, tmp) { - if (m->msg_msgid == msg->msg_msgid && - m->msg_exchange == hdr->ike_exchange) { - TAILQ_REMOVE(queue, m, msg_entry); - timer_del(env, &m->msg_timer); - ikev2_msg_cleanup(env, m); - free(m); - } - } -} + if ((mr = ikev2_msg_lookup(env, &sa->sa_responses, msg, exchange)) + == NULL) + return (0); -int -ikev2_msg_lookup_retransmit_all(struct iked *env, struct iked_msgqueue *queue, - struct iked_message *msg, struct ike_header *hdr, struct iked_sa *sa) -{ - struct iked_message *m = NULL, *tmp = NULL; - int count = 0; - - TAILQ_FOREACH_SAFE(m, queue, msg_entry, tmp) { - if (m->msg_msgid == msg->msg_msgid && - m->msg_exchange == hdr->ike_exchange) { - if (ikev2_msg_retransmit_response(env, sa, m)) - return -1; - count++; + TAILQ_FOREACH(m, &mr->mrt_frags, msg_entry) { + if (sendtofrom(m->msg_fd, ibuf_data(m->msg_data), + ibuf_size(m->msg_data), 0, + (struct sockaddr *)&m->msg_peer, m->msg_peerlen, + (struct sockaddr *)&m->msg_local, m->msg_locallen) == -1) { + log_warn("%s: sendtofrom", __func__); + return (-1); } + log_info("%sretransmit %s res %u local %s peer %s", + SPI_SA(sa, NULL), + print_map(exchange, ikev2_exchange_map), + m->msg_msgid, + print_host((struct sockaddr *)&m->msg_local, NULL, 0), + print_host((struct sockaddr *)&m->msg_peer, NULL, 0)); } - return count; -} - -int -ikev2_msg_retransmit_response(struct iked *env, struct iked_sa *sa, - struct iked_message *msg) -{ - if (sendtofrom(msg->msg_fd, ibuf_data(msg->msg_data), - ibuf_size(msg->msg_data), 0, - (struct sockaddr *)&msg->msg_peer, msg->msg_peerlen, - (struct sockaddr *)&msg->msg_local, msg->msg_locallen) == -1) { - log_warn("%s: sendtofrom", __func__); - return (-1); - } - log_info("%sretransmit %s res %u local %s peer %s", - SPI_SA(sa, NULL), - print_map(msg->msg_exchange, ikev2_exchange_map), - msg->msg_msgid, - print_host((struct sockaddr *)&msg->msg_local, NULL, 0), - print_host((struct sockaddr *)&msg->msg_peer, NULL, 0)); - timer_add(env, &msg->msg_timer, IKED_RESPONSE_TIMEOUT); + timer_add(env, &mr->mrt_timer, IKED_RESPONSE_TIMEOUT); return (0); } void ikev2_msg_response_timeout(struct iked *env, void *arg) { - struct iked_message *msg = arg; - struct iked_sa *sa = msg->msg_sa; + struct iked_msg_retransmit *mr = arg; + struct iked_sa *sa; - ikev2_msg_dispose(env, &sa->sa_responses, msg); + sa = TAILQ_FIRST(&mr->mrt_frags)->msg_sa; + ikev2_msg_dispose(env, &sa->sa_responses, mr); } void ikev2_msg_retransmit_timeout(struct iked *env, void *arg) { - struct iked_message *msg = arg; + struct iked_msg_retransmit *mr = arg; + struct iked_message *msg = TAILQ_FIRST(&mr->mrt_frags); struct iked_sa *sa = msg->msg_sa; - if (msg->msg_tries < IKED_RETRANSMIT_TRIES) { - if (sendtofrom(msg->msg_fd, ibuf_data(msg->msg_data), - ibuf_size(msg->msg_data), 0, - (struct sockaddr *)&msg->msg_peer, msg->msg_peerlen, - (struct sockaddr *)&msg->msg_local, - msg->msg_locallen) == -1) { - log_warn("%s: sendtofrom", __func__); - ikev2_ike_sa_setreason(sa, "retransmit failed"); - sa_free(env, sa); - return; + if (mr->mrt_tries < IKED_RETRANSMIT_TRIES) { + TAILQ_FOREACH(msg, &mr->mrt_frags, msg_entry) { + if (sendtofrom(msg->msg_fd, ibuf_data(msg->msg_data), + ibuf_size(msg->msg_data), 0, + (struct sockaddr *)&msg->msg_peer, msg->msg_peerlen, + (struct sockaddr *)&msg->msg_local, + msg->msg_locallen) == -1) { + log_warn("%s: sendtofrom", __func__); + ikev2_ike_sa_setreason(sa, "retransmit failed"); + sa_free(env, sa); + return; + } + log_info("%sretransmit %d %s req %u peer %s " + "local %s", SPI_SA(sa, NULL), mr->mrt_tries + 1, + print_map(msg->msg_exchange, ikev2_exchange_map), + msg->msg_msgid, + print_host((struct sockaddr *)&msg->msg_peer, NULL, 0), + print_host((struct sockaddr *)&msg->msg_local, NULL, 0)); } /* Exponential timeout */ - timer_add(env, &msg->msg_timer, - IKED_RETRANSMIT_TIMEOUT * (2 << (msg->msg_tries++))); - log_info("%sretransmit %d %s req %u peer %s local %s", - SPI_SA(sa, NULL), - msg->msg_tries, - print_map(msg->msg_exchange, ikev2_exchange_map), - msg->msg_msgid, - print_host((struct sockaddr *)&msg->msg_peer, NULL, 0), - print_host((struct sockaddr *)&msg->msg_local, NULL, 0)); + timer_add(env, &mr->mrt_timer, + IKED_RETRANSMIT_TIMEOUT * (2 << (mr->mrt_tries++))); } else { log_debug("%s: retransmit limit reached for req %u", __func__, msg->msg_msgid); diff --git a/sbin/iked/ikev2_pld.c b/sbin/iked/ikev2_pld.c index 5f88fed6a86..a68b7bfab74 100644 --- a/sbin/iked/ikev2_pld.c +++ b/sbin/iked/ikev2_pld.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ikev2_pld.c,v 1.122 2021/12/01 16:42:13 deraadt Exp $ */ +/* $OpenBSD: ikev2_pld.c,v 1.123 2022/03/14 12:58:55 tobhe Exp $ */ /* * Copyright (c) 2019 Tobias Heider @@ -1747,6 +1747,12 @@ ikev2_frags_reassemble(struct iked *env, struct ikev2_payload *pld, sa_frag->frag_total_size); print_hex(ibuf_data(e), 0, sa_frag->frag_total_size); + /* Drop the original request's packets from the retransmit queue */ + if (msg->msg_response) + ikev2_msg_dispose(env, &msg->msg_sa->sa_requests, + ikev2_msg_lookup(env, &msg->msg_sa->sa_requests, msg, + msg->msg_exchange)); + /* * Parse decrypted payload */ -- 2.20.1