1093595 - Treat SSL3 and RC4 as broken

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Masatoshi Kimura [:emk]]

Because they are indeed broken.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: Treat SSL3 and RC4 as broken

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: Change strings to add a description about weak encryption

Feel free to improve the wording :)

Comment 3

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

Comment on attachment 8516660 [details] [diff] [review]
Treat SSL3 and RC4 as broken

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

r- for a bit of code re-organization. Otherwise, this looks fine. I think we should be careful of how this interacts with mixed content UI indicators.

::: security/manager/ssl/src/nsNSSCallbacks.cpp
@@ +1178,5 @@
> +  MOZ_ASSERT(channelInfoFound);
> +  SSLCipherSuiteInfo cipherInfo;
> +  bool cipherInfoFound = false;
> +  bool weakEncryption = false;
> +  if (channelInfoFound) {

Rather than having indicator variables channelInfoFound and cipherInfoFound, we should have all of the code that depends on them being present in the same area (so either move this down there or move what's down there up here).

Comment 4

[Masatoshi Kimura [:emk]]

(In reply to David Keeler (:keeler) [use needinfo?] from comment #3)
> r- for a bit of code re-organization. Otherwise, this looks fine. I think we
> should be careful of how this interacts with mixed content UI indicators.

The broken status is transitive. That is, if at least one sub-resource is broken in the page, the top-level page will also turn to broken.
This behavior should be desirable because it will not miss insecure sub-resources unlike the rejected patch in bug 947149.
I couldn't find any interfering with mixed content blocker.

> Rather than having indicator variables channelInfoFound and cipherInfoFound,
> we should have all of the code that depends on them being present in the
> same area (so either move this down there or move what's down there up here).

Done.

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: Treat SSL3 and RC4 as broken, v2

Comment 6

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

Comment on attachment 8518130 [details] [diff] [review]
Treat SSL3 and RC4 as broken, v2

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

LGTM.

Comment 7

[Masatoshi Kimura [:emk]]

Comment on attachment 8516661 [details] [diff] [review]
Change strings to add a description about weak encryption

Oops, I accidentally obsoleted a wrong patch.

Comment 8

[Justin Dolske [:Dolske]]

Comment on attachment 8516661 [details] [diff] [review]
Change strings to add a description about weak encryption

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

I'm a really on the fence about this. SSL errors are notoriously complex/vague/confusing, and it would sure be nice to have an error that's more precise than "here are 2 different possible causes of what went wrong". Particularly so for site owners trying to understand the issue. OTOH, given the existing poor error messaging, user understanding,  and risk of adding more complexity for an uncommon (?) case, I don't know that it's worth worrying about.

So r+ for this, but if we fine ourselves overloading this state further we should break it apart. [If/when we do so, it might be useful to have separate fields for the state and reason. EG, SSL could be broken for multiple reasons, and it might be useful to report that somewhere instead of dribbling out the causes one by one as some poor webdev struggles to fix them. :)]

Comment 9

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba3cbe4ab1a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/beda2bdc1fe1

Comment 10

[Masatoshi Kimura [:emk]]

Bug 1092835 will help webdevs.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ba3cbe4ab1a4
https://hg.mozilla.org/mozilla-central/rev/beda2bdc1fe1

Comment 12

[Masatoshi Kimura [:emk]]

We should announce the deprecation of RC4.

Comment 13

[Jean-Yves Perrier [:teoli]]

I've added a sentence there: https://developer.mozilla.org/en-US/Firefox/Releases/36#Security
Can you review it before we ddc the bug and ask for a release note? I'm not 100% to have grasped all of this :-)

Comment 14

[Masatoshi Kimura [:emk]]

Looks good, but it would be better to note that SSLv3 is disabled by default since Firefox 34. The UI has been changed so that the users do not forget to make their browsers insecure.

Comment 15

[Jean-Yves Perrier [:teoli]]

Thanks, fixed.

Comment 16

[Tanvi Vyas[:tanvi]]

Just seeing this now.  I agree with Justin.  This takes two complicated topics (mixed content and deprecated ciphers in certs) and puts them together, making it even harder for the user/developer to understand why the ssl page they are on doesn't show the lock icon.

Do we have data on how often the grey triangle shows up because of a cert using RC4?

This bug updates the warning text for identity.broken_loaded.  It will tell the user that either they have mixed passive content OR they are using weak encryption.  But the AND scenario is also possible.  And the text for pages with mixed active content has not been updated - identity.mixed_active_loaded2 (which is probably a good thing).

Instead, I think the right thing to do here would be to continue using the grey triangle, but be more specific in the warning text - if the lock is missing because of weak encryption, communicate only that.  If it's because of mixed content, indicate that.  And if it's both, then give the current message that tells the user that both things are a problem.  This does make the code more complicated - but it can be achieved by adding an nsIWebProgressListener flag for weak encryption.  We can use this in the future for Sha1 and other deprecated ciphers. 

Before (or at least in addition to) surfacing this in UI, I think we should have had a webconsole error message so that developers and users know why they aren't seeing the lock icon.  Can we add a webconsole message asap?  And request an uplift to at least aurora.

Comment 17

[Tanvi Vyas[:tanvi]]

Looks like we do have warnings in the browser console but not the web console starting in Firefox 37 - bug 1092835.

Comment 18

[Masatoshi Kimura [:emk]]

I improved the error UI in bug 1126413. Unfortunately it is impossible to uplift it to beta because string changes.

Comment 19

[Tanvi Vyas[:tanvi]]

This string doesn't make sense:
pageInfo_Privacy_Broken1=Parts of the page you are viewing were not encrypted or the encryption is not strong enough before being transmitted over the Internet.

It should be:
pageInfo_Privacy_Broken1=Parts of the page you are viewing were not encrypted before being transmitted over the Internet or the encryption is not strong enough.

Comment 20

[Masatoshi Kimura [:emk]]

(In reply to Tanvi Vyas [:tanvi] from comment #19)
> This string doesn't make sense:
> pageInfo_Privacy_Broken1=Parts of the page you are viewing were not
> encrypted or the encryption is not strong enough before being transmitted
> over the Internet.
> 
> It should be:
> pageInfo_Privacy_Broken1=Parts of the page you are viewing were not
> encrypted before being transmitted over the Internet or the encryption is
> not strong enough.

Sorry for my poor English. Nobody pointed out it before landing :(

Comment 21

[Tanvi Vyas[:tanvi]]

Thanks Masatoshi for filing a followup bug for the string change.

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.

Also, do we have any data on what percentage of page loads use RC4?  From the patch, it doesn't look like there is a telemetry probe for weak encryption, but it can probably be added pretty easily:

Telemetry::Accumulate(Telemetry::SSL_WEAK_ENCRYPTION, weakEncryption ? 1 : 0)

Comment 22

[Masatoshi Kimura [:emk]]

(In reply to Tanvi Vyas [:tanvi] from comment #21)
> Thanks Masatoshi for filing a followup bug for the string change.
> 
> 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.

Unfortunately, it is not so easy to separate the flag. We will have to review all current STATE_IS_BROKEN usage:
https://mxr.mozilla.org/mozilla-central/search?string=STATE_IS_BROKEN
Also, add-ons and third-party themes will also be affected.

> Also, do we have any data on what percentage of page loads use RC4?  From
> the patch, it doesn't look like there is a telemetry probe for weak
> encryption, but it can probably be added pretty easily:

We already have a telemetry probe for used cipher suite (SSL_CIPHER_SUITE_FULL and SSL_CIPHER_SUITE_RESUMED).

Comment 23

[Tanvi Vyas[:tanvi]]

(In reply to Masatoshi Kimura [:emk] from comment #22)
> Unfortunately, it is not so easy to separate the flag. We will have to
> review all current STATE_IS_BROKEN usage:
> https://mxr.mozilla.org/mozilla-central/search?string=STATE_IS_BROKEN
> Also, add-ons and third-party themes will also be affected.
> 
We don't need to touch or remove the STATE_IS_BROKEN flag.  We just need to add a new flag STATE_USES_WEAK_CRYPTO that is set in addition to STATE_IS_BROKEN.