Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Document request forgery #3996
Document request forgery #3996
Changes from all commits
a66b21e
951da28
e80ca89
d273e12
aaf51ff
3e64135
718b774
5e7f92e
dfc8827
4252d1c
1cfbb7f
a587bf0
7adbc31
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a paragraph here that provides examples of real harm? What protocols are both UDP-based and non-authenticated in a way that would make them vulnerable to a request forgery from a QUIC stack?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have anything concrete. Any example would be hard to describe with anything other than "this person/organization did something really unwise". Given the absence of a news-worthy example that would be easy to reference, this is really just a request to search for someone to shame.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not trying to shame anyone. I'm trying to ascertain whether this change is the right one: we're worsening the performance of the protocol (by requiring extra round-trips in some scenarios) and if the attack is purely theoretical then we shouldn't do that. Having a concrete example of actual harm would help motivate the performance degradation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only place this adds a round trip is for a migration to a preferred address, but that only delays migration, it doesn't prevent the connection from being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Message Send Service (RFC 1312) seems like a potential target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe that this is correct as written if there is a Retry because the server gets to control the next DCID. I don't think that this is relevant because you would need a round trip to make it work (I think!) but...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mentioned in the last paragraph of this section. Perhaps we just need to reference that here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, the NEW_TOKEN frame is not received from a server in an Initial packet, correct? The first few times I read this, I though that was what you intended.
Assuming I'm now reading this correctly, this SHOULD NOT would seem to be very restrictive for domains which use DNS to load balance traffic. Previously, I assumed that NEW_TOKEN would be used whenever hostnames matched. Am I understanding correctly?
If we're going to add this normative restriction, I would suggest placing the restriction in the section on address validation/etc and then referencing this section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have text that explicitly mandates that replies to a RETRY packet MUST be sent to the same address?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if that is enough, as this is not directly a requirement, but more of an assumption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing this normative requirement in Security Considerations makes me uneasy as an implementer might miss it. Could we instead move the
MUST NOT
to the section that defines preferred_address and then only reference it here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that repetition will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do find it somewhat ironic that address validation might make this worse: the client sending a PATH_CHALLENGE to the server can pretty much guarantee that the server will send a PATH_RESPONSE with client-provided content to the client's spoofed source address. Should we mention that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered it, but thought that the generic text was enough. Note also that there is no guarantee that PATH_RESPONSE is sent on the same path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's nice, but in many scenarios any of the node addresses can be used to reach services local to the node, in the same way as loopback addresses. In server farm scenarios, this requires guessing the local IP of the specific server, but that can probably be done. Same for clients behind NAT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@huitema This is certainly a way to mount an attack on services that assume that they are serving friendly clients and the network perimeter is secure. It happens, especially when services listen only on RFC1918/unique-local or link-local IPs. It happens even more often with services only listen to loopback and, therefore, think they are immune to any threats coming from outside of the host.
That's why the following is generalizing "non-loopback -> loopback" to “globally routable IP space” -> “RFC1918/unique-local IP space” -> “link-local” -> “loopback”.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is indeed a valid scenario for client and server discovering that they are behind the same NAT. Wasn't that scenario addressed in Web-RTC using a discovery protocol involving temporary names, in an attempt at privacy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're referring to draft-ietf-rtcweb-mdns-ice-candidates. For us to do something like that we'd need a way to convey hostnames in preferred_address or elsewhere, and I'm not sure we should be doing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with David. I don't see any way that the MDNS thing would make sense except in a very narrow domain (which WebRTC is). Nothing stopping someone from defining an extension, of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This depends on what you mean by efficient. Some of the recommendations in this section will add a round-trip which is not efficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must be very confused, but your round trip point seems wrong. The key point here is that the round trip is not safe, so any time you attempt that you are exposing services to a potential attack. The point of this text is to describe heuristics that might block attacks without sending a packet at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's definitely possible that I'm the one confused. In my understanding, we have two ways of solving the "QUIC endpoint is sending attacker-controlled data to unvalidated address" issue:
I do agree with you that we want (1) instead of (2), but saying that (1) is more efficient than (2) isn't necessarily true if you measure efficiency as QUIC performance, because (1) can add a round trip but (2) doesn't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first requires that the endpoint decide before it sends anything. It can't add a round trip, because that round trip would enable the attack.