tls: fix various tlsClient()/tlsServer() related bugs

- TLSconn structure on stack but not initialized (zeroed)
- original filedescriptor double closed in error case
- original filedescriptor leaked in success case
- leaked TLSconn.sessionID and TLSconn.cert
- clarify in pushtls(2) and pushssl(2)

4 files changed, 49 insertions, 60 deletions

diff --git a/sys/src/cmd/upas/fs/pop3.c b/sys/src/cmd/upas/fs/pop3.c
index fbb6f6c5e..257fc8970 100644
--- a/sys/src/cmd/upas/fs/pop3.c
+++ b/sys/src/cmd/upas/fs/pop3.c
@@ -129,6 +129,9 @@ pop3pushtls(Pop *pop)
 		err = "tls error";
 		goto out;
 	}
+	pop->fd = fd;
+	Binit(&pop->bin, pop->fd, OREAD);
+	Binit(&pop->bout, pop->fd, OWRITE);
 	if(conn.cert==nil || conn.certlen <= 0){
 		err = "server did not provide TLS certificate";
 		goto out;
@@ -140,17 +143,10 @@ pop3pushtls(Pop *pop)
 		err = "bad server certificate";
 		goto out;
 	}
-	close(pop->fd);
-	pop->fd = fd;
 	pop->encrypted = 1;
-	Binit(&pop->bin, pop->fd, OREAD);
-	Binit(&pop->bout, pop->fd, OWRITE);
-	fd = -1;
 out:
 	free(conn.sessionID);
 	free(conn.cert);
-	if(fd >= 0)
-		close(fd);
 	return err;
 }
 
diff --git a/sys/src/cmd/upas/pop3/pop3.c b/sys/src/cmd/upas/pop3/pop3.c
index 61b418062..1416f03ca 100644
--- a/sys/src/cmd/upas/pop3/pop3.c
+++ b/sys/src/cmd/upas/pop3/pop3.c
@@ -551,27 +551,31 @@ trace(char *fmt, ...)
 static int
 stlscmd(char*)
 {
-	int fd;
 	TLSconn conn;
+	int fd;
 
 	if(didtls)
 		return senderr("tls already started");
 	if(!tlscert)
 		return senderr("don't have any tls credentials");
-	sendok("");
-	Bflush(&out);
-
 	memset(&conn, 0, sizeof conn);
-	conn.cert = tlscert;
 	conn.certlen = ntlscert;
+	conn.cert = malloc(ntlscert);
+	if(conn.cert == nil)
+		return senderr("out of memory");
+	memmove(conn.cert, tlscert, ntlscert);
 	if(debug)
 		conn.trace = trace;
+	sendok("");
+	Bflush(&out);
 	fd = tlsServer(0, &conn);
 	if(fd < 0)
 		sysfatal("tlsServer: %r");
 	dup(fd, 0);
 	dup(fd, 1);
 	close(fd);
+	free(conn.cert);
+	free(conn.sessionID);
 	Binit(&in, 0, OREAD);
 	Binit(&out, 1, OWRITE);
 	didtls = 1;
diff --git a/sys/src/cmd/upas/smtp/smtp.c b/sys/src/cmd/upas/smtp/smtp.c
index 895c84914..41395899a 100644
--- a/sys/src/cmd/upas/smtp/smtp.c
+++ b/sys/src/cmd/upas/smtp/smtp.c
@@ -324,59 +324,53 @@ wraptls(void)
 {
 	TLSconn *c;
 	Thumbprint *goodcerts;
-	char *h;
+	char *h, *err;
 	int fd;
 	uchar hash[SHA1dlen];
 
+	err = Giveup;
 	c = mallocz(sizeof(*c), 1);
 	if (c == nil)
-		return Giveup;
+		return err;
 
 	fd = tlsClient(Bfildes(&bout), c);
 	if (fd < 0) {
 		syslog(0, "smtp", "tlsClient to %q: %r", ddomain);
-		free(c);
-		return Giveup;
+		goto Out;
 	}
+	Bterm(&bout);
+	Binit(&bout, fd, OWRITE);
+	fd = dup(fd, Bfildes(&bin));
+	Bterm(&bin);
+	Binit(&bin, fd, OREAD);
+
 	goodcerts = initThumbprints(smtpthumbs, smtpexclthumbs);
 	if (goodcerts == nil) {
 		syslog(0, "smtp", "bad thumbprints in %s", smtpthumbs);
-		close(fd);
-		free(c);
-		return Giveup;		/* how to recover? TLS is started */
+		goto Out;
 	}
 	/* compute sha1 hash of remote's certificate, see if we know it */
 	sha1(c->cert, c->certlen, hash, nil);
 	if (!okThumbprint(hash, goodcerts)) {
 		/* TODO? if not excluded, add hash to thumb list */
 		h = malloc(2*sizeof hash + 1);
-		if (h != nil) {
-			enc16(h, 2*sizeof hash + 1, hash, sizeof hash);
-			// fprint(2, "x509 sha1=%s", h);
-			syslog(0, "smtp",
-		"remote cert. has bad thumbprint: x509 sha1=%s server=%q",
-				h, ddomain);
-			free(h);
-		}
-		close(fd);
-		free(c);
-		return Giveup;		/* how to recover? TLS is started */
+		if (h == nil)
+			goto Out;
+		enc16(h, 2*sizeof hash + 1, hash, sizeof hash);
+		syslog(0, "smtp", "remote cert. has bad thumbprint: x509 sha1=%s server=%q",
+			h, ddomain);
+		free(h);
+		goto Out;
 	}
-	freeThumbprints(goodcerts);
-	free(c);
-	Bterm(&bin);
-	Bterm(&bout);
-
-	/*
-	 * set up bin & bout to use the TLS fd, i/o upon which generates
-	 * i/o on the original, underlying fd.
-	 */
-	Binit(&bin, fd, OREAD);
-	fd = dup(fd, -1);
-	Binit(&bout, fd, OWRITE);
-
 	syslog(0, "smtp", "started TLS to %q", ddomain);
-	return nil;
+	err = nil;
+Out:
+	if(goodcerts != nil)
+		freeThumbprints(goodcerts);
+	free(c->cert);
+	free(c->sessionID);
+	free(c);
+	return err;
 }
 
 /*
diff --git a/sys/src/cmd/upas/smtp/smtpd.c b/sys/src/cmd/upas/smtp/smtpd.c
index 8d9ebcb56..caa66dc06 100644
--- a/sys/src/cmd/upas/smtp/smtpd.c
+++ b/sys/src/cmd/upas/smtp/smtpd.c
@@ -1551,38 +1551,33 @@ starttls(void)
 {
 	int certlen, fd;
 	uchar *cert;
-	TLSconn *conn;
+	TLSconn conn;
 
 	if (tlscert == nil) {
 		reply("500 5.5.1 illegal command or bad syntax\r\n");
 		return;
 	}
-	conn = mallocz(sizeof *conn, 1);
 	cert = readcert(tlscert, &certlen);
-	if (conn == nil || cert == nil) {
-		if (conn != nil)
-			free(conn);
+	if (cert == nil) {
 		reply("454 4.7.5 TLS not available\r\n");
 		return;
 	}
 	reply("220 2.0.0 Go ahead make my day\r\n");
-	conn->cert = cert;
-	conn->certlen = certlen;
-	fd = tlsServer(Bfildes(&bin), conn);
+	memset(&conn, 0, sizeof(conn));
+	conn.cert = cert;
+	conn.certlen = certlen;
+	fd = tlsServer(Bfildes(&bin), &conn);
 	if (fd < 0) {
-		free(cert);
-		free(conn);
 		syslog(0, "smtpd", "TLS start-up failed with %s", him);
-
-		/* force the client to hang up */
-		close(Bfildes(&bin));		/* probably fd 0 */
-		close(1);
 		exits("tls failed");
 	}
 	Bterm(&bin);
-	Binit(&bin, fd, OREAD);
-	if (dup(fd, 1) < 0)
+	if (dup(fd, 0) < 0 || dup(fd, 1) < 0)
 		fprint(2, "dup of %d failed: %r\n", fd);
+	close(fd);
+	Binit(&bin, 0, OREAD);
+	free(conn.cert);
+	free(conn.sessionID);
 	passwordinclear = 1;
 	syslog(0, "smtpd", "started TLS with %s", him);
 }