Fix an assertion failure which happens when a DTLS 1.3 client receive…

…s a HelloVerifyRequest.
fwh-dc · Jun 4, 2024 · 9fcf094 · 9fcf094
1 parent a4bb3b8
commit 9fcf094
Show file tree

Hide file tree

Showing 7 changed files with 50 additions and 32 deletions.
diff --git 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
@@ -103,6 +103,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
@@ -1992,6 +1992,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
@@ -639,12 +639,14 @@ static int add_key_share(SSL_CONNECTION *s, WPACKET *pkt, unsigned int curve_id)
     size_t encodedlen;
 
     if (s->s3.tmp.pkey != NULL) {
-        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)) {
             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.pkey;
     } else {

diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
@@ -1279,7 +1279,9 @@ CON_FUNC_RETURN tls_construct_client_hello(SSL_CONNECTION *s, WPACKET *pkt)
     }
 
     /* cookie stuff for DTLS */
-    if (SSL_CONNECTION_IS_DTLS(s)) {
+    if (SSL_CONNECTION_IS_DTLS(s)
+        && ossl_assert(!(s->d1->hello_verify_request == SSL_HVR_RECEIVED)
+                       || s->d1->cookie_len > 0)) {
         if (s->d1->cookie_len > sizeof(s->d1->cookie)
                 || !WPACKET_sub_memcpy_u8(pkt, s->d1->cookie,
                                           s->d1->cookie_len)) {
@@ -1342,6 +1344,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)) {
@@ -1361,6 +1365,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
@@ -2538,6 +2538,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
@@ -2553,21 +2554,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
@@ -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
+     *  tls_psk_do_binder:binder does not verify:ssl/statem/extensions.c:1690:
+     *
+     * Check when https://github.com/openssl/openssl/pull/24426 has been merged
      */
     if (!TEST_true(create_ssl_ctx_pair(NULL, DTLS_server_method(),
                                        DTLS_client_method(),
@@ -612,14 +613,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;