Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ICMP DOS #2063

Closed
mikkelfj opened this issue Nov 28, 2018 · 11 comments
Closed

ICMP DOS #2063

mikkelfj opened this issue Nov 28, 2018 · 11 comments
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.

Comments

@mikkelfj
Copy link
Contributor

mikkelfj commented Nov 28, 2018

I read through the recent PR #2036 which cleans up the packet size discussion. The text does touch upon ICMP denial of service attacks but I'm afraid the protection is too fragile because it is required that transmission stops on a path where PMTU is suspected to have been decreased. It is not explicitly stated (as far as I can tell) that ICMP packet too big is used for this decision, but it is used in PMTU discovery.

There is no major problem with attacks on discovery, but if ICMP can later close a connection, that is a big problem.

There is protection against off path attackers, but it is easy for a man on the side attack to generate ICMP messages that would pass validation. This only requires one compromised machine on a LAN.

This issue also relates to the ongoing discussion on protection against the initial handshake by man on the side stopping handshake by generating invalid early handshake packets, only here it can happen long after the connection is established.

A possible mitigation could be that a path should not be dropped based in ICMP messages alone but only after one or two new new PMTU probes. However, these new probes could also be vulnerable to forged ICMP messages.

@martinthomson martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -transport labels Nov 28, 2018
@mikkelfj
Copy link
Contributor Author

A possible resolution is to never allow ICMP to reduce a path below 1200 bytes or so, corresponding to the handshake discovery. Dropping a path should then solely be based on a) having a better path to switch to, or b) that packets loss rate is getting too high for the application to tolerate.

@martinthomson
Copy link
Member

Not allowing ICMP to reduce the PMTU below 1280 was always the intent. I can't see that anywhere, so we should add it.

martinthomson added a commit that referenced this issue Nov 29, 2018
The loss of packets on the path will be the signal that we use here.

Closes #2063.
martinthomson added a commit that referenced this issue Nov 29, 2018
The loss of packets on the path will be the signal that we use here.

Closes #2063.
@ekr
Copy link
Collaborator

ekr commented Dec 13, 2018

I just caught this and I don't think that mandating it like this is appropriate. There are other ways to get a lower MTU (e.g., your interface MTU shrinks). I'm happy to have it forbid lowering in response to ICMP, but this is too broad.

@ekr ekr reopened this Dec 13, 2018
@martinthomson
Copy link
Member

I disagree. The fix was to specifically a prohibition against accepting an ICMP PTB. We also had consensus not to support QUIC on paths that had a lower PMTU, but we don't have any text suggesting others.

@ekr
Copy link
Collaborator

ekr commented Dec 14, 2018

I agree we had consensus not to support QUIC on paths with a lower MTU, but the problem is the over-specific requirement that I do something about it.

@martinthomson
Copy link
Member

What are you being asked to do? The only requirement in text is that you ignore ICMP PTB with a size of <1280.

@ekr
Copy link
Collaborator

ekr commented Dec 14, 2018

the relevant text is in 14.1.

   If a QUIC endpoint determines that the PMTU between any pair of local
   and remote IP addresses has fallen below the size needed to support
   the smallest allowed maximum packet size, it MUST immediately cease
   sending QUIC packets on the affected path.  An endpoint MAY terminate
   the connection if an alternative path cannot be found.

This would apply to other methods than ICMP PTB, for instance, Ethernet or VPN MTU reduction

@martinthomson
Copy link
Member

Yep. We had consensus for that text.

@ekr
Copy link
Collaborator

ekr commented Dec 14, 2018

Can you please point me to where that consensus call was taken?

@martinthomson
Copy link
Member

This text is in -02, and the chairs made a call here: https://mailarchive.ietf.org/arch/msg/quic/ZGN6z-5oYnEXRGafi_zvqKjDcnM

That lists "#139 - Minimum MTU"

@ekr
Copy link
Collaborator

ekr commented Dec 14, 2018

I don't really read the thread that way, but It's not worth fighting over.

@ekr ekr closed this as completed Dec 14, 2018
@mnot mnot added the has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. label Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.
Projects
None yet
Development

No branches or pull requests

4 participants