Subject: Re: [quicwg/base-drafts] Packet number encryption (#1079)

From: janaiyengar <[email-masked]>
Date: Wed, 02 May 2018 18:39:24 -0700

janaiyengar commented on this pull request.

This is really close. A few comments.

> @@ -908,6 +918,80 @@ the connection that is hosted on stream 0.  This sequence number is not visible
 to QUIC.

+## Packet Number Protection {#pn-encrypt}
+
+QUIC packets are protected using a key that is derived from the current set of
+secrets.  The key derived using the "pn" label is used to protect the packet
+number from casual observation.  The packet number protection algorithm depends
+on the negotiated AEAD.
+
+Packet number protection is applied after packet protection is applied (see
+{{aead}}).  The ciphertext of the packet is sampled and used as input to an
+encryption algorithm.
+
+For packets with a long header, the ciphertext starting
+immediately after the packet number is used (that is, octet 17 onwards).

+1. This needs to be fixed.

> @@ -859,6 +865,11 @@ used for QUIC packet protection is AEAD that is negotiated for use with the TLS
 connection.  For example, if TLS is using the TLS_AES_128_GCM_SHA256, the
 AEAD_AES_128_GCM function is used.
 
+QUIC packets are protected prior to applying packet number encryption
+({{pn-encrypt}}).  Thus, the unprotected packet number is part of the AAD.  When

s/AAD/AEAD/

> +assumed to be its largest possible length (4 octets).  Thus, for a short header,
+the sampled ciphertext starts at either octet 5 when the connection ID is
+omitted, or octet 13 when the connection is present.
+
+The protected packet might not have produced enough input for the negotiated
+packet protection algorithm.  This might happen if a packet with a short header
+contains minimal data and uses a packet number encoding that is shorter than 4
+octets.  Additional zero octets are added to the end of the sequence to reach
+the required amount of data.
+
+Before a TLS ciphersuite can be used with QUIC, a packet protection algorithm
+MUST be specifed for the AEAD used with that ciphersuite.  This document defines
+algorithms for AEAD_AES_128_GCM, AEAD_AES_128_CCM, AEAD_AES_256_GCM,
+AEAD_AES_256_CCM (all AES AEADs are defined in {{!RFC5116}}), and
+AEAD_CHACHA20_POLY1305 ({{!CHACHA=RFC7539}}).
+

Is this resolved now? Does the text below address @huitema and @kazuho 's concerns?

> @@ -345,7 +345,8 @@ Payload Length:
 Packet Number:
 
 : The Packet Number is a 32-bit field that follows the two connection IDs.
-  {{packet-numbers}} describes the use of packet numbers.
+  {{packet-numbers}} describes the use of packet numbers.  Packet numbers are
+  protected separately to the packet payload.

s/to/from/. Or is this because of Englishes?

> @@ -452,7 +453,8 @@ Destination Connection ID:
 Packet Number:
 
 : The length of the packet number field depends on the packet type.  This field
-  can be 1, 2 or 4 octets long depending on the short packet type.
+  can be 1, 2 or 4 octets long depending on the short packet type.  Packet
+  numbers are protected separate to the packet payload.

ditto. Also, "separately".

> @@ -612,8 +614,11 @@ The server includes a connection ID of its choice in the Source Connection ID
 field.  The client MUST use this connection ID in the Destination Connection ID
 of subsequent packets that it sends.
 
-The packet number field echoes the packet number field from the triggering
-client packet.
+The Packet Number field of a Retry packet MUST be set to 0.  This value is
+subsequently protected as normal. \[\[Editor's Note: This isn't ideal, because
+it creates a "cheat" where the client assumes a value.  That's a problem, so I'm
+tempted to suggest that this include any value less than 2^30 so that normal
+processing works - and can be properly exercised.]]

This also makes a client that uses a 0-length CID vulnerable to injection of an arbitrary Retry packet. 

> @@ -693,8 +696,10 @@ continue to use the same Destination Connection ID for 0-RTT packets, see
 
 The version field for protected packets is the current QUIC version.
 
-The packet number field contains a packet number, which increases with each
-packet sent, see {{packet-numbers}} for details.
+The packet number field contains a packet number, which is protected separately
+from the rest of the packet (see {{QUIC-TLS}} for details).  The underlying

You're using "separately from" here. I would make it the same in other places.

-- 
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/1079#pullrequestreview-117117640

---

From: Martin Thomson <[email-masked]>
Date: Wed, 02 May 2018 19:33:52 -0700

martinthomson commented on this pull request.

The 0-length connection ID is only possible after the client has received a Retry, after which it will discard any Retry packets that it receives.  So I don't think so.

-- 
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/1079#discussion_r185687219

---

From: Martin Thomson <[email-masked]>
Date: Wed, 02 May 2018 19:36:14 -0700

Reworded all of these to emphasize *additional* confidentiality protection, rather than *separateness*, which is a tiny bit misleading.  That also neatly avoids the to/from/than problems I seem to have.

-- 
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/1079#discussion_r185687404

---

From: Martin Thomson <[email-masked]>
Date: Wed, 02 May 2018 19:36:59 -0700

AAD is correct, but it's not clear and it's the first use of AAD, so I have reworded to use consistent terminology.

-- 
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/1079#discussion_r185687488

---

From: Martin Thomson <[email-masked]>
Date: Wed, 02 May 2018 19:38:52 -0700

I believe so.

-- 
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/1079#discussion_r185687626

---

From: Martin Thomson <[email-masked]>
Date: Thu, 03 May 2018 02:39:37 +0000 (UTC)

@martinthomson pushed 1 commit.

8e7540f  Addressing nits from final review

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1079/files/a1c4d41f7110eedb5536bab02b1d927cc01b68c8..8e7540f9ace4075ad0a3886944779b6ad8039958

---

From: Nick Banks <[email-masked]>
Date: Wed, 02 May 2018 19:51:12 -0700

nibanks commented on this pull request.

+
+QUIC packets are protected using a key that is derived from the current set of
+secrets.  The key derived using the "pn" label is used to protect the packet
+number from casual observation.  The packet number protection algorithm depends
+on the negotiated AEAD.
+
+Packet number protection is applied after packet protection is applied (see
+{{aead}}).  The ciphertext of the packet is sampled and used as input to an
+encryption algorithm.
+
+For packets with a long header, the ciphertext starting immediately after the
+packet number is used.
+
+For packets with a short header, the packet number length is not known before
+decryption, so it is assumed to be the smaller of the maximum possible packet

I'm a bit confused. Why is the packet number length not known before hand? Doesn't the short packet header still indicate the length?

-- 
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/1079#pullrequestreview-117127081

---

From: Martin Thomson <[email-masked]>
Date: Wed, 02 May 2018 19:57:49 -0700

+

This is in preparation for going to varint packet numbers.  It would need a complete rewrite if this assumption were lifted.

-- 
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/1079#discussion_r185689008

---

From: Nick Banks <[email-masked]>
Date: Wed, 02 May 2018 20:05:38 -0700

+

But this PR doesn't change the short header to use varint packet numbers... unless I missed something. Assuming I didn't, it looks like one of three things need to happen then:

1. An additional PR that changes short headers to varint packet numbers needs to be merged first, and then this rebased on that.
2. This PR should also change the short headers to varint packet numbers.
3. This PR should be written to the current format of short header packet numbers.

If we are going to do it, I don't see why we don't just do (2) right now. Do you expect that to be a huge change?

-- 
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/1079#discussion_r185689487

---

From: janaiyengar <[email-masked]>
Date: Wed, 02 May 2018 20:33:44 -0700

Right, of course.

-- 
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/1079#discussion_r185691320

---

From: janaiyengar <[email-masked]>
Date: Wed, 02 May 2018 20:39:56 -0700

+

Given where this PR is right now, given that the chairs have green-lighted this, and given that we're unicorning, I recommend landing this PR as is. Nick, can you open an issue to track this editorial issue, so that it doesn't get dropped?

-- 
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/1079#discussion_r185691737

---

From: janaiyengar <[email-masked]>
Date: Wed, 02 May 2018 20:40:11 -0700

janaiyengar 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/1079#pullrequestreview-117130930

---

From: Nick Banks <[email-masked]>
Date: Wed, 02 May 2018 20:49:16 -0700

+

I can open an issue, no problem. But we have waited and discussed all this time to ensure we get a good solution. Can't we take a bit longer just to iron out the details and get a complete solution merged all at once? It should be trivial enough to change the short header packet number encoding to varints.

-- 
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/1079#discussion_r185692450

---

From: Martin Thomson <[email-masked]>
Date: Wed, 02 May 2018 21:24:40 -0700

947db7f  Remove confusion without changing the design

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1079/files/8e7540f9ace4075ad0a3886944779b6ad8039958..947db7f42e392d3db77d6ee68ef7f4c0d74a6247

---

From: Martin Thomson <[email-masked]>
Date: Wed, 02 May 2018 21:25:01 -0700

+

I have fixed the incorrect statement, but left the design unchanged.

-- 
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/1079#discussion_r185694920

---

From: Martin Thomson <[email-masked]>
Date: Wed, 02 May 2018 21:25:58 -0700

Merged #1079.

-- 
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/1079#event-1606880808