[Openvpn-devel,1/1] Let mbedtls_ssl_configs find reloaded CRLs

Message ID E1lSI8m-0000tY-FA@sfs-ml-4.v29.lw.sourceforge.com
State Changes Requested
Headers show
Series
  • CRL reloading issue with mbedTLS and chroot
Related show

Commit Message

Max Fillinger April 2, 2021, 11:24 a.m.
From: Maximilian Fillinger <maximilian.fillinger@fox-it.com>

If the CRL file cannot be read during initialization, a NULL pointer is
passed to the mbedtls_ssl_config in key_state_ssl_init(). Then, if the
CRL file is successfully read later, the config won't have a pointer to
it. Therefore, the CRL won't actually take effect.

This commit fixes the bug by creating an empty CRL if crl-verify is in
the config, but the file cannot be read during initialization. That way,
we can always give the mbedtls_ssl_config struct a pointer to the
location where the CRL will be if it is successfully read later.

This commit also fixes an additional issue:

When a CRL file is present but cannot be parsed, OpenVPN rejects all
incoming connections. When the CRL file cannot be stat'ed, OpenVPN keeps
using the previous CRL. This commit makes it so that OpenVPN rejects
connections in both situations.
---
 src/openvpn/ssl.c         | 11 +++++++++++
 src/openvpn/ssl_mbedtls.c | 13 +++++++++++++
 src/openvpn/ssl_mbedtls.h |  3 +++
 3 files changed, 27 insertions(+)

Comments

Arne Schwabe April 3, 2021, 12:08 p.m. | #1
>  }
>  
>  void
> +make_empty_crl(struct tls_root_ctx *ctx)
> +{
> +    if (ctx->crl == NULL)
> +    {
> +        ALLOC_OBJ_CLEAR(ctx->crl, mbedtls_x509_crl);
> +    }
> +    else
> +    {
> +        mbedtls_x509_crl_free(ctx->crl);
> +    }
> +}
> +

This function is confsung me. This needs at least more documentation
what it is doing as docstring etc. I would also expect to have
mbedtls_x509_crl_init to init the struct and not just a malloc that set
the whole structure to zero. And mbedtls_x509_crl_free does from its
description doesn't guarantee that the object is left in a proper state.

Arne
Max Fillinger April 6, 2021, 5:12 p.m. | #2
> >  }
> >
> >  void
> > +make_empty_crl(struct tls_root_ctx *ctx)
> > +{
> > +    if (ctx->crl == NULL)
> > +    {
> > +        ALLOC_OBJ_CLEAR(ctx->crl, mbedtls_x509_crl);
> > +    }
> > +    else
> > +    {
> > +        mbedtls_x509_crl_free(ctx->crl);
> > +    }
> > +}
> > +
> 
> This function is confsung me. This needs at least more documentation
> what it is doing as docstring etc. I would also expect to have
> mbedtls_x509_crl_init to init the struct and not just a malloc that set
> the whole structure to zero. And mbedtls_x509_crl_free does from its
> description doesn't guarantee that the object is left in a proper state.

All mbedtls_x509_crl_init() does is set the struct to 0, but I agree it's
better from a readability standpoint to call it anyway. Another function in
the file also uses ALLOC_OBJ_CLEAR instead of the init function; I'll fix
that too.

When OpenVPN fails to parse a CRL file, it also calls mbedtls_x509_crl_free()
and leaves it at that (see backend_tls_ctx_reload_crl()). This leaves it in
a state where all connection attempts are rejected. The same happens when you
initialize a CRL struct and then don't do anything further with it. So that's
what I did in this function. You're right, this needs more comments. If we want
to do it that way at all.

As far as I'm aware, none of that is documented. Actually, if I understand correctly,
you're not even supposed to touch the mbedtls_ssl_config struct (which contains
a pointer to the CRL) after calling mbedtls_ssl_setup(). So far, reloading the CRL
has worked anyway. It looks like the code in ssl_mbedtls.c needs some rewriting if
we want to do it properly. I'll need to think about that some more.

Patch

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 08222b5e..f20771d7 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -559,6 +559,17 @@  tls_ctx_reload_crl(struct tls_root_ctx *ssl_ctx, const char *crl_file,
     else if (platform_stat(crl_file, &crl_stat) < 0)
     {
         msg(M_WARN, "WARNING: Failed to stat CRL file, not (re)loading CRL.");
+
+#ifdef ENABLE_CRYPTO_MBEDTLS
+        /* Store an empty CRL in ssl_ctx. This is so that we can give the
+         * mbedtls_ssl_configs in the key_states a pointer to the location where
+         * the CRL will be if we successfully reload the file later.
+         *
+         * Storing an empty CRL also causes all connection attempts to be rejected
+         * until an actual CRL is loaded. */
+        make_empty_crl(ssl_ctx);
+#endif
+
         return;
     }
 
diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
index 4626e983..5d7af351 100644
--- a/src/openvpn/ssl_mbedtls.c
+++ b/src/openvpn/ssl_mbedtls.c
@@ -1044,6 +1044,19 @@  err:
 }
 
 void
+make_empty_crl(struct tls_root_ctx *ctx)
+{
+    if (ctx->crl == NULL)
+    {
+        ALLOC_OBJ_CLEAR(ctx->crl, mbedtls_x509_crl);
+    }
+    else
+    {
+        mbedtls_x509_crl_free(ctx->crl);
+    }
+}
+
+void
 key_state_ssl_init(struct key_state_ssl *ks_ssl,
                    const struct tls_root_ctx *ssl_ctx, bool is_server,
                    struct tls_session *session)
diff --git a/src/openvpn/ssl_mbedtls.h b/src/openvpn/ssl_mbedtls.h
index ff64e17c..579e3c8e 100644
--- a/src/openvpn/ssl_mbedtls.h
+++ b/src/openvpn/ssl_mbedtls.h
@@ -144,4 +144,7 @@  int tls_ctx_use_external_signing_func(struct tls_root_ctx *ctx,
                                       external_sign_func sign_func,
                                       void *sign_ctx);
 
+void make_empty_crl(struct tls_root_ctx *);
+
+
 #endif /* SSL_MBEDTLS_H_ */