Convert legacy server kex to one-shot sign/verify

This converts ssl3_{get,send}_server_key_exchange() to EVP_DigestVerify()
and EVP_DigestSign(). In order to do this, build the full signed_params
up front and rework the way the key exchange parameters are constructed.
This way we can do the verify and sign steps in one go and at the same
use a more idiomatic approach with CBB/CBS.

with/ok jsing

2 files changed, 62 insertions, 47 deletions

diff --git a/src/lib/libssl/ssl_clnt.c b/src/lib/libssl/ssl_clnt.c
index 2ab90b5c37..6aea590132 100644
--- a/src/lib/libssl/ssl_clnt.c
+++ b/src/lib/libssl/ssl_clnt.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_clnt.c,v 1.159 2023/06/11 18:50:51 tb Exp $ */
+/* $OpenBSD: ssl_clnt.c,v 1.160 2023/06/11 19:01:01 tb Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -1299,13 +1299,17 @@ ssl3_get_server_kex_ecdhe(SSL *s, CBS *cbs)
 static int
 ssl3_get_server_key_exchange(SSL *s)
 {
-        CBS cbs, signature;
+        CBB cbb;
+        CBS cbs, params, signature;
         EVP_MD_CTX *md_ctx;
-        const unsigned char *param;
-        size_t param_len;
+        unsigned char *signed_params = NULL;
+        size_t signed_params_len;
+        size_t params_len;
         long alg_k, alg_a;
         int al, ret;
 
+        memset(&cbb, 0, sizeof(cbb));
+
         alg_k = s->s3->hs.cipher->algorithm_mkey;
         alg_a = s->s3->hs.cipher->algorithm_auth;
 
@@ -1341,8 +1345,14 @@ ssl3_get_server_key_exchange(SSL *s)
                 return (1);
         }
 
-        param = CBS_data(&cbs);
-        param_len = CBS_len(&cbs);
+        if (!CBB_init(&cbb, 0))
+                goto err;
+        if (!CBB_add_bytes(&cbb, s->s3->client_random, SSL3_RANDOM_SIZE))
+                goto err;
+        if (!CBB_add_bytes(&cbb, s->s3->server_random, SSL3_RANDOM_SIZE))
+                goto err;
+
+        CBS_dup(&cbs, &params);
 
         if (alg_k & SSL_kDHE) {
                 if (!ssl3_get_server_kex_dhe(s, &cbs))
@@ -1356,7 +1366,12 @@ ssl3_get_server_key_exchange(SSL *s)
                 goto fatal_err;
         }
 
-        param_len -= CBS_len(&cbs);
+        if ((params_len = CBS_offset(&cbs)) > CBS_len(&params))
+                goto err;
+        if (!CBB_add_bytes(&cbb, CBS_data(&params), params_len))
+                goto err;
+        if (!CBB_finish(&cbb, &signed_params, &signed_params_len))
+                goto err;
 
         /* if it was signed, check the signature */
         if ((alg_a & SSL_aNULL) == 0) {
@@ -1400,21 +1415,13 @@ ssl3_get_server_key_exchange(SSL *s)
                 if (!EVP_DigestVerifyInit(md_ctx, &pctx, sigalg->md(),
                     NULL, pkey))
                         goto err;
-                if (!EVP_DigestVerifyUpdate(md_ctx, s->s3->client_random,
-                    SSL3_RANDOM_SIZE))
-                        goto err;
                 if ((sigalg->flags & SIGALG_FLAG_RSA_PSS) &&
                     (!EVP_PKEY_CTX_set_rsa_padding(pctx,
                     RSA_PKCS1_PSS_PADDING) ||
                     !EVP_PKEY_CTX_set_rsa_pss_saltlen(pctx, -1)))
                         goto err;
-                if (!EVP_DigestVerifyUpdate(md_ctx, s->s3->server_random,
-                    SSL3_RANDOM_SIZE))
-                        goto err;
-                if (!EVP_DigestVerifyUpdate(md_ctx, param, param_len))
-                        goto err;
-                if (EVP_DigestVerifyFinal(md_ctx, CBS_data(&signature),
-                    CBS_len(&signature)) <= 0) {
+                if (EVP_DigestVerify(md_ctx, CBS_data(&signature),
+                    CBS_len(&signature), signed_params, signed_params_len) <= 0) {
                         al = SSL_AD_DECRYPT_ERROR;
                         SSLerror(s, SSL_R_BAD_SIGNATURE);
                         goto fatal_err;
@@ -1428,6 +1435,7 @@ ssl3_get_server_key_exchange(SSL *s)
         }
 
         EVP_MD_CTX_free(md_ctx);
+        free(signed_params);
 
         return (1);
 
@@ -1439,7 +1447,9 @@ ssl3_get_server_key_exchange(SSL *s)
         ssl3_send_alert(s, SSL3_AL_FATAL, al);
 
  err:
+        CBB_cleanup(&cbb);
         EVP_MD_CTX_free(md_ctx);
+        free(signed_params);
 
         return (-1);
 }
diff --git a/src/lib/libssl/ssl_srvr.c b/src/lib/libssl/ssl_srvr.c
index d0814a8455..8edbf77156 100644
--- a/src/lib/libssl/ssl_srvr.c
+++ b/src/lib/libssl/ssl_srvr.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_srvr.c,v 1.154 2023/06/11 18:50:51 tb Exp $ */
+/* $OpenBSD: ssl_srvr.c,v 1.155 2023/06/11 19:01:01 tb Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -1431,12 +1431,13 @@ ssl3_send_server_kex_ecdhe(SSL *s, CBB *cbb)
 static int
 ssl3_send_server_key_exchange(SSL *s)
 {
-        CBB cbb, cbb_params, cbb_signature, server_kex;
+        CBB cbb, cbb_signature, cbb_signed_params, server_kex;
+        CBS params;
         const struct ssl_sigalg *sigalg = NULL;
+        unsigned char *signed_params = NULL;
+        size_t signed_params_len;
         unsigned char *signature = NULL;
         size_t signature_len = 0;
-        unsigned char *params = NULL;
-        size_t params_len;
         const EVP_MD *md = NULL;
         unsigned long type;
         EVP_MD_CTX *md_ctx = NULL;
@@ -1445,7 +1446,7 @@ ssl3_send_server_key_exchange(SSL *s)
         int al;
 
         memset(&cbb, 0, sizeof(cbb));
-        memset(&cbb_params, 0, sizeof(cbb_params));
+        memset(&cbb_signed_params, 0, sizeof(cbb_signed_params));
 
         if ((md_ctx = EVP_MD_CTX_new()) == NULL)
                 goto err;
@@ -1456,15 +1457,26 @@ ssl3_send_server_key_exchange(SSL *s)
                     SSL3_MT_SERVER_KEY_EXCHANGE))
                         goto err;
 
-                if (!CBB_init(&cbb_params, 0))
+                if (!CBB_init(&cbb_signed_params, 0))
                         goto err;
 
+                if (!CBB_add_bytes(&cbb_signed_params, s->s3->client_random,
+                    SSL3_RANDOM_SIZE)) {
+                        SSLerror(s, ERR_R_INTERNAL_ERROR);
+                        goto err;
+                }
+                if (!CBB_add_bytes(&cbb_signed_params, s->s3->server_random,
+                    SSL3_RANDOM_SIZE)) {
+                        SSLerror(s, ERR_R_INTERNAL_ERROR);
+                        goto err;
+                }
+
                 type = s->s3->hs.cipher->algorithm_mkey;
                 if (type & SSL_kDHE) {
-                        if (!ssl3_send_server_kex_dhe(s, &cbb_params))
+                        if (!ssl3_send_server_kex_dhe(s, &cbb_signed_params))
                                 goto err;
                 } else if (type & SSL_kECDHE) {
-                        if (!ssl3_send_server_kex_ecdhe(s, &cbb_params))
+                        if (!ssl3_send_server_kex_ecdhe(s, &cbb_signed_params))
                                 goto err;
                 } else {
                         al = SSL_AD_HANDSHAKE_FAILURE;
@@ -1472,10 +1484,16 @@ ssl3_send_server_key_exchange(SSL *s)
                         goto fatal_err;
                 }
 
-                if (!CBB_finish(&cbb_params, &params, &params_len))
+                if (!CBB_finish(&cbb_signed_params, &signed_params,
+                    &signed_params_len))
+                        goto err;
+
+                CBS_init(&params, signed_params, signed_params_len);
+                if (!CBS_skip(&params, 2 * SSL3_RANDOM_SIZE))
                         goto err;
 
-                if (!CBB_add_bytes(&server_kex, params, params_len))
+                if (!CBB_add_bytes(&server_kex, CBS_data(&params),
+                    CBS_len(&params)))
                         goto err;
 
                 /* Add signature unless anonymous. */
@@ -1507,22 +1525,8 @@ ssl3_send_server_key_exchange(SSL *s)
                                 SSLerror(s, ERR_R_EVP_LIB);
                                 goto err;
                         }
-                        if (!EVP_DigestSignUpdate(md_ctx, s->s3->client_random,
-                            SSL3_RANDOM_SIZE)) {
-                                SSLerror(s, ERR_R_EVP_LIB);
-                                goto err;
-                        }
-                        if (!EVP_DigestSignUpdate(md_ctx, s->s3->server_random,
-                            SSL3_RANDOM_SIZE)) {
-                                SSLerror(s, ERR_R_EVP_LIB);
-                                goto err;
-                        }
-                        if (!EVP_DigestSignUpdate(md_ctx, params, params_len)) {
-                                SSLerror(s, ERR_R_EVP_LIB);
-                                goto err;
-                        }
-                        if (!EVP_DigestSignFinal(md_ctx, NULL, &signature_len) ||
-                            !signature_len) {
+                        if (!EVP_DigestSign(md_ctx, NULL, &signature_len,
+                            signed_params, signed_params_len)) {
                                 SSLerror(s, ERR_R_EVP_LIB);
                                 goto err;
                         }
@@ -1530,7 +1534,8 @@ ssl3_send_server_key_exchange(SSL *s)
                                 SSLerror(s, ERR_R_MALLOC_FAILURE);
                                 goto err;
                         }
-                        if (!EVP_DigestSignFinal(md_ctx, signature, &signature_len)) {
+                        if (!EVP_DigestSign(md_ctx, signature, &signature_len,
+                            signed_params, signed_params_len)) {
                                 SSLerror(s, ERR_R_EVP_LIB);
                                 goto err;
                         }
@@ -1550,19 +1555,19 @@ ssl3_send_server_key_exchange(SSL *s)
         }
 
         EVP_MD_CTX_free(md_ctx);
-        free(params);
         free(signature);
+        free(signed_params);
 
         return (ssl3_handshake_write(s));
 
  fatal_err:
         ssl3_send_alert(s, SSL3_AL_FATAL, al);
  err:
-        CBB_cleanup(&cbb_params);
+        CBB_cleanup(&cbb_signed_params);
         CBB_cleanup(&cbb);
         EVP_MD_CTX_free(md_ctx);
-        free(params);
         free(signature);
+        free(signed_params);
 
         return (-1);
 }