Fix an assertion failure which happens when a DTLS 1.3 client receives a HelloVerifyRequest.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/24509)

fuzz/dtlsclient.c

@@ -72,12 +72,7 @@ int FuzzerTestOneInput(const uint8_t *buf, size_t len)
     if (client == NULL)
         goto end;
     OPENSSL_assert(SSL_set_min_proto_version(client, 0) == 1);
-    /**
+    OPENSSL_assert(SSL_set_max_proto_version(client, 0) == 1);
-     * TODO(DTLSv1.3): Fuzzing fails with
-     * ssl/statem/extensions_clnt.c:624: OpenSSL internal error:
-     *      Assertion failed: s->hello_retry_request == SSL_HRR_PENDING
-     */
-    OPENSSL_assert(SSL_set_max_proto_version(client, DTLS1_2_VERSION) == 1);
     OPENSSL_assert(SSL_set_cipher_list(client, "ALL:eNULL:@SECLEVEL=0") == 1);
     SSL_set_tlsext_host_name(client, "localhost");
     in = BIO_new(BIO_s_mem());

ssl/d1_lib.c

@@ -105,6 +105,7 @@ int dtls1_new(SSL *ssl)
 
     d1->link_mtu = 0;
     d1->mtu = 0;
+    d1->hello_verify_request = SSL_HVR_NONE;
 
     if (d1->buffered_messages == NULL || d1->sent_messages == NULL) {
         pqueue_free(d1->buffered_messages);

ssl/ssl_local.h

@@ -2014,6 +2014,8 @@ typedef struct dtls1_state_st {
     pqueue *buffered_messages;
     /* Buffered (sent) handshake records */
     pqueue *sent_messages;
+    /* Flag to indicate current HelloVerifyRequest status */
+    enum {SSL_HVR_NONE = 0, SSL_HVR_RECEIVED} hello_verify_request;
     size_t link_mtu;      /* max on-the-wire DTLS packet size */
     size_t mtu;           /* max DTLS packet size */
     struct hm_header_st w_msg_hdr;

ssl/statem/extensions_clnt.c

@@ -654,13 +654,15 @@ static int add_key_share(SSL_CONNECTION *s, WPACKET *pkt, unsigned int group_id,
     size_t encodedlen;
 
     if (loop_num < s->s3.tmp.num_ks_pkey) {
-        if (!ossl_assert(s->hello_retry_request == SSL_HRR_PENDING)
+        if (!ossl_assert(s->hello_retry_request == SSL_HRR_PENDING
+                         || s->d1->hello_verify_request == SSL_HVR_RECEIVED)
             || !ossl_assert(s->s3.tmp.ks_pkey[loop_num] != NULL)) {
             SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
             return 0;
         }
         /*
-         * Could happen if we got an HRR that wasn't requesting a new key_share
+         * Could happen if we got a HRR that wasn't requesting a new key_share
+         * or if we got a HelloVerifyRequest
          */
         key_share_key = s->s3.tmp.ks_pkey[loop_num];
     } else {

ssl/statem/statem_clnt.c

@@ -1357,6 +1357,8 @@ MSG_PROCESS_RETURN dtls_process_hello_verify(SSL_CONNECTION *s, PACKET *pkt)
 {
     size_t cookie_len;
     PACKET cookiepkt;
+    int min_version;
+    SSL *ssl = SSL_CONNECTION_GET_SSL(s);
 
     if (!PACKET_forward(pkt, 2)
         || !PACKET_get_length_prefixed_1(pkt, &cookiepkt)) {
@@ -1376,6 +1378,25 @@ MSG_PROCESS_RETURN dtls_process_hello_verify(SSL_CONNECTION *s, PACKET *pkt)
     }
     s->d1->cookie_len = cookie_len;
 
+    if (ssl == NULL) {
+        SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
+        return MSG_PROCESS_ERROR;
+    }
+
+    min_version = SSL_get_min_proto_version(ssl);
+
+    /*
+     * Server responds with a HelloVerify which means we cannot negotiate a
+     * higher version than DTLSv1.2.
+     */
+    if (min_version != 0
+            && ssl_version_cmp(s, min_version, DTLS1_2_VERSION) > 0) {
+        SSLfatal(s, SSL_AD_PROTOCOL_VERSION, SSL_R_UNSUPPORTED_PROTOCOL);
+        return MSG_PROCESS_ERROR;
+    }
+
+    s->d1->hello_verify_request = SSL_HVR_RECEIVED;
+
     return MSG_PROCESS_FINISHED_READING;
 }
 

ssl/statem/statem_lib.c

@@ -2562,6 +2562,7 @@ int ssl_get_min_max_version(const SSL_CONNECTION *s, int *min_version,
 int ssl_set_client_hello_version(SSL_CONNECTION *s)
 {
     int ver_min, ver_max, ret;
+    const int version1_2 = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_2_VERSION : TLS1_2_VERSION;
 
     /*
      * In a renegotiation we always send the same client_version that we sent
@@ -2577,8 +2578,7 @@ int ssl_set_client_hello_version(SSL_CONNECTION *s)
 
     s->version = ver_max;
 
-    if (SSL_CONNECTION_IS_DTLS(s)) {
+    if (SSL_CONNECTION_IS_DTLS(s) && ver_max == DTLS1_BAD_VER) {
-        if (ver_max == DTLS1_BAD_VER) {
         /*
          * Even though this is technically before version negotiation,
          * because we have asked for DTLS1_BAD_VER we will never negotiate
@@ -2588,10 +2588,9 @@ int ssl_set_client_hello_version(SSL_CONNECTION *s)
          */
         if (!ssl_set_record_protocol_version(s, ver_max))
             return 0;
-        }
+    } else if (ssl_version_cmp(s, ver_max, version1_2) > 0) {
-    } else if (ver_max > TLS1_2_VERSION) {
+        /* (D)TLS1.3 always uses (D)TLS1.2 in the legacy_version field */
-        /* TLS1.3 always uses TLS1.2 in the legacy_version field */
+        ver_max = version1_2;
-        ver_max = TLS1_2_VERSION;
     }
 
     s->client_version = ver_max;

test/dtlstest.c

@@ -207,8 +207,9 @@ static int test_dtls_drop_records(int idx)
 
     /**
      * TODO(DTLSv1.3): Tests fails with
-     *  ssl/statem/extensions_clnt.c:624: OpenSSL internal error:
+     *  dtls1_read_bytes:ssl/tls alert unexpected message:
-     *      Assertion failed: s->hello_retry_request == SSL_HRR_PENDING
+     *      ssl/record/rec_layer_d1.c:454:SSL alert number 10
+     * And "no progress made"
      */
     if (!TEST_true(create_ssl_ctx_pair(NULL, DTLS_server_method(),
                                        DTLS_client_method(),
@@ -711,14 +712,9 @@ static int test_listen(void)
     SSL *serverssl = NULL, *clientssl = NULL;
     int testresult = 0;
 
-    /**
-     * TODO(DTLSv1.3): Tests fails with
-     *  ssl/statem/extensions_clnt.c:624: OpenSSL internal error:
-     *      Assertion failed: s->hello_retry_request == SSL_HRR_PENDING
-     */
     if (!TEST_true(create_ssl_ctx_pair(NULL, DTLS_server_method(),
                                        DTLS_client_method(),
-                                       DTLS1_VERSION, DTLS1_2_VERSION,
+                                       DTLS1_VERSION, 0,
                                        &sctx, &cctx, cert, privkey)))
         return 0;