From: jsing <jsing@openbsd.org>
Date: Sun, 3 Jul 2022 14:58:00 +0000 (+0000)
Subject: Simplify certificate list handling code in legacy server.
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=a5c907f4ff25c91efaf1cd597ddfc1eb2cc4ef01;p=openbsd

Simplify certificate list handling code in legacy server.

A client is required to send an empty list if it does not have a suitable
certificate - handle this case up front, rather than going through the
normal code path and ending up with an empty certificate list. This matches
what we do in the TLSv1.3 stack and will allow for ruther clean up (in
addition to making the code more readable).

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

ok tb@
---

diff --git a/lib/libssl/ssl_srvr.c b/lib/libssl/ssl_srvr.c
index 526d9e678b2..d665a568d1d 100644
--- a/lib/libssl/ssl_srvr.c
+++ b/lib/libssl/ssl_srvr.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_srvr.c,v 1.147 2022/07/02 16:00:12 tb Exp $ */
+/* $OpenBSD: ssl_srvr.c,v 1.148 2022/07/03 14:58:00 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -525,7 +525,7 @@ ssl3_accept(SSL *s)
 
 		case SSL3_ST_SR_CERT_A:
 		case SSL3_ST_SR_CERT_B:
-			if (s->s3->hs.tls12.cert_request) {
+			if (s->s3->hs.tls12.cert_request != 0) {
 				ret = ssl3_get_client_certificate(s);
 				if (ret <= 0)
 					goto end;
@@ -2156,11 +2156,11 @@ ssl3_get_cert_verify(SSL *s)
 int
 ssl3_get_client_certificate(SSL *s)
 {
-	CBS cbs, client_certs;
-	X509 *x = NULL;
-	const unsigned char *q;
-	STACK_OF(X509) *sk = NULL;
-	int i, al, ret;
+	CBS cbs, cert_list, cert_data;
+	STACK_OF(X509) *certs = NULL;
+	X509 *cert = NULL;
+	const uint8_t *p;
+	int al, ret;
 
 	if ((ret = ssl3_get_message(s, SSL3_ST_SR_CERT_A, SSL3_ST_SR_CERT_B,
 	    -1, s->internal->max_cert_list)) <= 0)
@@ -2175,13 +2175,8 @@ ssl3_get_client_certificate(SSL *s)
 			al = SSL_AD_HANDSHAKE_FAILURE;
 			goto fatal_err;
 		}
-		/*
-		 * If tls asked for a client cert,
-		 * the client must return a 0 list.
-		 */
-		if (s->s3->hs.tls12.cert_request) {
-			SSLerror(s, SSL_R_TLS_PEER_DID_NOT_RESPOND_WITH_CERTIFICATE_LIST
-			    );
+		if (s->s3->hs.tls12.cert_request != 0) {
+			SSLerror(s, SSL_R_TLS_PEER_DID_NOT_RESPOND_WITH_CERTIFICATE_LIST);
 			al = SSL_AD_UNEXPECTED_MESSAGE;
 			goto fatal_err;
 		}
@@ -2200,77 +2195,70 @@ ssl3_get_client_certificate(SSL *s)
 
 	CBS_init(&cbs, s->internal->init_msg, s->internal->init_num);
 
-	if ((sk = sk_X509_new_null()) == NULL) {
-		SSLerror(s, ERR_R_MALLOC_FAILURE);
-		goto err;
-	}
-
-	if (!CBS_get_u24_length_prefixed(&cbs, &client_certs) ||
-	    CBS_len(&cbs) != 0)
+	if (!CBS_get_u24_length_prefixed(&cbs, &cert_list))
+		goto decode_err;
+	if (CBS_len(&cbs) != 0)
 		goto decode_err;
 
-	while (CBS_len(&client_certs) > 0) {
-		CBS cert;
-
-		if (!CBS_get_u24_length_prefixed(&client_certs, &cert)) {
-			al = SSL_AD_DECODE_ERROR;
-			SSLerror(s, SSL_R_CERT_LENGTH_MISMATCH);
+	/*
+	 * A TLS client must send an empty certificate list, if no suitable
+	 * certificate is available (rather than omitting the Certificate
+	 * handshake message) - see RFC 5246 section 7.4.6.
+	 */
+	if (CBS_len(&cert_list) == 0) {
+		if ((s->verify_mode & SSL_VERIFY_PEER) &&
+		    (s->verify_mode & SSL_VERIFY_FAIL_IF_NO_PEER_CERT)) {
+			SSLerror(s, SSL_R_PEER_DID_NOT_RETURN_A_CERTIFICATE);
+			al = SSL_AD_HANDSHAKE_FAILURE;
 			goto fatal_err;
 		}
+		/* No client certificate so free transcript. */
+		tls1_transcript_free(s);
+		goto done;
+	}
 
-		q = CBS_data(&cert);
-		x = d2i_X509(NULL, &q, CBS_len(&cert));
-		if (x == NULL) {
+	if ((certs = sk_X509_new_null()) == NULL) {
+		SSLerror(s, ERR_R_MALLOC_FAILURE);
+		goto err;
+	}
+
+	while (CBS_len(&cert_list) > 0) {
+		if (!CBS_get_u24_length_prefixed(&cert_list, &cert_data))
+			goto decode_err;
+		p = CBS_data(&cert_data);
+		if ((cert = d2i_X509(NULL, &p, CBS_len(&cert_data))) == NULL) {
 			SSLerror(s, ERR_R_ASN1_LIB);
 			goto 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;
 	}
 
-	if (sk_X509_num(sk) <= 0) {
-		/*
-		 * TLS does not mind 0 certs returned.
-		 * Fail for TLS only if we required a certificate.
-		 */
-		if ((s->verify_mode & SSL_VERIFY_PEER) &&
-		    (s->verify_mode & SSL_VERIFY_FAIL_IF_NO_PEER_CERT)) {
-			SSLerror(s, SSL_R_PEER_DID_NOT_RETURN_A_CERTIFICATE);
-			al = SSL_AD_HANDSHAKE_FAILURE;
-			goto fatal_err;
-		}
-		/* No client certificate so free transcript. */
-		tls1_transcript_free(s);
-	} else {
-		i = ssl_verify_cert_chain(s, sk);
-		if (i <= 0) {
-			al = ssl_verify_alarm_type(s->verify_result);
-			SSLerror(s, SSL_R_NO_CERTIFICATE_RETURNED);
-			goto fatal_err;
-		}
+	if (ssl_verify_cert_chain(s, certs) <= 0) {
+		al = ssl_verify_alarm_type(s->verify_result);
+		SSLerror(s, SSL_R_NO_CERTIFICATE_RETURNED);
+		goto fatal_err;
 	}
 
 	X509_free(s->session->peer_cert);
-	s->session->peer_cert = sk_X509_shift(sk);
+	s->session->peer_cert = sk_X509_shift(certs);
 
 	/*
 	 * Inconsistency alert: cert_chain does *not* include the
 	 * peer's own certificate, while we do include it in s3_clnt.c
 	 */
 	sk_X509_pop_free(s->session->cert_chain, X509_free);
-	s->session->cert_chain = sk;
-	sk = NULL;
+	s->session->cert_chain = certs;
+	certs = NULL;
 
 	s->session->verify_result = s->verify_result;
 
+ done:
 	ret = 1;
 	if (0) {
  decode_err:
@@ -2280,8 +2268,8 @@ ssl3_get_client_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);
 }