Re: [TLS] draft-ietf-tls-dtls13-31 comments

["Christopher Wood" <[e-mail]>]

Thanks for the review, David! Please see inline below for comments. 

On Wed, Apr 3, 2019, at 12:43 PM, David Benjamin wrote:
> Hi all,
> 
> I read through draft-ietf-tls-dtls13-31 and gathered a bunch of 
> comments. I uploaded https://github.com/tlswg/dtls13-spec/pull/85 for 
> the easy bits. The remainder I figured I should send to the list. 
> Hopefully this is a usable format. Apologies for the very late look 
> over it. I haven't had time to page it in before now.
> 
> In section 4, I assume that if not negotiated, CID cannot be present 
> and the receiver rejects any records with it? It might be worth 
> spelling that out explicitly.

That's my understanding as well.

> Section 4.2.1 reads odd to me. I assume the references to 1 and 2 are 
> just as an example and not a particular oddity from those epochs. Maybe 
> stick a "for example" or say "earlier and later epoch". It also says 
> that during the handshake, "implementations MUST accept packets from 
> the old epoch", which is ambiguous as there are several epochs in the 
> handshake. That sentence also runs counter to the SHOULD immediately at 
> the start of the paragraph, which is odd.

Agreed that some clarifying text would help hear. 

> Section 4..5.1 talks about timing channels for the sequence number. 
> Note even uncompressing the sequence number (finding the right upper 
> bytes) introduces a timing channel. Granted, it is a much much weaker 
> one than record deprotection, but invalid DTLS records being non-fatal 
> means signals can be amplified....

Perhaps the text could acknowledge this additional, albeit smaller timing channel?

> Section 5 says that implementations MUST ignore ChangeCipherSpec 
> messages. This contradicts section 4.1, which does not include 
> change_cipher_spec as one of the possible record rtypes for 
> DTLSPlaintext. It does fall through to section 4.5.2, but 4.5.2 allows 
> for implementations to reject these. (Is there actually a need to 
> ignore them in DTLS 1.3 if the middlebox hacks aren't applicable?)

Perhaps 4.1 could leave a comment saying change_cipher_spec is not permitted, along with a forward pointer to Section 5?

> I don't understand the last paragraph of section 5.1. What's it trying 
> to describe? If the client got its hands on an invalid cookie, there's 
> no recovering that handshake. The server won't accept the cookie, and 
> the client won't accept a second HRR.

Perhaps what's missing is precisely that clarifying statement. Namely, that clients which find themselves in such a situation cannot recover.

> Section 5.3 says that the supported_versions extension is used without 
> modification from TLS 1.3, but that still leaves its contents 
> ambiguous. Is DTLS 1.3 0304 or fefc? I assume 0304 since fefc isn't 
> mentioned. But then how are DTLS 1.0 and DTLS 1.2 encoded? (One could 
> imagine omitting them, but that doesn't match how TLS 1.3 did it.) I 
> assume they remain feff and fefd, so we don't need to answer whether 
> DTLS 1.0 is 0301 or 0302. Either way, this should probably be spelled 
> out.

My interpretation was 0304 as well. 

> This is a nitpick and exists in DTLS 1.2 too, but section 5.4 uses the 
> term "maximum handshake fragment size", but nowhere else in the 
> document is the term defined or referenced. This means the largest 
> handshake fragment such that the resulting DTLS record fits in a 
> packet. (Though even that's incomplete because you might have 
> previously allocated a smaller fragment and then have less room left 
> for the next fragment. It gets further messier when packing fragments 
> into records and having to worry about when you are and aren't forced 
> to add a record boundary for epoch change...)

+1 -- defining this term would be good.

> 
> Section 5.9 is weird. With warning alerts gone, all alerts are fatal. 
> Either you've torn down all local state and don't resend alerts (I 
> suspect most applications just do this, for better or worse), or maybe 
> you're hanging around for a while to resend them. If the latter, you 
> probably don't want to be trying to detect that the offending record 
> got resent, since that involves processing records out of a connection 
> in an error state. It seems better to blindly retransmit on every 
> packet until you get bored.
> 
> Relatedly, what does close_notify mean in DTLS? This draft and DTLS 1.2 
> don't seem to make any mention of it. Bidirectional shutdown doesn't 
> make much sense. Not sure if unidirectional is meaningful, given lack 
> of retransmits. Maybe a best effort thing to save the peer a timeout.

+1

> 
> Section 6.1 says:
> 
>  Implementations that receive a payload with an epoch value for which
>  no corresponding cipher state can be determined MUST generate a
>  "unexpected_message" alert.
> 
> This seems to be cover data from the next epoch, which contradicts 
> section 4.2.1. (If I receive, say, EncryptedExtensions before 
> ServerHello, I can't determine the cipher state for that epoch.) 
> Section 4.2.1 says you should either buffer or drop those records, not 
> reject them.

Good catch. This should probably be cleaned up to ensure out-of-order handshake packets are handled correctly and do not yield an alert. 

> 
> Section 7 defines ACKs with a uint64 record_number. I assume this is a 
> concatenated epoch and sequence number, but I don't see anywhere 
> defines this. Perhaps just write it as:
> 
>  struct {
>  uint16 epoch;
>  uint48 sequence_number;
>  } RecordNumber;
> 
>  struct {
>  RecordNumber record_numbers<0..2^16-1>;
>  } ACK;

Good clarification! 

> 
> Relatedly, what is the "record sequence number" input to the AEAD (RFC 
> 8446, section 5.3)? DTLS believes that the "sequence number" is 48 
> bits, but the rest of TLS 1.3 is written expecting a 64-bit sequence 
> number. Using the concatenation is simplest and also matches RFC 7905, 
> but should be mentioned explicitly. (RFC 6347 didn't have text to this 
> effect because it only had AES-GCM which, in (D)TLS 1.2, used explicit 
> IVs.)
> 
> (I will probably have other comments on the ACK bits, but I think I 
> need to digest it more carefully to try to understand what's going on. 
> It does seem a little overkill for what is ultimately a small handful 
> of packets...)

Sounds good. Thanks again for the review!

Ekr, Hannes, Nagendra: can you please have a look through the comments and try to address them?

Best,
Chris