Subject: Re: [quicwg/base-drafts] Rewrite key update section (#3050)

From: Kazuho Oku <[email-masked]>
Date: Fri, 01 Nov 2019 00:34:06 -0700

kazuho requested changes on this pull request.

-The KEY_PHASE bit allows a recipient to detect a change in keying material
-without necessarily needing to receive the first packet that triggered the
-change.  An endpoint that notices a changed KEY_PHASE bit can update keys and
-decrypt the packet that contains the changed bit.
+The Key Phase bit indicates which packet protection keys are used to protect the
+packet.  The Key Phase bit is initially set to 0 for the first set of 1-RTT
+packets and toggled to signal each subsequent key update.

I think that this sentence, along with the sentence stating that "an endpoint MUST NOT initiate a key update prior to having received an acknowledgment for a packet that it sent protected with keys from the current key phase" is introducing a design change.

At the moment, we allow the first key update to be executed when the handshake is confirmed, while prohibiting subsequent key updates happening before receiving an ACK for the preceding key update. That has meant that if an endpoint were to send its first 1-RTT packet after the handshake has been confirmed could send the first 1-RTT packet with the Key Phase bit set to 1. The new text prohibits that.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3050#pullrequestreview-310322927

---

From: Kazuho Oku <[email-masked]>
Date: Fri, 01 Nov 2019 00:57:20 -0700

kazuho commented on this pull request.

Aside from my previous review comment, I only have nitpicks.

Sorry for submitting comments at this moment.

> @@ -1168,89 +1168,226 @@ anticipation of receiving a ClientHello.
 
 # Key Update
 
-Once the handshake is confirmed, it is possible to update the keys. The
-KEY_PHASE bit in the short header is used to indicate whether key updates
-have occurred. The KEY_PHASE bit is initially set to 0 and then inverted
-with each key update.
+Once the handshake is confirmed (see {{handshake-confirmed}}), it is possible to
+update the keys used to protect packets.

We might want to change "possible to update the keys" to "possible to initiate a key update"? This is because an endpoint might receive a key update(s) before it confirms the handshake.

Or, we can punt such a change to the PR that will fix the handshake key discard problem.

 For the sending of packets, construction and protection of packet payloads and
 packet numbers MUST be free from side-channels that would reveal the packet
 number or its encoded size.
 
+During a key update, the time taken to generate new keys could reveal through
+timing side-channels that a key update has occurred.  Alternatively, where an
+attacker injects packets this side-channel could reveal the value of the Key
+Phase on injected packets.  After receiving a key update, an endpoint SHOULD
+generate and save the next set of receive packet protection keys.  By generating

Maybe add ", following the advices in {{receive-key-generation}}"?

I suggest this is because without such a note, "After receiving a key update" could be confusing.

 For the sending of packets, construction and protection of packet payloads and
 packet numbers MUST be free from side-channels that would reveal the packet
 number or its encoded size.
 
+During a key update, the time taken to generate new keys could reveal through
+timing side-channels that a key update has occurred.  Alternatively, where an
+attacker injects packets this side-channel could reveal the value of the Key
+Phase on injected packets.  After receiving a key update, an endpoint SHOULD
+generate and save the next set of receive packet protection keys.  By generating
+new keys before a key update is received, receipt of packets will not create
+timing signals that leak the value of the Key Phase.
+
+This depends on not doing this key generation during packet processing and it
+can require that endpoints maintain three sets of packet protection keys for
+receiving: for the previous key phase, for the current key phase, and for the
+next key phase.  Endpoints MAY instead choose to defer generation of the next

Maybe s/MAY/can/ as we have concrete advice using normative language in {{receive-key-generation}}.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3050#pullrequestreview-310324691

---

From: Martin Thomson <[email-masked]>
Date: Sun, 03 Nov 2019 15:31:57 -0800

martinthomson commented on this pull request.

I don't follow this comment.  Handshake confirmation requires ACK of a packet that was sent after handshake completion.  I'll try to remove that dependency, but it is currently functionally identical.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3050#discussion_r341880148

---

From: Martin Thomson <[email-masked]>
Date: Sun, 03 Nov 2019 15:33:55 -0800

@martinthomson pushed 3 commits.

bf1394db3c610de54286bbf7aafdb235599ba46f  Some small adjustments
289e57d5ed1f37e2f3ef1c36dc427c782b418576  Merge branch 'master' into rework-key-update-2
a7b0addaa2cb1a0fab865adb8f1d2437dca302d9  Split first from subsequent again

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3050/files/b1d56947b2cf5542481ec65fa7dfb7e4d434fcf9..a7b0addaa2cb1a0fab865adb8f1d2437dca302d9

---

From: Kazuho Oku <[email-masked]>
Date: Sun, 03 Nov 2019 17:07:06 -0800

Yeah that's true, but it's like filling a land mine that would explode once we change the definition of handshake complete.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3050#discussion_r341885240

---

From: Martin Thomson <[email-masked]>
Date: Sun, 03 Nov 2019 17:58:42 -0800

Merged #3050 into master.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3050#event-2766404836