diff --git a/fuzz/dtlsclient.c b/fuzz/dtlsclient.c index 85fb1144d6d9de..68dd7d658f7e3f 100644 --- 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 f36d5a3d5f4770..fa30dbcbb662b6 100644 --- 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 index 31d92f473e09f3..7b1093ef4b1ac1 100644 --- 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 index c2aecff67ea050..ad4863a907d34e 100644 --- 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 index 91ef9c4f412093..e457ca1c6967a7 100644 --- 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 index 1537c25ab0386f..109cf917814427 100644 --- 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 index 19bd8aab8c47da..c46de26242ab51 100644 --- 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;