Padding outside QUIC packet

[tatsuhiro-t]

https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#section-14 only says that padding is done by PADDING frame. https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#section-14

But it looks like it is a common practice to add 0 or garbage outside QUIC packet to fill remaining UDP packet payload to expand packet size.
If it is a good practice (and/or recommended), I think draft should say something about it.
It might slightly simplify the implementation.
Padding in this way has different properties than PADDING frame; for example, it does not contribute to in-flight bytes.

[ekr]

The only time you can do this is when you have only long header packets (essentially during the handshake). Because short header packets are not length delimited, if you pad on the outside it will be interpreted as part of the packet and the deprotection will fail.

[hawkinsw]

https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#section-14 only says that padding is done by PADDING frame. https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#section-14

But it looks like it is a common practice to add 0 or garbage outside QUIC packet to fill remaining UDP packet payload to expand packet size.

Just FYI: Adding 0-value bytes outside QUIC packets to fill remaining UDP datagram is the way that neqo currently performs padding. Of course, as @ekr says, it only does this when it is possible (there are only QUIC packets with long headers in the datagram).

If it is a good practice (and/or recommended), I think draft should say something about it.
It might slightly simplify the implementation.
Padding in this way has different properties than PADDING frame; for example, it does not contribute to in-flight bytes.

I, too, think it would be nice to have some clarification like this in the standard.

[tatsuhiro-t]

Yes, this kind of padding is for handshake packets only.

[ianswett]

As @ekr pointed out, this type of padding doesn't allow coalescing 0.5RTT server data with the server's first flight, so it limits coalescing in common use cases, though I agree may be easier to implement.

To me, the biggest risk here is that the non-QUIC padding is removed by something on the path. However, that risk seems similar to the risk that something on the path would split a coalesced packet into two UDP datagrams(discussed in #3317), so it seems acceptable.

Because it prevents coalescing in some useful cases, I don't think it should be recommended, but I can't come up with a strong reason for disallowing it, so I think it should continue to be allowed.

[tatsuhiro-t]

Another concern about this way of padding is that it does not increase the transmission limit for server.

https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#section-8.1

Prior to validating the client address, servers MUST NOT send more than three times as many bytes as the number of bytes they have received. This limits the magnitude of any amplification attack that can be mounted using spoofed source addresses. In determining this limit, servers only count the size of successfully processed packets.

Because bytes outside of QUIC packet are most likely unprocessable, it is not counted to the data size that server has received. That means that server might get choked during handshake with packet losses.

[nibanks]

@tatsuhiro-t my understanding is that amplification limit is based on the UDP datagram layer, not the QUIC packet layer. If you receive any bytes from the peer you can see 3x times that in response. They don't have to be valid QUIC packets. I thought there was text that was explicit about that. If not, the text should probably be clarified.

[ekr]

This is my understanding as well

…

On Mon, Jan 13, 2020 at 7:51 AM Nick Banks ***@***.***> wrote: @tatsuhiro-t <https://github.com/tatsuhiro-t> my understanding is that amplification limit is based on the UDP datagram layer, not the QUIC packet layer. If you receive any bytes from the peer you can see 3x times that in response. They don't have to be valid QUIC packets. I thought there was text that was explicit about that. If not, the text should probably be clarified. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3333?email_source=notifications&email_token=AAIPLIOXMS6JZ2HAISYFFGDQ5SEYNA5CNFSM4KFV7MQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIZHBJI#issuecomment-573730981>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIPLILWO76VAV5Q32P4QATQ5SEYNANCNFSM4KFV7MQQ> .

[hawkinsw]

As @ekr pointed out, this type of padding doesn't allow coalescing 0.5RTT server data with the server's first flight, so it limits coalescing in common use cases, though I agree may be easier to implement.

A server is not required to pad, is it? https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#name-packet-size

(edited for typo.)

To me, the biggest risk here is that the non-QUIC padding is removed by something on the path. However, that risk seems similar to the risk that something on the path would split a coalesced packet into two UDP datagrams(discussed in #3317), so it seems acceptable.

Because it prevents coalescing in some useful cases, I don't think it should be recommended, but I can't come up with a strong reason for disallowing it, so I think it should continue to be allowed.

[ianswett]

No, padding is not required on the server side, though it's still sometimes useful for path MTU.

[kazuho]

This behavior should continue to be allowed. We allow endpoints to send a QUIC packet (as part of a coalesced packet) knowing that the peer would de unable to decrypt it (see section 14.3.1). The issue being discussed here is a variant of that.

That said, I am not sure if we want to suggest padding outside of a QUIC packet is a possibility, because such a design might have wired implications to the CC logic. IIUC, current congestion logic is based on the following principles:

• an ack-eliciting packet consumes bytes-in-flight
• an ack-only packet does not consume bytes-in-flight
• we allow ack-eliciting packets and ack-only packets to be coalesced, because that does not change the load on the network

I am not sure how we can adjust these principles so that padding outside QUIC packets can be taken into consideration (when necessary), or if making such adjustment is worth the additional complexity.

[tatsuhiro-t]

I think the current text "In determining this limit, servers only count the size of successfully processed packets." has 2 problems.

• It is unclear that what "packets" indicate: is it UDP packet or QUIC packet?
• It is unclear what "successfully processed packets" means.

[marten-seemann]

Because bytes outside of QUIC packet are most likely unprocessable, it is not counted to the data size that server has received. That means that server might get choked during handshake with packet losses.

@tatsuhiro-t raises an interesting point here. While it's certainly not impossible to use the size of the UDP datagram for the amplification limit, this would be quite hard to do in my implementation. Undecryptable packets are discarded way down in the stack, and passing up that information to the congestion controller would require quite a bit of extra work.

[kazuho]

It seems that the text regarding the amplification limit (Section 8.1) has been last changed in response to #1863 which is marked as editorial.

While the intension of the change was to clarify that the amplification limit applies at the QUIC packet level (rather than UDP datagram level), I think @nibanks might be correct in pointing out that our understanding have been the contrary (see #3333 (comment)).

Consider the case where a client is using 0-RTT. When building the first datagram consisting of an Initial and a 0-RTT packet, the only way the client can build the datagram in one-pass is by first building the Initial packet, then the 0-RTT packet due to the ordering requirement. As it is hard to tell the size of the 0-RTT packet before building it, the most likely outcome is that the 0-RTT packet would be padded. However, the server might not be able to process the 0-RTT packet, when it has lost it's resumption secret.

That means that if we are to say that the amplification limit is applied at the QUIC packet layer (for the QUIC packets that are only processed successfully), the client might end up in having very little room. I do not think that is a good outcome.

[ekr]

I concur with Kazuho. My understanding had always been that the amplification liit was on the UDP datagram, more or less for the reasons Kazuho lays out here.

…

On Mon, Jan 13, 2020 at 7:20 PM Kazuho Oku ***@***.***> wrote: It seems that the text regarding the amplification limit (Section 8.1 <https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#section-8.1-2>) has been last changed in response to #1863 <#1863> which is marked as editorial. While the intension of the change was to clarify that the amplification limit applies at the QUIC packet level (rather than UDP datagram level), I think @nibanks <https://github.com/nibanks> might be correct in pointing out that our understanding have been the contrary (see #3333 (comment) <#3333 (comment)> ). Consider the case where a client is using 0-RTT. When building the first datagram consisting of an Initial and a 0-RTT packet, the only way the client can build the datagram in one-pass is by first building the Initial packet, then the 0-RTT packet due to the ordering requirement <https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#section-12.2-3>. As it is hard to tell the size of the 0-RTT packet before building it, the most likely outcome is that the 0-RTT packet would be padded. However, the server might not be able to process the 0-RTT packet, when it has lost it's resumption secret. That means that if we are to say that the amplification limit is applied at the QUIC packet layer (for the QUIC packets that are only processed successfully), the client might end up in having very little room. I do not think that is a good outcome. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3333?email_source=notifications&email_token=AAIPLIMKHR6FXO4GKKJQILTQ5UVO7A5CNFSM4KFV7MQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEI3EBCQ#issuecomment-573980810>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIPLIKFTPEHZ7NZJLSXD4TQ5UVO7ANCNFSM4KFV7MQQ> .

[martinthomson]

The CC implications are interesting for sure, but though amplification should be based on bytes received from an address prior to processing (so that discarded 0-RTT and junk counts):

In determining this limit, servers only count the size of successfully processed packets.

So #3340.

[kazuho]

@martinthomson Thank you for opening a new issue. I share the view that the issue deserves its own.

Going back to the original topic, I was a bit confused in my previous comment, but I think that my argument still holds; we should recommend padding inside QUIC packet, because CC happens at QUIC packet layer, and the size of padding affects CC. I am afraid that the specification would become needlessly complex, if we are to suggest that padding outside of QUIC packet is a possibility.

[agrover]

I am afraid that the specification would become needlessly complex, if we are to suggest that padding outside of QUIC packet is a possibility.

Many other implementations didn't initially work against Neqo (which pads outside) and the Wireshark QUIC dissector does not currently handle it either. If the spec doesn't say anything then future implementations are bound to incorrectly handle outside padding. People are bound to assume it is not allowed if the spec says nothing, or just not consider the possibility of it.

[kazuho]

My point is that while I'm not against making an editorial change stating that a receiver should process coalesced packets of a datagram until it sees a broken packet, I would not prefer adding a statement that implies that a client might pad outside of QUIC packets, because doing so is might have negative effects, and trying to resolve those negative effects are likely to introduce complexity.

To be clear, my understanding is that this issue is about a misbehaving client failing to talk to some servers. The specification says that an endpoint MUST expand the packet to 1,200 bytes by "padding to packets in the datagram as necessary," (section 8.1.3) which explicitly means that padding should happen at the packet level, not at the datagram level.

We might even argue that some servers not handling those broken datagram is a benefit, as it helps us find bugs in the client.

[ekr]

Well, this is lawyering, but it doesn't say "by adding", but rather "adding...as necessary". My argument here is that it's not necessary if you pad in the datagram.

…

On Fri, Jan 17, 2020 at 3:36 AM Kazuho Oku ***@***.***> wrote: My point is that while I'm not against making an editorial change stating that a receiver should process coalesced packets of a datagram until it sees a broken packet, I would not prefer adding a statement that implies that a client might pad outside of QUIC packets, because doing so is might have negative effects, and trying to resolve those negative effects are likely to introduce complexity. To be clear, my understanding is that this issue is about a misbehaving client failing to talk to some servers. The specification says that an endpoint MUST expand the packet to 1,200 bytes by "padding to packets in the datagram as necessary," (section 8.1.3 <https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#section-8.1-3>) which explicitly means that padding should happen at the packet level, not at the datagram level. We might even argue that some servers not handling those broken datagram is a benefit, as it helps us find bugs in the client. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3333?email_source=notifications&email_token=AAIPLIOSM5LNHTZ4JC3RV73Q6GJ2PA5CNFSM4KFV7MQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJHNKCA#issuecomment-575591688>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIPLIP42EHA6Q3RX45QSHLQ6GJ2PANCNFSM4KFV7MQQ> .

[martinthomson]

I have to admit that I found Kazuho's points about congestion control somewhat compelling. However, I would say that there is nothing inherently wrong with the approach we take, except for the way that we count those junk bytes toward bytes-in-flight, and particularly how we remove them in the absence of a direct ACK. But ACKs in Initial packets are worthless anyway, so counting them as though they were Initial probably goes most of the way to fixing the problem.

Right now we don't account for these bytes at all, which avoids accounting errors, but might not be ideal in terms of congestion control. INIT_CWND 10 becomes INIT_CWND "10 and a bit".

[huitema]

I think it is much simpler to say that:

1. For the purpose of verifying that an Initial packet is larger than the required minimum, all bytes count.
2. For all other purposes, including congestion control and amplification control, only valid packets count.

Then it becomes a choice of implementations. Padding with padding frames is not hard, many implementations do it. Implementations that want to save a few lines of code or a few CPU cycles could pad their initial packets otherwise, but they will bear the impact on performance.

Also, consider the coalescing requirement about DCID: "Senders MUST NOT coalesce QUIC packets for different connections into a single UDP datagram. Receivers SHOULD ignore any subsequent packets with a different Destination Connection ID than the first packet in the datagram." That's consistent with "ignore the junk for all purposes except maybe verifying packet size".

[Lekensteyn]

Do note that appending zeroes in the UDP datagram makes it very easy to fingerprint QUIC connections for neqo and others that do the same.

Is the flexibility on using PADDING frames or appending garbage necessary to achieve the minimum datagram size necessary? It does not feel entirely right.

[ekr]

"
For the purpose of verifying that an Initial packet is larger than the required minimum, all bytes count.
For all other purposes, including congestion control and amplification control, only valid packets count.
"

I don't agree that these bytes shouldn't count for amplification control. Amplification control is about received byes, and these are bytes.

[marten-seemann]

I don't agree that these bytes shouldn't count for amplification control. Amplification control is about received byes, and these are bytes.

Does that mean random garbage (that's not appended to a QUIC packet) is supposed to count as well? That would require implementations to do 5-tuple tracking.

[RyanTheOptimist]

On Mon, Feb 3, 2020 at 6:18 AM Marten Seemann ***@***.***> wrote: I don't agree that these bytes shouldn't count for amplification control. Amplification control is about received byes, and these are bytes. Does that mean random garbage (that's not appended to a QUIC packet) is supposed to count as well? That would require implementations to do 5-tuple tracking.

That doesn't seem to follow necessarily. If a UDP datagram contains a valid QUIC packet, count the bytes of the datagram. Otherwise, drop the datagram.

…

[kazuho]

IIRC we discussed and agreed that amplification control happens an the UDP datagram level. The rational was that unless we do so, clients using 0-RTT by coalescing Initial and 0-RTT packets into a single datagram would see inferior performance.

[igorlord]

Whatever the language about outside padding, an endpoint must not discard the entire datagram, just because data after a long-header packet looks unparseable to it. It is possible that the client is trying a 0-RTT resumption with packet coalescing using a QUIC version/extension that is unknown to the server. The right behavior is to process packets that are understood (which may result in version negotiation) and stop processing that datagram.

[huitema]

This "unparseable PAD" looks like a "kindness of strangers" issue. An implementer may want to do that because it looks like it will save 3 CPU cycles somewhere, but that only works if all other implementation agree that it is a good idea. Experience shows that they don't. So just accept that this will just not work well.

[martinthomson]

"For the purposes of avoiding amplification prior to address validation, servers MUST count the bytes received in datagrams that are uniquely attributed to a connection. This includes datagrams that contain packets that are successfully processed and datagrams that contain packets that are entirely discarded."

[igorlord]

@martinthomson "that are uniquely attributable to a connection" is vague and will differ by implementation (for example, one implementation will not even try to attribute a packet with an invalid "SCID Len" but another will).

How about: "... well-formed packets that are uniquely attributable to a connection" ?

[larseggert]

Discussed in ZRH. Proposed resolution is @martinthomson's quoted comment above.

[ekr]

MT "packets that are discarded, including when those packets are the sole packets in the datagram"

[mikkelfj]

I don't like the idea of allowing any kind of data in the datagram outside of the QUIC framework. Even for coalesced packets, they are required to belong to the same connection, possibly even the same packet number space.

Additional information in the datagram can be removed or fingerprinted, as already mentioned, but worse, information can be added. This could be used to leak cooperate data, tracking etc. - preventing this requires dropping all datagrams that cannot be fully decrypted, but don't I think there is consensus for that.