let smtpd use user-provided Diffie-Hellman parameters for ephemeral key

exchange. if no DH parameters are found, fallback to builtin parameters
as was done until now.

since we now accept user-provided DH parameters, make smtpd more strict
and fatal() if the parameters are bogus.

bump the key size of the DH parameters from 512bits to 1024bits, it might
be bumped further after some more research.

thanks to mikeb@ for his suggestions

diff ok mikeb@ , man ok jmc@

5 files changed, 138 insertions, 71 deletions

diff --git a/usr.sbin/smtpd/smtp.c b/usr.sbin/smtpd/smtp.c
index c823e59589a..83e126c77ff 100644
--- a/usr.sbin/smtpd/smtp.c
+++ b/usr.sbin/smtpd/smtp.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: smtp.c,v 1.80 2011/03/09 20:59:22 gilles Exp $	*/
+/*	$OpenBSD: smtp.c,v 1.81 2011/03/15 19:24:55 gilles Exp $	*/
 
 /*
  * Copyright (c) 2008 Gilles Chehade <gilles@openbsd.org>
@@ -202,6 +202,13 @@ smtp_imsg(struct smtpd *env, struct imsgev *iev, struct imsg *imsg)
 			    ssl->ssl_cert_len);
 			if (ssl->ssl_key == NULL)
 				fatal(NULL);
+			if (ssl->ssl_dhparams_len) {
+				ssl->ssl_dhparams = strdup((char *)imsg->data
+				    + sizeof *ssl + ssl->ssl_cert_len +
+				    ssl->ssl_key_len);
+				if (ssl->ssl_dhparams == NULL)
+					fatal(NULL);
+			}
 			SPLAY_INSERT(ssltree, env->sc_ssl, ssl);
 			return;
 
diff --git a/usr.sbin/smtpd/smtpd.c b/usr.sbin/smtpd/smtpd.c
index 9d3926da9c4..bd3a27dafaf 100644
--- a/usr.sbin/smtpd/smtpd.c
+++ b/usr.sbin/smtpd/smtpd.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: smtpd.c,v 1.115 2010/11/28 14:35:58 gilles Exp $	*/
+/*	$OpenBSD: smtpd.c,v 1.116 2011/03/15 19:24:55 gilles Exp $	*/
 
 /*
  * Copyright (c) 2008 Gilles Chehade <gilles@openbsd.org>
@@ -222,7 +222,7 @@ parent_send_config_listeners(struct smtpd *env)
 {
 	struct listener		*l;
 	struct ssl		*s;
-	struct iovec		 iov[3];
+	struct iovec		 iov[4];
 	int			 opt;
 
 	log_debug("parent_send_config: configuring smtp");
@@ -239,6 +239,8 @@ parent_send_config_listeners(struct smtpd *env)
 		iov[1].iov_len = s->ssl_cert_len;
 		iov[2].iov_base = s->ssl_key;
 		iov[2].iov_len = s->ssl_key_len;
+		iov[3].iov_base = s->ssl_dhparams;
+		iov[3].iov_len = s->ssl_dhparams_len;
 
 		imsg_composev(&env->sc_ievs[PROC_SMTP]->ibuf,
 		    IMSG_CONF_SSL, 0, 0, -1, iov, nitems(iov));
@@ -265,7 +267,7 @@ void
 parent_send_config_client_certs(struct smtpd *env)
 {
 	struct ssl		*s;
-	struct iovec		 iov[3];
+	struct iovec		 iov[4];
 
 	log_debug("parent_send_config_client_certs: configuring smtp");
 	imsg_compose_event(env->sc_ievs[PROC_MTA], IMSG_CONF_START,
@@ -281,6 +283,8 @@ parent_send_config_client_certs(struct smtpd *env)
 		iov[1].iov_len = s->ssl_cert_len;
 		iov[2].iov_base = s->ssl_key;
 		iov[2].iov_len = s->ssl_key_len;
+		iov[3].iov_base = s->ssl_dhparams;
+		iov[3].iov_len = s->ssl_dhparams_len;
 
 		imsg_composev(&env->sc_ievs[PROC_MTA]->ibuf, IMSG_CONF_SSL,
 		    0, 0, -1, iov, nitems(iov));
diff --git a/usr.sbin/smtpd/smtpd.conf.5 b/usr.sbin/smtpd/smtpd.conf.5
index 114b59c4403..5f5c47f0603 100644
--- a/usr.sbin/smtpd/smtpd.conf.5
+++ b/usr.sbin/smtpd/smtpd.conf.5
@@ -1,4 +1,4 @@
-.\"	$OpenBSD: smtpd.conf.5,v 1.37 2010/12/18 22:25:24 jmc Exp $
+.\"	$OpenBSD: smtpd.conf.5,v 1.38 2011/03/15 19:24:55 gilles Exp $
 .\"
 .\" Copyright (c) 2008 Janne Johansson <jj@openbsd.org>
 .\" Copyright (c) 2009 Jacek Masiulaniec <jacekm@dobremiasto.net>
@@ -16,7 +16,7 @@
 .\" OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 .\"
 .\"
-.Dd $Mdocdate: December 18 2010 $
+.Dd $Mdocdate: March 15 2011 $
 .Dt SMTPD.CONF 5
 .Os
 .Sh NAME
@@ -118,18 +118,23 @@ If
 .Ic certificate
 is specified,
 a certificate
-.Ao Ar name Ac Ns .crt
-and key
+.Ao Ar name Ac Ns .crt ,
+a key
 .Ao Ar name Ac Ns .key
+and Diffie-Hellman parameters
+.Ao Ar name Ac Ns .dh
 are searched for.
 If no
 .Ic certificate
 is specified,
 the default interface name is instead used,
 for example
-.Pa fxp0.crt
+.Pa fxp0.crt ,
+.Pa fxp0.key ,
 and
-.Pa fxp0.key .
+.Pa fxp0.dh .
+If no DH parameters are provided, smtpd will use
+built-in parameters.
 Creation of certificates is documented in
 .Xr starttls 8 .
 .Pp
diff --git a/usr.sbin/smtpd/smtpd.h b/usr.sbin/smtpd/smtpd.h
index dffecd947bb..6b31d83727b 100644
--- a/usr.sbin/smtpd/smtpd.h
+++ b/usr.sbin/smtpd/smtpd.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: smtpd.h,v 1.201 2011/03/09 20:59:22 gilles Exp $	*/
+/*	$OpenBSD: smtpd.h,v 1.202 2011/03/15 19:24:55 gilles Exp $	*/
 
 /*
  * Copyright (c) 2008 Gilles Chehade <gilles@openbsd.org>
@@ -526,6 +526,8 @@ struct ssl {
 	off_t			 ssl_cert_len;
 	char			*ssl_key;
 	off_t			 ssl_key_len;
+	char			*ssl_dhparams;
+	off_t			 ssl_dhparams_len;
 	u_int8_t		 flags;
 };
 
diff --git a/usr.sbin/smtpd/ssl.c b/usr.sbin/smtpd/ssl.c
index d4c05574fb9..9f42764a6f0 100644
--- a/usr.sbin/smtpd/ssl.c
+++ b/usr.sbin/smtpd/ssl.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ssl.c,v 1.31 2010/11/28 13:56:43 gilles Exp $	*/
+/*	$OpenBSD: ssl.c,v 1.32 2011/03/15 19:24:55 gilles Exp $	*/
 
 /*
  * Copyright (c) 2008 Pierre-Yves Ritschard <pyr@openbsd.org>
@@ -57,37 +57,13 @@ SSL	*ssl_client_init(int, char *, size_t, char *, size_t);
 int	 ssl_buf_read(SSL *, struct ibuf_read *);
 int	 ssl_buf_write(SSL *, struct msgbuf *);
 
-DH	*get_dh512(void);
-void	 ssl_set_ephemeral_key_exchange(SSL_CTX *);
+DH	*get_dh1024(void);
+DH	*get_dh_from_memory(u_int8_t *, size_t);
+void	 ssl_set_ephemeral_key_exchange(SSL_CTX *, DH *);
 
 extern void	bufferevent_read_pressure_cb(struct evbuffer *, size_t,
 		    size_t, void *);
 
-/* From OpenSSL's documentation:
- *
- * If "strong" primes were used to generate the DH parameters, it is
- * not strictly necessary to generate a new key for each handshake
- * but it does improve forward secrecy.
- *
- * These are the parameters used by both sendmail and openssl's
- * s_server.
- *
- * -- gilles@
- */
-
-unsigned char dh512_p[] = {
-        0xDA,0x58,0x3C,0x16,0xD9,0x85,0x22,0x89,0xD0,0xE4,0xAF,0x75,
-        0x6F,0x4C,0xCA,0x92,0xDD,0x4B,0xE5,0x33,0xB8,0x04,0xFB,0x0F,
-        0xED,0x94,0xEF,0x9C,0x8A,0x44,0x03,0xED,0x57,0x46,0x50,0xD3,
-        0x69,0x99,0xDB,0x29,0xD7,0x76,0x27,0x6B,0xA2,0xD3,0xD4,0x12,
-        0xE2,0x18,0xF4,0xDD,0x1E,0x08,0x4C,0xF6,0xD8,0x00,0x3E,0x7C,
-        0x47,0x74,0xE8,0x33,    
-};
-
-unsigned char dh512_g[] = {
-        0x02,
-};
-
 void
 ssl_connect(int fd, short event, void *p)
 {
@@ -395,32 +371,42 @@ ssl_load_certfile(struct smtpd *env, const char *name, u_int8_t flags)
 	(void)strlcpy(s->ssl_name, key.ssl_name, sizeof(s->ssl_name));
 
 	if (! bsnprintf(certfile, sizeof(certfile),
-		"/etc/mail/certs/%s.crt", name)) {
-		free(s);
-		return (-1);
-	}
+		"/etc/mail/certs/%s.crt", name))
+		goto err;
 
-	if ((s->ssl_cert = ssl_load_file(certfile, &s->ssl_cert_len)) == NULL) {
-		free(s);
-		return (-1);
-	}
+	if ((s->ssl_cert = ssl_load_file(certfile, &s->ssl_cert_len)) == NULL)
+		goto err;
 
 	if (! bsnprintf(certfile, sizeof(certfile),
-		"/etc/mail/certs/%s.key", name)) {
-		free(s->ssl_cert);
-		free(s);
-		return -1;
-	}
+		"/etc/mail/certs/%s.key", name))
+		goto err;
 
-	if ((s->ssl_key = ssl_load_file(certfile, &s->ssl_key_len)) == NULL) {
-		free(s->ssl_cert);
-		free(s);
-		return (-1);
+	if ((s->ssl_key = ssl_load_file(certfile, &s->ssl_key_len)) == NULL)
+		goto err;
+
+	if (! bsnprintf(certfile, sizeof(certfile),
+		"/etc/mail/certs/%s.dh", name))
+		goto err;
+
+	if ((s->ssl_dhparams = ssl_load_file(certfile,
+		    &s->ssl_dhparams_len)) == NULL) {
+		log_warnx("no DH parameters found in %s", certfile);
+		log_warnx("using built-in DH parameters");
 	}
 
 	SPLAY_INSERT(ssltree, env->sc_ssl, s);
 
 	return (0);
+err:
+	if (s->ssl_cert != NULL)
+		free(s->ssl_cert);
+	if (s->ssl_key != NULL)
+		free(s->ssl_key);
+	if (s->ssl_dhparams != NULL)
+		free(s->ssl_dhparams);
+	if (s != NULL)
+		free(s);
+	return (-1);
 }
 
 void
@@ -440,6 +426,7 @@ void
 ssl_setup(struct smtpd *env, struct listener *l)
 {
 	struct ssl	key;
+	DH *dh;
 
 	if (!(l->flags & F_SSL))
 		return;
@@ -463,10 +450,18 @@ ssl_setup(struct smtpd *env, struct listener *l)
 	if (!SSL_CTX_check_private_key(l->ssl_ctx))
 		goto err;
 	if (!SSL_CTX_set_session_id_context(l->ssl_ctx,
-		(const unsigned char *)l->ssl_cert_name, strlen(l->ssl_cert_name) + 1))
+		(const unsigned char *)l->ssl_cert_name,
+		strlen(l->ssl_cert_name) + 1))
 		goto err;
 
-	ssl_set_ephemeral_key_exchange(l->ssl_ctx);
+
+
+	if (l->ssl->ssl_dhparams_len == 0)
+		dh = get_dh1024();
+	else
+		dh = get_dh_from_memory(l->ssl->ssl_dhparams,
+		    l->ssl->ssl_dhparams_len);
+	ssl_set_ephemeral_key_exchange(l->ssl_ctx, dh);
 
 	log_debug("ssl_setup: ssl setup finished for listener: %p", l);
 	return;
@@ -694,29 +689,83 @@ ssl_buf_write(SSL *s, struct msgbuf *msgbuf)
 	return SSL_get_error(s, ret);
 }
 
+/* From OpenSSL's documentation:
+ *
+ * If "strong" primes were used to generate the DH parameters, it is
+ * not strictly necessary to generate a new key for each handshake
+ * but it does improve forward secrecy.
+ *
+ * -- gilles@
+ */
 DH *
-get_dh512(void)
+get_dh1024(void)
 {
-        DH *dh;
-	
+	DH *dh;
+	unsigned char dh1024_p[] = {
+		0xAD,0x37,0xBB,0x26,0x75,0x01,0x27,0x75,
+		0x06,0xB5,0xE7,0x1E,0x1F,0x2B,0xBC,0x51,
+		0xC0,0xF4,0xEB,0x42,0x7A,0x2A,0x83,0x1E,
+		0xE8,0xD1,0xD8,0xCC,0x9E,0xE6,0x15,0x1D,
+		0x06,0x46,0x50,0x94,0xB9,0xEE,0xB6,0x89,
+		0xB7,0x3C,0xAC,0x07,0x5E,0x29,0x37,0xCC,
+		0x8F,0xDF,0x48,0x56,0x85,0x83,0x26,0x02,
+		0xB8,0xB6,0x63,0xAF,0x2D,0x4A,0x57,0x93,
+		0x6B,0x54,0xE1,0x8F,0x28,0x76,0x9C,0x5D,
+		0x90,0x65,0xD1,0x07,0xFE,0x5B,0x05,0x65,
+		0xDA,0xD2,0xE2,0xAF,0x23,0xCA,0x2F,0xD6,
+		0x4B,0xD2,0x04,0xFE,0xDF,0x21,0x2A,0xE1,
+		0xCD,0x1B,0x70,0x76,0xB3,0x51,0xA4,0xC9,
+		0x2B,0x68,0xE3,0xDD,0xCB,0x97,0xDA,0x59,
+		0x50,0x93,0xEE,0xDB,0xBF,0xC7,0xFA,0xA7,
+		0x47,0xC4,0x4D,0xF0,0xC6,0x09,0x4A,0x4B
+	};
+	unsigned char dh1024_g[] = {
+		0x02
+	};
+
         if ((dh = DH_new()) == NULL)
 		return NULL;
 
-        dh->p = BN_bin2bn(dh512_p, sizeof(dh512_p), NULL);
-        dh->g = BN_bin2bn(dh512_g, sizeof(dh512_g), NULL);
-        if (dh->p == NULL || dh->g == NULL)
-                return NULL;
+        dh->p = BN_bin2bn(dh1024_p, sizeof(dh1024_p), NULL);
+        dh->g = BN_bin2bn(dh1024_g, sizeof(dh1024_g), NULL);
+        if (dh->p == NULL || dh->g == NULL) {
+		DH_free(dh);
+		return NULL;
+	}
 
         return dh;
 }
 
-
-void
-ssl_set_ephemeral_key_exchange(SSL_CTX *ctx)
+DH *
+get_dh_from_memory(u_int8_t *params, size_t len)
 {
-	DH *dh;
+	BIO *mem;
+        DH *dh;
 
-	dh = get_dh512();
+	mem = BIO_new_mem_buf(params, len);
+	if (mem == NULL)
+		return NULL;
+	dh = PEM_read_bio_DHparams(mem, NULL, NULL, NULL);
+	if (dh == NULL)
+		goto err;
+        if (dh->p == NULL || dh->g == NULL)
+		goto err;
+	return dh;
+
+err:
+	if (mem != NULL)
+		BIO_free(mem);
 	if (dh != NULL)
-		SSL_CTX_set_tmp_dh(ctx, dh);
+		DH_free(dh);
+	return NULL;
+}
+
+
+void
+ssl_set_ephemeral_key_exchange(SSL_CTX *ctx, DH *dh)
+{
+	if (dh == NULL || !SSL_CTX_set_tmp_dh(ctx, dh)) {
+		ssl_error("ssl_set_ephemeral_key_exchange");
+		fatal("ssl_set_ephemeral_key_exchange: cannot set tmp dh");
+	}
 }