From 8c35f87075d7447937181b620b41dbddac5b799f Mon Sep 17 00:00:00 2001
From: jsing <jsing@openbsd.org>
Date: Sun, 3 Jul 2022 14:52:39 +0000
Subject: [PATCH] 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@
---
 lib/libssl/ssl_clnt.c | 78 ++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 45 deletions(-)

diff --git a/lib/libssl/ssl_clnt.c b/lib/libssl/ssl_clnt.c
index 8fe416b74a5..224aa1032fb 100644
--- a/lib/libssl/ssl_clnt.c
+++ b/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);
 }
-- 
2.20.1