tlswg / dtls13-spec

Repo for DTLS 1.3
32 stars 25 forks source link

Imprecise definitions for handling invalid records #283

Open martinthomson opened 1 year ago

martinthomson commented 1 year ago

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 commented 1 year ago

@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 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 commented 1 year ago

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.