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

TLS-RFC Compliance #4613

Open
mmaehren opened this issue Jun 3, 2021 · 6 comments
Open

TLS-RFC Compliance #4613

mmaehren opened this issue Jun 3, 2021 · 6 comments

Comments

@mmaehren
Copy link

mmaehren commented Jun 3, 2021

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

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

    • we specifically tested this with a fragment length of 50 meaning that each record only conveys up to 50 plaintext bytes of a handshake message
  • [C+S] The following deprecated groups are supported: SECP224K1,SECP192R1,SECP224R1,SECP256K1,SECP192K1

    • [S] Expected result: a fatal alert if no other groups are offered
    • [C] Expected result: not sending these groups
    • RFC: 8422, Section: 5.1.1 Supported Elliptic Curves Extension

      This specification is deprecating the rest (with numbers 1-22). This specification also deprecates the explicit curves with identifiers 0xFF01 and 0xFF02.

Session not aborted

  • [S] Session resumption is accepted based on session IDs even if the previous connection was terminated using a fatal alert
    • Expected result: fatal alert
    • RFC: 5246, Section: 7.2.2 Error Alerts

      Thus, any connection terminated with a fatal alert MUST NOT be resumed.

  • [S] The point format extension sent by the client is not validated
    • Expected result: illegal_parameter alert
    • Invalid or deprecated point format values are ignored. The server simply responds with an extension stating it uses the uncompressed format
    • RFC: 8422, Section: 5.1. Client Hello Extensions

      If the client sends the extension and the extension does not contain the uncompressed point format, and the client has used the Supported Groups extension to indicate support for any of the curves defined in this specification, then the server MUST abort the handshake and return an illegal_parameter alert.

  • [C] The client ignores if the server chooses an AEAD cipher suite and also negotiates encrypt-then-MAC
    • Expected result: a fatal alert
    • RFC: 7366, Section: 3. Applying Encrypt-then-MAC

      If a server receives an encrypt-then-MAC request extension from a client and then selects a stream or Authenticated Encryption with Associated Data (AEAD) ciphersuite, it MUST NOT send an encrypt-then-MAC response extension back to the client.

  • [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
    • RFC: 8422, Section: 4. TLS Extensions for ECC

      The client MUST NOT include these extensions in the ClientHello message if it does not propose any ECC cipher suites.

  • [S] The content of the the ClientHello Padding Extension is not verified
    • Please leave a comment if your implementation does not support this extension and hence ignores the content
    • RFC: 7685, Section: 3. Padding Extension

      The client MUST fill the padding extension completely with zero bytes, although the padding extension_data field may be empty.

  • [C] The client ignores if the server includes (unproposed) GREASE extensions in its ServerHello
    • Expected result: a fatal alert
    • RFC: 5246, Section:7.4.1.4. Hello Extensions

      If a client receives an extension type in ServerHello that it did not request in the associated ClientHello, it MUST abort the handshake with an unsupported_extension fatal alert.

    • RFC: 8701, Section: 4. Server-Initiated Extension Points

      Clients MUST reject GREASE values when negotiated by the server. In particular, the client MUST fail the connection if a GREASE value appears in any of the following: Any ServerHello extension

Only session closed but no alert sent

  • No alert is sent upon receiving
    • [C+S] close_notify
      • Expected result: a close notify of its own
      • RFC: 5246, Section: 7.2.1 Closure Alerts

        Unless some other fatal alert has been transmitted, each party is required to send a close_notify alert before closing the write side of the connection. The other party MUST respond with a close_notify alert of its own and close down the connection immediately, discarding any pending writes.

    • [S] a ChangeCipherSpec message with an valid content (i.e. not 1)
    • [S] a Messages out of order. Tested with the following scenarios
      • Sending FIN after SeverHelloDone and before sending CKE
      • Sending a second CCS message
      • Sending a second CH message
      • Sending a second CH message after the SH
      • Sending FIN after SH
      • Sending a HeartbeatRequest after CCS/CH/CKE
      • Sending ApplicationData/EmptyApplicationData/CCS/Finished as the first message
    • [S] a CH or CKE with an invalid length (tested with -1)
    • [C+S] a Handshake/Application Record with a length of 0 set in the TLSCiphertext (struct in RFC5246)
      • For handshake we sent an empty record when a finished should have been sent
      • i.e. no ciphertext sent, just an empty record
    • [C+S] a Record/Fragment with a length of 0 set in the TLSPlaintext (struct in RFC5246)
      • Tested when sending a:
        • [C] SH
        • [S] CH, Alert
        • [C+S] CCS
      • i.e. a ciphertext sent with no payload
      • RFC: 5246, Section: 6.2.1 Fragmentation

      Implementations MUST NOT send zero-length fragments of Handshake, Alert, or ChangeCipherSpec content types.

    • [C+S] a Record with an invalid Type
      • Tested: When sending a CH/SH/CCS+FIN
      • RFC 5246 Section 6

        Implementations MUST NOT send record types not defined in this document unless negotiated by some extension. If a TLS implementation receives an unexpected record type, it MUST send an unexpected_message alert.

    • [C+S] a FF-DH share which is out of bounds
      • We'd expect a handshake_failure or illegal_parameter alert
      • RFC: 7919, Section: 4. Server Behavior

        [...] the server MUST verify that 1 < dh_Yc < dh_p - 1. If dh_Yc is out of range, the server MUST terminate the connection with a fatal handshake_failure(40) alert.

      • RFC: 8446, Section 6. Alert Protocol

        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.

    • [C+S] a ClientKeyExchangeMessage with an invalid length field (reduced by 1, so parsing must fail) when ECDH(E) is used
      • Note that RSA CKE messages with the same modification lead to a bad_record_mac alert
    • [C+S] Records exceeding the maximum length
      • Expected alert: record_overflow
      • This holds for a too long plaintext
        • RFC: 5264, Section: 6.2.1 Fragmentation

          The length (in bytes) of the following TLSPlaintext.fragment. The length MUST NOT exceed 2^14.

      • This holds for a too long ciphertext
        • RFC: 5264, Section: 6.2.1 Fragmentation

          The length (in bytes) of the following TLSCiphertext.fragment. The length MUST NOT exceed 2^14 + 2048.

    • [C] An extension in the ServerHello that the Client did not offer
      • Expected alert: unsupported_extension
      • RFC: 5246, Section: 7.4.1.4. Hello Extensions

        If a client receives an extension type in ServerHello that it did not request in the associated ClientHello, it MUST abort the handshake with an unsupported_extension fatal alert.

    • [C+S] A ClientHello/ServerHello with SSL2 set as the protocol version
    • [C] A ServerKeyExchange with an elliptic curve point that is not on the negotiated curve
    • [C] A ServerHello that contains a GREASE version as the selected protocol version
@gilles-peskine-arm
Copy link
Contributor

gilles-peskine-arm commented Jun 3, 2021

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 #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
Copy link
Author

mmaehren commented Jun 4, 2021

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
Copy link
Contributor

mpg commented Jun 4, 2021

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:

  • [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
  • [C+S] a Record/Fragment with a length of 0 set in the TLSPlaintext
  • and perhaps others

(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
Copy link
Author

mmaehren commented Jun 4, 2021

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
Copy link
Author

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
Copy link
Contributor

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

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

No branches or pull requests

5 participants