From: Frederik Wedel-Heinen Date: Fri, 17 Jan 2025 08:05:59 +0000 (+0100) Subject: This change fixes an issue where a DTLS 1.3 would calculate a wrong transcript hash. X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=718f5b4cfb526ca8edd1a17782842ba1b0c15d2c;p=thirdparty%2Fopenssl.git This change fixes an issue where a DTLS 1.3 would calculate a wrong transcript hash. A wrong transcript hash was calculated when the client received a HRR which caused interop failures with WolfSSL. This change also refactors the internal calls to ssl3_finish_mac() that no longer requires the "incl_hdr" argument. Reviewed-by: Viktor Dukhovni Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/26465) --- diff --git a/include/openssl/prov_ssl.h b/include/openssl/prov_ssl.h index 9f3e8197e30..09d1f652c6e 100644 --- a/include/openssl/prov_ssl.h +++ b/include/openssl/prov_ssl.h @@ -30,6 +30,8 @@ extern "C" { # define DTLS1_3_VERSION 0xFEFC # define DTLS1_BAD_VER 0x0100 +# define PROTO_VERSION_UNSET 0 + /* QUIC uses a 4 byte unsigned version number */ # define OSSL_QUIC1_VERSION 0x0000001 diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index 4225ff0aafc..b4f89580deb 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -1476,11 +1476,14 @@ int ssl_set_new_record_layer(SSL_CONNECTION *s, int version, int ssl_set_record_protocol_version(SSL_CONNECTION *s, int vers) { - if (!ossl_assert(s->rlayer.rrlmethod != NULL) + if ((s->negotiated_version != PROTO_VERSION_UNSET && s->negotiated_version != vers) + || !ossl_assert(s->rlayer.rrlmethod != NULL) || !ossl_assert(s->rlayer.wrlmethod != NULL) || !s->rlayer.rrlmethod->set_protocol_version(s->rlayer.rrl, vers) || !s->rlayer.wrlmethod->set_protocol_version(s->rlayer.wrl, vers)) return 0; + s->negotiated_version = vers; + return 1; } diff --git a/ssl/s3_enc.c b/ssl/s3_enc.c index 58adcb448b6..3aa9c2abde9 100644 --- a/ssl/s3_enc.c +++ b/ssl/s3_enc.c @@ -244,7 +244,7 @@ void ssl3_free_digest_list(SSL_CONNECTION *s) s->s3.handshake_dgst = NULL; } -int ssl3_finish_mac(SSL_CONNECTION *s, const unsigned char *buf, size_t len, int hmhdr_incl) +int ssl3_finish_mac(SSL_CONNECTION *s, const unsigned char *buf, size_t len) { int ret; @@ -273,20 +273,58 @@ int ssl3_finish_mac(SSL_CONNECTION *s, const unsigned char *buf, size_t len, int * we calculate the digest when initiating s->s3.handshake_dgst at which * point we know what the protocol version is. */ + if (s->negotiated_version == DTLS1_3_VERSION) { + /* + * In DTLS 1.3 we need to parse the messages that are buffered to + * be able to remove message_sequence, fragment_size and fragment_offset + * from the Transcript Hash calculation. + */ + while (len > 0) { + PACKET hmhdr; + unsigned long hmbodylen; + unsigned int msgtype; + size_t hmhdrlen; + + if (!ossl_assert(len >= SSL3_HM_HEADER_LENGTH) + || !PACKET_buf_init(&hmhdr, buf, SSL3_HM_HEADER_LENGTH) + || !PACKET_get_1(&hmhdr, &msgtype) + || !PACKET_get_net_3(&hmhdr, &hmbodylen)) { + SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + return 0; + } - if (SSL_CONNECTION_IS_DTLS13(s) && hmhdr_incl - && ossl_assert(len >= DTLS1_HM_HEADER_LENGTH)) { - ret = EVP_DigestUpdate(s->s3.handshake_dgst, buf, SSL3_HM_HEADER_LENGTH); - ret &= EVP_DigestUpdate(s->s3.handshake_dgst, - buf + DTLS1_HM_HEADER_LENGTH, - len - DTLS1_HM_HEADER_LENGTH); - } else { - ret = EVP_DigestUpdate(s->s3.handshake_dgst, buf, len); - } + /* + * SSL3_MT_MESSAGE_HASH is a dummy message type only used when + * calculating the transcript hash of the synthetic message in + * (D)TLS 1.3. + */ + if (msgtype == SSL3_MT_MESSAGE_HASH) + hmhdrlen = SSL3_HM_HEADER_LENGTH; + else + hmhdrlen = DTLS1_HM_HEADER_LENGTH; + + /* + * In DTLS 1.3 the transcript hash is calculated excluding the + * message_sequence, fragment_size and fragment_offset header + * fields which are carried in the last + * DTLS1_HM_HEADER_LENGTH - SSL3_HM_HEADER_LENGTH header bytes + * of the DTLS handshake message header. + */ + if (!ossl_assert(hmhdrlen + hmbodylen <= len) + || !EVP_DigestUpdate(s->s3.handshake_dgst, buf, SSL3_HM_HEADER_LENGTH) + || !EVP_DigestUpdate(s->s3.handshake_dgst, buf + hmhdrlen, hmbodylen)) { + SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + return 0; + } - if (!ret) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); - return 0; + buf += hmhdrlen + hmbodylen; + len -= hmhdrlen + hmbodylen; + } + } else { + if (!EVP_DigestUpdate(s->s3.handshake_dgst, buf, len)) { + SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + return 0; + } } } return 1; @@ -321,33 +359,9 @@ int ssl3_digest_cached_records(SSL_CONNECTION *s, int keep) SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); return 0; } - - if (SSL_CONNECTION_IS_DTLS13(s)) { - unsigned char *hm = hdata; - size_t remlen = hdatalen; - - while (remlen > 0) { - PACKET hmhdr; - unsigned long msglen; - - if (remlen < DTLS1_HM_HEADER_LENGTH - || !PACKET_buf_init(&hmhdr, hm, DTLS1_HM_HEADER_LENGTH) - || !PACKET_forward(&hmhdr, 1) - || !PACKET_get_net_3(&hmhdr, &msglen) - || (msglen + DTLS1_HM_HEADER_LENGTH) > remlen - || !ssl3_finish_mac(s, hm, msglen + DTLS1_HM_HEADER_LENGTH, 1)) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); - return 0; - } - - hm += msglen + DTLS1_HM_HEADER_LENGTH; - remlen -= msglen + DTLS1_HM_HEADER_LENGTH; - } - } else { - if (!ssl3_finish_mac(s, hdata, hdatalen, 1)) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); - return 0; - } + if (!ssl3_finish_mac(s, hdata, hdatalen)) { + /* SSLfatal() already called */ + return 0; } } if (keep == 0) { diff --git a/ssl/ssl_local.h b/ssl/ssl_local.h index 1ae86d2e9de..3c20ab516aa 100644 --- a/ssl/ssl_local.h +++ b/ssl/ssl_local.h @@ -1276,6 +1276,13 @@ struct ssl_connection_st { * DTLS1_3_VERSION) */ int version; + + /* + * The negotiated version for the connection. Initially PROTO_VERSION_UNSET. + * Set by ssl_set_negotiated_protocol_version(). + */ + int negotiated_version; + /* * There are 2 BIO's even though they are normally both the same. This * is so data can be read and written to different handlers @@ -2710,7 +2717,7 @@ __owur int ssl3_dispatch_alert(SSL *s); __owur size_t ssl3_final_finish_mac(SSL_CONNECTION *s, const char *sender, size_t slen, unsigned char *p); __owur int ssl3_finish_mac(SSL_CONNECTION *s, const unsigned char *buf, - size_t len, int hmhdr_incl); + size_t len); void ssl3_free_digest_list(SSL_CONNECTION *s); __owur unsigned long ssl3_output_cert_chain(SSL_CONNECTION *s, WPACKET *pkt, CERT_PKEY *cpk, int for_comp); diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index 7a771254d9f..11d2cdff365 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -710,7 +710,7 @@ WORK_STATE ossl_statem_client_pre_work(SSL_CONNECTION *s, WORK_STATE wst) case TLS_ST_CW_CLNT_HELLO: s->shutdown = 0; - if (SSL_CONNECTION_IS_DTLS(s)) { + if (SSL_CONNECTION_IS_DTLS(s) && s->hello_retry_request != SSL_HRR_PENDING) { /* every DTLS ClientHello resets Finished MAC */ if (!ssl3_init_finished_mac(s)) { /* SSLfatal() already called */ @@ -1900,7 +1900,7 @@ static MSG_PROCESS_RETURN tls_process_as_hello_retry_request(SSL_CONNECTION *s, * for HRR messages. */ if (!ssl3_finish_mac(s, (unsigned char *)s->init_buf->data, - s->init_num + msghdrlen, 1)) { + s->init_num + msghdrlen)) { /* SSLfatal() already called */ goto err; } diff --git a/ssl/statem/statem_dtls.c b/ssl/statem/statem_dtls.c index a218e787a00..819881174a1 100644 --- a/ssl/statem/statem_dtls.c +++ b/ssl/statem/statem_dtls.c @@ -291,15 +291,14 @@ int dtls1_do_write(SSL_CONNECTION *s, uint8_t recordtype) assert(s->s3.tmp.new_compression != NULL || BIO_wpending(s->wbio) <= (int)s->d1->mtu); - if (recordtype == SSL3_RT_HANDSHAKE && !s->d1->retransmitting) { - /* - * should not be done for 'Hello Request's, but in that case - * we'll ignore the result anyway - */ - size_t xlen; - int hmhdr_incl; + if (recordtype == SSL3_RT_HANDSHAKE && !s->d1->retransmitting + && s->init_off == 0) { + size_t xlen = s->init_num; - if (s->init_off == 0 && s->version != DTLS1_BAD_VER) { + if (s->version == DTLS1_BAD_VER) { + msgstart += DTLS1_HM_HEADER_LENGTH; + xlen -= DTLS1_HM_HEADER_LENGTH; + } else { /* * Now prepare to calculate the transcript hash. For * versions prior to DTLSv1.3 this means: @@ -318,14 +317,8 @@ int dtls1_do_write(SSL_CONNECTION *s, uint8_t recordtype) if (!dtls1_write_hm_header(msgstart, msg_type, msg_len, msg_seq, 0, msg_len)) return -1; - - xlen = written; - hmhdr_incl = 1; - } else { - msgstart += DTLS1_HM_HEADER_LENGTH; - xlen = written - DTLS1_HM_HEADER_LENGTH; - hmhdr_incl = 0; } + /* * should not be done for 'Hello Request's, but in that case we'll * ignore the result anyway @@ -334,12 +327,10 @@ int dtls1_do_write(SSL_CONNECTION *s, uint8_t recordtype) if (!SSL_CONNECTION_IS_DTLS13(s) || (s->statem.hand_state != TLS_ST_SW_SESSION_TICKET && s->statem.hand_state != TLS_ST_CW_KEY_UPDATE - && s->statem.hand_state != TLS_ST_SW_KEY_UPDATE)) { - if (!ssl3_finish_mac(s, msgstart, xlen, hmhdr_incl)) + && s->statem.hand_state != TLS_ST_SW_KEY_UPDATE)) + if (!ssl3_finish_mac(s, msgstart, xlen)) return -1; - } } - if (written == s->init_num) { if (s->msg_callback) s->msg_callback(1, s->version, recordtype, s->init_buf->data, diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index b738090d72c..faf76dd23b7 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -107,7 +107,7 @@ int ssl3_do_write(SSL_CONNECTION *s, uint8_t type) && s->statem.hand_state != TLS_ST_SW_KEY_UPDATE)) if (!ssl3_finish_mac(s, (unsigned char *)&s->init_buf->data[s->init_off], - written, 1)) + written)) return -1; if (written == s->init_num) { s->statem.write_in_progress = 0; @@ -1657,7 +1657,7 @@ int tls_common_finish_mac(SSL_CONNECTION *s) { /* Feed this message into MAC computation. */ if (RECORD_LAYER_is_sslv2_record(&s->rlayer) - && !ssl3_finish_mac(s, msg, msg_len, 1)) { + && !ssl3_finish_mac(s, msg, msg_len)) { /* SSLfatal() already called */ return 0; } else { @@ -1678,7 +1678,7 @@ int tls_common_finish_mac(SSL_CONNECTION *s) { || memcmp(hrrrandom, s->init_buf->data + srvhellorandom_offs, SSL3_RANDOM_SIZE) != 0) { - if (!ssl3_finish_mac(s, msg, msg_len, 1)) + if (!ssl3_finish_mac(s, msg, msg_len)) /* SSLfatal() already called */ return 0; } @@ -2654,23 +2654,27 @@ int create_synthetic_message_hash(SSL_CONNECTION *s, size_t hashlen, const unsigned char *hrr, size_t hrrlen) { - unsigned char hashvaltmp[EVP_MAX_MD_SIZE]; - unsigned char synmsghdr[SSL3_HM_HEADER_LENGTH]; + unsigned char *hashvaltmp; + unsigned char synmsg[SSL3_HM_HEADER_LENGTH + EVP_MAX_MD_SIZE]; size_t currmsghdr_len = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_HM_HEADER_LENGTH : SSL3_HM_HEADER_LENGTH; - memset(synmsghdr, 0, sizeof(synmsghdr)); + memset(synmsg, 0, SSL3_HM_HEADER_LENGTH); + hashvaltmp = synmsg + SSL3_HM_HEADER_LENGTH; if (hashval == NULL) { - hashval = hashvaltmp; - hashlen = 0; /* Get the hash of the initial ClientHello */ if (!ssl3_digest_cached_records(s, 0) - || !ssl_handshake_hash(s, hashvaltmp, sizeof(hashvaltmp), + || !ssl_handshake_hash(s, hashvaltmp, EVP_MAX_MD_SIZE, &hashlen)) { /* SSLfatal() already called */ return 0; } + } else { + if (!ossl_assert(hashlen <= EVP_MAX_MD_SIZE)) + return 0; + + memcpy(hashvaltmp, hashval, hashlen); } /* Reinitialise the transcript hash */ @@ -2680,10 +2684,9 @@ int create_synthetic_message_hash(SSL_CONNECTION *s, } /* Inject the synthetic message_hash message */ - synmsghdr[0] = SSL3_MT_MESSAGE_HASH; - synmsghdr[SSL3_HM_HEADER_LENGTH - 1] = (unsigned char)hashlen; - if (!ssl3_finish_mac(s, synmsghdr, SSL3_HM_HEADER_LENGTH, 0) - || !ssl3_finish_mac(s, hashval, hashlen, 0)) { + synmsg[0] = SSL3_MT_MESSAGE_HASH; + synmsg[SSL3_HM_HEADER_LENGTH - 1] = (unsigned char)hashlen; + if (!ssl3_finish_mac(s, synmsg, SSL3_HM_HEADER_LENGTH + hashlen)) { /* SSLfatal() already called */ return 0; } @@ -2694,9 +2697,9 @@ int create_synthetic_message_hash(SSL_CONNECTION *s, * receiving a ClientHello2 with a cookie. */ if (hrr != NULL - && (!ssl3_finish_mac(s, hrr, hrrlen, 1) + && (!ssl3_finish_mac(s, hrr, hrrlen) || !ssl3_finish_mac(s, (unsigned char *)s->init_buf->data, - s->s3.tmp.message_size + currmsghdr_len, 1))) { + s->s3.tmp.message_size + currmsghdr_len))) { /* SSLfatal() already called */ return 0; }