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

Imprecise definitions for handling invalid records #283

Open
martinthomson opened this issue Sep 18, 2023 · 2 comments
Open

Imprecise definitions for handling invalid records #283

martinthomson opened this issue Sep 18, 2023 · 2 comments

Comments

@martinthomson
Copy link
Contributor

Section 4.5.2 talks about invalid record handling in a way that is not sufficiently precise.

Take this:

Unlike TLS, DTLS is resilient in the face of invalid records (e.g., invalid formatting, length, MAC, etc.). In general, invalid records SHOULD be silently discarded, thus preserving the association; [...]

This recommendation is not great, particularly as it attached to the preceding list, which is a little non-specific.

Quoting @dennisjackson from another context:

I do still think the phrasing in 4.5.2 is needlessly hostile by introducing the concept of an invalid record in the first place. If we asked implementers which of the following are considered invalid records, I'd suspect they'd say "all of them":

  • A record which consists of a zero length handshake message
  • A record which contains a fragment of an alert message (or two alert messages).
  • A record of application data which appears between two records of a fragmented handshake message.
  • A record which has an invalid inner content type.
  • A record which has an invalid outer content type.

I do think 4.5.2 would be significantly clearer if it didn't mention "invalid records" at all and just spoke about "errors arising from record handling".

To this point, it would be clearer if the spec were more definitive. Records that do not authenticate would always be discarded (i.e., use MUST) and records that do authenticate, but are otherwise invalid, result in fatal alerts always (i.e., use MUST). I don't see much value in having ambiguous language for something so important.

@ekr
Copy link
Collaborator

ekr commented Sep 18, 2023

@martinthomson thanks for raising this. I agree this text could be better, and as you suggest, this was intended to cover errors at the record layer (which was of course somewhat simpler in TLS 1.2). I agree with the resolution you propose but you omit the question of plaintext records. My sense is that they should be treated as authenticated for the purpose above. My reasoning for this is that there are three basic ways to get something that is malformed.

  • The peer generated invalid data.
  • Some kind of unintentional mangling (e.g., a bit error)
  • Deliberate mangling by an attacker (yes, I realize that the line between these last two is not as clear as one would like).

The primary reason to have DTLS discard malformed data is to make it difficult for attackers to DoS a DTLS connection the way they can with TLS. However, the attacker can obviously manipulate unprotected records and DoS the connection, so discarding them if they are malformed doesn't help that much. One could of course say that you're also worried about unintentional damage to the handshake but the UDP checksum makes that comparatively rare, so it's not clear it's worth the effort.

With all that said, we already published 9147, so it's a bit unobvious what to do about this. Do you have thoughts?

@martinthomson
Copy link
Contributor Author

Ah, I forgot to record my views on the handshake. I agree that it is probably best to treat these as having been authenticated, as you propose. That allows a somewhat consistent treatment throughout, falling back on Finished as the only means of authenticating those. As you say, the unintentional damage case is the only major gap here and that is not one that is statistically significant enough to worry about (just try the handshake again if you like).

I wouldn't consider this to be worth a revision of or update to RFC 9147. Maybe we can hold onto this until such a time as we need to make other changes.

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

No branches or pull requests

2 participants