From: Matt Caswell Date: Wed, 26 Apr 2017 08:08:00 +0000 (+0100) Subject: Don't overwrite the alert value if there is no alert to send X-Git-Tag: OpenSSL_1_1_1-pre1~1667 X-Git-Url: https://git.librecmc.org/?a=commitdiff_plain;h=bf846a6d47a0f94b9771ead5ce52786045e58f49;p=oweals%2Fopenssl.git Don't overwrite the alert value if there is no alert to send The function tls_early_post_process_client_hello() was overwriting the passed "al" parameter even if it was successful. The caller of that function, tls_post_process_client_hello(), sets "al" to a sensible default (HANDSHAKE_FAILURE), but this was being overwritten to be INTERNAL_ERROR. The result is a "no shared cipher" error (and probably other similar errors) were being reported back to the client with an incorrect INTERNAL_ERROR alert. Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/3314) --- diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index 919469faa0..d751502aba 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -1444,10 +1444,10 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt) return MSG_PROCESS_ERROR; } -static int tls_early_post_process_client_hello(SSL *s, int *al) +static int tls_early_post_process_client_hello(SSL *s, int *pal) { unsigned int j; - int i; + int i, al = SSL_AD_INTERNAL_ERROR; int protverr; size_t loop; unsigned long id; @@ -1460,13 +1460,12 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) CLIENTHELLO_MSG *clienthello = s->clienthello; DOWNGRADE dgrd = DOWNGRADE_NONE; - *al = SSL_AD_INTERNAL_ERROR; /* Finished parsing the ClientHello, now we can start processing it */ /* Give the early callback a crack at things */ if (s->ctx->early_cb != NULL) { int code; /* A failure in the early callback terminates the connection. */ - code = s->ctx->early_cb(s, al, s->ctx->early_cb_arg); + code = s->ctx->early_cb(s, &al, s->ctx->early_cb_arg); if (code == 0) goto err; if (code < 0) { @@ -1513,13 +1512,13 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) /* like ssl3_get_record, send alert using remote version number */ s->version = s->client_version = clienthello->legacy_version; } - *al = SSL_AD_PROTOCOL_VERSION; + al = SSL_AD_PROTOCOL_VERSION; goto err; } /* TLSv1.3 specifies that a ClientHello must end on a record boundary */ if (SSL_IS_TLS13(s) && RECORD_LAYER_processed_read_pending(&s->rlayer)) { - *al = SSL_AD_UNEXPECTED_MESSAGE; + al = SSL_AD_UNEXPECTED_MESSAGE; SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO, SSL_R_NOT_ON_RECORD_BOUNDARY); goto err; @@ -1531,7 +1530,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) if (s->ctx->app_verify_cookie_cb != NULL) { if (s->ctx->app_verify_cookie_cb(s, clienthello->dtls_cookie, clienthello->dtls_cookie_len) == 0) { - *al = SSL_AD_HANDSHAKE_FAILURE; + al = SSL_AD_HANDSHAKE_FAILURE; SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO, SSL_R_COOKIE_MISMATCH); goto err; @@ -1541,7 +1540,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) } else if (s->d1->cookie_len != clienthello->dtls_cookie_len || memcmp(clienthello->dtls_cookie, s->d1->cookie, s->d1->cookie_len) != 0) { - *al = SSL_AD_HANDSHAKE_FAILURE; + al = SSL_AD_HANDSHAKE_FAILURE; SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO, SSL_R_COOKIE_MISMATCH); goto err; } @@ -1552,7 +1551,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) if (protverr != 0) { SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO, protverr); s->version = s->client_version; - *al = SSL_AD_PROTOCOL_VERSION; + al = SSL_AD_PROTOCOL_VERSION; goto err; } } @@ -1563,7 +1562,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) /* We need to do this before getting the session */ if (!tls_parse_extension(s, TLSEXT_IDX_extended_master_secret, SSL_EXT_CLIENT_HELLO, - clienthello->pre_proc_exts, NULL, 0, al)) { + clienthello->pre_proc_exts, NULL, 0, &al)) { SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO, SSL_R_CLIENTHELLO_TLSEXT); goto err; } @@ -1590,7 +1589,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) if (!ssl_get_new_session(s, 1)) goto err; } else { - i = ssl_get_prev_session(s, clienthello, al); + i = ssl_get_prev_session(s, clienthello, &al); if (i == 1) { /* previous session */ s->hit = 1; @@ -1604,9 +1603,9 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) } if (!ssl_cache_cipherlist(s, &clienthello->ciphersuites, - clienthello->isv2, al) || + clienthello->isv2, &al) || !bytes_to_cipher_list(s, &clienthello->ciphersuites, &ciphers, &scsvs, - clienthello->isv2, al)) { + clienthello->isv2, &al)) { goto err; } @@ -1620,7 +1619,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) /* SCSV is fatal if renegotiating */ SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO, SSL_R_SCSV_RECEIVED_WHEN_RENEGOTIATING); - *al = SSL_AD_HANDSHAKE_FAILURE; + al = SSL_AD_HANDSHAKE_FAILURE; goto err; } s->s3->send_connection_binding = 1; @@ -1635,7 +1634,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) */ SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO, SSL_R_INAPPROPRIATE_FALLBACK); - *al = SSL_AD_INAPPROPRIATE_FALLBACK; + al = SSL_AD_INAPPROPRIATE_FALLBACK; goto err; } } @@ -1665,7 +1664,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) * we need to have the cipher in the cipher list if we are asked * to reuse it */ - *al = SSL_AD_ILLEGAL_PARAMETER; + al = SSL_AD_ILLEGAL_PARAMETER; SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO, SSL_R_REQUIRED_CIPHER_MISSING); goto err; @@ -1679,7 +1678,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) if (loop >= clienthello->compressions_len) { /* no compress */ - *al = SSL_AD_DECODE_ERROR; + al = SSL_AD_DECODE_ERROR; SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO, SSL_R_NO_COMPRESSION_SPECIFIED); goto err; } @@ -1691,7 +1690,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) /* TLS extensions */ if (!tls_parse_all_extensions(s, SSL_EXT_CLIENT_HELLO, - clienthello->pre_proc_exts, NULL, 0, al)) { + clienthello->pre_proc_exts, NULL, 0, &al)) { SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO, SSL_R_PARSE_TLSEXT); goto err; } @@ -1736,7 +1735,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) pref_cipher = ssl3_choose_cipher(s, s->session->ciphers, SSL_get_ciphers(s)); if (pref_cipher == NULL) { - *al = SSL_AD_HANDSHAKE_FAILURE; + al = SSL_AD_HANDSHAKE_FAILURE; SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO, SSL_R_NO_SHARED_CIPHER); goto err; } @@ -1786,7 +1785,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) break; } if (k >= clienthello->compressions_len) { - *al = SSL_AD_ILLEGAL_PARAMETER; + al = SSL_AD_ILLEGAL_PARAMETER; SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO, SSL_R_REQUIRED_COMPRESSION_ALGORITHM_MISSING); goto err; @@ -1836,7 +1835,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) sk_SSL_CIPHER_free(s->session->ciphers); s->session->ciphers = ciphers; if (ciphers == NULL) { - *al = SSL_AD_INTERNAL_ERROR; + al = SSL_AD_INTERNAL_ERROR; SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO, ERR_R_INTERNAL_ERROR); goto err; } @@ -1863,6 +1862,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) return 1; err: ossl_statem_set_error(s); + *pal = al; sk_SSL_CIPHER_free(ciphers); sk_SSL_CIPHER_free(scsvs);