Subject: Re: [quicwg/base-drafts] Add a section on what to do when unprotecting a packet fails (#345)

From: ianswett <[email-masked]>
Date: Wed, 01 Mar 2017 07:57:04 -0800

ianswett commented on this pull request.

> @@ -778,6 +778,21 @@ the connection that is hosted on stream 1.  This sequence number is not visible
 to QUIC.

+## Receiving Protected Packets
+
+Once an endpoint successfully receives a packet with a given packet number, it
+MUST discard all packets with higher packet numbers if they cannot be
+successfully unprotected with either the same key, or - if there is a key update
+- the next packet protection key (see {{key-update}}).  Similarly, a packet that

We could require closing the connection if a packet was already processed, under the theory it's either an implementation error or someone intentionally trying to corrupt the connection?

I believe it's as simple as checking if any packets with larger have been processed when a key change occurs?

> @@ -778,6 +778,21 @@ the connection that is hosted on stream 1.  This sequence number is not visible
 to QUIC.

+## Receiving Protected Packets
+
+Once an endpoint successfully receives a packet with a given packet number, it
+MUST discard all packets with higher packet numbers if they cannot be
+successfully unprotected with either the same key, or - if there is a key update
+- the next packet protection key (see {{key-update}}).  Similarly, a packet that
+appears to trigger a key update, but cannot be unprotected successfully MUST be
+discarded.
+
+Failure to unprotect a packet does not necessarily indicate the existence of a
+protocol error in a peer or an attack.  The truncated packet number encoding

I think it's more likely the internet corrupts the packet.  Also, incorrect truncation is a form of peer bug I'd argue.

-- 
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/345#pullrequestreview-24512765

---

From: Martin Thomson <[email-masked]>
Date: Wed, 01 Mar 2017 15:19:02 -0800

martinthomson commented on this pull request.

 to QUIC.

The problem there is that it's such a bizarre corner case that you wouldn't necessarily have code for it.  If the packets arrive in the other order, you will drop the higher packet number because you don't even try to decrypt it.  Maintaining the invariant that order of processing doesn't matter is hard here.  It's also the case that the obvious implementation of this doesn't necessarily couple the choice of advancing the keys with updating the last received packet number (I've implemented this for DTLS, and while the code is nearby, it's much easier not to entangle these concepts).

I think that I would rather say "MAY kill the connection".  That leaves the possibility open, and I don't have to explain how tricky it is to get this all lined 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/345#discussion_r103815242

---

From: Martin Thomson <[email-masked]>
Date: Wed, 01 Mar 2017 15:21:59 -0800

to QUIC.

I thought that too, but a few factors lead me to think that this is still worth saying.  Where the error comes from doesn't change the outcome.  And severe reordering does happen on occasional packets.  Also, your current scheme for picking the truncation length fails if there is a sudden large increase in send rate; packets before the jump that are delayed until after the jump in rate might have shorter packet numbers and thus could be decoded incorrectly.

-- 
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/345#discussion_r103815721

---

From: Martin Thomson <[email-masked]>
Date: Wed, 01 Mar 2017 16:04:22 -0800

@martinthomson pushed 1 commit.

da610a0  Deal with bad key phase changes somehow

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/345/files/ef7f5c3740ade2150bd6673f0f514b13ed5843d7..da610a0be9b5ab4b5469c7b0827e588afa4e25b9

---

From: Martin Thomson <[email-masked]>
Date: Wed, 01 Mar 2017 16:07:48 -0800

I added a small piece of text about the weird reordering Marten pointed out.  I flip-flopped on whether to use MUST, but I think that it is the right answer here.  You will observe that detection isn't guaranteed, so I decided that making the MUST conditional on detection gives implementations a tiny bit of wiggle room if they decide that detecting the condition is too tricky.

Note that it is not possible to generate packets that guarantee triggering this behaviour, so failure of a compliance test on this point is going to be hard to use as justification for a non-compliance label.

-- 
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/345#issuecomment-283513393

---

From: Sean Turner <[email-masked]>
Date: Thu, 09 Mar 2017 17:09:28 -0800

@seanturner pushed 1 commit.

081c551  Making it clear that an endpoint using an old key is indeciperable from an attack.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/345/files/da610a0be9b5ab4b5469c7b0827e588afa4e25b9..081c5517f8fc952a8accc914d74036e7d1b22906

---

From: Sean Turner <[email-masked]>
Date: Thu, 09 Mar 2017 17:10:38 -0800

seanturner approved this pull request.

I'd like to make it clear that if the client starts using old keys it's hard to differentiate that from an attack.  Other than that I'm good with it.

-- 
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/345#pullrequestreview-26185981

---

From: Martin Thomson <[email-masked]>
Date: Thu, 09 Mar 2017 17:13:29 -0800

Merged #345.

-- 
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/345#event-994180892