Subject: [quicwg/base-drafts] Add retry integrity tag (#3120)

From: David Schinazi <[email-masked]>
Date: Thu, 17 Oct 2019 16:08:54 -0700

As discussed in Cupertino, this PR adds an integrity tag to retry packets to prevent on-network packet corruption from breaking QUIC handshakes.

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

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

-- Commit Summary --

  * Add retry integrity tag

-- File Changes --

    M draft-ietf-quic-tls.md (58)
    M draft-ietf-quic-transport.md (44)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/3120.patch
https://github.com/quicwg/base-drafts/pull/3120.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/3120

---

From: Marten Seemann <[email-masked]>
Date: Thu, 17 Oct 2019 18:59:29 -0700

marten-seemann commented on this pull request.

If you remove the ODCID Len from the pseudo-packet, this might allow you to simplify the description such that you don't need to define a pseudo-packet in the first place.

> ++-+-+-+-+-+-+-+-+
+|1|1| 3 | Unused|
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                         Version (32)                          |
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+| DCID Len (8)  |
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|               Destination Connection ID (0..160)            ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+| SCID Len (8)  |
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                 Source Connection ID (0..160)               ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                        Retry Token (*)                      ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+| ODCID Len (8) |

Why do we need the length here?

> @@ -4196,10 +4189,7 @@ processing a Retry packet; {{packet-0rtt}} contains more information on this.
 
 A server acknowledges the use of a Retry packet for a connection using the
 original_connection_id transport parameter (see
-{{transport-parameter-definitions}}).  If the server sends a Retry packet, it
-MUST include the value of the Original Destination Connection ID field of the
-Retry packet (that is, the Destination Connection ID field from the client's
-first Initial packet) in the transport parameter.
+{{transport-parameter-definitions}}).

This could be more concrete, e.g. `by setting the original_connection_id transport parameter to the Destination Connection ID from the client's first Initial packet`.

-- 
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/3120#pullrequestreview-303628842

---

From: Kazuho Oku <[email-masked]>
Date: Fri, 18 Oct 2019 01:15:57 -0700

kazuho commented on this pull request.

+|1|1| 3 | Unused|

We need the length, as if it's absent, an attacker on the path from client to server but not on the opposite direction can obtain an Initial carrying DCID=X, race an Initial carrying DCID=`prefix || X`. Assuming that the Retry in response to the spoofed Initial wins the race, the client would learn that the value of the token is `correct_token  || prefix`.

That said, I do not think having ODCID Len at this position prevents the attack, because the Retry Token field preceding this field does not have an explicit length. Rather, I think that we need to either place ODCID Len and ODCID before the Retry packet, or add the length of the Retry packet at the very front.

-- 
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/3120#discussion_r336369075

---

From: MikkelFJ <[email-masked]>
Date: Fri, 18 Oct 2019 02:05:30 -0700

mikkelfj commented on this pull request.

+|1|1| 3 | Unused|

Well, if the token is followed by an integrity tag, the would implicitly define the token length, no?

-- 
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/3120#discussion_r336389853

---

From: Nick Banks <[email-masked]>
Date: Fri, 18 Oct 2019 08:43:29 -0700

nibanks commented on this pull request.

I agree with @marten-seemann here. The server MUST set the transport parameter. You're removing normative language 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/3120#discussion_r336555522

---

From: Kazuho Oku <[email-masked]>
Date: Fri, 18 Oct 2019 09:04:17 -0700

+|1|1| 3 | Unused|

@mikkelfj This is about the format of the pseudo packet that we use to *calculate* the integrity tag.

-- 
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/3120#discussion_r336564687

---

From: David Schinazi <[email-masked]>
Date: Fri, 18 Oct 2019 10:08:33 -0700

@DavidSchinazi pushed 1 commit.

3e9418312ad9c224063fce42da0be441acd9a7ab  fix retry tp

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3120/files/f220d9994308b4c348aa9ec79954c4256797da36..3e9418312ad9c224063fce42da0be441acd9a7ab

---

From: David Schinazi <[email-masked]>
Date: Fri, 18 Oct 2019 10:08:45 -0700

DavidSchinazi commented on this pull request.

Good point, fixed.

-- 
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/3120#discussion_r336589350

---

From: David Schinazi <[email-masked]>
Date: Fri, 18 Oct 2019 10:12:10 -0700

a7fa53b81c09e2031051f3dbe9e120c52f3174eb  fix retry tp

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3120/files/3e9418312ad9c224063fce42da0be441acd9a7ab..a7fa53b81c09e2031051f3dbe9e120c52f3174eb

---

From: David Schinazi <[email-masked]>
Date: Fri, 18 Oct 2019 10:12:47 -0700

+|1|1| 3 | Unused|

Good catch, moved ODCID to the start to remove the ambiguity

-- 
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/3120#discussion_r336590866

---

From: MikkelFJ <[email-masked]>
Date: Fri, 18 Oct 2019 10:37:51 -0700

+|1|1| 3 | Unused|

@kazuho I would assume it had a pseudo packet length use to compute the tag, otherwise it would be a futile exercise. I'm probably still missing something. A weaker integrity check would need some padding protection but GCM should be fine.

-- 
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/3120#discussion_r336601002

---

From: MikkelFJ <[email-masked]>
Date: Fri, 18 Oct 2019 10:39:31 -0700

+|1|1| 3 | Unused|

yeah, I'm missing that two fields without a length is ambigious.

-- 
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/3120#discussion_r336601769

---

From: Christopher Wood <[email-masked]>
Date: Fri, 18 Oct 2019 11:34:16 -0700

chris-wood commented on this pull request.

> @@ -1197,6 +1197,64 @@ TLS ClientHello.  The server MAY retain these packets for later decryption in
 anticipation of receiving a ClientHello.

+## Retry Packet Integrity {#retry-integrity}
+
+Retry packets (see the Retry Packet section of {{QUIC-TRANSPORT}}) carry a
+Retry Integrity Tag that provides two properties: it allows discarding
+packets that have accidentally been corrupted by the network, and it ensures
+that valid Retry packets cannot be sent by off-path attackers.

nit: perhaps soften this to, "... , and it helps mitigate off-path attacker Retry packet injection"? (The word "ensures" is a little misleading to me.)

-- 
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/3120#pullrequestreview-304067675

---

From: David Schinazi <[email-masked]>
Date: Fri, 18 Oct 2019 15:06:38 -0700

4aae5f258dfc27ca950b64481a433da39c3c8893  cawood's review

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3120/files/a7fa53b81c09e2031051f3dbe9e120c52f3174eb..4aae5f258dfc27ca950b64481a433da39c3c8893

---

From: David Schinazi <[email-masked]>
Date: Fri, 18 Oct 2019 15:06:47 -0700

Good point, thanks! Fixed.

-- 
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/3120#discussion_r336692166

---

From: ianswett <[email-masked]>
Date: Fri, 18 Oct 2019 20:36:58 -0700

ianswett commented on this pull request.

I'm not in love with the pseudo-packet concept, FWIW.

+## Retry Packet Integrity {#retry-integrity}
+
+Retry packets (see the Retry Packet section of {{QUIC-TRANSPORT}}) carry a
+Retry Integrity Tag that provides two properties: it allows discarding
+packets that have accidentally been corrupted by the network, and it mitigates
+off-path attackers' ability to send valid Retry packets.

I'm unsure how this achieves the latter goal?

++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+| DCID Len (8)  |
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|               Destination Connection ID (0..160)            ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+| SCID Len (8)  |
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                 Source Connection ID (0..160)               ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                        Retry Token (*)                      ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+~~~
+{: #retry-pseudo title="Retry Pseudo-Packet"}
+
+The Retry Pseudo-Packet is not sent over the wire. It is computed by taking

I'm confused by the term Psuedo-Packet.  If you're intending to remove these fields in a future PR, I'd prefer to keep them in for now to make this simpler.

-- 
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/3120#pullrequestreview-304193586

---

From: David Schinazi <[email-masked]>
Date: Fri, 18 Oct 2019 22:49:38 -0700

We prevent off-path attackers from injecting Retry packets by including the Original Destination Connection ID in the associated data here. If they didn't see the client's Initial their Retry will not validate.

-- 
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/3120#discussion_r336723606

---

From: David Schinazi <[email-masked]>
Date: Fri, 18 Oct 2019 22:51:33 -0700

This concept comes from how TCP computes its checksum to include fields in the checksum that aren't transmitted in the TCP header: https://tools.ietf.org/html/rfc793#section-3.1

-- 
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/3120#discussion_r336723652

---

From: ianswett <[email-masked]>
Date: Sun, 20 Oct 2019 05:49:02 -0700

ianswett approved this pull request.

If possible, I think it'd be nice to add this to the transport draft, since if we ever changed crypto handshakes, I expect we'd still want this.  If that conversation is a wider discussion, I'm happy to have it on a separate PR.

I somehow missed the removal of ODCID and ODCID len upon first review, so this makes sense now.

Q: Is there a reason this needs to be in TLS?  It seems like this doesn't interact with TLS at all.

Thanks, how about being being specific and saying "to send valid retry packets without having seen the original destination connection ID."?

That was also true before this change, so I was thinking there was some new protection I was unaware of.

-- 
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/3120#pullrequestreview-304257230

---

From: David Schinazi <[email-masked]>
Date: Sun, 20 Oct 2019 14:46:03 -0700

This is in TLS because the transport doc currently has no dependency on AES. I'll let the editors decide where this should live.

-- 
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/3120#discussion_r336799382

---

From: Martin Thomson <[email-masked]>
Date: Mon, 21 Oct 2019 18:40:17 -0700

martinthomson approved this pull request.

Some editorial suggestions and one question about a length field.

I'm ambivalent with the implicit ODCID thing.  I like implicit things generally because you can't succeed if you don't have the right information, but this seems like it might still fail if the client doesn't check the tag.  We could completely avoid that being an issue if we enciphered the token, but that might change the optimization options enough that it isn't worthwhile.

+## Retry Packet Integrity {#retry-integrity}
+
+Retry packets (see the Retry Packet section of {{QUIC-TRANSPORT}}) carry a
+Retry Integrity Tag that provides two properties: it allows discarding
+packets that have accidentally been corrupted by the network, and it mitigates
+off-path attackers' ability to send valid Retry packets.
+
+The Retry Integrity Tag is a 128-bit field that is computed as the output of
+AEAD_AES_128_GCM used with the following inputs:

```suggestion
AEAD_AES_128_GCM {{!AEAD=RFC5116}} used with the following inputs:
```

> @@ -2736,7 +2736,7 @@ available.
 
 ## Protected Packets {#packet-protected}
 
-All QUIC packets except Version Negotiation and Retry packets use authenticated
+All QUIC packets except Version Negotiation packets use authenticated
 encryption with additional data (AEAD) {{!RFC5116}} to provide confidentiality

The confidentiality bit means that this statement still applies to Retry.

> @@ -4111,37 +4111,31 @@ wishes to perform a retry (see {{validate-handshake}}).
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 |                 Source Connection ID (0..160)               ...
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-| ODCID Len (8) |
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|          Original Destination Connection ID (0..160)        ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 |                        Retry Token (*)                      ...

In the interests of cleaner parsing, should we add a length for the token?  That would increase the similarity of this header with the Initial header.

+## Retry Packet Integrity {#retry-integrity}
+
+Retry packets (see the Retry Packet section of {{QUIC-TRANSPORT}}) carry a
+Retry Integrity Tag that provides two properties: it allows discarding
+packets that have accidentally been corrupted by the network, and it mitigates
+off-path attackers' ability to send valid Retry packets.
+
+The Retry Integrity Tag is a 128-bit field that is computed as the output of
+AEAD_AES_128_GCM used with the following inputs:
+
+- The key is 128 bits all set to zero.

```suggestion
- The secret key, K, is 128 bits all set to zero.
```

+## Retry Packet Integrity {#retry-integrity}
+
+Retry packets (see the Retry Packet section of {{QUIC-TRANSPORT}}) carry a
+Retry Integrity Tag that provides two properties: it allows discarding
+packets that have accidentally been corrupted by the network, and it mitigates
+off-path attackers' ability to send valid Retry packets.
+
+The Retry Integrity Tag is a 128-bit field that is computed as the output of
+AEAD_AES_128_GCM used with the following inputs:
+
+- The key is 128 bits all set to zero.
+- The nonce is 96 bits all set to zero.

```suggestion
- The nonce, N, is 96 bits all set to zero.
```

and continue.

>  |                        Retry Token (*)                      ...
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                                                               |
++                                                               +
+|                                                               |
++                   Retry Integrity Tag (128)                   +

I wonder if we shouldn't just label this AEAD Ciphertext rather than just a tag.

> @@ -4163,10 +4157,9 @@ A client MUST accept and process at most one Retry packet for each connection
 attempt.  After the client has received and processed an Initial or Retry packet
 from the server, it MUST discard any subsequent Retry packets that it receives.
 
-Clients MUST discard Retry packets that contain an Original Destination
-Connection ID field that does not match the Destination Connection ID from its
-Initial packet.  This prevents an off-path attacker from injecting a Retry
-packet.
+Clients MUST discard Retry packets whose Retry Integrity Tag cannot be
+validated, see the Retry Packet Integrity section of {{QUIC-TLS}}. This
+mitigates an off-path attacker's ability to inject a Retry packet.

I think that we want to add: "and protects against accidental corruption of Retry packets."

-- 
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/3120#pullrequestreview-304931396

---

From: David Schinazi <[email-masked]>
Date: Mon, 21 Oct 2019 18:50:20 -0700

I'm not sure whether Retry packets have less confidentiality protection than Initial packets?

-- 
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/3120#discussion_r337307740

---

From: David Schinazi <[email-masked]>
Date: Mon, 21 Oct 2019 18:51:53 -0700

eeb4ce37462c90023a0edfc45df9442e37f49732  MT editorial changes

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3120/files/4aae5f258dfc27ca950b64481a433da39c3c8893..eeb4ce37462c90023a0edfc45df9442e37f49732

---

From: David Schinazi <[email-masked]>
Date: Mon, 21 Oct 2019 18:54:20 -0700

Done

-- 
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/3120#discussion_r337308215

---

From: David Schinazi <[email-masked]>
Date: Mon, 21 Oct 2019 18:55:24 -0700

I initially had checksum, which is somewhat closer to our intent by not fully correct. I'd personally rather have a name that conveys intent rather than construction, but I'll let the editors decide.

-- 
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/3120#discussion_r337308349

---

From: David Schinazi <[email-masked]>
Date: Mon, 21 Oct 2019 18:56:35 -0700

We could, but then we'd have to handle the case where the length is wrong. I don't feel too strongly either way

-- 
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/3120#discussion_r337308502

---

From: David Schinazi <[email-masked]>
Date: Mon, 21 Oct 2019 19:00:11 -0700

@martinthomson 
> I'm ambivalent with the implicit ODCID thing. I like implicit things generally because you can't succeed if you don't have the right information, but this seems like it might still fail if the clie…

The ODCID being implicit has the same checking properties as an explicit ODCID - the client could ignore that field entirely before.

That said I do like the idea of making it harder to not check, by encrypting the token. @nibanks if we were to encrypt the token with a zero-key, would that still be suitable? I think the overhead is very small as long as we don't use HKDF / SHA-2 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/3120#issuecomment-544777830

---

From: David Schinazi <[email-masked]>
Date: Mon, 21 Oct 2019 19:00:34 -0700

467e31ed2e742ae6d3aff6e472fb7987d284929f  Review suggestion from MT

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3120/files/eeb4ce37462c90023a0edfc45df9442e37f49732..467e31ed2e742ae6d3aff6e472fb7987d284929f

---

From: Mike Bishop <[email-masked]>
Date: Wed, 23 Oct 2019 13:23:21 -0700

MikeBishop approved this pull request.

+## Retry Packet Integrity {#retry-integrity}
+
+Retry packets (see the Retry Packet section of {{QUIC-TRANSPORT}}) carry a
+Retry Integrity Tag that provides two properties: it allows discarding
+packets that have accidentally been corrupted by the network, and it mitigates
+off-path attackers' ability to send valid Retry packets.
+
+The Retry Integrity Tag is a 128-bit field that is computed as the output of
+AEAD_AES_128_GCM {{!AEAD=RFC5116}} used with the following inputs:
+
+- The secret key, K, is 128 bits all set to zero.
+- The nonce, N, is 96 bits all set to zero.
+- The plaintext is empty.
+- The associated data is the contents of the Retry Pseudo-Packet, as described

I'd use a different verb than "described" when referencing the figure.  We've tried to follow the rule that illustrations aren't normative, they're just helpful illustrations of what the text says.

@martinthomson is the editor of both documents, so I'd say he gets to hold the baton here.

 

They're not encrypted, so kind of by definition....

We have a convention of having the final variable-length field have an implicit length.  It reduces the possibility of "impossible" packets.  I'm inclined to keep the PR as-is.

> @@ -4163,10 +4157,10 @@ A client MUST accept and process at most one Retry packet for each connection
 
+mitigates an off-path attacker's ability to inject a Retry packet, and protects

```suggestion
mitigates an off-path attacker's ability to inject a Retry packet and protects
```

-- 
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/3120#pullrequestreview-306162699

---

From: David Schinazi <[email-masked]>
Date: Wed, 23 Oct 2019 13:32:57 -0700

Fine by me. Can you suggest another verb?

-- 
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/3120#discussion_r338263786

---

From: David Schinazi <[email-masked]>
Date: Wed, 23 Oct 2019 13:34:34 -0700

8c45cc9e37ff1689497e844e3bf6b867138f1f95  Editorial fix from MikeBishop

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3120/files/467e31ed2e742ae6d3aff6e472fb7987d284929f..8c45cc9e37ff1689497e844e3bf6b867138f1f95

---

From: Martin Thomson <[email-masked]>
Date: Wed, 23 Oct 2019 16:02:57 -0700

martinthomson commented on this pull request.

```suggestion
- The associated data, A, is the contents of the Retry Pseudo-Packet, as illustrated
```

```suggestion
- The plaintext, P, is empty.
```

This is fine.  I mean, the split between the documents boils down to "all the crypto bits" and "everything else".  We have discussed moving packet protection to transport in the past, but that hasn't happened, and now probably won't.  As this is packet protection (ish), this is the right place for now.

 
-All QUIC packets except Version Negotiation and Retry packets use authenticated
+All QUIC packets except Version Negotiation packets use authenticated
 encryption with additional data (AEAD) {{!RFC5116}} to provide confidentiality
 and integrity protection. Details of packet protection are found in

```suggestion
 and integrity protection. Retry packets use an AEAD to provide integrity 
  protection.  Details of packet protection are found in
```

Note that the left indent here is weirdly broken.  Likely the result of a bug in GitHub suggestions.

We already have impossible packets.  The DCID Len field can indicate a length longer than the packet.

Unnecessary anthropomorphism (or at least implied possessive).
```suggestion
Clients MUST discard Retry packets that have a Retry Integrity Tag that cannot be
```

-- 
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/3120#pullrequestreview-306237557

---

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

c056d3063fe3ddf5ba3c31778ec4a721c15a49f3  MT suggestions

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

---

From: David Schinazi <[email-masked]>
Date: Thu, 24 Oct 2019 19:36:01 -0700

739d272564a791476b75cff0a6ea258f0e4d17a8  fixup line length

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3120/files/c056d3063fe3ddf5ba3c31778ec4a721c15a49f3..739d272564a791476b75cff0a6ea258f0e4d17a8

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 30 Oct 2019 01:14:27 -0700

kazuho approved this pull request.

Thank you for writing this up. Looks good to me.

I do not have a strong opinion regarding if we should encipher the token (using a fixed key) to enforce the client to consult the integrity tag. It's good in terms of enforcing verification. It does not change the complexity of the design. There would be a performance penalty when using a software cipher (i.e. when you do *not* have AESNI).

-- 
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/3120#pullrequestreview-309019991

---

From: MikkelFJ <[email-masked]>
Date: Wed, 30 Oct 2019 05:45:32 -0700

> There would be a performance penalty when using a software cipher (i.e. when you do not have AESNI).

Yes, this is also a concern I have but it might not be worth the complexity.

A use case goes like:

Embedded microcontroller links to hub via bluetooh and does not have an IP network of its own, but it does have a complete QUIC endpoint that the hub should not interfere with other than deliver to the proper IP remote endpoint and attach a local IP.

The device is constrained and only supports ChaCha/Poly crypto. The exception is then the Retry packet (and possibly the Initial packet, I don't recall). This requires additional software which may be a problem in limited flash storage even if the initial slowdown is otherwise acceptable.

-- 
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/3120#issuecomment-547883959

---

From: David Schinazi <[email-masked]>
Date: Wed, 30 Oct 2019 14:32:28 -0700

@kazuho encrypting using AES-GCM with an all-zero key and all-zero nonce is just a XOR operation between a known string and the cleartext so the performance cost is very low.

@mikkelfj QUICv1 uses AES-GCM for Initial packets so all QUIC clients need AES-GCM support. You're free to build a different version of QUIC that only uses ChaChaPoly, but that won't impact the design of QUICv1.

-- 
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/3120#issuecomment-548121709

---

From: MikkelFJ <[email-masked]>
Date: Wed, 30 Oct 2019 14:55:58 -0700

@DavidSchinazi on AES-GCM that is not correct: The crypto part yes, but you do not encrypt you only GHASH because it is AEAD only. GHASH is a 128-bit carryless multiplication over a Galois Field which completes the checksum. If you CLMUL instructions this is efficient, otherwise not so much. That is where Poly1305 is a gain. CLMUL is not very common on misc. controllers even if they have other accelerations, at least for the time being. E.g. ESP8266.

I agree that AES-GCM is the right choice given the circumstances but I think Poly1305 would be better for initial and retry because it would work in a broader range. The benefit of AES-GCM is that highly loaded servers will have the necessary accelleration and may slow down on AES-GCM - even if this is a tiny fraction of the overall connection cost, it may be important as it fends of noise from the broader internet.

-- 
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/3120#issuecomment-548129595

---

From: David Schinazi <[email-masked]>
Date: Wed, 30 Oct 2019 15:45:55 -0700

I'm going to have this PR match the algorithm used for Initial, and stick with AES-GCM. Feel free to file a separate issue to discuss using ChaChaPoly for both.

-- 
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/3120#issuecomment-548144308

---

From: MikkelFJ <[email-masked]>
Date: Wed, 30 Oct 2019 15:47:59 -0700

I think it is way too late to make such a change, which is why I wrote that given the circumstances I agree with the design choice.

-- 
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/3120#issuecomment-548144894

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 30 Oct 2019 16:33:02 -0700

@DavidSchinazi 
> encrypting using AES-GCM with an all-zero key and all-zero nonce is just a XOR operation between a known string and the cleartext so the performance cost is very low.

Oh I see what you mean. That's a keen observation. Though, I think that observation also shows the reason some of us thought use of encryption to be beneficial to be pointless.

IIUC, https://github.com/quicwg/base-drafts/pull/3120#pullrequestreview-304931396 suggested using encryption, because it would prevent implementations from using the token without checking the tag. The assumption behind is that AEAD-decryption is going to be an atomic operation.

But as you point out, in this particular case, there is a interesting optimization where endpoints can have a pre-built XOR vector, and just do GCM.

Doesn't that reduce the motivation to use encryption, as it is a complexity to apply XOR?

-- 
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/3120#issuecomment-548155607

---

From: ianswett <[email-masked]>
Date: Wed, 30 Oct 2019 16:35:40 -0700

One reason to use encryption was some implementations(@larseggert ?) already had an AES-GCM implementation, and didn't want to add another hash or crypto function.

-- 
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/3120#issuecomment-548156190

---

From: Martin Thomson <[email-masked]>
Date: Wed, 30 Oct 2019 17:30:21 -0700

So though there is a simple and easy optimization, many implementations will have AES-GCM available, but won't have an independent GHASH on tap.  And getting the mask requires effort.  And client's can't predict how many bytes of mask they need, so they would have to get quite a few.  I'm speculating here, but I tend to think that this puts a thumb on the scale in the right way.

-- 
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/3120#issuecomment-548167917

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 30 Oct 2019 19:00:51 -0700

@martinthomson Yeah, my train of thought as a server stack developer that happens to also work as a client was that I might be interested in having a pre-built XOR mask on the server-side. Then, I might expand that constant vector to the 1,200 bytes, and use it for decoding the Retry packets too.

That said, I do agree that not all stack developers would (think and) actually implement that way. So I agree that there's some value in encrypting the token rather than just applying GMAC, at the cost of as small as requiring XOR on the server-side.

-- 
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/3120#issuecomment-548185054

---

From: MikkelFJ <[email-masked]>
Date: Thu, 31 Oct 2019 08:52:34 +0000 (UTC)

XOR'ing with zero keys provides no confidentiality and just adds overhead. It is fine to use AES-GCM to tap into the GHASH logic by adding only authenticated additional data. But requiring a large static buffer of zero encrypted data is just overhead. For servers it harms expensive L1 cache and for embedded devices it would require encryption as usual.

As I said before GHASH is not the ideal hash for broader use, but given what we have it makes sense for authenticated additional data.

-- 
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/3120#issuecomment-548270968