detect that a certificate chain will not fit in imsg calls before passing

part of it and failing others, this may leave the lookup process in a weird
state and cause use-after-free and out-of-bounds memory reads, leading to
crashes or potential arbitrary code execution in unprivileged process.

reported by Qualys Security

1 files changed, 77 insertions, 34 deletions

diff --git a/usr.sbin/smtpd/mta_session.c b/usr.sbin/smtpd/mta_session.c
index f45a141e972..4ad29d2bf4a 100644
--- a/usr.sbin/smtpd/mta_session.c
+++ b/usr.sbin/smtpd/mta_session.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: mta_session.c,v 1.71 2015/01/20 17:37:54 deraadt Exp $	*/
+/*	$OpenBSD: mta_session.c,v 1.72 2015/10/02 00:44:30 gilles Exp $	*/
 
 /*
  * Copyright (c) 2008 Pierre-Yves Ritschard <pyr@openbsd.org>
@@ -1520,12 +1520,28 @@ mta_start_tls(struct mta_session *s)
 static int
 mta_verify_certificate(struct mta_session *s)
 {
+#define MAX_CERTS	16
+#define MAX_CERT_LEN	(MAX_IMSGSIZE - (IMSG_HEADER_SIZE + sizeof(req_ca_vrfy)))
 	struct ca_vrfy_req_msg	req_ca_vrfy;
 	struct iovec		iov[2];
 	X509		       *x;
 	STACK_OF(X509)	       *xchain;
-	int			i;
 	const char	       *pkiname;
+	unsigned char	       *cert_der[MAX_CERTS];
+	int			cert_len[MAX_CERTS];
+	int			i, cert_count, res;
+
+	res = 0;
+	memset(cert_der, 0, sizeof(cert_der));
+	memset(&req_ca_vrfy, 0, sizeof req_ca_vrfy);
+
+	if (s->relay->pki_name)
+		pkiname = s->relay->pki_name;
+	else
+		pkiname = s->helo;
+	if (strlcpy(req_ca_vrfy.pkiname, pkiname, sizeof req_ca_vrfy.pkiname)
+	    >= sizeof req_ca_vrfy.pkiname)
+		return 0;
 
 	x = SSL_get_peer_certificate(s->io.ssl);
 	if (x == NULL)
@@ -1540,47 +1556,68 @@ mta_verify_certificate(struct mta_session *s)
 	 *
 	 */
 
+	cert_len[0] = i2d_X509(x, &cert_der[0]);
+	X509_free(x);
+
+	if (cert_len[0] < 0) {
+		log_warnx("warn: failed to encode certificate");	
+		goto end;
+	}
+	log_debug("debug: certificate 0: len=%d", cert_len[0]);
+	if (cert_len[0] > (int)MAX_CERT_LEN) {
+		log_warnx("warn: certificate too long");	
+		goto end;
+	}
+
+	if (xchain) {
+		cert_count = sk_X509_num(xchain);
+		log_debug("debug: certificate chain len: %d", cert_count);
+		if (cert_count >= MAX_CERTS) {
+			log_warnx("warn: certificate chain too long");
+			goto end;
+		}
+	}
+	else
+		cert_count = 0;
+
+	for (i = 0; i < cert_count; ++i) {
+		x = sk_X509_value(xchain, i);
+		cert_len[i+1] = i2d_X509(x, &cert_der[i+1]);
+		if (cert_len[i+1] < 0) {
+			log_warnx("warn: failed to encode certificate");	
+			goto end;
+		}
+		log_debug("debug: certificate %i: len=%d", i+1, cert_len[i+1]);
+		if (cert_len[i+1] > (int)MAX_CERT_LEN) {
+			log_warnx("warn: certificate too long");	
+			goto end;
+		}
+	}
+
 	tree_xset(&wait_ssl_verify, s->id, s);
 	s->flags |= MTA_WAIT;
 
 	/* Send the client certificate */
-	memset(&req_ca_vrfy, 0, sizeof req_ca_vrfy);
-	if (s->relay->pki_name)
-		pkiname = s->relay->pki_name;
-	else
-		pkiname = s->helo;
-	if (strlcpy(req_ca_vrfy.pkiname, pkiname, sizeof req_ca_vrfy.pkiname)
-	    >= sizeof req_ca_vrfy.pkiname)
-		return 0;
-
 	req_ca_vrfy.reqid = s->id;
-	req_ca_vrfy.cert_len = i2d_X509(x, &req_ca_vrfy.cert);
-	if (xchain)
-		req_ca_vrfy.n_chain = sk_X509_num(xchain);
+	req_ca_vrfy.cert_len = cert_len[0];
+	req_ca_vrfy.n_chain = cert_count;
 	iov[0].iov_base = &req_ca_vrfy;
 	iov[0].iov_len = sizeof(req_ca_vrfy);
-	iov[1].iov_base = req_ca_vrfy.cert;
-	iov[1].iov_len = req_ca_vrfy.cert_len;
+	iov[1].iov_base = cert_der[0];
+	iov[1].iov_len = cert_len[0];
 	m_composev(p_lka, IMSG_MTA_SSL_VERIFY_CERT, 0, 0, -1,
 	    iov, nitems(iov));
-	free(req_ca_vrfy.cert);
-	X509_free(x);
 
-	if (xchain) {		
-		/* Send the chain, one cert at a time */
-		for (i = 0; i < sk_X509_num(xchain); ++i) {
-			memset(&req_ca_vrfy, 0, sizeof req_ca_vrfy);
-			req_ca_vrfy.reqid = s->id;
-			x = sk_X509_value(xchain, i);
-			req_ca_vrfy.cert_len = i2d_X509(x, &req_ca_vrfy.cert);
-			iov[0].iov_base = &req_ca_vrfy;
-			iov[0].iov_len  = sizeof(req_ca_vrfy);
-			iov[1].iov_base = req_ca_vrfy.cert;
-			iov[1].iov_len  = req_ca_vrfy.cert_len;
-			m_composev(p_lka, IMSG_MTA_SSL_VERIFY_CHAIN, 0, 0, -1,
-			    iov, nitems(iov));
-			free(req_ca_vrfy.cert);
-		}
+	memset(&req_ca_vrfy, 0, sizeof req_ca_vrfy);
+	req_ca_vrfy.reqid = s->id;
+
+	/* Send the chain, one cert at a time */
+	for (i = 0; i < cert_count; ++i) {
+		req_ca_vrfy.cert_len = cert_len[i+1];
+		iov[1].iov_base = cert_der[i+1];
+		iov[1].iov_len  = cert_len[i+1];
+		m_composev(p_lka, IMSG_MTA_SSL_VERIFY_CHAIN, 0, 0, -1,
+		    iov, nitems(iov));
 	}
 
 	/* Tell lookup process that it can start verifying, we're done */
@@ -1589,7 +1626,13 @@ mta_verify_certificate(struct mta_session *s)
 	m_compose(p_lka, IMSG_MTA_SSL_VERIFY, 0, 0, -1,
 	    &req_ca_vrfy, sizeof req_ca_vrfy);
 
-	return 1;
+	res = 1;
+
+    end:
+	for (i = 0; i < MAX_CERTS; ++i)
+		free(cert_der[i]);
+
+	return res;
 }
 
 static const char *