mbedTLS: clean-up insecure/deprecated code, and other fixes #19983
Conversation
|
If no other comments, it's all good to go. The failing check looks like an aberration. |
|
All tests are passing (except a stalled out test due to CI failure), addressed all comments. Good to go unless there are other comments. |
|
@vszakats any remaining remarks or thoughts? |
|
@wyattoday ah, it has a merge conflict now. If you can just fix this and force-push, I'm ready to merge! |
LGTM! |
|
edit: I think a note in the PR message saying that from now on (also rebased to resolve a conflict in comment) |
…not able to connect to any server due to broken logic in curl's `mbed_set_ssl_version_min_max()`. Now it correctly sets the minimum supported TLS version based on what is compiled in the library.
2. If debugging is enabled, move the debugging enabling earlier in the `mbed_connect_step1()` so that verbose errors are actually displayed if failures happen (see the previous point -- it would've made debugging that issue easier).
3. Remove the constant `mbedtls_x509_crt_profile_fr` and instead use mbedTLS-included profile `mbedtls_x509_crt_profile_next` with `mbedtls_ssl_conf_cert_profile()`.
This will follow the latest standards as new mbedTLS versions are released (rather than being stuck-in-time until someone comes along to fix what was hard-coded here). This has the immediate benefit of no longer supporting SHA1 certs and insecure RSA key-lengths (1024). This fix immediately prevents previously possible MITM attacks (SHA1 hashes and RSA-1024 keys can be forged relatively easily by nation-state actors and criminal organizations with deep-pockets).
4. Added [predictive resistance](https://mbed-tls.readthedocs.io/en/latest/kb/how-to/add-a-random-generator/#enabling-prediction-resistance) to the random number generator (adding more entropy to the RNG).
5. Split the random number generator into initialization, the actual random generation, and the "freeing" of the resources. This significantly reduces the overhead of using the RNG.
6. Removed the separate RNG function in the TLS connect stage (instead use the "main" one) and remove the ad-hoc threading support. Instead properly document how to enable threading in mbedTLS. As it was, other internals of mbedTLS could have race conditions (in the RSA module in particular) if `MBEDTLS_THREADING_C` was *not* enabled. And if it is enabled, then these race-conditions cannot happen. And also, if MBEDTLS_THREADING_C is enabled then the RNG functions [are fully thread-safe](https://mbed-tls.readthedocs.io/en/latest/kb/development/thread-safety-and-multi-threading/).
So, the previous ad-hoc threading support was both partial and broken.
7. Enable support for disabling `MBEDTLS_PEM_PARSE_C`.
8. Add support for `CURLOPT_SSLCERTTYPE` so user can specify `PEM` or `DER` and get faster execution.
…n/max if it's disabled -- which was part of the problem).
vszakats
force-pushed
the
mbedtls-tls13-fixes
branch
from
5fdcf52 to
c828009
Compare
It was required before for 3.x (it would just error out lower in the code, now it errors out immediately telling the end-user how to fix their config) |
This wasn't obvious or intentional, thus I still think it'd be useful |
|
Okie doke. Added it as point 9 above. |
|
Thanks! |
3 participants
This is the first of 3 PRs that improve mbedTLS usage in curl/libcurl. This is the redo of this PR: #18161
But I will break down the changes here so it's more clear what I've done and why. Long story short: this increases the security and reduces some cruft:
With
MBEDTLS_SSL_PROTO_TLS1_2not enabled, the mbedTLS code was not able to connect to any server due to broken logic in curl'smbed_set_ssl_version_min_max(). Now it correctly sets the minimum supported TLS version based on what is compiled in the library.If debugging is enabled, move the debugging enabling earlier in the
mbed_connect_step1()so that verbose errors are actually displayed if failures happen (see the previous point -- it would've made debugging that issue easier).Remove the constant
mbedtls_x509_crt_profile_frand instead use mbedTLS-included profilembedtls_x509_crt_profile_nextwithmbedtls_ssl_conf_cert_profile().This will follow the latest standards as new mbedTLS versions are released (rather than being stuck-in-time until someone comes along to fix what was hard-coded here). This has the immediate benefit of no longer supporting SHA1 certs and insecure RSA key-lengths (1024). This fix immediately prevents previously possible MITM attacks (SHA1 hashes and RSA-1024 keys can be forged relatively easily by nation-state actors and criminal organizations with deep-pockets).
Added predictive resistance to the random number generator (adding more entropy to the RNG).
Split the random number generator into initialization, the actual random generation, and the "freeing" of the resources. This significantly reduces the overhead of using the RNG.
Removed the separate RNG function in the TLS connect stage (instead use the "main" one) and remove the ad-hoc threading support. Instead properly document how to enable threading in mbedTLS. As it was, other internals of mbedTLS could have race conditions (in the RSA module in particular) if
MBEDTLS_THREADING_Cwas not enabled. And if it is enabled, then these race-conditions cannot happen. And also, if MBEDTLS_THREADING_C is enabled then the RNG functions are fully thread-safe.So, the previous ad-hoc threading support was both partial and broken.
Enable support for disabling
MBEDTLS_PEM_PARSE_C.Add support for
CURLOPT_SSLCERTTYPEso user can specifyPEMorDERand get faster execution.MBEDTLS_CTR_DRBG_Cis required for mbedTLS 3.x (it was before, but now it errors out immediately so you can fix your mbedTLS config).After this PR is merged I'll adapt the next PR to the head of the main branch and post that PR.