Subject: [QUIC] Review of draft-thomson-quic-tls-01

From: Jana Iyengar <[email-masked]>
Date: Fri, 11 Nov 2016 22:02:13 -0800

Thanks for your work so far -- this is an important document, so thanks for
the work you've put in!

Overall, I keep having to check what TLS 1.3 mechanisms and messages are
being referred to in the draft. Just as you have a QUIC overview I would
add a TLS 1.3 overview.

Since there are two encryptors at play (packet and tls messages), the
document should probably explain this more clearly, including  in key
transitions. I remain confused about which packets are encrypted in this
proposal.

More detailed comments below.

- Section 2: (bullet 3) Version Negotiation is a type of packet, there are
no frames in it.

- Section 2: (bullet 4) "Framing" is used when perhaps you mean to say
"Frames"? Usage of QUIC frames is not framing; at least it's not been
defined in that way. It's also confusing since framing is generally used
more commonly to refer to wire formats. I would reword.

- Section 2: (bullet 5) There's confidentiality and integrity protection
for the entire packet, not just for frames.

- Fig 1 is confusing. Specifically, abstractions, framing, and functions
are all present in the same layered picture which makes it confusing. For
eg., a "Stream" is an abstraction, "congestion control" is a function, and
"frames" refer to wire format representation. I'm not sure this picture
helps clarify how various functions are layered.

- Fig 1: What is "Envelope"? Is this the QUIC header? If so, some of it is
currently encrypted, which means encryption cannot be above it.

- Section 3: "Prior to the completion of the TLS handshake, QUIC frames can
be exchanged. However, these frames are not authenticated or
confidentiality protected." This is not true. 0-RTT data will be sent
protected before the handshake is completed.

- Section 3.1: What key is the packet encrypted with? The application key?
I recall the plan being packets containing handshake messages to go
unencrypted, but I don’t remember why. Is this still the plan? If so, are
KeyUpdate messages sent later also sent in an unencrypted packet? Why not
just use the application key for encrypting the packet at all times?

- What does “QUIC provides a record protection layer” mean? This is new
terminology at the least, so the “record protection layer” should be
defined.

- Section 4.1 says:
   “The KEY_PHASE bit allows a recipient to detect a change in keying
   material without needing to receive the message that triggers the
   change.  This avoids head-of-line blocking around transitions between
   keys without relying on trial decryption.”
How does this avoid head-of-line blocking around transitions? What
head-of-line blocking do you have in mind here? Or do you mean to say that
this bit allows handling of reordered packets correctly without relying on
trial-decryption?

- Why does the server need to send anything with a 0-RTT encrypter? Doesn’t
the server move immediately to 1RTT keys for sending, but keeps 0RTT-key
and 1RTT-key decrypters?

- I like the phase changes, but I’m still a bit nervous. I think (I need to
think more) that it will work out ok, but some more text here will help,
especially on why a KEY_PHASE of 0 or 1 is unambiguous on both sides, 0-RTT
or not, and why it is not possible to end up in strange states. (I now see
text that follows explains this. Perhaps add a forward pointer here.)

- “0-RTT keys SHOULD be retained until the handshake is complete.” Why’s
this a SHOULD and not a MUST? Why is this sentence required at all?

- “A client would be unable to decrypt retransmissions of a server's
   handshake messages that are protected using the 1-RTT keys, since the
   calculation of the application data keys depends on the contents of
   the handshake messages.”
This is a confusing sentence. Since you’re talking about decryption of
handshake messages, why do the application data keys matter?

- “As shown in Figure 3 and Figure 4, there is never a situation where
   there are more than two different sets of keying material that might
   be received by a peer.”
With reordering, I don’t think this statement is true. We need rules using
packet numbers, such that an old packet with a key phase matching the
current expected should not cause the connection to blow up.

- Section 4.1.2: I should probably go read about Key Updates in TLS 1.3
before trying to understand the details in this section. If so, a pointer
somewhere would be useful.

- Section 4.3: “The nonce, N, for the AEAD is formed by combining either
the Client
Write IV or Server Write IV with the sequence numbers.” What’s the
Client/Server Write IV? The next couple of paragraphs are quite confusing.
Why is there no Associated Data? QUIC currently uses the unencrypted packet
header as Associated Data. The QUIC packet does not have frames following
the packet number…. it’s not clear what it means for P to be the “QUIC
frame following the packet number”?

- I’m surprised to see a TLS sequence number, given that QUIC has one (and
currently uses just that.) It seems like this may be for Key Updates later
in the connection. I’d like to understand this better. In particular, it’s
not clear to me why reusing sequence numbers is better than not. I’m also
curious to understand how important Key Update is, given that 0-RTT
connection establishment nearly eliminates the cost of re-establishing a
QUIC connection.

- Section 5: “Implementations MUST NOT exchange data on any stream other
than
   stream 1 prior to the completion of the TLS handshake.” This does not
work, since 0-RTT data is sent on other streams before handshake completion.

- Section 5.1.2: I would like an explicit  requirement on ACKs to be
encrypted at the same level or higher as the packet that the ACK
acknowledges. Is that adequate for this section?

- Section 5.1.4: “Note that
   the client could retransmit its Finished message to the server, so
   the server cannot reject such a message.” Don’t understand this sentence.

- “Never assume that because it isn't in the security considerations
   section it doesn't affect security.  Most of this document does.” Seems
unnecessary. Also, seems silly to say that “most” of this document affects
security, since “all” of it does.

---

From: Ian Swett <[email-masked]>
Date: Sat, 12 Nov 2016 21:04:24 -0500

Thanks for the great progress on this document.  It's definitely getting
much closer.

I had some of the same comments as Jana and added some notes and a few new
comments.

On Sat, Nov 12, 2016 at 1:02 AM, Jana Iyengar <[email-masked]> wrote:

> Thanks for your work so far -- this is an important document, so thanks
> for the work you've put in!
>

Now that private flags are gone, there is no encrypted portion of the QUIC
header.   That being said, it's likely worth clarifying what envelope is.

>
> - Section 3: "Prior to the completion of the TLS handshake, QUIC frames
> can be exchanged. However, these frames are not authenticated or
> - Why does the server need to send anything with a 0-RTT encrypter?
> Doesn’t the server move immediately to 1RTT keys for sending, but keeps
> 0RTT-key and 1RTT-key decrypters?
>
> - I like the phase changes, but I’m still a bit nervous. I think (I need
> to think more) that it will work out ok, but some more text here will help,

> acknowledges. Is that adequate for this section?

I think that's what this section says, but it could be a bit more direct
about it.

> security, since “all” of it does.
Section 4.1 says: "The KEY_PHASE bit on the public flags is the most
significant bit (0x80)."  I would suggest we use the current
diversification nonce bit.(0x04), as it is otherwise unused in the TLS 1.3
version of QUIC.

Section 4.5 goes into Packet Number reconstruction.  I don't think it's
necessary to do this here(and there is a small error), though if you want
to state properties of packet numbers(ie: monotonically increasing on the
sender side) that seems fine if it's helpful to the mapping doc.

Section 5.1.4 says "The ability to shadow packets means that an attacker
does not need to be on path."  I believe the attacker must be on path,
unless I'm misunderstanding something?  In regards to starting at a
randomized packet number, that does provide some level of protection, and
seems wise.  It also reduces the chances of ossification on the handshake
packets always starting at packet number 1.

I think "In addition to denying endpoints messages, an attacker to generate
packets that cause no state change in a recipient." needs to be
reworded, because I'm rather confused on what it's saying.

Can you clarify "MUST reject empty TLS handshake records and any record
that is not permitted by the TLS state machine".  Should these packets be
ignored, under the assumption they're attack packets?  Or should the
handshake fail?

5.3 is confusing to me.  For example, it says "Due to reordering and loss,
protected packets might be received by an endpoint before the final
handshake messages are received. If these can be decrypted successfully,
such packets MAY be stored and used once the handshake is complete."  I
think this deserves more specificity.  In cases when the packets are
encrypted with 1RTT keys, may should be stored for later decryption, but
there's no chance of them being decrypted before the SHLO wasn't received.
I'm a bit confused about what other cases you are thinking of in the
section?

6.4 says "Even if the source IP address remains constant, the port number
is likely to be different." between TCP and QUIC.  In typical cases, the
port number is new for every connection anyway, so I'm not sure this is a
relevant concern.  I would just remove that sentence.

7: clanger, though it looks like dangers, is not the same word :)

7.1: I think we need to loosen "A server SHOULD use a HelloRetryRequest if
the size of the handshake messages it sends is likely to exceed the size of
the ClientHello." slightly to "a small multiple of the size of the
ClientHello", or something similar, because multipacket certs are extremely
common.

---

From: Martin Thomson <[email-masked]>
Date: Sun, 13 Nov 2016 11:22:36 +0900

Thanks guys, as you might imagine, I'm a little busy this week, but I
will look at your comments and respond when I have more time.  I hope
that I can also update the doc.

Ian, clangers AND dangers, sometimes even both, sometimes the latter
because of the former :)

On 13 November 2016 at 11:04, Ian Swett <[email-masked]> wrote:
>> being referred to in the draft. Just as you have a QUIC overview I would add
>> a TLS 1.3 overview.
>>
>> the packet number…. it’s not clear what it means for P to be the “QUIC frame
>> following the packet number”?
>>
> significant bit (0x80)."  I would suggest we use the current diversification
> nonce bit.(0x04), as it is otherwise unused in the TLS 1.3 version of QUIC.
>
> necessary to do this here(and there is a small error), though if you want to
> state properties of packet numbers(ie: monotonically increasing on the
> packets that cause no state change in a recipient." needs to be reworded,
> because I'm rather confused on what it's saying.
>
> Can you clarify "MUST reject empty TLS handshake records and any record that
> is not permitted by the TLS state machine".  Should these packets be
> 6.4 says "Even if the source IP address remains constant, the port number is
> likely to be different." between TCP and QUIC.  In typical cases, the port
> number is new for every connection anyway, so I'm not sure this is a

---

From: Jana Iyengar <[email-masked]>
Date: Sat, 12 Nov 2016 22:13:34 -0800

> header.   That being said, it's likely worth clarifying what envelope is.

Good point -- I forgot that we didn't have private flags anymore.

---

From: Martin Thomson <[email-masked]>
Date: Mon, 28 Nov 2016 17:46:40 +1100

Jana, Ian,

Thanks for the reviews.

For the biggest issue here, I've been working on getting a better
overview in.  It's not 100% complete, because I've run into the issue
with KEY_PHASE that I was going to discuss at the meeting.

You can see where I'm at here:

PR: https://github.com/quicwg/base-drafts/pull/30
HTML: https://quicwg.github.io/base-drafts/tls-interface-overview/draft-ietf-quic-tls.html

I've fix the obvious issues you identified, but I've also opened a ton
of issues, either because the answer isn't easy or because I'm lazy
and want to defer work.  See inline.  Ian, I've responded to your
comments as well.

> - Section 2: (bullet 3) Version Negotiation is a type of packet, there are

I've killed this section; it's too much effort to maintain.

> be exchanged. However, these frames are not authenticated or confidentiality
> protected." This is not true. 0-RTT data will be sent protected before the
> handshake is completed.

Yes, this was intended to capture the 1-RTT data.  I've removed the
offending text, but there are probably a few other things that need
correcting too.

I predict that what will catch us up is that 1-RTT keys are available
before the handshake completes.  I don't have a good fix for that yet.

> - Section 3.1: What key is the packet encrypted with? The application key? I
> recall the plan being packets containing handshake messages to go

Several responses are needed here:

The purpose of this section is to talk about QUIC packet protection.

Regarding handshake encryption: There's a trick here, and I think that
I've done a better job of fixing things to make it clear, let me know
if I missed the mark.  TLS protects its own handshake, but QUIC
doesn't.  That means that handshake encryption is at the TLS layer and
the QUIC packet protection sees cleartext.  This is simpler because it
means that QUIC has one fewer key to deal with.

Regarding KeyUpdate:  The design for KeyUpdate was flawed in that
version of the draft.  I shared a version that describes how to use
the scheme that Hannes Tschofenig proposed in his DTLS 1.3 document,
which I think is more robust.  That scheme doesn't require any packets
on stream 1 to be exchanged, just an update to the in-use keys and a
flip of the KEY_PHASE bit.

> terminology at the least, so the “record protection layer” should be

Yes, we've agreed that it should be packet protection.  That keeps the
terminology cleanly separated from TLS: TLS uses *record* protection.

> this bit allows handling of reordered packets correctly without relying on

Yes, that.  There are some designs where the content of a KeyUpdate
message would be required in order to get the next set of keys, which
would cause there to be HOL blocking, but I haven't proposed that (and
don't plan to).

> the server move immediately to 1RTT keys for sending, but keeps 0RTT-key and
> 1RTT-key decrypters?

Yeah, this is corrected in the latest version.

> text that follows explains this. Perhaps add a forward pointer here.)

I'm still working out how to properly explain this.  It's even more
complicated than I've managed to convey.  More on this when I get a
fix to the bug with KEY_PHASE and 0-RTT (issue #27).

> this a SHOULD and not a MUST? Why is this sentence required at all?

When we originally implemented 0-RTT in TLS 1.3, we stopped sending
early data as soon as we saw the ServerHello.  But if we lost parts of
the server's first flight of handshake messages, we might have
processed the ServerHello a whole round trip (or more) before the rest
of the messages that follow it.  That meant dead air because we
stopped sending early data but we couldn't send the fully-protected
data yet because we hadn't finished with the handshake.  It's more
complicated code now, but we wait until the first flight is done
before switching to 1-RTT.

This won't be an issue with the fix I have in mind for #27.

> handshake messages, why do the application data keys matter?

Yeah, it's definitely convoluted.

The usual rules are that endpoints retransmit frames with the latest
keys.  If the server follows those rules, then it will encrypt
retransmissions of its handshake messages with 1-RTT keys.  But the
client needs those handshake messages to derive the 1-RTT keys.

I've put what I think is a clearer description in the latest.

> current expected should not cause the connection to blow up.

Yeah, you can't assume that because you have acked an update, you will
have completely flushed out everything from 2 generations ago.

https://github.com/quicwg/base-drafts/issues/32
https://github.com/quicwg/base-drafts/issues/33

The second one is a special one just because you assumed that we'd
drop a connection if decryption failed.  I assume that you didn't
think about it very long, but we have to get this cemented into place:
if it doesn't decrypt, it didn't exist (with exceptions, because all
the best rules have exceptions).

> somewhere would be useful.

I wouldn't bother, it's a mess in that version of the document, the
draft in the repo has a bunch of changes that (I think) make it much
clearer.

> - Section 4.3: “The nonce, N, for the AEAD is formed by combining either the
> Client

Write IV?  Most of the confusion here is due to TLS terminology
leakage.  I hope.  I've tried to fix that up.

There is no associated data because we don't need it.  That's nice
because the packet protection design is exactly the same as the (D)TLS
record protection design, and more efficient as a result.

> - I’m surprised to see a TLS sequence number,

A straight up mistake on my part.

> acknowledges. Is that adequate for this section?

Sounds like a great idea, but it's not easy to handle 0-RTT:
https://github.com/quicwg/base-drafts/issues/34

>    the server cannot reject such a message.” Don’t understand this sentence.

Nor do I.  Deleted.

> security, since “all” of it does.

Yeah, it's my little response to the insistence that we have security
considerations in every document, even the security ones.

And it *is* only most of the document.

> header.   That being said, it's likely worth clarifying what envelope is.

Yeah, I'm going to see if I can clobber that term instead.

> nonce bit.(0x04), as it is otherwise unused in the TLS 1.3 version of QUIC.

Yeah, https://github.com/quicwg/base-drafts/issues/26

> sender side) that seems fine if it's helpful to the mapping doc.

Already gone :)

> packets always starting at packet number 1.

The attacker needs to know the connection ID, I guess.  And that means
on-path, so I guess that randomization might not help there.
https://github.com/quicwg/base-drafts/issues/36

Let's track the starting sequence number here:
https://github.com/quicwg/base-drafts/issues/35

> because I'm rather confused on what it's saying.

Yeah, I'm trying to get at the classic DoS vector: cause someone to do
something, but have them think that they did nothing so that they
don't blame you for the wasted resources.  If you have a message that
causes busy work, but results in no change to state, sometimes that
slips through the net.  I've taken another stab at this.

> ignored, under the assumption they're attack packets?  Or should the

Handshake failure I think.  We haven't gotten that bedded down yet:
https://github.com/quicwg/base-drafts/issues/37

> I'm a bit confused about what other cases you are thinking of in the

Yeah, this is actually wrong.  There is only one case where you
receive a packet that you can decrypt, but can't use.  That is when
you receive a 1-RTT packet at the server, but you haven't
authenticated the connection (because the client Finished was
delayed).

I'm not 100% confident about the text here, so I've created a PR for
this one: https://github.com/quicwg/base-drafts/pull/39

> relevant concern.  I would just remove that sentence.

The point is to highlight that source address validation isn't
portable between the two protocols.  I think that's still a relevant
point to make.

> 7: clanger, though it looks like dangers, is not the same word :)

Yep :)

> ClientHello", or something similar, because multipacket certs are extremely

Yeah, the right answer will need some horse trading:
https://github.com/quicwg/base-drafts/issues/38

---

From: Ian Swett <[email-masked]>
Date: Tue, 29 Nov 2016 14:47:46 -0500

Thanks for your large update and all the new open issues.  I'm adding a few
more comments, but it's great progress.

On Mon, Nov 28, 2016 at 1:46 AM, Martin Thomson <[email-masked]>
wrote:

> can
> confidentiality
> Doesn’t
> and
> to
> the
> diversification
> that