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@

diff --git a/sbin/iked/iked.h b/sbin/iked/iked.h
index 527ab17..ff6e66c 100644 (file)
--- 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 <tobias.heider@stusta.de>
@@ -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 b646c7f..3107b06 100644 (file)
--- 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 <tobias.heider@stusta.de>
@@ -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 0f47eb4..feab4aa 100644 (file)
--- 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 <tobias.heider@stusta.de>
@@ -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 5f88fed..a68b7bf 100644 (file)
--- 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 <tobias.heider@stusta.de>
@@ -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
         */