botan icon indicating copy to clipboard operation
botan copied to clipboard

RFC-compliance of TLS

Open mmaehren opened this issue 3 years ago • 5 comments

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.17.3. We used the example implementations for client and server.

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

Session not aborted

  • [C] when a server chooses a non-CBC cipher suite but negotiates encrypt-then-MAC

    • RFC 7366 - 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.

  • [C] when a server omits the ChangeCipherSpec message and directly sends an encrypted Finished message, Botan client does not reject the received record immediately based on the record content type handshake (instead of change cipher spec)

    • For ChaCha20_Poly1305 ciphertexts, the bytes of the ciphertext are parsed and likely result in a seemingly long handshake message. If the given Record does not contain enough bytes, Botan waits for additional records until enough bytes have been received before rejecting the message as unexpected.
    • The record can be rejected immediately based on the record content type or as soon as the handshake message type can be parsed (the latter can also be used if a server is expected to send a NewSessionTicket message)
  • [S] upon receiving a Padding extension that contains bytes other than 0x00

    • RFC 7685 - 3. Padding Extension

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

  • [S] upon receiving a SupportedPointFormat extension that only accepts compressed points or an invalid format

    • 8422 - 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.

  • [S] upon receiving a ClientHello that contains elliptic curve extensions but no ECC cipher suite

    • 8422 - 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.

Only session closed but no alert sent

  • [S] upon receiving an empty handshake record

  • [S] upon receiving an undefined or unexpected record content type before a ClientHello has been received

Different alert sent than defined by the RFC

  • [C] upon receiving a ServerHello that negotiates an unsupported legacy version. Botan sends a 'handshake failure' alert.
    • RFC 5246 - E.1. Compatibility with TLS 1.0/1.1 and SSL 3.0

      If the version chosen by the server is not supported by the client (or not acceptable), the client MUST send a "protocol_version" alert message and close the connection.

mmaehren avatar May 31 '21 17:05 mmaehren

Thanks!

My only comment at this time is that the padding extension finding is spurious - because Botan does not implement this extension, the padding bytes are opaque to us.

randombit avatar Jun 01 '21 12:06 randombit

Thank you for the fast feedback (yes, the padding extension is a false positive, sorry)

jurajsomorovsky avatar Jun 02 '21 07:06 jurajsomorovsky

For these

when a server chooses a non-CBC cipher suite but negotiates encrypt-then-MAC upon receiving a ClientHello that contains elliptic curve extensions but no ECC cipher suite

I'm not sure what to do. The RFC is expressing that one side MUST do something, and we acting as the peer of that side allow the peer to deviate. But the RFC only states that one side MUST X not that the other side must verify X. (In comparison this checking behavior is clearly specified for the case "upon receiving a SupportedPointFormat extension that only accepts compressed points or an invalid format")

randombit avatar Jun 05 '21 14:06 randombit

Yes, the RFCs aren't very clear what the other party should/must do in these cases. For TLS 1.3, RFC 8446 is clearer stating in 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 would apply to the 'non-CBC cipher suite negotiated + encrypt-then-MAC extension sent' case, if this was a TLS 1.3 session. Our tests showed that libraries in some cases do enforce these statements in TLS 1.2. However, other developers mentioned that it might not be favorable to enforce all of them.

mmaehren avatar Jun 06 '21 08:06 mmaehren

Hey, a minor addition to the above: Botan does not abort the handshake upon computing an all-zero premaster secret for curve X25519:

  • RFC 8422 - 5.11. Public Key Validation

With X25519 and X448, a receiving party MUST check whether the computed premaster secret is the all-zero value and abort the handshake if so, as described in Section 6 of [RFC7748].

Please note that care must be taken when implementing this check to avoid a side channel. This also applies to version 2.19.0

mmaehren avatar Jan 28 '22 09:01 mmaehren

cc @reneme

randombit avatar Mar 25 '23 11:03 randombit

Adding to the 3.0 milestone to keep a tap on this. Might slip to 3.1, though.

reneme avatar Mar 25 '23 12:03 reneme

In order to finally close this issue ticket, I sum up the fixes and observations we made regarding the findings discussed here.

Session not aborted

[C] when a server chooses a non-CBC cipher suite but negotiates encrypt-then-MAC

Fixed in 8222cf3199280c380df18c470383d0ae3d57f651.

[C] when a server omits the ChangeCipherSpec message and directly sends an encrypted Finished message, Botan client does not reject the received record immediately based on the record content type handshake (instead of change cipher spec)

Fixed in a27bf8f

[S] upon receiving a Padding extension that contains bytes other than 0x00

False positive

[S] upon receiving a SupportedPointFormat extension that only accepts compressed points or an invalid format

Fixed in afd6baab8

[S] upon receiving a ClientHello that contains elliptic curve extensions but no ECC cipher suite

I tried to fix this issue. However, it seems that the BoGo test suite fails with this additional check. Since we cannot see any security implications of this finding we decided to leave it out for now. Note that Botan does not send such invalid ClientHellos, it only allows other Clients to do so.

Only session closed but no alert sent

[S] upon receiving an empty handshake record

I probed this behavior in the current version using TLS-Attacker. We receive an Alert(FATAL,DECODE_ERROR) in this case. So it seems that the issue was already fixed since this issue was opened.

[S] upon receiving an undefined or unexpected record content type before a ClientHello has been received

I also probed this finding. It's also already fixed.

Different alert sent than defined by the RFC

[C] upon receiving a ServerHello that negotiates an unsupported legacy version. Botan sends a 'handshake failure' alert.

Also already fixed.

Other

With X25519 and X448, a receiving party MUST check whether the computed premaster secret is the all-zero value and abort the handshake if so, as described in Section 6 of [RFC7748].

Fixed in 4731b9eb

FAlbertDev avatar May 23 '23 07:05 FAlbertDev