From 635c8f771574fbf48281b2372a2f7aba0c673544 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Thu, 23 Nov 2017 11:41:40 +0000 Subject: [PATCH] Fix up a few places in the state machine that got missed with SSLfatal() Reviewed-by: Richard Levitte (Merged from https://github.com/openssl/openssl/pull/4778) --- crypto/err/openssl.txt | 1 + include/openssl/sslerr.h | 1 + ssl/ssl_err.c | 2 ++ ssl/statem/extensions.c | 38 +++++++++++++++++++++++------------ ssl/statem/extensions_clnt.c | 39 +++++++++++++++++------------------- ssl/statem/extensions_srvr.c | 3 +-- ssl/statem/statem_clnt.c | 7 ++++--- 7 files changed, 52 insertions(+), 39 deletions(-) diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt index 9a95662b11..601f1e7c01 100644 --- a/crypto/err/openssl.txt +++ b/crypto/err/openssl.txt @@ -2371,6 +2371,7 @@ SSL_R_BAD_SRTP_PROTECTION_PROFILE_LIST:353:bad srtp protection profile list SSL_R_BAD_SSL_FILETYPE:124:bad ssl filetype SSL_R_BAD_VALUE:384:bad value SSL_R_BAD_WRITE_RETRY:127:bad write retry +SSL_R_BINDER_DOES_NOT_VERIFY:253:binder does not verify SSL_R_BIO_NOT_SET:128:bio not set SSL_R_BLOCK_CIPHER_PAD_IS_WRONG:129:block cipher pad is wrong SSL_R_BN_LIB:130:bn lib diff --git a/include/openssl/sslerr.h b/include/openssl/sslerr.h index be7e0c6f6b..9774f9ff19 100644 --- a/include/openssl/sslerr.h +++ b/include/openssl/sslerr.h @@ -455,6 +455,7 @@ int ERR_load_SSL_strings(void); # define SSL_R_BAD_SSL_FILETYPE 124 # define SSL_R_BAD_VALUE 384 # define SSL_R_BAD_WRITE_RETRY 127 +# define SSL_R_BINDER_DOES_NOT_VERIFY 253 # define SSL_R_BIO_NOT_SET 128 # define SSL_R_BLOCK_CIPHER_PAD_IS_WRONG 129 # define SSL_R_BN_LIB 130 diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c index 1bfa56328e..88e741e9fd 100644 --- a/ssl/ssl_err.c +++ b/ssl/ssl_err.c @@ -714,6 +714,8 @@ static const ERR_STRING_DATA SSL_str_reasons[] = { {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BAD_SSL_FILETYPE), "bad ssl filetype"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BAD_VALUE), "bad value"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BAD_WRITE_RETRY), "bad write retry"}, + {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BINDER_DOES_NOT_VERIFY), + "binder does not verify"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BIO_NOT_SET), "bio not set"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BLOCK_CIPHER_PAD_IS_WRONG), "block cipher pad is wrong"}, diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c index 062fc0438b..f4aa98e6d1 100644 --- a/ssl/statem/extensions.c +++ b/ssl/statem/extensions.c @@ -1296,7 +1296,8 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart, } if (sess->master_key_length != hashsize) { - SSLerr(SSL_F_TLS_PSK_DO_BINDER, SSL_R_BAD_PSK); + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PSK_DO_BINDER, + SSL_R_BAD_PSK); goto err; } @@ -1308,7 +1309,7 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart, (const unsigned char *)nonce_label, sizeof(nonce_label) - 1, sess->ext.tick_nonce, sess->ext.tick_nonce_len, psk, hashsize)) { - SSLerr(SSL_F_TLS_PSK_DO_BINDER, ERR_R_INTERNAL_ERROR); + /* SSLfatal() already called */ goto err; } } @@ -1326,7 +1327,7 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart, else early_secret = (unsigned char *)sess->early_secret; if (!tls13_generate_secret(s, md, NULL, psk, hashsize, early_secret)) { - SSLerr(SSL_F_TLS_PSK_DO_BINDER, ERR_R_INTERNAL_ERROR); + /* SSLfatal() already called */ goto err; } @@ -1338,25 +1339,27 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart, if (mctx == NULL || EVP_DigestInit_ex(mctx, md, NULL) <= 0 || EVP_DigestFinal_ex(mctx, hash, NULL) <= 0) { - SSLerr(SSL_F_TLS_PSK_DO_BINDER, ERR_R_INTERNAL_ERROR); + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PSK_DO_BINDER, + ERR_R_INTERNAL_ERROR); goto err; } /* Generate the binder key */ if (!tls13_hkdf_expand(s, md, early_secret, (unsigned char *)label, labelsize, hash, hashsize, binderkey, hashsize)) { - SSLerr(SSL_F_TLS_PSK_DO_BINDER, ERR_R_INTERNAL_ERROR); + /* SSLfatal() already called */ goto err; } /* Generate the finished key */ if (!tls13_derive_finishedkey(s, md, binderkey, finishedkey, hashsize)) { - SSLerr(SSL_F_TLS_PSK_DO_BINDER, ERR_R_INTERNAL_ERROR); + /* SSLfatal() already called */ goto err; } if (EVP_DigestInit_ex(mctx, md, NULL) <= 0) { - SSLerr(SSL_F_TLS_PSK_DO_BINDER, ERR_R_INTERNAL_ERROR); + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PSK_DO_BINDER, + ERR_R_INTERNAL_ERROR); goto err; } @@ -1371,7 +1374,8 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart, hdatalen = BIO_get_mem_data(s->s3->handshake_buffer, &hdata); if (hdatalen <= 0) { - SSLerr(SSL_F_TLS_PSK_DO_BINDER, SSL_R_BAD_HANDSHAKE_LENGTH); + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PSK_DO_BINDER, + SSL_R_BAD_HANDSHAKE_LENGTH); goto err; } @@ -1388,27 +1392,31 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart, || !PACKET_get_length_prefixed_3(&hashprefix, &msg) || !PACKET_forward(&hashprefix, 1) || !PACKET_get_length_prefixed_3(&hashprefix, &msg)) { - SSLerr(SSL_F_TLS_PSK_DO_BINDER, ERR_R_INTERNAL_ERROR); + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PSK_DO_BINDER, + ERR_R_INTERNAL_ERROR); goto err; } hdatalen -= PACKET_remaining(&hashprefix); } if (EVP_DigestUpdate(mctx, hdata, hdatalen) <= 0) { - SSLerr(SSL_F_TLS_PSK_DO_BINDER, ERR_R_INTERNAL_ERROR); + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PSK_DO_BINDER, + ERR_R_INTERNAL_ERROR); goto err; } } if (EVP_DigestUpdate(mctx, msgstart, binderoffset) <= 0 || EVP_DigestFinal_ex(mctx, hash, NULL) <= 0) { - SSLerr(SSL_F_TLS_PSK_DO_BINDER, ERR_R_INTERNAL_ERROR); + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PSK_DO_BINDER, + ERR_R_INTERNAL_ERROR); goto err; } mackey = EVP_PKEY_new_mac_key(EVP_PKEY_HMAC, NULL, finishedkey, hashsize); if (mackey == NULL) { - SSLerr(SSL_F_TLS_PSK_DO_BINDER, ERR_R_INTERNAL_ERROR); + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PSK_DO_BINDER, + ERR_R_INTERNAL_ERROR); goto err; } @@ -1420,7 +1428,8 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart, || EVP_DigestSignUpdate(mctx, hash, hashsize) <= 0 || EVP_DigestSignFinal(mctx, binderout, &bindersize) <= 0 || bindersize != hashsize) { - SSLerr(SSL_F_TLS_PSK_DO_BINDER, ERR_R_INTERNAL_ERROR); + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PSK_DO_BINDER, + ERR_R_INTERNAL_ERROR); goto err; } @@ -1429,6 +1438,9 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart, } else { /* HMAC keys can't do EVP_DigestVerify* - use CRYPTO_memcmp instead */ ret = (CRYPTO_memcmp(binderin, binderout, hashsize) == 0); + if (!ret) + SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, SSL_F_TLS_PSK_DO_BINDER, + SSL_R_BINDER_DOES_NOT_VERIFY); } err: diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c index 326d77eb56..b7ef54e8b7 100644 --- a/ssl/statem/extensions_clnt.c +++ b/ssl/statem/extensions_clnt.c @@ -196,15 +196,17 @@ EXT_RETURN tls_construct_ctos_supported_groups(SSL *s, WPACKET *pkt, if (tls_curve_allowed(s, ctmp, SSL_SECOP_CURVE_SUPPORTED)) { if (!WPACKET_put_bytes_u16(pkt, ctmp)) { - SSLerr(SSL_F_TLS_CONSTRUCT_CTOS_SUPPORTED_GROUPS, - ERR_R_INTERNAL_ERROR); + SSLfatal(s, SSL_AD_INTERNAL_ERROR, + SSL_F_TLS_CONSTRUCT_CTOS_SUPPORTED_GROUPS, + ERR_R_INTERNAL_ERROR); return EXT_RETURN_FAIL; } } } if (!WPACKET_close(pkt) || !WPACKET_close(pkt)) { - SSLerr(SSL_F_TLS_CONSTRUCT_CTOS_SUPPORTED_GROUPS, - ERR_R_INTERNAL_ERROR); + SSLfatal(s, SSL_AD_INTERNAL_ERROR, + SSL_F_TLS_CONSTRUCT_CTOS_SUPPORTED_GROUPS, + ERR_R_INTERNAL_ERROR); return EXT_RETURN_FAIL; } @@ -934,7 +936,6 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context, size_t reshashsize = 0, pskhashsize = 0, binderoffset, msglen; unsigned char *resbinder = NULL, *pskbinder = NULL, *msgstart = NULL; const EVP_MD *handmd = NULL, *mdres = NULL, *mdpsk = NULL; - EXT_RETURN ret = EXT_RETURN_FAIL; int dores = 0; s->session->ext.tick_identity = TLSEXT_PSK_BAD_IDENTITY; @@ -961,7 +962,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context, if (s->session->cipher == NULL) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CTOS_PSK, ERR_R_INTERNAL_ERROR); - goto err; + return EXT_RETURN_FAIL; } mdres = ssl_md(s->session->cipher->algorithm2); if (mdres == NULL) { @@ -1033,7 +1034,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context, */ SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CTOS_PSK, SSL_R_BAD_PSK); - goto err; + return EXT_RETURN_FAIL; } if (s->hello_retry_request && mdpsk != handmd) { @@ -1043,7 +1044,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context, */ SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CTOS_PSK, SSL_R_BAD_PSK); - goto err; + return EXT_RETURN_FAIL; } pskhashsize = EVP_MD_size(mdpsk); @@ -1055,7 +1056,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context, || !WPACKET_start_sub_packet_u16(pkt)) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CTOS_PSK, ERR_R_INTERNAL_ERROR); - goto err; + return EXT_RETURN_FAIL; } if (dores) { @@ -1064,7 +1065,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context, || !WPACKET_put_bytes_u32(pkt, agems)) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CTOS_PSK, ERR_R_INTERNAL_ERROR); - goto err; + return EXT_RETURN_FAIL; } } @@ -1074,7 +1075,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context, || !WPACKET_put_bytes_u32(pkt, 0)) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CTOS_PSK, ERR_R_INTERNAL_ERROR); - goto err; + return EXT_RETURN_FAIL; } } @@ -1095,7 +1096,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context, || !WPACKET_fill_lengths(pkt)) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CTOS_PSK, ERR_R_INTERNAL_ERROR); - goto err; + return EXT_RETURN_FAIL; } msgstart = WPACKET_get_curr(pkt) - msglen; @@ -1103,17 +1104,15 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context, if (dores && tls_psk_do_binder(s, mdres, msgstart, binderoffset, NULL, resbinder, s->session, 1, 0) != 1) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CTOS_PSK, - ERR_R_INTERNAL_ERROR); - goto err; + /* SSLfatal() already called */ + return EXT_RETURN_FAIL; } if (s->psksession != NULL && tls_psk_do_binder(s, mdpsk, msgstart, binderoffset, NULL, pskbinder, s->psksession, 1, 1) != 1) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CTOS_PSK, - ERR_R_INTERNAL_ERROR); - goto err; + /* SSLfatal() already called */ + return EXT_RETURN_FAIL; } if (dores) @@ -1121,9 +1120,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context, if (s->psksession != NULL) s->psksession->ext.tick_identity = (dores ? 1 : 0); - ret = EXT_RETURN_SENT; - err: - return ret; + return EXT_RETURN_SENT; #else return EXT_RETURN_NOT_SENT; #endif diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c index 7adb555e44..ca1cef59a8 100644 --- a/ssl/statem/extensions_srvr.c +++ b/ssl/statem/extensions_srvr.c @@ -858,8 +858,7 @@ int tls_parse_ctos_psk(SSL *s, PACKET *pkt, unsigned int context, X509 *x, (const unsigned char *)s->init_buf->data, binderoffset, PACKET_data(&binder), NULL, sess, 0, ext) != 1) { - SSLfatal(s, SSL_AD_DECODE_ERROR, SSL_F_TLS_PARSE_CTOS_PSK, - SSL_R_BAD_EXTENSION); + /* SSLfatal() already called */ goto err; } diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index e6f4c34bf5..4bd94572bb 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -1152,7 +1152,7 @@ int tls_construct_client_hello(SSL *s, WPACKET *pkt) ERR_R_INTERNAL_ERROR); return 0; } - /* ssl_cipher_list_to_bytes() raises SSLerr if appropriate */ + if (!ssl_cipher_list_to_bytes(s, SSL_get_ciphers(s), pkt)) { /* SSLfatal() already called */ return 0; @@ -3643,8 +3643,9 @@ int tls_construct_end_of_early_data(SSL *s, WPACKET *pkt) { if (s->early_data_state != SSL_EARLY_DATA_WRITE_RETRY && s->early_data_state != SSL_EARLY_DATA_FINISHED_WRITING) { - SSLerr(SSL_F_TLS_CONSTRUCT_END_OF_EARLY_DATA, - ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); + SSLfatal(s, SSL_AD_INTERNAL_ERROR, + SSL_F_TLS_CONSTRUCT_END_OF_EARLY_DATA, + ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); return 0; } -- 2.39.2