Clean up ssl3_{send,get}_client_kex_gost()

Fix leaks, use sizeof() instead of hardcoded sizes, actually check return
codes, explicit_bzero() the premaster secret on the server side and
generally try to kick the GOST kex code into some sort of shape.

ok inoguchi@ tb@

2 files changed, 46 insertions, 41 deletions

diff --git a/lib/libssl/ssl_clnt.c b/lib/libssl/ssl_clnt.c
index cc66ed0473e..ca54515a329 100644
--- a/lib/libssl/ssl_clnt.c
+++ b/lib/libssl/ssl_clnt.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_clnt.c,v 1.131 2022/01/09 15:34:21 jsing Exp $ */
+/* $OpenBSD: ssl_clnt.c,v 1.132 2022/01/09 15:40:13 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -1925,40 +1925,44 @@ static int
 ssl3_send_client_kex_gost(SSL *s, CBB *cbb)
 {
 	unsigned char premaster_secret[32], shared_ukm[32], tmp[256];
-	EVP_PKEY *pub_key = NULL;
-	EVP_PKEY_CTX *pkey_ctx;
+	EVP_PKEY_CTX *pkey_ctx = NULL;
+	EVP_MD_CTX *ukm_hash = NULL;
+	EVP_PKEY *pub_key;
 	X509 *peer_cert;
 	size_t msglen;
 	unsigned int md_len;
-	EVP_MD_CTX *ukm_hash;
-	int nid;
 	CBB gostblob;
+	int nid;
 	int ret = 0;
 
 	/* Get server sertificate PKEY and create ctx from it */
 	peer_cert = s->session->peer_pkeys[SSL_PKEY_GOST01].x509;
-	if (peer_cert == NULL) {
+	if ((pub_key = X509_get0_pubkey(peer_cert)) == NULL) {
 		SSLerror(s, SSL_R_NO_GOST_CERTIFICATE_SENT_BY_PEER);
 		goto err;
 	}
-
-	pub_key = X509_get_pubkey(peer_cert);
-	pkey_ctx = EVP_PKEY_CTX_new(pub_key, NULL);
+	if ((pkey_ctx = EVP_PKEY_CTX_new(pub_key, NULL)) == NULL) {
+		SSLerror(s, ERR_R_MALLOC_FAILURE);
+		goto err;
+	}
 
 	/*
 	 * If we have send a certificate, and certificate key parameters match
 	 * those of server certificate, use certificate key for key exchange.
 	 * Otherwise, generate ephemeral key pair.
 	 */
-	EVP_PKEY_encrypt_init(pkey_ctx);
+	if (EVP_PKEY_encrypt_init(pkey_ctx) <= 0)
+		goto err;
 
 	/* Generate session key. */
-	arc4random_buf(premaster_secret, 32);
+	arc4random_buf(premaster_secret, sizeof(premaster_secret));
 
 	/*
 	 * If we have client certificate, use its secret as peer key.
+	 * XXX - this presumably lacks PFS.
 	 */
-	if (S3I(s)->hs.tls12.cert_request && s->cert->key->privatekey) {
+	if (S3I(s)->hs.tls12.cert_request != 0 &&
+	    s->cert->key->privatekey != NULL) {
 		if (EVP_PKEY_derive_set_peer(pkey_ctx,
 		    s->cert->key->privatekey) <=0) {
 			/*
@@ -1972,8 +1976,7 @@ ssl3_send_client_kex_gost(SSL *s, CBB *cbb)
 	/*
 	 * Compute shared IV and store it in algorithm-specific context data.
 	 */
-	ukm_hash = EVP_MD_CTX_new();
-	if (ukm_hash == NULL) {
+	if ((ukm_hash = EVP_MD_CTX_new()) == NULL) {
 		SSLerror(s, ERR_R_MALLOC_FAILURE);
 		goto err;
 	}
@@ -1985,10 +1988,12 @@ ssl3_send_client_kex_gost(SSL *s, CBB *cbb)
 		nid = NID_id_tc26_gost3411_2012_256;
 	if (!EVP_DigestInit(ukm_hash, EVP_get_digestbynid(nid)))
 		goto err;
-	EVP_DigestUpdate(ukm_hash, s->s3->client_random, SSL3_RANDOM_SIZE);
-	EVP_DigestUpdate(ukm_hash, s->s3->server_random, SSL3_RANDOM_SIZE);
-	EVP_DigestFinal_ex(ukm_hash, shared_ukm, &md_len);
-	EVP_MD_CTX_free(ukm_hash);
+	if (!EVP_DigestUpdate(ukm_hash, s->s3->client_random, SSL3_RANDOM_SIZE))
+		goto err;
+	if (!EVP_DigestUpdate(ukm_hash, s->s3->server_random, SSL3_RANDOM_SIZE))
+		goto err;
+	if (!EVP_DigestFinal_ex(ukm_hash, shared_ukm, &md_len))
+		goto err;
 	if (EVP_PKEY_CTX_ctrl(pkey_ctx, -1, EVP_PKEY_OP_ENCRYPT,
 	    EVP_PKEY_CTRL_SET_IV, 8, shared_ukm) < 0) {
 		SSLerror(s, SSL_R_LIBRARY_BUG);
@@ -2000,7 +2005,7 @@ ssl3_send_client_kex_gost(SSL *s, CBB *cbb)
 	 */
 	msglen = 255;
 	if (EVP_PKEY_encrypt(pkey_ctx, tmp, &msglen, premaster_secret,
-	    32) < 0) {
+	    sizeof(premaster_secret)) < 0) {
 		SSLerror(s, SSL_R_LIBRARY_BUG);
 		goto err;
 	}
@@ -2016,7 +2021,6 @@ ssl3_send_client_kex_gost(SSL *s, CBB *cbb)
 	if (EVP_PKEY_CTX_ctrl(pkey_ctx, -1, -1, EVP_PKEY_CTRL_PEER_KEY, 2,
 	    NULL) > 0)
 		s->s3->flags |= TLS1_FLAGS_SKIP_CERT_VERIFY;
-	EVP_PKEY_CTX_free(pkey_ctx);
 
 	if (!tls12_derive_master_secret(s, premaster_secret, 32))
 		goto err;
@@ -2025,7 +2029,8 @@ ssl3_send_client_kex_gost(SSL *s, CBB *cbb)
 
  err:
 	explicit_bzero(premaster_secret, sizeof(premaster_secret));
-	EVP_PKEY_free(pub_key);
+	EVP_PKEY_CTX_free(pkey_ctx);
+	EVP_MD_CTX_free(ukm_hash);
 
 	return ret;
 }
diff --git a/lib/libssl/ssl_srvr.c b/lib/libssl/ssl_srvr.c
index 1f6753fdf45..0979750e222 100644
--- a/lib/libssl/ssl_srvr.c
+++ b/lib/libssl/ssl_srvr.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_srvr.c,v 1.136 2022/01/09 15:34:21 jsing Exp $ */
+/* $OpenBSD: ssl_srvr.c,v 1.137 2022/01/09 15:40:13 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -1769,23 +1769,21 @@ ssl3_get_client_kex_ecdhe(SSL *s, CBS *cbs)
 static int
 ssl3_get_client_kex_gost(SSL *s, CBS *cbs)
 {
-	EVP_PKEY_CTX *pkey_ctx;
-	EVP_PKEY *client_pub_pkey = NULL, *pk = NULL;
 	unsigned char premaster_secret[32];
-	unsigned long alg_a;
-	size_t outlen = 32;
+	EVP_PKEY_CTX *pkey_ctx = NULL;
+	EVP_PKEY *client_pubkey;
+	EVP_PKEY *pkey = NULL;
+	size_t outlen;
 	CBS gostblob;
-	int al;
 
 	/* Get our certificate private key*/
-	alg_a = S3I(s)->hs.cipher->algorithm_auth;
-	if (alg_a & SSL_aGOST01)
-		pk = s->cert->pkeys[SSL_PKEY_GOST01].privatekey;
+	if ((S3I(s)->hs.cipher->algorithm_auth & SSL_aGOST01) != 0)
+		pkey = s->cert->pkeys[SSL_PKEY_GOST01].privatekey;
 
-	if ((pkey_ctx = EVP_PKEY_CTX_new(pk, NULL)) == NULL)
+	if ((pkey_ctx = EVP_PKEY_CTX_new(pkey, NULL)) == NULL)
 		goto err;
 	if (EVP_PKEY_decrypt_init(pkey_ctx) <= 0)
-		goto gerr;
+		goto err;
 
 	/*
 	 * If client certificate is present and is of the same type,
@@ -1794,9 +1792,8 @@ ssl3_get_client_kex_gost(SSL *s, CBS *cbs)
 	 * it is completely valid to use a client certificate for
 	 * authorization only.
 	 */
-	if ((client_pub_pkey = X509_get_pubkey(s->session->peer)) != NULL) {
-		if (EVP_PKEY_derive_set_peer(pkey_ctx,
-		    client_pub_pkey) <= 0)
+	if ((client_pubkey = X509_get0_pubkey(s->session->peer)) != NULL) {
+		if (EVP_PKEY_derive_set_peer(pkey_ctx, client_pubkey) <= 0)
 			ERR_clear_error();
 	}
 
@@ -1805,13 +1802,15 @@ ssl3_get_client_kex_gost(SSL *s, CBS *cbs)
 		goto decode_err;
 	if (CBS_len(cbs) != 0)
 		goto decode_err;
+	outlen = sizeof(premaster_secret);
 	if (EVP_PKEY_decrypt(pkey_ctx, premaster_secret, &outlen,
 	    CBS_data(&gostblob), CBS_len(&gostblob)) <= 0) {
 		SSLerror(s, SSL_R_DECRYPTION_FAILED);
-		goto gerr;
+		goto err;
 	}
 
-	if (!tls12_derive_master_secret(s, premaster_secret, 32))
+	if (!tls12_derive_master_secret(s, premaster_secret,
+	    sizeof(premaster_secret)))
 		goto err;
 
 	/* Check if pubkey from client certificate was used */
@@ -1819,17 +1818,18 @@ ssl3_get_client_kex_gost(SSL *s, CBS *cbs)
 	    2, NULL) > 0)
 		s->s3->flags |= TLS1_FLAGS_SKIP_CERT_VERIFY;
 
- gerr:
-	EVP_PKEY_free(client_pub_pkey);
+	explicit_bzero(premaster_secret, sizeof(premaster_secret));
 	EVP_PKEY_CTX_free(pkey_ctx);
 
 	return 1;
 
  decode_err:
-	al = SSL_AD_DECODE_ERROR;
 	SSLerror(s, SSL_R_BAD_PACKET_LENGTH);
-	ssl3_send_alert(s, SSL3_AL_FATAL, al);
+	ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
  err:
+	explicit_bzero(premaster_secret, sizeof(premaster_secret));
+	EVP_PKEY_CTX_free(pkey_ctx);
+
 	return 0;
 }