Martin Thomson [:mt:] Reporter
	Description • 10 years ago

If we're too scared to disable RC4, maybe we can stop offering it initially.  That's a less aggressive approach, and it should help us learn how much RC4 is really used out there.

	Martin Thomson [:mt:] Reporter
	Comment 1 • 10 years ago

Here's a rough cut at this.

I chose to simply enumerate the suites here.  This is quicker than iterating through all supported suites and checking which are both enabled and use cipher_rc4, since this is on the hot path.

I think that bug 999544 is still better in the long run, but this might help build the case for that.  (This is also what MS do, approximately, with perhaps a different version number involved.)

	Masatoshi Kimura [:emk] Assignee
	Comment 2 • 10 years ago

This will not work if the server supports TLS 1.2, incompatible suites, RC4, and fallback SCSV.
For example, if the server only supports TLS_RSA_WITH_RC4_128_SHA and TLS_RSA_WITH_AES_128_GCM_SHA256,
1. Try with TLS 1.2 without RC4 suites first.
2. The handshake will fail with no_cypher_overlap alert because no common cipher suites found.
3. Retry with TLS 1.1 + RC4 suites. We will send the fallback scsv because TLS 1.1 is not the highest version we support.
4. The fallback scsv will protect users from our downgrading attack :)
I found it when I was testing a similar patch on YouTube.
We need to retry with RC4 suites *before* downgrading the protocol version.

	Martin Thomson [:mt:] Reporter
	Comment 3 • 10 years ago

:emk, That's not quite right: both peers are required to offer/support TLS_RSA_WITH_AES_128_CBC_SHA

That's part of why I recommended that we disable RC4 in TLS 1.2 only, and not simply the first handshake.  If the first handshake is at a lower version, we might get stuck in the situation you describe.

If the first handshake is TLS 1.3, it won't include RC4 anyway, so there's no point there.  It might be OK to do the same for TLS 1.1, but that's the last version of TLS that has a reliable mandatory cipher suite (the mandatory cipher suite in TLS 1.0 is borked).

	Masatoshi Kimura [:emk] Assignee
	Comment 4 • 10 years ago

(In reply to Martin Thomson [:mt] from comment #3)
> :emk, That's not quite right: both peers are required to offer/support
> TLS_RSA_WITH_AES_128_CBC_SHA

In theory, it is. In practice, YouTube doesn't support TLS_RSA_WITH_AES_128_CBC_SHA. And in the ideal world, we don't have to implement the insecure fallback in the first place.

> That's part of why I recommended that we disable RC4 in TLS 1.2 only, and
> not simply the first handshake.  If the first handshake is at a lower
> version, we might get stuck in the situation you describe.

If the first handshake is at the lower version, it means that we configured to disable TLS 1.2. So we will not send the fallback scsv in the first handshake.

> If the first handshake is TLS 1.3, it won't include RC4 anyway, so there's
> no point there.

Wrong. We will send all supported cipher suites we support even if the max version of the first handshake is TLS 1.3. Servers can select TLS 1.2 and RC4 (or any other cipher suites unavailable in TLS 1.3) if they do not support TLS 1.3. It is the sacure negotiation the TLS spec intended. Why are you premised on the insecure fallback?

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 5 • 10 years ago

(In reply to Martin Thomson [:mt] from comment #3)
> :emk, That's not quite right: both peers are required to offer/support
> TLS_RSA_WITH_AES_128_CBC_SHA

Masatoshi is describing the behavior of *.googlevideo.com:

https://www.ssllabs.com/ssltest/analyze.html?d=r9---sn-nwj7km7e.googlevideo.com

These servers are important because these are the servers that serve the video content for https://youtube.com, which is a very popular website.

> no point there.  It might be OK to do the same for TLS 1.1, but that's the
> last version of TLS that has a reliable mandatory cipher suite (the
> mandatory cipher suite in TLS 1.0 is borked).

The mandatory cipher suite stuff in any/all of the TLS specifications doesn't matter in practice.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 6 • 10 years ago

(In reply to Masatoshi Kimura [:emk] from comment #4)
> Wrong. We will send all supported cipher suites we support even if the max
> version of the first handshake is TLS 1.3. Servers can select TLS 1.2 and
> RC4 (or any other cipher suites unavailable in TLS 1.3) if they do not
> support TLS 1.3. It is the sacure negotiation the TLS spec intended. Why are
> you premised on the insecure fallback?

I agree. It seems like very bad practice to build stuff that is premised on the non-secure fallback being around forever.

	Martin Thomson [:mt:] Reporter
	Comment 7 • 10 years ago

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #6)
> (In reply to Masatoshi Kimura [:emk] from comment #4)
> > Wrong. We will send all supported cipher suites we support even if the max
> > version of the first handshake is TLS 1.3. Servers can select TLS 1.2 and
> > RC4 (or any other cipher suites unavailable in TLS 1.3) if they do not
> > support TLS 1.3. It is the sacure negotiation the TLS spec intended. Why are
> > you premised on the insecure fallback?

I see your point.  And yes, this relies on having insecure fallback in place.  But it's intended as a short term step to let us learn how prevalent RC4 really is, which should inform a decision to disable it entirely.

> I agree. It seems like very bad practice to build stuff that is premised on
> the non-secure fallback being around forever.

The suggestion that Youtube might operate a non-compliant stack with the downgrade SCSV makes me reconsider though.  They are important enough to make this a problem...if they support the SCSV as well.  I'll have to check that (upgrading to Yosemite turned out to have consequences).

	Martin Thomson [:mt:] Reporter
	Comment 8 • 10 years ago

TLS_RSA_WITH_AES_128_GCM_SHA256 is supported in NSS.  Maybe Brian knows why we disable it.

	Masatoshi Kimura [:emk] Assignee
	Comment 9 • 10 years ago

(In reply to Martin Thomson [:mt] from comment #8)
> TLS_RSA_WITH_AES_128_GCM_SHA256 is supported in NSS.  Maybe Brian knows why
> we disable it.

See bug 1029179.

	Masatoshi Kimura [:emk] Assignee
	Comment 10 • 10 years ago

Here is a sketch of my counter proposal.
- This patch did not break googlevideo.com (I tested).
- This patch will work even if we bumped the security.tls.version.fallback-limit pref to 3.

Basically this patch will change the fallback order to TLS 1.2(->TLS 1.2+RC4)->TLS 1.1(+RC4)->TLS 1.0(+RC4). The RC4 fallback will occur only if the server fails with no_cypher_overlap alert.

	Martin Thomson [:mt:] Reporter
	Comment 11 • 10 years ago

Comment on attachment 8511474 [details] [diff] [review]
bug1088915_no_rc4_in_first_handshake

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

This looks mostly fine.  It looks like it does this though: TLS1.2-RC4, TLS1.2+RC4, TLS1.1-RC4 (!), TLS1.1+RC4, TLS1.0-RC4, TLS1.0+RC4.

That's probably too many fallbacks.

I'd also like to see some telemetry for this so that we can judge its effectiveness.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +987,5 @@
>        range.max = entry.intolerant - 1;
>      }
>    }
> +  if (needWeakCipher) {
> +    *needWeakCipher = entry.needWeakCipher;

I think that you need to expand the check to be (needWeakCipher || entry.intolerant)

@@ +1207,5 @@
>                                   socketInfo->GetPort(),
>                                   range.max + 1);
>  
>      return false;
> +  } else if (err == SSL_ERROR_NO_CYPHER_OVERLAP) {

This error covers a lot of cases more than just having no overlap, including the case where our minimum version is higher than their maximum. 

As a result, this is going to have some false positives.  I think that's OK though.

@@ +2591,3 @@
>            fd, static_cast<unsigned int>(range.min),
> +              static_cast<unsigned int>(range.max),
> +          needWeakCipher ? " with RC4 ciphers" : ""));

s/RC4/weak/

::: security/manager/ssl/src/nsNSSIOLayer.h
@@ +203,5 @@
>      uint16_t tolerant;
>      uint16_t intolerant;
>      PRErrorCode intoleranceReason;
> +    bool forceStrongCipher;
> +    bool needWeakCipher;

If you fix the version check, then this probably needs a better name.  Since this is only true after the first fallback-inducing error, it's slightly misleading.  A comment might help too.

	Masatoshi Kimura [:emk] Assignee
	Comment 12 • 10 years ago

(In reply to Martin Thomson [:mt] from comment #11)
> Comment on attachment 8511474 [details] [diff] [review]
> bug1088915_no_rc4_in_first_handshake
> 
> Review of attachment 8511474 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks mostly fine.  It looks like it does this though: TLS1.2-RC4,
> TLS1.2+RC4, TLS1.1-RC4 (!), TLS1.1+RC4, TLS1.0-RC4, TLS1.0+RC4.

Really? My intention was TLS1.2-RC4, TLS1.2+RC4, TLS1.1+RC4, TLS1.0+RC4. Once needWeakCipher is set, it will never be unset until fallback goes to TLS1.0+RC4 and everything are reset.

> That's probably too many fallbacks.

So the fallback count was only increased by one (if I implemented my idea correctly). Luckily, we reduced the fallback count by one in recent.

> I'd also like to see some telemetry for this so that we can judge its
> effectiveness.

Will do (hence "sketch").

> ::: security/manager/ssl/src/nsNSSIOLayer.cpp
> @@ +987,5 @@
> >        range.max = entry.intolerant - 1;
> >      }
> >    }
> > +  if (needWeakCipher) {
> > +    *needWeakCipher = entry.needWeakCipher;
> 
> I think that you need to expand the check to be (needWeakCipher ||
> entry.intolerant)

This is a null check of the pointer. Note that needWeakCipher is a pointer.

> @@ +1207,5 @@
> >                                   socketInfo->GetPort(),
> >                                   range.max + 1);
> >  
> >      return false;
> > +  } else if (err == SSL_ERROR_NO_CYPHER_OVERLAP) {
> 
> This error covers a lot of cases more than just having no overlap, including
> the case where our minimum version is higher than their maximum. 

Isn't it protocol_version alert?

> ::: security/manager/ssl/src/nsNSSIOLayer.h
> @@ +203,5 @@
> >      uint16_t tolerant;
> >      uint16_t intolerant;
> >      PRErrorCode intoleranceReason;
> > +    bool forceStrongCipher;
> > +    bool needWeakCipher;
> 
> If you fix the version check, then this probably needs a better name.  Since
> this is only true after the first fallback-inducing error, it's slightly
> misleading.  A comment might help too.

Could you suggest a better name? Probably you speak English better than me.

	Martin Thomson [:mt:] Reporter
	Comment 13 • 10 years ago

(In reply to Masatoshi Kimura [:emk] from comment #12)
> Really? My intention was TLS1.2-RC4, TLS1.2+RC4, TLS1.1+RC4, TLS1.0+RC4.
> Once needWeakCipher is set, it will never be unset until fallback goes to
> TLS1.0+RC4 and everything are reset.

You set needWeakCipher in rememberIntolerant..(), but I see now that that is OK.  This is now surprisingly complex code.

> This is a null check of the pointer. Note that needWeakCipher is a pointer.

This was my lame attempt to fix the above (if already intolerant, then always ensure that needWeakCipher is set).  The || should have been && if that was the case.  It's not needed.

> Isn't it protocol_version alert?

Not from what I've seen.  The error is generated locally.  An NSS quirk.

A non-relevant example:
http://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c#907

That makes me think.  The code you added to retryDueToTLSIntolerance exits early in a couple of cases.  I think that you probably want to let that fall through to the other logic.  Otherwise you are letting cipher suite intolerance short-circuit the whole fallback routine and cipher suite intolerance is an already established reason for version fallback.

Perhaps a better approach is to have a flag called strongCiphersFailed and use that as the inverse of needWeakCipher.  Then set this to true if you get NO_CYPHER_OVERLAP and you don't have it's pair already set: strongCiphersWorked, which is exactly equivalent to forceStrongCipher.

> > > +    bool needWeakCipher;
> > 
> > If you fix the version check, then this probably needs a better name.  Since
> > this is only true after the first fallback-inducing error, it's slightly
> > misleading.  A comment might help too.
> 
> Could you suggest a better name? Probably you speak English better than me.

What ever made you think that?  It's more about thinking clearly about the problem than it is about language skills.  See above though.

	Masatoshi Kimura [:emk] Assignee
	Comment 14 • 10 years ago

- Added some tests.
- Added telemetry.
- s/RC4/weak/ in the message.
- Renamed added members as suggested.
- Fall through if rememberStrongCiphers returns false.

Green on try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b74ee6ee10c6

	Masatoshi Kimura [:emk] Assignee
	Comment 15 • 10 years ago

Forgot to initialize strongCiphersFailed in the test.

	Martin Thomson [:mt:] Reporter
	Comment 16 • 10 years ago

Comment on attachment 8511618 [details] [diff] [review]
Stop offering RC4 in the first handshakes

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

I know that you weren't looking for a review from me, but this looks good.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +813,5 @@
> +    for (; cp->pref; ++cp) {
> +      if (cp->weak && cp->enabled) {
> +        weakCipherEnabled = true;
> +      }
> +    }

I don't think this second loop is needed.

@@ +1686,5 @@
> +      if (cp->enabled) {
> +        weakCipherEnabled = true;
> +      }
> +      // Do not enable weak ciphers by default
> +      continue;

Using `else` is probably better than continue

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +855,5 @@
>        entry.intolerant = entry.tolerant + 1;
>        entry.intoleranceReason = 0; // lose the reason
>      }
> +    if (!entry.strongCiphersWorked && !entry.strongCiphersFailed) {
> +      entry.strongCiphersWorked = true;

No need to check !strongCiphersWorked

@@ +937,5 @@
> +
> +  IntoleranceEntry entry;
> +  if (mTLSIntoleranceInfo.Get(key, &entry)) {
> +    entry.AssertInvariant();
> +    if (entry.strongCiphersWorked) {

Looking at this block of code, I wonder if an enum might be easier to follow:

enum {
  strong_cipher_worked,
  strong_cipher_failed,
  strong_cipher_unknown
} strongCipherResult;

(I'm unclear on the naming convention)

::: security/manager/ssl/tests/gtest/TLSIntoleranceTest.cpp
@@ +261,5 @@
>    helpers.rememberTolerantAtVersion(HOST, 1, SSL_LIBRARY_VERSION_TLS_1_2);
>    ASSERT_EQ(0, helpers.getIntoleranceReason(HOST, 1));
>  }
> +
> +TEST_F(TLSIntoleranceTest, Test_Strong_Ciphers_Failed)

it's not necessary to replicate the remainder of the test beyond probably the third block here.  You want to make sure that you go from strong to weak and you want to make sure that the progress continues correctly (so dropping to 1.1 makes sense), but dropping back to 1.0 and SSL3 just duplicates code.

::: toolkit/components/telemetry/Histograms.json
@@ +6384,5 @@
>    },
> +  "SSL_WEAK_CIPHERS_FALLBACK": {
> +    "expires_in_version": "never",
> +    "kind": "boolean",
> +    "description": "Fallback attempted when server did not support any strong ciphers"

s/ciphers/cipher suites/

	Masatoshi Kimura [:emk] Assignee
	Comment 17 • 10 years ago

(In reply to Martin Thomson [:mt] from comment #16)
> ::: security/manager/ssl/src/nsNSSComponent.cpp
> @@ +813,5 @@
> > +    for (; cp->pref; ++cp) {
> > +      if (cp->weak && cp->enabled) {
> > +        weakCipherEnabled = true;
> > +      }
> > +    }
> 
> I don't think this second loop is needed.

This second loop is needed if we found a strong cipher whose pref was changed before any other enabled weak ciphers.
Otherwise weakCipherEnabled will be turned off incorrectly.

I'll fix other style nits.

	Masatoshi Kimura [:emk] Assignee
	Comment 18 • 10 years ago

(In reply to Masatoshi Kimura [:emk] from comment #17)
> This second loop is needed if we found a strong cipher whose pref was
> changed before any other enabled weak ciphers.
> Otherwise weakCipherEnabled will be turned off incorrectly.

Ah, but I forgot the case a enabled weak cipher was found before pref-changed cipher. I'll fix it.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 19 • 10 years ago

Comment on attachment 8511618 [details] [diff] [review]
Stop offering RC4 in the first handshakes

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

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +624,5 @@
>    const char* pref;
>    long id;
>    bool enabledByDefault;
> +  bool weak;
> +  mutable bool enabled;

Why mutable? I think instead you meant to make the other members const.

@@ +693,5 @@
>  
>   { nullptr, 0 } // end marker
>  };
>  
> +static bool weakCipherEnabled;

If you keep this then you need to document how to use it safely in a multi-threaded manner. However, I suggest you take my suggestion below, and remove this and IsWeakCiupherEnabled, merging this into EnableWeakCiphers.

@@ +696,5 @@
>  
> +static bool weakCipherEnabled;
> +
> +//static
> +bool nsNSSComponent::IsWeakCipherEnabled()

Nit: Use the Mozilla Coding Style:

    /*static*/ bool
    nsNSSComponent::IsWeakCipherEnabled()

Don't you need to hold a mutex for this? Since weakCipherSpec is written on the main thread but read on the socket transport thread?

Please document the thread-safety of this function.

It might be better to merge this with EnableWeakCiphers, and have EnableWeakCiphers return a boolean indicating whether the fallback connection should be done.

@@ +701,5 @@
> +{
> +  return weakCipherEnabled;
> +}
> +
> +//static

Nit: Mozilla Style, and document the thread safety.

@@ +706,5 @@
> +void nsNSSComponent::EnableWeakCiphers(PRFileDesc* fd)
> +{
> +  for (const CipherPref* cp = sCipherPrefs; cp->pref; ++cp) {
> +    if (cp->weak && cp->enabled) {
> +      SSL_CipherPrefSet(fd, cp->id, cp->enabled);

Don't you need a mutex for this, like above?

@@ +792,5 @@
>    NS_ASSERTION(NS_IsMainThread(), "CipherSuiteChangeObserver::Observe can only be accessed in main thread");
>    if (nsCRT::strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID) == 0) {
>      NS_ConvertUTF16toUTF8  prefName(someData);
>      // Look through the cipher table and set according to pref setting
> +    weakCipherEnabled = false;

Please write a comment explaining the purpose of this code.

@@ +809,4 @@
>          SSL_ClearSessionCache();
>          break;
>        }
>      }

Please add a comment here explaining why the second loop is needed.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +937,5 @@
> +
> +  IntoleranceEntry entry;
> +  if (mTLSIntoleranceInfo.Get(key, &entry)) {
> +    entry.AssertInvariant();
> +    if (entry.strongCiphersWorked) {

enum StrongCipherStatus {
   UNKNOWN,
   WORKED,
   FAILED
};

I agree that an enum is clearer.

	Masatoshi Kimura [:emk] Assignee
	Comment 20 • 10 years ago

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #19)
> It might be better to merge this with EnableWeakCiphers, and have
> EnableWeakCiphers return a boolean indicating whether the fallback
> connection should be done.

The problem is that EnableWeakCiphers must be called on a new connection after we decided we need a fallback. Is there a way to get around this?

	Masatoshi Kimura [:emk] Assignee
	Comment 21 • 10 years ago

Resolved review comments.

I've completely rewritten the logic about enabled weak cipher suites. Now EnableWeakCiphers and IsWeakCipherEnabled are thread-safe. I also replaced the boolean pair with an enum.

	Martin Thomson [:mt:] Reporter
	Comment 22 • 10 years ago

Comment on attachment 8511936 [details] [diff] [review]
Stop offering RC4 in the first handshakes, v4

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

Nits only.

I think that the tests could be cut back a tiny bit.  Not because I hate testing, but because there is a fair bit of unnecessary code duplication in there now.  I like that these are very thorough though and will defer to :keeler's judgment.

::: security/manager/ssl/tests/gtest/TLSIntoleranceTest.cpp
@@ +275,5 @@
> +    StrongCipherStatus strongCipherStatus = helpers.UNKNOWN;
> +    helpers.adjustForTLSIntolerance(HOST, PORT, range, strongCipherStatus);
> +    ASSERT_EQ(SSL_LIBRARY_VERSION_3_0, range.min);
> +    ASSERT_EQ(SSL_LIBRARY_VERSION_TLS_1_2, range.max);
> +    ASSERT_EQ(helpers.UNKNOWN, strongCipherStatus);

Everything above here is just duplicated from elsewhere.  Try to limit the amount of new test code.

@@ +375,5 @@
> +}
> +
> +TEST_F(TLSIntoleranceTest, Test_Weak_Ciphers_Fallback_Only_Once)
> +{
> +  helpers.mVersionFallbackLimit = SSL_LIBRARY_VERSION_3_0;

Rather than duplicate the code in Test_1_2_through_3_0, I think that perhaps you should change that function to test the entire range of fallback (i.e., keep this test and delete the other).  A name like Test_Full_Fallback_Process might be good.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 23 • 10 years ago

Comment on attachment 8511936 [details] [diff] [review]
Stop offering RC4 in the first handshakes, v4

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

This looks good. I think there should be a little more documentation on, for example, how sEnabledWeakCiphers works and what it means. I also made some suggestions in terms of naming functions. In particular, we should be careful to distinguish between weak ciphers that are enabled in preferences but not used by default on TLS sockets and enabling those ciphers for a specific socket. Also, I agree with :mt's comments on the tests. r=me with comments addressed.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +697,5 @@
> +static_assert(MOZ_ARRAY_LENGTH(sCipherPrefs) - 1 <= sizeof(uint32_t) * CHAR_BIT,
> +              "too many cipher suites");
> +
> +/*static*/ bool
> +nsNSSComponent::IsWeakCipherEnabled()

Maybe "AreAnyWeakCiphersEnabled"? Or "AtLeastOneWeakCipherEnabled"? (although I guess that's a bit cumbersome) (The issue I'm trying to address here is that "IsWeakCipherEnabled" would seem to refer to one specific cipher, whereas what we're asking is if any of the ciphers we think are weak are enabled.)

@@ +703,5 @@
> +  return !!sEnabledWeakCiphers;
> +}
> +
> +/*static*/ void
> +nsNSSComponent::EnableWeakCiphers(PRFileDesc* fd)

Maybe "UseWeakCiphersOnSocket"? The issue I'm to address here is that there's a difference between a weak cipher that is enabled in preferences but hasn't been enabled for use on any TLS sockets. Since this function does the latter, I think we should use slightly different terminology.

@@ +707,5 @@
> +nsNSSComponent::EnableWeakCiphers(PRFileDesc* fd)
> +{
> +  const uint32_t enabledWeakCiphers = sEnabledWeakCiphers;
> +  for (const CipherPref* cp = sCipherPrefs; cp->pref; ++cp) {
> +    if (enabledWeakCiphers & ((uint32_t)1 << (cp - sCipherPrefs))) {

Instead of doing pointer subtraction here, let's use an index to iterate across sCipherPrefs and then use that same integer to access the bits of sEnabledWeakCiphers.

@@ +794,5 @@
>    NS_ASSERTION(NS_IsMainThread(), "CipherSuiteChangeObserver::Observe can only be accessed in main thread");
>    if (nsCRT::strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID) == 0) {
>      NS_ConvertUTF16toUTF8  prefName(someData);
>      // Look through the cipher table and set according to pref setting
>      for (const CipherPref* cp = sCipherPrefs; cp->pref; ++cp) {

Same here in terms of indexing vs. pointer arithmetic.

@@ +798,5 @@
>      for (const CipherPref* cp = sCipherPrefs; cp->pref; ++cp) {
>        if (prefName.Equals(cp->pref)) {
>          bool cipherEnabled = Preferences::GetBool(cp->pref,
>                                                    cp->enabledByDefault);
> +        if (cp->weak) {

We should probably include a note that weak ciphers that are enabled in prefs aren't actually used by default, and are only used as part of a fallback mechanism (hence SSL_CipherPrefSetDefault won't be called for them, which is a bit surprising when first reading this code).

@@ +1683,1 @@
>    for (const CipherPref* cp = sCipherPrefs; cp->pref; ++cp) {

Same idea with using an index instead of doing pointer arithmetic.

::: security/manager/ssl/src/nsNSSIOLayer.h
@@ +197,5 @@
>    void setWarnLevelMissingRFC5746(int32_t level);
>    int32_t getWarnLevelMissingRFC5746();
>  
> +  enum StrongCipherStatus {
> +    UNKNOWN,

How about StrongCiphersStatusUnknown, StrongCiphersWorked, StrongCiphersFailed or something? (that way, when accessing these, it'll be nsSSLIOLayerHelpers::StrongCiphersFailed, etc. instead of nsSSLIOLayerHelpers::FAILED, which isn't very descriptive.

	Masatoshi Kimura [:emk] Assignee
	Comment 24 • 10 years ago

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7a9ab4746066
- Added some comments.
- Fixed function names as suggested.
- Changed pointer arithmetic to an index.
- Reduced the code duplication in the test.
- Merged to bug 1088950.

	Carsten Book [:Tomcat]
	Comment 26 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c7ca0dc4155

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/2c7ca0dc4155

	sjw
	Comment 28 • 10 years ago

Verified on various sites, that prefer RC4. Sites that requires RC4 are still working. Thank you for fixing this.

	Simon
	Comment 29 • 10 years ago

Can we get uplifted into firefox 35

	Dave Garrett
	Comment 30 • 10 years ago

Yeah, an Aurora uplift might be worth considering. Needinfoing...

	Masatoshi Kimura [:emk] Assignee
	Comment 31 • 10 years ago

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: It will be delayed to collect data to drop support for RC4.
[Describe test coverage new/current, TBPL]: on m-c
[Risks and why]: The compatibility risk would be minimal. Servers that require RC4 will still work.
[String/UUID change made/needed]: none

	Anne (:annevk)
	Comment 32 • 10 years ago

https://panel.dreamhost.com/ no longer works. Requires RC4 over TLS 1.0. See bug 1092952.

	Masatoshi Kimura [:emk] Assignee
	Comment 33 • 10 years ago

Comment on attachment 8515582 [details] [diff] [review]
patch for aurora

Canceling the uplift request because multiple regressions are reported.

	Masatoshi Kimura [:emk] Assignee
	Comment 34 • 10 years ago

Both reported sites are TLS intolerant, so bug 1084025 will break them anyway.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 35 • 10 years ago

(In reply to Masatoshi Kimura [:emk] from comment #34)
> Both reported sites are TLS intolerant, so bug 1084025 will break them
> anyway.

I don't think you should depend on that logic. Bug 1084025 hasn't landed and could easily get backed out or limited.

The patch for this bug shouldn't be causing compatibility issues. If it is, that's a bug in the patch. What is the patch doing? Is it doing this:

    1. TLS 1.2 w/o RC4
    2. TLS 1.2 w/ RC4
    3. TLS 1.1 w/ RC4
    4. TLS 1.0 w/ RC4

Or is it doing this:

    1. TLS 1.2 w/o RC4
    2. TLS 1.2 w/ RC4
    3. TLS 1.1 w/o RC4
    4. TLS 1.0 w/o RC4

It should be doing the first one, but from the above comments, it sounds like it is doing the second one. That's a bug.

	Masatoshi Kimura [:emk] Assignee
	Comment 36 • 10 years ago

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #35)
> (In reply to Masatoshi Kimura [:emk] from comment #34)
> > Both reported sites are TLS intolerant, so bug 1084025 will break them
> > anyway.
> 
> I don't think you should depend on that logic. Bug 1084025 hasn't landed and
> could easily get backed out or limited.

Sorry, comment #34 was wrong. Both servers are TLS tolerant. I'm confused by my local WIP patch...

> The patch for this bug shouldn't be causing compatibility issues. If it is,
> that's a bug in the patch. What is the patch doing? Is it doing this:
> 
>     1. TLS 1.2 w/o RC4
>     2. TLS 1.2 w/ RC4
>     3. TLS 1.1 w/ RC4
>     4. TLS 1.0 w/ RC4
> 
> Or is it doing this:
> 
>     1. TLS 1.2 w/o RC4
>     2. TLS 1.2 w/ RC4
>     3. TLS 1.1 w/o RC4
>     4. TLS 1.0 w/o RC4
> 
> It should be doing the first one, but from the above comments, it sounds
> like it is doing the second one. That's a bug.

It's doing:
     1. TLS 1.2 w/o RC4
     2. TLS 1.1 w/o RC4
     3. TLS 1.0 w/o RC4

Because the RC4 fallback logic works only if the NSS error code was SSL_ERROR_NO_CYPHER_OVERLAP.

	sjw
	Comment 37 • 9 years ago

Can we get a release note or even a blog post to share this?


Release Note Request (optional, but appreciated)

[Why is this notable]:
This is a great security improvement and everyone should know, that RC4 is weak.

[Suggested wording]:
Firefox do no longer accept insecure RC4 ciphers whenever possible.

[Links (documentation, blog post, etc)]:
Would be great to have a blog post for this, too.

	Dave Garrett
	Comment 38 • 9 years ago

(In reply to sjw from comment #37)
> [Why is this notable]:
> This is a great security improvement and everyone should know, that RC4 is
> weak.
> 
> [Suggested wording]:
> Firefox do no longer accept insecure RC4 ciphers whenever possible.

-> and some particularly broken servers cease to function with this. (bug 1116891)

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 39 • 9 years ago

(In reply to sjw from comment #37)
> [Why is this notable]:
> This is a great security improvement and everyone should know, that RC4 is
> weak.

This doesn't improve security very much, actually, because a MitM can still trigger the fallback handshake where RC4 will be enabled.

	Yuhong Bao
	Comment 40 • 9 years ago

It is intended to prevent passive spying using RC4 biases that is possible if you have access to the backbone for example.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 41 • 9 years ago

(In reply to Yuhong Bao from comment #40)
> It is intended to prevent passive spying using RC4 biases that is possible
> if you have access to the backbone for example.

Such completely passive attacks would be very difficult to execute, even if RC4 was much more broken than we thought. Also, this patch makes some active attacks, like POODLE and BEAST-style attacks, easier for most of the servers it has an effect on, since it causes them to select CBC-mode cipher suites instead of RC4 cipher suites. (If they support AES-GCM cipher suites then they're usually already preferring them over RC4 or CBC.)

Also, IIRC, this patch reduced the use of forward secrecy by a few percentage points, because the ECDHE-RSA-RC4 cipher suite was disabled on initial handshakes.

So, it is far from clear whether the real security impact of this change was positive or negative in the short-term. In the long term, I do think it's a positive change.

	Dave Garrett
	Comment 42 • 9 years ago

The main security impact is an increase in the ease of phasing out RC4 entirely.

Nobody is yet willing to disable RC4 outright. Phasing it out like this will let more connections not use RC4, if other ciphers are available. Once this is in the release for a while, we'll be able to get real telemetry on how much RC4 is depended on, not just used by badly configured servers that support better.

	sjw
	Comment 43 • 9 years ago

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #39)
> (In reply to sjw from comment #37)
> > [Why is this notable]:
> > This is a great security improvement and everyone should know, that RC4 is
> > weak.
> 
> This doesn't improve security very much, actually, because a MitM can still
> trigger the fallback handshake where RC4 will be enabled.

What about the fix in bug 1084025?

	Masatoshi Kimura [:emk] Assignee
	Comment 44 • 9 years ago

(In reply to sjw from comment #43)
> > This doesn't improve security very much, actually, because a MitM can still
> > trigger the fallback handshake where RC4 will be enabled.
> 
> What about the fix in bug 1084025?

It only disables TLS *version* intolerance fallback. Disabling RC4 fallback means disabling RC4 outright.
The aim of this bug is to be an intermediate step to disable RC4 completely. Bug 1093595 was impossible to land if 25% of secure pages suddenly turned to broken.
Anyway, we should add relnote and blog post to make our stance clear (we are on the load to kill RC4).

	Sylvestre Ledru [:Sylvestre]
	Comment 45 • 9 years ago

Added to the release notes with "No longer accept insecure RC4 ciphers whenever possible" as wording.
A documentation on this would be great!

	Jean-Yves Perrier [:teoli]
	Comment 46 • 9 years ago

I added a note there:
https://developer.mozilla.org/en-US/Firefox/Releases/36#Security

Not sure we need more documentation (I don't think we have a list of ciphers we support in the different version of TLS/SSL)

Is it worth maintaining?

	Masatoshi Kimura [:emk] Assignee
	Comment 47 • 9 years ago

> Also, RC4 is now offered in the initial handshake of TLS 1.2

RC4 is *no longer* offered in the initial handshake. Also, "TLS 1.2" is not needed because RC4 fallback may occur in lower versions.

	Jean-Yves Perrier [:teoli]
	Comment 48 • 9 years ago

Arrgh, thanks! It was I wanted to wrote, but my brain stopped working. Silly me. Sorry, fixed.

	Anne (:annevk)
	Comment 49 • 9 years ago

(In reply to Jean-Yves Perrier [:teoli] from comment #46)
> Is it worth maintaining?

Yes, we should absolutely document that on MDN. Just as important as documenting CSS properties. We have bug 1073668 on file to get TLS documented on MDN, though not much progress yet.

	Yuhong Bao
	Comment 50 • 9 years ago

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #41)
> (In reply to Yuhong Bao from comment #40)
> > It is intended to prevent passive spying using RC4 biases that is possible
> > if you have access to the backbone for example.
> 
> Such completely passive attacks would be very difficult to execute, even if
> RC4 was much more broken than we thought. Also, this patch makes some active
> attacks, like POODLE and BEAST-style attacks, easier for most of the servers
> it has an effect on, since it causes them to select CBC-mode cipher suites
> instead of RC4 cipher suites. (If they support AES-GCM cipher suites then
> they're usually already preferring them over RC4 or CBC.)
> 
> Also, IIRC, this patch reduced the use of forward secrecy by a few
> percentage points, because the ECDHE-RSA-RC4 cipher suite was disabled on
> initial handshakes.
> 
> So, it is far from clear whether the real security impact of this change was
> positive or negative in the short-term. In the long term, I do think it's a
> positive change.

Funny that the BAR-MITZVA attack was mentioned only weeks after this comment:
https://www.blackhat.com/asia-15/briefings.html#bar-mitzva-attack-breaking-ssl-with-13-year-old-rc4-weaknes