ssl_openssl: Fix some CRL mixups #844
Conversation
There are two ways to load CRLs in OpenSSL. They can be loaded at the X509_STORE, shared across verifications, or loaded per verification at the X509_STORE_CTX. OpenVPN currently does the former. However, it also supports CRL reloading, and tries to reload the CRL file before each connection. OpenSSL does not really have a good way to unload objects from an X509_STORE. OpenVPN currently does it by grabbing the STACK_OF(X509_OBJECT) out of the X509_STORE and manually deleting all the CRLs from it. This mutates an OpenSSL internal object which bumps into problems if OpenSSL ever switches to a more efficient representation. See openssl/openssl#28599 (It's also not thread-safe, though it doesn't look like that impacts OpenVPN? Actually even reading that list doesn't work. See CVE-2024-0397. This OpenSSL API was simply broken.) Additionally, this seems to cause two OpenVPN features to not work together. I gather backend_tls_ctx_reload_crl is trying to clear the CRLs loaded from last time it ran. But tls_ctx_load_ca with a ca_file can also load CRLs. tls_ctx_load_ca with ca_path will also pick up CRLs and backend_tls_ctx_reload_crl actually ends up clobbering some state X509_LOOKUP_hash_dir internally maintains on the X509_STORE. Likewise, tls_verify_crl_missing can get confused between backend_tls_ctx_reload_crl's crl_file-based CRLs and CRLs from tls_ctx_load_ca. Avoid all this by tracking the two CRLs separately. crl_file-based CRLs now go onto a STACK_OF(X509_CRL) tracked on the tls_root_ctx. Now this field can be freely reloaded by OpenVPN without reconfiguring OpenSSL. Instead, pass the current value into OpenSSL at verification time. To do so, we need to use the SSL_CTX_set_cert_verify_callback, which allows swapping out the X509_verify_cert call, and also tweaking the X509_STORE_CTX configuration before starting certificate verification. Context: SSL_CTX_set_cert_verify_callback and the existing verify_callback are not the same. SSL_CTX_set_cert_verify_callback wraps the verification while verify_callback is called multiple times throughout verification. It's too late to reconfigure X509_STORE_CTX in verify_callback. verify_callback is usually not what you want. Sometimes current_cert and error_depth don't quite line up, and cert_hash_remember may end up called multiple times for a single certificate. I suspect some of the other verify_callback logic would also be better done in the new callback, but I've left it alone to keep this change minimal. verify_callback is really only usable for suppressing errors. Application bookkeeping is better down elsewhere. Signed-off-by: David Benjamin <davidben@google.com>
|
The preferred way for patch submission (these days) is our gerrit, https://gerrit.openvpn.com. The second best way is the mailing list, but it has been a bit unreliable in the last weeks. Also, you need to be subscribed to be allowed to post (but my colleagues should see the moderation request and permit it). Github MRs will not be merged - they can serve as a starting point for the discussion (and we do use GH issues), but in the end the final patch needs to be taken to the list or gerrit - one of us can help with it, if needed. Now, for the issue at hand, I defer to people who understand OpenSSL :-) |
|
I'm guessing that was meant to be https://gerrit.openvpn.net. The other URL took me somewhere confusing. :-) If I followed the links right, looks Gerrit uses the wiki credentials and I need to ask IRC to get an account? I've gone and done that. (There's no mention of the Gerrit at https://github.com/OpenVPN/openvpn/blob/master/CONTRIBUTING.rst by the way. Maybe worth updating?) |
|
To close the loop, the conclusion from IRC seems to have been that Gerrit accounts are only given to long-time contributors and that, for a one-off fix, GitHub was probably best? I'll defer to you all on how you wish to do this. For now, I'm assuming that this is the right place and that this PR is just awaiting review. Let me know if you all would prefer another venue change. |
|
Gerrit accounts should be also given to other contributers from my understanding. We only automatically run the tests automatically for long time contributers/trusted users. |
|
@schwabe is right. Sorry @davidben for the back and forth. Any chance you could create an account at https://gerrit.openvpn.net/pwm/public/newuser and push the patch to gerrit? |
|
I... attempted to make an account but I keep getting "The browser session is invalid or has expired. Please try again" in the after the first page. Sometimes it breaks on the first page too. I tried both Chrome and Safari. |
| verify_flags = SSL_VERIFY_PEER; | ||
| } | ||
| SSL_CTX_set_verify(ctx->ctx, verify_flags, verify_callback); | ||
| SSL_CTX_set_cert_verify_callback(ctx->ctx, cert_verify_callback, NULL); |
There was a problem hiding this comment.
The OpenSSL man page says:
In any case a viable verification result value must be reflected in the error member of x509_store_ctx, which can be done using X509_STORE_CTX_set_error(3).
I also don't see the X509_verify_cert calling this function, which is what OpenSSL also does when SSL_CTX_set_cert_verify_callback is not used.
The code looks good when compared with what OpenSSL internally does but does not do what the man page indicates it should do....
There was a problem hiding this comment.
Oh, that's because OpenSSL tends not to use their accessors internally and just writes into the struct directly. I'll send them a change to add a sentence saying that X509_verify_cert will do this for you. I think this documentation is if you were going to use this callback to swap out the verification process entirely.
@davidben it looks like we have an issue with captcha. To speedup the process I created an account for you with the username and email you tried. Could you please try to log in by resetting the password via https://gerrit.openvpn.net/pwm/public/forgottenpassword |
|
Sorry, this got buried in my inbox. Thanks! I got the account working and pushed the change to https://gerrit.openvpn.net/c/openvpn/+/1289 (It took me a bit to figure out the git URL to push to. I couldn't find it documented and had to guess URLs until I found it. Might be worth documenting that somewhere, unless I just missed it.) |
Actually that's not the URL you should be using (and I'm surprised that this actually works). But it seems Gerrit is smart enough to take incoming commits on any URL... The documentation for uploading is here (linked from the "documentation" tab on top of gerrit): https://gerrit.openvpn.net/Documentation/user-upload.html |
6 participants
Hi all! I tried to send this to openvpn-devel but it looks like it might not have gotten through? (I'm probably confused or just did something wrong.) It sounds like you all do look at GitHub though, so I figure I'll start here and we can figure out the mailing list step afterwards?
I've tested this by making sure things build and that the tests pass. However, it looks like some tests were skipped because they require manually configuring some things in ways I hadn't figured out. It's also unclear to me how well-tested CRL support is. There's no mention of CRLs in the tests directory. I'm not sure how you all test CRL-related changes.
There are two ways to load CRLs in OpenSSL. They can be loaded at the
X509_STORE, shared across verifications, or loaded per verification at theX509_STORE_CTX.OpenVPN currently does the former. However, it also supports CRL reloading, and tries to reload the CRL file before each connection. OpenSSL does not really have a good way to unload objects from an
X509_STORE. OpenVPN currently does it by grabbing theSTACK_OF(X509_OBJECT)out of theX509_STOREand manually deleting all the CRLs from it.This mutates an OpenSSL internal object which bumps into problems if OpenSSL ever switches to a more efficient representation. See openssl/openssl#28599
(It's also not thread-safe, though it doesn't look like that impacts OpenVPN? Actually even reading that list doesn't work. See CVE-2024-0397. This OpenSSL API was simply broken.)
Additionally, this seems to cause two OpenVPN features to not work together. I gather
backend_tls_ctx_reload_crlis trying to clear the CRLs loaded from last time it ran. Buttls_ctx_load_cawith a ca_file can also load CRLs.tls_ctx_load_cawith ca_path will also pick up CRLs andbackend_tls_ctx_reload_crlactually ends up clobbering some stateX509_LOOKUP_hash_dirinternally maintains on theX509_STORE. Likewise,tls_verify_crl_missingcan get confused betweenbackend_tls_ctx_reload_crl's crl_file-based CRLs and CRLs fromtls_ctx_load_ca.Avoid all this by tracking the two CRLs separately.
crl_file-based CRLs now go onto aSTACK_OF(X509_CRL)tracked on thetls_root_ctx. Now this field can be freely reloaded by OpenVPN without reconfiguring OpenSSL. Instead, pass the current value into OpenSSL at verification time. To do so, we need to useSSL_CTX_set_cert_verify_callback, which allows swapping out theX509_verify_certcall, and also tweaking theX509_STORE_CTXconfiguration before starting certificate verification.Context:
SSL_CTX_set_cert_verify_callbackand the existing verify_callback are not the same.SSL_CTX_set_cert_verify_callbackwraps the verification whileverify_callbackis called multiple times throughout verification. It's too late to reconfigureX509_STORE_CTXinverify_callback.verify_callbackis usually not what you want. Sometimescurrent_certanderror_depthdon't quite line up, andcert_hash_remembermay end up called multiple times for a single certificate.I suspect some of the other
verify_callbacklogic would also be better done in the new callback, but I've left it alone to keep this change minimal.verify_callbackis really only usable for suppressing errors. Application bookkeeping is better done elsewhere.