Skip to contentSkip to conversation

Fix regression in setups without a client certificate

PATCH Superseded Steffan Karger 2 participants 3 messages Jan 3, 2016

lore
Share & tools
lore

Steffan Karger

author
#0 lore raw

PATCH 1/1 Fix regression in setups without a client certificate

This fixes a null-pointer dereference in tls_ctx_cert_time(), which will occur on clients that do not use a client certificate (ie that only have auth-user-pass in the config, but no key and cert). This bug was introduced by commit 091edd8e on the master branch, and commit dfd940bb on the release/2.3 branch (which is part of 2.3.9).

While touching this function, I also made this function conform to the openvpn coding style.

Signed-off-by Steffan Karger <[email protected]...>
---
 src/openvpn/ssl_openssl.c  | 15 +++++++++++----
 src/openvpn/ssl_polarssl.c |  6 ++++++
 2 files changed, 17 insertions(+), 4 deletions(-)
src/openvpn/ssl_openssl.c +11 -4
index 0a7f14b..5cf6d7f 100644
@@ -356,15 +356,22 @@ tls_ctx_check_cert_time (const struct tls_root_ctx *ctx)
   int ret;
   const X509 *cert;
 
+  ASSERT (ctx);
+
 #if OPENSSL_VERSION_NUMBER >= 0x10002000L
   /* OpenSSL 1.0.2 and up */
-  cert = SSL_CTX_get0_certificate(ctx->ctx);
+  cert = SSL_CTX_get0_certificate (ctx->ctx);
 #else
   /* OpenSSL 1.0.1 and earlier need an SSL object to get at the certificate */
-  SSL *ssl = SSL_new(ctx->ctx);
-  cert = SSL_get_certificate(ssl);
+  SSL *ssl = SSL_new (ctx->ctx);
+  cert = SSL_get_certificate (ssl);
 #endif
 
+  if (cert == NULL)
+    {
+      return; /* Nothing to check if there is no certificate */
+    }
+
   ret = X509_cmp_time (X509_get_notBefore (cert), NULL);
   if (ret == 0)
     {

@@ -385,7 +392,7 @@ tls_ctx_check_cert_time (const struct tls_root_ctx *ctx)
       msg (M_WARN, "WARNING: Your certificate has expired!");
     }
 #if OPENSSL_VERSION_NUMBER < 0x10002000L
-  SSL_free(ssl);
+  SSL_free (ssl);
 #endif
 }
src/openvpn/ssl_polarssl.c +6 -0
index 4782469..58b2116 100644
@@ -219,6 +219,12 @@ tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers)
 void
 tls_ctx_check_cert_time (const struct tls_root_ctx *ctx)
 {
+  ASSERT (ctx);
+  if (ctx->crt_chain == NULL)
+    {
+      return; /* Nothing to check if there is no certificate */
+    }
+
   if (x509_time_future (&ctx->crt_chain->valid_from))
     {
       msg (M_WARN, "WARNING: Your certificate is not yet valid!");
Signature · 1 line
2.5.0
author
#1 lore raw

PATCH v2 1/1 Fix regression in setups without a client certificate

This fixes a null-pointer dereference in tls_ctx_cert_time(), which will occur on clients that do not use a client certificate (ie that only have auth-user-pass in the config, but no key and cert). This bug was introduced by commit 091edd8e on the master branch, and commit dfd940bb on the release/2.3 branch.

This bug was found by chipitsine and reported in trac ticket #644.

While touching this function, I also made this function conform to the openvpn coding style.

v2 - fix memory leak in builds using pre-1.0.2 openssl

Signed-off-by Steffan Karger <[email protected]...>
---
 src/openvpn/ssl_openssl.c  | 18 ++++++++++++++----
 src/openvpn/ssl_polarssl.c |  6 ++++++
 2 files changed, 20 insertions(+), 4 deletions(-)
src/openvpn/ssl_openssl.c +14 -4
index 0a7f14b..d2f40e7 100644
@@ -356,15 +356,22 @@ tls_ctx_check_cert_time (const struct tls_root_ctx *ctx)
   int ret;
   const X509 *cert;
 
+  ASSERT (ctx);
+
 #if OPENSSL_VERSION_NUMBER >= 0x10002000L
   /* OpenSSL 1.0.2 and up */
-  cert = SSL_CTX_get0_certificate(ctx->ctx);
+  cert = SSL_CTX_get0_certificate (ctx->ctx);
 #else
   /* OpenSSL 1.0.1 and earlier need an SSL object to get at the certificate */
-  SSL *ssl = SSL_new(ctx->ctx);
-  cert = SSL_get_certificate(ssl);
+  SSL *ssl = SSL_new (ctx->ctx);
+  cert = SSL_get_certificate (ssl);
 #endif
 
+  if (cert == NULL)
+    {
+      goto cleanup; /* Nothing to check if there is no certificate */
+    }
+
   ret = X509_cmp_time (X509_get_notBefore (cert), NULL);
   if (ret == 0)
     {

@@ -384,9 +391,12 @@ tls_ctx_check_cert_time (const struct tls_root_ctx *ctx)
     {
       msg (M_WARN, "WARNING: Your certificate has expired!");
     }
+
+cleanup:
 #if OPENSSL_VERSION_NUMBER < 0x10002000L
-  SSL_free(ssl);
+  SSL_free (ssl);
 #endif
+  return;
 }
 
 void
src/openvpn/ssl_polarssl.c +6 -0
index 4782469..58b2116 100644
@@ -219,6 +219,12 @@ tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers)
 void
 tls_ctx_check_cert_time (const struct tls_root_ctx *ctx)
 {
+  ASSERT (ctx);
+  if (ctx->crt_chain == NULL)
+    {
+      return; /* Nothing to check if there is no certificate */
+    }
+
   if (x509_time_future (&ctx->crt_chain->valid_from))
     {
       msg (M_WARN, "WARNING: Your certificate is not yet valid!");
Signature · 1 line
2.5.0
#2 lore raw

ACK, thanks.

Your patch has been applied to the master and release/2.3 branch.

commit 868d9d01802da9bbbb3a758981f3c7310a905813 (master) commit f4bf11daa8d659e74915c308930145963867d64c (master)

Author: Steffan Karger
Date:   Sun Jan 3 10:47:56 2016 +0100
     Fix regression in setups without a client certificate

     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/10921
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering