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

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

diff --git a/fuzz/dtlsclient.c b/fuzz/dtlsclient.c
index c6d04b175dc8287e324f375206e1af200804adac..4aa84f88a0c3fb24865ccbad577ace8a9f1a4d3d 100644 (file)
--- a/fuzz/dtlsclient.c
+++ b/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);
-    /**
-     * 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_max_proto_version(client, 0) == 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());

diff --git a/ssl/d1_lib.c b/ssl/d1_lib.c
index c0724c10f5079f9e38913a2e130ed172f6bb19aa..e06b65a426e0fedbc58ab3e121deb2f8ab2280ba 100644 (file)
--- a/ssl/d1_lib.c
+++ b/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);

diff --git a/ssl/ssl_local.h b/ssl/ssl_local.h
index 5f66555d6b1b375b283cef13be60e6fca58b64e3..145042f97abaad0a18489b6510d4ea933c5c70d8 100644 (file)
--- a/ssl/ssl_local.h
+++ b/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;

diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c
index a7a4d11f250cd8014de8c6964504a331b1d0788f..1a615a1b0c4f27d2ec20082f6821548f4267e1d5 100644 (file)
--- a/ssl/statem/extensions_clnt.c
+++ b/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 {

diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index b51f141bced0bb5db677c3d8b4835a7efd2843b9..ba8d414d3dcb2cfbca29de0b7481c04a91328887 100644 (file)
--- a/ssl/statem/statem_clnt.c
+++ b/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;
 }
 

diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index 7e9c39e3fd36bc9005853ce2c46ba29c1f07a147..2e1e1b739b2ed7415de929d97f16d6c176739101 100644 (file)
--- a/ssl/statem/statem_lib.c
+++ b/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,21 +2578,19 @@ int ssl_set_client_hello_version(SSL_CONNECTION *s)
 
     s->version = ver_max;
 
-    if (SSL_CONNECTION_IS_DTLS(s)) {
-        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
-             * anything else, and this has impacts on the record layer for when
-             * we read the ServerHello. So we need to tell the record layer
-             * about this immediately.
-             */
-            if (!ssl_set_record_protocol_version(s, ver_max))
-                return 0;
-        }
-    } else if (ver_max > TLS1_2_VERSION) {
-        /* TLS1.3 always uses TLS1.2 in the legacy_version field */
-        ver_max = TLS1_2_VERSION;
+    if (SSL_CONNECTION_IS_DTLS(s) && 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
+         * anything else, and this has impacts on the record layer for when
+         * we read the ServerHello. So we need to tell the record layer
+         * about this immediately.
+         */
+        if (!ssl_set_record_protocol_version(s, ver_max))
+            return 0;
+    } else if (ssl_version_cmp(s, ver_max, version1_2) > 0) {
+        /* (D)TLS1.3 always uses (D)TLS1.2 in the legacy_version field */
+        ver_max = version1_2;
     }
 
     s->client_version = ver_max;

diff --git a/test/dtlstest.c b/test/dtlstest.c
index aacf67e76354181b50c00da9f03ddbddcfa1ae84..c8bf5125f438a12201efb40944c3fea7dfd1aeb4 100644 (file)
--- a/test/dtlstest.c
+++ b/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:
-     *      Assertion failed: s->hello_retry_request == SSL_HRR_PENDING
+     *  dtls1_read_bytes:ssl/tls alert unexpected message:
+     *      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;