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

Request Forgery Attacks through Version Negotiation #4258

Closed
huitema opened this issue Oct 21, 2020 · 2 comments · Fixed by #4339
Closed

Request Forgery Attacks through Version Negotiation #4258

huitema opened this issue Oct 21, 2020 · 2 comments · Fixed by #4339
Labels
-transport editorial An issue that does not affect the design of the protocol; does not require consensus. ietf-lc An issue that was raised during IETF Last Call.

Comments

@huitema
Copy link
Contributor

huitema commented Oct 21, 2020

The version negotiation packet is described in the QUIC Invariant specification as:

Version Negotiation Packet {
  Header Form (1) = 1,
  Unused (7),
  Version (32) = 0,
  Destination Connection ID Length (8),
  Destination Connection ID (0..2040),
  Source Connection ID Length (8),
  Source Connection ID (0..2040),
  Supported Version (32) ...,
}

The two connection ID fields, of length up to 255 bytes, contain data directly copied from an incoming packet that is not authenticated or verified in any way. Aren't we worried that this is a great avenue for request forgery attacks? Should that be documented in the security section of the invariant draft?

@larseggert larseggert added -transport ietf-lc An issue that was raised during IETF Last Call. labels Oct 22, 2020
@larseggert larseggert added this to Triage in Late Stage Processing via automation Oct 22, 2020
martinthomson added a commit that referenced this issue Nov 5, 2020
It's pretty bad.

Personally, I'm still comfortable with it.  Server deployments are far
less likely to made without due consideration to the environment into
which they find themselves.

Closes #4258.
@martinthomson
Copy link
Member

@marten-seemann, trying to move discussion here.

You suggest that in a p2p deployment, the ability to control deployment of "server" instances is limited.

My response is that in general you need some sort of signaling between peers to communicate addresses for p2p. NAT traversal generally requires it, and - if nothing else - you usually need to ship IP addresses for peers around. Given that, I think that it is reasonable to include - in that signaling - information about supported protocols such that you should never need to generate a QUIC Version Negotiation in that context.

Maybe you don't care for that, but I think that it is probably better than accepting the exposure.

If you have an idea for how we might avoid this particular sort of request forgery without that, then I'm open to suggestions. Right now, the proposal is that servers accept the risk and manage it. But if there is a protocol change that would improve the situation, that might be better. Assuming that it is minimally disruptive, of course.

@marten-seemann
Copy link
Contributor

My response is that in general you need some sort of signaling between peers to communicate addresses for p2p. NAT traversal generally requires it, and - if nothing else - you usually need to ship IP addresses for peers around.

This independent of NAT traversal then. You'd also need to advertise addresses in a world where NATs wouldn't exist, and you could include QUIC version information in those address advertisements.

This still leaves the Bootstrap nodes susceptible to this attack (assuming you only hard-code a list of IP:ports without QUIC version information), but maybe that's ok, since you typically control those nodes yourself, or at least you can ask people running those nodes to take precautionary steps.

@janaiyengar janaiyengar added the proposal-ready An issue which has a proposal that is believed to be ready for a consensus call. label Nov 10, 2020
@project-bot project-bot bot moved this from Triage to Consensus Emerging in Late Stage Processing Nov 10, 2020
@martinthomson martinthomson added the editorial An issue that does not affect the design of the protocol; does not require consensus. label Dec 8, 2020
@project-bot project-bot bot moved this from Consensus Emerging to Editorial Issues in Late Stage Processing Dec 8, 2020
@LPardue LPardue moved this from Editorial Issues to Consensus Emerging in Late Stage Processing Dec 8, 2020
Late Stage Processing automation moved this from Consensus Emerging to Issue Handled Dec 8, 2020
@LPardue LPardue removed the proposal-ready An issue which has a proposal that is believed to be ready for a consensus call. label Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport editorial An issue that does not affect the design of the protocol; does not require consensus. ietf-lc An issue that was raised during IETF Last Call.
Projects
Late Stage Processing
  
Issue Handled
Development

Successfully merging a pull request may close this issue.

6 participants