Skip to content

Commit

Permalink
Try to be more consistent about the alerts we send
Browse files Browse the repository at this point in the history 
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 <rsalz@openssl.org>
(Merged from #3480)
  • Loading branch information
@mattcaswell
mattcaswell committed May 19, 2017
1 parent d8028b2 commit fb34a0f
Show file tree
Hide file tree
Showing 12 changed files with 74 additions and 63 deletions.
6 changes: 3 additions & 3 deletions ssl/record/ssl3_record.c
View file
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand Down
6 changes: 3 additions & 3 deletions ssl/ssl_lib.c
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}

Expand Down
2 changes: 1 addition & 1 deletion ssl/ssl_rsa.c
View file
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion ssl/ssl_sess.c
View file
Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion ssl/statem/extensions.c
View file
Expand Up @@ -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;
}
Expand Down
32 changes: 22 additions & 10 deletions ssl/statem/extensions_clnt.c
View file
Expand Up @@ -930,23 +930,23 @@ 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;
}

/* Consistency check */
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;
}

/* Check that the extension matches */
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;
}

Expand All @@ -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;
}

Expand All @@ -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;
}

Expand Down Expand Up @@ -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;

Expand All @@ -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
Expand Down Expand Up @@ -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;
Expand Down
16 changes: 8 additions & 8 deletions ssl/statem/extensions_srvr.c
View file
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand All @@ -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;
}
}
Expand All @@ -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;
}

Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}

Expand Down
16 changes: 7 additions & 9 deletions ssl/statem/statem_clnt.c
View file
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -2201,7 +2199,7 @@ MSG_PROCESS_RETURN tls_process_key_exchange(SSL *s, PACKET *pkt)
if (!PACKET_get_sub_packet(&save_param_start, &params,
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;
}
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}
Expand Down
6 changes: 3 additions & 3 deletions ssl/statem/statem_dtls.c
View file
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}

Expand Down
8 changes: 3 additions & 5 deletions ssl/statem/statem_lib.c
View file
Expand Up @@ -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);

Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit fb34a0f

Please sign in to comment.