Closed
Bug 1093595
Opened 10 years ago
Closed 10 years ago
Treat SSL3 and RC4 as broken
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: emk, Assigned: emk)
References
Details
(Keywords: dev-doc-complete)
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
Dolske
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
Because they are indeed broken.
Assignee | ||
Comment 2•10 years ago
|
||
Feel free to improve the wording :)
Attachment #8516661 -
Flags: review?(dolske)
Assignee | ||
Updated•10 years ago
|
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).
Attachment #8516660 -
Flags: review?(dkeeler) → review-
Assignee | ||
Comment 4•10 years ago
|
||
(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.
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8516661 -
Attachment is obsolete: true
Attachment #8516661 -
Flags: review?(dolske)
Attachment #8518130 -
Flags: review?(dkeeler)
Comment on attachment 8518130 [details] [diff] [review] Treat SSL3 and RC4 as broken, v2 Review of attachment 8518130 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.
Attachment #8518130 -
Flags: review?(dkeeler) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #8516660 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8516661 [details] [diff] [review] Change strings to add a description about weak encryption Oops, I accidentally obsoleted a wrong patch.
Attachment #8516661 -
Attachment is obsolete: false
Attachment #8516661 -
Flags: review?(dolske)
Comment 8•10 years ago
|
||
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. :)]
Attachment #8516661 -
Flags: review?(dolske) → review+
Assignee | ||
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba3cbe4ab1a4 https://hg.mozilla.org/integration/mozilla-inbound/rev/beda2bdc1fe1
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ba3cbe4ab1a4 https://hg.mozilla.org/mozilla-central/rev/beda2bdc1fe1
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Assignee | ||
Comment 12•10 years ago
|
||
We should announce the deprecation of RC4.
Keywords: dev-doc-needed
Comment 13•10 years ago
|
||
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 :-)
Flags: needinfo?(VYV03354)
Assignee | ||
Comment 14•10 years ago
|
||
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.
Flags: needinfo?(VYV03354)
Comment 16•10 years ago
|
||
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•10 years ago
|
||
Looks like we do have warnings in the browser console but not the web console starting in Firefox 37 - bug 1092835.
Assignee | ||
Comment 18•10 years ago
|
||
I improved the error UI in bug 1126413. Unfortunately it is impossible to uplift it to beta because string changes.
Comment 19•10 years ago
|
||
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.
Assignee | ||
Comment 20•10 years ago
|
||
(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•10 years ago
|
||
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)
Assignee | ||
Comment 22•10 years ago
|
||
(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•9 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•