Closed
Bug 947149
Opened 11 years ago
Closed 10 years ago
Connection information claims RC4 is "high grade"
Categories
(Core Graveyard :: Security: UI, enhancement)
Core Graveyard
Security: UI
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla36
People
(Reporter: noloader, Assigned: emk)
References
Details
Attachments
(2 files, 3 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
emk
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release)
Build ID: 20131112160018
Steps to reproduce:
Visited a site (https://community.qualys.com) where negotiated cipher suite was RSA_WITH_RC4_128_SHA.
Expected results:
There is nothing "high grade" about RC4. It was the lesser of the evils in 2011 when Duong and Rizzo's BEAST was making headlines.
See, for example, "On the Security of RC4 in TLS and WPA", http://cr.yp.to/streamciphers/rc4biases-20130708.pdf:
... While the RC4 algorithm is known to have a
variety of cryptographic weaknesses (see [26]
for an excellent survey), it has not been previously
explored how these weaknesses can be exploited
in the context of TLS. Here we show that new and
recently discovered biases in the RC4 keystream
do create serious vulnerabilities in TLS when using
RC4 as its encryption algorithm.
If Firefox wants a stream cipher, use GCM mode or the upcoming Salsa20.
Updated•11 years ago
|
Component: Untriaged → Security
Product: Firefox → Core
Updated•11 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Comment 1•11 years ago
|
||
See also bug 956744.
Updated•11 years ago
|
Severity: normal → enhancement
Component: Security → Security: UI
Version: 25 Branch → Trunk
Updated•10 years ago
|
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8511583 -
Flags: review?(dkeeler)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → VYV03354
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8511583 [details] [diff] [review]
Don't treat RC4 as a strong cipher
Review of attachment 8511583 [details] [diff] [review]:
-----------------------------------------------------------------
Honestly, the distinction here is meaningless. Saying "high-grade" vs. "low-grade" isn't actionable. First, you've already visited the page using weak encryption to see this dialog, so your cookies have already been stolen. Second, the (vanishingly small number) of people who would (or indeed, could) use this information to inform whether or not they should continue using a site know how to decipher a string like "TLS_RSA_WITH_RC4_128_SHA", so "high-grade" vs. "low-grade" doesn't tell them anything they don't already know. Finally, even if the primary load of a page was delivered using a strong cipher, everything else on the page could have been delivered with a weak cipher, so presenting the information in this way is misleading at best in the first place. Really what we should do is expose this information per-load in the devtools. In the meantime, I would r+ a patch that removes the distinction here.
On the other hand, if we do expose what cipher suite each resource load used in the devtools, we probably would want to be able to classify each one according to best practices (including various things like perfect forward secrecy, key strength, etc. - see also bug 956744). So, I would also be interested in a patch that introduces a more general framework for classifying cipher suites.
Attachment #8511583 -
Flags: review?(dkeeler) → review-
Assignee | ||
Comment 4•10 years ago
|
||
Currently all supported cipher suites have >=90 bit keys. So the "weak" variants of strings are already dead.
I also remove the word "high-grade" which is useless and even misleading.
Attachment #8511583 -
Attachment is obsolete: true
Attachment #8512612 -
Flags: review?(dkeeler)
Comment on attachment 8512612 [details] [diff] [review]
Remove useless and even misleading word and dead code
Review of attachment 8512612 [details] [diff] [review]:
-----------------------------------------------------------------
This looks great. I just noted a couple nits. You should probably get a review from an actual Firefox (as in, the front-end) reviewer and sign-off from someone from UX (e.g. :phlsa) before landing this.
::: browser/base/content/pageinfo/security.js
@@ +256,5 @@
> hdr = pkiBundle.getString("pageInfo_MixedContent");
> msg1 = pkiBundle.getString("pageInfo_Privacy_Mixed1");
> msg2 = pkiBundle.getString("pageInfo_Privacy_None2");
> }
> else if (info.encryptionStrength >= 90) {
Hmmm - should this be changed to info.encryptionStrength > 0 now?
@@ -271,5 @@
> - [info.encryptionAlgorithm,
> - info.encryptionStrength + "",
> - info.version]);
> - msg1 = pkiBundle.getFormattedString("pageInfo_Privacy_Weak1", [info.hostName]);
> - msg2 = pkiBundle.getString("pageInfo_Privacy_Weak2");
Huh - looks like there was a bug here where security._cert wouldn't get set in the "weak encryption" case (although maybe that was on purpose - who knows).
::: security/manager/locales/en-US/chrome/pippki/pippki.properties
@@ +80,2 @@
> pageInfo_Privacy_Strong1=The page you are viewing was encrypted before being transmitted over the Internet.
> pageInfo_Privacy_Strong2=Encryption makes it very difficult for unauthorized people to view information traveling between computers. It is therefore very unlikely that anyone read this page as it traveled across the network.
Now that I'm really picking nits, I would get rid of the "very"s in this sentence, but it's not a big deal.
Attachment #8512612 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to David Keeler (:keeler) [use needinfo?] from comment #5)
> Now that I'm really picking nits, I would get rid of the "very"s in this
> sentence, but it's not a big deal.
OK, I've got the reason to rename the key :)
Attachment #8513038 -
Flags: review?(philipp)
Comment 7•10 years ago
|
||
Comment on attachment 8513038 [details] [diff] [review]
Remove useless and even misleading word and dead code, v2
>+pageInfo_Privacy_Encrypted1=The page you are viewing was encrypted before being transmitted over the Internet.
>+pageInfo_Privacy_Encrypted2=Encryption makes it difficult for unauthorized people to view information traveling between computers. It is therefore unlikely that anyone read this page as it traveled across the network.
I suggest just dropping the second sentence of pageInfo_Privacy_Encrypted2 entirely. It doesn't really add any information and just pads the text out. There's also the technicality of PFS being required for it to stay true.
Comment 8•10 years ago
|
||
Comment on attachment 8513038 [details] [diff] [review]
Remove useless and even misleading word and dead code, v2
Review of attachment 8513038 [details] [diff] [review]:
-----------------------------------------------------------------
*yoink* I think this is fine, and doesn't even need explicit ui-review.
But I did just want to explicitly confirm that comment 4 ("Currently all supported cipher suites have >=90 bit keys.") is true, which makes this is a no-brainer.
Looks so from https://www.ssllabs.com/ssltest/viewMyClient.html... I know there's still some MD5/3DES/RC4 in there that we'd like to get rid of, but that's a whole 'nuther discussion not suitable for this bug. (And, really, if we thought it was important enough to try and explain, we should really just treat them as broken SSL.)
Attachment #8513038 -
Flags: review?(philipp) → review+
Thanks, Dolske. Here's the list of ciphers enabled in firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=947149 (so yes, as far as I can tell, there are none that have <= 90-bit keys). See also bug 1088915 where we're exploring only using RC4 as a fallback.
(In reply to David Keeler (:keeler) [use needinfo?] from comment #9)
> Thanks, Dolske. Here's the list of ciphers enabled in firefox:
> https://bugzilla.mozilla.org/show_bug.cgi?id=947149
Er, I meant this: http://hg.mozilla.org/mozilla-central/annotate/675913ddbb55/security/manager/ssl/src/nsNSSComponent.cpp#l631
Assignee | ||
Comment 11•10 years ago
|
||
I would like to make RC4 broken (in another bug), but only after bug 1088915.
Because otherwise too many servers (>= 15%) will negotiate RC4.
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8512612 -
Attachment is obsolete: true
Attachment #8513038 -
Attachment is obsolete: true
Attachment #8514621 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 13•10 years ago
|
||
Keywords: checkin-needed
Comment 14•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 16•10 years ago
|
||
(In reply to David Keeler (:keeler) [use needinfo?] from comment #3)
> Honestly, the distinction here is meaningless. Saying "high-grade" vs.
> "low-grade" isn't actionable. First, you've already visited the page using
> weak encryption to see this dialog, so your cookies have already been
> stolen.
I think Mozilla browsers used to be able to warn when a "low-grade" (export) cipher suite is used with a modal dialog.
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•