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

3.6.0 fails in curl CI where 3.5.2 works #9210

Closed
icing opened this issue May 31, 2024 · 11 comments
Closed

3.6.0 fails in curl CI where 3.5.2 works #9210

icing opened this issue May 31, 2024 · 11 comments
Labels
bug component-tls size-s Estimated task size: small (~2d)

Comments

@icing
Copy link

icing commented May 31, 2024

Summary

Building current curl master with mbedtls 3.6.0 shows many test failures where there were none with 3.5.2.

System information

Mbed TLS version: 3.6.0
Operating system and version: macOS
Configuration (if not default, please attach mbedtls_config.h): default
Compiler and options (if you used a pre-built binary, please indicate how you obtained it): macos gcc
Additional environment information:

Expected behaviour

The curl test suite should succeed.

Actual behaviour

In the failed test cases, curl logs:

ssl_handshake returned - mbedTLS: (-0x2700) X509 - Certificate verification failed, e.g. CRL, CA or signature check failed

Steps to reproduce

Build curl master with mbedtls 3.6.0. Run make test in the curl checkout.

Additional information

There is a curl issue open related to this: curl/curl#13653

@gilles-peskine-arm
Copy link
Contributor

The most prominent change in Mbed TLS 3.6.0 is that TLS 1.3 is now enabled by default. Is the failing connection now using TLS 1.3?

@icing
Copy link
Author

icing commented May 31, 2024

The most prominent change in Mbed TLS 3.6.0 is that TLS 1.3 is now enabled by default. Is the failing connection now using TLS 1.3?

It negotiates 0x304 with our test server.

@gilles-peskine-arm
Copy link
Contributor

Thanks for checking! That's TLS 1.3 (TLS version encodings are weird).

I've been checking, and unfortunately we have a regression in Mbed TLS 3.6.0 where if you use the default configuration, the library now tries to negotiate TLS 1.3 in a state where it can't.

Workarounds

Call psa_crypto_init() before calling mbedtls_ssl_handshake. Then TLS connections will work even if they negotiate TLS 1.3.

or

Call mbedtls_ssl_conf_max_tls_version(ssl_config, MBEDTLS_SSL_VERSION_TLS1_2) to disable TLS 1.3 before calling mbedtls_ssl_setup.

@gilles-peskine-arm gilles-peskine-arm added bug component-tls size-s Estimated task size: small (~2d) labels May 31, 2024
@icing
Copy link
Author

icing commented May 31, 2024

Thanks, but calling psa_crypto_init() does not change this.

I found out that this error happens in the tests that have peer verification off. We seem to then not load any trust anchors into the mbedtls_x509_crt instance. This gives the expected verification skip in mbedtls < v3.6.0. Has the expected use changed here or is this a bug in the tls v1.3 peer verification?

@gilles-peskine-arm
Copy link
Contributor

peer verification off

We don't support disabling server authentication in TLS 1.3.

That's a separate matter, which could be its own feature request. Does psa_crypto_init help with tests that have peer verification enabled? Or do those tests pass even without psa_crypto_init?

@icing
Copy link
Author

icing commented May 31, 2024

peer verification off

We don't support disabling server authentication in TLS 1.3.

I believe this makes it the first TLS library that does this. What makes you think breaking applications that upgrade from 3.5 to 3.6 is an acceptable outcome?

That's a separate matter, which could be its own feature request. Does psa_crypto_init help with tests that have peer verification enabled? Or do those tests pass even without psa_crypto_init?

The tests with verification seem to work without psa_crypto_init, but I have not been able to pass the whole suite due to the verification issue. Too many failures to analyse every single one of them.

@icing
Copy link
Author

icing commented May 31, 2024

Ok, I managed to override the changed verification behaviour. I have all tests passing now except one. This one opens 50 HTTP/1.1 connections to a local server and the handshake fails with "(-0x7F00) SSL - Memory allocation failed". There is no way the process ran out of memory. Some bug lurking there probably.

Related to my adaptions for 3.6.0: I get now MBEDTLS_ERR_SSL_RECEIVED_NEW_SESSION_TICKET from a mbedtls_ssl_write() call. I assume that none of the passed data has been sent. Is that correct? It seems to work here locally, but I wanted to confirm this.

@gilles-peskine-arm
Copy link
Contributor

gilles-peskine-arm commented May 31, 2024

So I conclude that you're already calling psa_crypto_init, since you're getting TLS 1.3 connections.

50 HTTP/1.1 connections to a local server and the handshake fails with "(-0x7F00) SSL - Memory allocation failed"

This probably needs to be increased (edit mbedtls_config.h or build with e.g. -DMBEDTLS_PSA_KEY_SLOT_COUNT 256):

//#define MBEDTLS_PSA_KEY_SLOT_COUNT 32

32 is a compromise between highly constrained platforms where it's a lot of memory, and high-end platforms where it's too small. I don't think it's a good default value though. (Yes, I picked 32 a long time ago, but this was in a proof-of-concept, we should have revised it before going to production.)

@icing
Copy link
Author

icing commented May 31, 2024

Indeed we call that in our connection setup. Sorry for being not clear about it earlier.

Interesting to know about MBEDTLS_PSA_KEY_SLOT_COUNT. As I read it, it is a compile time constant? Since there are libcurl applications used thousands of parallel connections, this seems to restrict the combination of curl+mbedtls somewhat. Good to know.

@jay
Copy link

jay commented May 31, 2024

I get now MBEDTLS_ERR_SSL_RECEIVED_NEW_SESSION_TICKET from a mbedtls_ssl_write() call.

Possibly related to #9134 and #8749

We don't support disabling server authentication in TLS 1.3.

I think we have encountered this elsewhere in curl w/ mbedtls but I don't recall the issue. Why was this change made?

Ra2-IFV added a commit to Ra2-IFV/openwrt-packages that referenced this issue Jun 18, 2024
tlsv1.3 support is broken in curl 8.8.0 with mbedtls 3.6.0.
See curl/curl#13653 and Mbed-TLS/mbedtls#9210 for more details.
A workaround was implemented in upsteam codes, see
curl/curl@0c4b4c1
and curl/curl@5f9017d
This snapshot contains all the commits above.

Fixes openwrt#24365 openwrt#24386

Signed-off-by: Ryan Keane <the.ra2.ifv@gmail.com>
Ra2-IFV added a commit to Ra2-IFV/openwrt-packages that referenced this issue Jun 18, 2024
tlsv1.3 support is broken in curl 8.8.0 with mbedtls 3.6.0.
See curl/curl#13653 and Mbed-TLS/mbedtls#9210 for more details.
A workaround was implemented in upsteam codes, see
curl/curl@0c4b4c1 and curl/curl@5f9017d
This snapshot contains all the commits above.

Fixes openwrt#24365 openwrt#24386

Signed-off-by: Ryan Keane <the.ra2.ifv@gmail.com>
Ra2-IFV added a commit to Ra2-IFV/openwrt-packages that referenced this issue Jun 19, 2024
tlsv1.3 support is broken in curl 8.8.0 with mbedtls 3.6.0.
See curl/curl#13653 and Mbed-TLS/mbedtls#9210 for more details.
A workaround was implemented in upsteam code, see curl/curl@0c4b4c1 and curl/curl@5f9017d
This commit includes patches generated from upstream commits.

fix openwrt#24365 openwrt#24386

Signed-off-by: Ryan Keane <the.ra2.ifv@gmail.com>
Ra2-IFV added a commit to Ra2-IFV/openwrt-packages that referenced this issue Jun 19, 2024
tlsv1.3 support is broken in curl 8.8.0 with mbedtls 3.6.0.
See curl/curl#13653 and Mbed-TLS/mbedtls#9210 for more details.
A workaround was implemented in upsteam code, see curl/curl@0c4b4c1 and curl/curl@5f9017d
This commit includes patches generated from upstream commits.

fix openwrt#24365 openwrt#24386

Signed-off-by: Ryan Keane <the.ra2.ifv@gmail.com>
Ra2-IFV added a commit to Ra2-IFV/openwrt-packages that referenced this issue Jun 19, 2024
tlsv1.3 support is broken in curl 8.8.0 with mbedtls 3.6.0.
See curl/curl#13653 and Mbed-TLS/mbedtls#9210 for more details.
A workaround was implemented in upsteam code, see curl/curl@0c4b4c1 and curl/curl@5f9017d
This commit includes patches generated from upstream commits.

fix openwrt#24365 openwrt#24386

Signed-off-by: Ryan Keane <the.ra2.ifv@gmail.com>
Ra2-IFV added a commit to Ra2-IFV/openwrt-packages that referenced this issue Jun 20, 2024
tlsv1.3 support is broken in curl 8.8.0 with mbedtls 3.6.0.
See curl/curl#13653 and Mbed-TLS/mbedtls#9210 for more details.
A workaround was implemented in upsteam code, see curl/curl@0c4b4c1 and curl/curl@5f9017d
This commit includes patches generated from upstream commits.

fix openwrt#24365 openwrt#24386

Signed-off-by: Ryan Keane <the.ra2.ifv@gmail.com>
Ra2-IFV added a commit to Ra2-IFV/openwrt-packages that referenced this issue Jun 21, 2024
tlsv1.3 support is broken in curl 8.8.0 with mbedtls 3.6.0.
See curl/curl#13653 and Mbed-TLS/mbedtls#9210 for more details.
A workaround was implemented in upsteam code, see curl/curl@0c4b4c1 and curl/curl@5f9017d
This commit includes patches generated from upstream commits.

fix openwrt#24365 openwrt#24386

Signed-off-by: Ryan Keane <the.ra2.ifv@gmail.com>
liudf0716 pushed a commit to liudf0716/packages that referenced this issue Jul 10, 2024
tlsv1.3 support is broken in curl 8.8.0 with mbedtls 3.6.0.
See curl/curl#13653 and Mbed-TLS/mbedtls#9210 for more details.
A workaround was implemented in upsteam code, see curl/curl@0c4b4c1 and curl/curl@5f9017d
This commit includes patches generated from upstream commits.

fix openwrt#24365 openwrt#24386

Signed-off-by: Ryan Keane <the.ra2.ifv@gmail.com>
@mpg
Copy link
Contributor

mpg commented Sep 2, 2024

All the issues causing the failures mentioned here have been fixed in Mbed TLS 3.6.1, so I'm closing this issue. Please give the new version a try and let us know if it works for you!

@mpg mpg closed this as completed Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component-tls size-s Estimated task size: small (~2d)
Projects
Status: 3.6.1 patch release
Development

No branches or pull requests

4 participants