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

Fixups for 9.2 #615

Closed
wants to merge 9 commits into from
Closed

Fixups for 9.2 #615

wants to merge 9 commits into from

Conversation

martinthomson
Copy link
Collaborator

See commit log for details on changes.

@@ -3600,21 +3600,26 @@ HTTP2-Settings = token68
<x:ref>INADEQUATE_SECURITY</x:ref>.
</t>

<section anchor="TLSFeatures" title="TLS Features">
<t>
The restrictions on TLS usage in the following sections apply to use of TLS 1.2.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/to use/to the use/ ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

potato, potahto. Either is OK, but the "the" is probably a more precise form. I'll fix it.

</t>
<t>
HTTP/2 implementations MUST NOT offer or select cipher suites that have unknown
properties with respect to these properties. This avoids situations where peers
Copy link
Member

Choose a reason for hiding this comment

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

"unknown properties with respect to these properties"?

Suggest:

...that are not known to conform to these requirements.

alternative protocol, such as HTTP/1.1. Note that this entails a risk of downgrade attack
if the client does not properly validate that the TLS handshake has been completed; this
is especially likely if clients support <xref target="TLS-FALSE-START">TLS false
start</xref>.

Choose a reason for hiding this comment

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

Clients do not process received application data until the handshake has been completed, even with False Start, so the "Note that" sentence is unnecessary. An HTTP-level INADEQUATE_SECURITY is actually more secure than a TLS-level alert that cancels the handshake w.r.t. downgrade prevention.

Choose a reason for hiding this comment

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

OTOH, if the client is the one generating the INADEQUATE_SECURITY and then falling back (which I think no clients should do), then it MUST NOT send the INADEQUATE_SECURITY message until the handshake has completed. That is, don't false start if you detect INADEQUATE_SECURITY.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That second point is the one I wanted to capture. In most cases I'm aware of, it's the client who needs to send the INADEQUATE_SECURITY. It's a pretty screwed up case if the server is generating it.

Well, there's nothing wrong with generating the INADEQUATE_SECURITY, it's the falling back that's the issue here.

Choose a reason for hiding this comment

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

Good point. There are actually THREE different things to consider: How the INADEQUATE_SECURITY condition is detected, how INADEQUATE_SECURITY is sent, and whether fallback is done. I agree that false start isn't a factor for when it is safe to send INADEQUATE_SECURITY. However, it is a factor in the detection step, which precedes the sending step. That is, you don't want to detect/save the inadequate security bit for a server unless/until the full handshake has completed; otherwise, you would use that saved bit during your next (fallback) attempt.

To put it more concretely, in the case of NSS, it is OK to save the INADEQUATE_SECURITY bit for a server in the handshake callback, but not in the false start callback or the protocol negotiation callback. OTOH, hopefully you would never get to the handshake callback in the first place, because you really should have killed the connection earlier.

The side-effect of this is that, if one wants to do fallback, one MUST complete the handshake when inadaquate security is detected, instead of terminating the TLS handshake earlier, even though terminating the TLS handshake earlier is the superior thing to do otherwise. That's another reason why explicitly allowing the fallback to HTTP/1.1 here is bad.

non-intersecting sets of available cipher suites, since these prevent the use of the
cipher suite that TLS 1.2 makes mandatory. To avoid this problem, implementations of
non-intersecting sets of available cipher suites, since these restrictions prevent the
use of the mandatory TLS 1.2 cipher suite. To avoid this problem, implementations of
HTTP/2 that use TLS 1.2 MUST support TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 <xref
target="TLS-ECDHE"/> with P256 <xref target="FIPS186"/>.
</t>
<t>
Clients MAY advertise support of cipher suites that are prohibited by the above
restrictions in order to allow for connection to servers that do not support HTTP/2.
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think we should offer this style of fallback. It is this line that makes the handshake fragile. This PR now provides a mechanism for fallback in the line 3657. We should not bend over backwards to support weak ciphers - specially if they make the handshake fragile

@martinthomson
Copy link
Collaborator Author

Superceded by #644.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants