Improve libtls error messages.

The tls_set_error() function previously stored the errno but did nothing
with it. Change tls_set_error() to append the strerror(3) of the stored
errno so that we include useful information regarding failures.

Provide a tls_set_errorx() function that does not store the errno or
include strerror(3) in the error message. Call this function instead of
tls_set_error() for errors where the errno value has no useful meaning.

With feedback from and ok doug@

5 files changed, 101 insertions, 55 deletions

diff --git a/lib/libtls/tls.c b/lib/libtls/tls.c
index c79191ee157..445933d1760 100644
--- a/lib/libtls/tls.c
+++ b/lib/libtls/tls.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: tls.c,v 1.14 2015/08/27 14:34:46 jsing Exp $ */
+/* $OpenBSD: tls.c,v 1.15 2015/08/27 15:26:49 jsing Exp $ */
 /*
  * Copyright (c) 2014 Joel Sing <jsing@openbsd.org>
  *
@@ -58,18 +58,61 @@ tls_error(struct tls *ctx)
 	return ctx->errmsg;
 }
 
+static int
+tls_set_verror(struct tls *ctx, int errnum, const char *fmt, va_list ap)
+{
+	char *errmsg = NULL;
+	int rv = -1;
+
+	free(ctx->errmsg);
+	ctx->errmsg = NULL;
+
+	if (vasprintf(&errmsg, fmt, ap) == -1) {
+		errmsg = NULL;
+		goto err;
+	}
+
+	if (errnum == -1) {
+		ctx->errmsg = errmsg;
+		return (0);
+	}
+
+	if (asprintf(&ctx->errmsg, "%s: %s", errmsg, strerror(errnum)) == -1) {
+		ctx->errmsg = NULL;
+		goto err;
+	}
+	
+	rv = 0;
+
+err:
+	free(errmsg);
+
+	return (rv);
+}
+
 int
-tls_set_error(struct tls *ctx, char *fmt, ...)
+tls_set_error(struct tls *ctx, const char *fmt, ...)
 {
 	va_list ap;
 	int rv;
 
-	ctx->err = errno;
-	free(ctx->errmsg);
-	ctx->errmsg = NULL;
+	ctx->errnum = errno;
+
+	va_start(ap, fmt);
+	rv = tls_set_verror(ctx, ctx->errnum, fmt, ap);
+	va_end(ap);
+
+	return (rv);
+}
+
+int
+tls_set_errorx(struct tls *ctx, const char *fmt, ...)
+{
+	va_list ap;
+	int rv;
 
 	va_start(ap, fmt);
-	rv = vasprintf(&ctx->errmsg, fmt, ap);
+	rv = tls_set_verror(ctx, -1, fmt, ap);
 	va_end(ap);
 
 	return (rv);
@@ -113,35 +156,35 @@ tls_configure_keypair(struct tls *ctx)
 
 	if (ctx->config->cert_mem != NULL) {
 		if (ctx->config->cert_len > INT_MAX) {
-			tls_set_error(ctx, "certificate too long");
+			tls_set_errorx(ctx, "certificate too long");
 			goto err;
 		}
 
 		if (SSL_CTX_use_certificate_chain_mem(ctx->ssl_ctx,
 		    ctx->config->cert_mem, ctx->config->cert_len) != 1) {
-			tls_set_error(ctx, "failed to load certificate");
+			tls_set_errorx(ctx, "failed to load certificate");
 			goto err;
 		}
 		cert = NULL;
 	}
 	if (ctx->config->key_mem != NULL) {
 		if (ctx->config->key_len > INT_MAX) {
-			tls_set_error(ctx, "key too long");
+			tls_set_errorx(ctx, "key too long");
 			goto err;
 		}
 
 		if ((bio = BIO_new_mem_buf(ctx->config->key_mem,
 		    ctx->config->key_len)) == NULL) {
-			tls_set_error(ctx, "failed to create buffer");
+			tls_set_errorx(ctx, "failed to create buffer");
 			goto err;
 		}
 		if ((pkey = PEM_read_bio_PrivateKey(bio, NULL, NULL,
 		    NULL)) == NULL) {
-			tls_set_error(ctx, "failed to read private key");
+			tls_set_errorx(ctx, "failed to read private key");
 			goto err;
 		}
 		if (SSL_CTX_use_PrivateKey(ctx->ssl_ctx, pkey) != 1) {
-			tls_set_error(ctx, "failed to load private key");
+			tls_set_errorx(ctx, "failed to load private key");
 			goto err;
 		}
 		BIO_free(bio);
@@ -153,20 +196,20 @@ tls_configure_keypair(struct tls *ctx)
 	if (ctx->config->cert_file != NULL) {
 		if (SSL_CTX_use_certificate_chain_file(ctx->ssl_ctx,
 		    ctx->config->cert_file) != 1) {
-			tls_set_error(ctx, "failed to load certificate file");
+			tls_set_errorx(ctx, "failed to load certificate file");
 			goto err;
 		}
 	}
 	if (ctx->config->key_file != NULL) {
 		if (SSL_CTX_use_PrivateKey_file(ctx->ssl_ctx,
 		    ctx->config->key_file, SSL_FILETYPE_PEM) != 1) {
-			tls_set_error(ctx, "failed to load private key file");
+			tls_set_errorx(ctx, "failed to load private key file");
 			goto err;
 		}
 	}
 
 	if (SSL_CTX_check_private_key(ctx->ssl_ctx) != 1) {
-		tls_set_error(ctx, "private/public key mismatch");
+		tls_set_errorx(ctx, "private/public key mismatch");
 		goto err;
 	}
 
@@ -203,7 +246,7 @@ tls_configure_ssl(struct tls *ctx)
 	if (ctx->config->ciphers != NULL) {
 		if (SSL_CTX_set_cipher_list(ctx->ssl_ctx,
 		    ctx->config->ciphers) != 1) {
-			tls_set_error(ctx, "failed to set ciphers");
+			tls_set_errorx(ctx, "failed to set ciphers");
 			goto err;
 		}
 	}
@@ -235,9 +278,9 @@ tls_reset(struct tls *ctx)
 	ctx->socket = -1;
 	ctx->state = 0;
 
-	ctx->err = 0;
 	free(ctx->errmsg);
 	ctx->errmsg = NULL;
+	ctx->errnum = 0;
 }
 
 int
@@ -267,21 +310,21 @@ tls_ssl_error(struct tls *ctx, SSL *ssl_conn, int ssl_ret, const char *prefix)
 		} else if (ssl_ret == -1) {
 			errstr = strerror(errno);
 		}
-		tls_set_error(ctx, "%s failed: %s", prefix, errstr);
+		tls_set_errorx(ctx, "%s failed: %s", prefix, errstr);
 		return (-1);
 
 	case SSL_ERROR_SSL:
 		if ((err = ERR_peek_error()) != 0) {
 			errstr = ERR_error_string(err, NULL);
 		}
-		tls_set_error(ctx, "%s failed: %s", prefix, errstr);
+		tls_set_errorx(ctx, "%s failed: %s", prefix, errstr);
 		return (-1);
 
 	case SSL_ERROR_WANT_CONNECT:
 	case SSL_ERROR_WANT_ACCEPT:
 	case SSL_ERROR_WANT_X509_LOOKUP:
 	default:
-		tls_set_error(ctx, "%s failed (%i)", prefix, ssl_err);
+		tls_set_errorx(ctx, "%s failed (%i)", prefix, ssl_err);
 		return (-1);
 	}
 }
@@ -294,7 +337,7 @@ tls_read(struct tls *ctx, void *buf, size_t buflen, size_t *outlen)
 	*outlen = 0;
 
 	if (buflen > INT_MAX) {
-		tls_set_error(ctx, "buflen too long");
+		tls_set_errorx(ctx, "buflen too long");
 		return (-1);
 	}
 
@@ -315,7 +358,7 @@ tls_write(struct tls *ctx, const void *buf, size_t buflen, size_t *outlen)
 	*outlen = 0;
 
 	if (buflen > INT_MAX) {
-		tls_set_error(ctx, "buflen too long");
+		tls_set_errorx(ctx, "buflen too long");
 		return (-1);
 	}
 
diff --git a/lib/libtls/tls_client.c b/lib/libtls/tls_client.c
index 241c506676e..168a7089fca 100644
--- a/lib/libtls/tls_client.c
+++ b/lib/libtls/tls_client.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: tls_client.c,v 1.20 2015/08/27 14:34:46 jsing Exp $ */
+/* $OpenBSD: tls_client.c,v 1.21 2015/08/27 15:26:50 jsing Exp $ */
 /*
  * Copyright (c) 2014 Joel Sing <jsing@openbsd.org>
  *
@@ -95,12 +95,12 @@ tls_connect_servername(struct tls *ctx, const char *host, const char *port,
 	int rv = -1, s = -1, ret;
 
 	if ((ctx->flags & TLS_CLIENT) == 0) {
-		tls_set_error(ctx, "not a client context");
+		tls_set_errorx(ctx, "not a client context");
 		goto err;
 	}
 
 	if (host == NULL) {
-		tls_set_error(ctx, "host not specified");
+		tls_set_errorx(ctx, "host not specified");
 		goto err;
 	}
 
@@ -111,7 +111,7 @@ tls_connect_servername(struct tls *ctx, const char *host, const char *port,
 	if ((p = (char *)port) == NULL) {
 		ret = tls_host_port(host, &hs, &ps);
 		if (ret == -1) {
-			tls_set_error(ctx, "memory allocation failure");
+			tls_set_errorx(ctx, "memory allocation failure");
 			goto err;
 		}
 		if (ret != 0)
@@ -169,7 +169,7 @@ tls_connect_fds(struct tls *ctx, int fd_read, int fd_write,
 	int ret, err;
 
 	if ((ctx->flags & TLS_CLIENT) == 0) {
-		tls_set_error(ctx, "not a client context");
+		tls_set_errorx(ctx, "not a client context");
 		goto err;
 	}
 
@@ -177,12 +177,12 @@ tls_connect_fds(struct tls *ctx, int fd_read, int fd_write,
 		goto connecting;
 
 	if (fd_read < 0 || fd_write < 0) {
-		tls_set_error(ctx, "invalid file descriptors");
+		tls_set_errorx(ctx, "invalid file descriptors");
 		return (-1);
 	}
 
 	if ((ctx->ssl_ctx = SSL_CTX_new(SSLv23_client_method())) == NULL) {
-		tls_set_error(ctx, "ssl context failure");
+		tls_set_errorx(ctx, "ssl context failure");
 		goto err;
 	}
 
@@ -191,7 +191,7 @@ tls_connect_fds(struct tls *ctx, int fd_read, int fd_write,
 
 	if (ctx->config->verify_name) {
 		if (servername == NULL) {
-			tls_set_error(ctx, "server name not specified");
+			tls_set_errorx(ctx, "server name not specified");
 			goto err;
 		}
 	}
@@ -201,19 +201,19 @@ tls_connect_fds(struct tls *ctx, int fd_read, int fd_write,
 
 		if (ctx->config->ca_mem != NULL) {
 			if (ctx->config->ca_len > INT_MAX) {
-				tls_set_error(ctx, "ca too long");
+				tls_set_errorx(ctx, "ca too long");
 				goto err;
 			}
 
 			if (SSL_CTX_load_verify_mem(ctx->ssl_ctx,
 			    ctx->config->ca_mem, ctx->config->ca_len) != 1) {
-				tls_set_error(ctx,
+				tls_set_errorx(ctx,
 				    "ssl verify memory setup failure");
 				goto err;
 			}
 		} else if (SSL_CTX_load_verify_locations(ctx->ssl_ctx,
 		    ctx->config->ca_file, ctx->config->ca_path) != 1) {
-			tls_set_error(ctx, "ssl verify setup failure");
+			tls_set_errorx(ctx, "ssl verify setup failure");
 			goto err;
 		}
 		if (ctx->config->verify_depth >= 0)
@@ -222,16 +222,16 @@ tls_connect_fds(struct tls *ctx, int fd_read, int fd_write,
 	}
 
 	if ((ctx->ssl_conn = SSL_new(ctx->ssl_ctx)) == NULL) {
-		tls_set_error(ctx, "ssl connection failure");
+		tls_set_errorx(ctx, "ssl connection failure");
 		goto err;
 	}
 	if (SSL_set_app_data(ctx->ssl_conn, ctx) != 1) {
-		tls_set_error(ctx, "ssl application data failure");
+		tls_set_errorx(ctx, "ssl application data failure");
 		goto err;
 	}
 	if (SSL_set_rfd(ctx->ssl_conn, fd_read) != 1 ||
 	    SSL_set_wfd(ctx->ssl_conn, fd_write) != 1) {
-		tls_set_error(ctx, "ssl file descriptor failure");
+		tls_set_errorx(ctx, "ssl file descriptor failure");
 		goto err;
 	}
 
@@ -243,7 +243,7 @@ tls_connect_fds(struct tls *ctx, int fd_read, int fd_write,
 	    inet_pton(AF_INET, servername, &addrbuf) != 1 &&
 	    inet_pton(AF_INET6, servername, &addrbuf) != 1) {
 		if (SSL_set_tlsext_host_name(ctx->ssl_conn, servername) == 0) {
-			tls_set_error(ctx, "server name indication failure");
+			tls_set_errorx(ctx, "server name indication failure");
 			goto err;
 		}
 	}
@@ -262,12 +262,12 @@ connecting:
 	if (ctx->config->verify_name) {
 		cert = SSL_get_peer_certificate(ctx->ssl_conn);
 		if (cert == NULL) {
-			tls_set_error(ctx, "no server certificate");
+			tls_set_errorx(ctx, "no server certificate");
 			goto err;
 		}
 		if ((ret = tls_check_servername(ctx, cert, servername)) != 0) {
 			if (ret != -2)
-				tls_set_error(ctx, "name `%s' not present in"
+				tls_set_errorx(ctx, "name `%s' not present in"
 				    " server certificate", servername);
 			goto err;
 		}
diff --git a/lib/libtls/tls_internal.h b/lib/libtls/tls_internal.h
index cf4a8e28ad3..4503c20ab7e 100644
--- a/lib/libtls/tls_internal.h
+++ b/lib/libtls/tls_internal.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: tls_internal.h,v 1.13 2015/08/27 14:34:46 jsing Exp $ */
+/* $OpenBSD: tls_internal.h,v 1.14 2015/08/27 15:26:50 jsing Exp $ */
 /*
  * Copyright (c) 2014 Jeremie Courreges-Anglas <jca@openbsd.org>
  * Copyright (c) 2014 Joel Sing <jsing@openbsd.org>
@@ -59,8 +59,8 @@ struct tls {
 	uint32_t flags;
 	uint32_t state;
 
-	int err;
 	char *errmsg;
+	int errnum;
 
 	int socket;
 
@@ -76,7 +76,10 @@ int tls_configure_keypair(struct tls *ctx);
 int tls_configure_server(struct tls *ctx);
 int tls_configure_ssl(struct tls *ctx);
 int tls_host_port(const char *hostport, char **host, char **port);
-int tls_set_error(struct tls *ctx, char *fmt, ...)
+int tls_set_error(struct tls *ctx, const char *fmt, ...)
+    __attribute__((__format__ (printf, 2, 3)))
+    __attribute__((__nonnull__ (2)));
+int tls_set_errorx(struct tls *ctx, const char *fmt, ...)
     __attribute__((__format__ (printf, 2, 3)))
     __attribute__((__nonnull__ (2)));
 int tls_ssl_error(struct tls *ctx, SSL *ssl_conn, int ssl_ret,
diff --git a/lib/libtls/tls_server.c b/lib/libtls/tls_server.c
index 605ab69219b..bb29c7ce425 100644
--- a/lib/libtls/tls_server.c
+++ b/lib/libtls/tls_server.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: tls_server.c,v 1.9 2015/08/22 14:52:39 jsing Exp $ */
+/* $OpenBSD: tls_server.c,v 1.10 2015/08/27 15:26:50 jsing Exp $ */
 /*
  * Copyright (c) 2014 Joel Sing <jsing@openbsd.org>
  *
@@ -54,7 +54,7 @@ tls_configure_server(struct tls *ctx)
 	unsigned char sid[SSL_MAX_SSL_SESSION_ID_LENGTH];
 
 	if ((ctx->ssl_ctx = SSL_CTX_new(SSLv23_server_method())) == NULL) {
-		tls_set_error(ctx, "ssl context failure");
+		tls_set_errorx(ctx, "ssl context failure");
 		goto err;
 	}
 
@@ -73,7 +73,7 @@ tls_configure_server(struct tls *ctx)
 	} else if (ctx->config->ecdhecurve != NID_undef) {
 		if ((ecdh_key = EC_KEY_new_by_curve_name(
 		    ctx->config->ecdhecurve)) == NULL) {
-			tls_set_error(ctx, "failed to set ECDHE curve");
+			tls_set_errorx(ctx, "failed to set ECDHE curve");
 			goto err;
 		}
 		SSL_CTX_set_options(ctx->ssl_ctx, SSL_OP_SINGLE_ECDH_USE);
@@ -88,7 +88,7 @@ tls_configure_server(struct tls *ctx)
 	 */
 	arc4random_buf(sid, sizeof(sid));
 	if (!SSL_CTX_set_session_id_context(ctx->ssl_ctx, sid, sizeof(sid))) {
-		tls_set_error(ctx, "failed to set session id context");
+		tls_set_errorx(ctx, "failed to set session id context");
 		goto err;
 	}
 
@@ -105,28 +105,28 @@ tls_accept_fds(struct tls *ctx, struct tls **cctx, int fd_read, int fd_write)
 	int ret, err;
 	
 	if ((ctx->flags & TLS_SERVER) == 0) {
-		tls_set_error(ctx, "not a server context");
+		tls_set_errorx(ctx, "not a server context");
 		goto err;
 	}
 
 	if (conn_ctx == NULL) {
 		if ((conn_ctx = tls_server_conn(ctx)) == NULL) {
-			tls_set_error(ctx, "connection context failure");
+			tls_set_errorx(ctx, "connection context failure");
 			goto err;
 		}
 		*cctx = conn_ctx;
 
 		if ((conn_ctx->ssl_conn = SSL_new(ctx->ssl_ctx)) == NULL) {
-			tls_set_error(ctx, "ssl failure");
+			tls_set_errorx(ctx, "ssl failure");
 			goto err;
 		}
 		if (SSL_set_app_data(conn_ctx->ssl_conn, conn_ctx) != 1) {
-			tls_set_error(ctx, "ssl application data failure");
+			tls_set_errorx(ctx, "ssl application data failure");
 			goto err;
 		}
 		if (SSL_set_rfd(conn_ctx->ssl_conn, fd_read) != 1 ||
 		    SSL_set_wfd(conn_ctx->ssl_conn, fd_write) != 1) {
-			tls_set_error(ctx, "ssl file descriptor failure");
+			tls_set_errorx(ctx, "ssl file descriptor failure");
 			goto err;
 		}
 	}
diff --git a/lib/libtls/tls_verify.c b/lib/libtls/tls_verify.c
index 8ddc68a8f1d..c603ca8f73d 100644
--- a/lib/libtls/tls_verify.c
+++ b/lib/libtls/tls_verify.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: tls_verify.c,v 1.9 2015/08/27 07:15:39 jsing Exp $ */
+/* $OpenBSD: tls_verify.c,v 1.10 2015/08/27 15:26:50 jsing Exp $ */
 /*
  * Copyright (c) 2014 Jeremie Courreges-Anglas <jca@openbsd.org>
  *
@@ -125,7 +125,7 @@ tls_check_subject_altname(struct tls *ctx, X509 *cert, const char *name)
 				len = ASN1_STRING_length(altname->d.dNSName);
 
 				if (len < 0 || len != strlen(data)) {
-					tls_set_error(ctx,
+					tls_set_errorx(ctx,
 					    "error verifying name '%s': "
 					    "NUL byte in subjectAltName, "
 					    "probably a malicious certificate",
@@ -168,7 +168,7 @@ tls_check_subject_altname(struct tls *ctx, X509 *cert, const char *name)
 			data = ASN1_STRING_data(altname->d.iPAddress);
 
 			if (datalen < 0) {
-				tls_set_error(ctx,
+				tls_set_errorx(ctx,
 				    "Unexpected negative length for an "
 				    "IP address: %d", datalen);
 				rv = -2;
@@ -218,7 +218,7 @@ tls_check_common_name(struct tls *ctx, X509 *cert, const char *name)
 
 	/* NUL bytes in CN? */
 	if (common_name_len != strlen(common_name)) {
-		tls_set_error(ctx, "error verifying name '%s': "
+		tls_set_errorx(ctx, "error verifying name '%s': "
 		    "NUL byte in Common Name field, "
 		    "probably a malicious certificate", name);
 		rv = -2;