-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
mbedtls: Fix ssl_init error with mbedTLS 3.1.0+ #8238
Conversation
Since mbedTLS 3.1.0, mbedtls_ssl_setup() fails if the provided config struct is not valid. mbedtls_ssl_config_defaults() needs to be called before the config struct is passed to mbedtls_ssl_setup().
@trackpadpro as you've been editing this code recently, does this change look good to you as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks good to me, but I cannot test it right now on one of my computers. I would like to see the error log you ran into that required this change, but in general, this does not seem like it would cause any problems.
I encountered this problem after updating to mbedTLS 3.1.0 when trying to connect to an SMTP:
As far as I can tell, the config check was introduced here: https://github.com/ARMmbed/mbedtls/pull/4853/files#diff-76b73bf2a8157ee95fdd3b688384812654c63878be4ecce6ee56402eb52b1f4fR3205-R3206 mbedtls_ssl_setup() calls ssl_conf_check(), which fails if the config is not valid. The change I proposed fixes the problem on my end, but I haven't tested it on mbedTLS < 3.1.0. |
Also, the mbedtls CI job runs a lot of tests just fine with this PR applied so it seems rather safe. |
I agree that it's rather safe, but to be absolutely certain of backwards compatibility you could always add some version conditionals ("Mbed TLS 2.28 is a long-time support branch. It will be supported with bug-fixes and security fixes until end of 2024."). v3.1.0 is MBEDTLS_VERSION_NUMBER == 0x03010000 if you want to add the (possibly overkill) checks |
It would be enough if someone just built with that version and ran through a few tests |
I'll test with mbedTLS 3.0, mbedTLS 2.28 and 2.16 later today, and report back to you. |
First, without the patch: # mbedtls-3.1.0
$ ./curl --no-progress-meter -o/dev/null https://www.google.fr/ && echo Success
curl: (35) mbedTLS: ssl_init failed
# mbedtls-3.0.0
$ ./curl --no-progress-meter -o/dev/null https://www.google.fr/ && echo Success
Success
# mbedtls-2.28.0
$ ./curl --no-progress-meter -o/dev/null https://www.google.fr/ && echo Success
Success
# mbedtls-2.16.12
$ ./curl --no-progress-meter -o/dev/null https://www.google.fr/ && echo Success
Success And now with the fix applied: # mbedtls-3.1.0
$ ./curl --no-progress-meter -o/dev/null https://www.google.fr/ && echo Success
Success
# mbedtls-3.0.0
$ ./curl --no-progress-meter -o/dev/null https://www.google.fr/ && echo Success
Success
# mbedtls-2.28.0
$ ./curl --no-progress-meter -o/dev/null https://www.google.fr/ && echo Success
Success
# mbedtls-2.16.12
$ ./curl --no-progress-meter -o/dev/null https://www.google.fr/ && echo Success
Success |
Awesome, great work. Merging now... |
Thanks! |
Since mbedTLS 3.1.0, mbedtls_ssl_setup() fails if the provided
config struct is not valid.
mbedtls_ssl_config_defaults() needs to be called before the config
struct is passed to mbedtls_ssl_setup().