Subject: [quicwg/base-drafts] Mask packet numbers with a per-connection-ID key (#1043)

From: Martin Thomson <[email-masked]>
Date: Tue, 09 Jan 2018 06:22:11 +0000 (UTC)

This changes the key schedule so that connection ID is part of the key
derivation.  Secrets are generally unchanged, except that the handshake secret
is now fixed (we integrate the connection ID via key derivation).

Packet numbers now start from zero, with an additive masking value that is
derived in the same way that packet protection keys are.  Also, the type.

Closes #1034, #850.
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/1043

-- Commit Summary --

  * Mask packet numbers with a per-connection-ID key

-- File Changes --

    M draft-ietf-quic-tls.md (199)
    M draft-ietf-quic-transport.md (168)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/1043.patch
https://github.com/quicwg/base-drafts/pull/1043.diff

-- 
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/1043

---

From: MikkelFJ <[email-masked]>
Date: Tue, 09 Jan 2018 08:14:00 +0000 (UTC)

mikkelfj commented on this pull request.

> -indistinguishable from random. The packet number gap for a connection
-ID with sequence number is computed by encoding the sequence number
-as a 32-bit integer in big-endian format, and then computing:
-
-~~~
-Gap = HKDF-Expand-Label(packet_number_secret,
-                        "QUIC packet sequence gap", sequence, 4)
-~~~
-
-The output of HKDF-Expand-Label is interpreted as a big-endian
-number. "packet_number_secret" is derived from the TLS key exchange,
-as described in Section 5.6 of {{QUIC-TLS}}.
+connection ID provided by the server.  Using a new connection ID will produce a
+new packet protection key and IV.  New values for obscuring header fields (see
+{{header-obscuring}}) ensure that packet numbers can't be used to link activity
+on paths that use different connection IDs.

Is this optional. @ianswett suggested elsewhere that a packet might be dropped if it receives a packet with a tuple that does not match the connection ID. If so, a new connection ID must be used when transmitting on a new path. OTOH, but I am not sure about the implications with NAT rebinding.

> @@ -2377,8 +2386,11 @@ Unlike TCP SACKs, QUIC acknowledgements are irrevocable.  Once a packet has
 been acknowledged, even if it does not appear in a future ACK frame,
 it remains acknowledged.
 
-A client MUST NOT acknowledge Version Negotiation or Retry packets.  These
-packet types contain packet numbers selected by the client, not the server.
+A client MUST NOT acknowledge Version Negotiation or Retry packets.  Version
+Negotiation packets don't contain a packet number and Retry packets include the
+packet number from the Initial packet it responds to.  Rather than relying on
+ACK frames, these packets are implicitly acknowledged by the next Initial packet
+sent by the client.

Not exclusive to this, but is there text to discuss DoS and amplification attacks related to triggering multiple VN and retry packets? One could imagine slowly triggering a lot of retry packets that are never responded to and though the server backs off, it might effectively send a flood to one target.

-- 
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/1043#pullrequestreview-87436853

---

From: Martin Thomson <[email-masked]>
Date: Tue, 09 Jan 2018 00:42:56 -0800

martinthomson commented on this pull request.

It can't be optional if it is to be interoperable.

I'm not sure which point Ian was making.  If you lose connection ID and the address tuple changes, you are lost, but either of those can be enough to recover.  The address tuple is only sufficient if the server isn't routing based on connection ID though, and we expect that to be quite common.

-- 
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/1043#discussion_r160344942

---

From: Martin Thomson <[email-masked]>
Date: Tue, 09 Jan 2018 08:43:57 +0000 (UTC)

We've discussed this previously.  Retry and Version Negotiation are necessarily smaller than the Initial packet that triggers them.  It would therefore be more efficient for the attacker to send its packets directly toward the victim.

-- 
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/1043#discussion_r160345135

---

From: MikkelFJ <[email-masked]>
Date: Tue, 09 Jan 2018 08:51:21 +0000 (UTC)

I think it was in relation to two separate connections during handshake in long form, so it might not actually apply here.

-- 
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/1043#discussion_r160346571

---

From: Marten Seemann <[email-masked]>
Date: Tue, 09 Jan 2018 09:41:30 +0000 (UTC)

marten-seemann requested changes on this pull request.

> @@ -707,8 +710,8 @@ packets MUST use connection ID selected by the client.
 The packet number is an integer in the range 0 to 2^62-1. The value is used in
 determining the cryptographic nonce for packet encryption.  Each endpoint
 maintains a separate packet number for sending and receiving.  The packet number
-for sending MUST increase by at least one after sending any packet, unless
-otherwise specified (see {{initial-packet-number}}).
+for sending starts at zero for the first packet set and MUST increase by one
+after sending a packet.

As discussed in #1030, random packet number gaps still need to be permitted to defend against the optimistic ACK attack.
If we want to change the defense against this attack, I'd prefer to discuss this in a separate PR.

-- 
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/1043#pullrequestreview-87457182

---

From: Mike Bishop <[email-masked]>
Date: Tue, 09 Jan 2018 18:14:27 +0000 (UTC)

MikeBishop commented on this pull request.

Looks reasonable; minor nits.

 ~~~
-    struct {
-        uint16 length = Length;
-        opaque label<6..255> = "QUIC " + Label;
-        uint8 hashLength = 0;
-    } QuicHkdfLabel;
+struct {
+   uint16 length = Length;
+   opaque label<6..255> = "QUIC " + Label;
+   uint8 connectionId<0>;

Is this included just to emphasize that it's zero-length, or does it actually include a byte representing the zero-length array?

> -TLS state machine reports that the ClientHello has been sent, the 0-RTT keys can
-be generated and installed for writing.  When the TLS state machine reports
-completion of the handshake, the 1-RTT keys can be generated and installed for
-writing.
+Note:
+
+: A connection ID might be omitted from the encoding of a packet, as is
+  permitted when the omit_connection_id transport parameter is used, but the
+  connection ID is still used by QHKDF-Expand to derive these values.
+
+The QUIC record protection initially starts with keying material derived from
+handshake keys.  When the TLS state machine reports that the ClientHello has
+been sent, 0-RTT keys can be generated and installed for writing, if 0-RTT is
+available.  Finally, the TLS state machine reports completion of the handshake,
+the 1-RTT keys can be generated and installed for writing.  Additionally, new
+keys are installed each time that the connection ID changes.

Does this need to mention TLS key updates?

> @@ -606,9 +606,12 @@ A Retry packet uses long headers with a type value of 0x7E.  It carries
 cryptographic handshake messages and acknowledgments.  It is used by a server
 that wishes to perform a stateless retry (see {{stateless-retry}}).
 
-The packet number and connection ID fields echo the corresponding fields from
-the triggering client packet.  This allows a client to verify that the server
-received its packet.
+The connection ID field echoes the corresponding fields from the triggering

One field

> @@ -606,9 +606,12 @@ A Retry packet uses long headers with a type value of 0x7E.  It carries
 cryptographic handshake messages and acknowledgments.  It is used by a server
 that wishes to perform a stateless retry (see {{stateless-retry}}).
 
-The packet number and connection ID fields echo the corresponding fields from
-the triggering client packet.  This allows a client to verify that the server
-received its packet.
+The connection ID field echoes the corresponding fields from the triggering
+client packet.  This allows a client to correlate a Retry with the Initial
+packet that it sends that the server received its packet.

This sentence doesn't parse.

> @@ -642,11 +645,11 @@ server and client.
 The connection ID field in a Handshake packet contains a connection ID
 that is chosen by the server (see {{connection-id}}).
 
-The first Handshake packet sent by a server contains a randomized packet number.
-This value is increased for each subsequent packet sent by the server as
-described in {{packet-numbers}}.  The client increments the packet number from
-its previous packet by one for each Handshake packet that it sends (which might
-be an Initial, 0-RTT Protected, or Handshake packet).
+The first Handshake packet sent by a server contains a packet number of 0.  This
+value is increased for each subsequent packet sent by the server as described in
+{{packet-numbers}}.  The client increments the packet number from its previous
+packet by one for each packet that it sends (which might be an
+Initial, 0-RTT Protected, or Handshake packet).

I know this isn't new text, but this statement about the client is redundant -- the client's behavior is already described at line 586.  (In fact, it might be worth consolidating them and simply describing that the first packet sent by each party has a packet number of zero and is incremented with each packet.)

> +packet_number = (masked_packet_number - pn_mask) MOD num_values
+~~~
+
+Packet numbers are not obscured when encoded in frames, such as ACK
+({{frame-ack}}).
+
+These changes are applied before packet protection, so the additional
+authenticated data (AAD) input includes masked values.
+
+These are not true cryptographic confidentiality protections, so entities other
+than endpoints are likely to be able to recover the underlying values by
+observing multiple packets.  For instance, in this version of QUIC, packet
+numbers still increase monotonically as long as the connection ID remains
+constant.  However, these values are different for client and server, and they
+change when a connection ID changes, together ensuring that flows with different
+connection IDs are not linkable based on the value of these fields.

Strong statement; maybe we stick to making it more difficult to link?

-- 
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/1043#pullrequestreview-87606002

---

From: ianswett <[email-masked]>
Date: Tue, 09 Jan 2018 21:02:13 +0000 (UTC)

I need some more time to think about this, but is there a discussion which led to this solution I missed?

Also, this PR is large.  Is it possible to make it more focused?

-- 
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/1043#issuecomment-356412953

---

From: Martin Thomson <[email-masked]>
Date: Tue, 09 Jan 2018 19:09:01 -0800

@martinthomson pushed 3 commits.

f341608  At least one
e451864  connectionIdLength rather than an empty vector, which is unclear from TLS how to encode
75606ba  Mike's editorial comments

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1043/files/795657864b0d171366dc87498298e76b0d18484f..75606ba92263d988b4a938a50934ac1500b84f64

---

From: Martin Thomson <[email-masked]>
Date: Wed, 10 Jan 2018 23:54:48 +0000 (UTC)

~~~
-    struct {

Hmm, I meant to check up on this, and now that I have, I don't actually know how `<0>` would be encoded.

Key updates in TLS are mentioned elsewhere, I think.  We don't need to use them.

It's also wrong :/

Trimmed down.

That was an error.  I wish that we had a better defense against optimistic ACK attacks though.

+~~~
+

I believe that this is true as qualified.  Based on the values of these fields alone there isn't linkability: they are effectively random.  There are ways to mess this up (and multipath will definitely risk doing 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/1043#discussion_r160580026

---

From: Martin Thomson <[email-masked]>
Date: Tue, 16 Jan 2018 15:41:52 -0800

@martinthomson pushed 1 commit.

c13e788  Let's make the connection ID thing more consistent

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1043/files/483a59da0d64e768a9ec1fe9ff823e023a099e9f..c13e7885da3ec5dc001e9b61f487a0f11aac5610