search

Mbed-TLS / mbedtls

An open source, portable, easy to use, readable and flexible TLS library, and reference implementation of the PSA Cryptography API. Releases are on a varying cadence, typically around 3 - 6 months between releases.
https://www.trustedfirmware.org/projects/mbed-tls/
Other
5.03k stars 2.51k forks source link

Fix issue in handling legacy_compression_methods in ssl_tls13_parse_client_hello() #9244

Open waleed-elmelegy-arm opened 2 weeks ago

waleed-elmelegy-arm commented 2 weeks ago

Description

Please write a few sentences describing the overall goals of the pull request's commits. fixes #9243 & #8995

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

Notes for the submitter

Please refer to the contributing guidelines, especially the checklist for PR contributors.

Help make review efficient:

Aethedor commented 2 weeks ago

Yes, that seems to fix the issue with ssllabs.com failing its test. While I used to get an A+ score for my server, now I only get an A. I think it has to do with the fact that my server now doesn't support TLS_FALLBACK_SCSV. Any comment on that?

gilles-peskine-arm commented 2 weeks ago

server now doesn't support TLS_FALLBACK_SCSV. Any comment on that?

FALLBACK_SCSV is about avoiding a downgrade from TLS 1.2. Since Mbed TLS no longer supports earlier protocol versions (TLS 1.0 and 1.1 were removed in Mbed TLS 3.0), FALLBACK_SCSV has no effect when either side is Mbed TLS ≥3.0.

Aethedor commented 2 weeks ago

FALLBACK_SCSV has no effect when either side is Mbed TLS ≥3.0.

Okay, thanks. So ssllabs.com should fix that in their scan I guess.

Anyway, the issue seems fixed. Thanks for the patch!

tom-cosgrove-arm commented 1 week ago

CI is green

waleed-elmelegy-arm commented 5 days ago

There is a misleading comment that I would prefer to fix. Otherwise, did you think about a non-regression test using GnuTLS or OpenSSL?

OpenSSL needs to be recompiled with a flag to allow for legacy compression methods and GnuTLS discontinued legacy compression methods in 3.6 so the test won't work in next version so it's going to be difficult to do a regression test unless we can send the ClientHello message hardcoded.

ronald-cron-arm commented 4 days ago

There is a misleading comment that I would prefer to fix. Otherwise, did you think about a non-regression test using GnuTLS or OpenSSL?

OpenSSL needs to be recompiled with a flag to allow for legacy compression methods and GnuTLS discontinued legacy compression methods in 3.6 so the test won't work in next version so it's going to be difficult to do a regression test unless we can send the ClientHello message hardcoded.

Thus I would say that a test with our current version of GnuTLS (3.4.6) would do the job for some time at least. As far as I know we still plan to base some tests on rather old versions of OpenSSL, GnuTLS for that kind of situation. As it is a situation we can encounter in real life (worth saying it in a comment associated to the tests by the way) it is better to have a non-regression test.

gilles-peskine-arm commented 4 days ago

As far as I know we still plan to base some tests on rather old versions of OpenSSL, GnuTLS for that kind of situation

That's right. We do plan to keep some old versions of OpenSSL and GnuTLS around in our test environment for a long time, as long as there is at least one maintained version of Mbed TLS that has a feature that has been dropped from recent OpenSSL/GnuTLS. Even if we change the default versions that we test against, we are not going to remove GnuTLS 3.3.8 for at least as long as 2.28 is around (I'm don't know offhand if it's still relevant for 3.6), and I think we'll keep 3.4.6 for at least as long as 3.6 is around. It's not a problem to keep it as long as 4.x is around.