From 29bfd5b79a1ec2be8bea307ffb50810fce11915c Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Thu, 23 Nov 2017 13:11:42 +0000 Subject: [PATCH] Add some more cleanups Follow up from the conversion to use SSLfatal() in the state machine to clean things up a bit more. [extended tests] Reviewed-by: Richard Levitte (Merged from https://github.com/openssl/openssl/pull/4778) --- crypto/err/openssl.txt | 2 ++ include/openssl/sslerr.h | 2 ++ ssl/ssl_err.c | 4 ++++ ssl/ssl_locl.h | 3 +-- ssl/statem/extensions.c | 22 ++++++++---------- ssl/statem/extensions_srvr.c | 5 +--- ssl/statem/statem_clnt.c | 9 +++----- ssl/statem/statem_lib.c | 42 +++++++++++++++++++-------------- ssl/statem/statem_srvr.c | 45 ++++++++++++++++++------------------ 9 files changed, 70 insertions(+), 64 deletions(-) diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt index 1220d3a8ee..932fc466aa 100644 --- a/crypto/err/openssl.txt +++ b/crypto/err/openssl.txt @@ -1079,7 +1079,9 @@ SSL_F_SSL_CERT_NEW:162:ssl_cert_new SSL_F_SSL_CERT_SET0_CHAIN:340:ssl_cert_set0_chain SSL_F_SSL_CHECK_PRIVATE_KEY:163:SSL_check_private_key SSL_F_SSL_CHECK_SERVERHELLO_TLSEXT:280:* +SSL_F_SSL_CHECK_SRP_EXT_CLIENTHELLO:606:ssl_check_srp_ext_ClientHello SSL_F_SSL_CHECK_SRVR_ECC_CERT_AND_ALG:279:ssl_check_srvr_ecc_cert_and_alg +SSL_F_SSL_CHOOSE_CLIENT_VERSION:607:ssl_choose_client_version SSL_F_SSL_CIPHER_LIST_TO_BYTES:425:ssl_cipher_list_to_bytes SSL_F_SSL_CIPHER_PROCESS_RULESTR:230:ssl_cipher_process_rulestr SSL_F_SSL_CIPHER_STRENGTH_SORT:231:ssl_cipher_strength_sort diff --git a/include/openssl/sslerr.h b/include/openssl/sslerr.h index 6662f667e6..ef6b9dd403 100644 --- a/include/openssl/sslerr.h +++ b/include/openssl/sslerr.h @@ -132,7 +132,9 @@ int ERR_load_SSL_strings(void); # define SSL_F_SSL_CERT_SET0_CHAIN 340 # define SSL_F_SSL_CHECK_PRIVATE_KEY 163 # define SSL_F_SSL_CHECK_SERVERHELLO_TLSEXT 280 +# define SSL_F_SSL_CHECK_SRP_EXT_CLIENTHELLO 606 # define SSL_F_SSL_CHECK_SRVR_ECC_CERT_AND_ALG 279 +# define SSL_F_SSL_CHOOSE_CLIENT_VERSION 607 # define SSL_F_SSL_CIPHER_LIST_TO_BYTES 425 # define SSL_F_SSL_CIPHER_PROCESS_RULESTR 230 # define SSL_F_SSL_CIPHER_STRENGTH_SORT 231 diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c index b9bef5aa95..7710ef0fd8 100644 --- a/ssl/ssl_err.c +++ b/ssl/ssl_err.c @@ -180,8 +180,12 @@ static const ERR_STRING_DATA SSL_str_functs[] = { {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_CHECK_PRIVATE_KEY, 0), "SSL_check_private_key"}, {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_CHECK_SERVERHELLO_TLSEXT, 0), ""}, + {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_CHECK_SRP_EXT_CLIENTHELLO, 0), + "ssl_check_srp_ext_ClientHello"}, {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_CHECK_SRVR_ECC_CERT_AND_ALG, 0), "ssl_check_srvr_ecc_cert_and_alg"}, + {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_CHOOSE_CLIENT_VERSION, 0), + "ssl_choose_client_version"}, {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_CIPHER_LIST_TO_BYTES, 0), "ssl_cipher_list_to_bytes"}, {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_CIPHER_PROCESS_RULESTR, 0), diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index 020d7748f6..c812eef4de 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -2266,8 +2266,7 @@ __owur int ssl_check_version_downgrade(SSL *s); __owur int ssl_set_version_bound(int method_version, int version, int *bound); __owur int ssl_choose_server_version(SSL *s, CLIENTHELLO_MSG *hello, DOWNGRADE *dgrd); -__owur int ssl_choose_client_version(SSL *s, int version, int checkdgrd, - int *al); +__owur int ssl_choose_client_version(SSL *s, int version, int checkdgrd); int ssl_get_min_max_version(const SSL *s, int *min_version, int *max_version); __owur long tls1_default_timeout(void); diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c index f4aa98e6d1..464a5ef5eb 100644 --- a/ssl/statem/extensions.c +++ b/ssl/statem/extensions.c @@ -439,12 +439,11 @@ int extension_is_relevant(SSL *s, unsigned int extctx, unsigned int thisctx) /* * Gather a list of all the extensions from the data in |packet]. |context| * tells us which message this extension is for. The raw extension data is - * stored in |*res| on success. In the event of an error the alert type to use - * is stored in |*al|. We don't actually process the content of the extensions - * yet, except to check their types. This function also runs the initialiser - * functions for all known extensions if |init| is nonzero (whether we have - * collected them or not). If successful the caller is responsible for freeing - * the contents of |*res|. + * stored in |*res| on success. We don't actually process the content of the + * extensions yet, except to check their types. This function also runs the + * initialiser functions for all known extensions if |init| is nonzero (whether + * we have collected them or not). If successful the caller is responsible for + * freeing the contents of |*res|. * * Per http://tools.ietf.org/html/rfc5246#section-7.4.1.4, there may not be * more than one extension of the same type in a ClientHello or ServerHello. @@ -579,9 +578,8 @@ int tls_collect_extensions(SSL *s, PACKET *packet, unsigned int context, * given |context| and the parser has not already been run. If this is for a * Certificate message, then we also provide the parser with the relevant * Certificate |x| and its position in the |chainidx| with 0 being the first - * Certificate. Returns 1 on success or 0 on failure. In the event of a failure - * |*al| is populated with a suitable alert code. If an extension is not present - * this counted as success. + * Certificate. Returns 1 on success or 0 on failure. If an extension is not + * present this counted as success. */ int tls_parse_extension(SSL *s, TLSEXT_INDEX idx, int context, RAW_EXTENSION *exts, X509 *x, size_t chainidx) @@ -631,8 +629,7 @@ int tls_parse_extension(SSL *s, TLSEXT_INDEX idx, int context, * finalisation for all extensions at the end if |fin| is nonzero, whether we * collected them or not. Returns 1 for success or 0 for failure. If we are * working on a Certificate message then we also pass the Certificate |x| and - * its position in the |chainidx|, with 0 being the first certificate. On - * failure, |*al| is populated with a suitable alert code. + * its position in the |chainidx|, with 0 being the first certificate. */ int tls_parse_all_extensions(SSL *s, int context, RAW_EXTENSION *exts, X509 *x, size_t chainidx, int fin) @@ -782,8 +779,7 @@ int tls_construct_extensions(SSL *s, WPACKET *pkt, unsigned int context, * Built in extension finalisation and initialisation functions. All initialise * or finalise the associated extension type for the given |context|. For * finalisers |sent| is set to 1 if we saw the extension during parsing, and 0 - * otherwise. These functions return 1 on success or 0 on failure. In the event - * of a failure then |*al| is populated with a suitable error code. + * otherwise. These functions return 1 on success or 0 on failure. */ static int final_renegotiate(SSL *s, unsigned int context, int sent) diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c index ca1cef59a8..c626ba61c7 100644 --- a/ssl/statem/extensions_srvr.c +++ b/ssl/statem/extensions_srvr.c @@ -400,8 +400,7 @@ int tls_parse_ctos_npn(SSL *s, PACKET *pkt, unsigned int context, X509 *x, /* * Save the ALPN extension in a ClientHello.|pkt| holds the contents of the ALPN - * extension, not including type and length. |al| is a pointer to the alert - * value to send in the event of a failure. Returns: 1 on success, 0 on error. + * extension, not including type and length. Returns: 1 on success, 0 on error. */ int tls_parse_ctos_alpn(SSL *s, PACKET *pkt, unsigned int context, X509 *x, size_t chainidx) @@ -523,7 +522,6 @@ int tls_parse_ctos_etm(SSL *s, PACKET *pkt, unsigned int context, X509 *x, /* * Process a psk_kex_modes extension received in the ClientHello. |pkt| contains * the raw PACKET data for the extension. Returns 1 on success or 0 on failure. - * If a failure occurs then |*al| is set to an appropriate alert value. */ int tls_parse_ctos_psk_kex_modes(SSL *s, PACKET *pkt, unsigned int context, X509 *x, size_t chainidx) @@ -554,7 +552,6 @@ int tls_parse_ctos_psk_kex_modes(SSL *s, PACKET *pkt, unsigned int context, /* * Process a key_share extension received in the ClientHello. |pkt| contains * the raw PACKET data for the extension. Returns 1 on success or 0 on failure. - * If a failure occurs then |*al| is set to an appropriate alert value. */ int tls_parse_ctos_key_share(SSL *s, PACKET *pkt, unsigned int context, X509 *x, size_t chainidx) diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index 038fac93c6..48e6a0a224 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -1310,11 +1310,10 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt) PACKET session_id, extpkt; size_t session_id_len; const unsigned char *cipherchars; - int al = SSL_AD_INTERNAL_ERROR; unsigned int compression; unsigned int sversion; unsigned int context; - int protverr, discard; + int discard; RAW_EXTENSION *extensions = NULL; #ifndef OPENSSL_NO_COMP SSL_COMP *comp; @@ -1338,10 +1337,8 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt) * Must be done after reading the random data so we can check for the * TLSv1.3 downgrade sentinels */ - protverr = ssl_choose_client_version(s, sversion, 1, &al); - if (protverr != 0) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PROCESS_SERVER_HELLO, - protverr); + if (!ssl_choose_client_version(s, sversion, 1)) { + /* SSLfatal() already called */ goto err; } diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index 65c3aa3374..b8e094bdc4 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -1740,11 +1740,10 @@ int ssl_choose_server_version(SSL *s, CLIENTHELLO_MSG *hello, DOWNGRADE *dgrd) * @s: client SSL handle. * @version: The proposed version from the server's HELLO. * @checkdgrd: Whether to check the downgrade sentinels in the server_random - * @al: Where to store any alert value that may be generated * - * Returns 0 on success or an SSL error reason number on failure. + * Returns 1 on success or 0 on error. */ -int ssl_choose_client_version(SSL *s, int version, int checkdgrd, int *al) +int ssl_choose_client_version(SSL *s, int version, int checkdgrd) { const version_info *vent; const version_info *table; @@ -1755,15 +1754,18 @@ int ssl_choose_client_version(SSL *s, int version, int checkdgrd, int *al) version = TLS1_3_VERSION; if (s->hello_retry_request && version != TLS1_3_VERSION) { - *al = SSL_AD_PROTOCOL_VERSION; - return SSL_R_WRONG_SSL_VERSION; + SSLfatal(s, SSL_AD_PROTOCOL_VERSION, SSL_F_SSL_CHOOSE_CLIENT_VERSION, + SSL_R_WRONG_SSL_VERSION); + return 0; } switch (s->method->version) { default: if (version != s->version) { - *al = SSL_AD_PROTOCOL_VERSION; - return SSL_R_WRONG_SSL_VERSION; + SSLfatal(s, SSL_AD_PROTOCOL_VERSION, + SSL_F_SSL_CHOOSE_CLIENT_VERSION, + SSL_R_WRONG_SSL_VERSION); + return 0; } /* * If this SSL handle is not from a version flexible method we don't @@ -1772,7 +1774,7 @@ int ssl_choose_client_version(SSL *s, int version, int checkdgrd, int *al) * versions they don't want. If not, then easy to fix, just return * ssl_method_error(s, s->method) */ - return 0; + return 1; case TLS_ANY_VERSION: table = tls_version_table; break; @@ -1795,8 +1797,9 @@ int ssl_choose_client_version(SSL *s, int version, int checkdgrd, int *al) err = ssl_method_error(s, method); if (err != 0) { if (version == vent->version) { - *al = SSL_AD_PROTOCOL_VERSION; - return err; + SSLfatal(s, SSL_AD_PROTOCOL_VERSION, + SSL_F_SSL_CHOOSE_CLIENT_VERSION, err); + return 0; } continue; @@ -1815,8 +1818,10 @@ int ssl_choose_client_version(SSL *s, int version, int checkdgrd, int *al) s->s3->server_random + SSL3_RANDOM_SIZE - sizeof(tls12downgrade), sizeof(tls12downgrade)) == 0) { - *al = SSL_AD_ILLEGAL_PARAMETER; - return SSL_R_INAPPROPRIATE_FALLBACK; + SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, + SSL_F_SSL_CHOOSE_CLIENT_VERSION, + SSL_R_INAPPROPRIATE_FALLBACK); + return 0; } } else if (!SSL_IS_DTLS(s) && version < TLS1_2_VERSION @@ -1825,8 +1830,10 @@ int ssl_choose_client_version(SSL *s, int version, int checkdgrd, int *al) s->s3->server_random + SSL3_RANDOM_SIZE - sizeof(tls11downgrade), sizeof(tls11downgrade)) == 0) { - *al = SSL_AD_ILLEGAL_PARAMETER; - return SSL_R_INAPPROPRIATE_FALLBACK; + SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, + SSL_F_SSL_CHOOSE_CLIENT_VERSION, + SSL_R_INAPPROPRIATE_FALLBACK); + return 0; } } } @@ -1834,11 +1841,12 @@ int ssl_choose_client_version(SSL *s, int version, int checkdgrd, int *al) s->method = method; s->version = version; - return 0; + return 1; } - *al = SSL_AD_PROTOCOL_VERSION; - return SSL_R_UNSUPPORTED_PROTOCOL; + SSLfatal(s, SSL_AD_PROTOCOL_VERSION, SSL_F_SSL_CHOOSE_CLIENT_VERSION, + SSL_R_UNSUPPORTED_PROTOCOL); + return 0; } /* diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index 7e6aa94169..0ba7f89e30 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -1102,11 +1102,11 @@ WORK_STATE ossl_statem_server_post_process_message(SSL *s, WORK_STATE wst) } #ifndef OPENSSL_NO_SRP -static int ssl_check_srp_ext_ClientHello(SSL *s, int *al) +/* Returns 1 on success, 0 for retryable error, -1 for fatal error */ +static int ssl_check_srp_ext_ClientHello(SSL *s) { - int ret = SSL_ERROR_NONE; - - *al = SSL_AD_UNRECOGNIZED_NAME; + int ret; + int al = SSL_AD_UNRECOGNIZED_NAME; if ((s->s3->tmp.new_cipher->algorithm_mkey & SSL_kSRP) && (s->srp_ctx.TLS_ext_srp_username_callback != NULL)) { @@ -1115,13 +1115,24 @@ static int ssl_check_srp_ext_ClientHello(SSL *s, int *al) * RFC 5054 says SHOULD reject, we do so if There is no srp * login name */ - ret = SSL3_AL_FATAL; - *al = SSL_AD_UNKNOWN_PSK_IDENTITY; + SSLfatal(s, SSL_AD_UNKNOWN_PSK_IDENTITY, + SSL_F_SSL_CHECK_SRP_EXT_CLIENTHELLO, + SSL_R_PSK_IDENTITY_NOT_FOUND); + return -1; } else { - ret = SSL_srp_server_param_with_username(s, al); + ret = SSL_srp_server_param_with_username(s, &al); + if (ret < 0) + return 0; + if (ret == SSL3_AL_FATAL) { + SSLfatal(s, al, SSL_F_SSL_CHECK_SRP_EXT_CLIENTHELLO, + al == SSL_AD_UNKNOWN_PSK_IDENTITY + ? SSL_R_PSK_IDENTITY_NOT_FOUND + : SSL_R_CLIENTHELLO_TLSEXT); + return -1; + } } } - return ret; + return 1; } #endif @@ -1986,7 +1997,7 @@ static int tls_handle_status_request(SSL *s) /* * Call the alpn_select callback if needed. Upon success, returns 1. - * Upon failure, returns 0 and sets |*al| to the appropriate fatal alert. + * Upon failure, returns 0. */ int tls_handle_alpn(SSL *s) { @@ -2058,7 +2069,6 @@ int tls_handle_alpn(SSL *s) WORK_STATE tls_post_process_client_hello(SSL *s, WORK_STATE wst) { - int al = SSL_AD_HANDSHAKE_FAILURE; const SSL_CIPHER *cipher; if (wst == WORK_MORE_A) { @@ -2158,24 +2168,15 @@ WORK_STATE tls_post_process_client_hello(SSL *s, WORK_STATE wst) #ifndef OPENSSL_NO_SRP if (wst == WORK_MORE_C) { int ret; - if ((ret = ssl_check_srp_ext_ClientHello(s, &al)) < 0) { + if ((ret = ssl_check_srp_ext_ClientHello(s)) == 0) { /* * callback indicates further work to be done */ s->rwstate = SSL_X509_LOOKUP; return WORK_MORE_C; } - if (ret != SSL_ERROR_NONE) { - /* - * This is not really an error but the only means to for - * a client to detect whether srp is supported. - */ - if (al != TLS1_AD_UNKNOWN_PSK_IDENTITY) - SSLfatal(s, al, SSL_F_TLS_POST_PROCESS_CLIENT_HELLO, - SSL_R_CLIENTHELLO_TLSEXT); - else - SSLfatal(s, al, SSL_F_TLS_POST_PROCESS_CLIENT_HELLO, - SSL_R_PSK_IDENTITY_NOT_FOUND); + if (ret < 0) { + /* SSLfatal() already called */ goto err; } }