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)

2 files changed, 37 insertions, 48 deletions

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);
 }