511808 - Disable Camellia cipher on Windows CE until 508113 can be checked in

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: don't enable the camellia ciphers on wince

This is a small bandaid for avoiding crashes on Windows CE (which doesn't do fixup for unaligned data access), until we can figure out a way to get bug 508113 checked in (which might be bug 511743).  I want to get this in so that people building from unmodified source don't get a crashy build.

Comment 1

[Jason Duell]

no sr needed

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Thanks; I'll do the checkin myself shortly.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Fixed on m-c and 1.9.2.

Comment 4

[Wan-Teh Chang]

Vlad, I believe that you also need to comment out the
Camellia entries in the CipherPrefs array in
security/manager/ssl/src/nsNSSComponent.cpp.  See

http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSComponent.cpp#1670

1650       PRBool enabled;
           ...
1670       // Now only set SSL/TLS ciphers we knew about at compile time
1671       for (CipherPref* cp = CipherPrefs; cp->pref; ++cp) {
1672         mPrefBranch->GetBoolPref(cp->pref, &enabled);
1673 
1674         SSL_CipherPrefSetDefault(cp->id, enabled);
1675       }

I am not sure if mPrefBranch->GetBoolPref() sets |enabled|
to PR_FALSE when the preference doesn't exist.  Note that
we aren't checking the return value of mPrefBranch->GetBoolPref().
I suspect that you may be depending on the two cipher suites
listed before the Camellia cipher suites in CipherPrefs being
disabled by default:

"security.ssl3.rsa_rc2_40_md5": weak encryption
"security.ssl3.ecdh_rsa_null_sha": no encryption

I think we should comment out the Camellia cipher suites
in CipherPrefs for WINCE, as well as checking the return
value of mPrefBranch->GetBoolPref(), like this:

      // Now only set SSL/TLS ciphers we knew about at compile time
      for (CipherPref* cp = CipherPrefs; cp->pref; ++cp) {
        rv = mPrefBranch->GetBoolPref(cp->pref, &enabled);
        if (NS_SUCCEEDED(rv))
          SSL_CipherPrefSetDefault(cp->id, enabled);
      }

Comment 5

[Wan-Teh Chang]

To verify my theory, please set the values of these
two preferences to "true":

  "security.ssl3.rsa_rc2_40_md5"
  "security.ssl3.ecdh_rsa_null_sha"

and see if your workaround still works.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: followup patch

Ah, good catch -- I forgot to check whether we were checking whether the pref even existed.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: followup patch, better

This is better.

Comment 8

[Johnathan Nightingale [:johnath]]

Comment on attachment 396799 [details] [diff] [review]
followup patch, better

Checking for failure makes sense, and the default on failure preserves existing behaviour.

I'm not a peer here, but have taken "trivial" reviews in the past. r=me, but I'll cc kaie and rrelyea to make sure they see this,

Comment 9

[Wan-Teh Chang]

Comment on attachment 396799 [details] [diff] [review]
followup patch, better

r=wtc.  The reason the first patch (attachment 396796 [details] [diff] [review]) also works is
that in the for loop before this for loop, we've already disabled all
the implemented SSL/TLS ciphers:

http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSComponent.cpp#1663

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked followup in on trunk and m-c.

Comment 11

[Wan-Teh Chang]

Question: is m-c (mozilla-central?) not the trunk?

Comment 12

[Johnathan Nightingale [:johnath]]

(In reply to comment #11)
> Question: is m-c (mozilla-central?) not the trunk?

He meant m-c and m-1.9.2.  m-c is, indeed, trunk.