From fb34a0f4e033246ef5f957bc57d2ebc904a519fc Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Tue, 16 May 2017 17:28:23 +0100 Subject: [PATCH] Try to be more consistent about the alerts we send We are quite inconsistent about which alerts get sent. Specifically, these alerts should be used (normally) in the following circumstances: SSL_AD_DECODE_ERROR = The peer sent a syntactically incorrect message SSL_AD_ILLEGAL_PARAMETER = The peer sent a message which was syntactically correct, but a parameter given is invalid for the context SSL_AD_HANDSHAKE_FAILURE = The peer's messages were syntactically and semantically correct, but the parameters provided were unacceptable to us (e.g. because we do not support the requested parameters) SSL_AD_INTERNAL_ERROR = We messed up (e.g. malloc failure) The standards themselves aren't always consistent but I think the above represents the best interpretation. Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/3480) --- ssl/record/ssl3_record.c | 6 +++--- ssl/ssl_lib.c | 6 +++--- ssl/ssl_rsa.c | 2 +- ssl/ssl_sess.c | 2 +- ssl/statem/extensions.c | 2 +- ssl/statem/extensions_clnt.c | 32 ++++++++++++++++++++++---------- ssl/statem/extensions_srvr.c | 16 ++++++++-------- ssl/statem/statem_clnt.c | 16 +++++++--------- ssl/statem/statem_dtls.c | 6 +++--- ssl/statem/statem_lib.c | 8 +++----- ssl/statem/statem_srvr.c | 35 +++++++++++++++++++---------------- ssl/t1_lib.c | 6 +++--- 12 files changed, 74 insertions(+), 63 deletions(-) diff --git a/ssl/record/ssl3_record.c b/ssl/record/ssl3_record.c index bafc976cde..85d726fb20 100644 --- a/ssl/record/ssl3_record.c +++ b/ssl/record/ssl3_record.c @@ -209,7 +209,7 @@ int ssl3_get_record(SSL *s) sslv2pkt = pkt; if (!PACKET_get_net_2_len(&sslv2pkt, &sslv2len) || !PACKET_get_1(&sslv2pkt, &type)) { - al = SSL_AD_INTERNAL_ERROR; + al = SSL_AD_DECODE_ERROR; SSLerr(SSL_F_SSL3_GET_RECORD, ERR_R_INTERNAL_ERROR); goto f_err; } @@ -241,7 +241,7 @@ int ssl3_get_record(SSL *s) } if (thisrr->length < MIN_SSL2_RECORD_LEN) { - al = SSL_AD_HANDSHAKE_FAILURE; + al = SSL_AD_DECODE_ERROR; SSLerr(SSL_F_SSL3_GET_RECORD, SSL_R_LENGTH_TOO_SHORT); goto f_err; } @@ -255,7 +255,7 @@ int ssl3_get_record(SSL *s) if (!PACKET_get_1(&pkt, &type) || !PACKET_get_net_2(&pkt, &version) || !PACKET_get_net_2_len(&pkt, &thisrr->length)) { - al = SSL_AD_INTERNAL_ERROR; + al = SSL_AD_DECODE_ERROR; SSLerr(SSL_F_SSL3_GET_RECORD, ERR_R_INTERNAL_ERROR); goto f_err; } diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index a12800d6d3..b81b9ea477 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -4753,7 +4753,7 @@ int ssl_cache_cipherlist(SSL *s, PACKET *cipher_suites, int sslv2format, TLS_CIPHER_LEN)) || (leadbyte != 0 && !PACKET_forward(&sslv2ciphers, TLS_CIPHER_LEN))) { - *al = SSL_AD_INTERNAL_ERROR; + *al = SSL_AD_DECODE_ERROR; OPENSSL_free(s->s3->tmp.ciphers_raw); s->s3->tmp.ciphers_raw = NULL; s->s3->tmp.ciphers_rawlen = 0; @@ -4840,8 +4840,8 @@ int bytes_to_cipher_list(SSL *s, PACKET *cipher_suites, } } if (PACKET_remaining(cipher_suites) > 0) { - *al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_BYTES_TO_CIPHER_LIST, ERR_R_INTERNAL_ERROR); + *al = SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_BYTES_TO_CIPHER_LIST, SSL_R_BAD_LENGTH); goto err; } diff --git a/ssl/ssl_rsa.c b/ssl/ssl_rsa.c index 6f1c380b9b..1ee80568ff 100644 --- a/ssl/ssl_rsa.c +++ b/ssl/ssl_rsa.c @@ -775,7 +775,7 @@ static int serverinfoex_srv_add_cb(SSL *s, unsigned int ext_type, int retval = serverinfo_find_extension(serverinfo, serverinfo_length, ext_type, out, outlen); if (retval == -1) { - *al = SSL_AD_DECODE_ERROR; + *al = SSL_AD_INTERNAL_ERROR; return -1; /* Error */ } if (retval == 0) diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c index 7a3d858c0a..5bef168abd 100644 --- a/ssl/ssl_sess.c +++ b/ssl/ssl_sess.c @@ -603,7 +603,7 @@ int ssl_get_prev_session(SSL *s, CLIENTHELLO_MSG *hello, int *al) /* If old session includes extms, but new does not: abort handshake */ if (!(s->s3->flags & TLS1_FLAGS_RECEIVED_EXTMS)) { SSLerr(SSL_F_SSL_GET_PREV_SESSION, SSL_R_INCONSISTENT_EXTMS); - ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE); + ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER); fatal = 1; goto err; } diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c index 68d8cea0bd..d77d4935e9 100644 --- a/ssl/statem/extensions.c +++ b/ssl/statem/extensions.c @@ -1116,7 +1116,7 @@ static int final_key_share(SSL *s, unsigned int context, int sent, int *al) && (!s->hit || (s->ext.psk_kex_mode & TLSEXT_KEX_MODE_FLAG_KE) == 0)) { /* Nothing left we can do - just fail */ - *al = SSL_AD_HANDSHAKE_FAILURE; + *al = SSL_AD_MISSING_EXTENSION; SSLerr(SSL_F_FINAL_KEY_SHARE, SSL_R_NO_SUITABLE_KEY_SHARE); return 0; } diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c index bbe94d0020..c5f8d3d1e5 100644 --- a/ssl/statem/extensions_clnt.c +++ b/ssl/statem/extensions_clnt.c @@ -930,7 +930,7 @@ int tls_parse_stoc_renegotiate(SSL *s, PACKET *pkt, unsigned int context, if (!PACKET_get_1_len(pkt, &ilen)) { SSLerr(SSL_F_TLS_PARSE_STOC_RENEGOTIATE, SSL_R_RENEGOTIATION_ENCODING_ERR); - *al = SSL_AD_ILLEGAL_PARAMETER; + *al = SSL_AD_DECODE_ERROR; return 0; } @@ -938,7 +938,7 @@ int tls_parse_stoc_renegotiate(SSL *s, PACKET *pkt, unsigned int context, if (PACKET_remaining(pkt) != ilen) { SSLerr(SSL_F_TLS_PARSE_STOC_RENEGOTIATE, SSL_R_RENEGOTIATION_ENCODING_ERR); - *al = SSL_AD_ILLEGAL_PARAMETER; + *al = SSL_AD_DECODE_ERROR; return 0; } @@ -946,7 +946,7 @@ int tls_parse_stoc_renegotiate(SSL *s, PACKET *pkt, unsigned int context, if (ilen != expected_len) { SSLerr(SSL_F_TLS_PARSE_STOC_RENEGOTIATE, SSL_R_RENEGOTIATION_MISMATCH); - *al = SSL_AD_HANDSHAKE_FAILURE; + *al = SSL_AD_ILLEGAL_PARAMETER; return 0; } @@ -955,7 +955,7 @@ int tls_parse_stoc_renegotiate(SSL *s, PACKET *pkt, unsigned int context, s->s3->previous_client_finished_len) != 0) { SSLerr(SSL_F_TLS_PARSE_STOC_RENEGOTIATE, SSL_R_RENEGOTIATION_MISMATCH); - *al = SSL_AD_HANDSHAKE_FAILURE; + *al = SSL_AD_ILLEGAL_PARAMETER; return 0; } @@ -975,8 +975,13 @@ int tls_parse_stoc_renegotiate(SSL *s, PACKET *pkt, unsigned int context, int tls_parse_stoc_server_name(SSL *s, PACKET *pkt, unsigned int context, X509 *x, size_t chainidx, int *al) { - if (s->ext.hostname == NULL || PACKET_remaining(pkt) > 0) { - *al = SSL_AD_UNRECOGNIZED_NAME; + if (s->ext.hostname == NULL) { + *al = SSL_AD_INTERNAL_ERROR; + return 0; + } + + if (PACKET_remaining(pkt) > 0) { + *al = SSL_AD_DECODE_ERROR; return 0; } @@ -1042,10 +1047,14 @@ int tls_parse_stoc_session_ticket(SSL *s, PACKET *pkt, unsigned int context, return 0; } - if (!tls_use_ticket(s) || PACKET_remaining(pkt) > 0) { + if (!tls_use_ticket(s)) { *al = SSL_AD_UNSUPPORTED_EXTENSION; return 0; } + if (PACKET_remaining(pkt) > 0) { + *al = SSL_AD_DECODE_ERROR; + return 0; + } s->ext.ticket_expected = 1; @@ -1060,11 +1069,14 @@ int tls_parse_stoc_status_request(SSL *s, PACKET *pkt, unsigned int context, * MUST only be sent if we've requested a status * request message. In TLS <= 1.2 it must also be empty. */ - if (s->ext.status_type != TLSEXT_STATUSTYPE_ocsp - || (!SSL_IS_TLS13(s) && PACKET_remaining(pkt) > 0)) { + if (s->ext.status_type != TLSEXT_STATUSTYPE_ocsp) { *al = SSL_AD_UNSUPPORTED_EXTENSION; return 0; } + if (!SSL_IS_TLS13(s) && PACKET_remaining(pkt) > 0) { + *al = SSL_AD_DECODE_ERROR; + return 0; + } if (SSL_IS_TLS13(s)) { /* We only know how to handle this if it's for the first Certificate in @@ -1407,7 +1419,7 @@ int tls_parse_stoc_key_share(SSL *s, PACKET *pkt, unsigned int context, X509 *x, } if (!EVP_PKEY_set1_tls_encodedpoint(skey, PACKET_data(&encoded_pt), PACKET_remaining(&encoded_pt))) { - *al = SSL_AD_DECODE_ERROR; + *al = SSL_AD_ILLEGAL_PARAMETER; SSLerr(SSL_F_TLS_PARSE_STOC_KEY_SHARE, SSL_R_BAD_ECPOINT); EVP_PKEY_free(skey); return 0; diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c index 9cde6955d3..95bacdffc4 100644 --- a/ssl/statem/extensions_srvr.c +++ b/ssl/statem/extensions_srvr.c @@ -25,7 +25,7 @@ int tls_parse_ctos_renegotiate(SSL *s, PACKET *pkt, unsigned int context, || !PACKET_get_bytes(pkt, &data, ilen)) { SSLerr(SSL_F_TLS_PARSE_CTOS_RENEGOTIATE, SSL_R_RENEGOTIATION_ENCODING_ERR); - *al = SSL_AD_ILLEGAL_PARAMETER; + *al = SSL_AD_DECODE_ERROR; return 0; } @@ -154,7 +154,7 @@ int tls_parse_ctos_srp(SSL *s, PACKET *pkt, unsigned int context, X509 *x, * upon resumption. Instead, we MUST ignore the login. */ if (!PACKET_strndup(&srp_I, &s->srp_ctx.login)) { - *al = TLS1_AD_INTERNAL_ERROR; + *al = SSL_AD_INTERNAL_ERROR; return 0; } @@ -178,7 +178,7 @@ int tls_parse_ctos_ec_pt_formats(SSL *s, PACKET *pkt, unsigned int context, if (!PACKET_memdup(&ec_point_format_list, &s->session->ext.ecpointformats, &s->session->ext.ecpointformats_len)) { - *al = TLS1_AD_INTERNAL_ERROR; + *al = SSL_AD_INTERNAL_ERROR; return 0; } } @@ -194,7 +194,7 @@ int tls_parse_ctos_session_ticket(SSL *s, PACKET *pkt, unsigned int context, !s->ext.session_ticket_cb(s, PACKET_data(pkt), PACKET_remaining(pkt), s->ext.session_ticket_cb_arg)) { - *al = TLS1_AD_INTERNAL_ERROR; + *al = SSL_AD_INTERNAL_ERROR; return 0; } @@ -213,7 +213,7 @@ int tls_parse_ctos_sig_algs(SSL *s, PACKET *pkt, unsigned int context, X509 *x, } if (!s->hit && !tls1_save_sigalgs(s, &supported_sig_algs)) { - *al = TLS1_AD_DECODE_ERROR; + *al = SSL_AD_DECODE_ERROR; return 0; } @@ -368,7 +368,7 @@ int tls_parse_ctos_alpn(SSL *s, PACKET *pkt, unsigned int context, X509 *x, s->s3->alpn_proposed_len = 0; if (!PACKET_memdup(&save_protocol_list, &s->s3->alpn_proposed, &s->s3->alpn_proposed_len)) { - *al = TLS1_AD_INTERNAL_ERROR; + *al = SSL_AD_INTERNAL_ERROR; return 0; } @@ -614,7 +614,7 @@ int tls_parse_ctos_key_share(SSL *s, PACKET *pkt, unsigned int context, X509 *x, if (!EVP_PKEY_set1_tls_encodedpoint(s->s3->peer_tmp, PACKET_data(&encoded_pt), PACKET_remaining(&encoded_pt))) { - *al = SSL_AD_DECODE_ERROR; + *al = SSL_AD_ILLEGAL_PARAMETER; SSLerr(SSL_F_TLS_PARSE_CTOS_KEY_SHARE, SSL_R_BAD_ECPOINT); return 0; } @@ -646,7 +646,7 @@ int tls_parse_ctos_supported_groups(SSL *s, PACKET *pkt, unsigned int context, if (!PACKET_memdup(&supported_groups_list, &s->session->ext.supportedgroups, &s->session->ext.supportedgroups_len)) { - *al = SSL_AD_DECODE_ERROR; + *al = SSL_AD_INTERNAL_ERROR; return 0; } diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index e6a0b35148..d4382e89cc 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -1183,7 +1183,6 @@ int tls_construct_client_hello(SSL *s, WPACKET *pkt) /* TLS extensions */ if (!tls_construct_extensions(s, pkt, SSL_EXT_CLIENT_HELLO, NULL, 0, &al)) { - ssl3_send_alert(s, SSL3_AL_FATAL, al); SSLerr(SSL_F_TLS_CONSTRUCT_CLIENT_HELLO, ERR_R_INTERNAL_ERROR); return 0; } @@ -1928,7 +1927,6 @@ static int tls_process_ske_srp(SSL *s, PACKET *pkt, EVP_PKEY **pkey, int *al) } if (!srp_verify_server_param(s, al)) { - *al = SSL_AD_DECODE_ERROR; SSLerr(SSL_F_TLS_PROCESS_SKE_SRP, SSL_R_BAD_SRP_PARAMETERS); return 0; } @@ -1987,7 +1985,7 @@ static int tls_process_ske_dhe(SSL *s, PACKET *pkt, EVP_PKEY **pkey, int *al) /* test non-zero pubkey */ if (BN_is_zero(bnpub_key)) { - *al = SSL_AD_DECODE_ERROR; + *al = SSL_AD_ILLEGAL_PARAMETER; SSLerr(SSL_F_TLS_PROCESS_SKE_DHE, SSL_R_BAD_DH_VALUE); goto err; } @@ -2000,7 +1998,7 @@ static int tls_process_ske_dhe(SSL *s, PACKET *pkt, EVP_PKEY **pkey, int *al) p = g = NULL; if (DH_check_params(dh, &check_bits) == 0 || check_bits != 0) { - *al = SSL_AD_DECODE_ERROR; + *al = SSL_AD_ILLEGAL_PARAMETER; SSLerr(SSL_F_TLS_PROCESS_SKE_DHE, SSL_R_BAD_DH_VALUE); goto err; } @@ -2075,7 +2073,7 @@ static int tls_process_ske_ecdhe(SSL *s, PACKET *pkt, EVP_PKEY **pkey, int *al) * invalid curve. ECParameters is 3 bytes. */ if (!tls1_check_curve(s, ecparams, 3)) { - *al = SSL_AD_DECODE_ERROR; + *al = SSL_AD_ILLEGAL_PARAMETER; SSLerr(SSL_F_TLS_PROCESS_SKE_ECDHE, SSL_R_WRONG_CURVE); return 0; } @@ -2124,7 +2122,7 @@ static int tls_process_ske_ecdhe(SSL *s, PACKET *pkt, EVP_PKEY **pkey, int *al) if (!EVP_PKEY_set1_tls_encodedpoint(s->s3->peer_tmp, PACKET_data(&encoded_pt), PACKET_remaining(&encoded_pt))) { - *al = SSL_AD_DECODE_ERROR; + *al = SSL_AD_ILLEGAL_PARAMETER; SSLerr(SSL_F_TLS_PROCESS_SKE_ECDHE, SSL_R_BAD_ECPOINT); return 0; } @@ -2201,7 +2199,7 @@ MSG_PROCESS_RETURN tls_process_key_exchange(SSL *s, PACKET *pkt) if (!PACKET_get_sub_packet(&save_param_start, ¶ms, PACKET_remaining(&save_param_start) - PACKET_remaining(pkt))) { - al = SSL_AD_INTERNAL_ERROR; + al = SSL_AD_DECODE_ERROR; SSLerr(SSL_F_TLS_PROCESS_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR); goto err; } @@ -2750,7 +2748,7 @@ static int tls_construct_cke_psk_preamble(SSL *s, WPACKET *pkt, int *al) identitylen = strlen(identity); if (identitylen > PSK_MAX_IDENTITY_LEN) { SSLerr(SSL_F_TLS_CONSTRUCT_CKE_PSK_PREAMBLE, ERR_R_INTERNAL_ERROR); - *al = SSL_AD_HANDSHAKE_FAILURE; + *al = SSL_AD_INTERNAL_ERROR; goto err; } @@ -3137,7 +3135,7 @@ int tls_construct_client_key_exchange(SSL *s, WPACKET *pkt) if (!tls_construct_cke_srp(s, pkt, &al)) goto err; } else if (!(alg_k & SSL_kPSK)) { - ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE); + ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR); SSLerr(SSL_F_TLS_CONSTRUCT_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR); goto err; } diff --git a/ssl/statem/statem_dtls.c b/ssl/statem/statem_dtls.c index b2ba35763a..5cab35544f 100644 --- a/ssl/statem/statem_dtls.c +++ b/ssl/statem/statem_dtls.c @@ -770,7 +770,7 @@ static int dtls_get_reassembled_message(SSL *s, int *errtype, size_t *len) * Fragments must not span records. */ if (frag_len > RECORD_LAYER_get_rrec_length(&s->rlayer)) { - al = SSL3_AD_ILLEGAL_PARAMETER; + al = SSL_AD_ILLEGAL_PARAMETER; SSLerr(SSL_F_DTLS_GET_REASSEMBLED_MESSAGE, SSL_R_BAD_LENGTH); goto f_err; } @@ -845,8 +845,8 @@ static int dtls_get_reassembled_message(SSL *s, int *errtype, size_t *len) * to fail */ if (readbytes != frag_len) { - al = SSL3_AD_ILLEGAL_PARAMETER; - SSLerr(SSL_F_DTLS_GET_REASSEMBLED_MESSAGE, SSL3_AD_ILLEGAL_PARAMETER); + al = SSL_AD_ILLEGAL_PARAMETER; + SSLerr(SSL_F_DTLS_GET_REASSEMBLED_MESSAGE, SSL_R_BAD_LENGTH); goto f_err; } diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index 4a399ca15c..e6f62937cb 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -333,10 +333,8 @@ MSG_PROCESS_RETURN tls_process_cert_verify(SSL *s, PACKET *pkt) peer = s->session->peer; pkey = X509_get0_pubkey(peer); - if (pkey == NULL) { - al = SSL_AD_INTERNAL_ERROR; + if (pkey == NULL) goto f_err; - } type = X509_certificate_type(peer, pkey); @@ -670,14 +668,14 @@ MSG_PROCESS_RETURN tls_process_change_cipher_spec(SSL *s, PACKET *pkt) && remain != DTLS1_CCS_HEADER_LENGTH + 1) || (s->version != DTLS1_BAD_VER && remain != DTLS1_CCS_HEADER_LENGTH - 1)) { - al = SSL_AD_ILLEGAL_PARAMETER; + al = SSL_AD_DECODE_ERROR; SSLerr(SSL_F_TLS_PROCESS_CHANGE_CIPHER_SPEC, SSL_R_BAD_CHANGE_CIPHER_SPEC); goto f_err; } } else { if (remain != 0) { - al = SSL_AD_ILLEGAL_PARAMETER; + al = SSL_AD_DECODE_ERROR; SSLerr(SSL_F_TLS_PROCESS_CHANGE_CIPHER_SPEC, SSL_R_BAD_CHANGE_CIPHER_SPEC); goto f_err; diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index 5c22ba7b1c..02c6e569d8 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -1260,7 +1260,7 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt) unsigned int mt; if (!SSL_IS_FIRST_HANDSHAKE(s) || s->hello_retry_request) { - al = SSL_AD_HANDSHAKE_FAILURE; + al = SSL_AD_UNEXPECTED_MESSAGE; SSLerr(SSL_F_TLS_PROCESS_CLIENT_HELLO, SSL_R_UNEXPECTED_MESSAGE); goto f_err; } @@ -1318,7 +1318,7 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt) } if (session_id_len > SSL_MAX_SSL_SESSION_ID_LENGTH) { - al = SSL_AD_DECODE_ERROR; + al = SSL_AD_ILLEGAL_PARAMETER; SSLerr(SSL_F_TLS_PROCESS_CLIENT_HELLO, SSL_R_LENGTH_MISMATCH); goto f_err; } @@ -1376,8 +1376,8 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt) if (!PACKET_copy_all(&cookie, clienthello->dtls_cookie, DTLS1_COOKIE_LENGTH, &clienthello->dtls_cookie_len)) { - al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_TLS_PROCESS_CLIENT_HELLO, SSL_R_LENGTH_MISMATCH); + al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_TLS_PROCESS_CLIENT_HELLO, ERR_R_INTERNAL_ERROR); goto f_err; } /* @@ -1419,8 +1419,8 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt) if (!PACKET_copy_all(&compression, clienthello->compressions, MAX_COMPRESSIONS_SIZE, &clienthello->compressions_len)) { - al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_TLS_PROCESS_CLIENT_HELLO, SSL_R_LENGTH_MISMATCH); + al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_TLS_PROCESS_CLIENT_HELLO, ERR_R_INTERNAL_ERROR); goto f_err; } @@ -2221,7 +2221,7 @@ int tls_construct_server_key_exchange(SSL *s, WPACKET *pkt) pkdhp = pkdh; } if (pkdhp == NULL) { - al = SSL_AD_HANDSHAKE_FAILURE; + al = SSL_AD_INTERNAL_ERROR; SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_KEY_EXCHANGE, SSL_R_MISSING_TMP_DH_KEY); goto f_err; @@ -2314,7 +2314,7 @@ int tls_construct_server_key_exchange(SSL *s, WPACKET *pkt) } else #endif { - al = SSL_AD_HANDSHAKE_FAILURE; + al = SSL_AD_INTERNAL_ERROR; SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_KEY_EXCHANGE, SSL_R_UNKNOWN_KEY_EXCHANGE_TYPE); goto f_err; @@ -2631,7 +2631,7 @@ static int tls_process_cke_rsa(SSL *s, PACKET *pkt, int *al) rsa = EVP_PKEY_get0_RSA(s->cert->pkeys[SSL_PKEY_RSA].privatekey); if (rsa == NULL) { - *al = SSL_AD_HANDSHAKE_FAILURE; + *al = SSL_AD_INTERNAL_ERROR; SSLerr(SSL_F_TLS_PROCESS_CKE_RSA, SSL_R_MISSING_RSA_CERTIFICATE); return 0; } @@ -2794,20 +2794,20 @@ static int tls_process_cke_dhe(SSL *s, PACKET *pkt, int *al) int ret = 0; if (!PACKET_get_net_2(pkt, &i) || PACKET_remaining(pkt) != i) { - *al = SSL_AD_HANDSHAKE_FAILURE; + *al = SSL_AD_DECODE_ERROR; SSLerr(SSL_F_TLS_PROCESS_CKE_DHE, SSL_R_DH_PUBLIC_VALUE_LENGTH_IS_WRONG); goto err; } skey = s->s3->tmp.pkey; if (skey == NULL) { - *al = SSL_AD_HANDSHAKE_FAILURE; + *al = SSL_AD_INTERNAL_ERROR; SSLerr(SSL_F_TLS_PROCESS_CKE_DHE, SSL_R_MISSING_TMP_DH_KEY); goto err; } if (PACKET_remaining(pkt) == 0L) { - *al = SSL_AD_HANDSHAKE_FAILURE; + *al = SSL_AD_DECODE_ERROR; SSLerr(SSL_F_TLS_PROCESS_CKE_DHE, SSL_R_MISSING_TMP_DH_KEY); goto err; } @@ -2886,7 +2886,7 @@ static int tls_process_cke_ecdhe(SSL *s, PACKET *pkt, int *al) goto err; } if (EVP_PKEY_set1_tls_encodedpoint(ckey, data, i) == 0) { - *al = SSL_AD_HANDSHAKE_FAILURE; + *al = SSL_AD_ILLEGAL_PARAMETER; SSLerr(SSL_F_TLS_PROCESS_CKE_ECDHE, ERR_R_EC_LIB); goto err; } @@ -2926,6 +2926,7 @@ static int tls_process_cke_srp(SSL *s, PACKET *pkt, int *al) return 0; } if ((s->srp_ctx.A = BN_bin2bn(data, i, NULL)) == NULL) { + *al = SSL_AD_INTERNAL_ERROR; SSLerr(SSL_F_TLS_PROCESS_CKE_SRP, ERR_R_BN_LIB); return 0; } @@ -3070,7 +3071,7 @@ MSG_PROCESS_RETURN tls_process_client_key_exchange(SSL *s, PACKET *pkt) if (alg_k & SSL_kPSK) { /* Identity extracted earlier: should be nothing left */ if (PACKET_remaining(pkt) != 0) { - al = SSL_AD_HANDSHAKE_FAILURE; + al = SSL_AD_DECODE_ERROR; SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, SSL_R_LENGTH_MISMATCH); goto err; @@ -3097,7 +3098,7 @@ MSG_PROCESS_RETURN tls_process_client_key_exchange(SSL *s, PACKET *pkt) if (!tls_process_cke_gost(s, pkt, &al)) goto err; } else { - al = SSL_AD_HANDSHAKE_FAILURE; + al = SSL_AD_INTERNAL_ERROR; SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, SSL_R_UNKNOWN_CIPHER_TYPE); goto err; @@ -3584,6 +3585,7 @@ MSG_PROCESS_RETURN tls_process_next_proto(SSL *s, PACKET *pkt) { PACKET next_proto, padding; size_t next_proto_len; + int al = SSL_AD_INTERNAL_ERROR; /*- * The payload looks like: @@ -3595,6 +3597,7 @@ MSG_PROCESS_RETURN tls_process_next_proto(SSL *s, PACKET *pkt) if (!PACKET_get_length_prefixed_1(pkt, &next_proto) || !PACKET_get_length_prefixed_1(pkt, &padding) || PACKET_remaining(pkt) > 0) { + al = SSL_AD_DECODE_ERROR; SSLerr(SSL_F_TLS_PROCESS_NEXT_PROTO, SSL_R_LENGTH_MISMATCH); goto err; } @@ -3608,6 +3611,7 @@ MSG_PROCESS_RETURN tls_process_next_proto(SSL *s, PACKET *pkt) return MSG_PROCESS_CONTINUE_READING; err: + ssl3_send_alert(s, SSL3_AL_FATAL, al); ossl_statem_set_error(s); return MSG_PROCESS_ERROR; } @@ -3621,7 +3625,6 @@ static int tls_construct_encrypted_extensions(SSL *s, WPACKET *pkt) NULL, 0, &al)) { ssl3_send_alert(s, SSL3_AL_FATAL, al); SSLerr(SSL_F_TLS_CONSTRUCT_ENCRYPTED_EXTENSIONS, ERR_R_INTERNAL_ERROR); - ssl3_send_alert(s, SSL3_AL_FATAL, al); return 0; } diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 0e1a97e175..232bb41fe0 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -1117,9 +1117,9 @@ int tls1_set_server_sigalgs(SSL *s) } if (s->cert->shared_sigalgs != NULL) return 1; - /* Fatal error is no shared signature algorithms */ + /* Fatal error if no shared signature algorithms */ SSLerr(SSL_F_TLS1_SET_SERVER_SIGALGS, SSL_R_NO_SHARED_SIGNATURE_ALGORITHMS); - al = SSL_AD_ILLEGAL_PARAMETER; + al = SSL_AD_HANDSHAKE_FAILURE; err: ssl3_send_alert(s, SSL3_AL_FATAL, al); return 0; @@ -2408,7 +2408,7 @@ int tls_choose_sigalg(SSL *s, int *al) if (al == NULL) return 1; SSLerr(SSL_F_TLS_CHOOSE_SIGALG, SSL_R_WRONG_SIGNATURE_TYPE); - *al = SSL_AD_HANDSHAKE_FAILURE; + *al = SSL_AD_ILLEGAL_PARAMETER; return 0; } } -- 2.34.1