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
_______________________________________________
TLS mailing list
TLS@ietf.org
https://www.ietf.org/mailman/listinfo/tls
