fix bad logic in SNI handling causing race condition that can lead to…

… a crash
OpenSMTPD · Apr 16, 2015 · 8033379 · 8033379
1 parent 1717627
commit 8033379
Show file tree

Hide file tree

Showing 3 changed files with 20 additions and 10 deletions.
diff --git a/smtpd/smtp_session.c b/smtpd/smtp_session.c
@@ -193,6 +193,7 @@ static uint8_t dsn_notify_str_to_uint8(const char *);
 static void smtp_auth_failure_pause(struct smtp_session *);
 static void smtp_auth_failure_resume(int, short, void *);
 static int smtp_sni_callback(SSL *, int *, void *);
+static const char * smtp_sni_get_servername(struct smtp_session *);
 
 static void smtp_filter_connect(struct smtp_session *, struct sockaddr *);
 static void smtp_filter_rset(struct smtp_session *);
@@ -1043,7 +1044,7 @@ smtp_session_imsg(struct mproc *p, struct imsg *imsg)
 		else
 			pkiname = s->smtpname;
 		ssl_ctx = dict_get(env->sc_ssl_dict, pkiname);
-		ssl = ssl_smtp_init(ssl_ctx, smtp_sni_callback, s);
+		ssl = ssl_smtp_init(ssl_ctx, smtp_sni_callback);
 		io_set_read(&s->io);
 		io_start_tls(&s->io, ssl);
 
@@ -1315,6 +1316,7 @@ smtp_io(struct io *io, int evt)
 {
 	struct ca_cert_req_msg	req_ca_cert;
 	struct smtp_session    *s = io->arg;
+	const char	       *sn;
 	char		       *line;
 	size_t			len, i;
 	X509		       *x;
@@ -1332,6 +1334,14 @@ smtp_io(struct io *io, int evt)
 		s->kickcount = 0;
 		s->phase = PHASE_INIT;
 
+		sn = smtp_sni_get_servername(s);
+		if (sn) {
+			if (strlcpy(s->sni, sn, sizeof s->sni) >= sizeof s->sni) {
+				smtp_free(s, "client SNI exceeds max hostname length");
+				return;
+			}
+		}
+
 		if (smtp_verify_certificate(s)) {
 			io_pause(&s->io, IO_PAUSE_IN);
 			break;
@@ -2451,6 +2461,12 @@ smtp_auth_failure_pause(struct smtp_session *s)
 	evtimer_add(&s->pause, &tv);
 }
 
+static const char *
+smtp_sni_get_servername(struct smtp_session *s)
+{
+	return SSL_get_servername(s->io.ssl, TLSEXT_NAMETYPE_host_name);
+}
+
 static int
 smtp_sni_callback(SSL *ssl, int *ad, void *arg)
 {
@@ -2461,10 +2477,6 @@ smtp_sni_callback(SSL *ssl, int *ad, void *arg)
 	sn = SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name);
 	if (sn == NULL)
 		return SSL_TLSEXT_ERR_NOACK;
-	if (strlcpy(s->sni, sn, sizeof s->sni) >= sizeof s->sni) {
-		log_warnx("warn: client SNI exceeds max hostname length");
-		return SSL_TLSEXT_ERR_NOACK;
-	}
 	ssl_ctx = dict_get(env->sc_ssl_dict, sn);
 	if (ssl_ctx == NULL) {
 		log_info("smtp-in: session %016"PRIx64

diff --git a/smtpd/smtpd.h b/smtpd/smtpd.h
@@ -1380,7 +1380,7 @@ int fork_proc_backend(const char *, const char *, const char *);
 
 /* ssl_smtpd.c */
 void   *ssl_mta_init(void *, char *, off_t, const char *);
-void   *ssl_smtp_init(void *, void *, void *);
+void   *ssl_smtp_init(void *, void *);
 
 
 /* stat_backend.c */

diff --git a/smtpd/ssl_smtpd.c b/smtpd/ssl_smtpd.c
@@ -82,7 +82,7 @@ dummy_verify(int ok, X509_STORE_CTX *store)
 }
 
 void *
-ssl_smtp_init(void *ssl_ctx, void *sni, void *arg)
+ssl_smtp_init(void *ssl_ctx, void *sni)
 {
 	SSL	*ssl = NULL;
 	int	(*cb)(SSL *,int *,void *) = sni;
@@ -91,10 +91,8 @@ ssl_smtp_init(void *ssl_ctx, void *sni, void *arg)
 
 	SSL_CTX_set_verify(ssl_ctx, SSL_VERIFY_PEER, dummy_verify);
 
-	if (cb) {
+	if (cb)
 		SSL_CTX_set_tlsext_servername_callback(ssl_ctx, cb);
-		SSL_CTX_set_tlsext_servername_arg(ssl_ctx, arg);
-	}
 
 	if ((ssl = SSL_new(ssl_ctx)) == NULL)
 		goto err;