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 <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/3314)

diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index 919469faa09c486b4c79a0a99182e219cd541e29..d751502aba726fa2647c637d7c6e5f52ae8bc1c8 100644 (file)
--- 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);