-
Notifications
You must be signed in to change notification settings - Fork 205
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
Move negotiated_version #959
Conversation
This makes it possible for a client to use substantially-similar versions of QUIC without having to regenerate a ClientHello. That's not a huge benefit, but given that it is no more vulnerable to downgrade attack than before, it seems like a worthwhile thing to do. Note that this will have the effect of violating the terms of the version downgrade contract. I do this knowing that this is still very much in development and that no sane person would deploy this protocol in its current form expecting it to be secure. Conveniently, it appears that endpoints that support the newer version will not be exposed to downgrade, only those that support versions prior to the change (and even then, it's only if they can cause the server to emit a transport parameter that looks like the current version. That is possible, but for QUIC version 4 only (assuming we retain the same basic format for that long). Closes #710.
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 basically what GQUIC does. Christian should take a look, but LGTM modulo one nit.
draft-ietf-quic-transport.md
Outdated
version is not listed in the supported_versions list. A client MUST terminate | ||
with a VERSION_NEGOTIATION_ERROR error code if version negotiation occurred but | ||
it would have selected a different version if the versions in the | ||
supported_versions list appears in a Version Negotiation packet. |
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 a run-on. The old sentence reads fine -- leave it as it is.
draft-ietf-quic-transport.md
Outdated
The client includes the initial_version field in its transport parameters. The | ||
initial_version is the version that the client initially attempted to use. If | ||
the server did not send a version negotiation packet {{packet-version}}, this | ||
will be identical to the negotiated_version. |
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 the negotiated_version in the encrypted_extensions.
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 allowed to use TLS message names, but I'll use "server transport parameters".
|
||
The negotiated_version field is the version that is in use. This MUST be set by | ||
the server to the value that is on the Initial packet that it accepts (not an | ||
Initial packet that triggers a Retry or Version Negotiation packet). A client |
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.
Why the Retry packet?
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.
Both are (potentially) stateless from the server perspective. You could save the packet number in the Retry cookie, but I don't think we need to require that servers do that. Note that the size of this cookie is strictly limited. It has to fit into the padding space in the Initial packet.
@huitema, can you review this PR please? |
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.
Looks good enough, but the text in section 7.4.4 could be improved.
QuicVersion initial_version; | ||
|
||
case encrypted_extensions: | ||
QuicVersion negotiated_version; | ||
QuicVersion supported_versions<4..2^8-4>; | ||
|
||
case new_session_ticket: |
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.
LGTM.
version is not listed in the supported_versions list. A client MUST terminate | ||
with a VERSION_NEGOTIATION_ERROR error code if version negotiation occurred but | ||
it would have selected a different version based on the value of the | ||
supported_versions list. |
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 OK, but I had to read the text several times, so it probably should be clearer. How about something like that:
The whole point is to prevent version downgrade by a third party, by locking the version ID inside TLS extensions protected by the TLS protocol. The two types of downgrade attacks are:
- MITM spoofing the client initial, replacing the original value by a downgrade value;
- MITM dropping the client initial and sending a spoofed version negotiation packet that proposes the downgrade value, or alternately MITM intercepting a genuine negotiation packet and reducing the proposed list to a downgrade value.
Both types will be detected. In the first case, the server will see the initial version in the transport parameters sent by the client, and detect that it should have been accepted. In the second case, the client will see that the transport parameters of the server include a version that it would have preferred over the downgrade version.
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.
Still LGTM.
This makes it possible for a client to use substantially-similar versions of
QUIC without having to regenerate a ClientHello. That's not a huge benefit,
but given that it is no more vulnerable to downgrade attack than before, it
seems like a worthwhile thing to do.
Note that this will have the effect of violating the terms of the version
downgrade contract. I do this knowing that this is still very much in
development and that no sane person would deploy this protocol in its current
form expecting it to be secure. Conveniently, it appears that endpoints that
support the newer version will not be exposed to downgrade, only those that
support versions prior to the change (and even then, it's only if they can
cause the server to emit a transport parameter that looks like the current
version. That is possible, but for QUIC version 4 only (assuming we retain the
same basic format for that long).
Closes #710.