1112399 - Detect TLS version intolerant servers

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[Masatoshi Kimura [:emk]]

PR_CONNECT_RESET_ERROR and PR_END_OF_FILE_ERROR are too vague for average users to know what's going on. We should detect TLS version intolerant servers and show appropriate message somehow.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: patch

I'm (ab)using cert verification callback again, but this time:
- This patch does not rely on the racy SSL_GetChannelInfo() (or any other NSS functions).
- We don't have to worry about MitM tampering the message because this patch won't trigger further fallbacks. Rather, this patch will stop fallbacks and warn about potential attacks.
- IMO we shouldn't modify NSS for this because NSS does not have the concept of the "non-secure fallback". It should be closed under PSM.

Brian, is this acceptable?

Comment 2

[Yuhong Bao]

This is also an issue with the disabling of SSLv3, especially when the real cause is TLS extensions intolerance for example.

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: Detect TLS version intolerant servers, v2

Ah, I forgot to deal with sync cert verification. Since we don't have to wait for the cert verification result, I just moved the check to the top of the callback.

Comment 4

[Masatoshi Kimura [:emk]]

Attachment: Detect TLS version intolerant servers, v3

Do not overwrite SSL errors.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8539653 [details] [diff] [review]
Detect TLS version intolerant servers, v3

Review of attachment 8539653 [details] [diff] [review]:
-----------------------------------------------------------------

Masatoshi-san, I'm not understanding what you're trying to do with this patch. In particular, why are we dealing with this in the auth certificate hook? When the server closes or resets the connection due to TLS intolerance, that will happen way before we'd ever call the auth certificate hook. If the auth certificate hook is called, that means that the server responded with a ServerHello+Certificate+...+ServerHelloDone, which means it is NOT intolerant at all. Please add some comments to the patch to indicate what this new code is doing, and the logic behind its design.

::: security/manager/locales/en-US/chrome/pipnss/nsserrors.properties
@@ +314,5 @@
>  MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE=The server uses key pinning (HPKP) but no trusted certificate chain could be constructed that matches the pinset. Key pinning violations cannot be overridden.
>  MOZILLA_PKIX_ERROR_CA_CERT_USED_AS_END_ENTITY=The server uses a certificate with a basic constraints extension identifying it as a certificate authority. For a properly-issued certificate, this should not be the case.
>  MOZILLA_PKIX_ERROR_INADEQUATE_KEY_SIZE=The server presented a certificate with a key size that is too small to establish a secure connection.
>  MOZILLA_PKIX_ERROR_V1_CERT_USED_AS_CA=An X.509 version 1 certificate that is not a trust anchor was used to issue the server's certificate. X.509 version 1 certificates are deprecated and should not be used to sign other certificates.
> +PSM_ERROR_FALLBACK_LIMIT_REACHED=The server failed to negotiate the protocol version securely or the connection failed due to network glitches. Since these are indistinguishable from downgrading attacks, the connection attempt has been terminated.

I've got no opinion on the wording here.

Comment 6

[Masatoshi Kimura [:emk]]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #5)
> Masatoshi-san, I'm not understanding what you're trying to do with this
> patch. In particular, why are we dealing with this in the auth certificate
> hook? When the server closes or resets the connection due to TLS
> intolerance, that will happen way before we'd ever call the auth certificate
> hook.

This code is trying to detect the server is intolerant at version N, but is tolerant at version N-1. And if N-1 is lower than the fallback limit, it will terminate the handshake to prevent downgrading attacks.

> If the auth certificate hook is called, that means that the server
> responded with a ServerHello+Certificate+...+ServerHelloDone, which means it
> is NOT intolerant at all. Please add some comments to the patch to indicate
> what this new code is doing, and the logic behind its design.

Yes, the certificate hook is used to prove that the server is tolerant at version N-1.
I'll add more comments to clarify that.

Comment 7

[Masatoshi Kimura [:emk]]

(In reply to Masatoshi Kimura [:emk] from comment #6)
> is tolerant at version N-1

This was not accurate. More precisely, "is tolerant at any version lower than N".

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: Detect TLS version intolerant servers, v4

Added some comments.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8540469 [details] [diff] [review]
Detect TLS version intolerant servers, v4

Review of attachment 8540469 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay. Thanks for adding the comments, and thanks for your explanation here in the bug. I think I understand what your patch does: When we try to connect, we're going to first try TLS 1.2. If that connection fails, and we've disable version fallback, then go ahead and try version fallback anyway. But, if that version fallback seems to succeed, then cancel that handshake and report a special version intolerance error message.

I din't think we should be making the SSL handshake state machine even more complicated this way, just to show a better error message and to collect more accurate telemetry. I'd like to suggest a simpler alternative approach:

In the logic (in nsDocShell.cpp, IIRC, and maybe other places) that shows network error messages, if the error message is for the connection reset or connection close error codes, and if the URL was "https://", then just change the error message text to your text that adds the bit about TLS intolerance bing a possibility, without doing a connection test. (Note that the connection test would be inconclusive anyway, as your proposed error message text already indicates.) Also, if a connection closed/reset error message occurs for an HTTPS page, then make sure the SSL error reporting UI is shown for it, so that Mozilla can collect a list of websites to test for TLS intolerance, as the next step in avoiding version intolerance errors. Finally, make sure that the support.mozilla.org documentation for the connection reset and connection closed errors explains TLS intolerance and links to documentation on how to fix it on the server.

This way, we ensure that we don't add any security bugs into the already-complicated handshaking logic. Also, there would not be a new error code that would hide the original cause of the failed load (connection reset vs. connection closed is an important diagnostic detail). Finally, such a simpler approach would give more time to work on things like bug 1114816 and other things that will minimize the number of times users encounter version intolerance fallback in the first place.

Comment 10

[Masatoshi Kimura [:emk]]

Attachment: Treat NS_ERROR_NET_INTERRUPT and NS_ERROR_NET_RESET as SSL errors on https URLs

I didn't add a text specific to these errors because MiTM can also craft other SSL errors.

Comment 11

[Patrick McManus [:mcmanus]]

Comment on attachment 8546922 [details] [diff] [review]
Treat NS_ERROR_NET_INTERRUPT and NS_ERROR_NET_RESET as SSL errors on https URLs

conceptually I think this is mostly fine.. I suspect you'll end up handing vanilla connection errors too (not intolerant related) - is that ok?

r?bz as this is docshell code and I am thankfully not a peer of that :)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8546922 [details] [diff] [review]
Treat NS_ERROR_NET_INTERRUPT and NS_ERROR_NET_RESET as SSL errors on https URLs

Yeah, I don't think we can win here.  r=me

Comment 13

[Masatoshi Kimura [:emk]]

(In reply to Patrick McManus [:mcmanus] from comment #11)
> conceptually I think this is mostly fine.. I suspect you'll end up handing
> vanilla connection errors too (not intolerant related) - is that ok?

Yes, that's what Brian suggested. See comment #9.

Comment 14

[Masatoshi Kimura [:emk]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5af849988b3

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a2b37d4c1e2

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6a2b37d4c1e2

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Does this patch enable the "report this problem to Mozilla" checkbox for these errors? If not, we should change it so it does. Then we should uplift this to Firefox 36. That way, Mozilla can start gathering more broad information about which servers are TLS intolerance so that it can make a whitelist of them.

Comment 17

[Masatoshi Kimura [:emk]]

Yes. This patch treats NS_ERROR_NET_INTERRUPT and NS_ERROR_NET_RESET as if they are SSL errors, so all UIs for SSL errors (including SSL error reporter) will be available.
Leaving ni? so that I don't forget the uplift request.

Comment 18

[Masatoshi Kimura [:emk]]

Unfortunately, there is no point in uplifting this to Firefox 36 due to bug 1096197 :(

Comment 19

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Why not just uplift bug 1096197 too? It doesn't seem like too much to uplift.

Comment 20

[Masatoshi Kimura [:emk]]

Comment on attachment 8546922 [details] [diff] [review]
Treat NS_ERROR_NET_INTERRUPT and NS_ERROR_NET_RESET as SSL errors on https URLs

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: We can't gather information about TLS version intolerant servers to maintain the whitelist (see bug 1114816).
[Describe test coverage new/current, TreeHerder]: no
[Risks and why]: Very low. This will only change the displayed error page for two errors.
[String/UUID change made/needed]: none.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ba67c22c1427

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Did I miss something in comment 21?

Comment 23

[Masatoshi Kimura [:emk]]

Sorry, I overlooked the comment.

Comment 24

[Matt Wobensmith [:mwobensmith][:matt:]]

I verified the fix by checking the following two TLS-intolerant sites. I set the TLS fallback limit to 3, and compared error pages in Fx35 vs Fx36/Fx37.

https://allyours.virginmedia.com NS_ERROR_NET_INTERRUPT
https://api.hecc.wisc.edu NS_ERROR_NET_RESET

Both sites now display the SSL error page instead of the network error page, including the UI that allows the user to report the error.

Comment 25

[Nickolay_Ponomarev]

> Finally, make sure that the support.mozilla.org documentation for the connection reset and connection
> closed errors explains TLS intolerance and links to documentation on how to fix it on the server.

Looks like this didn't happen - https://support.mozilla.org/en-US/kb/secure-connection-failed-error-message doesn't mention the new type of error page at all, making it very confusing. See bug 1195370.