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 <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/4778)
diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt
index 1220d3a..932fc46 100644
--- a/crypto/err/openssl.txt
+++ b/crypto/err/openssl.txt
@@ -1079,7 +1079,9 @@
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 6662f66..ef6b9dd 100644
--- a/include/openssl/sslerr.h
+++ b/include/openssl/sslerr.h
@@ -132,7 +132,9 @@
# 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 b9bef5a..7710ef0 100644
--- a/ssl/ssl_err.c
+++ b/ssl/ssl_err.c
@@ -180,8 +180,12 @@
{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 020d774..c812eef 100644
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -2266,8 +2266,7 @@
__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 f4aa98e..464a5ef 100644
--- a/ssl/statem/extensions.c
+++ b/ssl/statem/extensions.c
@@ -439,12 +439,11 @@
/*
* 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 @@
* 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 @@
* 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 @@
* 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 ca1cef5..c626ba6 100644
--- a/ssl/statem/extensions_srvr.c
+++ b/ssl/statem/extensions_srvr.c
@@ -400,8 +400,7 @@
/*
* 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 @@
/*
* 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 @@
/*
* 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 038fac9..48e6a0a 100644
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -1310,11 +1310,10 @@
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 @@
* 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 65c3aa3..b8e094b 100644
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -1740,11 +1740,10 @@
* @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 @@
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 @@
* 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 @@
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 @@
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 @@
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 @@
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 7e6aa94..0ba7f89 100644
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -1102,11 +1102,11 @@
}
#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 @@
* 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 @@
/*
* 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 @@
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 @@
#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;
}
}
