Skip to content
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

Closed
wants to merge 1 commit into from
Closed

mbedtls: Fix ssl_init error with mbedTLS 3.1.0+ #8238

wants to merge 1 commit into from

Conversation

Koromix
Copy link
Contributor

@Koromix Koromix commented Jan 7, 2022

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().

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().
@bagder bagder added the TLS label Jan 7, 2022
@bagder
Copy link
Member

bagder commented Jan 7, 2022

@trackpadpro as you've been editing this code recently, does this change look good to you as well?

Copy link
Contributor

@trackpadpro trackpadpro left a 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.

@Koromix
Copy link
Contributor Author

Koromix commented Jan 8, 2022

I encountered this problem after updating to mbedTLS 3.1.0 when trying to connect to an SMTP:

* Connected to mail.gandi.net (217.70.178.9) port 465 (#0)
* mbedTLS: Connecting to mail.gandi.net:465
* mbedTLS: ssl_init failed
* Closing connection 0
::ffff:127.0.0.1: Error: Failed to perform mail call: SSL connect error

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.

@bagder
Copy link
Member

bagder commented Jan 8, 2022

Also, the mbedtls CI job runs a lot of tests just fine with this PR applied so it seems rather safe.

@trackpadpro
Copy link
Contributor

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

@bagder
Copy link
Member

bagder commented Jan 8, 2022

It would be enough if someone just built with that version and ran through a few tests

@Koromix
Copy link
Contributor Author

Koromix commented Jan 9, 2022

I'll test with mbedTLS 3.0, mbedTLS 2.28 and 2.16 later today, and report back to you.

@Koromix
Copy link
Contributor Author

Koromix commented Jan 9, 2022

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

@bagder
Copy link
Member

bagder commented Jan 9, 2022

Awesome, great work. Merging now...

@bagder bagder closed this in 919baa5 Jan 9, 2022
@bagder
Copy link
Member

bagder commented Jan 9, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants