Subject: [quicwg/base-drafts] Add protection against abusive packer number sequences (#1752)

From: MikkelFJ <[email-masked]>
Date: Sun, 16 Sep 2018 02:06:39 -0700

Fixes #1751 
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Add protection against abusive packer number sequences

-- File Changes --

    M draft-ietf-quic-transport.md (26)

-- Patch Links --

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

---

From: MikkelFJ <[email-masked]>
Date: Sun, 16 Sep 2018 07:03:40 -0700

@mikkelfj pushed 1 commit.

6f507ae  considerd

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1752/files/94110f39c56d3263f92023b2cd9f11009dee5060..6f507ae3e8d389c1815dd6c7944db0c0c56cf4db

---

From: MikkelFJ <[email-masked]>
Date: Sun, 16 Sep 2018 07:06:29 -0700

8ddf698  Also monitor gap count

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1752/files/6f507ae3e8d389c1815dd6c7944db0c0c56cf4db..8ddf6984be292d1a59bd670a4fdc11bb29d5b9d2

---

From: MikkelFJ <[email-masked]>
Date: Sun, 16 Sep 2018 07:11:30 -0700

db09026  Also add guideline on gap count

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

---

From: MikkelFJ <[email-masked]>
Date: Sun, 16 Sep 2018 07:34:46 -0700

73193e5  Fix optimistic-ack-attack reference

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1752/files/db09026d5f864e9a683bb052eaa824e4e7390b19..73193e5133b8fa7b36a377713ff2bb82f701eb42

---

From: MikkelFJ <[email-masked]>
Date: Sun, 16 Sep 2018 11:11:39 -0700

963b582  accceptable

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1752/files/73193e5133b8fa7b36a377713ff2bb82f701eb42..963b58264887e05b92b18cac8ba0da66fc846202

---

From: MikkelFJ <[email-masked]>
Date: Sun, 16 Sep 2018 11:24:45 -0700

12192c7  Uppercase SHOULD

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1752/files/963b58264887e05b92b18cac8ba0da66fc846202..12192c70c5bc6ae1672f92a477fc86891952c25b

---

From: ianswett <[email-masked]>
Date: Mon, 17 Sep 2018 06:29:58 -0700

ianswett commented on this pull request.

> @@ -1093,6 +1093,14 @@ protection for the reasons described in Section 9.3 of {{QUIC-TLS}}. An
 efficient algorithm for duplicate suppression can be found in Section 3.4.3 of
 {{?RFC2406}}.
 
+Packet numbers SHOULD be incremented by one. Parallel processing and optimistic
+ACK attack mitigation {{optimistic-ack-attack}} can lead to gaps. The difference
+between the largest sent and the largest acknowledged packet number
+SHOULD be no more than twice the actual number of packets sent.  The number of
+gaps and their sizes SHOULD be kept to a minimum.  A peer MAY close a connection
+with the error BAD_LINK when the observed gap count or loss rate based on packet

If we add an error code, I'd like to be specific about what it means.  ie: too many gaps?   BAD_LINK doesn't tell me if the connection is too poor to continue with or if there are too many gaps, which are potentially quite different.

-- 
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/1752#pullrequestreview-155922986

---

From: MikkelFJ <[email-masked]>
Date: Mon, 17 Sep 2018 06:38:47 -0700

mikkelfj commented on this pull request.

It was intended to cover all cases where transport cannot proceed without any factual errors in dataformat covered by other errors. We could have a TOO_MANY_GAPS error, but then it would not cover TOO_LARGE_GAPS, SUSPECTED_MALICOUS_REORDERING, NO_ACKS_RECEIVED_FOR_A_WHILE etc. So we could have a more specific error, but there are many, none of which are covered by other error codes at present. But I am open to suggestions.

-- 
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/1752#discussion_r218071123

---

From: MikkelFJ <[email-masked]>
Date: Mon, 17 Sep 2018 06:56:47 -0700

We could add a note about "SHOULD add a reason phrase" when using the BAD_LINK error.

-- 
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/1752#discussion_r218077981

---

From: Marten Seemann <[email-masked]>
Date: Mon, 17 Sep 2018 07:10:00 -0700

marten-seemann commented on this pull request.

I'm not a fan of adding a separate error here. There are a lot of places where an implementation might introduce some limit as a DoS defense (e.g. too many gaps in STREAM data), many of those implementation specific. BAD_LINK certainly isn't appropriate in many of those 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/1752#discussion_r218083404

---

From: MikkelFJ <[email-masked]>
Date: Mon, 17 Sep 2018 07:11:02 -0700

Which error would you use then? Or would you silently drop? I considered that 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/1752#discussion_r218083753

---

From: Nick Banks <[email-masked]>
Date: Mon, 17 Sep 2018 08:13:31 -0700

nibanks commented on this pull request.

I agree with @marten-seemann here. I think if an implementation has a specific limit/threshold, it should just silently drop.

-- 
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/1752#discussion_r218108295

---

From: MikkelFJ <[email-masked]>
Date: Mon, 17 Sep 2018 09:19:02 -0700

The reason I chose not to silently drop was the subsequent reset that would follow.

-- 
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/1752#discussion_r218134406

---

From: Martin Thomson <[email-masked]>
Date: Mon, 17 Sep 2018 14:13:30 -0700

martinthomson commented on this pull request.

As noted, the main problem here is stream fragmentation, not packet fragmentation.

I don't think we want this 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/1752#pullrequestreview-156121761

---

From: Martin Thomson <[email-masked]>
Date: Mon, 24 Sep 2018 15:25:12 -0700

Closing based on discussion.  We concluded that while there are optimizations that might be exposed to some sort of manipulation, the most obvious implementation isn't vulnerable.  Anyone optimizing can probably work this out for themselves.

-- 
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/1752#issuecomment-424145727

---

From: Martin Thomson <[email-masked]>
Date: Mon, 24 Sep 2018 22:25:17 +0000 (UTC)

Closed #1752.

-- 
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/1752#event-1864332780