184940 - Enable additional SSL ciphers, add configuration UI

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, enhancement)

Description

[Kai Engert (:KaiE:)]

The intention of this bug is to:
- make Mozilla's SSL code use additional ciphers available in NSS
- provide an appropriate UI to control which ciphers are enabled
- enable additional new ciphers by default

The dialog shown "Edit/Preferences/Privacy/SSL/Edit Ciphers" should get
changed appropriately.

(This bug was originally filed as bug 102633, and some historical information
can be found there. However, because we are now using a different strategy for
implementation, and given the fact the other bug is already long and complex,
I've decided to use a fresh bug.)

Comment 1

[Kai Engert (:KaiE:)]

*** Bug 102633 has been marked as a duplicate of this bug. ***

Comment 2

[Kai Engert (:KaiE:)]

Instead of the complex UI using scrolling checkboxes attempted in bug 102633, we
would like to use a simple UI that avoid scrolling, but distributes the
checkboxes on three panes in a tabbed dialog.

The wording for the checkboxes (including for the new ciphers) has been cleaned
up and optimized by Nelson. Thanks!

I'm reusing some code from the the former patches in bug 102633. We provide the
new ability to open a small detailed information dialog that provides more
information about a cipher in a structured layout.

Here is the list of new ciphers and the names of their preference strings, that
will be ENABLED BY DEFAULT:
security.ssl3.rsa_rc4_128_sha      SSL_RSA_WITH_RC4_128_SHA
security.ssl3.dhe_rsa_aes_256_sha  TLS_DHE_RSA_WITH_AES_256_CBC_SHA
security.ssl3.dhe_dss_aes_256_sha  TLS_DHE_DSS_WITH_AES_256_CBC_SHA
security.ssl3.rsa_aes_256_sha      TLS_RSA_WITH_AES_256_CBC_SHA
security.ssl3.dhe_rsa_aes_128_sha  TLS_DHE_RSA_WITH_AES_128_CBC_SHA
security.ssl3.dhe_dss_aes_128_sha  TLS_DHE_DSS_WITH_AES_128_CBC_SHA
security.ssl3.rsa_aes_128_sha      TLS_RSA_WITH_AES_128_CBC_SHA
security.ssl3.dhe_rsa_des_ede3_sha SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA
security.ssl3.dhe_dss_des_ede3_sha SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA
security.ssl3.dhe_rsa_des_sha      SSL_DHE_RSA_WITH_DES_CBC_SHA
security.ssl3.dhe_dss_des_sha      SSL_DHE_DSS_WITH_DES_CBC_SHA

For completenes, here is one new cipher that will be supported, but will be
DISABLED by default:
security.ssl3.rsa_null_sha         SSL_RSA_WITH_NULL_SHA

Comment 3

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Comment 4

[Kai Engert (:KaiE:)]

Attachment: Screenshot 1/4

Comment 5

[Kai Engert (:KaiE:)]

Attachment: Screenshot 2/4

Comment 6

[Kai Engert (:KaiE:)]

Attachment: Screenshot 3/4

Comment 7

[Kai Engert (:KaiE:)]

Comment on attachment 109069 [details]
Screenshot 1/4

Marking obsolete, wrong content-type used...

Comment 8

[Kai Engert (:KaiE:)]

Attachment: Screenshot 4/4

Comment 9

[Kai Engert (:KaiE:)]

Attachment: Screenshot 1/4

Comment 10

[Kai Engert (:KaiE:)]

Attachment: Screenshot 2/4

Comment 11

[Kai Engert (:KaiE:)]

Comment on attachment 109067 [details] [diff] [review]
Patch v1

Javi, can you please review?

Comment 12

[Kai Engert (:KaiE:)]

*** Bug 144510 has been marked as a duplicate of this bug. ***

Comment 13

[Kai Engert (:KaiE:)]

*** Bug 85788 has been marked as a duplicate of this bug. ***

Comment 14

[Javier Delgadillo]

Comment on attachment 109067 [details] [diff] [review]
Patch v1

r=javi

Comment 15

[Kai Engert (:KaiE:)]

Comment on attachment 109067 [details] [diff] [review]
Patch v1

Jag, can you please review?

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Kai,  Do you have a binary for Win2K you want me to test?

Comment 17

[Kai Engert (:KaiE:)]

I have uploaded a win32 test build to:
  http://www.kuix.de/mozilla/mozilla-win32-184940.zip

Just unzip and run bin/mozilla.exe

md5sum:
f407e14b4faf0222efcd1db2bf6209a8 *mozilla-win32-184940.zip

Comment 18

[Kai Engert (:KaiE:)]

Sean looked at the screenshots, and asks, whether we should change
"Ciphersuites" into "Cipher Suites".

Nelson, do you have a preference? Sean suggests, we could leave the current
wording "Ciphersuites" if it appears to be more common.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

I used the term "ciphersuites" (no space) because that is the term defined 
in the SSL3 and TLS specifications.  However, for localization purposes, 
I'm sure it will be less confusing if it's separated into two words.

Comment 20

[Wan-Teh Chang]

The TLS RFC uses both "cipher suite" and "CipherSuite"
(note the capitalization).  I think the latter is the
name of a type in the formal protocol specification.
In plain English prose I think it is better to use the
former.

Comment 21

[Kai Engert (:KaiE:)]

Thanks for your comments. After the superreview, I will change it to "cipher suite".

Comment 22

[jag (Peter Annema)]

Comment on attachment 109067 [details] [diff] [review]
Patch v1

>Index: mozilla/security/manager/pki/resources/content/cipherinfo.js
>===================================================================

>+  var info_name;
>+  var info_encryption;
>+  var info_authAlg;
>+  var info_keyAlg;
>+  var info_keySize;
>+  var info_macAlg;
>+  var info_fips;
>+  var info_exportable;
>+
>+  info_name = document.getElementById("name");
>+  info_encryption = document.getElementById("encryption");
>+  info_authAlg = document.getElementById("authAlg");
>+  info_keyAlg = document.getElementById("keyAlg");
>+  info_keySize = document.getElementById("keySize");
>+  info_macAlg = document.getElementById("macAlg");
>+  info_fips = document.getElementById("fips");
>+  info_exportable = document.getElementById("exportable");

This is a little odd, I'd merge these two blocks.

>+    if (typeof(prefValue) == "string") prefValue = (prefValue == "true");

Split this into two lines.

>Index: mozilla/security/manager/ssl/src/nsCipherInfo.cpp
>===================================================================

>+nsCipherInfoService::nsCipherInfoService()
>+{
>+  NS_INIT_ISUPPORTS();

NS_INIT_ISUPPORTS() is no longer needed (see its definition).

Comment 23

[Kai Engert (:KaiE:)]

Attachment: Patch v2

New patch with all requests addressed.

Comment 24

[Kai Engert (:KaiE:)]

Comment on attachment 110829 [details] [diff] [review]
Patch v2

carrying forward reviews

Comment 25

[Kai Engert (:KaiE:)]

Updating summary to describe better what we are doing.

Comment 26

[Kai Engert (:KaiE:)]

Checked in, marking fixed.

Comment 27

[John Unruh]

Reopening.

Comment 28

[Kai Engert (:KaiE:)]

John, I think something must have gone wrong when you reopened the bug.
You reopened without explaining why. I don't understand why you did.
You added a dependency to unconfirmed bug 190160, but I don't see any relation.

I'm removing the dependency and marking the bug fixed again.

Comment 29

[John Unruh]

Verified. I must not have had my coffee yet and thought I was on a different bug.