Correctly handle a retransmitted ClientHello

If we receive a ClientHello and send back a HelloVerifyRequest, we need
to be able to handle the scenario where the HelloVerifyRequest gets lost
and we receive another ClientHello with the message sequence number set to
0.

Fixes #18635

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/18654)

1 files changed, 85 insertions, 10 deletions

diff --git a/ssl/statem/statem_dtls.c b/ssl/statem/statem_dtls.c
index 4fa70be723..d83e7404cb 100644
--- a/ssl/statem/statem_dtls.c
+++ b/ssl/statem/statem_dtls.c
@@ -496,23 +496,64 @@ static int dtls1_retrieve_buffered_fragment(SSL_CONNECTION *s, size_t *len)
      * (2) update s->init_num
      */
     pitem *item;
+    piterator iter;
     hm_fragment *frag;
     int ret;
+    int chretran = 0;
 
+    iter = pqueue_iterator(s->d1->buffered_messages);
     do {
-        item = pqueue_peek(s->d1->buffered_messages);
+        item = pqueue_next(&iter);
         if (item == NULL)
             return 0;
 
         frag = (hm_fragment *)item->data;
 
         if (frag->msg_header.seq < s->d1->handshake_read_seq) {
-            /* This is a stale message that has been buffered so clear it */
-            pqueue_pop(s->d1->buffered_messages);
-            dtls1_hm_fragment_free(frag);
-            pitem_free(item);
-            item = NULL;
-            frag = NULL;
+            pitem *next;
+            hm_fragment *nextfrag;
+
+            if (!s->server
+                    || frag->msg_header.seq != 0
+                    || s->d1->handshake_read_seq != 1
+                    || s->statem.hand_state != DTLS_ST_SW_HELLO_VERIFY_REQUEST) {
+                /*
+                 * This is a stale message that has been buffered so clear it.
+                 * It is safe to pop this message from the queue even though
+                 * we have an active iterator
+                 */
+                pqueue_pop(s->d1->buffered_messages);
+                dtls1_hm_fragment_free(frag);
+                pitem_free(item);
+                item = NULL;
+                frag = NULL;
+            } else {
+                /*
+                 * We have fragments for a ClientHello without a cookie,
+                 * even though we have sent a HelloVerifyRequest. It is possible
+                 * that the HelloVerifyRequest got lost and this is a
+                 * retransmission of the original ClientHello
+                 */
+                next = pqueue_next(&iter);
+                if (next != NULL) {
+                    nextfrag = (hm_fragment *)next->data;
+                    if (nextfrag->msg_header.seq == s->d1->handshake_read_seq) {
+                        /*
+                        * We have fragments for both a ClientHello without
+                        * cookie and one with. Ditch the one without.
+                        */
+                        pqueue_pop(s->d1->buffered_messages);
+                        dtls1_hm_fragment_free(frag);
+                        pitem_free(item);
+                        item = next;
+                        frag = nextfrag;
+                    } else {
+                        chretran = 1;
+                    }
+                } else {
+                    chretran = 1;
+                }
+            }
         }
     } while (item == NULL);
 
@@ -520,7 +561,7 @@ static int dtls1_retrieve_buffered_fragment(SSL_CONNECTION *s, size_t *len)
     if (frag->reassembly != NULL)
         return 0;
 
-    if (s->d1->handshake_read_seq == frag->msg_header.seq) {
+    if (s->d1->handshake_read_seq == frag->msg_header.seq || chretran) {
         size_t frag_len = frag->msg_header.frag_len;
         pqueue_pop(s->d1->buffered_messages);
 
@@ -538,6 +579,16 @@ static int dtls1_retrieve_buffered_fragment(SSL_CONNECTION *s, size_t *len)
         pitem_free(item);
 
         if (ret) {
+            if (chretran) {
+                /*
+                 * We got a new ClientHello with a message sequence of 0.
+                 * Reset the read/write sequences back to the beginning.
+                 * We process it like this is the first time we've seen a
+                 * ClientHello from the client.
+                 */
+                s->d1->handshake_read_seq = 0;
+                s->d1->next_handshake_write_seq = 0;
+            }
             *len = frag_len;
             return 1;
         }
@@ -768,6 +819,7 @@ static int dtls_get_reassembled_message(SSL_CONNECTION *s, int *errtype,
     struct hm_header_st msg_hdr;
     size_t readbytes;
     SSL *ssl = SSL_CONNECTION_GET_SSL(s);
+    int chretran = 0;
 
     *errtype = 0;
 
@@ -837,8 +889,20 @@ static int dtls_get_reassembled_message(SSL_CONNECTION *s, int *errtype,
      * although we're still expecting seq 0 (ClientHello)
      */
     if (msg_hdr.seq != s->d1->handshake_read_seq) {
-        *errtype = dtls1_process_out_of_seq_message(s, &msg_hdr);
-        return 0;
+        if (!s->server
+                || msg_hdr.seq != 0
+                || s->d1->handshake_read_seq != 1
+                || wire[0] != SSL3_MT_CLIENT_HELLO
+                || s->statem.hand_state != DTLS_ST_SW_HELLO_VERIFY_REQUEST) {
+            *errtype = dtls1_process_out_of_seq_message(s, &msg_hdr);
+            return 0;
+        }
+        /*
+         * We received a ClientHello and sent back a HelloVerifyRequest. We
+         * now seem to have received a retransmitted initial ClientHello. That
+         * is allowed (possibly our HelloVerifyRequest got lost).
+         */
+        chretran = 1;
     }
 
     if (frag_len && frag_len < mlen) {
@@ -904,6 +968,17 @@ static int dtls_get_reassembled_message(SSL_CONNECTION *s, int *errtype,
         goto f_err;
     }
 
+    if (chretran) {
+        /*
+         * We got a new ClientHello with a message sequence of 0.
+         * Reset the read/write sequences back to the beginning.
+         * We process it like this is the first time we've seen a ClientHello
+         * from the client.
+         */
+        s->d1->handshake_read_seq = 0;
+        s->d1->next_handshake_write_seq = 0;
+    }
+
     /*
      * Note that s->init_num is *not* used as current offset in
      * s->init_buf->data, but as a counter summing up fragments' lengths: as