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)

7 files changed, 66 insertions, 64 deletions

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;
         }
     }