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/src/lib/libtls/tls.c b/src/lib/libtls/tls.c
index c79191ee15..445933d176 100644
--- a/src/lib/libtls/tls.c
+++ b/src/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/src/lib/libtls/tls_client.c b/src/lib/libtls/tls_client.c
index 241c506676..168a7089fc 100644
--- a/src/lib/libtls/tls_client.c
+++ b/src/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/src/lib/libtls/tls_internal.h b/src/lib/libtls/tls_internal.h
index cf4a8e28ad..4503c20ab7 100644
--- a/src/lib/libtls/tls_internal.h
+++ b/src/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/src/lib/libtls/tls_server.c b/src/lib/libtls/tls_server.c
index 605ab69219..bb29c7ce42 100644
--- a/src/lib/libtls/tls_server.c
+++ b/src/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/src/lib/libtls/tls_verify.c b/src/lib/libtls/tls_verify.c
index 8ddc68a8f1..c603ca8f73 100644
--- a/src/lib/libtls/tls_verify.c
+++ b/src/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;