1252068 - Add functionality to reset key SSL prefs to their defaults when encountering an error likely caused by a changed pref

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P1)

Description

[Armen [:armenzg]]

I tried to load this page:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1244936&startday=2016-02-22&endday=2016-02-28&tree=all

yet Firefox does not give me a way to do so. I can't acknoledge that I accept the risk.

Here's a screenshot of the message:
http://people.mozilla.org/~armenzg/sattap/2b6b747f.png

Comment 1

[:Gijs (he/him)]

What error are you getting? That page loads fine for me. I believe in some cases we purposefully do not provide an override. Maybe Tim can provide more details.

Comment 2

[Armen [:armenzg]]

glob tried on Mac and did not have the issue.
I don't have any add-ons enabled.
----------------------------------------------------
Secure Connection Failed

The connection to brasstacks.mozilla.com was interrupted while the page was loading.

    The page you are trying to view cannot be shown because the authenticity of the received data could not be verified.
    Please contact the website owners to inform them of this problem.

Learn more…

Report errors like this to help Mozilla identify and block malicious sites

Comment 3

[Armen [:armenzg]]

I refreshed nightly and I don't hit the issue anymore.

Comment 4

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

This error indicates that the server is terminating the connection. In general, it is not possible for Firefox to connect to the server in these cases, regardless of what security properties the user wants to waive. We should consider updating the message to be more clear as to what's going on and what recourse the user has (i.e., in general, none). (To be clear, this may also indicate that the server is TLS 1.2-intolerant or uses RC4 and that our automatic mitigation efforts have failed. We might also consider linking to a more in-depth troubleshooting page that explores these options in depth.)

Comment 5

[Armen [:armenzg]]

It is happening again this morning:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1204281&startday=2016-02-29&endday=2016-02-29&tree=all

Comment 6

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

It seems that something is wrong with brasstacks.mozilla.com itself. I filed bug 1252504. Let's keep this bug focused on the UI issue.

Comment 7

[Armen [:armenzg]]

I also see this happening in here:
https://data.sparkfun.com/robs_office

It also says that these kinds of reports are reported back to Mozilla; does any of you know where it reports to?

Comment 8

[Armen [:armenzg]]

I get this (which seems different now that I look at it closely):

Secure Connection Failed

An error occurred during a connection to data.sparkfun.com. Peer reports incompatible or unsupported protocol version. Error code: SSL_ERROR_PROTOCOL_VERSION_ALERT

    The page you are trying to view cannot be shown because the authenticity of the received data could not be verified.
    Please contact the website owners to inform them of this problem.

Learn more…

Report errors like this to help Mozilla identify and block malicious sites

Comment 9

[:Atoll]

I'm duping bug 1252504 into this bug for now, because we have evidence going to two places, and it's not working keeping them split up and copying data back and forth between them. If you want to move this bug into the IT queue and ask us to hand it back when we're done, that's certainly acceptable.

Comment 11

[:Atoll]

So, the error you got in comment 8 indicates that you're not negotiating a valid SSL connection. I don't know why that is happening to sparkfun's site, and I notice in comment 3 that the issue is sporadic for you and is happening on Mozilla websites as well.

I assume you're running Nightly here, so IT asks that you please perform a series of mildly-annoying, but absolutely critical, diagnostic tests so we can verify that the issue isn't Nightly.

Could you please downgrade to Release channel *with a fresh profile* and use it for long enough to either reproduce the issue, or to be certain that it's an issue with Nightly and/or your existing profile? We've been making a lot of product TLS changes recently and based on the variability of this issue (without Chrome being affected, and with unusual errors) it is entirely possible you've uncovered a real bug in Nightly that needs to be addressed.

Comment 12

[Armen [:armenzg]]

Nightly + current profile: issue exists
Firefox + current profile: issue exists
Nightly + new profile:     no more issues
Firefox + new profile:     no more issues

Note that I refreshed my profile from comment 0 to comment 3 and it started happening again.

It seems that refreshing does something good, however, the issue comes back after updating to a new nightly OR after a number of hours.

Is there anything common to those two sites?

Comment 13

[:Gijs (he/him)]

(In reply to Armen Zambrano [:armenzg] - Engineering productivity from comment #12)
> Nightly + current profile: issue exists
> Firefox + current profile: issue exists
> Nightly + new profile:     no more issues
> Firefox + new profile:     no more issues
> 
> Note that I refreshed my profile from comment 0 to comment 3 and it started
> happening again.
> 
> It seems that refreshing does something good, however, the issue comes back
> after updating to a new nightly OR after a number of hours.

Can you reproduce with the "broken" / current profile from within safe mode?

Comment 14

[Armen [:armenzg]]

It does not work within safe mode.

Comment 15

[:Atoll]

Nope, nothing that I can see anyways. You might reach out to the Desktop TLS team since you're getting really bizarre TLS errors here.

Comment 16

[:Gijs (he/him)]

(In reply to Armen Zambrano [:armenzg] - Engineering productivity from comment #14)
> It does not work within safe mode.

Can you open about:support on the broken profile and dump the set of changed prefs here, especially any of the ones that include "security" or "ssl" ?

Comment 17

[Armen [:armenzg]]

Application Basics
------------------

Name: Firefox
Version: 47.0a1
Build ID: 20160302030209
Update Channel: nightly
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0
OS: Linux 3.13.0-79-generic x86-64
Multiprocess Windows: 2/2 (Enabled by default)
Safe Mode: false

Crash Reports for the Last 3 Days
---------------------------------

All Crash Reports

Extensions
----------

Name: Firefox Hello Beta
Version: 1.1.9
Enabled: true
ID: [e-mail]

Name: Pocket
Version: 1.0
Enabled: true
ID: [e-mail]

Name: YouTube ALL HTML5
Version: 3.0.3
Enabled: true
ID: jid1-qj0w91o64N7Eeg@jetpack

Name: Adblock Plus
Version: 2.7.2
Enabled: false
ID: {d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d}

Name: CodeViewer
Version: 1.0.4.1.1-signed
Enabled: false
ID: [e-mail]

Name: ghostery
Version: 6.0.0
Enabled: false
ID: [e-mail]

Name: JSONView
Version: 1.1.0
Enabled: false
ID: [e-mail]

Name: Mass Password Reset
Version: 1.05.1-signed
Enabled: false
ID: [e-mail]

Name: Mozilla Tree Status
Version: 1.0.2
Enabled: false
ID: [e-mail]

Name: Tab Groups Helper
Version: 1.0.15
Enabled: false
ID: [e-mail]

Name: TabSubmit
Version: 1.0.2.4.1-signed.1-let-fixed
Enabled: false
ID: {421e87b4-d3d2-49c8-b08f-b83f4dc88444}

Name: Tree Style Tab
Version: 0.16.2016021602
Enabled: false
ID: [e-mail]

Name: TreeStat
Version: 0.3
Enabled: false
ID: [e-mail]

Name: Ubuntu Modifications
Version: 3.2
Enabled: false
ID: [e-mail]

Name: Vertical Tabs
Version: 0.10b1.1-signed
Enabled: false
ID: [e-mail]

Name: Whimsy Pro
Version: 1.1.3
Enabled: false
ID: jid1-6mUPixNFCjAgkg@jetpack

Graphics
--------

Adapter Description: NVIDIA Corporation -- Quadro K1000M/PCIe/SSE2
Asynchronous Pan/Zoom: wheel input enabled
Device ID: Quadro K1000M/PCIe/SSE2
Driver Version: 4.4.0 NVIDIA 340.96
GPU Accelerated Windows: 0/2 Basic (OMTC)
Supports Hardware H264 Decoding: No
Vendor ID: NVIDIA Corporation
WebGL Renderer: NVIDIA Corporation -- Quadro K1000M/PCIe/SSE2
windowLayerManagerRemote: true
AzureCanvasAccelerated: 0
AzureCanvasBackend: cairo
AzureContentBackend: cairo
AzureFallbackCanvasBackend: none
CairoUseXRender: 0

Important Modified Preferences
------------------------------

accessibility.typeaheadfind.flashBar: 0
browser.cache.disk.capacity: 358400
browser.cache.disk.filesystem_reported: 1
browser.cache.disk.smart_size.first_run: false
browser.cache.disk.smart_size.use_old_max: false
browser.cache.frecency_experiment: 4
browser.download.importedFromSqlite: true
browser.places.smartBookmarksVersion: 7
browser.sessionstore.upgradeBackup.latestBuildID: 20160302030209
browser.startup.homepage_override.buildID: 20160302030209
browser.startup.homepage_override.mstone: 47.0a1
browser.tabs.warnOnClose: false
browser.urlbar.suggest.searches: true
browser.urlbar.userMadeSearchSuggestionsChoice: true
dom.apps.reset-permissions: true
dom.mozApps.used: true
extensions.lastAppVersion: 47.0a1
media.gmp-gmpopenh264.abi: x86_64-gcc3
media.gmp-gmpopenh264.lastUpdate: 1456756075
media.gmp-gmpopenh264.version: 1.5.3
media.gmp-manager.buildID: 20160302030209
media.gmp-manager.lastCheck: 1456954360
network.cookie.prefsMigrated: true
network.predictor.cleaned-up: true
places.database.lastMaintenance: 1456756839
places.history.expiration.transient_current_max_pages: 104858
plugin.disable_full_page_plugin_for_types: application/pdf
plugin.importedState: true
privacy.donottrackheader.enabled: true
privacy.sanitize.migrateClearSavedPwdsOnExit: true
security.ssl.errorReporting.automatic: true
security.tls.version.max: 1
services.sync.declinedEngines:
services.sync.engine.prefs.modified: false
services.sync.lastPing: 1456867969
services.sync.lastSync: Wed Mar 02 2016 16:20:26 GMT-0500 (EST)
services.sync.numClients: 3
storage.vacuum.last.index: 1
storage.vacuum.last.places.sqlite: 1456756837

Important Locked Preferences
----------------------------

JavaScript
----------

Incremental GC: true

Accessibility
-------------

Activated: false
Prevent Accessibility: 0

Library Versions
----------------

NSPR
Expected minimum version: 4.12
Version in use: 4.12

NSS
Expected minimum version: 3.23 Basic ECC Beta
Version in use: 3.23 Basic ECC Beta

NSSSMIME
Expected minimum version: 3.23 Basic ECC Beta
Version in use: 3.23 Basic ECC Beta

NSSSSL
Expected minimum version: 3.23 Basic ECC Beta
Version in use: 3.23 Basic ECC Beta

NSSUTIL
Expected minimum version: 3.23 Beta
Version in use: 3.23 Beta

Experimental Features
---------------------

Sandbox
-------

Seccomp-BPF (System Call Filtering): true
Seccomp Thread Synchronization: true
User Namespaces: true
Media Plugin Sandboxing: true

Comment 18

[:Gijs (he/him)]

If you revert security.tls.version.max to the default value in about:config, does that make the problem go away?

Comment 19

[Armen [:armenzg]]

Both sites start working again.

How did this pref change? I didn't do it myself.

Comment 20

[:Gijs (he/him)]

(In reply to Armen Zambrano [:armenzg] - Engineering productivity from comment #19)
> Both sites start working again.
> 
> How did this pref change? I didn't do it myself.

I don't know; it could have been one of the add-ons?

atoll/keeler, is there something we can do in PSM / the UI to make fixing this kind of failure easier or more obvious, like sanity-checking if any algorithms or tls version limits have been changed from their default settings? Can we detect this kind of failure as distinct from other types?

Comment 21

[:Atoll]

I have no idea, sorry, I'm just here from IT to make sure this is truly a client bug and not something on our servers.

Comment 22

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

We can certainly add "see if resetting these prefs helps" as a suggestion to try, but in general there's no way to know if that's the issue without actually trying it. This can also go in a support page. I suppose that's what this bug morphed into around comment 4 - the error page offers no help to the user (unless the learn more link has any relevant information, and I don't think it actually does).

Comment 23

[:Gijs (he/him)]

Panos, is this something the security/privacy folks could look at improving? Seems like there might be some low-hanging fruit in terms of just checking prefHasUserValue() for anything under security.ssl3 or security.ssl or security.tls (excluding tls and ssl error reporting) and, if any are non-default, display something like "It looks like your SSL settings might be causing this. Do you want Firefox to reset them and try again?" .

Cross-referencing with 1266441 because the original trigger for this bugreport was the same type of error page (connection reset, but in ssl/tls land).

Comment 24

[Panos Astithas (he/him) [:past] (please ni?)]

This sounds like a good idea.

Comment 26

[Jonathan Kingston [:jkt] he/him]

Attachment: MozReview Request: Bug 1252068 - Adding in reset prefs button on SSLNetError page

Review commit: https://reviewboard.mozilla.org/r/50523/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50523/

Comment 27

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8748766 [details]
MozReview Request: Bug 1252068 - Adding in reset prefs button on SSLNetError page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50523/diff/1-2/

Comment 28

[Jonathan Kingston [:jkt] he/him]

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

Comment 29

[:Cykesiopka]

https://reviewboard.mozilla.org/r/50523/#review47407

::: browser/base/content/browser.js:2667
(Diff revision 2)
>  const TLS_ERROR_REPORT_TELEMETRY_AUTO_UNCHECKED = 3;
>  const TLS_ERROR_REPORT_TELEMETRY_MANUAL_SEND    = 4;
>  const TLS_ERROR_REPORT_TELEMETRY_AUTO_SEND      = 5;
>  
> +const PREF_SSL_IMPACT = [
> +  "security.ssl.treat_unsafe_negotiation_as_broken",

Is this supposed to be security.ssl.require_safe_negotiation instead?
IIRC, security.ssl.treat_unsafe_negotiation_as_broken only affects the lock icon UI, and doesn't actually result in any interstitials.

Comment 30

[:Gijs (he/him)]

Comment on attachment 8748766 [details]
MozReview Request: Bug 1252068 - Adding in reset prefs button on SSLNetError page

https://reviewboard.mozilla.org/r/50523/#review47473

This looks pretty OK already, but I'd like to tweak how we determine what prefs to reset. Would probably want that stamped by :keeler or :ttaubert or someone else who knows the NSS side of things.

Also, I think we should limit when we show this warning to errors where there isn't some other reason things broke. Offering to reset prefs when the certificate authority is unknown, or when the cert is not valid because of timing restrictions or the host being wrong, that wouldn't be so useful. Can we only unhide the functionality in "the other cases", whatever they are? :-)

::: browser/base/content/aboutNetError.xhtml:121
(Diff revision 2)
> +          var event = new CustomEvent("AboutNetErrorResetPreferences",
> +            {bubbles:true});

Nit: stick the params object on the previous line - it'll still be shoerter than the line above it, so it doesn't seem useful to wrap it.

::: browser/base/content/browser.js:2750
(Diff revision 2)
>        break;
> +      case "Browser:ResetSSLPreferences":
> +        for (let prefName of PREF_SSL_IMPACT) {
> +          Services.prefs.clearUserPref(prefName);
> +        }
> +        gBrowser.reload();

You should call reload on the target, like `EnableOnlineMode` a bit higher up: `msg.target.reload()` -- in case the browser for which we got the message isn't the selected browser anymore by the time the message reaches the parent.

::: browser/base/content/content.js:236
(Diff revision 2)
> +const PREF_SSL_IMPACT = [
> +  "security.ssl.treat_unsafe_negotiation_as_broken",
> +  "security.tls.version.max",
> +  "security.tls.version.min",
> +  "security.tls.version.fallback-limit"
> +];

As per IRC discussion, I think it would make sense to reset whatever we find under "security.ssl.", "security.tls." and "security.ssl3.". That will also make duplicating those lists feel a little less hacky.

::: browser/base/content/content.js:372
(Diff revision 2)
> +    sendAsyncMessage("Browser:ResetSSLPreferences", {
> +      automatic: evt.detail
> +    });

It looks like there's some copy-paste leftovers here. You can omit the `evt` parameter (and not pass it at the callsite!), and just use:

`sendAsyncMessage("Browser:ResetSSLPreferences");`

as `automatic` is never used for this message.

::: browser/locales/en-US/chrome/overrides/netError.dtd:209
(Diff revision 2)
> +<!ENTITY prefReset.longDesc "It looks like your SSL settings might be causing this. Do you want Firefox to reset them and try again?">
> +<!ENTITY prefReset.label "Reset ssl settings">

So, as I understand it, /technically/ the term "SSL" is outdated at this point. Should we just say "SSL/TLS" or maybe even "security" instead?

For the second sentence, perhaps we can say "Do you want the default settings to be restored instead?"

(I'm taking out "Firefox" because we should use the correct brand name (IceWeasel/Nightly/Devedition/...) if any, and for network error pages there isn't an easy way to do that, unfortunately.)

Comment 31

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8748766 [details]
MozReview Request: Bug 1252068 - Adding in reset prefs button on SSLNetError page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50523/diff/2-3/

Comment 32

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8748766 [details]
MozReview Request: Bug 1252068 - Adding in reset prefs button on SSLNetError page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50523/diff/3-4/

Comment 33

[:Gijs (he/him)]

Comment on attachment 8748766 [details]
MozReview Request: Bug 1252068 - Adding in reset prefs button on SSLNetError page

https://reviewboard.mozilla.org/r/50523/#review47503

Some small nits below, but generally this looks good. Before landing though (and I'm sorry, I should have asked this last time) - can we add a test for this? I'd like to make sure it won't accidentally break if NSS changes stuff. :-)

::: browser/base/content/aboutNetError.xhtml:332
(Diff revision 4)
> +            const hasPrefStyleError = [
> +              'interrupted', // This happens with subresources that are above the max tls
> +              'SSL_ERROR_NO_CYPHER_OVERLAP',
> +              'SSL_ERROR_NO_CIPHERS_SUPPORTED'
> +            ].reduce((matches, matchString) => matches || getDescription().includes(matchString), false);

Nit:

```
const hasPrefStyleError = [
  ...
].some(substring => getDescription().includes(substring);
```

PS: love the CYPHER/CIPHERS difference in the different error codes... (the references are correct, but it's odd that we spell it differently...)

::: browser/base/content/aboutNetError.xhtml:338
(Diff revision 4)
> +            if (getErrorCode() == "nssFailure2" && hasPrefStyleError) {
> +              if (options && options.changedCertPrefs) {

Nit: No lonely ifs, please:

```
if (a && b && c && d)
```

rather than

```
if (a && b) {
  if (c && d) {
  // stuff
  }
}
```

Comment 34

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/50523/#review47503

> Nit:
> 
> ```
> const hasPrefStyleError = [
>   ...
> ].some(substring => getDescription().includes(substring);
> ```
> 
> PS: love the CYPHER/CIPHERS difference in the different error codes... (the references are correct, but it's odd that we spell it differently...)

Eh, and I left out a closing ')' here, but you get the idea.

Comment 35

[Jonathan Kingston [:jkt] he/him]

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

Comment 36

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8748766 [details]
MozReview Request: Bug 1252068 - Adding in reset prefs button on SSLNetError page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50523/diff/4-5/

Comment 37

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8748766 [details]
MozReview Request: Bug 1252068 - Adding in reset prefs button on SSLNetError page

So I think I have covered all your last review points.
The test required me to make a change to the ssl tunnel code to add support for tls1 checks. This may require extra testing from others perhaps?

Comment 38

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/50523/#review47821

We really need eslint running on .xhtml…

::: browser/base/content/aboutNetError.xhtml:114
(Diff revision 5)
>          document.getElementById('certificateErrorReporting').style.display = 'block';
>        }
>  
> +      function showPrefChangeContainer() {
> +        const panel = document.getElementById('prefChangeContainer')
> +        panel.style.display = "block";
> +        document.getElementById("prefResetButton").addEventListener("click", function resetPreferences(e) {

Nit: use double-quotes in browser/

Can you fix the one existing single quote?

::: browser/base/content/aboutNetError.xhtml:325
(Diff revision 5)
>  
>                checkbox.addEventListener('change', function(evt) {
>                    var event = new CustomEvent("AboutNetErrorSetAutomatic",

Double-quotes

::: browser/base/content/aboutNetError.xhtml:333
(Diff revision 5)
> +              'interrupted', // This happens with subresources that are above the max tls
> +              'SSL_ERROR_UNSUPPORTED_VERSION',
> +              'SSL_ERROR_NO_CYPHER_OVERLAP',
> +              'SSL_ERROR_NO_CIPHERS_SUPPORTED'

Double quotes

Comment 39

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8748766 [details]
MozReview Request: Bug 1252068 - Adding in reset prefs button on SSLNetError page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50523/diff/5-6/

Comment 40

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #38)
> https://reviewboard.mozilla.org/r/50523/#review47821
> 
> We really need eslint running on .xhtml…

I thought it already was - not sure we have rules for single/double quotes enabled though. Can you file a bug?

Comment 41

[:Gijs (he/him)]

Comment on attachment 8748766 [details]
MozReview Request: Bug 1252068 - Adding in reset prefs button on SSLNetError page

https://reviewboard.mozilla.org/r/50523/#review48049

r=me on the browser changes with the nits below fixed. However, please get the tls/ssltunnel changes reviewed by a peer of that module. :-)

::: browser/base/content/test/general/browser_aboutNetError.js:10
(Diff revision 6)
> +
> +// Set ourselves up for TLS error
> +Services.prefs.setIntPref('security.tls.version.max', 3);
> +Services.prefs.setIntPref('security.tls.version.min', 3);
> +
> +const BAD_TLS = "https://tls1.example.com/";

Nit: Maybe rename this "LOW_TLS_VERSION" or something?

::: browser/base/content/test/general/browser_aboutNetError.js:37
(Diff revision 6)
> +  let pageshowPromise = promiseWaitForEvent(browser, "pageshow");
> +  let message = yield ContentTask.spawn(browser, null, function* () {
> +    content.document.getElementById("prefResetButton").click();
> +  });
> +  yield pageshowPromise;
> +  Assert.equal(content.document.getElementById('errorPageContainer'), null, "Should not have an error container");

Maybe just check content.document.documentURI matching BAD_TLS ?

::: browser/base/content/test/general/browser_aboutNetError.js:43
(Diff revision 6)
> +function waitForPageLoad(browser) {
> +  return new Promise(resolve => {
> +    info("Waiting for DOMContentLoaded event");
> +    browser.addEventListener("DOMContentLoaded", function load() {
> +      browser.removeEventListener("DOMContentLoaded", load, false, true);

Instead of this, please see if you can use BrowserTestUtils.waitForErrorPage(browser);

::: build/pgo/server-locations.txt:263
(Diff revision 6)
>  
>  # Hosts for ssl3/rc4 console warning tests
>  https://ssl3.example.com:443        privileged,ssl3
>  https://rc4.example.com:443         privileged,rc4
>  https://ssl3rc4.example.com:443     privileged,ssl3,rc4
> +https://tls1.example.com:443     privileged,tls1

Nit: indent 'privileged,tls1' to the same level as the other lines

Comment 42

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8748766 [details]
MozReview Request: Bug 1252068 - Adding in reset prefs button on SSLNetError page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50523/diff/6-7/

Comment 43

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8748766 [details]
MozReview Request: Bug 1252068 - Adding in reset prefs button on SSLNetError page

Tim would you be able to review resetting the preference branches: "security.tls.", "security.ssl.", "security.ssl3."

Also could you check over the ssltunnel change to add a tls1.0 feature?

Thanks!

Comment 44

[Al Billings [:abillings - ex-MoCo]]

Why did you ask for sec-approval? This is an open bug and not a sec-critical or sec-high issue.

https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 45

[Jonathan Kingston [:jkt] he/him]

Sorry I was trying to get someone from a security team to review the code changes, it was a mistake.

Comment 46

[Justin Dolske [:Dolske]]

So... It certainly makes sense to provide a more detailed error message for this situation (the generic page in the comment 0 is pretty bad, though now we tend to at least show an SSL error code).

But why is this patch adding a button to reset prefs? We don't expose UI for these in prefs in the first place, so it seems strange to add UI to reset them. I'd expect that these prefs are seldom-set, and that a Google-able error code (and maybe a string explicitly mentioning the relevant pref) would be adequate.

Comment 47

[Jonathan Kingston [:jkt] he/him]

Gijs might be able to explain further his thinking however my understanding is that it will reduce the complexity of fixing issues like this. This could be set by many addons, stale profiles or the users themselves. The addon use case seems to be something worth tackling with a button.

The use case may be less needed than warranted to have a button perhaps? (I'm new so don't really know), Is this something that could use some metrics as it goes in perhaps?

Is your issue with this patch that it could allow users to degrade security easily?

Comment 48

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8748766 [details]
MozReview Request: Bug 1252068 - Adding in reset prefs button on SSLNetError page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50523/diff/7-8/

Comment 49

[:Gijs (he/him)]

(In reply to Justin Dolske [:Dolske] from comment #46)
> So... It certainly makes sense to provide a more detailed error message for
> this situation (the generic page in the comment 0 is pretty bad, though now
> we tend to at least show an SSL error code).
> 
> But why is this patch adding a button to reset prefs? We don't expose UI for
> these in prefs in the first place, so it seems strange to add UI to reset
> them. I'd expect that these prefs are seldom-set, and that a Google-able
> error code (and maybe a string explicitly mentioning the relevant pref)
> would be adequate.

Jonathan pretty much described this already. Add-ons or people set these prefs (some sites tell people to set the prefs!) to "make SSL work better with website X" or to be "more secure", only to forget that that happened / not unset them, and then they break websites. This kind of stuff is why people are told to use Firefox refresh (which effectively does the same thing as we're doing here, but more invasively).

My gut feeling is that Googlable error codes are unlikely to help because we actually get fairly generic information from NSS/docshell here, in part because it doesn't know, either, AFAICT. The error page linked in comment #0 is effectively a "net reset" http error, but for an SSL connection. There are a lot of reasons that could happen, including but not limited to a "normal" http net reset. But we can *know* that the user has messed with these settings, in which case it is surprisingly likely that resetting them fixes the problem. We could only provide the error code if people have messed with the settings... but at that stage, why not just let the user fix it then and there, much like we do for e.g. being in offline mode? That seems like a better user experience...

Comment 50

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

https://reviewboard.mozilla.org/r/50523/#review48479

This looks good. One concern I have is that in the case where the user hasn't modified any prefs, the page doesn't quite clarify that the problem is probably with the server or somewhere in the network (i.e. it's not Firefox's fault and it's not the user's fault).

::: browser/base/content/browser.js:2669
(Diff revision 8)
>  const TLS_ERROR_REPORT_TELEMETRY_AUTO_SEND      = 5;
>  
> +const PREF_SSL_IMPACT_ROOTS = ["security.tls.", "security.ssl.", "security.ssl3."];
> +
> +const PREF_SSL_IMPACT = PREF_SSL_IMPACT_ROOTS.reduce((prefs, root) => {
> +  return prefs.concat(Services.prefs.getChildList(root));

This looks like it will get prefs like security.ssl.errorReporting.*, which are unrelated. In fact, I would probably leave out the whole security.ssl branch, since changing those probably won't result in a connection reset error (and resetting them probably won't fix things).

security.tls.version.fallback-limit, security.tls.insecure_fallback_hosts, and security.tls.unrestricted_rc4_fallback should probably be left alone, because they wouldn't cause a connection reset by being set.

Long story short I would restructure this to just consist of:

security.tls.version.max
security.tls.version.min
security.ssl3.*

::: browser/locales/en-US/chrome/overrides/netError.dtd:209
(Diff revision 8)
>  <!ENTITY inadequateSecurityError.title "Your connection is not secure">
>  <!-- LOCALIZATION NOTE (inadequateSecurityError.longDesc) - Do not translate
>       "NS_ERROR_NET_INADEQUATE_SECURITY". -->
>  <!ENTITY inadequateSecurityError.longDesc "<p><span class='hostname'></span> uses security technology that is outdated and vulnerable to attack. An attacker could easily reveal information which you thought to be safe. The website administrator will need to fix the server first before you can visit the site.</p><p>Error code: NS_ERROR_NET_INADEQUATE_SECURITY</p>">
> +
> +<!ENTITY prefReset.longDesc "It looks like your SSL settings might be causing this. Do you want the default settings to be restored instead?">

We should probably use "TLS" instead of "SSL". Either way, it's a bit jargony - can we explain what the issue is without using jargon?

Also, not sure "instead" is needed at the end there.

::: browser/locales/en-US/chrome/overrides/netError.dtd:210
(Diff revision 8)
>  <!-- LOCALIZATION NOTE (inadequateSecurityError.longDesc) - Do not translate
>       "NS_ERROR_NET_INADEQUATE_SECURITY". -->
>  <!ENTITY inadequateSecurityError.longDesc "<p><span class='hostname'></span> uses security technology that is outdated and vulnerable to attack. An attacker could easily reveal information which you thought to be safe. The website administrator will need to fix the server first before you can visit the site.</p><p>Error code: NS_ERROR_NET_INADEQUATE_SECURITY</p>">
> +
> +<!ENTITY prefReset.longDesc "It looks like your SSL settings might be causing this. Do you want the default settings to be restored instead?">
> +<!ENTITY prefReset.label "Fix settings">

"Fix" might be a bit of a strong guarantee. Maybe "Try with default settings"?

Comment 51

[Matthew N. [:MattN]]

(In reply to :Gijs Kruitbosch from comment #40)
> (In reply to Matthew N. [:MattN] (behind on reviews) from comment #38)
> > https://reviewboard.mozilla.org/r/50523/#review47821
> > 
> > We really need eslint running on .xhtml…
> 
> I thought it already was

No, that was .html but people forgot about .xhtml which is what we actually use in-product (mostly due to l10n).

I filed bug 1270926 and upstream already has a potential fix.

> - not sure we have rules for single/double quotes enabled though. Can you file a bug?

That rule is enabled for some directories already. I filed bug 1272216 to make it the default for the tree.

Comment 52

[Jonathan Kingston [:jkt] he/him]

https://reviewboard.mozilla.org/r/50523/#review48479

The box will only show when there is changed settings. So granted a user with strong settings may see this and it not fix anything so it's restricted to only showing when my modified prefs wouldn't display against a default.

It's only a subtle change but I think the wording change of "It looks like your strong security settings might be causing this." and "Try with default settings" reduces the risk of the user thinking this is their fault.

I suspect the non pref change (including addon pref changes) should be a follow up bug (will file after finishing this update :D).

Thanks for the comments.

Comment 53

[:Gijs (he/him)]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #50)
> "Fix" might be a bit of a strong guarantee. Maybe "Try with default
> settings"?

I see your point, but we should probably still use something like "Change settings" or "Restore default settings", because "try with default settings" could be read as just ignoring the settings for this connection without changing them permanently, which would make the final result (your settings are reset) surprising.

Comment 54

[Jonathan Kingston [:jkt] he/him]

https://reviewboard.mozilla.org/r/50523/#review48479

> This looks like it will get prefs like security.ssl.errorReporting.*, which are unrelated. In fact, I would probably leave out the whole security.ssl branch, since changing those probably won't result in a connection reset error (and resetting them probably won't fix things).
> 
> security.tls.version.fallback-limit, security.tls.insecure_fallback_hosts, and security.tls.unrestricted_rc4_fallback should probably be left alone, because they wouldn't cause a connection reset by being set.
> 
> Long story short I would restructure this to just consist of:
> 
> security.tls.version.max
> security.tls.version.min
> security.ssl3.*

Will change to this as it seems to cover the core cases, the ones I was not sure if should be included were:
"security.ssl.require_safe_negotiation"
"security.ssl.treat_unsafe_negotiation_as_broken"
"security.ssl.false_start.require-npn"
"security.tls.version.fallback-limit"

> We should probably use "TLS" instead of "SSL". Either way, it's a bit jargony - can we explain what the issue is without using jargon?
> 
> Also, not sure "instead" is needed at the end there.

I changed to "network security settings" after some thought (I'm was worried with just using security users may confuse with password settings or other security settings), as mentioned before we can handle the other case of no prefs in another bug.

Comment 55

[Jonathan Kingston [:jkt] he/him]

(In reply to :Gijs Kruitbosch from comment #53)
> (In reply to David Keeler [:keeler] (use needinfo?) from comment #50)
> > "Fix" might be a bit of a strong guarantee. Maybe "Try with default
> > settings"?
> 
> I see your point, but we should probably still use something like "Change
> settings" or "Restore default settings", because "try with default settings"
> could be read as just ignoring the settings for this connection without
> changing them permanently, which would make the final result (your settings
> are reset) surprising.

"Restore default settings" seems the most accurate and addresses the issue Gijs mentions also, will change to this :D

Comment 56

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8748766 [details]
MozReview Request: Bug 1252068 - Adding in reset prefs button on SSLNetError page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50523/diff/8-9/

Comment 57

[Jonathan Kingston [:jkt] he/him]

I think we can safely check this in as it resolves all your issues now.

Comment 58

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/00b8a2d2825f

Comment 59

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/00b8a2d2825f

Comment 60

[:Cykesiopka]

I changed the summary to what I view is what this bug morphed into.
If the new summary doesn't accurately described what happened, feel free to change it to something more appropriate.

Comment 61

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

Paul, when you verify this bug, here is one test case that you could use. 

1. Go to about:config and set "security.tls.version.min" to 3.
2. View this site: https://aim.nextjump.com/
3. Verify that error page has new UI with "Restore default settings" button.
4. Click on this button and verify that site now loads correctly.
5. Go back to about:config and verify that preference has been reset to a value of 1.

Comment 62

[Paul Silaghi, QA [:pauly]]

Thanks Matt.
Verified fixed FX 49.0a1 (2016-05-25) Win 7, Ubuntu 16.04, OS X 10.11.