See comments 16-22 in https://bugzilla.mozilla.org/show_bug.cgi?id=1093595. Instead of telling users and developers that the problem with their page is that they either are using a weak cipher OR have mixed content, we should be more specific and tell them the error they are really having. https://bugzilla.mozilla.org/show_bug.cgi?id=1093595#c21 outlines how to implement this. +++ This bug was initially created as a clone of Bug #1093595 +++

Instructions on how to fix this bug copied from https://bugzilla.mozilla.org/show_bug.cgi?id=1093595#c21 It looks like it is easier to separate mixed content from weak encryption because of your work in https://bugzilla.mozilla.org/show_bug.cgi?id=1092835 to add a nsIWebProgressListener flag for STATE_USES_WEAK_CRYPTO. * Add a string in https://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties#300 for just weak crypto: identity.usesWeakCypher = The encryption algorithm used by this website is not strong and has become obsolete. (or somethign like that) * Change identity.broken_loaded back to what it was. * Create a new identity mode in browser.js - https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6600. * Check the webprogresslistener weak crypto flag here https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6808. If it is set, set the identity mode to the one you just created. You could make similar changes in security.js and security/manager/locales/en-US/chrome/pippki/pippki.properties.

Currently on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b2fb588a971 The proposed changes disambiguate errors between Mixed Content and 1 of 2 possible errors: a session resumption error and a session that uses the rc4 cipher suite.

Comment on attachment 8627349 [details] [diff] [review] 1153010.patch This looks good Steve! Can you split the patches into two? The first will be all the /browser code and r? it to dolske. The second is for the one /security file which you can r? to keeler. We may need to work on this text a bit: +identity.uses_weak_cipher=The encryption used by this website is weak or broken. It should not be relied on to prevent other people from viewing your information or modifying the website's behavior. It should be similar to the text for other identity states. People may want to revamp all of them, but this is not the bug to do that in! Let's just find something we can agree on for this string. Keeler has proposed: This connection is not private as a result of weak encryption. needinfo'ing rbarnes if he has any thoughts on what the weak crypto string should say to the user.

My vote is for: "This connection is not private as a result of weak encryption. Other people can view your information or modify the website's behavior." My initial intention was to tone the second sentence down a bit, but that just ended up being too wordy.

Comment on attachment 8627449 [details] [diff] [review] 1153010-browser.patch Review of attachment 8627449 [details] [diff] [review]: ----------------------------------------------------------------- I'm not a browser peer, so I just gave some feedback. Whatever the copy ends up being, I think it should be consistent across locations (i.e. the site identity drop-down and the security tab of the page info dialog). ::: browser/locales/en-US/chrome/browser/browser.properties @@ +328,5 @@ > identity.identified.verifier=Verified by: %S > identity.identified.verified_by_you=You have added a security exception for this site. > identity.identified.state_and_country=%S, %S > > +identity.uses_weak_cipher=The encryption used by this website is weak or broken. It should not be relied on to prevent other people from viewing your information or modifying the website's behavior. How about something more like "Your connection to this website uses weak encryption and is not private. Other people may be able to view your information or modify this website's behavior." @@ +329,5 @@ > identity.identified.verified_by_you=You have added a security exception for this site. > identity.identified.state_and_country=%S, %S > > +identity.uses_weak_cipher=The encryption used by this website is weak or broken. It should not be relied on to prevent other people from viewing your information or modifying the website's behavior. > +identity.mixed_display_loaded=The connection to this website is not fully secure because it contains unencrypted elements (such as images). Maybe "Your connection to this website contains unencrypted display elements and is not private. Other people may be able to view your information or modify how this website looks." (Mixed display content can leak cookies, but that's maybe a bit too much jargon to actually put in the message, hence the "view your information" part.)

I've updated the rc4 error message to make it consistent with keeler's suggestion, but have left the display message as is.

Comment on attachment 8628632 [details] [diff] [review] 1153010-security.patch Review of attachment 8628632 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.

Comment on attachment 8628631 [details] [diff] [review] 1153010-browser.patch Review of attachment 8628631 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +6769,2 @@ > } else { > + this.setMode(this.IDENTITY_MODE_USES_WEAK_CIPHER); We should break this pattern of making the "new thing" the default |else|, such that the next person to come along has to figure out what flag to check for when they add a newer thing. So add a .STATE_USES_WEAK_CRYPTO flag check, and an else that... hmm. does a generic setMode(this.IDENTITY_MODE_UNKNOWN)? Or maybe just asserts and throws if it's an unknown state? Not sure what's best to do here offhand.

(In reply to Justin Dolske [:Dolske] from comment #13) > Comment on attachment 8628631 [details] [diff] [review] > 1153010-browser.patch > > Review of attachment 8628631 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/base/content/browser.js > @@ +6769,2 @@ > > } else { > > + this.setMode(this.IDENTITY_MODE_USES_WEAK_CIPHER); > > We should break this pattern of making the "new thing" the default |else|, > such that the next person to come along has to figure out what flag to check > for when they add a newer thing. > > So add a .STATE_USES_WEAK_CRYPTO flag check, and an else that... hmm. does a > generic setMode(this.IDENTITY_MODE_UNKNOWN)? Or maybe just asserts and > throws if it's an unknown state? Not sure what's best to do here offhand. Unfortunately, because of nsSecureBrowserUIImpl, this is harder than it sounds. The STATE_USES_WEAK_CRYPTO flag is set and then disappears later in that file. We can add it back by adding another lis_is_broken_weakcrypto flag. But really don't want to touch that file until https://bugzilla.mozilla.org/show_bug.cgi?id=832834 is fixed for fear for breaking something. In the meantime, the fallback to weak crypto for a broken security state is not a bad option. Broken security should show the grey yield sign (which is changing to new iconography with the new control center design).

Comment on attachment 8628632 [details] [diff] [review] 1153010-security.patch >+pageInfo_WeakCipher=Your connection to this website uses weak encryption and is not private. Other people can view your information or modify the website's behavior. Steve, please fix the whitespacing here.

carrying over r+ from dolske

carrying over r+ from keeler

Hi, this patch failed to apply: adding 1153010 to series file renamed 1153010 -> 1153010-browser.patch applying 1153010-browser.patch patching file browser/base/content/browser.js Hunk #3 FAILED at 6990 1 out of 3 hunks FAILED -- saving rejects to file browser/base/content/browser.js.rej patching file browser/locales/en-US/chrome/browser/browser.properties Hunk #1 FAILED at 323 1 out of 1 hunks FAILED -- saving rejects to file browser/locales/en-US/chrome/browser/browser.properties.rej patching file browser/themes/shared/controlcenter/panel.inc.css Hunk #1 FAILED at 0 1 out of 1 hunks FAILED -- saving rejects to file browser/themes/shared/controlcenter/panel.inc.css.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and refresh 1153010-browser.patch could you take a look, thanks!

Carrying over r+ from dolske. Needed to merge with new changes to browser.js.

I folded the two patches because they had the same description anyway.