Closed Bug 1223131 Opened 4 years ago Closed 4 years ago

security.tls.insecure_fallback_hosts modified when visiting site

Categories

(Core :: Security: PSM, defect)

44 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla45
Tracking Status
firefox44 --- fixed
firefox45 --- fixed

People

(Reporter: colin, Assigned: emk)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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
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
We shouldn't remove the host from the pref if the version fallback was needed.
Blocks: 1168635
Status: UNCONFIRMED → NEW
Component: Untriaged → Security: PSM
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Flags: needinfo?(VYV03354)
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Flags: needinfo?(VYV03354)
Attachment #8685393 - Flags: review?(dkeeler)
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.
Attachment #8685393 - Flags: review?(dkeeler) → review-
Attachment #8685982 - Attachment description: 1223131_dont_rm_whitelist_if_fallback → Don't remove a host from the whitelist if the version fallback was needed, v2
Attachment #8685982 - Attachment is patch: true
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.
Attachment #8685982 - Flags: review?(dkeeler) → review+
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
https://hg.mozilla.org/mozilla-central/rev/d864b4e62bb0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Colin, could you verify the fix with the latest Nightly? I'll request uplift if it works.
Flags: needinfo?(colin)
Yep, confirmed that it works. Thanks!
Flags: needinfo?(colin)
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
Attachment #8685982 - Flags: approval-mozilla-aurora?
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.
Attachment #8685982 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified based on comment 10.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.