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

Client validation #49

Merged
merged 6 commits into from Jul 26, 2021
Merged

Conversation

martinthomson
Copy link
Member

In light of discussion in slack, the problem here is that the validation
is based on the negotiated version. Validation needs to be based on
the client's selected version.

I've also moved text about validating lack of support for the original
version to the incompatible VN section, alongside a reminder to validate
connection IDs. Security does depend on this, but the client ignores
these bad VN packets. That provides more effective defense against
attack than what was here originally.

In light of discussion in slack, the problem here is that the validation
is based on the *negotiated* version.  Validation needs to be based on
the client's *selected* version.

I've also moved text about validating lack of support for the original
version to the incompatible VN section, alongside a reminder to validate
connection IDs.  Security does depend on this, but the client ignores
these bad VN packets.  That provides more effective defense against
attack than what was proposed.
Copy link
Contributor

@DavidSchinazi DavidSchinazi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for writing this up!

Also, can you please add Anthony Rossi to the Acknowledgements since he pointed out this issue?

@@ -28,6 +28,9 @@ author:
organization: Mozilla
email: ekr@rtfm.com

normative:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, does a syntax like {{Section 6 of !RFC8999}} not work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It didn't when I last touched that code.

draft-ietf-quic-version-negotiation.md Outdated Show resolved Hide resolved
draft-ietf-quic-version-negotiation.md Outdated Show resolved Hide resolved
`VERSION_NEGOTIATION_ERROR`. This connection closure prevents an attacker from
being able to use forged Version Negotiation packets to force a version
downgrade.
The client MUST validate the server `Other Versions` field by confirming that it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requirement only applies to clients that have reacted to a VN packet right? What validation MUST a client do if this connection didn't involve a VN packet? (in other words, why drop the "If a client has reacted to a Version Negotiation packet," part?)

Copy link

@anrossi anrossi Jul 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why shouldn't the client do the same validation during compatible version negotiation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are ways in which the two are different, so I think David is right here.

Let's say that there are two groups of versions, the versions in each group compatible with each other, but incompatible with the versions in the other group. The client prefers a version in the first group, but the client also knows that the versions in the first group are not widely implemented. As the client doesn't like the added latency of version negotiation, it chooses the second group, which contains a version that is totally OK. However, if it gets a Version Negotiation packet as a result, it will happily switch to the first group.

So yeah, let's go with the "if VN" condition.

martinthomson and others added 2 commits July 19, 2021 08:40
Co-authored-by: David Schinazi <DavidSchinazi@users.noreply.github.com>
Copy link
Contributor

@DavidSchinazi DavidSchinazi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Approved modulo two nits

draft-ietf-quic-version-negotiation.md Outdated Show resolved Hide resolved
draft-ietf-quic-version-negotiation.md Outdated Show resolved Hide resolved
Co-authored-by: David Schinazi <DavidSchinazi@users.noreply.github.com>
@DavidSchinazi DavidSchinazi merged commit f369534 into quicwg:main Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants