Subject: [quicwg/base-drafts] Remove DoS vector for spoofed connection migration (#2893)

From: Eric Kinnear <[email-masked]>
Date: Fri, 12 Jul 2019 15:43:04 -0700

Remove connection error response to a packets from a different source address when an endpoint has sent `disable_migration`, clarify an endpoints options when prohibited packets are received, rename `disable_migration` to `disable_active_migration`, and remove `INVALID_MIGRATION` error code.

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

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

-- Commit Summary --

  * Remove connection error response to a packets from a different source address when an endpoint has sent disable_migration, clarify an endpoints options when prohibited packets are received, rename disable_migration to disable_active_migration, and remove INVALID_MIGRATION error code.
  * Rewrap long line

-- File Changes --

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

-- Patch Links --

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

---

From: Martin Thomson <[email-masked]>
Date: Sun, 14 Jul 2019 17:08:54 -0700

martinthomson requested changes on this pull request.

In reviewing this, and in light of the changes in #2851, I'm no longer clear on what value this transport parameter provides.  It seems like the benefit extends only to the ability to not implement path validation or providing extra connection IDs.

I don't think that we should be encouraging implementations to not implement providing of new connection IDs.  So that leaves implementing path validation.  We should probably discuss that more.

All of that need not block this change.  With the removal of that sentence, it looks good to go.

> -Not all changes of peer address are intentional migrations. The peer could
-experience NAT rebinding: a change of address due to a middlebox, usually a NAT,
-allocating a new outgoing port or even a new outgoing IP address for a flow.  An
-endpoint MUST perform path validation ({{migrate-validate}}) if it detects any
-change to a peer's address, unless it has previously validated that address.
+An endpoint also MUST NOT send packets from a different local address, actively
+initiating migration, if the peer sent the `disable_active_migration` transport
+parameter during the handshake. An endpoint which has sent this transport
+parameter, but detects that a peer has nonetheless migrated to a different
+network MUST either drop the incoming packets on that path without generating a
+stateless reset or proceed with path validation and allow the peer to migrate.
+This helps to prevent third parties in the network causing connections to close
+due to spoofed addresses or other manipulation of observed traffic. An endpoint
+MUST NOT actively initiate migration if its peer supplies a zero-length
+connection ID as packets without a Destination Connection ID cannot be
+attributed to a connection based on address tuple.

With the change in #2851, we don't need this last sentence.  The point there is that source addresses aren't allowed to be used in routing of packets, so we don't need to worry about forbidding migration in that case.

-- 
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/2893#pullrequestreview-261606178

---

From: Eric Kinnear <[email-masked]>
Date: Wed, 17 Jul 2019 10:41:29 -0700

erickinnear commented on this pull request.

Good point! We cover this area much more thoroughly elsewhere, I'll remove this.

-- 
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/2893#discussion_r304559152

---

From: Eric Kinnear <[email-masked]>
Date: Wed, 17 Jul 2019 10:44:20 -0700

@erickinnear pushed 1 commit.

5a7293da9be6061ded0a277fe019a48235dff2d8  Remove sentence that's no longer necessary

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2893/files/b1bd4ac46dbcb0081c8ba46d4250c1cdf38688f8..5a7293da9be6061ded0a277fe019a48235dff2d8

---

From: Eric Kinnear <[email-masked]>
Date: Wed, 17 Jul 2019 10:44:35 -0700

Removed in 5a7293d

-- 
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/2893#discussion_r304560473

---

From: Eric Kinnear <[email-masked]>
Date: Wed, 17 Jul 2019 10:48:36 -0700

Re: The value of this TP, I tend to agree with @martinthomson, pretty much all of the machinery must be implemented whether or not the TP is sent. But agreed - let's discuss elsewhere, since I think this particular change is in pretty good shape now.

-- 
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/2893#issuecomment-512489783

---

From: ianswett <[email-masked]>
Date: Sat, 20 Jul 2019 13:31:23 -0700

ianswett approved this pull request.

One editorial suggestion

> -INVALID_MIGRATION.  Similarly, an endpoint MUST NOT initiate migration if its
-peer supplies a zero-length connection ID as packets without a Destination
-Connection ID cannot be attributed to a connection based on address tuple.
-

This = Not generating a stateless reset?  If so, how about "Generating a stateless reset would allow third parties in the network to cause a connection to close by spoofing or other manipulation of observed traffic."?

-- 
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/2893#pullrequestreview-264510290

---

From: Eric Kinnear <[email-masked]>
Date: Mon, 22 Jul 2019 13:42:31 -0700

e7d22976811f810ac5e0cb5d8927d659e5a3ad09  Reword per Ian's suggestion

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2893/files/5a7293da9be6061ded0a277fe019a48235dff2d8..e7d22976811f810ac5e0cb5d8927d659e5a3ad09

---

From: Eric Kinnear <[email-masked]>
Date: Mon, 22 Jul 2019 13:42:44 -0700

Changed in e7d2297, 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/2893#discussion_r306023502

---

From: Igor Lubashev <[email-masked]>
Date: Mon, 22 Jul 2019 20:18:01 -0700

I think PR is fine, right I would had added an explicit explanation of when a server may consider sending thing TP (other paths are likely to either not work or be very inefficient) and what a client that needs to migrate to a new path should do (recheck DNS from the new path and reconnect).

-- 
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/2893#issuecomment-514037478

---

From: Martin Thomson <[email-masked]>
Date: Tue, 23 Jul 2019 08:50:36 -0700

martinthomson 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/2893#pullrequestreview-265507965