Improve crypto/ssl logging

PATCH New Steffan Karger 3 participants 17 messages Oct 25, 2014 – Dec 31, 2014

lore

Share & tools

lore

Steffan Karger

author

2014-10-25 20:35 UTC #0 lore raw

PATCH 1/1 Improve crypto/ssl logging

Hi,

This patch set improves the logging from the crypto/ssl components. It adds some debug/logging wrappers to make it easy for developers to add proper logging, and makes the logging more verbose by reporting the underlying crypto library errors too.

When errors occur during the TLS handshake, users are quite often puzzled. We won't be able to fully prevent that, but let's at least be as verbose as possible about the underlying problems.

I prepared this set for the master branch, but on request I can backport it to 2.3 too, just let me know.

-Steffan

Steffan Karger

author

2014-10-25 20:35 UTC #1 lore raw

PATCH 1/6 polarssl: add polar_log_err() and polar_ok(), to easily log PolarSSL errors

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/crypto_polarssl.c | 26 ++++++++++++++++++++++++++
 src/openvpn/crypto_polarssl.h | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

src/openvpn/crypto_polarssl.c +26 -0

index 1a986db..ce1abd6 100644
@@ -86,6 +86,32 @@ crypto_clear_error (void)
 {
 }
 
+bool polar_log_err(unsigned int flags, int errval, const char *prefix)
+{
+  if (0 != errval)
+    {
+      char errstr[256];
+      polarssl_strerror(errval, errstr, sizeof(errstr));
+
+      if (NULL == prefix) prefix = "PolarSSL error";
+      msg (flags, "%s: %s", prefix, errstr);
+    }
+
+  return 0 == errval;
+}
+
+bool polar_log_func_line(unsigned int flags, int errval, const char *func,
+    int line)
+{
+  char prefix[256];
+
+  if (!openvpn_snprintf(prefix, sizeof(prefix), "%s:%d", func, line))
+    return polar_log_err(flags, errval, func);
+
+  return polar_log_err(flags, errval, prefix);
+}
+
+
 #ifdef DMALLOC
 void
 crypto_init_dmalloc (void)

src/openvpn/crypto_polarssl.h +40 -0

index b6da436..b7ce682 100644
@@ -91,4 +91,44 @@ ctr_drbg_context * rand_ctx_get();
 void rand_ctx_enable_prediction_resistance();
 #endif
 
+/**
+ * Log the supplied PolarSSL error, then print the supplied error message.
+ *
+ * @param flags		Flags to indicate error type and priority.
+ * @param errval	PolarSSL error code to convert to error message.
+ * @param prefix	Prefix to PolarSSL error message.
+ *
+ * @returns true if no errors are detected, false otherwise.
+ */
+bool polar_log_err(unsigned int flags, int errval, const char *prefix);
+
+/**
+ * Log the supplied PolarSSL error, then print the supplied error message.
+ *
+ * @param flags		Flags to indicate error type and priority.
+ * @param errval	PolarSSL error code to convert to error message.
+ * @param func		Function name where error was reported.
+ * @param line		Line number where error was reported.
+ *
+ * @returns true if no errors are detected, false otherwise.
+ */
+bool polar_log_func_line(unsigned int flags, int errval, const char *func,
+    int line);
+
+/**
+ * Check errval and log on error.
+ *
+ * Convenience wrapper to put around polarssl library calls, e.g.
+ *   if (!polar_ok(polarssl_func())) return 0;
+ * or
+ *   ASSERT (polar_ok(polarssl_func()));
+ *
+ * @param errval	PolarSSL error code to convert to error message.
+ *
+ * @returns true if no errors are detected, false otherwise.
+ */
+#define polar_ok(errval) \
+  polar_log_func_line(D_CRYPT_ERRORS, errval, __func__, __LINE__)
+
+
 #endif /* CRYPTO_POLARSSL_H_ */

Signature · 1 line

1.9.1

Steffan Karger

author

2014-10-27 20:50 UTC #2 lore raw

Re: [Openvpn-devel] [PATCH 1/6] polarssl: add polar_log_err() and polar_ok(), to easily log PolarSSL errors

Attached a v2 of this patch. The only difference is that it adds a missing #include <polarssl/error.h>.

Quoted 83 lines · Steffan Karger

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/crypto_polarssl.c | 26 ++++++++++++++++++++++++++
 src/openvpn/crypto_polarssl.h | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

src/openvpn/crypto_polarssl.c +26 -0

index 1a986db..ce1abd6 100644
@@ -86,6 +86,32 @@ crypto_clear_error (void)
 {
 }
 
+bool polar_log_err(unsigned int flags, int errval, const char *prefix)
+{
+  if (0 != errval)
+    {
+      char errstr[256];
+      polarssl_strerror(errval, errstr, sizeof(errstr));
+
+      if (NULL == prefix) prefix = "PolarSSL error";
+      msg (flags, "%s: %s", prefix, errstr);
+    }
+
+  return 0 == errval;
+}
+
+bool polar_log_func_line(unsigned int flags, int errval, const char *func,
+    int line)
+{
+  char prefix[256];
+
+  if (!openvpn_snprintf(prefix, sizeof(prefix), "%s:%d", func, line))
+    return polar_log_err(flags, errval, func);
+
+  return polar_log_err(flags, errval, prefix);
+}
+
+
 #ifdef DMALLOC
 void
 crypto_init_dmalloc (void)

src/openvpn/crypto_polarssl.h +31 -0

index b6da436..b7ce682 100644
@@ -91,4 +91,44 @@ ctr_drbg_context * rand_ctx_get();
 void rand_ctx_enable_prediction_resistance();
 #endif
 
+/**
+ * Log the supplied PolarSSL error, then print the supplied error message.
+ *
+ * @param flags		Flags to indicate error type and priority.
+ * @param errval	PolarSSL error code to convert to error message.
+ * @param prefix	Prefix to PolarSSL error message.
+ *
+ * @returns true if no errors are detected, false otherwise.
+ */
+bool polar_log_err(unsigned int flags, int errval, const char *prefix);
+
+/**
+ * Log the supplied PolarSSL error, then print the supplied error message.
+ *
+ * @param flags		Flags to indicate error type and priority.
+ * @param errval	PolarSSL error code to convert to error message.
+ * @param func		Function name where error was reported.
+ * @param line		Line number where error was reported.
+ *
+ * @returns true if no errors are detected, false otherwise.
+ */
+bool polar_log_func_line(unsigned int flags, int errval, const char *func,
+    int line);
+
+/**
+ * Check errval and log on error.
+ *
+ * Convenience wrapper to put around polarssl library calls, e.g.
+ *   if (!polar_ok(polarssl_func())) return 0;
+ * or
+ *   ASSERT (polar_ok(polarssl_func()));

src/openvpn/crypto_polarssl.h ctr_drbg_context * rand_ctx_get(); · line 134

Quoted 10 lines

src/openvpn/crypto_polarssl.h +9 -0

+ *
+ * @param errval	PolarSSL error code to convert to error message.
+ *
+ * @returns true if no errors are detected, false otherwise.
+ */
+#define polar_ok(errval) \
+  polar_log_func_line(D_CRYPT_ERRORS, errval, __func__, __LINE__)
+
+
 #endif /* CRYPTO_POLARSSL_H_ */

From eda4b8242c1c65c1dedbe866b8159a96b6552b08 Mon Sep 17 00:00:00 2001
From: Steffan Karger <[email protected]...>
Date: Sat, 25 Oct 2014 20:04:33 +0200
Subject: [PATCH 1/6] polarssl: add polar_log_err() and polar_ok(), to easily
log PolarSSL errors

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/crypto_polarssl.c | 27 +++++++++++++++++++++++++++
 src/openvpn/crypto_polarssl.h | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)

src/openvpn/crypto_polarssl.c +27 -0

index 1a986db..9aedadf 100644
@@ -46,6 +46,7 @@
 #include "misc.h"
 
 #include <polarssl/des.h>
+#include <polarssl/error.h>
 #include <polarssl/md5.h>
 #include <polarssl/cipher.h>
 #include <polarssl/havege.h>

@@ -86,6 +87,32 @@ crypto_clear_error (void)
 {
 }
 
+bool polar_log_err(unsigned int flags, int errval, const char *prefix)
+{
+  if (0 != errval)
+    {
+      char errstr[256];
+      polarssl_strerror(errval, errstr, sizeof(errstr));
+
+      if (NULL == prefix) prefix = "PolarSSL error";
+      msg (flags, "%s: %s", prefix, errstr);
+    }
+
+  return 0 == errval;
+}
+
+bool polar_log_func_line(unsigned int flags, int errval, const char *func,
+    int line)
+{
+  char prefix[256];
+
+  if (!openvpn_snprintf(prefix, sizeof(prefix), "%s:%d", func, line))
+    return polar_log_err(flags, errval, func);
+
+  return polar_log_err(flags, errval, prefix);
+}
+
+
 #ifdef DMALLOC
 void
 crypto_init_dmalloc (void)

src/openvpn/crypto_polarssl.h +40 -0

index b6da436..b7ce682 100644
@@ -91,4 +91,44 @@ ctr_drbg_context * rand_ctx_get();
 void rand_ctx_enable_prediction_resistance();
 #endif
 
+/**
+ * Log the supplied PolarSSL error, then print the supplied error message.
+ *
+ * @param flags		Flags to indicate error type and priority.
+ * @param errval	PolarSSL error code to convert to error message.
+ * @param prefix	Prefix to PolarSSL error message.
+ *
+ * @returns true if no errors are detected, false otherwise.
+ */
+bool polar_log_err(unsigned int flags, int errval, const char *prefix);
+
+/**
+ * Log the supplied PolarSSL error, then print the supplied error message.
+ *
+ * @param flags		Flags to indicate error type and priority.
+ * @param errval	PolarSSL error code to convert to error message.
+ * @param func		Function name where error was reported.
+ * @param line		Line number where error was reported.
+ *
+ * @returns true if no errors are detected, false otherwise.
+ */
+bool polar_log_func_line(unsigned int flags, int errval, const char *func,
+    int line);
+
+/**
+ * Check errval and log on error.
+ *
+ * Convenience wrapper to put around polarssl library calls, e.g.
+ *   if (!polar_ok(polarssl_func())) return 0;
+ * or
+ *   ASSERT (polar_ok(polarssl_func()));
+ *
+ * @param errval	PolarSSL error code to convert to error message.
+ *
+ * @returns true if no errors are detected, false otherwise.
+ */
+#define polar_ok(errval) \
+  polar_log_func_line(D_CRYPT_ERRORS, errval, __func__, __LINE__)
+
+
 #endif /* CRYPTO_POLARSSL_H_ */

Signature · 1 line

1.9.1

Steffan Karger

author

2014-10-25 20:35 UTC #3 lore raw

PATCH 2/6 polarssl: Improve PolarSSL logging

In reply to Steffan Karger

Use the new polar_log_err() and polar_ok() functions introduced in the previous commit to provide more log/debug output on SSL errors.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/crypto_polarssl.c     | 35 ++++++++-------
 src/openvpn/ssl_polarssl.c        | 91 ++++++++++++++++++---------------------
 src/openvpn/ssl_verify_polarssl.c | 26 +++--------
 3 files changed, 67 insertions(+), 85 deletions(-)

src/openvpn/crypto_polarssl.c +19 ~~-16~~

index ce1abd6..1b267d7 100644
@@ -260,7 +260,8 @@ ctr_drbg_context * rand_ctx_get()
       /* Initialise PolarSSL RNG, and built-in entropy sources */
       entropy_init(&ec);
 
-      if (0 != ctr_drbg_init(&cd_ctx, entropy_func, &ec, BPTR(&pers_string), BLEN(&pers_string)))
+      if (!polar_ok(ctr_drbg_init(&cd_ctx, entropy_func, &ec,
+		    BPTR(&pers_string), BLEN(&pers_string))))
         msg (M_FATAL, "Failed to initialize random generator");
 
       gc_free(&gc);

@@ -471,10 +472,10 @@ cipher_ctx_init (cipher_context_t *ctx, uint8_t *key, int key_len,
 
   CLEAR (*ctx);
 
-  if (0 != cipher_init_ctx(ctx, kt))
+  if (!polar_ok(cipher_init_ctx(ctx, kt)))
     msg (M_FATAL, "PolarSSL cipher context init #1");
 
-  if (0 != cipher_setkey(ctx, key, key_len*8, enc))
+  if (!polar_ok(cipher_setkey(ctx, key, key_len*8, enc)))
     msg (M_FATAL, "PolarSSL cipher set key");
 
   /* make sure we used a big enough key */

@@ -483,7 +484,7 @@ cipher_ctx_init (cipher_context_t *ctx, uint8_t *key, int key_len,
 
 void cipher_ctx_cleanup (cipher_context_t *ctx)
 {
-  cipher_free_ctx(ctx);
+  ASSERT (polar_ok(cipher_free_ctx(ctx)));
 }
 
 int cipher_ctx_iv_length (const cipher_context_t *ctx)

@@ -513,36 +514,38 @@ cipher_ctx_get_cipher_kt (const cipher_ctx_t *ctx)
 
 int cipher_ctx_reset (cipher_context_t *ctx, uint8_t *iv_buf)
 {
-  int retval = cipher_reset(ctx);
+  if (!polar_ok(cipher_reset(ctx)))
+    return 0;
 
-  if (0 == retval)
-    retval = cipher_set_iv(ctx, iv_buf, ctx->cipher_info->iv_size);
+  if (!polar_ok(cipher_set_iv(ctx, iv_buf, ctx->cipher_info->iv_size)))
+    return 0;
 
-  return 0 == retval;
+  return 1;
 }
 
 int cipher_ctx_update (cipher_context_t *ctx, uint8_t *dst, int *dst_len,
     uint8_t *src, int src_len)
 {
-  int retval = 0;
   size_t s_dst_len = *dst_len;
 
-  retval = cipher_update(ctx, src, (size_t)src_len, dst, &s_dst_len);
+  if (!polar_ok(cipher_update(ctx, src, (size_t)src_len, dst, &s_dst_len)))
+    return 0;
 
   *dst_len = s_dst_len;
 
-  return 0 == retval;
+  return 1;
 }
 
 int cipher_ctx_final (cipher_context_t *ctx, uint8_t *dst, int *dst_len)
 {
-  int retval = 0;
   size_t s_dst_len = *dst_len;
 
-  retval = cipher_finish(ctx, dst, &s_dst_len);
+  if (!polar_ok(cipher_finish(ctx, dst, &s_dst_len)))
+    return 0;
+
   *dst_len = s_dst_len;
 
-  return 0 == retval;
+  return 1;
 }
 
 void

@@ -552,8 +555,8 @@ cipher_des_encrypt_ecb (const unsigned char key[DES_KEY_LENGTH],
 {
     des_context ctx;
 
-    des_setkey_enc(&ctx, key);
-    des_crypt_ecb(&ctx, src, dst);
+    ASSERT (polar_ok(des_setkey_enc(&ctx, key)));
+    ASSERT (polar_ok(des_crypt_ecb(&ctx, src, dst)));
 }

src/openvpn/ssl_polarssl.c +41 ~~-50~~

225 lines not on this page · raw message

src/openvpn/ssl_verify_polarssl.c +7 ~~-19~~

index 2b7c214..ed0297b 100644
@@ -131,17 +131,12 @@ backend_x509_get_serial (openvpn_x509_cert_t *cert, struct gc_arena *gc)
   char *buf = NULL;
   size_t buflen = 0;
   mpi serial_mpi = { 0 };
-  int retval = 0;
 
   /* Transform asn1 integer serial into PolarSSL MPI */
   mpi_init(&serial_mpi);
-  retval = mpi_read_binary(&serial_mpi, cert->serial.p, cert->serial.len);
-  if (retval < 0)
+  if (!polar_ok(mpi_read_binary(&serial_mpi, cert->serial.p, cert->serial.len)))
     {
-      char errbuf[128];
-      polarssl_strerror(retval, errbuf, sizeof(errbuf));
-
-      msg(M_WARN, "Failed to retrieve serial from certificate: %s.", errbuf);
+      msg(M_WARN, "Failed to retrieve serial from certificate.");
       return NULL;
     }

@@ -150,13 +145,9 @@ backend_x509_get_serial (openvpn_x509_cert_t *cert, struct gc_arena *gc)
   buf = gc_malloc(buflen, true, gc);
 
   /* Write MPI serial as decimal string into buffer */
-  retval = mpi_write_string(&serial_mpi, 10, buf, &buflen);
-  if (retval < 0)
+  if (!polar_ok(mpi_write_string(&serial_mpi, 10, buf, &buflen)))
     {
-      char errbuf[128];
-      polarssl_strerror(retval, errbuf, sizeof(errbuf));
-
-      msg(M_WARN, "Failed to write serial to string: %s.", errbuf);
+      msg(M_WARN, "Failed to write serial to string.");
       return NULL;
     }

@@ -371,12 +362,9 @@ x509_verify_crl(const char *crl_file, x509_crt *cert, const char *subject)
   result_t retval = FAILURE;
   x509_crl crl = {0};
 
-  int polar_retval = x509_crl_parse_file(&crl, crl_file);
-  if (polar_retval != 0)
+  if (!polar_ok(x509_crl_parse_file(&crl, crl_file)))
     {
-      char errstr[128];
-      polarssl_strerror(polar_retval, errstr, sizeof(errstr));
-      msg (M_WARN, "CRL: cannot read CRL from file %s (%s)", crl_file, errstr);
+      msg (M_WARN, "CRL: cannot read CRL from file %s", crl_file);
       goto end;
     }

@@ -389,7 +377,7 @@ x509_verify_crl(const char *crl_file, x509_crt *cert, const char *subject)
       goto end;
     }
 
-  if (0 != x509_crt_revoked(cert, &crl))
+  if (!polar_ok(x509_crt_revoked(cert, &crl)))
     {
       msg (D_HANDSHAKE, "CRL CHECK FAILED: %s is REVOKED", subject);
       goto end;

Signature · 1 line

1.9.1

Steffan Karger

author

2014-10-25 20:35 UTC #4 lore raw

PATCH 3/6 polarssl: don't ignore failed ssl_init(), but ASSERT() on failure

In reply to Steffan Karger

Since key_state_ssl_init() returns void, it *must* succeed. By changing the 'if(a) { success } else ignore' into 'ASSERT(a); success', we make sure we don't fail (which we in practice don't do, because otherwise we would have had complaining users with hard to trace errors).

This patch just changes the if() to an ASSERT(), but that causes a lot of indenting changes, making this patch look a lot bigger than it really is.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/ssl_polarssl.c | 128 ++++++++++++++++++++++-----------------------
 1 file changed, 64 insertions(+), 64 deletions(-)

src/openvpn/ssl_polarssl.c +64 ~~-64~~

index b332021..1a77c30 100644
@@ -682,89 +682,89 @@ void key_state_ssl_init(struct key_state_ssl *ks_ssl,
   CLEAR(*ks_ssl);
 
   ALLOC_OBJ_CLEAR(ks_ssl->ctx, ssl_context);
-  if (polar_ok(ssl_init(ks_ssl->ctx))) /* FIXME ASSERT() or propagate errors */
-    {
-      /* Initialise SSL context */
-      ssl_set_dbg (ks_ssl->ctx, my_debug, NULL);
-      ssl_set_endpoint (ks_ssl->ctx, ssl_ctx->endpoint);
 
-      ssl_set_rng (ks_ssl->ctx, ctr_drbg_random, rand_ctx_get());
+  ASSERT (polar_ok(ssl_init(ks_ssl->ctx)));
+
+  /* Initialise SSL context */
+  ssl_set_dbg (ks_ssl->ctx, my_debug, NULL);
+  ssl_set_endpoint (ks_ssl->ctx, ssl_ctx->endpoint);
+
+  ssl_set_rng (ks_ssl->ctx, ctr_drbg_random, rand_ctx_get());
 
-      if (ssl_ctx->allowed_ciphers)
-	ssl_set_ciphersuites (ks_ssl->ctx, ssl_ctx->allowed_ciphers);
+  if (ssl_ctx->allowed_ciphers)
+    ssl_set_ciphersuites (ks_ssl->ctx, ssl_ctx->allowed_ciphers);
 
-      /* Initialise authentication information */
-      if (is_server)
-	ASSERT (polar_ok(ssl_set_dh_param_ctx(ks_ssl->ctx, ssl_ctx->dhm_ctx)));
+  /* Initialise authentication information */
+  if (is_server)
+    ASSERT (polar_ok(ssl_set_dh_param_ctx(ks_ssl->ctx, ssl_ctx->dhm_ctx)));
 #if defined(ENABLE_PKCS11)
-      if (ssl_ctx->priv_key_pkcs11 != NULL)
-	ASSERT (polar_ok(ssl_set_own_cert_alt(ks_ssl->ctx, ssl_ctx->crt_chain,
-	    ssl_ctx->priv_key_pkcs11, ssl_pkcs11_decrypt, ssl_pkcs11_sign,
-	    ssl_pkcs11_key_len)));
-      else
+  if (ssl_ctx->priv_key_pkcs11 != NULL)
+    ASSERT (polar_ok(ssl_set_own_cert_alt(ks_ssl->ctx, ssl_ctx->crt_chain,
+	ssl_ctx->priv_key_pkcs11, ssl_pkcs11_decrypt, ssl_pkcs11_sign,
+	ssl_pkcs11_key_len)));
+  else
 #endif
 #if defined(MANAGMENT_EXTERNAL_KEY)
-      if (ssl_ctx->external_key != NULL)
-	ASSERT (polar_ok(ssl_set_own_cert_alt(ks_ssl->ctx, ssl_ctx->crt_chain,
-	   ssl_ctx->external_key, NULL, external_pkcs1_sign,
-	   external_key_len)));
-      else
+  if (ssl_ctx->external_key != NULL)
+    ASSERT (polar_ok(ssl_set_own_cert_alt(ks_ssl->ctx, ssl_ctx->crt_chain,
+       ssl_ctx->external_key, NULL, external_pkcs1_sign,
+       external_key_len)));
+  else
 #endif
-	ASSERT (polar_ok(ssl_set_own_cert( ks_ssl->ctx, ssl_ctx->crt_chain,
-	    ssl_ctx->priv_key )));
+    ASSERT (polar_ok(ssl_set_own_cert( ks_ssl->ctx, ssl_ctx->crt_chain,
+	ssl_ctx->priv_key )));
 
-      /* Initialise SSL verification */
+  /* Initialise SSL verification */
 #if P2MP_SERVER
-      if (session->opt->ssl_flags & SSLF_CLIENT_CERT_NOT_REQUIRED)
-	{
-	  msg (M_WARN, "WARNING: POTENTIALLY DANGEROUS OPTION "
-	   "--client-cert-not-required may accept clients which do not present "
-	   "a certificate");
-	}
-      else
+  if (session->opt->ssl_flags & SSLF_CLIENT_CERT_NOT_REQUIRED)
+    {
+      msg (M_WARN, "WARNING: POTENTIALLY DANGEROUS OPTION "
+       "--client-cert-not-required may accept clients which do not present "
+       "a certificate");
+    }
+  else
 #endif
-      {
-	ssl_set_authmode (ks_ssl->ctx, SSL_VERIFY_REQUIRED);
-	ssl_set_verify (ks_ssl->ctx, verify_callback, session);
-      }
+  {
+    ssl_set_authmode (ks_ssl->ctx, SSL_VERIFY_REQUIRED);
+    ssl_set_verify (ks_ssl->ctx, verify_callback, session);
+  }
 
-      /* TODO: PolarSSL does not currently support sending the CA chain to the client */
-      ssl_set_ca_chain (ks_ssl->ctx, ssl_ctx->ca_chain, NULL, NULL );
+  /* TODO: PolarSSL does not currently support sending the CA chain to the client */
+  ssl_set_ca_chain (ks_ssl->ctx, ssl_ctx->ca_chain, NULL, NULL );
 
-      /* Initialize minimum TLS version */
+  /* Initialize minimum TLS version */
+  {
+    const int tls_version_min = (session->opt->ssl_flags >> SSLF_TLS_VERSION_SHIFT) & SSLF_TLS_VERSION_MASK;
+    int polar_major;
+    int polar_minor;
+    switch (tls_version_min)
       {
-	const int tls_version_min = (session->opt->ssl_flags >> SSLF_TLS_VERSION_SHIFT) & SSLF_TLS_VERSION_MASK;
-	int polar_major;
-	int polar_minor;
-	switch (tls_version_min)
-	  {
-	  case TLS_VER_1_0:
-	  default:
-	    polar_major = SSL_MAJOR_VERSION_3;
-	    polar_minor = SSL_MINOR_VERSION_1;
-	    break;
+      case TLS_VER_1_0:
+      default:
+	polar_major = SSL_MAJOR_VERSION_3;
+	polar_minor = SSL_MINOR_VERSION_1;
+	break;
 #if defined(SSL_MAJOR_VERSION_3) && defined(SSL_MINOR_VERSION_2)
-	  case TLS_VER_1_1:
-	    polar_major = SSL_MAJOR_VERSION_3;
-	    polar_minor = SSL_MINOR_VERSION_2;
-	    break;
+      case TLS_VER_1_1:
+	polar_major = SSL_MAJOR_VERSION_3;
+	polar_minor = SSL_MINOR_VERSION_2;
+	break;
 #endif
 #if defined(SSL_MAJOR_VERSION_3) && defined(SSL_MINOR_VERSION_3)
-	  case TLS_VER_1_2:
-	    polar_major = SSL_MAJOR_VERSION_3;
-	    polar_minor = SSL_MINOR_VERSION_3;
-	    break;
+      case TLS_VER_1_2:
+	polar_major = SSL_MAJOR_VERSION_3;
+	polar_minor = SSL_MINOR_VERSION_3;
+	break;
 #endif
-	  }
-	ssl_set_min_version(ks_ssl->ctx, polar_major, polar_minor);
       }
+    ssl_set_min_version(ks_ssl->ctx, polar_major, polar_minor);
+  }
 
-      /* Initialise BIOs */
-      ALLOC_OBJ_CLEAR (ks_ssl->ct_in, endless_buffer);
-      ALLOC_OBJ_CLEAR (ks_ssl->ct_out, endless_buffer);
-      ssl_set_bio (ks_ssl->ctx, endless_buf_read, ks_ssl->ct_in,
-	  endless_buf_write, ks_ssl->ct_out);
-    }
+  /* Initialise BIOs */
+  ALLOC_OBJ_CLEAR (ks_ssl->ct_in, endless_buffer);
+  ALLOC_OBJ_CLEAR (ks_ssl->ct_out, endless_buffer);
+  ssl_set_bio (ks_ssl->ctx, endless_buf_read, ks_ssl->ct_in,
+      endless_buf_write, ks_ssl->ct_out);
 }
 
 void

Signature · 1 line

1.9.1

Arne Schwabe

2014-10-27 20:14 UTC #5 lore raw

Re: [Openvpn-devel] [PATCH 3/6] polarssl: don't ignore failed ssl_init(), but ASSERT() on failure

Am 25.10.14 22:35, schrieb Steffan Karger:

Quoted 9 lines

Since key_state_ssl_init() returns void, it *must* succeed. By changing the 'if(a) { success } else ignore' into 'ASSERT(a); success', we make sure we don't fail (which we in practice don't do, because otherwise we would have had complaining users with hard to trace errors).
This patch just changes the if() to an ASSERT(), but that causes a lot of indenting changes, making this patch look a lot bigger than it really is.

ACK. Bailing out on these kind of crypto errors seems better than to continue in an unknown state.

Arne

Steffan Karger

author

2014-10-25 20:35 UTC #6 lore raw

PATCH 4/6 openssl: add crypto_msg(), to easily log openssl errors

In reply to Steffan Karger

This works towards removing OpenSSL-specific error printing code from error.c. The crypto_msg() functions provide convenience wrappers, specific to OpenSSL. Instead of passing the magical 'M_SSLERR' flag to msg(), a developer now just calls crypto_msg() to get OpenSSL errors dumped to log.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/crypto_openssl.c |  9 +++++++++
 src/openvpn/crypto_openssl.h | 25 +++++++++++++++++++++++++
 src/openvpn/error.h          |  6 ++++++
 3 files changed, 40 insertions(+)

src/openvpn/crypto_openssl.c +9 -0

index 0ac89a1..25d8cf9 100644
@@ -195,6 +195,15 @@ crypto_clear_error (void)
   ERR_clear_error ();
 }
 
+void
+crypto_print_openssl_errors(const unsigned int flags) {
+  size_t err = 0;
+
+  while ((err = ERR_get_error ()))
+    msg (flags, "OpenSSL: %s", ERR_error_string (err, NULL));
+}
+
+
 /*
  *
  * OpenSSL memory debugging.  If dmalloc debugging is enabled, tell

src/openvpn/crypto_openssl.h +25 -0

index f883c2a..42c7e9a 100644
@@ -70,4 +70,29 @@ typedef HMAC_CTX hmac_ctx_t;
 #define DES_KEY_LENGTH 8
 #define MD4_DIGEST_LENGTH 	16
 
+/**
+ * Retrieve any occurred OpenSSL errors and print those errors.
+ *
+ * Note that this function uses the not thread-safe OpenSSL error API.
+ *
+ * @param flags		Flags to indicate error type and priority.
+ */
+void crypto_print_openssl_errors(const unsigned int flags);
+
+/**
+ * Retrieve any OpenSSL errors, then print the supplied error message.
+ *
+ * This is just a convenience wrapper for often occurring situations.
+ *
+ * @param flags		Flags to indicate error type and priority.
+ * @param format	Format string to print.
+ * @param format args	(optional) arguments for the format string.
+ */
+# define crypto_msg(flags, ...) \
+do { \
+  crypto_print_openssl_errors(nonfatal(flags)); \
+  msg((flags), __VA_ARGS__); \
+} while (false)
+
+
 #endif /* CRYPTO_OPENSSL_H_ */

src/openvpn/error.h +6 -0

index 1e1f2ac..a977f51 100644
@@ -354,6 +354,12 @@ ignore_sys_error (const int err)
   return false;
 }
 
+/** Convert fatal errors to nonfatal, don't touch other errors */
+static inline const unsigned int
+nonfatal(const unsigned int err) {
+  return err & M_FATAL ? (err ^ M_FATAL) | M_NONFATAL : err;
+}
+
 #include "errlevel.h"
 
 #endif

Signature · 1 line

1.9.1

Gert Doering

2014-12-31 14:31 UTC #7 lore raw

[Openvpn-devel] [PATCH applied] Re: openssl: add crypto_msg(), to easily log openssl errors

Your patch has been applied to the master branch.

commit e795d6ba57e6e79bfae941ab048e44e47179865c

Author: Steffan Karger
Date:   Sat Oct 25 22:35:22 2014 +0200

     openssl: add crypto_msg(), to easily log openssl errors

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/9199
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Steffan Karger

author

2014-10-25 20:35 UTC #8 lore raw

PATCH 5/6 openssl: use crypto_msg(), get rid of openssl-specific code in error.c

In reply to Steffan Karger

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/crypto_openssl.c |  30 +++++++-----
 src/openvpn/error.c          |  29 ------------
 src/openvpn/error.h          |   5 --
 src/openvpn/ssl_openssl.c    | 107 +++++++++++++++++++++++--------------------
 4 files changed, 75 insertions(+), 96 deletions(-)

src/openvpn/crypto_openssl.c +18 ~~-12~~

index 25d8cf9..114a856 100644
@@ -100,13 +100,14 @@ setup_engine (const char *engine)
       if ((e = ENGINE_by_id (engine)) == NULL
 	 && (e = try_load_engine (engine)) == NULL)
 	{
-	  msg (M_FATAL, "OpenSSL error: cannot load engine '%s'", engine);
+	  crypto_msg (M_FATAL, "OpenSSL error: cannot load engine '%s'",
+	      engine);
 	}
 
       if (!ENGINE_set_default (e, ENGINE_METHOD_ALL))
 	{
-	  msg (M_FATAL, "OpenSSL error: ENGINE_set_default failed on engine '%s'",
-	       engine);
+	  crypto_msg (M_FATAL, "OpenSSL error: ENGINE_set_default failed on "
+	      "engine '%s'", engine);
 	}
 
       msg (M_INFO, "Initializing OpenSSL support for engine '%s'",

@@ -395,17 +396,20 @@ key_des_check (uint8_t *key, int key_len, int ndc)
       DES_cblock *dc = (DES_cblock*) buf_read_alloc (&b, sizeof (DES_cblock));
       if (!dc)
 	{
-	  msg (D_CRYPT_ERRORS, "CRYPTO INFO: check_key_DES: insufficient key material");
+	  msg (D_CRYPT_ERRORS, "CRYPTO INFO: check_key_DES: insufficient key "
+	      "material");
 	  goto err;
 	}
       if (DES_is_weak_key(dc))
 	{
-	  msg (D_CRYPT_ERRORS, "CRYPTO INFO: check_key_DES: weak key detected");
+	  crypto_msg (D_CRYPT_ERRORS, "CRYPTO INFO: check_key_DES: weak key "
+	      "detected");
 	  goto err;
 	}
       if (!DES_check_key_parity (dc))
 	{
-	  msg (D_CRYPT_ERRORS, "CRYPTO INFO: check_key_DES: bad parity detected");
+	  crypto_msg (D_CRYPT_ERRORS, "CRYPTO INFO: check_key_DES: bad parity "
+	      "detected");
 	  goto err;
 	}
     }

@@ -454,7 +458,7 @@ cipher_kt_get (const char *ciphername)
   cipher = EVP_get_cipherbyname (ciphername);
 
   if (NULL == cipher)
-    msg (M_SSLERR, "Cipher algorithm '%s' not found", ciphername);
+    crypto_msg (M_FATAL, "Cipher algorithm '%s' not found", ciphername);
 
   if (EVP_CIPHER_key_length (cipher) > MAX_CIPHER_KEY_LENGTH)
     msg (M_FATAL, "Cipher algorithm '%s' uses a default key size (%d bytes) which is larger than " PACKAGE_NAME "'s current maximum key size (%d bytes)",

@@ -538,13 +542,13 @@ cipher_ctx_init (EVP_CIPHER_CTX *ctx, uint8_t *key, int key_len,
 
   EVP_CIPHER_CTX_init (ctx);
   if (!EVP_CipherInit (ctx, kt, NULL, NULL, enc))
-    msg (M_SSLERR, "EVP cipher init #1");
+    crypto_msg (M_FATAL, "EVP cipher init #1");
 #ifdef HAVE_EVP_CIPHER_CTX_SET_KEY_LENGTH
   if (!EVP_CIPHER_CTX_set_key_length (ctx, key_len))
-    msg (M_SSLERR, "EVP set key size");
+    crypto_msg (M_FATAL, "EVP set key size");
 #endif
   if (!EVP_CipherInit (ctx, NULL, key, NULL, enc))
-    msg (M_SSLERR, "EVP cipher init #2");
+    crypto_msg (M_FATAL, "EVP cipher init #2");
 
   /* make sure we used a big enough key */
   ASSERT (EVP_CIPHER_CTX_key_length (ctx) <= key_len);

@@ -626,9 +630,11 @@ md_kt_get (const char *digest)
   ASSERT (digest);
   md = EVP_get_digestbyname (digest);
   if (!md)
-    msg (M_SSLERR, "Message hash algorithm '%s' not found", digest);
+    crypto_msg (M_FATAL, "Message hash algorithm '%s' not found", digest);
   if (EVP_MD_size (md) > MAX_HMAC_KEY_LENGTH)
-    msg (M_FATAL, "Message hash algorithm '%s' uses a default hash size (%d bytes) which is larger than " PACKAGE_NAME "'s current maximum hash size (%d bytes)",
+    crypto_msg (M_FATAL, "Message hash algorithm '%s' uses a default hash size "
+	 "(%d bytes) which is larger than " PACKAGE_NAME "'s current maximum "
+	 "hash size (%d bytes)",
 	 digest,
 	 EVP_MD_size (md),
 	 MAX_HMAC_KEY_LENGTH);

src/openvpn/error.c +0 ~~-29~~

index fd9f19d..9d52962 100644
@@ -43,13 +43,6 @@
 #include "ps.h"
 #include "mstats.h"
 
-#ifdef ENABLE_CRYPTO
-#ifdef ENABLE_CRYPTO_OPENSSL
-#include <openssl/err.h>
-#endif
-#endif
-
-#include "memdbg.h"
 
 #if SYSLOG_CAPABILITY
 #ifndef LOG_OPENVPN

@@ -265,28 +258,6 @@ void x_msg_va (const unsigned int flags, const char *format, va_list arglist)
       SWAP;
     }
 
-#ifdef ENABLE_CRYPTO
-#ifdef ENABLE_CRYPTO_OPENSSL
-  if (flags & M_SSL)
-    {
-      int nerrs = 0;
-      size_t err;
-      while ((err = ERR_get_error ()))
-	{
-	  openvpn_snprintf (m2, ERR_BUF_SIZE, "%s: %s",
-			    m1, ERR_error_string (err, NULL));
-	  SWAP;
-	  ++nerrs;
-	}
-      if (!nerrs)
-	{
-	  openvpn_snprintf (m2, ERR_BUF_SIZE, "%s (OpenSSL)", m1);
-	  SWAP;
-	}
-    }
-#endif
-#endif
-
   if (flags & M_OPTERR)
     {
       openvpn_snprintf (m2, ERR_BUF_SIZE, "Options error: %s", m1);

src/openvpn/error.h +0 -5

index a977f51..d5204f3 100644
@@ -93,10 +93,6 @@ extern int x_msg_line_num;
 
 #define M_ERRNO           (1<<8)	 /* show errno description */
 
-#ifdef ENABLE_CRYPTO_OPENSSL
-#  define M_SSL             (1<<10)	 /* show SSL error */
-#endif
-
 #define M_NOMUTE          (1<<11)        /* don't do mute processing */
 #define M_NOPREFIX        (1<<12)        /* don't show date/time prefix */
 #define M_USAGE_SMALL     (1<<13)        /* fatal options error, call usage_small */

@@ -107,7 +103,6 @@ extern int x_msg_line_num;
 
 /* flag combinations which are frequently used */
 #define M_ERR     (M_FATAL | M_ERRNO)
-#define M_SSLERR  (M_FATAL | M_SSL)
 #define M_USAGE   (M_USAGE_SMALL | M_NOPREFIX | M_OPTERR)
 #define M_CLIENT  (M_MSG_VIRT_OUT | M_NOMUTE | M_NOIPREFIX)

src/openvpn/ssl_openssl.c +57 ~~-50~~

345 lines not on this page · raw message

Signature · 1 line

1.9.1

Gert Doering

2014-12-31 14:30 UTC #9 lore raw

Re: [Openvpn-devel] [PATCH 5/6] openssl: use crypto_msg(), get rid of openssl-specific code in error.c

Hi,

src/openvpn/crypto_openssl.c key_des_check (uint8_t *key, int key_len, int ndc) · line 401

Quoted 8 lines · Steffan Karger

src/openvpn/crypto_openssl.c +2 -1

@@ -395,17 +396,20 @@ key_des_check (uint8_t *key, int key_len, int ndc)
       DES_cblock *dc = (DES_cblock*) buf_read_alloc (&b, sizeof (DES_cblock));
       if (!dc)
 	{
-	  msg (D_CRYPT_ERRORS, "CRYPTO INFO: check_key_DES: insufficient key material");
+	  msg (D_CRYPT_ERRORS, "CRYPTO INFO: check_key_DES: insufficient key "
+	      "material");
 	  goto err;

Shouldn't that be a "crypto_msg()" here?

In general, I'd prefer to avoid breaking strings into multiple lines if it's just for one or two words - I don't think it improves readability, and so it's "just an extra change"...

(Besides these, feature-ACK)

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Steffan Karger

author

2014-12-31 16:36 UTC #10 lore raw

Re: [Openvpn-devel] [PATCH 5/6] openssl: use crypto_msg(), get rid of openssl-specific code in error.c

Quoted 11 lines · Gert Doering

Review key_des_check (uint8_t *key, int key_len, int ndc) · line 401

Quoted 8 lines · Steffan Karger

(quoted diff) +2 -1

@@ -395,17 +396,20 @@ key_des_check (uint8_t *key, int key_len, int ndc)
       DES_cblock *dc = (DES_cblock*) buf_read_alloc (&b, sizeof (DES_cblock));
       if (!dc)
 	{
-	  msg (D_CRYPT_ERRORS, "CRYPTO INFO: check_key_DES: insufficient key material");
+	  msg (D_CRYPT_ERRORS, "CRYPTO INFO: check_key_DES: insufficient key "
+	      "material");
 	  goto err;

Shouldn't that be a "crypto_msg()" here?

Yes, fixed.

Quoted 3 lines

In general, I'd prefer to avoid breaking strings into multiple lines if it's just for one or two words - I don't think it improves readability, and so it's "just an extra change"...

I agree, updated. While I was looking at this again, I also made sure curly braces are used for ifs surrounding multiline crypto_msg() calls.

Attached a v2.

-Steffan

From 0d6c48bc571e76d078903adcbe0f1ba34816de17 Mon Sep 17 00:00:00 2001
From: Steffan Karger <[email protected]...>
Date: Sat, 25 Oct 2014 20:20:13 +0200
Subject: [PATCH] openssl: use crypto_msg(), get rid of openssl-specific code
in error.c

v2 - don't wrap string for a couple of words, and be more consistent
with wrapping (use curly braces for ifs with wrapping statements)

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/crypto_openssl.c |  41 +++++++++------
 src/openvpn/error.c          |  29 -----------
 src/openvpn/error.h          |   5 --
 src/openvpn/ssl_openssl.c    | 117 +++++++++++++++++++++++--------------------
 4 files changed, 89 insertions(+), 103 deletions(-)

src/openvpn/crypto_openssl.c +25 ~~-16~~

index 336994f..2d81a6d 100644
@@ -103,13 +103,15 @@ setup_engine (const char *engine)
       if ((e = ENGINE_by_id (engine)) == NULL
 	 && (e = try_load_engine (engine)) == NULL)
 	{
-	  msg (M_FATAL, "OpenSSL error: cannot load engine '%s'", engine);
+	  crypto_msg (M_FATAL, "OpenSSL error: cannot load engine '%s'",
+	      engine);
 	}
 
       if (!ENGINE_set_default (e, ENGINE_METHOD_ALL))
 	{
-	  msg (M_FATAL, "OpenSSL error: ENGINE_set_default failed on engine '%s'",
-	       engine);
+	  crypto_msg (M_FATAL,
+	      "OpenSSL error: ENGINE_set_default failed on engine '%s'",
+	      engine);
 	}
 
       msg (M_INFO, "Initializing OpenSSL support for engine '%s'",

@@ -393,17 +395,20 @@ key_des_check (uint8_t *key, int key_len, int ndc)
       DES_cblock *dc = (DES_cblock*) buf_read_alloc (&b, sizeof (DES_cblock));
       if (!dc)
 	{
-	  msg (D_CRYPT_ERRORS, "CRYPTO INFO: check_key_DES: insufficient key material");
+	  crypto_msg (D_CRYPT_ERRORS,
+	      "CRYPTO INFO: check_key_DES: insufficient key material");
 	  goto err;
 	}
       if (DES_is_weak_key(dc))
 	{
-	  msg (D_CRYPT_ERRORS, "CRYPTO INFO: check_key_DES: weak key detected");
+	  crypto_msg (D_CRYPT_ERRORS,
+	      "CRYPTO INFO: check_key_DES: weak key detected");
 	  goto err;
 	}
       if (!DES_check_key_parity (dc))
 	{
-	  msg (D_CRYPT_ERRORS, "CRYPTO INFO: check_key_DES: bad parity detected");
+	  crypto_msg (D_CRYPT_ERRORS,
+	      "CRYPTO INFO: check_key_DES: bad parity detected");
 	  goto err;
 	}
     }

@@ -452,7 +457,7 @@ cipher_kt_get (const char *ciphername)
   cipher = EVP_get_cipherbyname (ciphername);
 
   if (NULL == cipher)
-    msg (M_SSLERR, "Cipher algorithm '%s' not found", ciphername);
+    crypto_msg (M_FATAL, "Cipher algorithm '%s' not found", ciphername);
 
   if (EVP_CIPHER_key_length (cipher) > MAX_CIPHER_KEY_LENGTH)
     msg (M_FATAL, "Cipher algorithm '%s' uses a default key size (%d bytes) which is larger than " PACKAGE_NAME "'s current maximum key size (%d bytes)",

@@ -536,13 +541,13 @@ cipher_ctx_init (EVP_CIPHER_CTX *ctx, uint8_t *key, int key_len,
 
   EVP_CIPHER_CTX_init (ctx);
   if (!EVP_CipherInit (ctx, kt, NULL, NULL, enc))
-    msg (M_SSLERR, "EVP cipher init #1");
+    crypto_msg (M_FATAL, "EVP cipher init #1");
 #ifdef HAVE_EVP_CIPHER_CTX_SET_KEY_LENGTH
   if (!EVP_CIPHER_CTX_set_key_length (ctx, key_len))
-    msg (M_SSLERR, "EVP set key size");
+    crypto_msg (M_FATAL, "EVP set key size");
 #endif
   if (!EVP_CipherInit (ctx, NULL, key, NULL, enc))
-    msg (M_SSLERR, "EVP cipher init #2");
+    crypto_msg (M_FATAL, "EVP cipher init #2");
 
   /* make sure we used a big enough key */
   ASSERT (EVP_CIPHER_CTX_key_length (ctx) <= key_len);

@@ -589,7 +594,9 @@ int
 cipher_ctx_update (EVP_CIPHER_CTX *ctx, uint8_t *dst, int *dst_len,
     uint8_t *src, int src_len)
 {
-  return EVP_CipherUpdate (ctx, dst, dst_len, src, src_len);
+  if (!EVP_CipherUpdate (ctx, dst, dst_len, src, src_len))
+    crypto_msg(M_FATAL, "%s: EVP_CipherUpdate() failed", __func__);
+  return 1;
 }
 
 int

@@ -624,12 +631,14 @@ md_kt_get (const char *digest)
   ASSERT (digest);
   md = EVP_get_digestbyname (digest);
   if (!md)
-    msg (M_SSLERR, "Message hash algorithm '%s' not found", digest);
+    crypto_msg (M_FATAL, "Message hash algorithm '%s' not found", digest);
   if (EVP_MD_size (md) > MAX_HMAC_KEY_LENGTH)
-    msg (M_FATAL, "Message hash algorithm '%s' uses a default hash size (%d bytes) which is larger than " PACKAGE_NAME "'s current maximum hash size (%d bytes)",
-	 digest,
-	 EVP_MD_size (md),
-	 MAX_HMAC_KEY_LENGTH);
+    {
+      crypto_msg (M_FATAL, "Message hash algorithm '%s' uses a default hash "
+	  "size (%d bytes) which is larger than " PACKAGE_NAME "'s current "
+	  "maximum hash size (%d bytes)",
+	  digest, EVP_MD_size (md), MAX_HMAC_KEY_LENGTH);
+    }
   return md;
 }

src/openvpn/error.c +0 ~~-29~~

index fd9f19d..9d52962 100644
@@ -43,13 +43,6 @@
 #include "ps.h"
 #include "mstats.h"
 
-#ifdef ENABLE_CRYPTO
-#ifdef ENABLE_CRYPTO_OPENSSL
-#include <openssl/err.h>
-#endif
-#endif
-
-#include "memdbg.h"
 
 #if SYSLOG_CAPABILITY
 #ifndef LOG_OPENVPN

@@ -265,28 +258,6 @@ void x_msg_va (const unsigned int flags, const char *format, va_list arglist)
       SWAP;
     }
 
-#ifdef ENABLE_CRYPTO
-#ifdef ENABLE_CRYPTO_OPENSSL
-  if (flags & M_SSL)
-    {
-      int nerrs = 0;
-      size_t err;
-      while ((err = ERR_get_error ()))
-	{
-	  openvpn_snprintf (m2, ERR_BUF_SIZE, "%s: %s",
-			    m1, ERR_error_string (err, NULL));
-	  SWAP;
-	  ++nerrs;
-	}
-      if (!nerrs)
-	{
-	  openvpn_snprintf (m2, ERR_BUF_SIZE, "%s (OpenSSL)", m1);
-	  SWAP;
-	}
-    }
-#endif
-#endif
-
   if (flags & M_OPTERR)
     {
       openvpn_snprintf (m2, ERR_BUF_SIZE, "Options error: %s", m1);

src/openvpn/error.h +0 -5

index a977f51..d5204f3 100644
@@ -93,10 +93,6 @@ extern int x_msg_line_num;
 
 #define M_ERRNO           (1<<8)	 /* show errno description */
 
-#ifdef ENABLE_CRYPTO_OPENSSL
-#  define M_SSL             (1<<10)	 /* show SSL error */
-#endif
-
 #define M_NOMUTE          (1<<11)        /* don't do mute processing */
 #define M_NOPREFIX        (1<<12)        /* don't show date/time prefix */
 #define M_USAGE_SMALL     (1<<13)        /* fatal options error, call usage_small */

@@ -107,7 +103,6 @@ extern int x_msg_line_num;
 
 /* flag combinations which are frequently used */
 #define M_ERR     (M_FATAL | M_ERRNO)
-#define M_SSLERR  (M_FATAL | M_SSL)
 #define M_USAGE   (M_USAGE_SMALL | M_NOPREFIX | M_OPTERR)
 #define M_CLIENT  (M_MSG_VIRT_OUT | M_NOMUTE | M_NOIPREFIX)

src/openvpn/ssl_openssl.c +64 ~~-53~~

349 lines not on this page · raw message

Signature · 1 line

2.1.0

Steffan Karger

author

2014-12-31 17:21 UTC #11 lore raw

Re: [Openvpn-devel] [PATCH 5/6] openssl: use crypto_msg(), get rid of openssl-specific code in error.c

Hi,

Quoted 1 line · Steffan Karger

Attached a v2.

... and a v3 that changes an incorrect crypto_msg() call back to msg() (as discussed on #openvpn-devel).

-Steffan

From b2cfda494ec450605994874e547778f05f06b505 Mon Sep 17 00:00:00 2001
From: Steffan Karger <[email protected]...>
Date: Sat, 25 Oct 2014 20:20:13 +0200
Subject: [PATCH] openssl: use crypto_msg(), get rid of openssl-specific code
in error.c

v2 - don't wrap string for a couple of words, and be more consistent
with wrapping (use curly braces for ifs with wrapping statements)

v3 - change an incorrect crypto_msg() call back to msg()

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/crypto_openssl.c |  41 +++++++++------
 src/openvpn/error.c          |  29 -----------
 src/openvpn/error.h          |   5 --
 src/openvpn/ssl_openssl.c    | 117 +++++++++++++++++++++++--------------------
 4 files changed, 89 insertions(+), 103 deletions(-)

src/openvpn/crypto_openssl.c +25 ~~-16~~

index 336994f..2d81a6d 100644
@@ -103,13 +103,15 @@ setup_engine (const char *engine)
       if ((e = ENGINE_by_id (engine)) == NULL
 	 && (e = try_load_engine (engine)) == NULL)
 	{
-	  msg (M_FATAL, "OpenSSL error: cannot load engine '%s'", engine);
+	  crypto_msg (M_FATAL, "OpenSSL error: cannot load engine '%s'",
+	      engine);
 	}
 
       if (!ENGINE_set_default (e, ENGINE_METHOD_ALL))
 	{
-	  msg (M_FATAL, "OpenSSL error: ENGINE_set_default failed on engine '%s'",
-	       engine);
+	  crypto_msg (M_FATAL,
+	      "OpenSSL error: ENGINE_set_default failed on engine '%s'",
+	      engine);
 	}
 
       msg (M_INFO, "Initializing OpenSSL support for engine '%s'",

@@ -393,17 +395,20 @@ key_des_check (uint8_t *key, int key_len, int ndc)
       DES_cblock *dc = (DES_cblock*) buf_read_alloc (&b, sizeof (DES_cblock));
       if (!dc)
 	{
-	  msg (D_CRYPT_ERRORS, "CRYPTO INFO: check_key_DES: insufficient key material");
+	  crypto_msg (D_CRYPT_ERRORS,
+	      "CRYPTO INFO: check_key_DES: insufficient key material");
 	  goto err;
 	}
       if (DES_is_weak_key(dc))
 	{
-	  msg (D_CRYPT_ERRORS, "CRYPTO INFO: check_key_DES: weak key detected");
+	  crypto_msg (D_CRYPT_ERRORS,
+	      "CRYPTO INFO: check_key_DES: weak key detected");
 	  goto err;
 	}
       if (!DES_check_key_parity (dc))
 	{
-	  msg (D_CRYPT_ERRORS, "CRYPTO INFO: check_key_DES: bad parity detected");
+	  crypto_msg (D_CRYPT_ERRORS,
+	      "CRYPTO INFO: check_key_DES: bad parity detected");
 	  goto err;
 	}
     }

@@ -452,7 +457,7 @@ cipher_kt_get (const char *ciphername)
   cipher = EVP_get_cipherbyname (ciphername);
 
   if (NULL == cipher)
-    msg (M_SSLERR, "Cipher algorithm '%s' not found", ciphername);
+    crypto_msg (M_FATAL, "Cipher algorithm '%s' not found", ciphername);
 
   if (EVP_CIPHER_key_length (cipher) > MAX_CIPHER_KEY_LENGTH)
     msg (M_FATAL, "Cipher algorithm '%s' uses a default key size (%d bytes) which is larger than " PACKAGE_NAME "'s current maximum key size (%d bytes)",

@@ -536,13 +541,13 @@ cipher_ctx_init (EVP_CIPHER_CTX *ctx, uint8_t *key, int key_len,
 
   EVP_CIPHER_CTX_init (ctx);
   if (!EVP_CipherInit (ctx, kt, NULL, NULL, enc))
-    msg (M_SSLERR, "EVP cipher init #1");
+    crypto_msg (M_FATAL, "EVP cipher init #1");
 #ifdef HAVE_EVP_CIPHER_CTX_SET_KEY_LENGTH
   if (!EVP_CIPHER_CTX_set_key_length (ctx, key_len))
-    msg (M_SSLERR, "EVP set key size");
+    crypto_msg (M_FATAL, "EVP set key size");
 #endif
   if (!EVP_CipherInit (ctx, NULL, key, NULL, enc))
-    msg (M_SSLERR, "EVP cipher init #2");
+    crypto_msg (M_FATAL, "EVP cipher init #2");
 
   /* make sure we used a big enough key */
   ASSERT (EVP_CIPHER_CTX_key_length (ctx) <= key_len);

@@ -589,7 +594,9 @@ int
 cipher_ctx_update (EVP_CIPHER_CTX *ctx, uint8_t *dst, int *dst_len,
     uint8_t *src, int src_len)
 {
-  return EVP_CipherUpdate (ctx, dst, dst_len, src, src_len);
+  if (!EVP_CipherUpdate (ctx, dst, dst_len, src, src_len))
+    crypto_msg(M_FATAL, "%s: EVP_CipherUpdate() failed", __func__);
+  return 1;
 }
 
 int

@@ -624,12 +631,14 @@ md_kt_get (const char *digest)
   ASSERT (digest);
   md = EVP_get_digestbyname (digest);
   if (!md)
-    msg (M_SSLERR, "Message hash algorithm '%s' not found", digest);
+    crypto_msg (M_FATAL, "Message hash algorithm '%s' not found", digest);
   if (EVP_MD_size (md) > MAX_HMAC_KEY_LENGTH)
-    msg (M_FATAL, "Message hash algorithm '%s' uses a default hash size (%d bytes) which is larger than " PACKAGE_NAME "'s current maximum hash size (%d bytes)",
-	 digest,
-	 EVP_MD_size (md),
-	 MAX_HMAC_KEY_LENGTH);
+    {
+      crypto_msg (M_FATAL, "Message hash algorithm '%s' uses a default hash "
+	  "size (%d bytes) which is larger than " PACKAGE_NAME "'s current "
+	  "maximum hash size (%d bytes)",
+	  digest, EVP_MD_size (md), MAX_HMAC_KEY_LENGTH);
+    }
   return md;
 }

src/openvpn/error.c +0 ~~-29~~

index fd9f19d..9d52962 100644
@@ -43,13 +43,6 @@
 #include "ps.h"
 #include "mstats.h"
 
-#ifdef ENABLE_CRYPTO
-#ifdef ENABLE_CRYPTO_OPENSSL
-#include <openssl/err.h>
-#endif
-#endif
-
-#include "memdbg.h"
 
 #if SYSLOG_CAPABILITY
 #ifndef LOG_OPENVPN

@@ -265,28 +258,6 @@ void x_msg_va (const unsigned int flags, const char *format, va_list arglist)
       SWAP;
     }
 
-#ifdef ENABLE_CRYPTO
-#ifdef ENABLE_CRYPTO_OPENSSL
-  if (flags & M_SSL)
-    {
-      int nerrs = 0;
-      size_t err;
-      while ((err = ERR_get_error ()))
-	{
-	  openvpn_snprintf (m2, ERR_BUF_SIZE, "%s: %s",
-			    m1, ERR_error_string (err, NULL));
-	  SWAP;
-	  ++nerrs;
-	}
-      if (!nerrs)
-	{
-	  openvpn_snprintf (m2, ERR_BUF_SIZE, "%s (OpenSSL)", m1);
-	  SWAP;
-	}
-    }
-#endif
-#endif
-
   if (flags & M_OPTERR)
     {
       openvpn_snprintf (m2, ERR_BUF_SIZE, "Options error: %s", m1);

src/openvpn/error.h +0 -5

index a977f51..d5204f3 100644
@@ -93,10 +93,6 @@ extern int x_msg_line_num;
 
 #define M_ERRNO           (1<<8)	 /* show errno description */
 
-#ifdef ENABLE_CRYPTO_OPENSSL
-#  define M_SSL             (1<<10)	 /* show SSL error */
-#endif
-
 #define M_NOMUTE          (1<<11)        /* don't do mute processing */
 #define M_NOPREFIX        (1<<12)        /* don't show date/time prefix */
 #define M_USAGE_SMALL     (1<<13)        /* fatal options error, call usage_small */

@@ -107,7 +103,6 @@ extern int x_msg_line_num;
 
 /* flag combinations which are frequently used */
 #define M_ERR     (M_FATAL | M_ERRNO)
-#define M_SSLERR  (M_FATAL | M_SSL)
 #define M_USAGE   (M_USAGE_SMALL | M_NOPREFIX | M_OPTERR)
 #define M_CLIENT  (M_MSG_VIRT_OUT | M_NOMUTE | M_NOIPREFIX)

src/openvpn/ssl_openssl.c +64 ~~-53~~

349 lines not on this page · raw message

Signature · 1 line

2.1.0

Gert Doering

2014-12-31 19:25 UTC #12 lore raw

[Openvpn-devel] [PATCH applied] Re: openssl: use crypto_msg(), get rid of openssl-specific code in error.c

ACK. Your patch has been applied to the master branch.

commit 98ea2ec5d8085a6b7bd4ac125a68bd4d5cf3e092

Author: Steffan Karger
Date:   Sat Oct 25 20:20:13 2014 +0200

     openssl: use crypto_msg(), get rid of openssl-specific code in error.c

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/9374
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Steffan Karger

author

2014-10-25 20:35 UTC #13 lore raw

PATCH 6/6 openssl: add more descriptive message for "no shared cipher" error

In reply to Steffan Karger

Overzealous users using the --tls-cipher option, or users with actual incompatible crypto libaries often waste quite some time debugging the "no shared cipher" error from openssl. See e.g. trac ticket #359:
https://community.openvpn.net/openvpn/ticket/359

This change adds a more clear, verb 1 error message reporting the problem directly to the user, instead of just printing the openssl error.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/crypto_openssl.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

src/openvpn/crypto_openssl.c +13 -1

index 114a856..6c02d9c 100644
@@ -45,6 +45,7 @@
 #include <openssl/objects.h>
 #include <openssl/evp.h>
 #include <openssl/des.h>
+#include <openssl/ssl.h>
 
 /*
  * Check for key size creepage.

@@ -201,7 +202,18 @@ crypto_print_openssl_errors(const unsigned int flags) {
   size_t err = 0;
 
   while ((err = ERR_get_error ()))
-    msg (flags, "OpenSSL: %s", ERR_error_string (err, NULL));
+    {
+      /* Be more clear about frequently occurring "no shared cipher" error */
+      if (err == ERR_PACK(ERR_LIB_SSL,SSL_F_SSL3_GET_CLIENT_HELLO,
+	  SSL_R_NO_SHARED_CIPHER))
+	{
+	  msg (D_CRYPT_ERRORS, "TLS error: The server has no TLS ciphersuites "
+	      "in common with the client. Your --tls-cipher setting might be "
+	      "too restrictive.");
+	}
+
+      msg (flags, "OpenSSL: %s", ERR_error_string (err, NULL));
+    }
 }

Signature · 1 line

1.9.1

Arne Schwabe

2014-10-27 20:15 UTC #14 lore raw

Re: [Openvpn-devel] [PATCH 6/6] openssl: add more descriptive message for "no shared cipher" error

Am 25.10.14 22:35, schrieb Steffan Karger:

Quoted 9 lines

Overzealous users using the --tls-cipher option, or users with actual incompatible crypto libaries often waste quite some time debugging the "no shared cipher" error from openssl. See e.g. trac ticket #359:
https://community.openvpn.net/openvpn/ticket/359
This change adds a more clear, verb 1 error message reporting the problem directly to the user, instead of just printing the openssl error.
S

ACK.

Arne

Steffan Karger

author

2014-10-27 20:55 UTC #15 lore raw

Re: [Openvpn-devel] [PATCH 6/6] openssl: add more descriptive message for "no shared cipher" error

Quoted 11 lines · Arne Schwabe

Am 25.10.14 22:35, schrieb Steffan Karger:
Quoted 9 lines
Overzealous users using the --tls-cipher option, or users with actual incompatible crypto libaries often waste quite some time debugging the "no shared cipher" error from openssl. See e.g. trac ticket #359:
https://community.openvpn.net/openvpn/ticket/359
This change adds a more clear, verb 1 error message reporting the problem directly to the user, instead of just printing the openssl error.
S
ACK.

Thanks. Attached a v2 of this patch. Only difference is that adds an #include <openssl/err.h> (and reorders the openssl includes since I was touching them now anyway). On my machine (ubuntu 14.04) both gcc and clang compile happily without the extra include, but Arne reported (off-list) that without the include it breaks while compiling for Android.

-Steffan

From 12aa3470bb5881f3b4fddab4d13852b1e38e3715 Mon Sep 17 00:00:00 2001
From: Steffan Karger <[email protected]...>
Date: Sat, 25 Oct 2014 20:49:26 +0200
Subject: [PATCH 6/6] openssl: add more descriptive message for "no shared
cipher" error

This change adds a more clear, verb 1 error message reporting the problem directly to the user, instead of just printing the openssl error.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/crypto_openssl.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

src/openvpn/crypto_openssl.c +17 -3

index 114a856..cc00a7d 100644
@@ -42,9 +42,12 @@
 #include "integer.h"
 #include "crypto.h"
 #include "crypto_backend.h"
-#include <openssl/objects.h>
-#include <openssl/evp.h>
+
 #include <openssl/des.h>
+#include <openssl/err.h>
+#include <openssl/evp.h>
+#include <openssl/objects.h>
+#include <openssl/ssl.h>
 
 /*
  * Check for key size creepage.

@@ -201,7 +204,18 @@ crypto_print_openssl_errors(const unsigned int flags) {
   size_t err = 0;
 
   while ((err = ERR_get_error ()))
-    msg (flags, "OpenSSL: %s", ERR_error_string (err, NULL));
+    {
+      /* Be more clear about frequently occurring "no shared cipher" error */
+      if (err == ERR_PACK(ERR_LIB_SSL,SSL_F_SSL3_GET_CLIENT_HELLO,
+	  SSL_R_NO_SHARED_CIPHER))
+	{
+	  msg (D_CRYPT_ERRORS, "TLS error: The server has no TLS ciphersuites "
+	      "in common with the client. Your --tls-cipher setting might be "
+	      "too restrictive.");
+	}
+
+      msg (flags, "OpenSSL: %s", ERR_error_string (err, NULL));
+    }
 }

Signature · 1 line

1.9.1

Gert Doering

2014-12-31 14:31 UTC #16 lore raw

[Openvpn-devel] [PATCH applied] Re: openssl: add more descriptive message for 'no shared cipher' error

Your patch has been applied to the master branch.

commit c3e1809f540db16c23fc74f06d6e8c29a4a6941a

Author: Steffan Karger
Date:   Sat Oct 25 20:49:26 2014 +0200

     openssl: add more descriptive message for 'no shared cipher' error

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Arne Schwabe <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/9209
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering