quicwg / base-drafts

Internet-Drafts that make up the base QUIC specification
https://quicwg.org
1.63k stars 204 forks source link

Endpoints cannot verify padding #4253

Closed kazuho closed 3 years ago

kazuho commented 3 years ago

In #4188 #4183, we added the following sentence, but I think it does not make sense.

When a client receives an ack-eliciting Initial packet that is carried in a UDP datagram with a payload that is less than 1200 bytes, that client MAY close the connection by sending a CONNECTION_CLOSE frame.

Generally speaking, a receiver of a datagram cannot verify if the sender sent a full-sized datagram.

That is because a datagram as a whole is not authenticated. Instead, we authenticate each of the QUIC packets that constitute the datagram.

Therefore, if the sender sends a datagram consisting of multiple QUIC packets, it is (almost always) possible for a MOTS MITM to take one of the QUIC packets, coalesce that with a few octets, and forward that datagram. That forwarded datagram would be smaller than the original datagram.

Note also that a sender can send coalesced packet after the handshake too; see section 14.4.1 of the transport draft.

kazuho commented 3 years ago

As a step forward, I think we should do the following:

janaiyengar commented 3 years ago

@kazuho: I hate to make more changes, but I think your point is a good one. If a receiver does the enforcement that we added in #4188 -- of enforcing the datagram size -- that makes us vulnerable to this MOTS attack.

The newly added text in #4188 hasn't shipped yet in a draft, so if we can get agreement, I'd like to take this in before we ship the next draft.

martinthomson commented 3 years ago

A timely reminder that making changes always results in making more changes and no matter how careful or small, there are almost always problems. This seems like a good proposed tweak, but this has to stop.

gloinul commented 3 years ago

Let us discuss what purpose that "MAY discard" rule has and compare it to the issue. What is important for an endpoint is to ensure that its path to the peer is possible to forward packets and supporting at least minimal MTU to make QUIC work. I think that requirement lies on each endpoint to ensure that. Does the client need to enforce the server behavior for that verification? Is that a risk in itself that is greater, equal or less than the MOTS risk the enforcement has?

kazuho commented 3 years ago

@martinthomson @gloinul I totally agree that this has to stop. I'm sorry to have missed this point when working on #4188; I realized the problem when I was wondering about the MUST vs. SHOULD discussion in #4241.

That said, I do think that it would be a good idea to adopt the proposal in this issue.

Let me go through the facts first. They are:

Due to these facts, "MUST NOT close, can discard" is a principle that we've always had. The reason we hadn't written it down is because we used to have only one requirement regarding datagram length, that correctly follow the principle, stating that "servers MUST discard non-full-sized datagrams containing Initial packets."

But now that we have emerging consensus that datagrams carrying PATH_CHALLENGE / PATH_RESPONSE MUST be padded to full size (#4241), we need to write down that the receiver cannot enforce that.

Realizing that, I started to wonder if the "MAY close" rule that we added in #4188 has been correct.

As @janaiyengar points out, #4188 is a change that has not shipped yet. It's a recent change due to an issue discovered during this IETF last call. I do not think we have official consensus yet.

Therefore, I tend to think there would be less confusion if the next draft would contain:

rather than having two different of rules for handling small-sized datagrams, of which one does not follow the principle.

ianswett commented 3 years ago

Given we explicitly allow coalesced packets for PathMTU, I agree that closing the connection seems like the wrong thing to do. The PR looks sensible.

martinthomson commented 3 years ago

Agree with Ian. My plea for stopping was not about the fix (the fix here is good), but the original change. We would not have had this problem if we were able to accept that the protocol wasn't absolutely perfect.

larseggert commented 3 years ago

@kazuho has described a way forward on the mailing list for this issue - are we good with that? If so, let's merge, close and do the consensus establishment during IETF LC.