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.27k stars 2.57k forks source link

TLS-RFC Compliance #4613

Open mmaehren opened 3 years ago

mmaehren commented 3 years ago

Hi, we (@jurajsomorovsky @ic0ns @mmaehren @XoMEX @Kavakuo) are performing an analysis of the RFC-compliance of open-source TLS implementations. Below we list our findings for this implementation. We admit that some are rather nit-picky, but we added them for the sake of completeness. We tried to keep the descriptions brief and didn’t want to spam the issues section so feel free to split up the list into individual issues as you see fit. If you disagree with our interpretation of certain RFC statements, please leave feedback as we’re interested in your view.

Our results apply to the default configuration of version 2.25.0. We used the example implementations for client and server.

[S] = Applies to server [C] = Applies to client [C+S] = Applies to both

Misc

Session not aborted

Only session closed but no alert sent

gilles-peskine-arm commented 3 years ago

Thank your for this detailed report! As there are many issued, I expect that it will take us some time to analyze them all. I've done a first pass of just reading the report and have a few remarks and follow-up questions.

[C+S] mbedTLS does not process handshake messages spread across multiple records, we assume that there is no support for record fragmentation

Are we talking about TLS or DTLS here? Mbed TLS does support a DTLS handshake split across multiple datagrams, but not for TLS.

[C+S] The following deprecated groups are supported: SECP224K1,SECP192R1,SECP224R1,SECP256K1,SECP192K1

They won't be in Mbed TLS 3.0 (but we won't be changing anything for 2.x long-time support branches). I hadn't noticed that secp256k1 was deprecated. Do you happen to know why? Did I miss a weakness in SEC Koblitz curves?

[S] Session resumption is accepted based on session IDs even if the previous connection was terminated using a fatal alert

This does sound like a definite bug that could help attackers when combined with another vulnerability so I've filed it as https://github.com/ARMmbed/mbedtls/issues/4614.

[S] The point format extension sent by the client is not validated [C] The client ignores if the server chooses an AEAD cipher suite and also negotiates encrypt-then-MAC [S] It is not verified that the ECC Extensions (ec_points_format, supported_groups) are only included if EC suites are advertised by the client

These seem pretty low-risk to me. That doesn't mean we won't fix them, but the priority is low.

[S] The content of the the ClientHello Padding Extension is not verified

Mbed TLS does not support this extension.

[C] The client ignores if the server includes (unproposed) GREASE extensions in its ServerHello Again, seems low-risk.

Only session closed but no alert sent

We are aware that Mbed TLS does not always send an alert when it should and may occasionally send an alert when it should close the connection abruptly. We last worked on this four years ago which I'm afraid shows it's just not been a priority for us, though having a list of specific cases definitely makes it easier to handle those cases. By the way, how much coverage do you think your testing gave regarding sending alerts? Should we treat these cases as the tip of the iceberg or as a mostly comprehensive list?

Final question: if we fix some issues, how much trouble would it be to re-run part of your analysis to confirm the fix?

mmaehren commented 3 years ago

Thank you for the fast feedback. We meant record fragmentation in TLS. Regarding the deprecated curves, I'm not aware of a grave weakness of secp256k1 and RFC 8422 only stated

RFC 4492 defined 25 different curves in the NamedCurve registry (now renamed the "TLS Supported Groups" registry, although the enumeration below is still named NamedCurve) for use in TLS. Only three have seen much use. This specification is deprecating the rest (with numbers 1-22).

It's hard to estimate a coverage for alerts. We tried to define testcases for as many RFC statements as possible. As our evaluation is mostly automated, we clould re-run the evaluation with a newer version to confirm fixes.

mpg commented 3 years ago

Thank you very much @mmaehren for doing this work and reporting the results, that's very helpful! As Gilles said, there are many points here, and for me as well this is just a first pass.

We admit that some are rather nit-picky, but we added them for the sake of completeness. [...] If you disagree with our interpretation of certain RFC statements, please leave feedback as we’re interested in your view.

I think completeness is good, but perhaps it would also be useful to have a categorization of the findings. For example, there are many situations where the RFC says "the server MUST NOT do X" but doesn't say explicitly whether the client should check for X, much less how it should react when faced with X (same with the roles reversed of course). I think it would be good to distinguish these from outright violations of a MUST (unless of course there's a plausible risk that an attacker could exploit the lack for checking from the receiving party).

IMO the following findings fall in this category:

(Contrast with the case of a server sending unsolicited extensions, where the RFC explicitly says that the client MUST abort in this case. Or the case of receiving unknown message types.)

[S] The point format extension sent by the client is not validated

Indeed, our support is still based on RFC 4492 and we haven't fully updated for RFC 8422 yet.

mmaehren commented 3 years ago

Thanks for the feedback. We do agree that these statements have a different meaning than an explicit requirement and will consider separate categories in the future.

mmaehren commented 3 years ago

Based on the feedback we received from @tomato42, we removed the reported issue where a client sends an elliptic curve extension but does not offer an elliptic curve cipher suite as terminating the connection may cause interoperability issues.

gilles-peskine-arm commented 1 year ago

Note: the test tool is now known as TLS-Anvil. The findings have been published at USENIX Securiy 2022 and RWC 2023.