rustls / rustls

A modern TLS library in Rust
Other
5.95k stars 636 forks source link

TLS-RFC Compliance (TLS-Anvil) #729

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 0.19.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

Different alert sent than defined by the RFC

djc commented 3 years ago

Wow, thanks for the detailed analysis! Can you say more about the context in which you're undertaking this analysis? Is this for some research paper? How do rustls compare against the other 6 implementations you've analyzed?

mmaehren commented 3 years ago

Yes, this is part of a research paper we're working on. The results for rustls are close to those of other well-known open source TLS libraries. We plan to release our results in the coming months. One thing we noticed in comparison to other libraries is that rustls for many error cases does not send an alert before closing the connection. Is there a reason why you chose not to send alerts?

ctz commented 3 years ago

Thanks for this analysis. I'll work through these.

([S] upon receiving a Padding extension that contains other bytes than 0x00) Please leave a comment if your implementation does not support this extension and hence ignores the content

Yes, we don't support this extension.

briansmith commented 3 years ago

Only session closed but no alert sent

" Whenever an implementation encounters a fatal error condition, it SHOULD send an appropriate fatal alert and MUST close the connection without sending or receiving any additional data. "

Sending an alert is never required.

ctz commented 3 years ago

Only session closed but no alert sent

I'm reasonably sure we send alerts in many/most of these situations. I'm wondering if it's possible they are getting lost somewhere in the test rig -- it's pretty common for errors to predate the IO needed to send an alert.

briansmith commented 3 years ago

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.

Hi, when reviewing a couple, e.g. stuff regarding legacy_version it seems like you are assuming that if the spec says that the sender MUST do something, then the receiver MUST check that the sender did it. However, that isn't the case; there are some requirements on senders that the receiver isn't required to enforce. I think it would be useful for you to separate these out from the ones that the spec does require the receiver to enforce. There might even be a third category, "things that the spec doesn't require the receiver to check, but that was a mistake and the spec should be updated to require a check."

Regarding sending alerts, I think that generally Rustls does intend to send alerts where the spec indicates an alert should be sent. Note the spec says:

In the rest of this specification, when the phrases "terminate the connection" and "abort the handshake" are used without a specific alert it means that the implementation SHOULD send the alert indicated by the descriptions below.

This wording was added specifically to make sending an alert optional; in particular, it was added to make it so that an implementation never has to implement any machinery to send alerts, and to allow an implementation to be very conservative about side channel leakage through alerts by avoiding ever sending them. The consequence is that the lack of an alert isn't necessarily non-conformant; what really matters is that the connection is shut down as that is a MUST requirement.

mmaehren commented 3 years ago

We agree that these cases should be listed separately in the future. We decided to include them based on the following statement from RFC 8446 section 6

Peers which receive a message which is syntactically correct but semantically invalid (e.g., a DHE share of p - 1, or an invalid enum) MUST terminate the connection with an "illegal_parameter" alert.

By our interpretation, this could apply to cases like the legacy version field. However, there is no such statement for TLS 1.2.

ic0ns commented 3 years ago

Only session closed but no alert sent

" Whenever an implementation encounters a fatal error condition, it SHOULD send an appropriate fatal alert and MUST close the connection without sending or receiving any additional data. "

Sending an alert is never required.

Ah, that's a good point. I wasn't aware. Apparently for TLS 1.3 this is a SHOULD - but for TLS 1.2 this is a MUST:

Whenever an implementation encounters a condition which is defined as a fatal alert, it MUST send the appropriate alert prior to closing the connection. We scanned the RFC's mostly for MUST's and have overlooked this.

briansmith commented 2 years ago

Sending an alert is never required.

Ah, that's a good point. I wasn't aware. Apparently for TLS 1.3 this is a SHOULD - but for TLS 1.2 this is a MUST:

I'm not sure exactly how TLS 1.3 updates TLS 1.2, but I believe that in such cases where TLS 1.3 changed things like this that also apply to earlier versions, it is valid to consider the TLS 1.3 rules to override the TLS 1.2 rules. And, besides, I would say that TLS 1.2 was must plain wrong to require that.

Ralith commented 2 years ago

Note that sending an alert (or rather, propagating it to the transport layer) is mandatory in various situations for QUIC.

briansmith commented 2 years ago

Note that sending an alert (or rather, propagating it to the transport layer) is mandatory in various situations for QUIC.

That's different than my understanding and would be a surprise since the TLS WG put some effort into ensuring that a TLS implementation would never have to send/generate alerts during TLS 1.3. I searched RFC 9001 for "alert" and I think my interpretation that alerts are never required to be generated/sent is compatible with what it says, though it could be clearer. I'd be happy to be corrected if I'm misunderstanding.

BTW, Rustls has always generated and sent alerts in most cases. I would actually like to introduce the option to stop doing that, but that's purely hypothetical.

Ralith commented 2 years ago

A couple examples:

RFC 9001 §6:

Endpoints MUST treat the receipt of a TLS KeyUpdate message as a connection error of type 0x010a, equivalent to a fatal TLS alert of unexpected_message

RFC 9001 §8.1:

When using ALPN, endpoints MUST immediately close a connection (see Section 10.2 of [QUIC-TRANSPORT]) with a no_application_protocol TLS alert (QUIC error code 0x0178; see Section 4.8) if an application protocol is not negotiated. While [ALPN] only specifies that servers use this alert, QUIC clients MUST use error 0x0178 to terminate a connection when ALPN negotiation fails.

While these don't necessarily have to be implemented as alerts on the rustls side, it is convenient to do so. You might not consider these to be alerts in the strict sense because they are communicated at the QUIC layer rather than within TLS, but this is also how TLS alerts are handled by QUIC in general, per RFC 9001 §4.8.

briansmith commented 2 years ago

close a connection [...] with a no_application_protocol TLS alert

RFC 8446 says:

The phrases "terminate the connection with an X alert" and "abort the handshake with an X alert" mean that the implementation MUST send alert X if it sends any alert.

In the case of the disallowed KeyUpdate message, I think the same logic apples.

Ralith commented 2 years ago

On review I believe you're correct. Relevant text from RFC 9000 §11:

Where this specification identifies error conditions, it also identifies the error code that is used; though these are worded as requirements, different implementation strategies might lead to different errors being reported. In particular, an endpoint MAY use any applicable error code when it detects an error condition; a generic error code (such as PROTOCOL_VIOLATION or INTERNAL_ERROR) can always be used in place of specific error codes.

Hence even regardless of RFC 8446, the "MUST" text from RFC 9001 quoted above is weak with respect to the specified error code; rustls therefore need not generate an alert and thereby communicate these specific codes. So long as rustls indicates that an error has occurred via read_hs regardless, all should be well.

briansmith commented 2 years ago

@mmaehren Where can we find the latest results from your work? I am very interested in working to close any RFC compliance issues. I would love it if there was an update to your results with some of the nuances above considered. Is there?

mmaehren commented 2 years ago

@briansmith We reached out to you via email

djc commented 2 years ago

Why not here, is something about your research confidential? I'd like to follow along.

ic0ns commented 2 years ago

@djc neither the tool nor the paper are currently publicly available, as the paper is not peer reviewed yet. It is not super confidential, but at this point, we would not like to publicly share preprints yet. Both will be available publicly as soon as the reviewers have some mercy with us...

mmaehren commented 2 years ago

Hey, just a minor addition: upon receiving a ClientHello with a PSK extension but no PSK key exchange modes extension, Rustls falls back to a full handshake instead of aborting the connection as defined in RFC 8446

This also seems to apply to version 0.20.2 of Rustls

briansmith commented 2 years ago

@mmaehren Thanks. I recommend also checking implementations' behaviors regarding close_notify. See #959 and #989 for some inspiration.

jsha commented 1 year ago

@mmaehren Congratulations on publishing!

Here's the link to the TLS-Anvil paper: https://tls-anvil.com/assets/files/TLS-Anvil-Paper-c3dbb77c9b27783fe7998d09765061c4.pdf

And a link to the video from Real World Crypto: https://youtu.be/WEjgFMuwIAc?t=2255