From 1ffc21a41f265a4910719c37b047e1583145350d Mon Sep 17 00:00:00 2001
From: jsing <>
Date: Sun, 3 Jul 2022 14:52:39 +0000
Subject: Simplify certificate list handling code in legacy client.

Tidy up CBS code and remove some unnecessary length checks. Use 'cert' and
'certs' for certificates, rather than 'x' and 'sk'.

ok tb@
---
 src/lib/libssl/ssl_clnt.c | 78 ++++++++++++++++++++---------------------------
 1 file changed, 33 insertions(+), 45 deletions(-)

(limited to 'src/lib/libssl')

diff --git a/src/lib/libssl/ssl_clnt.c b/src/lib/libssl/ssl_clnt.c
index 8fe416b74a..224aa1032f 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.150 2022/07/02 16:00:12 tb Exp $ */
+/* $OpenBSD: ssl_clnt.c,v 1.151 2022/07/03 14:52:39 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -1086,10 +1086,10 @@ ssl3_get_server_hello(SSL *s)
 int
 ssl3_get_server_certificate(SSL *s)
 {
-	CBS cbs, cert_list;
-	X509 *x = NULL;
-	const unsigned char *q;
-	STACK_OF(X509) *sk = NULL;
+	CBS cbs, cert_list, cert_data;
+	STACK_OF(X509) *certs = NULL;
+	X509 *cert = NULL;
+	const uint8_t *p;
 	EVP_PKEY *pkey;
 	int cert_type;
 	int al, ret;
@@ -1111,7 +1111,7 @@ ssl3_get_server_certificate(SSL *s)
 		goto fatal_err;
 	}
 
-	if ((sk = sk_X509_new_null()) == NULL) {
+	if ((certs = sk_X509_new_null()) == NULL) {
 		SSLerror(s, ERR_R_MALLOC_FAILURE);
 		goto err;
 	}
@@ -1120,47 +1120,37 @@ ssl3_get_server_certificate(SSL *s)
 		goto decode_err;
 
 	CBS_init(&cbs, s->internal->init_msg, s->internal->init_num);
-	if (CBS_len(&cbs) < 3)
-		goto decode_err;
 
-	if (!CBS_get_u24_length_prefixed(&cbs, &cert_list) ||
-	    CBS_len(&cbs) != 0) {
-		al = SSL_AD_DECODE_ERROR;
-		SSLerror(s, SSL_R_LENGTH_MISMATCH);
-		goto fatal_err;
-	}
+	if (!CBS_get_u24_length_prefixed(&cbs, &cert_list))
+		goto decode_err;
+	if (CBS_len(&cbs) != 0)
+		goto decode_err;
 
 	while (CBS_len(&cert_list) > 0) {
-		CBS cert;
-
-		if (CBS_len(&cert_list) < 3)
+		if (!CBS_get_u24_length_prefixed(&cert_list, &cert_data))
 			goto decode_err;
-		if (!CBS_get_u24_length_prefixed(&cert_list, &cert)) {
-			al = SSL_AD_DECODE_ERROR;
-			SSLerror(s, SSL_R_CERT_LENGTH_MISMATCH);
-			goto fatal_err;
-		}
-
-		q = CBS_data(&cert);
-		x = d2i_X509(NULL, &q, CBS_len(&cert));
-		if (x == NULL) {
+		p = CBS_data(&cert_data);
+		if ((cert = d2i_X509(NULL, &p, CBS_len(&cert_data))) == NULL) {
 			al = SSL_AD_BAD_CERTIFICATE;
 			SSLerror(s, ERR_R_ASN1_LIB);
 			goto fatal_err;
 		}
-		if (q != CBS_data(&cert) + CBS_len(&cert)) {
-			al = SSL_AD_DECODE_ERROR;
-			SSLerror(s, SSL_R_CERT_LENGTH_MISMATCH);
-			goto fatal_err;
-		}
-		if (!sk_X509_push(sk, x)) {
+		if (p != CBS_data(&cert_data) + CBS_len(&cert_data))
+			goto decode_err;
+		if (!sk_X509_push(certs, cert)) {
 			SSLerror(s, ERR_R_MALLOC_FAILURE);
 			goto err;
 		}
-		x = NULL;
+		cert = NULL;
+	}
+
+	/* A server must always provide a non-empty certificate list. */
+	if (sk_X509_num(certs) < 1) {
+		SSLerror(s, SSL_R_PEER_DID_NOT_RETURN_A_CERTIFICATE);
+		goto decode_err;
 	}
 
-	if (ssl_verify_cert_chain(s, sk) <= 0 &&
+	if (ssl_verify_cert_chain(s, certs) <= 0 &&
 	    s->verify_mode != SSL_VERIFY_NONE) {
 		al = ssl_verify_alarm_type(s->verify_result);
 		SSLerror(s, SSL_R_CERTIFICATE_VERIFY_FAILED);
@@ -1172,34 +1162,32 @@ ssl3_get_server_certificate(SSL *s)
 	 * Inconsistency alert: cert_chain does include the peer's
 	 * certificate, which we don't include in s3_srvr.c
 	 */
-	x = sk_X509_value(sk, 0);
+	cert = sk_X509_value(certs, 0);
+	X509_up_ref(cert);
 
-	if ((pkey = X509_get0_pubkey(x)) == NULL ||
+	if ((pkey = X509_get0_pubkey(cert)) == NULL ||
 	    EVP_PKEY_missing_parameters(pkey)) {
-		x = NULL;
 		al = SSL3_AL_FATAL;
 		SSLerror(s, SSL_R_UNABLE_TO_FIND_PUBLIC_KEY_PARAMETERS);
 		goto fatal_err;
 	}
 	if ((cert_type = ssl_cert_type(pkey)) < 0) {
-		x = NULL;
 		al = SSL3_AL_FATAL;
 		SSLerror(s, SSL_R_UNKNOWN_CERTIFICATE_TYPE);
 		goto fatal_err;
 	}
 
-	X509_up_ref(x);
 	X509_free(s->session->peer_cert);
-	s->session->peer_cert = x;
+	X509_up_ref(cert);
+	s->session->peer_cert = cert;
 	s->session->peer_cert_type = cert_type;
 
 	s->session->verify_result = s->verify_result;
 
 	sk_X509_pop_free(s->session->cert_chain, X509_free);
-	s->session->cert_chain = sk;
-	sk = NULL;
+	s->session->cert_chain = certs;
+	certs = NULL;
 
-	x = NULL;
 	ret = 1;
 
 	if (0) {
@@ -1211,8 +1199,8 @@ ssl3_get_server_certificate(SSL *s)
 		ssl3_send_alert(s, SSL3_AL_FATAL, al);
 	}
  err:
-	X509_free(x);
-	sk_X509_pop_free(sk, X509_free);
+	sk_X509_pop_free(certs, X509_free);
+	X509_free(cert);
 
 	return (ret);
 }
-- 
cgit v1.2.3-55-g6feb