Subject: Re: [quicwg/base-drafts] Apply the 3x amplification-limit to migration too (#4264)

From: Christian Huitema <[email-masked]>
Date: Tue, 08 Dec 2020 18:47:22 -0800

@huitema requested changes on this pull request.

Padding should be suggested (SHOULD), not mandated (MUST), at least in NAT traversal scenarios.

> @@ -2242,9 +2247,18 @@ The endpoint MUST use unpredictable data in every PATH_CHALLENGE frame so that
 it can associate the peer's response with the corresponding PATH_CHALLENGE.
 
 An endpoint MUST expand datagrams that contain a PATH_CHALLENGE frame to at
-least the smallest allowed maximum datagram size of 1200 bytes.  Sending UDP
-datagrams of this size ensures that the network path from the endpoint to the
-peer can be used for QUIC; see {{datagram-size}}.
+least the smallest allowed maximum datagram size of 1200 bytes, unless the
+anti-amplification limit for the path does not permit sending a datagram of
+this size.  Sending UDP datagrams of this size ensures that the network path
+from the endpoint to the peer can be used for QUIC; see {{datagram-size}}.
+

I know it is late in the process, but I would much prefer the paragraph to say that "An endpoint SHOULD expand datagrams that contain a PATH_CHALLENGE frame..." The reason is that if we say MUST, picky implementations will account the number of bytes sent and discard un-padded path challenges if they sent 400 bytes of data or more. Such measurements are very hard to get right, so we will end up with interoperability issues. 

> @@ -2242,9 +2247,18 @@ The endpoint MUST use unpredictable data in every PATH_CHALLENGE frame so that
 it can associate the peer's response with the corresponding PATH_CHALLENGE.
 
 An endpoint MUST expand datagrams that contain a PATH_CHALLENGE frame to at
-least the smallest allowed maximum datagram size of 1200 bytes.  Sending UDP
-datagrams of this size ensures that the network path from the endpoint to the
-peer can be used for QUIC; see {{datagram-size}}.
+least the smallest allowed maximum datagram size of 1200 bytes, unless the
+anti-amplification limit for the path does not permit sending a datagram of
+this size.  Sending UDP datagrams of this size ensures that the network path
+from the endpoint to the peer can be used for QUIC; see {{datagram-size}}.
+

This is specifically an issue for server responses to NAT traversal. A man on the side could trigger a number "fake" NAT traversal, and it makes sense for servers to avoid sending large packets in response to that.

-- 
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/4264#pullrequestreview-547794093

---

From: ianswett <[email-masked]>
Date: Wed, 09 Dec 2020 06:56:18 -0800

@ianswett commented on this pull request.

In practice, these fake NAT traversals don't seem so concerning because most ports on a NAT are going to be unused and dropped by the NAT itself.  And I believe most Large(ie: Carrier Grade) NATs are symmetric, so even if the port was open, the traffic would be dropped.

-- 
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/4264#discussion_r539374562

---

From: Christian Huitema <[email-masked]>
Date: Wed, 09 Dec 2020 08:11:50 -0800

@huitema commented on this pull request.

The problem happens when the attacker is not behind a NAT, say is using IPv6. Suppose the attacker is an evil client, intent on targeting IPv6 address X, or maybe IPv6 prefix X/64. It can set up a connection, then manufacture a series of short packets with faked source addresses and ports -- packets less than 100 bytes including the IP header, each triggering a 1200 byte response. The evil client can set up an arbitrary number of good connections to do that, effectively getting a factor 12 amplification for a large stream of 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/4264#discussion_r539439998

---

From: ianswett <[email-masked]>
Date: Wed, 09 Dec 2020 08:31:58 -0800

I agree that if the IP changes, assuming it is a NAT migration is a bit dangerous, so I only had port in mind.

-- 
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/4264#discussion_r539456996

---

From: Martin Thomson <[email-masked]>
Date: Wed, 09 Dec 2020 08:37:50 -0800

@martinthomson commented on this pull request.

I don't want to make a change here.  We already [prohibit enforcement of the padding rule](https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#section-14-7).  That's enough.

-- 
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/4264#discussion_r539462050

---

From: Christian Huitema <[email-masked]>
Date: Wed, 09 Dec 2020 09:11:39 -0800

See discussion and proposed fix in issue #4257 

-- 
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/4264#discussion_r539489463

---

From: Christian Huitema <[email-masked]>
Date: Wed, 09 Dec 2020 09:16:19 -0800

@martinthomson the padding enforcement rule says "an endpoint MUST NOT close a connection when it receives a datagram that does not meet size constraints; the endpoint MAY however discard such datagrams." The latter behavior, discarding the datagram, is exactly what leads to interop failure when the server does not pad because of amplification concerns, and the client drops because it interprets the clause as "MUST pad".

-- 
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/4264#discussion_r539493004

---

From: Martin Thomson <[email-masked]>
Date: Wed, 09 Dec 2020 18:49:15 -0800

Happy to add text specific to this instance that prohibits enforcement.

-- 
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/4264#discussion_r539803851

---

From: Martin Thomson <[email-masked]>
Date: Wed, 09 Dec 2020 18:56:27 -0800

@martinthomson pushed 1 commit.

8324d829dbf4add57887e10ce7cdb829b6afde3f  No-enforcement clause for path validation datagram size

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4264/files/3e5b0fd79e99bc1dfef884234b2699240ced1a3a..8324d829dbf4add57887e10ce7cdb829b6afde3f

---

From: Martin Thomson <[email-masked]>
Date: Wed, 09 Dec 2020 18:57:32 -0800

I've added some text that prohibits dropping.  I will note that at the point that you detect this violation, you have likely already started to process the packet, so enforcement of the requirement is a little dangerous.

-- 
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/4264#discussion_r539806765

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 09 Dec 2020 19:01:15 -0800

@kazuho 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/4264#pullrequestreview-548783049

---

From: David Schinazi <[email-masked]>
Date: Wed, 09 Dec 2020 19:16:26 -0800

@DavidSchinazi approved this pull request.

> @@ -1921,9 +1921,14 @@ generates more or larger packets in response to that packet, the attacker can
 use the server to send more data toward the victim than it would be able to send
 on its own.
 
-The primary defense against amplification attack is verifying that an endpoint
-is able to receive packets at the transport address that it claims.  Address
-validation is performed both during connection establishment (see
+The primary defense against amplification attack is verifying that a peer is

```suggestion
The primary defense against amplification attacks is verifying that a peer is
```

> @@ -6500,10 +6530,16 @@ that claims a given address is able to receive packets at that address. Address
 validation limits amplification attack targets to addresses for which an
 attacker can observe packets.
 
-Prior to validation, endpoints are limited in what they are able to send.
-During the handshake, a server cannot send more than three times the data it
-receives; clients that initiate new connections or migrate to a new network
-path are limited.
+Prior to address validation, endpoints are limited in what they are able to
+send.  Endpoints cannot send data toward an unvalidated address in excess of
+three times the data received from that address.
+
+Note:
+
+: The anti-amplification limit only applies when an endpoint responds to packets
+  received from an unvalidated address. The anti-amplification limit does not
+  apply to clients when establishing a new connection or when initiating path

```suggestion
  apply to clients when establishing a new connection or when initiating connection
```

IMO the added text prohibiting dropping resolves this problem

-- 
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/4264#pullrequestreview-548786001

---

From: Martin Thomson <[email-masked]>
Date: Wed, 09 Dec 2020 19:17:29 -0800

c549aeee5d2ddb9814c920e554c468d42761e70d  gramma

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4264/files/8324d829dbf4add57887e10ce7cdb829b6afde3f..c549aeee5d2ddb9814c920e554c468d42761e70d

---

From: Martin Thomson <[email-masked]>
Date: Wed, 09 Dec 2020 19:17:59 -0800

07989a2b43ce5bd09f6e5606584cef2bcfdc0970  Suggestion

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4264/files/c549aeee5d2ddb9814c920e554c468d42761e70d..07989a2b43ce5bd09f6e5606584cef2bcfdc0970

---

From: Martin Thomson <[email-masked]>
Date: Wed, 09 Dec 2020 19:18:14 -0800

Thanks.  Added manually to avoid line length limit blowout.

-- 
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/4264#discussion_r539813895

---

From: Christian Huitema <[email-masked]>
Date: Wed, 09 Dec 2020 19:39:38 -0800

The added text solves my issue.

-- 
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/4264#discussion_r539821252

---

From: Martin Thomson <[email-masked]>
Date: Wed, 09 Dec 2020 20:53:23 -0800

Merged #4264 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/4264#event-4093165440