Connection information claims RC4 is "high grade"

RESOLVED FIXED in mozilla36

Status

enhancement
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: noloader, Assigned: emk)

Tracking

Trunk
mozilla36

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Reporter

Description

6 years ago
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

6 years ago
Component: Untriaged → Security
Product: Firefox → Core

Updated

6 years ago
OS: Mac OS X → All
Hardware: x86 → All

Comment 1

6 years ago
See also bug 956744.
See Also: → 956744
Severity: normal → enhancement
Component: Security → Security: UI
Version: 25 Branch → Trunk

Updated

5 years ago
See Also: → RC4
Assignee

Comment 2

5 years ago
Attachment #8511583 - Flags: review?(dkeeler)
Assignee

Updated

5 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

5 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

5 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

5 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 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

5 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

5 years ago
Attachment #8512612 - Attachment is obsolete: true
Attachment #8513038 - Attachment is obsolete: true
Attachment #8514621 - Flags: review+
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7277fa721a3f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Duplicate of this bug: 956744

Comment 16

5 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.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.