102633 - PSM should allow the user to enable/disable additional available ciphers

Status: VERIFIED DUPLICATE of bug 184940

(Core Graveyard :: Security: UI, defect, P2)

Description

[(Don't use, old account) - Kai Engert]

"Edit/Preferences/Privacy/SSL/Edit Ciphers" shows a list of ciphers, but not all
available ciphers.

Here is a list of what we need to do, what I'd like to know to be able to
implement this.

1.)
===

PSM does not provide a user interface to enable/disable the following ciphers:

SSL_RSA_WITH_RC4_128_SHA
SSL_DHE_RSA_WITH_DES_CBC_SHA
SSL_DHE_DSS_WITH_DES_CBC_SHA
SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA
SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA
TLS_DHE_DSS_WITH_RC4_128_SHA
SSL_FORTEZZA_DMS_WITH_FORTEZZA_CBC_SHA
SSL_FORTEZZA_DMS_WITH_RC4_128_SHA
SSL_FORTEZZA_DMS_WITH_NULL_SHA

Should all of them be selectable in the user interface?


2.)
===

I notice that the user interface orders the ciphers by strength in the
enable/disable user interface (in two sections).

SSL 2
 10 SSL_EN_RC4_128_WITH_MD5
 20 SSL_EN_RC2_128_CBC_WITH_MD5
 30 SSL_EN_DES_192_EDE3_CBC_WITH_MD5
 40 SSL_EN_DES_64_CBC_WITH_MD5
 50 SSL_EN_RC4_128_EXPORT40_WITH_MD5
 60 SSL_EN_RC2_128_CBC_EXPORT40_WITH_MD5

SSL 3
 70 SSL_RSA_WITH_RC4_128_MD5
 80 SSL_RSA_FIPS_WITH_3DES_EDE_CBC_SHA
 90 SSL_RSA_WITH_3DES_EDE_CBC_SHA
100 SSL_RSA_FIPS_WITH_DES_CBC_SHA
110 SSL_RSA_WITH_DES_CBC_SHA
120 TLS_RSA_EXPORT1024_WITH_RC4_56_SHA
130 TLS_RSA_EXPORT1024_WITH_DES_CBC_SHA
140 SSL_RSA_EXPORT_WITH_RC4_40_MD5
150 SSL_RSA_EXPORT_WITH_RC2_CBC_40_MD5
160 SSL_RSA_WITH_NULL_MD5


At which positions should we insert the ciphers from 1.) in the user interface?

For example,
 75 SSL_RSA_WITH_RC4_128_SHA
?


3.)
===
We need user friendly plain text descriptions of the missing ciphers as listed
in 1.)

Example:

SSL_RSA_WITH_RC4_128_SHA
  "RC4 encryption with a 128-bit key and SHA-1 MAC"
?

Comment 1

[Stephane Saux]

->future
cc lord.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Don't forget the AES ciphers.

Comment 3

[(Don't use, old account) - Kai Engert]

Changing my prefered e-mail address.

Comment 4

[Kai Engert (:KaiE:)]

Nelson, you say I shouldn't forget the AES ciphers. I built that list by looking
at the ciphers that PSM currently enables during startup.

I looks like PSM currently does not have any reference to AES at all.

This means, if by default AES ciphers are not enabled by default, Mozilla does
not use AES ciphers currently.

Can we build a list of ciphers that should be enabled in Mozilla?

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

I was misremembering bug 95987.  
http://bugzilla.mozilla.org/show_bug.cgi?id=95987
I thought that bug was about AES ciphersuites, but I see now that it was
about the DHE ciphersuites.

In any case, the AES suites should also be enabled, and the UI should 
allow them to be disabled.  

All the SSL3 ciphersuites now supported by NSS should be candidates for
use in PSM, except those that do not encrypt, but only verify integity
and authenticity.

Comment 6

[Wan-Teh Chang]

The AES ciphersuites will be in NSS 3.4.
PSM is using NSS 3.3 branch right now,
which doesn't have the AES ciphersuites.

Comment 7

[Kai Engert (:KaiE:)]

Nominating for 2.2.

Mozilla has already the code that enables DHE ciphers. Nelson suggested the
application should allow the user to disable any available cipher, to empower a
user to disable any ciphers considered "flawed".

Resolving this bug is only little coding work. I can implement it as soon as I
have the answers to my questions (see top of this bug).

To make my questions clearer:

1) We agreed that all ciphers should be selectable. Should ciphers with FORTEZZA
be selectable in the UI, too?

2) Who can help defining the right order of cipher strengh? To which section
(SSL2/SSL3/TLS) should the ciphers from 1) be added?

3) Who can help define the cleartext strings for those ciphers? Can I simply
create new cleartext names using common sense, or are they well defined in some
document?
I wonder if we should implement this feature using a different approach. Instead
of hard coding the list of ciphers into the UI, we could use a dynamic UI that
is built from the list of ciphers the application decides to allow.

Comment 8

[Kai Engert (:KaiE:)]

=============
BUG EXTENSION
=============

I'm extending the scope of this bug. The current UI only lists RSA ciphers
without explicitly stating this fact.

The updated UI should include the string RSA for all RSA ciphers.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

libSSL already provides a list of all the implemented ciphersuites.
This list was intended to allow applications to see exactly what 
ciphersuites are implemented in libSSL at run time.  
The list is not presently sorted in any particular order (IIRC).

In http://bugzilla.mozilla.org/show_bug.cgi?id=78959, I have proposed the
addition of a new function, SSL_GetCipherSuiteInfo, that takes a ciphersuite 
number and returns name strings and other info about all the components of that 
ciphersuite, including such things as names of all the components 
- name of the entire ciphersuite              (e.g. SSL_RSA_WITH_RC4_128_MD5)
- name of the symmatric cipher,               (e.g., DES, RC4)
- name of the MAC,                            (e.g., MD5, SHA1)
- key sizes for symmetric cipher and mac,
- name of the key agreement algorithm,        (e.g., RSA, DHE, KEA)
- name of the server authentication algorithm (e.g., RSA, DSA)

That function, together with the list of ciphersuites, could be used to 
implement an always-up-to-date list of ciphersuites for the preferences
panel.  

I propose to sort the list of ciphersuites in the same order of preference
as is used by the SSL/TLS code itself.  

Please comment on my proposal in 
http://bugzilla.mozilla.org/show_bug.cgi?id=78959

Comment 10

[Stephane Saux]

Let's do that when we have 3.4

Comment 11

[John Unruh]

nsbeta1

Comment 12

[Kai Engert (:KaiE:)]

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

Comment 13

[Kai Engert (:KaiE:)]

Attachment: Screenshot of a suggested dynamic UI

This UI is dynamically rendered from the list of implemented ciphers that can
be obtained from NSS at runtime.

Comment 14

[Kai Engert (:KaiE:)]

Attachment: Patch

Comment 15

[Kai Engert (:KaiE:)]

This patch implements the previously shown UI.
The real dialog is smaller, and you need to scroll up-down to see all the ciphers.
I enlarged the dialog for the screenshot, to allow you to take a look at the
complete sort order that is the result of the dynamic code.

The patch adds support for allowing the user to configure all implemented ciphers.

The code does automatically generate preference strings for the ciphers.

In order to be backwards compatible regarding preferences, the preference
strings for those ciphers that were already supported in the past did not
change. Only for additional ciphers the preference strings are generated
automatically, for the other ciphers, the strings are hardcoded.

In order to activate additional ciphers we need to change the global preferences
file:
  netwerk/base/public/security-prefs.js

Current defaults are:
  pref("security.ssl2.des_64",        true);
  pref("security.ssl2.des_ede3_192",  true);
  pref("security.ssl2.rc2_128",       true);
  pref("security.ssl2.rc2_40",        true);
  pref("security.ssl2.rc4_128",       true);
  pref("security.ssl2.rc4_40",        true);
  pref("security.ssl3.rsa_1024_des_cbc_sha", true);
  pref("security.ssl3.rsa_1024_rc4_56_sha", true);
  pref("security.ssl3.rsa_des_ede3_sha", true);
  pref("security.ssl3.rsa_des_sha", true);
  pref("security.ssl3.rsa_fips_des_ede3_sha", true);
  pref("security.ssl3.rsa_fips_des_sha", true);
  pref("security.ssl3.rsa_null_md5", false);
  pref("security.ssl3.rsa_rc2_40_md5", true);
  pref("security.ssl3.rsa_rc4_128_md5", true);
  pref("security.ssl3.rsa_rc4_40_md5", true);

To the above file, we need to add default preferences for the following new
cipher pref strings:
  security.ssl3.rsa_dhe_aes_256_sha1
  security.ssl3.dsa_dhe_aes_256_sha1
  security.ssl3.rsa_rsa_aes_256_sha1
  security.ssl3.fortezza_rc4_sha
  security.ssl3.dsa_dhe_rc4_128_sha1
  security.ssl3.rsa_dhe_aes_128_sha1
  security.ssl3.dsa_dhe_aes_128_sha1
  security.ssl3.rsa_rsa_rc4_128_sha1
  security.ssl3.rsa_rsa_aes_128_sha1
  security.ssl3.rsa_dhe_3des_112_sha1
  security.ssl3.dsa_dhe_3des_112_sha1
  security.ssl3.fortezza_fortezza_sha
  security.ssl3.rsa_dhe_des_56_sha1
  security.ssl3.dsa_dhe_des_56_sha1
  security.ssl3.fortezza_null_sha

Comment 16

[Wan-Teh Chang]

Nelson, could you review the screen shot of Kai's proposed UI?

This fix should be in nsbeta1 because the new AES ciphersuites
for TLS can't be used without a UI to enable them.  I would also
propose the fix for mozilla1.0 because the new UI will allow
users to enable the DHE ciphersuites contributed by Dr. Henson,
one of the few major contributions made by people outside the
NSS team.

Comment 17

[Stephane Saux]

cc thayes

Comment 18

[Stephane Saux]

nsbeta1+ as per wtc's comment. Adding patch,review keywords.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Questions and comments about this UI.

1. Will this window display properly on a 640x480 screen?  
(I think all PSM windows should display properly on 640x480 screens)
Several of the columns appear to be much wider than the maximum width 
of the data displayed in those columns.  Maybe they can be made narrower.

2. I think each and every row in the table should be unique.  No two rows
should appear to be identical to the user.  Yet there are several rows 
that are repeated.

SSL3/TLS 3DES RSA RSA 112 SHA1 FIPS          appears twice.
SSL3/TLS  DES RSA RSA  56 SHA1 FIPS          appears 3 times, not consecutive.   

In these cases, there is probably additional info needed to explain the 
differences between the rows.  Some column should be avilable for that.

3. I'm not sure whether the FIPS column is supposed to mean 
a) This ciphersuite uses only FIPS approved algorithms,  or 
b) This ciphersuite uses only FIPS approved algorithms, AND NSS's 
implementation of those algorithms has been validated by NIST, or ???

If it means "a", then the AES cipher suites should also be marked FIPS
because AES is now a FIPS cipher.

4. There are 3 rows in this table that display KEA for the Authentication
algorithm.  These should only appear in the table if a fortezza token
(hardware or software) is present.

5. I recognize that some of these comments _may_ imply that changes are
needed to the info returned by SSL_GetChannelInfo, etc.

Comment 20

[Kai Engert (:KaiE:)]

> Questions and comments about this UI.
> 
> 1. Will this window display properly on a 640x480 screen?  

Yes. Note the screenshot does not show the default size.

Before making the screenshot, I enlarged the dialog,
to allow us to see all ciphers at once.

Normally, you will have to scroll to see all ciphers.


> 2. I think each and every row in the table should be unique.  No two rows
> should appear to be identical to the user.  Yet there are several rows 
> that are repeated.
> 
> In these cases, there is probably additional info needed to explain the 
> differences between the rows.  Some column should be avilable for that.

Good catch, I agree, we have to find out why there are duplicate rows.


> SSL3/TLS 3DES RSA RSA 112 SHA1 FIPS          appears twice.

These are generated by the following lines defined in sslinfo.c:

{0,CS(SSL_RSA_FIPS_WITH_3DES_EDE_CBC_SHA),    S_RSA, K_RSA, C_3DES,B_3DES,M_SHA,
1, },
{0,CS(SSL_RSA_WITH_3DES_EDE_CBC_SHA),         S_RSA, K_RSA, C_3DES,B_3DES,M_SHA,
1, },


> SSL3/TLS  DES RSA RSA  56 SHA1 FIPS          appears 3 times, not consecutive.   

{0,CS(SSL_RSA_FIPS_WITH_DES_CBC_SHA),         S_RSA, K_RSA, C_DES, B_DES, M_SHA,
1, },
{0,CS(SSL_RSA_WITH_DES_CBC_SHA),              S_RSA, K_RSA, C_DES, B_DES, M_SHA,
1, },
{0,CS(TLS_RSA_EXPORT1024_WITH_DES_CBC_SHA),   S_RSA, K_RSA, C_DES, B_DES, M_SHA,
1, },


Why are there multiple ciphers defined, that have, besides their names,
identical attributes?


> 3. I'm not sure whether the FIPS column is supposed to mean 
> a) This ciphersuite uses only FIPS approved algorithms,  or 
> b) This ciphersuite uses only FIPS approved algorithms, AND NSS's 
> implementation of those algorithms has been validated by NIST, or ???

Please look at the end of file sslinfo.c - By querying SSL_NumImplementedCiphers
and SSL_GetCipherSuiteInfo, I'm given the contents of the suiteInfo array,
and the last entries of those lines define a boolean "isFIPS".

I thought it makes sense to show that value.
I thought it means this attribute means, this cipher may be used while in FIPS mode.


> If it means "a", then the AES cipher suites should also be marked FIPS
> because AES is now a FIPS cipher.

In that case the table in sslinfo.c should get updated, and the UI will 
automatically display that.


> 4. There are 3 rows in this table that display KEA for the Authentication
> algorithm.  These should only appear in the table if a fortezza token
> (hardware or software) is present.

How I can query that availability?

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Here is a proposal for how to resolve these issues.  It has two parts.

1. Display another ciphersuite characteristic, the "is exportable", a.k.a. 
"is an export grade cipher suite" characteristic.  Many of the ciphersuites
in the list are old "export grade" (read: weaker) ciphersuites.  

In the late 1990s, the U.S. export regulations were changed, and 56-bit 
ciphers, including DES, that were formerly not exportable became exportable 
as long as they were used with public keys of 1024 bits or less.  

So, two new ciphersuites for SSL3 and TLS were created: 
TLS_RSA_EXPORT1024_WITH_RC4_56_SHA and TLS_RSA_EXPORT1024_WITH_DES_CBC_SHA

TLS_RSA_EXPORT1024_WITH_RC4_56_SHA is identical to the older non-export 
56-bit DES ciphersuite except that (a) it is exportable, and (b) it has that 
extra 1024-bit public key size limit.  If the UI showed the "is exportable" 
bit, then the exportable ciphersuite TLS_RSA_EXPORT1024_WITH_DES_CBC_SHA 
would be differentiated from the older "domestic" ciphersuite 
SSL_RSA_WITH_DES_CBC_SHA.  All the export ciphersuites should be distinguished
with some explicit identifier as being export ciphersuites, IMO.

I could add another bit to the struct SSLCipherSuiteInfoStr to show which
ciphersuites are exportable.  I'd suggest that, rather than adding another
column to the output, that the last column be changed to display other 
attributes, like FIPS or EXPORT.

2. There are two ciphersuites that are difficult to explain succinctly.  
They were added to NSS before RFC 2246 (the TLS RFC) was published, and 
before TLS support was added to NSS.  Now that NSS supports TLS, they are
obsolete (IMO).  They are SSL_RSA_FIPS_WITH_3DES_EDE_CBC_SHA and 
SSL_RSA_FIPS_WITH_DES_CBC_SHA.  When using TLS (as opposed to SSL v3.0), 
they are identical to SSL_RSA_WITH_3DES_EDE_CBC_SHA and 
SSL_RSA_WITH_DES_CBC_SHA respectively. 

I honestly believe that no-one in the world is using them, and I propose 
that we permanently disable them in PSM, and not display them in the UI.  
If additional discussion of this proposal is needed, let's not use this 
bug report as the vehicle for that discussion.  Please send me email
instead  Thanks.

Comment 22

[Stephane Saux]

rangan make sure your read this bug.

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Added dependency on bug 132438

Comment 24

[Kai Engert (:KaiE:)]

Jag, can you please have a look at nsCiphers::nsCiphers in the patch? When I use
this patch, an optimized build crashes inside that method. The debug build does
not crash.

The debugger claims it crashes in nsACString::do_AppendFromElementPtr, called
from nsCiphers::nsCiphers, but the rest of the debugger output looks damaged.

Is my string usage incorrect?

FYI, the variables info.*Name are const char*.

Thanks for looking.

Comment 25

[Kai Engert (:KaiE:)]

Jag, I think I know what happened, I was assuming that using operator= with a
string class would cause it to copy the assigned const char* pointer, I was
assuming that functionality, because it is used commonly in string classes from
other libraries I know. Anyway, I changed the code since yesterday, and I now
use Append instead of assignment, and I no longer crash.

Comment 26

[Kai Engert (:KaiE:)]

I changed the implementation of the cipher dialog completely, please see
attached new screenshot.

Multiple arguments for this change. 

I today learned that the user interface widget I was using to implement it will
go away very soon, so a re-work was necessary anyway.

I made some wrong assumptions about the distinguishability of ciphers.
While Nelson is helping me here, it is not sure whether the required change in
NSS will make it in time.
This new UI is independent of the change - although I'm using the information he
provides as implemented in bug 132438, I can easily remove that from the patch.

I built the preferences strings dynamically - I now decided to just use the name
of the cipher, this seems to be better.
If Nelson confirms that the names are unique, this sounds like the simpler
approach (because with the previous patch, I would have had to extend the
dynamic cipher preference name logic with the fips, export and non-standard
attributes).

I think one motivation for the cipher dialog is the ability for users to enable
ciphers.
And most likely, they will learn about ciphers to disable by hearing their
standard name.
That's why I'm now displaying that name in the UI, together with good
explanation of as much attributes as we know about.

Nelson explained that most of the new ciphers should be disabled by default.

In the prior version of the client, only a limited number of ciphers were in the UI.

For any new cipher that should become enabled by default, I would need to know
the exact name of that cipher, to be sure we talk about the same ciphers.

Thanks!

Comment 27

[Kai Engert (:KaiE:)]

Attachment: Screenshot of new UI suggestion

Comment 28

[Kai Engert (:KaiE:)]

Attachment: Patch implementing new UI

This patch uses the new bits provided in bug 132438. However, they are only
used to provide information to the user.

Comment 29

[jag (Peter Annema)]

Actually. operator= is supposed to copy, I suspect the problem is more subtle.
We don't have operator= for all Assign on all strings (since operator= can't be
inherited), and for some we have the generated operator= (which does a shallow
copy instead of a deep copy). I suspect you're running into one of these
problems (bugs exist on them) but for now it's safer to use .Assign, as you
already found.

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

Kai,  I like this new UI much better!

Comment 31

[Kai Engert (:KaiE:)]

Attachment: Updated patch

- filtering out fortezza ciphers
  They have always been disabled in Mozilla it seems.
  They will be activated soon, once we add correct Fortezza detection to PSM.

- filtering out old proprietary FIPS ciphers that should not longer be
supported

- rename "Exportable" to "USA Exportable"

- remove display of "non-standard" attribute

Comment 32

[Stephane Saux]

Nelson, Javi, Would you please formally review Kai's patch?
Thanks.

Comment 33

[Javier Delgadillo]

Comment on attachment 75956 [details] [diff] [review]
Updated patch

Move the call to SSL_GetCipherSuiteInfo outside of the NS_ASSERTION to ensure
it gets 
called in optimized builds as well.

>+          NS_ASSERTION(
>+            SECSuccess == 
>+              SSL_GetCipherSuiteInfo(cipher_id, &info, sizeof(SSLCipherSuiteInfo)), 
>+            "unable to get info for implemented cipher");
>+     

I don't see any Mac xml file changes for the Mac build.

Fix these two issues and r=javi

Comment 34

[Kai Engert (:KaiE:)]

Attachment: Updated patch

Thanks, Javi. I think you found the real reason for the crash I saw in the
optimized build.

This patch adds what you suggested.

Comment 35

[Kai Engert (:KaiE:)]

Sean, can you please review the string changes in the patch? Thanks.

(You can locate them by searching for "dtd" in the latest patch.)

Comment 36

[Kai Engert (:KaiE:)]

Attachment: Updated patch, includes suggestions from nelsonb

- added a comment that explains the cipher pref string logic
- enhanced sanity checking for matching expected struct size
- renamed a method

Comment 37

[Kai Engert (:KaiE:)]

Please note:

While the patch allows users to manually enable additional ciphers, none of
those additional ciphers will be enabled by default.

If we want any of the additional ciphers to become enabled by default, please
let me know, and I will create a separate patch to change
  mozilla/netwerk/base/public/security-prefs.js

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

I have reviewed the changes to the .h and .cpp files in the above patch.  
r=nelsonb   oX<-<   
             ^  
             | (me, bow tie signifies formal review. :)

I have some concerns about increasing the number of XPCOM idl interfaces 
required to implement the cipher selection UI, but those are outside the 
scope of this review.

Comment 39

[Kai Engert (:KaiE:)]

Comment on attachment 76184 [details] [diff] [review]
Updated patch, includes suggestions from nelsonb

patch has r=javi and portions r=nelsonb

Comment 40

[Kai Engert (:KaiE:)]

Alec, can you please review?

Comment 41

[Alec Flett]

Comment on attachment 76184 [details] [diff] [review]
Updated patch, includes suggestions from nelsonb



Also, this giant case statement, switch (cipher_id) would be much faster with a
hardcoded table that mapped a cipher_id to a pref.

in fact, the whole mCipherPrefs/mWantedCiphers	system could be implemented as
a static table, which would take up less memory and be faster, requiring no
initialization.

Also, nsIPref is deprecated, you should be using nsIPrefBranch (you can
QueryInterface nsIPref to nsIPrefBranch, or you can 

>+nsCiphers* nsCiphers::singleton = nsnull;
>+
>+void nsCiphers::InitSingleton()
>+{
>+  NS_ASSERTION(!singleton, "trying to instantiate nsCiphers::singleton twice");
>+
>+  singleton = new nsCiphers();
>+}
>+
>+void nsCiphers::DestroySingleton()
>+{
>+  delete singleton;
>+  singleton = nsnull;
>+}
>+
>+nsCiphers::nsCiphers()
>+{
>+  // count number of wanted ciphers
>+  
>+  mWantedCiphers = new PRBool[SSL_NumImplementedCiphers];
>+  mCipherPrefs = new CipherPref[SSL_NumImplementedCiphers];
>+

So this is fine, but you could make your code a WHOLE lot cleaner and less 
heap-intensive if you made SSL_NumImpementedCiphers into a #define, and then 
made member variables of nsCiphers like mWantedSpihers declared like

  PRBool mWantedCiphers[SSL_NumImplementedCiphers];

but since SSL_NumImplementedCiphers is a variable, you can't do this...


>+  if (!mCipherPrefs || !mWantedCiphers)
>+    return;
>+
>+  for (PRUint16 i = 0; i < SSL_NumImplementedCiphers; ++i)
>+  {
>+    PRUint16 cipher_id = SSL_ImplementedCiphers[i];
>+  
>+    switch (cipher_id)

in fact, this whole for-loop/switch statement is quite inefficient. you could
do this much
faster with some sort of static table
struct cipher_info {
  PRInt32 cipher_id;
  PRBool wanted;
  const char* pref;
};

then 
cipher_info cipher_defaults[] = {
{ { SSL_RSA_FIPS_WITH_3DES_EDE_CBC_SHA, PR_TRUE, "security.ssl2.rc4_128" },
  {...}

if the table is read-only, which it looks like to me, then you could just use
this table
as a direct reference, and you'd save yourself initialization time.


>+      default:
>+        {
>+          SSLCipherSuiteInfo info;
>+          SECStatus res = SSL_GetCipherSuiteInfo(cipher_id, &info, sizeof(info));


you'd handle this case, with some special flag, or |nsnull| for the pref name..

>+char *nsCiphers::GetPrefString(PRUint16 cipher_id)
>+{

this would be as simple as
{
 return ToNewCString(cipher_defaults[cipher_id].pref);
}



>Index: mozilla/security/manager/ssl/src/nsCipherInfo.h
>===================================================================
>+  static nsCiphers *singleton;
>+
>+  struct CipherPref {
>+    CipherPref() :id(0) {}
>+  
>+    PRUint16 id;
>+    nsCAutoString pref;

ack! autostrings should not be used inside structures, without at least a
comment 
to indicate why you're doing it. i.e. "safe because pref names are always < 64
characters,
so this saves us the extra allocation"

>+
>+private:
>+  PRBool good;
>+  SSLCipherSuiteInfo info;

These are member variables, and should begin with "m" as in "mGood" and "mInfo"
otherwise 
it's not clear in the implementation where "info." came from. (I was confused
until I 
saw this declaration)




>-static CipherPref CipherPrefs[] = {
>-/* SSL2 ciphers */
>- {"security.ssl2.rc4_128", SSL_EN_RC4_128_WITH_MD5},
>- {"security.ssl2.rc2_128", SSL_EN_RC2_128_CBC_WITH_MD5},
>- {"security.ssl2.des_ede3_192", SSL_EN_DES_192_EDE3_CBC_WITH_MD5},


oh my goodness! You already had this table! Why not use it? it seems perfect!

Comment 42

[Alec Flett]

argh, I added comments to the patch, but mozilla interrupted the submit before
the spam went out. Anyway, go read the comments :)

Comment 43

[Kai Engert (:KaiE:)]

Alec, I would like to veto some parts of your veto :)
But I realize my patch needs more explanation.

Mozilla and NSS are separate products, while Mozilla uses NSS as an external
library.

Up to now, Mozilla used a hardcoded UI, only offering some hard coded ciphers
for selection.

But we decided that with this approach, Mozilla is always "behind" NSS, because
for every added cipher in NSS we need to 
- become aware of that
- update the UI

Even worse, the number of ciphers is now large enough to justify a list with
ciphers, rather than using a dialog with static checkboxes.

You suggested that the "wanted" attribute should become part of a static table.
But I don't want a static table anymore, because that would mean, I again need
to hard code all ciphers in the code.

While there is some table of ciphers and their attributes in NSS, we can't add
the "wanted" attribute there, because what's wanted and what's not is a Mozilla
application decision, while NSS is used by many different applications.

The whole idea was, at runtime, query the available NSS library, and figure out
which encryption algorithms it supports. Filter out any algorithms that we don't
want to use for some reason, but keep all the others. Do not enable those
ciphers by default, but do allow users to enable those ciphers manually.

This also requires that preference strings for new ciphers must be created
dynamically. But we must be backwards compatible, and old cipher prefs, that
users will still have, must continue to work correctly, so we must continue to
use the old pref strings that have always been hard coded.

Therefore I think we don't have a choice and need to use the dynamic code.

Comment 44

[Kai Engert (:KaiE:)]

> ack! autostrings should not be used inside structures, without at least a
> comment 
> to indicate why you're doing it. i.e. "safe because pref names are always < 64
> characters,
> so this saves us the extra allocation"

Ok, I will change this to nsCString.

I think the pref strings will be <64 most of the time, but I can't guarantee that.

I thought, nsCAutoString is still save for usage in structs, because if the
string get's larger, a dynamic string will be used. But thinking about it, this
would cause 64 bytes to become wasted for each larger string, so I agree that
changing to nsCString is a good idea.


> These are member variables, and should begin with "m" as in "mGood" and "mInfo"
> otherwise 
> it's not clear in the implementation where "info." came from. (I was confused
> until I 
> saw this declaration)

Ok, sorry, changed.

Comment 45

[Nelson Bolyard (seldom reads bugmail)]

I agree that the intent here is to use NSS's information to build the 
UI dynamtically, rather than duplicating all that info in PSM, and thereby
doubling the maintenance burden (having to update PSM each time NSS is 
updated).  With Kai's dynamic UI code, if the user gets a new NSS DLL/DSO
that implements new ciphersuites, the UI will automatically show them, 
and will automatically create/handle preferences for them.

Comment 46

[Alec Flett]

ok, but your patch doesn't buy you any of that... the table is still read-only
and all you've done is move initialization of the table from a static page in
memory to a table that is initialized via C++ code. It's still a read-only table...

basically, you patch does two orthagonal things:
1) creates a way to enumerate the table of ciphers and gather data about each one
2) change the method of initialization of this table

I'm "vetoing" only the 2nd part. I think the interfaces and the XUL/JS are
great! The problem I see with doing initialization in the way you have, is that
maintenance of this list is going to be a huge headache, plus you've
signifigantly increased the complexity of the code for no real gain.

as a simple example, imagine this severely reduced case:

some_structure myArray[3];

for (i=0; i<3; i++) {
  some_structure& this_entry=myArray[i];
  switch (i) {
    case 0:
      this_entry.pref = "abc";
      break;
    case 1:
      this_entry.pref = "def";
      break;
    case 2:
      this_entry.pref = "ghi";
      break;
  } 
}

This is exactly the same as:

myArray[0].pref = "abc";
myArray[1].pref = "def";
myArray[2].pref = "ghi";

and do you see that this is equivalent to writing:

const some_structure myArray[] = 
{ { ...., "abc", ...} },
{ { ...., "def", ...} },
{ { ...., "ghi", ...} },

(the only difference being that this last case is read-only)

I do see the slight difference in that in SOME cases (i.e. the default:
fall-through to SSL_GetCipherSuiteInfo()) you need to dynamically fill in the
table, but you can either 
1) hide this dynamically behind the interface to this table by falling through
at runtime if the given entry in the table doesn't have the information you need.. 
2) doing a one-time copy of this table into a single structure in memory (i.e.
with memcpy()) and then tweaking the table if any information is missing.

Comment 47

[Nelson Bolyard (seldom reads bugmail)]

The numeric values of cipher_id are a sparse set, not numerically continuous.
So, rather than indexing the table myArray (the name from your example) 
with it, the cipher_id would be another entry in each row, and the table
would have to be searched.  I think the efficiency of that is comparable to 
the existing loop with switch. No?  

Comment 48

[Kai Engert (:KaiE:)]

> Also, nsIPref is deprecated, you should be using nsIPrefBranch (you can
> QueryInterface nsIPref to nsIPrefBranch, or you can 

I would like to ask we file a separate bug on this code maintenance issue, because:
- I'm not writing new code that accesses nsIPref
- in order to change all occurrences of nsIPref in the patch to nsIPrefBranch,
I'd be required to change all occurrences in nsNSSComponent. But it is using
some callback notification functionality, which nsIPrefBranch does not seem to
support. 

Comment 49

[Kai Engert (:KaiE:)]

Attachment: Updated patch

- UI strings in this patch are r=cotter
- added help button as discuseed with cotter and nelsonb
- renamed variable names as suggested by alecf
- now using nsCString instead of nsCAutoString

Comment 50

[Alec Flett]

re: the sparse-ness issue.
I disagree that the switch() is comparable...because in both cases (your patch
and my suggestion) you have to iterate through every entry in the table. There
is no searching involved in either your version or mine. Since my version would
actually execute less code in each iteration through the loop, I assert that
mine is more efficient :)

I really think that if you think about how one would go about implementing what
I'm suggesting, you'll find its really much better than you think :) - your
patch will be much smaller, the code will be faster, less memory will be
allocated, and maintenance (i.e. adding/removing new ciphers) should be easier.

re: nsIPref, that sounds fine to me.

Comment 51

[Kai Engert (:KaiE:)]

> I really think that if you think about how one would go about implementing what
> I'm suggesting, you'll find its really much better than you think :) - your
> patch will be much smaller, the code will be faster, less memory will be
> allocated, and maintenance (i.e. adding/removing new ciphers) should be easier.

I wish I did, but I don't.

I'm having trouble to find arguments, because I think everything has been said
already.

Are we only talking about the constructor of that singleton that will be only
executed once?

Are you aware that values of cipher_id will be like 0xFF01 ?

If I try to write an array like you suggested, that does not use indices like
0xFF01, but of course values like 0, 1, 2..., I will have to introduce yet
another look up array:

int hardCodedCipherIDs[] = {
  SSL_RSA_WITH_RC4_128_SHA,
  ...
};

const char *hardCodedPrefStrings[] = {
  "security.ssl2.bla",
  ...
};

struct dynamicPrefStrings {
  int cipher_id;
  nsCString pref;
}

And every time I need a string, I will have to do two steps:
- iterate over hardCodedCipherIDs and try to find the cipher_id
- if found, fetch the value from the hard coded array
- if not found, iterate over the array of dynamicPrefStrings

I CAN NOT use a single simple array for all the ciphers, because I don't know
that set at compile time.

I must use a dynamic structure.

If you want me to avoid copying the strings in that constructor, I'll have to
use a crazy structure that dynamically points to either a string from the data
segment, or one from the heap. And of course, I'll need another bool to indicate
that state, because I want to clean up on shut down.

Comment 52

[Kai Engert (:KaiE:)]

Attachment: Alternative patch

Alec, I hope this patch is closed to what you prefer.

Please note: This is a UI relevant patch. If we want that feature, it must land
by tomorrow.

Thanks!

Comment 53

[Kai Engert (:KaiE:)]

Typo, instead of "closed" I wanted to say:

Alec, I hope this is closer to what you prefer.

Comment 54

[Nelson Bolyard (seldom reads bugmail)]

As I predicted in comment 47, and Kaie did in comment 51, this latest 
patch, which seems to do what Alec requested, has to do a search to find
the entry in PSM's table corresponding to the entry in NSS's table.

If we knew that both tables were sorted in ascending order by cipher_id,
then we could remember the index in PSM's table to the previously found
matching entry, and start each search from that index and stop the search
when we found an entry in PSM's table with a greater cipher_id.  That
would probably be most efficient, even better than binary search.

Of course the difference in execution times between these is probably much
less than a millisecond, but if the point of Alec's review is efficiency,
then we really should pick the most efficient.

Comment 55

[Alec Flett]

upon my initial scan, this patch looks like it:
1) does a whole lot less initialization
2) spends just as much time searching as the previous patch

so, it looks good. I'm continuing to review now.

Comment 56

[Alec Flett]

The approach is generally better. Now, you may say "But now I'm doing an n^2
loop!" - the fact is that the old switch statement played the same roll as the
n^2 loop, but took up far more space in both code and data! In fact, with a
tigher loop, you keep your code pages small, and thus more data wants to stay in
the processor's cache. And now, you do have the option of someday improving
isCipherWithHistoricaPrefString() in some of the ways nelson suggested.

I have a few more comments (all pretty trivial)
1) put the "wanted" attribute in the struct_historical_cipher_pref_strings
 structure as "isWanted" - I would call the structure something like
struct_historical_cipher_overrides since they override more than just the pref -
you'll save yourself the 6 lookups for each iteration through the array (i.e.
leverage the existing call to isCipherWithHistoricalPrefString())

2) use PRPackedBool when you have more than 2 booleans in a structure - it keeps
the structure smaller by only using 8 bits per boolean (instead of 32)

3) use nsCAutoString when using strings on a stack. (i.e. see nsCString pref;)

4) nsIEnumerator is deprecated. please use nsISimpleEnumerator. (use
NS_NewArrayEnumerator() in nsEnumeratorUtils.h)

5) in ListCiphers, why bother re-creating the NS_ISupportsArray every time -
cache it in nsCipherInfoService and then all you have to do is call
NS_NewArrayEnumerator each time after the first initialization. (should be as
simple as just switching |array| to a member variable, and checking if
(!mCipherArray) to see if you need to initialize it lazily... it will save you
SSL_NumImplementedCiphers + 1 allocations for every call to ListCiphers()

6) use NS_NEWXPCOM when new'ing objects that have default constructors

7) you don't need this:
+    nsCOMPtr<nsICipherInfo> ci = nsCI;
+

you can just add NS_STATIC_CAST(nsICipherInfo*, nsCI) directly to the
nsISupportsArray (wherever it ends up)


I guess that's it. 

Comment 57

[Alec Flett]

I should add that one thing you could if you can't finish this today is check in
the additional strings in the DTD (i.e. without removing the old ones) - it
would be a lot easier to get special dispensation after today if the localizable
strings are already in the tree.

so, sr=alecf on the JS, xul, and DTD.

Comment 58

[Kai Engert (:KaiE:)]

Attachment: Patch part 1/3

Comment 59

[Kai Engert (:KaiE:)]

> 1) put the "wanted" attribute in the struct_historical_cipher_pref_strings
>  structure as "isWanted" - I would call the structure something like
> struct_historical_cipher_overrides since they override more than just the pref -
> you'll save yourself the 6 lookups for each iteration through the array (i.e.
> leverage the existing call to isCipherWithHistoricalPrefString())

I don't want to, actually I realize that my code should in a future version be
changed to dynamically look at that fortezza attribute. While currently all
fortezza type ciphers happen to have historical cipher pref names, this is not a
given fact.

Generally, that "wanted" attribute is something that can be decided dynamically
at runtime, by looking at the attributes of available ciphers, and is not
limited to those ciphers that had historical preference string names.


> 2) use PRPackedBool when you have more than 2 booleans in a structure - it keeps
> the structure smaller by only using 8 bits per boolean (instead of 32)

ok, done.


> 3) use nsCAutoString when using strings on a stack. (i.e. see nsCString pref;)

ok, done.

> 4) nsIEnumerator is deprecated. please use nsISimpleEnumerator. (use
> NS_NewArrayEnumerator() in nsEnumeratorUtils.h)

ok, done.


> 5) in ListCiphers, why bother re-creating the NS_ISupportsArray every time -
> cache it in nsCipherInfoService and then all you have to do is call
> NS_NewArrayEnumerator each time after the first initialization. (should be as
> simple as just switching |array| to a member variable, and checking if
> (!mCipherArray) to see if you need to initialize it lazily... it will save you
> SSL_NumImplementedCiphers + 1 allocations for every call to ListCiphers()

ok, done


> 6) use NS_NEWXPCOM when new'ing objects that have default constructors

ok, done.


> 7) you don't need this:
> +    nsCOMPtr<nsICipherInfo> ci = nsCI;
> +
> 
> you can just add NS_STATIC_CAST(nsICipherInfo*, nsCI) directly to the
> nsISupportsArray (wherever it ends up)

ok, done

Comment 60

[Kai Engert (:KaiE:)]

> I should add that one thing you could if you can't finish this today is check in
> the additional strings in the DTD (i.e. without removing the old ones) - it
> would be a lot easier to get special dispensation after today if the localizable
> strings are already in the tree.
>
> so, sr=alecf on the JS, xul, and DTD.

Thanks, but that would be complicated, the only thing I could check in were the
added strings, but all the rest would have to stay in place to not break any
functionality. It would be cooler if we could land it in a single piece today.

Comment 61

[Alec Flett]

Comment on attachment 76776 [details] [diff] [review]
Patch part 1/3

nice turnaround :)
sr=alecf

(I was only suggesting the DTD workaround in case you couldn't get this patch
ready by today - clearly no problem there!)

Comment 62

[Asa Dotzler [:asa]]

Comment on attachment 76776 [details] [diff] [review]
Patch part 1/3

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 63

[Jaime Rodriguez, Jr.]

Marking as adt1.0.0- per ADT triage.

Comment 64

[Stephane Saux]

remove nsbeta1+
Let's check this in the trunk.
target 2.3
We want to get this on the 1.0.0 branch at some point.

Comment 65

[Kai Engert (:KaiE:)]

Well.

The latest patch does not compile on Windows.
Luckily I tried before checking it in. :)

The problem is: I still have a "const char*" from my originally concept, and are
trying to assign and delete that pointer. Deletion fails on Windows.

But, changing the pointer to be "char*" does not work, either, because in that
case, assining a string from the data segment does not work (sigh).

I don't like casting away const, and this means I have to use another pointer
per cipher in our data structure.

I'm attaching an incremental patch, so we need review on the new patch in addition.

Comment 66

[Kai Engert (:KaiE:)]

Attachment: Incremental patch

Comment 67

[Kai Engert (:KaiE:)]

Attachment: Patch part 2/3

Actually, I realized that I must not use delete to delete results from
ToNewCString, but nsMemory::Free().

Comment 68

[Alec Flett]

Comment on attachment 78691 [details] [diff] [review]
Patch part 2/3

sr=alecf.. this protects you well.

Comment 69

[Kai Engert (:KaiE:)]

Checked in to the trunk.

Comment 70

[Kai Engert (:KaiE:)]

Unfortunately there is a problem with the UI used by the new implementation. The
code I produced does no longer work. I suspect this could be a general
regression in XUL code. Please see bug 137530 which tracks the new problem.

Comment 71

[Kai Engert (:KaiE:)]

Now that bug 137530 seems to be fixed, I'm marking this one fixed as well.

Comment 72

[John Unruh]

Verified on the 5/9 trunk

Comment 73

[Kai Engert (:KaiE:)]

Reopening. I don't get any traction on bug 137530.

The 1.1alpha freeze is tomorrow, and it does not make sense to have broken code
checked in.

I'm asking for permission to back the code out, reverting to the previous UI and
limited list of ciphers.

Javi, Alec, can you please give me r=/sr= to back this patch and the
non-succeeding patch from bug 137530?

Comment 74

[Alec Flett]

sr=alecf - really sorry to see this miss!

Comment 75

[Javier Delgadillo]

r=javi for backout.  Too bad.

Comment 76

[Kai Engert (:KaiE:)]

I have backed it out. We are back at the cipher UI and list that was in place
before I worked on this bug.

This is not the last word on this bug, and most of the code is still correct.

We just need to make sure that we use a kind of UI that is commonly used in
Mozilla. The approach used with this patch, checkboxes in
multi-column-listboxes, is not used anywhere else, and I couldn't find support
from XUL people who could make it work again.


I have two ideas how a simpler UI could look like.

Idea 1:
=======
Instead of using a multi-column-listbox with checkboxes, we could use a
single-column-listbox with checkboxes. Those are actually used already, for
example in Edit/Prefs/Advanced/Scripts, and in another place on Windows only.

However, that control is broken, too, when the list of items is large enough to
require a scrollbar (you can't toggle checkboxes further down in the list). Our
list is large.


Idea 2:
=======
Do not use any checkboxes within the listbox.
Just display a list where the list items show the name of the cipher, and a
string "enabled" or "disabled".
When a user selects an entry, we update the text info section.

In addition, the text info section also would contain a checkbox that allows to
enable or disable the cipher, and if the user toggles, we update the "enabled"
string in the listbox to disabled.

Would that UI be acceptable?

Comment 77

[Kai Engert (:KaiE:)]

Attachment: Suggested new UI, using simpler widgets

This is a screenshot of what I describe at the end of the previous comment.

Comment 78

[Kai Engert (:KaiE:)]

Comment on attachment 83632 [details]
Suggested new UI, using simpler widgets

wrong image

Comment 79

[Kai Engert (:KaiE:)]

Attachment: Suggested new UI, using simpler widgets

Comment 80

[Kai Engert (:KaiE:)]

Comment on attachment 76776 [details] [diff] [review]
Patch part 1/3

Required patch part 1

Comment 81

[Kai Engert (:KaiE:)]

Attachment: Patch part 3/3

This is an incremental patch, based on what has already been reviewed in the
previous patches.

It implements the new UI as shown in the screenshot.

It uses simpler widgets. No more checkboxes in listboxes.

Each line in the listbox displays the name of a cipher.
In front of the name, there will be an "x" shown if the cipher is currently
enabled.

If you click an entry in the list, the detailed information secction will
update.
In addition, the checkbox to the right of the list will be set to the enabled
state of the selected cipher.

If the user toggles the checkbox, the x in the list in front of the cipher name
will also be shown/removed.

Comment 82

[Kai Engert (:KaiE:)]

Attachment: Patch part 3/3 (same as previous, ignoring whitespace)

Comment 83

[Kai Engert (:KaiE:)]

Here is the explanation for the lot of spam today.

First, I thought, this bug would not have a high priority. Therefore I decided
to back it out, to at least have not broken code in the Mozilla trunk.

Then I received today the high priority request as described in bug 144510, to
add support for AES ciphers.
This changed things, and because I'd prefer to not work on the old UI any more,
and we all want all the ciphers, I decided to work on this bug right away.


I would like to land this on the trunk again.

I would then like to also land this on the branch. If ADT allows this new UI for
the branch, but we or ADT only want a different set of ciphers, we still could
land an adjusted variation of this on the branch.


I have tested the patch on Windows and Linux.

Comment 84

[Alec Flett]

just a UI comment - elsewhere in the product, we have enabled/disabled stuff on
the RIGHT side of the item you're enabling/disabling, and details about the item
below the list of items - see the mail filter UI, and the mail subscription UI
for enable/disable stuff in listboxes, and the security choose-a-certificate UI
for the details-below-the-list bit. 

I'm not sure an extra checkbox in the details side is really necessary - you can
just allow the user to check/uncheck the items in the list.

finally, I haven't looked at the patch in enough detail to know yet, but if
you're not using the standard checkmark/dot notation, go steal the styles from
the mail filters and mail subscription UI, for consistency.

Comment 85

[Kai Engert (:KaiE:)]

Alec, thanks a lot for your comment and the pointers to the other dialogs in
Mozilla. Sigh, I wish I had spotted them before.

The first implementation, the one that I backed out, actually used the same
approach as the mail filters. For some reason, my implementation of the UI did
not work correctly. I asked around, and nobody could explain why it did not work.

But now that I see that it actually works as expected in the mail filter dialog,
this gives me new hope. With that reference code, I should be able to find the
required XUL magic that makes the previous UI work.

Comment 86

[Kai Engert (:KaiE:)]

Alec, I looked at how the mail filter dialog is implemented.
It is not simple code, but it uses several tricks to make the
checkbox-in-listbox work. There are rules, actions, bindings, datasources,
manual mouse-click-to-clicked-cell calculation.

Of course it would be possible for me to duplicate that and do the same in the
cipher dialog.

But I wonder whether it is really worth to do all that additional work for the
simple requirement we have. Given that the cipher dialog will not be used often.

Given how much time I already put into this, I'd rather stay with the simple UI
and the separate check box.

If you prefer, I can put the x column to the right of the cipher name, and
display the info below the list, if you think that's better.

Comment 87

[Alec Flett]

I'm surprised its that much work - I'm guessing its just an XBL binding line in
CSS, maybe another line or two to get the images right...and then some sort of
onclick handler...but if its really more work then just file a bug for the
future behavior, you never know - someone else might tackle it..

Comment 88

[Kai Engert (:KaiE:)]

> I'm surprised its that much work - I'm guessing its just an XBL binding line in
> CSS, maybe another line or two to get the images right...and then some sort of
> onclick handler...

The work consists of:
- understand the XUL tricks the other dialogs are using
- test what I produce is correct and works on all platforms

There are no such requirements with the simple new UI I have implemented. It
uses simple widgets and will just work.


> but if its really more work then just file a bug for the
> future behavior, you never know - someone else might tackle it..

I have filed bug 144812

Comment 89

[Kai Engert (:KaiE:)]

Attachment: Suggested new UI, using simpler widgets, alternative screen layout suggested by alecf

Comment 90

[Kai Engert (:KaiE:)]

Attachment: Updated Patch part 3/3

Comment 91

[Kai Engert (:KaiE:)]

Attachment: Updated Part part 3/3 (same as previous, ignoring whitespace)

Comment 92

[Kai Engert (:KaiE:)]

Because this bug is already complex, and I'll be using a different strategy to
implement, I have filed the fresh bug 184940.

*** This bug has been marked as a duplicate of 184940 ***

Comment 93

[John Unruh]

Verified dupe.