Subject: [quicwg/base-drafts] Pad rather than shift the header protection offset (#2030)

From: Martin Thomson <[email-masked]>
Date: Tue, 20 Nov 2018 20:34:37 -0800

This simplifies a little, so it's a net win in my view.

Note that a naive implementation does very little to comply with this.  Always sending a 3 or 4 octet packet number is the easy way to comply.  Always padding to at least 3 octets is the other way, which in most cases will be unnecessary.  Dynamically padding is possible too, but we're talking about 2 octets.

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

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

-- Commit Summary --

  * Pad rather than shift the header protection offset

-- File Changes --

    M draft-ietf-quic-tls.md (28)

-- Patch Links --

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

---

From: Marten Seemann <[email-masked]>
Date: Tue, 20 Nov 2018 20:48:21 -0800

marten-seemann requested changes on this pull request.

> -is assumed to be 4 bytes long (its maximum possible encoded length), unless
-there is insufficient space in the packet for a complete sample.  The starting
-offset for the sample is set to 4 bytes after the start of the Packet Number
-field, then is reduced until there are enough bytes to sample.
+is assumed to be 4 bytes long (its maximum possible encoded length).
+
+An endpoint MUST discard packets that are not long enough to contain a complete
+sample.
+
+To ensure that sufficient data is available for sampling, packets are padded so
+that the combined lengths of the encoded packet number and protected payload is
+at least 4 bytes longer than the sample required for header protection.  For the
+AEAD functions defined in {{?TLS13}}, which have 16 byte expansion and 16 byte
+header protection samples, this results in needing at least 3 bytes of frames in
+the unprotected payload if the packet number is encoded on a single byte, or 2
+bytes of frames for a 2 byte packet number encoding.

This has implications on the minimum size of the stateless reset, we will need to increase the 166 random bits by 24.

-- 
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/2030#pullrequestreview-177075716

---

From: Martin Thomson <[email-masked]>
Date: Tue, 20 Nov 2018 22:23:45 -0800

martinthomson commented on this pull request.

Good catch.

-- 
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/2030#discussion_r235265216

---

From: Martin Thomson <[email-masked]>
Date: Tue, 20 Nov 2018 22:29:16 -0800

@martinthomson pushed 1 commit.

03d7c78  Fix stateless reset to match

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2030/files/3d2a951a08f9c01ccfd04e5971579738b4b534c8..03d7c78ae5a0e99410d7ba202afeadfc5a30bfdf

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 20 Nov 2018 23:02:47 -0800

kazuho requested changes on this pull request.

> @@ -2263,7 +2263,7 @@ following layout:
  0                   1                   2                   3
  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|0|1|                   Random Bytes (166..)                  ...
+|0|1|                   Random Bits (190..)                  ...
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Shouldn't this be 182?

The minimum sum of length(pn) + length(payload) is 2 in the current draft. That changes to 4 in the PR. Therefore the delta is 2*8, not 3*8.

> @@ -2285,18 +2285,18 @@ number of random bytes following it are set to unpredictable values.  The last
 16 bytes of the datagram contain a Stateless Reset Token.
 
 A stateless reset will be interpreted by a recipient as a packet with a short
-header.  For the packet to appear as valid, the Random Bytes field needs to
-include at least 20 bytes of random or unpredictable values.  This is intended
-to allow for a destination connection ID of the maximum length permitted, a
-packet number, and minimal payload.  The Stateless Reset Token corresponds to
-the minimum expansion of the packet protection AEAD.  More random bytes might be
-necessary if the endpoint could have negotiated a packet protection scheme with
-a larger minimum AEAD expansion.
+header.  For the packet to appear as valid, the Random Bits field needs to
+include at least 190 bits of random or unpredictable values (or 24 bytes, less

Same as above.

-- 
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/2030#pullrequestreview-177097726

---

From: MikkelFJ <[email-masked]>
Date: Tue, 20 Nov 2018 23:47:43 -0800

I’d much rather have fixed 4 byte packet numbers then. That would simplify a lot.

-- 
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/2030#issuecomment-440568353

---

From: Marten Seemann <[email-masked]>
Date: Tue, 20 Nov 2018 23:53:36 -0800

At least in my implementation, the simplification would be rather small. And the price for the overhead would be paid for every single 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/2030#issuecomment-440569632

---

From: MikkelFJ <[email-masked]>
Date: Wed, 21 Nov 2018 02:17:18 -0800

Fair point @marten-seemann but then I'd rather have the current solution because the sample location can the be determined with a single branchless min operation that cost 3 machine instructions rather than stuffing padding bytes on the wire in special cases.

-- 
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/2030#issuecomment-440610141

---

From: MikkelFJ <[email-masked]>
Date: Wed, 21 Nov 2018 10:24:58 +0000 (UTC)

```
static inline size_t intutil_min_size(size_t x, size_t y)
{
    return y ^ ((x ^ y) & -(x < y));
}
...
const size_t tag_size = 16;
size_t sample_pos = intutil_min_size(pn_pos + 4, end_pos - tag_size)
```

-- 
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/2030#issuecomment-440612817

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 21 Nov 2018 02:40:11 -0800

@mikkelfj 
> the current solution because the sample location can the be determined with a single branchless min operation that cost a few machine instructions rather than stuffing padding bytes on the wire in special cases.

But the downside of the current approach is that the adjustment needs to be calculated on both the sender and the receiver. The approach in the PR requires having just one branch (that almost always turns to one side) on the sender. Therefore, I'd assume that the it not only simplifies the code but would be superior in performance as well.

-- 
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/2030#issuecomment-440617573

---

From: MikkelFJ <[email-masked]>
Date: Wed, 21 Nov 2018 02:47:44 -0800

@kazuho I'm not sure I fully agree, but when you consider that you probably have padding anyway to ensure packet lengths do not reveal too much, it starts to make more sense 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/2030#issuecomment-440619858

---

From: Mike Bishop <[email-masked]>
Date: Wed, 21 Nov 2018 10:53:00 -0800

And padding or not, it should be exceptionally rare to have sub-three-byte payloads.  A packet that includes no frames is invalid; an ACK can't be smaller than five bytes at the minimum and is likely a good bit larger.

So what is that one- or two-byte packet you're sending?  Probably a keep-alive with a PING frame by itself, and you could just modify the keep-alive code to generate three PING frames instead of one.  At that point, the check for undersized packets will be nearly 100% on the not-undersized branch.

-- 
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/2030#issuecomment-440773510

---

From: David Schinazi <[email-masked]>
Date: Wed, 21 Nov 2018 12:02:17 -0800

I support this 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/2030#issuecomment-440792569

---

From: janaiyengar <[email-masked]>
Date: Wed, 21 Nov 2018 16:23:29 -0800

janaiyengar commented on this pull request.

> @@ -905,27 +905,31 @@ of the ciphertext from the packet Payload field.
 The same number of bytes are always sampled, but an allowance needs to be made
 for the endpoint removing protection, which will not know the length of the
 Packet Number field.  In sampling the packet ciphertext, the Packet Number field
-is assumed to be 4 bytes long (its maximum possible encoded length), unless
-there is insufficient space in the packet for a complete sample.  The starting
-offset for the sample is set to 4 bytes after the start of the Packet Number
-field, then is reduced until there are enough bytes to sample.
+is assumed to be 4 bytes long (its maximum possible encoded length).
+
+An endpoint MUST discard packets that are not long enough to contain a complete
+sample.
+
+To ensure that sufficient data is available for sampling, packets are padded so
+that the combined lengths of the encoded packet number and protected payload is
+at least 4 bytes longer than the sample required for header protection.  For the
+AEAD functions defined in {{?TLS13}}, which have 16 byte expansion and 16 byte

```suggestion
AEAD functions defined in {{?TLS13}}, which have 16-byte expansions and 16-byte
```

Compound adjective.
```suggestion
bytes of frames for a 2-byte packet number encoding.
```

 An endpoint SHOULD NOT send a stateless reset that is significantly larger than
 the packet it receives.  Endpoints MUST discard packets that are too small to be
 valid QUIC packets.  With the set of AEAD functions defined in {{QUIC-TLS}},
-packets less than 19 bytes long are never valid.
+packets less than 21 bytes long are never valid.

```suggestion
packets that are smaller than 21 bytes are never valid.
```

-- 
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/2030#pullrequestreview-177469014

---

From: Martin Thomson <[email-masked]>
Date: Wed, 21 Nov 2018 16:29:26 -0800

162d137  Hyphens an other such things

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2030/files/03d7c78ae5a0e99410d7ba202afeadfc5a30bfdf..162d1373f167eea6f85f2ab94af54d89caca0957

---

From: Martin Thomson <[email-masked]>
Date: Wed, 21 Nov 2018 16:30:31 -0800

0d714e1  182 bits

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2030/files/162d1373f167eea6f85f2ab94af54d89caca0957..0d714e1e4f059558c54995e435e55f1326c99654

---

From: Martin Thomson <[email-masked]>
Date: Wed, 21 Nov 2018 16:30:34 -0800

0                   1                   2                   3

And I very carefully calculated this, having first reached this conclusion (182), then recalculated and got 190.  Thanks.

-- 
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/2030#discussion_r235573289

---

From: Martin Thomson <[email-masked]>
Date: Wed, 21 Nov 2018 16:30:43 -0800

Merged #2030 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/2030#event-1981365496