Subject: [quicwg/base-drafts] allow dropping of Initial packets with invalid reserved bits (#2053)

From: Marten Seemann <[email-masked]>
Date: Mon, 26 Nov 2018 02:48:36 -0800

This won't give you full protection against injection attacks, but endpoints should at least be allowed to drop packets that are obviously not valid QUIC packets.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * allow dropping of Initial packets with invalid reserved bits

-- File Changes --

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

-- Patch Links --

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

---

From: Kazuho Oku <[email-masked]>
Date: Mon, 26 Nov 2018 02:58:00 -0800

kazuho approved this pull request.

:+1:

-- 
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/2053#pullrequestreview-178231210

---

From: ekr <[email-masked]>
Date: Mon, 26 Nov 2018 13:28:24 +0000 (UTC)

I don't see what this is buying us. In order to generate a packet which deprotects successfully but has bogus reserved bits, the attacker must know the CID, which effectively means it must be on-path. Such an attacker can easily generate a packet with correct reserved bits but bogus content (or an error). Can you please describe the scenario you are concerned about?

-- 
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/2053#issuecomment-441638613

---

From: MikkelFJ <[email-masked]>
Date: Mon, 26 Nov 2018 05:47:24 -0800

> I don't see what this is buying us. In order to generate a packet which deprotects successfully but has bogus reserved bits, the attacker must know the CID, which effectively means it must be on-path

That is right, but it does happen offer a fast rejection of random packets (unless we require full AEAD verification to avoid timing attacks, which I think we do?). Fast rejection can be useful in DDoS attacks.

-- 
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/2053#issuecomment-441644201

---

From: Marten Seemann <[email-masked]>
Date: Mon, 26 Nov 2018 06:01:47 -0800

@ekr: I never claimed that this will buy us additional security. I’m just saying that dropping a malformed Initial is an equally valid response than closing the connection (that’s why it’s a MAY, not a MUST).

-- 
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/2053#issuecomment-441648644

---

From: ekr <[email-masked]>
Date: Mon, 26 Nov 2018 06:58:13 -0800

I don't see why it's any more valid here than after the connection is established. I would prefer to require consistent behavior.

-- 
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/2053#issuecomment-441667932

---

From: Martin Thomson <[email-masked]>
Date: Mon, 26 Nov 2018 15:25:43 -0800

If this is the position we take, then we should apply the rules more broadly.  Namely, allow an endpoint to drop any unauthenticated packet (Initial, VN, and Retry) that is invalid, provided that they haven't already committed changes to their state that cannot be reversed.  Patching changes in piecemeal are problematic.

-- 
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/2053#issuecomment-441839303

---

From: Marten Seemann <[email-masked]>
Date: Mon, 26 Nov 2018 17:19:21 -0800

I agree that we should apply this more broadly, however I'd like to note that we're currently in no way consistent:

For example for VN:
> A client MUST ignore a Version Negotiation packet that lists the client’s chosen version.

Or for Retry:
> Clients MUST discard Retry packets that contain an Original Destination Connection ID field that does not match the Destination Connection ID from its Initial 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/2053#issuecomment-441861435

---

From: Martin Thomson <[email-masked]>
Date: Mon, 26 Nov 2018 17:24:53 -0800

Those aren't a fair comparison.  The first might be, but the Retry thing is specifically a defense against off-path attack, something we agreed is important to defend against.

-- 
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/2053#issuecomment-441862516

---

From: Marten Seemann <[email-masked]>
Date: Mon, 26 Nov 2018 17:29:15 -0800

You're right, I forgot about the special case of a 0-length destination connection ID (again)...

-- 
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/2053#issuecomment-441863408

---

From: Kazuho Oku <[email-masked]>
Date: Thu, 29 Nov 2018 17:54:49 -0800

kazuho commented on this pull request.

> @@ -3278,9 +3278,10 @@ Reserved Bits (R):
 
 : The next two bits (those with a mask of 0x0c) of byte 0 are reserved.  These
   bits are protected using header protection (see Section 5.4 of {{QUIC-TLS}}).
-  The value included prior to protection MUST be set to 0.  An endpoint MUST
-  treat receipt of a packet that has a non-zero value for these bits after
-  removing protection as a connection error of type PROTOCOL_VIOLATION.
+  The value included prior to protection MUST be set to 0.  For Handshake and
+  0-RTT packets and endpoint MUST treat a non-zero value after removing
+  protection as a connection error of type PROTOCOL_VIOLATION. For Initial
+  packets, and endpoint MAY instead choose to drop the packet.

How about stating that, for Initial packets:
* an endpoint MUST ignore CRYPTO frames once it obtains Handshake keys
* an endpoint MUST ignore CONNECTION_CLOSE frames once it receives a Handshake packet
* an endpoint MUST ignore unexpected packet numbers found in an ACK frame

I think these three requirements are sufficient to address the attacks that happen _after_ 1 RTT.

-- 
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/2053#pullrequestreview-180095379

---

From: Martin Thomson <[email-masked]>
Date: Thu, 29 Nov 2018 20:08:01 -0800

martinthomson commented on this pull request.

In the end, this might be simpler.  You can ignore the content of Initial packets entirely, with the exception of ACK, which you only use to drive retransmission of your own Initial 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/pull/2053#discussion_r237741634