Fix up a few places in the state machine that got missed with SSLfatal()

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/4778)

5 files changed, 50 insertions, 39 deletions

diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c
index 1bfa56328e..88e741e9fd 100644
--- a/ssl/ssl_err.c
+++ b/ssl/ssl_err.c
@@ -714,6 +714,8 @@ static const ERR_STRING_DATA SSL_str_reasons[] = {
     {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BAD_SSL_FILETYPE), "bad ssl filetype"},
     {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BAD_VALUE), "bad value"},
     {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BAD_WRITE_RETRY), "bad write retry"},
+    {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BINDER_DOES_NOT_VERIFY),
+    "binder does not verify"},
     {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BIO_NOT_SET), "bio not set"},
     {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BLOCK_CIPHER_PAD_IS_WRONG),
     "block cipher pad is wrong"},
diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c
index 062fc0438b..f4aa98e6d1 100644
--- a/ssl/statem/extensions.c
+++ b/ssl/statem/extensions.c
@@ -1296,7 +1296,8 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart,
     }
 
     if (sess->master_key_length != hashsize) {
-        SSLerr(SSL_F_TLS_PSK_DO_BINDER, SSL_R_BAD_PSK);
+        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PSK_DO_BINDER,
+                 SSL_R_BAD_PSK);
         goto err;
     }
 
@@ -1308,7 +1309,7 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart,
                                (const unsigned char *)nonce_label,
                                sizeof(nonce_label) - 1, sess->ext.tick_nonce,
                                sess->ext.tick_nonce_len, psk, hashsize)) {
-            SSLerr(SSL_F_TLS_PSK_DO_BINDER, ERR_R_INTERNAL_ERROR);
+            /* SSLfatal() already called */
             goto err;
         }
     }
@@ -1326,7 +1327,7 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart,
     else
         early_secret = (unsigned char *)sess->early_secret;
     if (!tls13_generate_secret(s, md, NULL, psk, hashsize, early_secret)) {
-        SSLerr(SSL_F_TLS_PSK_DO_BINDER, ERR_R_INTERNAL_ERROR);
+        /* SSLfatal() already called */
         goto err;
     }
 
@@ -1338,25 +1339,27 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart,
     if (mctx == NULL
             || EVP_DigestInit_ex(mctx, md, NULL) <= 0
             || EVP_DigestFinal_ex(mctx, hash, NULL) <= 0) {
-        SSLerr(SSL_F_TLS_PSK_DO_BINDER, ERR_R_INTERNAL_ERROR);
+        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PSK_DO_BINDER,
+                 ERR_R_INTERNAL_ERROR);
         goto err;
     }
 
     /* Generate the binder key */
     if (!tls13_hkdf_expand(s, md, early_secret, (unsigned char *)label,
                            labelsize, hash, hashsize, binderkey, hashsize)) {
-        SSLerr(SSL_F_TLS_PSK_DO_BINDER, ERR_R_INTERNAL_ERROR);
+        /* SSLfatal() already called */
         goto err;
     }
 
     /* Generate the finished key */
     if (!tls13_derive_finishedkey(s, md, binderkey, finishedkey, hashsize)) {
-        SSLerr(SSL_F_TLS_PSK_DO_BINDER, ERR_R_INTERNAL_ERROR);
+        /* SSLfatal() already called */
         goto err;
     }
 
     if (EVP_DigestInit_ex(mctx, md, NULL) <= 0) {
-        SSLerr(SSL_F_TLS_PSK_DO_BINDER, ERR_R_INTERNAL_ERROR);
+        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PSK_DO_BINDER,
+                 ERR_R_INTERNAL_ERROR);
         goto err;
     }
 
@@ -1371,7 +1374,8 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart,
 
         hdatalen = BIO_get_mem_data(s->s3->handshake_buffer, &hdata);
         if (hdatalen <= 0) {
-            SSLerr(SSL_F_TLS_PSK_DO_BINDER, SSL_R_BAD_HANDSHAKE_LENGTH);
+            SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PSK_DO_BINDER,
+                     SSL_R_BAD_HANDSHAKE_LENGTH);
             goto err;
         }
 
@@ -1388,27 +1392,31 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart,
                     || !PACKET_get_length_prefixed_3(&hashprefix, &msg)
                     || !PACKET_forward(&hashprefix, 1)
                     || !PACKET_get_length_prefixed_3(&hashprefix, &msg)) {
-                SSLerr(SSL_F_TLS_PSK_DO_BINDER, ERR_R_INTERNAL_ERROR);
+                SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PSK_DO_BINDER,
+                         ERR_R_INTERNAL_ERROR);
                 goto err;
             }
             hdatalen -= PACKET_remaining(&hashprefix);
         }
 
         if (EVP_DigestUpdate(mctx, hdata, hdatalen) <= 0) {
-            SSLerr(SSL_F_TLS_PSK_DO_BINDER, ERR_R_INTERNAL_ERROR);
+            SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PSK_DO_BINDER,
+                     ERR_R_INTERNAL_ERROR);
             goto err;
         }
     }
 
     if (EVP_DigestUpdate(mctx, msgstart, binderoffset) <= 0
             || EVP_DigestFinal_ex(mctx, hash, NULL) <= 0) {
-        SSLerr(SSL_F_TLS_PSK_DO_BINDER, ERR_R_INTERNAL_ERROR);
+        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PSK_DO_BINDER,
+                 ERR_R_INTERNAL_ERROR);
         goto err;
     }
 
     mackey = EVP_PKEY_new_mac_key(EVP_PKEY_HMAC, NULL, finishedkey, hashsize);
     if (mackey == NULL) {
-        SSLerr(SSL_F_TLS_PSK_DO_BINDER, ERR_R_INTERNAL_ERROR);
+        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PSK_DO_BINDER,
+                 ERR_R_INTERNAL_ERROR);
         goto err;
     }
 
@@ -1420,7 +1428,8 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart,
             || EVP_DigestSignUpdate(mctx, hash, hashsize) <= 0
             || EVP_DigestSignFinal(mctx, binderout, &bindersize) <= 0
             || bindersize != hashsize) {
-        SSLerr(SSL_F_TLS_PSK_DO_BINDER, ERR_R_INTERNAL_ERROR);
+        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PSK_DO_BINDER,
+                 ERR_R_INTERNAL_ERROR);
         goto err;
     }
 
@@ -1429,6 +1438,9 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart,
     } else {
         /* HMAC keys can't do EVP_DigestVerify* - use CRYPTO_memcmp instead */
         ret = (CRYPTO_memcmp(binderin, binderout, hashsize) == 0);
+        if (!ret)
+            SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, SSL_F_TLS_PSK_DO_BINDER,
+                     SSL_R_BINDER_DOES_NOT_VERIFY);
     }
 
  err:
diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c
index 326d77eb56..b7ef54e8b7 100644
--- a/ssl/statem/extensions_clnt.c
+++ b/ssl/statem/extensions_clnt.c
@@ -196,15 +196,17 @@ EXT_RETURN tls_construct_ctos_supported_groups(SSL *s, WPACKET *pkt,
 
         if (tls_curve_allowed(s, ctmp, SSL_SECOP_CURVE_SUPPORTED)) {
             if (!WPACKET_put_bytes_u16(pkt, ctmp)) {
-                    SSLerr(SSL_F_TLS_CONSTRUCT_CTOS_SUPPORTED_GROUPS,
-                           ERR_R_INTERNAL_ERROR);
+                    SSLfatal(s, SSL_AD_INTERNAL_ERROR,
+                             SSL_F_TLS_CONSTRUCT_CTOS_SUPPORTED_GROUPS,
+                             ERR_R_INTERNAL_ERROR);
                     return EXT_RETURN_FAIL;
                 }
         }
     }
     if (!WPACKET_close(pkt) || !WPACKET_close(pkt)) {
-        SSLerr(SSL_F_TLS_CONSTRUCT_CTOS_SUPPORTED_GROUPS,
-               ERR_R_INTERNAL_ERROR);
+        SSLfatal(s, SSL_AD_INTERNAL_ERROR,
+                 SSL_F_TLS_CONSTRUCT_CTOS_SUPPORTED_GROUPS,
+                 ERR_R_INTERNAL_ERROR);
         return EXT_RETURN_FAIL;
     }
 
@@ -934,7 +936,6 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context,
     size_t reshashsize = 0, pskhashsize = 0, binderoffset, msglen;
     unsigned char *resbinder = NULL, *pskbinder = NULL, *msgstart = NULL;
     const EVP_MD *handmd = NULL, *mdres = NULL, *mdpsk = NULL;
-    EXT_RETURN ret = EXT_RETURN_FAIL;
     int dores = 0;
 
     s->session->ext.tick_identity = TLSEXT_PSK_BAD_IDENTITY;
@@ -961,7 +962,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context,
         if (s->session->cipher == NULL) {
             SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CTOS_PSK,
                      ERR_R_INTERNAL_ERROR);
-            goto err;
+            return EXT_RETURN_FAIL;
         }
         mdres = ssl_md(s->session->cipher->algorithm2);
         if (mdres == NULL) {
@@ -1033,7 +1034,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context,
              */
             SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CTOS_PSK,
                      SSL_R_BAD_PSK);
-            goto err;
+            return EXT_RETURN_FAIL;
         }
 
         if (s->hello_retry_request && mdpsk != handmd) {
@@ -1043,7 +1044,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context,
              */
             SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CTOS_PSK,
                      SSL_R_BAD_PSK);
-            goto err;
+            return EXT_RETURN_FAIL;
         }
 
         pskhashsize = EVP_MD_size(mdpsk);
@@ -1055,7 +1056,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context,
             || !WPACKET_start_sub_packet_u16(pkt)) {
         SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CTOS_PSK,
                  ERR_R_INTERNAL_ERROR);
-        goto err;
+        return EXT_RETURN_FAIL;
     }
 
     if (dores) {
@@ -1064,7 +1065,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context,
                 || !WPACKET_put_bytes_u32(pkt, agems)) {
             SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CTOS_PSK,
                      ERR_R_INTERNAL_ERROR);
-            goto err;
+            return EXT_RETURN_FAIL;
         }
     }
 
@@ -1074,7 +1075,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context,
                 || !WPACKET_put_bytes_u32(pkt, 0)) {
             SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CTOS_PSK,
                      ERR_R_INTERNAL_ERROR);
-            goto err;
+            return EXT_RETURN_FAIL;
         }
     }
 
@@ -1095,7 +1096,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context,
             || !WPACKET_fill_lengths(pkt)) {
         SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CTOS_PSK,
                  ERR_R_INTERNAL_ERROR);
-        goto err;
+        return EXT_RETURN_FAIL;
     }
 
     msgstart = WPACKET_get_curr(pkt) - msglen;
@@ -1103,17 +1104,15 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context,
     if (dores
             && tls_psk_do_binder(s, mdres, msgstart, binderoffset, NULL,
                                  resbinder, s->session, 1, 0) != 1) {
-        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CTOS_PSK,
-                 ERR_R_INTERNAL_ERROR);
-        goto err;
+        /* SSLfatal() already called */
+        return EXT_RETURN_FAIL;
     }
 
     if (s->psksession != NULL
             && tls_psk_do_binder(s, mdpsk, msgstart, binderoffset, NULL,
                                  pskbinder, s->psksession, 1, 1) != 1) {
-        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CTOS_PSK,
-                 ERR_R_INTERNAL_ERROR);
-        goto err;
+        /* SSLfatal() already called */
+        return EXT_RETURN_FAIL;
     }
 
     if (dores)
@@ -1121,9 +1120,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context,
     if (s->psksession != NULL)
         s->psksession->ext.tick_identity = (dores ? 1 : 0);
 
-    ret = EXT_RETURN_SENT;
- err:
-    return ret;
+    return EXT_RETURN_SENT;
 #else
     return EXT_RETURN_NOT_SENT;
 #endif
diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c
index 7adb555e44..ca1cef59a8 100644
--- a/ssl/statem/extensions_srvr.c
+++ b/ssl/statem/extensions_srvr.c
@@ -858,8 +858,7 @@ int tls_parse_ctos_psk(SSL *s, PACKET *pkt, unsigned int context, X509 *x,
                                  (const unsigned char *)s->init_buf->data,
                                  binderoffset, PACKET_data(&binder), NULL,
                                  sess, 0, ext) != 1) {
-        SSLfatal(s, SSL_AD_DECODE_ERROR, SSL_F_TLS_PARSE_CTOS_PSK,
-                 SSL_R_BAD_EXTENSION);
+        /* SSLfatal() already called */
         goto err;
     }
 
diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index e6f4c34bf5..4bd94572bb 100644
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -1152,7 +1152,7 @@ int tls_construct_client_hello(SSL *s, WPACKET *pkt)
                  ERR_R_INTERNAL_ERROR);
         return 0;
     }
-    /* ssl_cipher_list_to_bytes() raises SSLerr if appropriate */
+
     if (!ssl_cipher_list_to_bytes(s, SSL_get_ciphers(s), pkt)) {
         /* SSLfatal() already called */
         return 0;
@@ -3643,8 +3643,9 @@ int tls_construct_end_of_early_data(SSL *s, WPACKET *pkt)
 {
     if (s->early_data_state != SSL_EARLY_DATA_WRITE_RETRY
             && s->early_data_state != SSL_EARLY_DATA_FINISHED_WRITING) {
-        SSLerr(SSL_F_TLS_CONSTRUCT_END_OF_EARLY_DATA,
-               ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
+        SSLfatal(s, SSL_AD_INTERNAL_ERROR,
+                 SSL_F_TLS_CONSTRUCT_END_OF_EARLY_DATA,
+                 ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
         return 0;
     }