Subject: [quicwg/base-drafts] minimum payload size requirement creates awkward special case (#2049)

From: Marten Seemann <[email-masked]>
Date: Sat, 24 Nov 2018 20:14:43 -0800

In #2030 we set the minimum payload size to `4-packet number length`.
This creates a really awkward special case in my code. When packing STREAM frames, I make sure that the last STREAM frame doesn't have the length flag set, so I can save the bytes for encoding the payload length.

In the corner case where I only have a single stream for new a stream with stream ID < 64 that is closing that stream at offset 0, the length of this STREAM frame is just 2 bytes: 1 (type byte) + 1 (stream ID).
If I now add a PADDING frame in order to fulfill the minimum size requirement, this will be interpreted as part of the payload of the STREAM frame, so I will need special logic to detect the special case described above, and set the length flag on the STREAM frame in that case.

The motivation for #2030 was to avoid a special case that would rarely be executed (and which would lead to an undecryptable packet in the worst case). It seems that we achieved that by introducing an even rarer special case, which leads to incorrect delivery of stream data if not properly taken care 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/issues/2049

---

From: Kazuho Oku <[email-masked]>
Date: Sun, 25 Nov 2018 00:00:28 -0800

This is an interesting pitfall!

Maybe you can prepend the PADDING frames instead?

-- 
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/issues/2049#issuecomment-441422530

---

From: Marten Seemann <[email-masked]>
Date: Sun, 25 Nov 2018 00:18:12 -0800

I could, if I knew that there I’ll run into this corner case beforehand. But in my implementation, I’ll only know that after packing the STREAM frames. I could also write a check for exactly this special case, and then modify the STREAM frame to include the length field. 

I’m not saying that this problem is not solvable - I just doubt that it’s easier than adjusting the sample offset, plus the consequences for getting it wrong a more severe. 

-- 
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/issues/2049#issuecomment-441423333

---

From: Kazuho Oku <[email-masked]>
Date: Sun, 25 Nov 2018 09:31:14 +0000 (UTC)

> I could, if I knew that there I’ll run into this corner case beforehand. But in my implementation, I’ll only know that after packing the STREAM frames.

What I mean is that when you notice that the length of the streams is below 4 bytes, you can move them backwards and insert PADDING before them. That logic would replace the logic you have, that appends PADDING frames.

The performance impact of prepending afterwards (i.e. moving and inserting) is negligible compared to the overhead of AEAD.

-- 
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/issues/2049#issuecomment-441427122

---

From: Marten Seemann <[email-masked]>
Date: Sun, 25 Nov 2018 02:17:11 -0800

> What I mean is that when you notice that the length of the streams is below 4 bytes, you can move them backwards and insert PADDING before them. That logic would replace the logic you have, that ap…

Sure, I can think of multiple ways to fix this, and I'm not concerned about performance at all. I'm concerned about correctness and implementation complexity.

My point is: I think the way of packing frames and then adding padding is a common way to pack packets, and many implementations will choose to implement it that way. The corner case I described is **very subtle**, and I expect this to be the source of nasty bugs in the future. I think we could save ourselves a lot of trouble by reverting #2030.

-- 
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/issues/2049#issuecomment-441429539

---

From: Nick Banks <[email-masked]>
Date: Sun, 25 Nov 2018 08:40:06 -0800

I think this can be trivially solved by updating your logic that decides if the length should be included in the STREAM frame. When writing the frame to the packet buffer, if the length of the stream data is less than 4 bytes just always include 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/issues/2049#issuecomment-441453821

---

From: MikkelFJ <[email-masked]>
Date: Sun, 25 Nov 2018 13:09:44 -0800

I think this header protection is starting to bleed into the core packet processing and I don't think that is a particular good idea.

If you revert to the old method header protection method, @kazuho argues that both sender and receiver must perform a check for adjustment. However, you must still check that the received packet is long enough to sample since the packet might be evil or faulty. If the length check is for the common case where no offset adjustment is needed, there is no extra overhead. If the check fails you need to perform a new length check that can handle short packet numbers with short content.

Special casing short packets is a bit unfortunate if you have a lot of short packets to process, but the the overhead is still limited and more importantly isolated to one place in the logic framework. Also, if there are many short packets, branch prediction will likely figure it out.

With the current padding solution you get the special case @marten-seemann mentions, but you might also still have to check if the sampling does not overlap with payload when the packet number is short and the payload is short. If you don't check this you could be triggered to xor the mask into the payload. Likely AEAD will catch this but it is an unsafe praxis, so you still have extra checks for short 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/issues/2049#issuecomment-441473483

---

From: Martin Thomson <[email-masked]>
Date: Sun, 25 Nov 2018 19:42:51 -0800

Maybe don't pad, but change the packet number encoding instead.

-- 
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/issues/2049#issuecomment-441510499

---

From: janaiyengar <[email-masked]>
Date: Mon, 26 Nov 2018 19:34:09 -0800

I'm with Nick on this one. I've dealt with this exact problem in a
different context. You need to have logic to decide whether the stream
frame is going to have a length field or not. A simple rule that says
include the length field if the stream frame data size is < 2 makes it a
simple fix.

On Sun, Nov 25, 2018 at 7:42 PM Martin Thomson <[email-masked]>
wrote:

> —
> Reply to this email directly, view it on GitHub
> <https://github.com/quicwg/base-drafts/issues/2049#issuecomment-441510499>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AKjg1KMkSdW_FJRHF6nep4Dx7dHAtgVZks5uy2M7gaJpZM4YxtMJ>
> .
>

-- 
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/issues/2049#issuecomment-441889385

---

From: MikkelFJ <[email-masked]>
Date: Tue, 27 Nov 2018 16:18:03 +0000 (UTC)

The problem with the simple stream frame check is that it buries logic far away from header protection where it is rather counter intuitive.

-- 
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/issues/2049#issuecomment-442119428

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 27 Nov 2018 17:32:32 -0800

@mikkelfj As I stated in 
https://github.com/quicwg/base-drafts/issues/2049#issuecomment-441427122, there are other approaches that you can consider.

-- 
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/issues/2049#issuecomment-442284241

---

From: janaiyengar <[email-masked]>
Date: Wed, 28 Nov 2018 18:29:53 -0800

@mikkelfj : I don't think that's a problem. Like I said, encoding stream frame size is related to packet size  anyway, so it is logic that is going to be a bit non-local as it is.

-- 
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/issues/2049#issuecomment-442683269