Subject: [quicwg/base-drafts] Discard Initial keys as soon as possible (#2045)

From: Martin Thomson <[email-masked]>
Date: Thu, 22 Nov 2018 17:00:18 -0800

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

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

-- Commit Summary --

  * Discard Initial keys as soon as possible

-- File Changes --

    M draft-ietf-quic-recovery.md (32)
    M draft-ietf-quic-tls.md (21)

-- Patch Links --

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

---

From: Marten Seemann <[email-masked]>
Date: Thu, 22 Nov 2018 20:47:20 -0800

marten-seemann approved this pull request.

> @@ -691,6 +692,24 @@ will be marked as lost before this, as they leave a gap in the sequence of
 packet numbers.

+## Discarding Initial Keys {#discard-initial}
+
+Packets protected with Initial secrets ({{initial-secrets}}) are not
+authenticated, meaning that an attacker could spoof packets with the intent to
+disrupt a connection.  To limit these attacks, Initial packet protection keys
+can be discarded more aggressively than other keys.
+
+The successful use of Handshake packets indicates that no more Initial packets
+need to be exchanged, as these keys can only be produced after receiving all
+CRYPTO frames from Initial packets.  Thus, a client MUST discard Initial keys
+when it first sends a Handshake packet and a server MUST discard Initial keys
+when it first successfully processes a Handshake packet.  Endpoints MUST NOT
+send Initial packets after this point.

Should we say that this means that the packet triggering this will never be acknowledged?

-- 
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/2045#pullrequestreview-177808290

---

From: ekr <[email-masked]>
Date: Fri, 23 Nov 2018 11:34:07 -0800

ekr requested changes on this pull request.

> @@ -439,6 +456,13 @@ Either packet indicates that the Initial was received but not processed.
 Neither packet can be treated as an acknowledgment for the Initial, but they MAY
 be used to improve the RTT estimate.
 
+#### Discarding Initial State
+
+As described in Section 4.10 of {{QUIC-TLS}}, endpoints stop sending and
+receiving Initial packets once they start exchanging Handshake packets.  At this
+point, all loss recovery state for the Initial encryption level is also
+discarded.

I'm not sure I understand what this mean; it seems like there is one set of variables. Do you mean you reset all the variables to the initial values.

> @@ -1059,6 +1083,14 @@ A sender that does not use pacing SHOULD reset its congestion window to the
 minimum of the current congestion window and the initial congestion window.
 This recommendation is based on Section 4.1 of {{?RFC5681}}.
 
+## In-Flight Packet Accounting
+
+When keys for an encryption level are discarded (see {{QUIC-TLS}}), any packets
+sent with those keys are removed from the count of bytes in flight.  No loss
+events will occur for these packets.  Note that it is expected that keys are
+discarded after those packets would be declared lost, but Initial secrets are
+destroyed earlier.

I'm not sure I understand this text.

 packet numbers.

I thought the sense of the discussion in BKK was *not* to do this. It's a weird kind of implicit ACKIng. This probably needs list and issue discussion, not just merging at this time.

-- 
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/2045#pullrequestreview-178014366

---

From: Marten Seemann <[email-masked]>
Date: Fri, 23 Nov 2018 19:22:50 -0800

marten-seemann commented on this pull request.

 packet numbers.

That's exactly how the defense against this attack works. Only by stopping to process Initial data as early as possible, we can reduce the attack surface of the injection attack. This means that an endpoint won't be able to read the ACK for the last Initial packet it sent either.
And if we're certain that the ACK can't be read, why send it in the first place?

-- 
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/2045#discussion_r236031146

---

From: ekr <[email-masked]>
Date: Fri, 23 Nov 2018 20:34:25 -0800

ekr commented on this pull request.

 packet numbers.

Yes, and it's not clear to me it's worth making *this* change to defend against this attack. As I said, I don't recall there being consensus to make 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/2045#discussion_r236032170

---

From: Rui Paulo <[email-masked]>
Date: Fri, 23 Nov 2018 21:34:19 -0800

rpaulo commented on this pull request.

 packet numbers.

I agree with @marten-seemann.  What this means is that implementations won't even bother sending any ACKs.  Perhaps the recommendation should be to discard the keys once the ACK arrives.

-- 
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/2045#discussion_r236032934

---

From: Marten Seemann <[email-masked]>
Date: Fri, 23 Nov 2018 21:41:18 -0800

packet numbers.

@rpaulo To clarify, I'm saying we should not send an ACK, and we should discard the keys as soon as possible.
Delaying to discard the keys until the ACK arrives is exactly what makes the attack possible in the first place: If we rely on ACKs, there's the problem that the ACK might get lost. Then the peer has to retransmit the last Initial packet, just to trigger the ACK (although it already knows that the packet was received, because new TLS keys became available). In the end, you'll end up generating and accepting Initial packets for **a lot** longer than the 3-way handshake in which they are actually needed, which is a big window for an attacker to inject a packet that will kill the connection.

-- 
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/2045#discussion_r236033019

---

From: Rui Paulo <[email-masked]>
Date: Sat, 24 Nov 2018 12:08:47 -0800

packet numbers.

Okay, I see what you mean.  I think that from a transport protocol perspective, not sending an ACK isn't good since you just ignored a possible RTT estimation.  You could estimate the RTT from the new handshake packet, I suppose, but that's not how the transport part works.

I'm not sure I understand the attack since to spoof the initial packet,  the third party must be doing a man in the middle attack.  In that case, why not do it early and abuse the stateless reset or retry?

-- 
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/2045#discussion_r236052467

---

From: MikkelFJ <[email-masked]>
Date: Sat, 24 Nov 2018 12:33:00 -0800

mikkelfj commented on this pull request.

 packet numbers.

It is not a man in the middle attack, it is a man on the side attack. The initial packet can be decrypted and a response can be constructed, but at the next encryption level that option is gone. This is why the initial is special. 

If it is possible to forge a reset from the initial packet, something is wrong in the design. It is not possible when a new connection id is produced, but I'm not sure if there the CID is fixed zero length. If there is an attack here, the reset token ought to moved further into the handshake.

-- 
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/2045#discussion_r236052984

---

From: Rui Paulo <[email-masked]>
Date: Sat, 24 Nov 2018 13:24:06 -0800

packet numbers.

Sure but the attacker my be on path. My point was that you need to know the IP, port and CIDs.  The attack gets significantly easier when the CID length is zero.

-- 
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/2045#discussion_r236054086

---

From: MikkelFJ <[email-masked]>
Date: Sat, 24 Nov 2018 13:29:11 -0800

packet numbers.

Just for the perspective, attackers with observation powers are very very real. Aside from intelligence service taps, there is also infiltration. Even if you don't control the path, you migt sit on the same LAN because another machine was easier to infect.

This is a good read: https://www.wired.com/story/notpetya-cyberattack-ukraine-russia-code-crashed-the-world/

-- 
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/2045#discussion_r236054189

---

From: Kazuho Oku <[email-masked]>
Date: Sun, 25 Nov 2018 19:14:46 -0800

@ekr 
> I thought the sense of the discussion in BKK was _not_ to do this. It's a weird kind of implicit ACKIng.

I share the view that it is a variant of implicit ACKing, something that we decided to not adopt in Kista. OTOH, it _is_ an improvement in terms of preventing man-on-the-side attacks that try to disrupt connection establishment.

IMO, the proposed approach is trivial to implement for everybody. In addition to that, it simplifies QUIC stacks that implement protection against Initial injection, because they are no longer required to continue responding to Initial packets (that might contain rogue information).

Considering the aspects, as stated in https://github.com/quicwg/base-drafts/issues/1951#issuecomment-438112817, I am now in favor of adopting 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/2045#issuecomment-441507070

---

From: Christian Huitema <[email-masked]>
Date: Sun, 25 Nov 2018 21:24:59 -0800

huitema commented on this pull request.

We discussed some of that in the chat room (because we enjoy splitting discussions on many channels with different note well rules). The general idea is that if packets are implicitly acknowledged, then the corresponding bytes are removed from the count of bytes in flight. To quote, "_Implicit ACK should be treated just like regular ACK, but there are 2 small issues -- spurious repeat, and ACK. The handshake can complete with a spurious repeat still in flight, or with a not-yet-received ACK. Still, that cannot be a huge deal._"

-- 
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/2045#discussion_r236125398

---

From: Christian Huitema <[email-masked]>
Date: Sun, 25 Nov 2018 21:42:49 -0800

packet numbers.

I agree with the spirit of the text: we must defend against this attack, and the proper way has to be some form of implicit ACK. But I would like to see the implicit acknowledgement mechanism specified in the transport draft, because it affects the transport state machine. Obviously, bad things would happen if one side decided to stop send sending Initial packets while the other side was still waiting for ACK of its own packets, and repeating them. That means we must be crystal clear on when exactly to perform this "initial cut-off": on the client side, when the Handshake secret is successfully received and the client starts being able receiving handshake packets; on the server side, when the first handshake packet is received from the server.

 packet numbers.

I also looked at how to implement a similar protection without this kind of cut-off, and it just does not work. Suppose for example that the client receives a repeated Initial packet after the cut-off. In the post cutoff defensive mode, it can decide to ignore the packet content, since it cannot possibly learn anything interesting, but it would still need to send an ACK, otherwise the peer will keep repeating 3 or 4 times and then give up on the connection. But then, if the packet from the "server" was forged, the ACK from the client will be treated as a protocol violation, and the server will close the connection. Catch 22, no good way to get out of 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/2045#discussion_r236126691

---

From: ianswett <[email-masked]>
Date: Mon, 26 Nov 2018 18:44:08 +0000 (UTC)

ianswett commented on this pull request.

I agree with @huitema that this needs to change the transport draft.

With this change, I don't believe there's a purpose to the server ACKing the client's initial, because the client's Initial is always 1 packet?  If we make it illegal for the server to ACK the client's initial, we can remove the text I wrote to resolve the deadlock caused by the anti-amplification factor.  #1764 

-- 
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/2045#pullrequestreview-178442133

---

From: Christian Huitema <[email-masked]>
Date: Mon, 26 Nov 2018 11:15:54 -0800

@ianswett the main problem with "no ACK" is that we will need special handling of retransmissions on the server. The server would need to trigger retransmission of Initial packets upon reception of a duplicate CH, and that's clearly some new ad hoc code. 

Also, what if the server does not like any of the key shares proposed in the Client Hello? Client sends CH1, server replies with HRR, client sends a modified CH2, server sends SH. If the client repeats CH1, the server should repeat HRR; if the client repeats CH2, the server needs to repeat the CH. I assume this could work if the client always coalesced an ACK with CH2, but this coalescing is optional, not mandated.

-- 
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/2045#issuecomment-441761641

---

From: ianswett <[email-masked]>
Date: Mon, 26 Nov 2018 11:24:41 -0800

The server's retransmission of an Initial is based on a timer, not receipt of the client's initial.  The exception is if the server can't send due to the anti-amplification attack limit.

Thanks for the reminder of HRR, handling that correctly would require more complex implicit ACK logic than I had 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/2045#issuecomment-441764495

---

From: Christian Huitema <[email-masked]>
Date: Mon, 26 Nov 2018 11:43:14 -0800

@ianswett Timer-based repetition of initial packets is an amplification: one packet in, N packets repeated until the server gives up. So it is complicated. Let's not try to specify ACK logic in this specific PR. It really belongs in its own PR, and as you note there will be many details to get right.

-- 
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/2045#issuecomment-441771885

---

From: ianswett <[email-masked]>
Date: Mon, 26 Nov 2018 12:30:46 -0800

@huitema I was trying to clarify that timer based repetition of the crypto data, including Initial, is already present in the recovery draft.

-- 
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/2045#issuecomment-441786975

---

From: Martin Thomson <[email-masked]>
Date: Tue, 27 Nov 2018 15:28:12 -0800

@martinthomson pushed 1 commit.

651f0ef  Add mention of discarding Initial to transport

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2045/files/1b928f21b3b50aecf271fdda148c693b660edca5..651f0ef36217eb3ac6ca74fa6b79a23674abc22f

---

From: Christian Huitema <[email-masked]>
Date: Tue, 27 Nov 2018 15:35:52 -0800

huitema 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/2045#pullrequestreview-179062971

---

From: Martin Thomson <[email-masked]>
Date: Tue, 27 Nov 2018 23:42:29 +0000 (UTC)

1c76c90  Add more stuff to transport about CRYPTO discards

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2045/files/651f0ef36217eb3ac6ca74fa6b79a23674abc22f..1c76c90291b4c132cf53feb91dc0de5cb89a6847

---

From: Martin Thomson <[email-masked]>
Date: Tue, 27 Nov 2018 16:53:46 -0800

98feded  when

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2045/files/1c76c90291b4c132cf53feb91dc0de5cb89a6847..98feded4a7630c7c0b6cfd47066d671706d1c25b

---

From: ianswett <[email-masked]>
Date: Tue, 27 Nov 2018 18:21:06 -0800

Thanks for the update Martin, I'd like to look at this PR more, but this is a clear improvement.

-- 
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/2045#pullrequestreview-179098650

---

From: Mike Bishop <[email-masked]>
Date: Wed, 28 Nov 2018 10:22:15 -0800

MikeBishop commented on this pull request.

> @@ -3592,6 +3594,21 @@ and will contain a CRYPTO frame with an offset matching the size of the CRYPTO
 frame sent in the first Initial packet.  Cryptographic handshake messages
 subsequent to the first do not need to fit within a single UDP datagram.
 
+
+### Abandoning Initial Packets {#discard-initial}
+
+A client stops both sending and accepting Initial packets when it sends its
+first Handshake packet.  A server stops sending and accepting Initial packets
+when it receives its first Handshake packet.  Though packets might still be in
+flight or awaiting acknowledgment, no further Initial packets need to be
+exchanged beyond this point.  Initial packet protection keys are discarded (see
+Section 4.10 of {{QUIC-TLS}}) along with any loss recovery and congestion
+control state (see Sections 5.3.1.2 and 6.9 of {{QUIC-RECOVERY}}).

Not my area, but I'm not sure implies the right thing to say that any congestion control state gets discarded here.  The congestion controller doesn't care what encryption level things are at, it just cares about bytes.

Perhaps it would be worth being more concrete:  "discarded(...), and all Initial packets are considered acknowledged for loss recovery and congestion control purposes (see...)."  This would also make the following paragraph unnecessary.

-- 
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/2045#pullrequestreview-179446792

---

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

janaiyengar commented on this pull request.

+

+1. @MikeBishop 's proposed text also explicitly articulates that this is an implicit acknowledgement, something that we've been shy to do before.

-- 
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/2045#discussion_r237331738

---

From: Martin Thomson <[email-masked]>
Date: Wed, 28 Nov 2018 18:14:08 -0800

martinthomson commented on this pull request.

+

I don't think that it is the case.  We don't really say anything from a loss recovery perspective - the packets are not declared lost.  The congestion controller is just told to remove the bytes from in flight - they don't cause the CWND to increase.  So it's not really acknowledgment.

As the changes to the recovery draft state: loss recovery state is just thrown out: you won't be retransmitting anything at the encryption level.  So saying anything more isn't really helpful there.  The effect on the congestion controller is intentionally neutral: don't increase CWND as though the packets got through, but don't keep the bytes on the books as though the packets were lost.

Or have I screwed this up?

-- 
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/2045#discussion_r237332488

---

From: Mike Bishop <[email-masked]>
Date: Thu, 29 Nov 2018 08:49:14 -0800

+

Okay, I see that reading of the -recovery text.  But that's really weird -- we *do* know that these packets have arrived, so why define a special operation for "forgetting" packets rather than just marking them arrived as normal and be done?

If one component were trying to game the other -- for example, by sending many Initial packets with redundant data -- then you wouldn't want to let the packetizer force many packets to be "acknowledged" when only one actually needed to get through to produce the effect.  But the ways to game that I see offhand are intra-implementation, and I think we can say that's not an attack of concern.

-- 
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/2045#discussion_r237569491