Hi Max,
On 07/04/2021 21:15, Max Fillinger wrote:
> This commit fixes the following two issues:
>
> The config belonging to a mbedtls_ssl_ctx struct is not supposed to be
> changed after mbedtls_ssl_setup() has been called. Previously, we
> modified the CRL structure in place when a new CRL was loaded, but a
> pointer to this struct appears in configs that are currently in use.
>
> This commit uses reference counting to ensure that CRLs remain in memory
> while the mbedtls_ssl_ctx struct and config remain.
>
To fix this issue, wouldn't be enough to reload the CRL *before* calling
mbedtls_ssl_setup()?
This way we know that a new ssl_ctx is always initialized with the
latest CRL, rather than having the old one still around?
I have attached a proposal for your review. Please let me know what you
think about it.
As a side note: how did you notice this issue?
> The other issue fixed by this commit is that, if OpenVPN failed to read
> the CRL in init_ssl(), but succeeded in reading it later, it would not
> actually use that CRL.
Mh... I have tried to reproduce this issue, but I was not able to.
Would you be able to provide me with the exact steps to hit this situation?
What I tried was:
1) create empty file crl.pem;
2) start server with --crl-verify crl.pem;
3) connect with client -> connection denied due to verification issue
(VERIFY ERROR: CRL not loaded);
4) I overwrote crl.pem with a real CRL (without restarting the server);
5) I connected with my client and it worked;
6) I connected with a certificate in the CRL -> connection denied.
Btw, Signed-off-by line is missing - a patch submitted for inclusion
should always have one.
Regards,
--
Antonio Quartulli
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index d8662d00..eda7fae1 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -899,6 +899,17 @@ key_state_init(struct tls_session *session, struct key_state *ks)
CLEAR(*ks);
+ /*
+ * Attempt CRL reload before any TLS operation. Won't be performed if
+ * the file was not modified since the last reload
+ */
+ if (session->opt->crl_file
+ && !(session->opt->ssl_flags & SSLF_CRL_VERIFY_DIR))
+ {
+ tls_ctx_reload_crl(&session->opt->ssl_ctx, session->opt->crl_file,
+ session->opt->crl_file_inline);
+ }
+
/*
* Build TLS object that reads/writes ciphertext
* to/from memory BIOs.
@@ -2728,17 +2739,6 @@ tls_process(struct tls_multi *multi,
ks->state = S_START;
state_change = true;
- /*
- * Attempt CRL reload before TLS negotiation. Won't be performed if
- * the file was not modified since the last reload
- */
- if (session->opt->crl_file
- && !(session->opt->ssl_flags & SSLF_CRL_VERIFY_DIR))
- {
- tls_ctx_reload_crl(&session->opt->ssl_ctx,
- session->opt->crl_file, session->opt->crl_file_inline);
- }
-
/* New connection, remove any old X509 env variables */
tls_x509_clear_env(session->opt->es);
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
