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.

Sign up for GitHub

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

Jump to bottom

CONNECTION_CLOSE for unpadded Initial #3269

Closed
martinthomson opened this issue Nov 20, 2019 · 4 comments · Fixed by #3292
Closed

CONNECTION_CLOSE for unpadded Initial #3269

martinthomson opened this issue Nov 20, 2019 · 4 comments · Fixed by #3292
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
Late Stage Processing

Comments

@martinthomson
Copy link
Member

The draft requires that if an Initial packet is not padded, the server sends a CONNECTION_CLOSE. I think that the goal was to have the server drop those packets and optionally send a signal. But that only applies if the server hasn't established state. We should reword that.

@nibanks says:

I'm more worried about this as an attack vector. Now that we support multiple initial packets for the client initial, there is more of a windows for an attacker to race a bad initial packet. Sending a 1 byte initial packet to kill your connection would be pretty easy; especially since the size validation check is going to happen pre-decryption, so the attacker doesn't even need to craft a well-formed packet.

Originally posted by @nibanks in #3262
@martinthomson martinthomson added -transport editorial An issue that does not affect the design of the protocol; does not require consensus. labels Nov 20, 2019
@martinthomson
Copy link
Member Author

Note that our principles here mean that we aren't obligated to defend against this style of attack, but rewording here is probably a good idea.

@nibanks
Copy link
Member

nibanks commented Nov 20, 2019

I agree that we've decided not to try to protect against attacks in the first flight, but this seems open up an extremely trivial attack vector that we can easily shutdown. I believe we should prevent it.

@marten-seemann
Copy link
Contributor

The draft requires that if an Initial packet is not padded, the server sends a CONNECTION_CLOSE.

Why would a server want to send a CONNECTION_CLOSE in the first place? I think dropping the packet is a perfectly valid thing to do, and in fact is preferable at such an early state in the connection.

@mnot mnot added this to Editorial Issues in Late Stage Processing Nov 25, 2019
martinthomson added a commit that referenced this issue Dec 9, 2019
@martinthomson
Curtail CONNECTION_CLOSE for small Initial
6f9f8d7 
This is trickier than I had imagined.  Sending CONNECTION_CLOSE is
probably fine, but it's harder to do this correctly now.  You can't just
send an unauthenticated CONNECTION_CLOSE because that might disrupt a
real connection.  So there are two goals in tension:

1. Don't kill an active connection (attempt) unnecessarily.

2. Provide feedback about errors.

The observation is that an attacker can disrupt connections by eliciting
a CONNECTION_CLOSE, so feedback naturally leads to an exposure to a DoS
attack.  That's unfortunate, but we have established that we don't care
about DoS by an on-path attacker prior to handshake completion.
Anything we do here has got to be best effort.

DoS prevention would say that you just discard junk, and that is
probably the right answer.  But we have a number of cases where the
robustness of the system depends on getting feedback.

Either way, we agreed to allow CONNECTION_CLOSE in Initial, so the
exposure exists anyway.  So this contains advice.  Maybe too much
advice, but I thought that I'd see what people thought.

Closes #3269.
@martinthomson martinthomson mentioned this issue Dec 9, 2019
Merged
@martinthomson martinthomson removed the editorial An issue that does not affect the design of the protocol; does not require consensus. label Dec 10, 2019
@martinthomson
Copy link
Member Author

I'm going to request that this go for a consensus call. It's borderline, but I think that it is worth running the process here. And it isn't critical that this get fixed right now.

@martinthomson martinthomson mentioned this issue Dec 11, 2019
Closed
@martinthomson martinthomson moved this from Editorial Issues to Triage in Late Stage Processing Dec 11, 2019
@mnot mnot added the design An issue that affects the design of the protocol; resolution requires consensus. label Jan 6, 2020
@mnot mnot moved this from Triage to Design Issues in Late Stage Processing Jan 6, 2020
@martinthomson martinthomson added the proposal-ready An issue which has a proposal that is believed to be ready for a consensus call. label Jan 14, 2020
@mnot mnot moved this from Design Issues to Consensus Emerging in Late Stage Processing Jan 17, 2020
@mnot mnot removed the proposal-ready An issue which has a proposal that is believed to be ready for a consensus call. label Jan 24, 2020
@mnot mnot moved this from Consensus Emerging to Consensus Call issued in Late Stage Processing Jan 24, 2020
@mnot mnot added the has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. label Feb 5, 2020
@project-bot project-bot bot moved this from Consensus Call issued to Consensus Declared in Late Stage Processing Feb 5, 2020
Late Stage Processing automation moved this from Consensus Declared to Text Incorporated Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
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
Late Stage Processing
  
Issue Handled
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Define stateless CONNECTION_CLOSE quicwg/base-drafts
4 participants
@martinthomson @mnot @marten-seemann @nibanks