1223131 - security.tls.insecure_fallback_hosts modified when visiting site

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[colin]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20151108004059

Steps to reproduce:

Add a URL to the security.tls.insecure_fallback_hosts value in about:config
Surf to that URL


Actual results:

Page loads without CSS or JS.
When reloading, page does not load at all, instead showing a 'Secure Connection Failed' error.
When I look at security.tls.insecure_fallback_hosts in about:config, the URL I just entered is missing


Expected results:

Site loads as expected

Comment 1

[colin]

Some more info:
- this is on Aurora/Developer Edition: 44.0a2 (2015-11-08)
- The site requires a certificate which is installed in the browser. This is a customers corporate intranet site, so I'm unable to provide access, nor easily make changes to the system.
- I assumed that the change from #1207137 may have to do with this, but I don't see the option to override the error
- The error displayed is: Error code: ssl_error_handshake_failure_alert

Comment 2

[Masatoshi Kimura [:emk]]

We shouldn't remove the host from the pref if the version fallback was needed.

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: Don't remove a host from the whitelist if the version fallback was needed

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8685393 [details] [diff] [review]
Don't remove a host from the whitelist if the version fallback was needed

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

This looks like the right approach, but I think we can be a bit more exact about the fallback test.

::: security/manager/ssl/nsNSSCallbacks.cpp
@@ +1244,5 @@
>      }
>    } else {
>      state = nsIWebProgressListener::STATE_IS_SECURE |
>              nsIWebProgressListener::STATE_SECURE_HIGH;
> +    if (channelInfo.protocolVersion >= SSL_LIBRARY_VERSION_TLS_1_2) {

Rather than hard-coding this to SSL_LIBRARY_VERSION_TLS_1_2, I imagine we should get the default version range (from SSL_VersionRangeGetDefault) and compare its max to either the max of the enabled version range on the socket (SSL_VersionRangeGet) or examine the information in nsSSLIOLayerHelpers::mTLSIntoleranceInfo (that way, we know if we've actually done a fallback or not).

Another idea (which we should maybe do anyway) would be to separate the weak cipher whitelist from the fallback whitelist. That's probably more work than we want to put into this, though.

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: Don't remove a host from the whitelist if the version fallback was needed, v2

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8685982 [details] [diff] [review]
Don't remove a host from the whitelist if the version fallback was needed, v2

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

Great.

Comment 7

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d864b4e62bb02a042a8e0ab90376eda87e21da52
Bug 1223131 - Don't remove a host from the whitelist if the version fallback was needed. r=keeler

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d864b4e62bb0

Comment 9

[Masatoshi Kimura [:emk]]

Colin, could you verify the fix with the latest Nightly? I'll request uplift if it works.

Comment 10

[colin]

Yep, confirmed that it works. Thanks!

Comment 11

[Masatoshi Kimura [:emk]]

Comment on attachment 8685982 [details] [diff] [review]
Don't remove a host from the whitelist if the version fallback was needed, v2

Approval Request Comment
[Feature/regressing bug #]: bug 1168635
[User impact if declined]: Users have no way to add exceptions for TLS intolerant servers.
[Describe test coverage new/current, TreeHerder]: Tested locally and baeked on m-c. Unfortunately, we can't automate TLS intolerance tests yet (bug 1082959).
[Risks and why]: Low, affects only TLS intolerance servers that are few on the internet now.
[String/UUID change made/needed]: none

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8685982 [details] [diff] [review]
Don't remove a host from the whitelist if the version fallback was needed, v2

Given that this fix has been on Nightly for a few days and verified by the bug submitter, it seems like a good idea to uplift to Aurora44.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Verified based on comment 10.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2c1d99f436dc