| 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 | expand |
> } > > 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
> > } > > > > 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.
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_ */
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(+)