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

From: ekr <[email-masked]>
Date: Tue, 01 Oct 2019 14:25:34 -0700

ekr requested changes on this pull request.

I'm skeptical of this anti-timing advice and think we should remove it.

-In deciding when to update keys, endpoints MUST NOT exceed the limits for use of
-specific keys, as described in Section 5.5 of {{!TLS13}}.
+## Initiating a Key Update {#key-update-initiate}
+
+Endpoints maintain separate read and write secrets for packet protection.  An
+endpoint initiates a key update by updating its packet protection write secret
+and using that to protect new packets.  The endpoint creates a new write secret
+from the existing write secret as performed in Section 7.2 of {{!TLS13}}.  This
+uses the KDF function provided by TLS with a label of "quic ku".  The

Why aren't you using HKDF-Expand-Label here. It's defined in QUIC-TLS 5.2

> +## Initiating a Key Update {#key-update-initiate}
+
+Endpoints maintain separate read and write secrets for packet protection.  An
+endpoint initiates a key update by updating its packet protection write secret
+and using that to protect new packets.  The endpoint creates a new write secret
+from the existing write secret as performed in Section 7.2 of {{!TLS13}}.  This
+uses the KDF function provided by TLS with a label of "quic ku".  The
+corresponding key and IV are created from that secret as defined in
+{{protection-keys}}.  The header protection key is not updated.
+
+For example, to update write keys with TLS 1.3, HKDF-Expand-Label is used as:
+
+~~~
+secret_<n+1> = HKDF-Expand-Label(secret_<n>, "quic ku",
+                                 "", Hash.length)
+~~~

This is weird because it results in "quic quic ku"

> +The endpoint toggles the value of the Key Phase bit and uses the updated key and
+IV to protect all subsequent packets.
+
+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.  This ensures that keys are available to both peers before another
+can be initiated.
+
+Note:
+
+: Keys of packets other than the 1-RTT packets are never updated; their keys are
+  derived solely from the TLS handshake state.
+
+The endpoint that initiates a key update also updates the keys that it uses for
+receiving packets.  These keys will be needed to process packets the peer sends
+after updating.

This text seems like dicta. You can wait until you receive the other side's first packet in the new epoch.

> +
+Endpoints responding to an apparent key update MUST NOT generate a timing
+side-channel signal that might indicate that the Key Phase bit was invalid (see
+{{header-protect-analysis}}).  Endpoints can use dummy packet protection keys in
+place of discarded keys when key updates are not permitted; using dummy keys
+will generate no variation in the timing signal produced by attempting to remove
+packet protection, but all packets with an invalid Key Phase bit will be
+rejected.
+
+The process of creating new packet protection keys for receiving packets could
+reveal that a key update has occurred.  An endpoint MAY perform this process as
+part of packet processing, but this creates a timing signal that can be used by
+an attacker to learn when key updates happen and thus the value of the Key Phase
+bit in certain packets.  Endpoints SHOULD instead defer the creation of the next
+set of receive packet protection keys until some time after a key update
+completes, up to three times the PTO; see {{old-keys-recv}}.

As I noted in Montreal, I don't think this is really that useful advice, for three reasons:

1. it's not really that useful to avoid leaking the key phase.
2. It's not clear to me what the timing signal is, given that one doesn't generally respond to QUIC packets immediately without any other processing and this timing signal isn't subject to the usual statistical amplification techniques.
3. It's a lot more complicated.

So we should remove the SHOULD here.

> +Once generated, the next set of packet protection keys SHOULD be retained, even
+if the packet that was received was subsequently discarded.  Packets containing
+apparent key updates are easy to forge and - while the process of key update
+does not require significant effort - triggering this process could be used by
+an attacker for DoS.
+
+For this reason, endpoints MUST be able to retain two sets of packet protection
+keys for receiving packets: the current and the next.  Retaining the previous
+keys in addition to these might improve performance, but this is not essential.
+
+The time taken to generate new keys could reveal through timing side channels
+that a key update has occurred, or where an attacker injects packets, be used to
+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.  After new keys are available, receipt of packets will not
+create timing signals about the value of the Key Phase.

See above. Absent a clearer description of the threat, this seems overcomplex.

> +
+For receiving packets during a key update, packets protected with older keys
+might arrive if they were delayed by the network.  Retaining old packet
+protection keys allows these packets to be successfully processed.
+
+As packet protected with keys from the next key phase use the same Key Phase
+value as those protected with keys from the previous key phase, it can be
+necessary to distinguish between the two.  This can be done using packet
+numbers.  A recovered packet number that is lower than any packet number from
+the current key phase uses the previous packet protection keys; a recovered
+packet number that is higher than any packet number from the current key phase
+requires the use of the next packet protection keys.
+
+Some care is necessary to ensure that any process for selecting between
+previous, current, and next packet protection keys does not expose a timing side
+channel that might reveal which keys were used to remove packet protection.

And again.... What is the thread?

-- 
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-295899646

---

From: Martin Thomson <[email-masked]>
Date: Thu, 10 Oct 2019 16:21:58 -0700

@martinthomson pushed 1 commit.

b35a5e4870cac65f2d7b0d37b21a837a7142a472  Ian's review comments

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

---

From: ianswett <[email-masked]>
Date: Fri, 11 Oct 2019 14:20:56 -0700

ianswett approved this pull request.

-- 
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-300933032

---

From: Martin Thomson <[email-masked]>
Date: Tue, 29 Oct 2019 23:44:23 -0700

martinthomson commented on this pull request.

@ekr, I think that it is up to us to work out what the key update side channel looks like.

(Sorry for leaving these comments pending for so long, I thought that I had posted them already.

 This mechanism replaces the TLS KeyUpdate message.  Endpoints MUST NOT send a
 TLS KeyUpdate message.  Endpoints MUST treat the receipt of a TLS KeyUpdate
 message as a connection error of type 0x10a, equivalent to a fatal TLS alert of
 unexpected_message (see {{tls-errors}}).
 
-An endpoint MUST NOT initiate the first key update until the handshake is
-confirmed ({{handshake-confirmed}}). An endpoint MUST NOT initiate a subsequent
-key update until it has received an acknowledgment for a packet sent at the
-current KEY_PHASE.  This can be implemented by tracking the lowest packet

My bad.  Refactoring collateral.

We are.  That's the KDF TLS provides.

To your point below, the label is "tls13 quic ku".  When we get TLS 1.4, assuming that it makes minimal changes, we'll use HKDF-Expand-Label with "tls14 quic ku".

+

See above.  We're not changing the label the KDF uses internally.

Elsewhere we have "tls13 " + "quic iv" or "quic key" or "quic hp".

This is part of the removal of timing side channels.  I know that you think that's overcomplex, but I am current disinclined to agree.  More later.

I disagree.

I agree with you that the leak is minor.  And that acquiring the signal might be difficult (but nor do I think that it's impossible).

However, I think that the fix is cheap.  As long as you aren't resource-constrained, you just ensure that you always have the next set of keys available.  The recommendation is really only around deferring that processing.  It's not zero effort, but I doubt that "a lot more complicated" is accurate.

-- 
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-300403036

---

From: Martin Thomson <[email-masked]>
Date: Wed, 30 Oct 2019 22:33:08 -0700

Though consensus has been declared, I talked to @ekr about reviewing this and I will wait for that review before merging.

-- 
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#issuecomment-548223154

---

From: David Schinazi <[email-masked]>
Date: Thu, 31 Oct 2019 11:52:47 -0700

While I'm fully supportive of the spirit of this PR, I still have some comments on here that haven't been addressed (for example the implementation guidance text was apparently accidentally removed)

-- 
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#issuecomment-548519441

---

From: Martin Thomson <[email-masked]>
Date: Thu, 31 Oct 2019 15:31:43 -0700

c131d08809b85bc2cdf56c67eab62741414571f2  Restore implementation advice

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

---

From: Martin Thomson <[email-masked]>
Date: Thu, 31 Oct 2019 15:32:09 -0700

My bad @DavidSchinazi.  I had restored that text in my local copy, but failed to push.

-- 
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#issuecomment-548595098

---

From: David Schinazi <[email-masked]>
Date: Thu, 31 Oct 2019 15:46:12 -0700

DavidSchinazi approved this pull request.

Thanks for adding back the implementation text. I like the new text.

I would personally prefer to see all the timing side-channel text in its own subsection, because it makes the more important part of the spec harder to read, and I don't see this attack as important enough to warrant worrying about when trying to understand key updates.

> +An endpoint SHOULD retain old keys so that packets sent by its peer prior to
+receiving the key update can be processed.  Discarding old keys too early can
+cause delayed packets to be discarded.  Discarding packets will be interpreted
+as packet loss by the peer and could adversely affect performance.
+
+
+## Responding to a Key Update
+
+A peer is permitted to initiate a key update after receiving an acknowledgement
+of a packet in the current key phase.  If a packet is received with a key phase
+that differs from the value the endpoint used to protect the last packet it
+sent, the endpoint uses the next packet protection keys for reading and the
+corresponding key and IV; see {{receive-key-generation}} for considerations
+about generating these keys.
+
+An endpoint uses the same key derivation process as its peer uses to generate

I'm not parsing this sentence well. What does it mean?

-- 
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-310218613

---

From: Martin Thomson <[email-masked]>
Date: Thu, 31 Oct 2019 15:59:14 -0700

c764362e3e50f0647b930648de5b645e4cc65ad4  Move some text around

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

---

From: Martin Thomson <[email-masked]>
Date: Thu, 31 Oct 2019 15:59:58 -0700

I cut it.  It isn't necessary.

The point was to say that you need to generate the same keys for reading that your peer uses for writing.  But it was full of fail.

-- 
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_r341399076

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 31 Oct 2019 21:06:31 -0700

janaiyengar approved this pull request.

A few nits. I've asked @kazuho if he could take another look as well.

```suggestion
key phase.  This ensures that keys are available to both peers before another key update
```

+after updating.
+

It might be kinder to the reader if you could split this 3-line sentence into two sentences.

> +its peer has updated keys twice without awaiting confirmation.  An endpoint MAY
+treat consecutive key updates as a connection error of type KEY_UPDATE_ERROR.
+
+An endpoint that receives an acknowledgement that is carried in a packet
+protected with old keys where any acknowledged packet was protected with newer
+keys MAY treat that as a connection error of type KEY_UPDATE_ERROR.  This
+indicates that a peer has received and acknowledged a packet that initiates a
+key update, but has not updated keys in response.
+
+
+## Timing of Receive Key Generation {#receive-key-generation}
+
+Endpoints responding to an apparent key update MUST NOT generate a timing
+side-channel signal that might indicate that the Key Phase bit was invalid (see
+{{header-protect-analysis}}).  Endpoints can use dummy packet protection keys in
+place of discarded keys when key updates are not yet permitted; using dummy keys

```suggestion
place of discarded keys when key updates are not yet permitted. Using dummy keys
```

-- 
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-310283028

---

From: Martin Thomson <[email-masked]>
Date: Thu, 31 Oct 2019 21:31:02 -0700

b1d56947b2cf5542481ec65fa7dfb7e4d434fcf9  janaiyengar's suggestions

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