235773 - (eccui) TLS ECC cipher suites: PSM backend, SeaMonkey UI

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement, P1)

Description

[Sheueling Chang]

User-Agent:       
Build Identifier: Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.4) Gecko/20031021

Request for Mozilla to be enhanced to support the Internet
Draft for Elliptic Curve Cipher suites in SSL.

The latest Internet-draft is available at:

   http://www.ietf.org/internet-drafts/draft-ietf-tls-ecc-05.txt

The underlying Elliptic Curve cryptographic library has already 
been checked into NSS3.9 library earlier.

Reproducible: Always
Steps to Reproduce:
1. N/A
2.
3.

Actual Results:  
There is no current support for Elliptic Curve.

Expected Results:  
N/A

N/A

Comment 1

[Sheueling Chang]

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

Comment 2

[Sheueling Chang]

Attachment: This patch adds ECC cipher suites in Mozilla's cipher suite configuration panel. It also adds support for ECC key generation in the KEYGEN tag to allow ECC-based PKI deployment.

This patch modifies files in mozilla/security/manager/....
This patch adds ECC cipher suites in Mozilla's cipher suite configuration panel

and adds support for ECC key generation in the KEYGEN tag
to allow the deployment of ECC-based PKI

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

I confirm this enhancement request.  Thanks for the patch.

Comment 4

[Sheueling Chang]

Comment on attachment 142389 [details] [diff] [review]
This patch adds ECC cipher suites in  Mozilla's cipher suite configuration panel. It also adds support for ECC key generation in the KEYGEN tag to allow ECC-based PKI deployment. 

Hello,
I would like to ask you to review this small patch to enhance Mozilla to
support ECC cipher suites. The bulk of ECC work has been done and we have
already worked with Nelson Bolyard to check in Elliptic Curve crypto into NSS
3.9.

Comment 5

[John G. Myers]

This patch adds a huge number of OIDs and possible key parameters, I have a hard
time believing half of them would be necessary.  I suspect the IETF process will
pare these down a bit an believe we should wait for it to do so.

You should indeed have the pulldown selections for the keygen tag be specific to
the keytype.  It's not acceptable to give the RSA keygen users such confusing
choices.  I suspect it's not necessary to give ECC keygen users the low-security
choice.

The signature of nssComponent->GetPIPNSSBundleString() has changed, you need to
update your patch.

Do you have a test website one could give this code a spin against?

Comment 6

[Vipul Gupta]

(In reply to comment #5)

  Hi John, 

    Thank you for reviewing the patch. My comments are below.

> This patch adds a huge number of OIDs and possible key parameters, I have a hard
> time believing half of them would be necessary.  I suspect the IETF process will
> pare these down a bit an believe we should wait for it to do so.

  Most of the OIDs are for the standard elliptic curves and they've been defined
  by bodies other than the IETF (NIST, ANSI and SECG) so I'm not sure if
  the IETF would be paring these down. But I understand your concern --
  especially in the case of the KEYGEN tag, it doesn't make sense to overwhelm
  the enduser with the too many curve names (most users won't know what
  to make of them).
 
> You should indeed have the pulldown selections for the keygen tag be specific to
> the keytype.  It's not acceptable to give the RSA keygen users such confusing
> choices.  I suspect it's not necessary to give ECC keygen users the low-security
> choice.

   Agreed. I'm not too familiar with this part of the Mozilla code. Is there someone
  on the Mozilla team we could work with? In particular, a few things aren't quite
  clear to me yet:

    -  How does one insert code for conditional compilation in a xul file (recall that 
       ECC support in NSS is only enabled when the flag NSS_ENABLE_ECC is set)?
       Do XUL files also pass through the C pre-processor? (Actually, this comment 
      is  for the code dealing with cipher suite check boxes in Preferences->
      Security and Privacy->SSL)

    - How does one create a drop down menu for the KEYGEN tag such that the items
      displayed in the  menu depend on the parameters passed to the  KEYGEN tag?

    - What is the right thing to do in terms of user interface for the KEYGEN tag --
       If the keytype is ec, should the browser display a fixed set of available curves
       or should we add a new keyparams parameter to KEYGEN and only list the
       curves that represent the intersection of what's implemented in the underlying
       libraries and what's listed in keyparams? Letting the certificate authority (CA)
       send out keyparams can help with the following usability issue arising from
       the use of EC keys -- while size is the only thing one cares about for RSA
      (and even that can be hidden from the user by saying 'low', 'medium', 'high')
      there are more variables for EC keys (e.g. which field -- prime or binary polynomial,
      what size and what specific curve for a given size).  Most end users
      would not be in a position to judge which curve is appropriate for their needs
      so it makes sense to let the CA specify a small set to choose from and the UI
      could hide the actual curve behind an adjective like 'low', 'medium', 'high'.

      Perhaps, initially we can restrict ourselves only to a very small number of
      NIST standardized prime-field curves. 

> The signature of nssComponent->GetPIPNSSBundleString() has changed, you need to
> update your patch.

> Do you have a test website one could give this code a spin against?

   We have a test CA set up internally at Sun. I'll set one up on the general Internet
   and let you know. I'll also enable ECC cipher suite support so that feature of
   Mozilla can also be tested.

   vipul

Comment 7

[John G. Myers]

Comment on attachment 142389 [details] [diff] [review]
This patch adds ECC cipher suites in  Mozilla's cipher suite configuration panel. It also adds support for ECC key generation in the KEYGEN tag to allow ECC-based PKI deployment. 

I'd already minused this.  The patch is not, in its current form, ready to be
checked in.

Comment 8

[John G. Myers]

(In reply to comment #6)
> Agreed. I'm not too familiar with this part of the Mozilla code. Is there someone
>   on the Mozilla team we could work with?

There's me, there's Kai (who doesn't have much time to give to Mozilla), and
there's whoever else you can find on the newsgroups, etc.  I'll be reviewing the
patch, but for areas outside my expertise the best I can do is point you towards
likely places to find other people.

 In particular, a few things aren't quite
>     -  How does one insert code for conditional compilation in a xul file 

You got me there.  I suggest asking on netscape.public.mozilla.xpfe or .builds

>     - How does one create a drop down menu for the KEYGEN tag such that the items
>       displayed in the  menu depend on the parameters passed to the  KEYGEN tag?

It looks like you have to modify nsKeygenFormProcessor::ProvideContent() to
return a different value based on the context.  I suspect you would get at that
context in much the same way that nsKeygenFormProcessor::ProcessValue() does.

(And I certainly wouldn't mind your adding a private method to class
nsKeygenFormProcessor() to permit ProvideContent and ProcessValue to share code.)

>     - What is the right thing to do in terms of user interface for the KEYGEN
tag --
>        If the keytype is ec, should the browser display a fixed set of
available curves
>        or should we add a new keyparams parameter to KEYGEN and only list the
>        curves that represent the intersection of what's implemented in the
underlying
>        libraries and what's listed in keyparams?

I definitely like the idea of adding a new parameter.

>       Perhaps, initially we can restrict ourselves only to a very small number of
>       NIST standardized prime-field curves. 

I would prefer we restrict ourselves to a small number of standardized curves. 
That was what I meant about the IETF paring the list down.

Also, is there a way to query to see if NSS has EC support compiled in?  In the
case where an EC-enabled PSM is dynamically linked against a non-EC-enabled NSS,
it should be able to hide the EC-related UI.  Actually, I believe there is some
way to get at the list of enabled SSL ciphers, that may be a way to address the
conditional XUL issue.

I'm assigning the bug to you as you're responsible for getting the patch in shape.

Comment 9

[Julien Pierre]

John,

My take on this bug is that going forward, we should have NSS ECC-enabled by
default and eliminate the conditional compilations issues (which may or may not
be possible to deal with in XUL anyway). For the Mozilla clients, there is no
good reason not to have ECC in NSS

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Several comments about turning ECC on in mozilla by default.

1. Best to wait until the ECC-TLS drafts become an RFC.  

2. IINM, the implementation of ECC/TLS in NSS matches an older draft,
not draft 5 (current draft as of this writing). 

Draft 5 includes negotiation of ECC curves in TLS/SSL.  The earlier draft,
now implemented in NSS (and IINM in OpenSSL and elsewhere) did not feature
negotiation of ECC curves.  This meant that a client and a server that both
supported ECC ciphersuites (but disjoint sets of curves) and also both 
supported a common set of non-ECC ciphersuites could (and probably would)
negotiate an ECC ciphersuite and then fail to complete the handshake, 
rather than succesfully handshaking with a Non-ECC ciphersuite.  

For a browser user, that problem would be highly similar to the "TLS
intolerance" problem, which mozilla now works around by reconnecting and
rehandshaking with TLS disabled after a failed handshake with it enabled.  
This is known as "transparent recovery".  Before mozilla added that, users
found it necessary to disable TLS to connect to many secure web sites.
I think it's clear that we don't want users having to do that for ECC.
IMO, we also don't want mozilla to make more than 2 attempts to connect 
and handshake in order to succesfully conclude one handshake, as it might 
if it had to try once with ECC, once with TLS but not ECC, and finally 
perhaps a third time with only SSL3 and not ECC.  

Internet Draft 5 of ECC/TLS provides ECC curve negotiation which will 
solve this problem, but only if/when SSL/TLS handshakes stop using SSL2
compatible client hellos.  IOW, SSL2 must die before ECC/TLS becomes
viable (IMO).  And curve negotiation is not yet implemented in NSS, IINM.

So, I think we've got a ways further to go before we're ready to enable
ECC/TLS by default.  But I do hope we'll get there before too long.
And adding UI to enable/disable ECC/TLS is a good step in the meantime.

Comment 11

[Vipul Gupta]

(In reply to comment #10)
 
> So, I think we've got a ways further to go before we're ready to enable
> ECC/TLS by default.  But I do hope we'll get there before too long.
> And adding UI to enable/disable ECC/TLS is a good step in the meantime.

I agree with Nelson that it would be premature to enable the ECC cipher
suites by default -- the IETF specification has not yet been approved as
an RFC.

However, I don't see any problems with exposing the ECC cipher suites
to mozilla users under Preferences->Privacy and Security->SSL->Edit
Ciphers->Extra SSL3/TLS. One of the things we could do is append
"(Experimental)" at the end of the cipher name until the RFC is issued.
This is similar to how OpenSSL does not expose ECC ciphers by default
but makes them accessible by turning on ECCdraft (the draft indicates
that the specification is not yet an RFC).

Furthermore, I'm not sure I see the need to make Bug 235773 dependent
on the addition of curve negotiation for Mozilla. Curve negotiation is
a mechanism for constrained clients that do not implement all of the 
curves listed in the spec to avoid the problem described by Nelson. 
However, Mozilla does implement all of the listed curves. Section 4
of http://www.ietf.org/internet-drafts/draft-ietf-tls-ecc-05.txt allows
such clients to not send the extension.

What am I missing?

vipul

Comment 12

[Vipul Gupta]

(In reply to comment #6)

>    We have a test CA set up internally at Sun. I'll set one up on the general Internet
>    and let you know. I'll also enable ECC cipher suite support so that feature of
>    Mozilla can also be tested.
> 
>    vipul

There is now an externally accessible Apache webserver that can communicate via both
RSA and ECC cipher suites. The server is available at dev.experimentalstuff.com
(HTTP on port 8081, HTTPS on port 8443).

If you access the URL https://dev.experimentalstuff.com:8443/test.php,
the page you get will tell you what kind of SSL connection was negotiated, e.g.
for a traditional browser, the CIPHER SUITE will be RC4-SHA or something
similar indicating RSA key exch (for RSA, the key exch isn't mentioned
explicitly). If, however, one uses an ECC-enabled version of Mozilla with
ECC ciphers enabled (check the ECC cipher suites in 
Preferences->Privacy and Security->SSL->Edit Ciphers
->Extra SSL3/TLS Ciphers) then the page will say:

CIPHER SUITE  ECDHE-ECDSA-AES128-SHA

(or similar depending on which ECC ciphers were enabled)

In the next few days, I'll also set up a demo certificate authority
on the same server capable of issuing ECC certificates. This
would allow testing of the new ec keytype in Mozilla's
KEYGEN tag handling code.

Please let me know if you encounter any problems.

vipul

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

In response to comment 11, 2 points:

1. I have no problem allowing the existing ECC cipher suites to be exposed
in the UI, so that knowledgable users can turn them on if they want to,
provided that they are all disabled by default.  Such users would also be
responsible to turn them off if SSL handshakes began to fail due to ECC
ciphersuites.  The whole point of my prior comment had to do with the 
*default condition* of the ECC ciphersuites.

2. Today, NSS's implementation may implement all known standardized curves,
but it only takes one new one to make that no longer true.  On the day that
that first new(er) ECC curve is rolled out on some popular server, client
that cannot negotiate curves will begin to experience the problem I described.
We know that this whole field of ECC is still in its infancy.  So, we cannot
expect that the space of standard curves will not soon grow.

Comment 14

[Vipul Gupta]

(In reply to comment #13)

  I agree. Looks like we are both saying that:

   -  it is ok to make the new cipher suites visible in the UI as 
       long as these ciphers are not turned on by default (the
       checkboxes will be unchecked)
   - curve negotiation ought to be added to NSS before ECC 
      cipher suites are turned on by default

  Given this, should the dependency of 235773 on 236245
  be removed (236245 should still be left as unresolved)?

  vipul

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

I've filed bug 236507 about enabling ECC by default.  That bug depends on 
bug 236245, so this one need not.

Comment 16

[John G. Myers]

The patch needs to also be changed to test the keytype like the recently checked
in fix to bug 139473.

Comment 17

[Vipul Gupta]

Attachment: Updated patch for Mar 10, 2004 CVS tip (not yet ready for checkin though)

I've updated the previous patch in response to changes made to the
Mozilla source since mozilla-1.4 (the previous patch was for 1.4).

This patch isn't quite ready for checkin. Here are the two outstanding
issues that need to be resolved. I'm placing the patch here simply for
testing purposes (an ECC enabled Apache webserver is now available
at https://dev.experimentalstuff.com:8443/test.php).

  - the patch must add ECC cipher suites in the UI only if the
     underlying crypto library (NSS) has ECC support turned on

  - the drop down menu displayed for the KEYGEN tag needs
     to be dependent on the keytype and keyparams attributes
     (this will be easy once bug 236684 is resolved)

vipul

Comment 18

[Vipul Gupta]

(In reply to comment #12)
  
> In the next few days, I'll also set up a demo certificate authority
> on the same server capable of issuing ECC certificates. This
> would allow testing of the new ec keytype in Mozilla's
> KEYGEN tag handling code.

I have set up a demo certification authority capable of
issuing Elliptic Curve certificates. To try this out, point
your browser at the following URL and follow directions.

  http://dev.experimentalstuff.com:8081/RequestCert.html

Currently there is no provision to send email from this server which
limits the CA's ability to verify the email address of the cert
requester. This CA is only intended for demo/testing.

Please let me know if you encounter any problems.

vipul

Comment 19

[Vipul Gupta]

Attachment: Patch for Mozilla-1.6 (work in progress)

Adds the same functionality as attachment 143528 [details] [diff] [review] (above) but targets
Mozilla-1.6.

Comment 20

[Vipul Gupta]

Attachment: Updated patch for Mozilla-1.6

The previous patch did not correctly handle the "low grade" security option in
the KEYGEN tag for ECC keytype. Unlike Mozilla 1.7 which only supports "medium"
and "high" grade security, Mozilla 1.6 still supports "low" grade (512-bit RSA
which corresponds to 112-bit ECC).

Comment 21

[Vipul Gupta]

Attachment: Patch for Mozilla 1.6

Attachment 144757 [details] [diff] had a typo in nsNSSComponent.cpp due to which
the ECDH-ECDSA with RC4 and SHA1 cipher suite could not be activated.
This attachment fixes that typo.

Comment 22

[Vipul Gupta]

Attachment: Patch for Firefox (Aviary 1.0 branch)

The firefox source places pref-ssl.dtd and pipnss.properties files
in different directories compared to Mozilla. Otherwise, this patch
is pretty much the same as attachment (id=143528).

Comment 23

[timeless]

Comment on attachment 163877 [details] [diff] [review]
Patch for Firefox (Aviary 1.0 branch)

i'm currently auditing one of the files related to this patch. this review is
based on my general areas of concern, namely: bad code and failure to check for
failure.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/util/secit
em.h&rev=1.4&mark=58,63-64#52
 58 ** The resulting item is returned; NULL if any error occurs.
 63 extern SECItem *SECITEM_AllocItem(PRArenaPool *arena, SECItem *item,
 64				      unsigned int len);

>Index: mozilla/security/manager/ssl/src/nsKeygenHandler.cpp
>+static SECKEYECParams * 
>+decode_ec_params(char *curve)
>+    ecparams = SECITEM_AllocItem(NULL, NULL, (2 + oidData->oid.len));
>+    ecparams->data[0] = SEC_ASN1_OBJECT_ID;
you just crashed. please check for allocation failure.

> nsKeygenFormProcessor::GetPublicKey(nsAString& aValue, nsAString& aChallenge, 
> 				    nsAFlatString& aKeyType,
>+				    nsAString& aOutPublicKey, nsAString& aKeyParams)
>@@ -406,12 +540,12 @@
>     } else if (aKeyType.EqualsIgnoreCase("dsa")) {
>         char * end;
>+        keyparamsString = ToNewCString(aKeyParams);
>         type = dsaKey;
>         keyGenMechanism = CKM_DSA_KEY_PAIR_GEN;
>+        if (strcmp(keyparamsString, "null") == 0)
you just crashed in libc because no one null checked ToNewCString (yes, i know,
you're just changing blame and variable names. i don't care.)

>@@ -424,6 +558,13 @@
> found_match:
>         pqgParams = decode_pqg_params(str);
>+#ifdef NSS_ENABLE_ECC
>+    } else if (aKeyType.EqualsIgnoreCase("ec")) {
>+        keyparamsString = ToNewCString(aKeyParams);
i'm guessing this will cause issues if you don't check the allocation for
failure.

>@@ -443,6 +584,47 @@
>+#ifdef NSS_ENABLE_ECC
somewhere in this block you mention pqg but your patch seems to replace most
instances of pqg with keyparams ...
>+        case CKM_EC_KEY_PAIR_GEN:
>+            /* XXX We ought to rethink how the KEYGEN tag is 
will there be a bug for this XXX?
>+            /* XXX The signature algorithm ought to choose the hashing
will there be a bug for this XXX?
>+             * algorithm based on key size once ECDSA variations based
>+             * on SHA256 SHA384 and SHA512 are standardized.
is there a thread where someone can follow to find more information about the
process for this standardization?

>Index: mozilla/security/manager/ssl/src/nsNSSCertificate.cpp
>@@ -1074,6 +1075,8 @@
>                       nsINSSComponent *nssComponent,
>                       nsIASN1Sequence **retSequence)
> {
>+  SECOidTag algOIDTag = SECOID_FindOIDTag(&algID->algorithm);
>+  SECItem paramsOID = { siBuffer, NULL, 0 };
>   nsCOMPtr<nsIASN1Sequence> sequence = new nsNSSASN1Sequence();
>   if (sequence == nsnull)
>     return NS_ERROR_OUT_OF_MEMORY;
>@@ -1095,8 +1098,16 @@
>     printableItem = new nsNSSASN1PrintableItem();
no one null checked this for alloc failure, so you're now responsible for the
crash that follows.
>     asn1Objects->AppendElement(printableItem, PR_FALSE);
>     nssComponent->GetPIPNSSBundleString("CertDumpParams", text);
>+    printableItem->SetDisplayName(text);

Comment 24

[Douglas Stebila]

Attachment: Update for Dec. 02 2005 CVS tip

Update for current tip of tree.  Uses latest revisions to NSS in patch bug #236245.  All ECC cipher suites are added to about:config; only four cipher suites are added to the Preferences UI in Mozilla.  Patch should work on Mozilla and Firefox.  Requires tip of NSS as well; see patch in bug #317620.  Used to successfully build an ECC-enabled version of Firefox, see http://dev.experimentalstuff.com:8082/binaries

Comment 25

[Kai Engert [:KaiE:]]

Attachment: Update for 09-Jan-2006 CVS tip

Comment 26

[Kai Engert [:KaiE:]]

The Mozilla suite, and its successor SeaMonkey, show the UI changed by this patch.

However, I can't find UI that allows to control the list of ciphers in Firefox or Thunderbird. It appears the developers of FF and TB did not include this ability in their new preferences sections (probably in their attempt to create a simpler UI).

Comment 27

[Vipul Gupta]

(In reply to comment #26)
> The Mozilla suite, and its successor SeaMonkey, show the UI changed by this
> patch.
> 
> However, I can't find UI that allows to control the list of ciphers in Firefox
> or Thunderbird. It appears the developers of FF and TB did not include this
> ability in their new preferences sections (probably in their attempt to create
> a simpler UI).

I can't comment about TB, but for FF, the only way I know of to change
cipher specs is to point FF to the URL "about:config", type "ssl" in the
Filter and double click on the cipher (such as security.ssl3.dhe_rsa_des_sha 
under the Preference Name column) to toggle its Value (enable/disable it).

Perhaps TB has a similar mechanism.

I hope this helps,

vipul

Comment 28

[Kai Engert [:KaiE:]]

Yes, TB does offer access to about:config, too, it's accessible from preferences.

I guess we agree that there is no other UI that needs to be adjusted.

However, we might want to enhance the security information shown in "page info", Bob Relyea just suggested to me, we should mention that ECC is in use. But that can be a separate bug.

Comment 29

[Robert Relyea]

I think having an 'enable Suite B' or 'Suite B only' button should be available somewhere reasonable accessible

Comment 30

[Vipul Gupta]

(In reply to comment #28)
 
  "Page Info" today does not mention anything about the
asymmetric algorithm (e.g. RSA)  used for key exchange.
Instead, it only mentions the symmetric key algorithms
used in SSL bulk encryption as in:

"Connection Encrypted: High-grade Encryption (AES-256 256 bit)"

Currently, the only way to see what key exchange mechanism
was used is to click "View" certificate and then peek
at the Algorithm Identifier within "Subject Public Key"
in the Details panel. Even this method does not 
always tell you exactly what key exchange method was
used, e.g. there are ciphers like *_ECDHE_RSA_* or 
*_DHE_RSA_* where the public key in the cert is RSA
but the key exchange mechanism is (EC)DHE.

We may wish to consider altering the Page Info content
to mention the complete cipher suite, i.e. mention
the key exchange mechanism in addition to the
bulk encryption algorithms. But then one
has to deal with the issue that some customers
might not understand how 224-bit ECC can be
stronger than 1024-bit RSA (most users have no
clue about key sizes and those that don't
have the full picture tend to think bigger
keys are always better).

vipul

Comment 31

[Kai Engert [:KaiE:]]

> I think having an 'enable Suite B' or 'Suite B only' button should be available
> somewhere reasonable accessible

If I understand correctly, your proposal applies to Firefox/Thunderbird.

Lacking a mechanism to individually control the enabled ciphers by end user UI, in Firefox and Thunderbird, you are proposing to have at least a button, to disable all non-ECC ciphers.

My understanding is 'Suite B only' means 'disable all non ECC ciphers'.

Is your proposal restricted to SSL?
If your proposal is restricted to SSL, we could lobby for an appropriate checkbox be added to FF/TB prefs, near the existing SSL/TLS protocol checkboxes.

Or do you propose this setting should disable non-ECC S/MIME ciphers, too?
(As of today, we don't have any prefs to control the use of S/MIME ciphers.)

Comment 32

[Kai Engert [:KaiE:]]

Attachment: Update for 07-Feb-2006 CVS tip

Comment 33

[Kai Engert [:KaiE:]]

Attachment: Update for 07-Feb-2006 CVS MOZILLA_1_8_BRANCH

Comment 34

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #30)
>   "Page Info" today does not mention anything about the
> asymmetric algorithm (e.g. RSA)  used for key exchange.
> Instead, it only mentions the symmetric key algorithms
> used in SSL bulk encryption as in:
> 
> "Connection Encrypted: High-grade Encryption (AES-256 256 bit)"

I wonder when that changed.  It certainly did display info about the 
key exchange/transport/agreement algorithm at one time.  

There are numerous functions in NSS put there expressly to make it easy for 
programs like mozilla display all the components of the current ciphersuite, 
AND to display information about the strength of the keys in the certs.  
If that's not being used, how sad!

Comment 35

[Vipul Gupta]

(In reply to comment #31)
> > I think having an 'enable Suite B' or 'Suite B only' button should be available
> > somewhere reasonable accessible

   We need to be careful about this.  "Suite B" only includes
   a small subset of the total curves supported in NSS and currently
   there is no API in NSS to enable/disable specific curves. One can
   enable/disable specific ECC cipher suites in TLS (for example) 
   but there's no knob to choose specific curves.

   When NSS supports curve negotiation via TLS extensions
   (bug 236245, bug 226271), then we'll need a knob like
   this and we could support selection of Suite B curves as
   a group.

   vipul

Comment 36

[Kai Engert [:KaiE:]]

I'm proposing to devote this bug 235773 to the UI changes only, as the bug summary already suggests.

I have filed separate bug 326159 for all issues remaining to in-browser-key-generation, be it using the HTML KEYGEN tag (which already appears to be working, but I might post some test results) and the JavaScript function crypto.generateCRMFRequest which is not yet implemented - I'm looking into this now.

Comment 37

[Kai Engert [:KaiE:]]

Your patch adds configuration options for 20 new ciphers.
However, you only added 4 of them to the UI.
Why?

Comment 38

[Vipul Gupta]

(In reply to comment #37)
> Your patch adds configuration options for 20 new ciphers.
> However, you only added 4 of them to the UI.
> Why?

  Hi Kai,

    Now might be a good time to revisit this decision. The original
   reasoning behind choosing to expose only a few was that we were
   using these builds only internally for demos etc (pending standardization)
   and didn't want to make the already long list of Extra ciphers unwieldy 
   especially since no one else was using ECC ciphers.

    In later versions of the patches, the actual choice of which ciphers to expose
    was influenced (perhaps unduely) by the list of ciphers to be included
    in vista. Given that Mozilla foundation's new flagship browser Firefox completely
    does away with a GUI for choosing ciphers (using, instead, "about:config"), this
    issue didn't get much attention.

    I'd love to see some more discussion around this especially from someone
    like yourself who is more in tune with the future plans for the
    Mozilla suite.

    vipul

Comment 39

[Kai Engert [:KaiE:]]

Hi Vipul, thanks for explaining the history of the UI changes.

I agree that we don't need changes for Firefox/Thunderbird,
as the product owners had decided on their own to remove
the dialog to control the ciphers.

Regarding SeaMonkey (Mozilla Suite), my opinion is,
as long as there is a dialog to control ciphers,
it should display (at least) all the ciphers that
are enabled by default.

Please correct me if I'm wrong, but I believe that
sooner or later we will enable the ciphers you are adding
in the patch by default.

This means a total of 20 new ciphers.

I am proposing to undo the change you proposed to the
"Extra SSL3/TLS" tab, but add all EC ciphers in new
tabs of the dialog.

As it wasn't a lot of work, I quickly made that change.
I will attach a new patch and two screenshots that
illustrate this new proposal.

If you agree with this change, could you please review
the human readable wording for all the EC ciphers?

I didn't think much about it, but split the 20 ciphers
into groups and tabs of 10 ECDHE and 10 ECDH ciphers.

The new patch contains an additional small change,
it exports function decode_ec_params so it can be used
in bug 326159.

Comment 40

[Kai Engert [:KaiE:]]

Rereading comment 26, comment 29 and comment 35, I think it will need more thinking what new ECC/Suite-B UI we should propose for Firefox/Thunderbird.

Comment 41

[Kai Engert [:KaiE:]]

Changing summary to indicate this bug is only partially about UI.

Comment 42

[Kai Engert [:KaiE:]]

The new patch also addresses timeless' code review comment 23.

Comment 43

[Kai Engert [:KaiE:]]

> >Index: mozilla/security/manager/ssl/src/nsKeygenHandler.cpp
> >+static SECKEYECParams * 
> >+decode_ec_params(char *curve)
> >+    ecparams = SECITEM_AllocItem(NULL, NULL, (2 + oidData->oid.len));
> >+    ecparams->data[0] = SEC_ASN1_OBJECT_ID;
> you just crashed. please check for allocation failure.

Null check added


> > nsKeygenFormProcessor::GetPublicKey(nsAString& aValue, nsAString& aChallenge, 
> > 				    nsAFlatString& aKeyType,
> >+				    nsAString& aOutPublicKey, nsAString& aKeyParams)
> >@@ -406,12 +540,12 @@
> >     } else if (aKeyType.EqualsIgnoreCase("dsa")) {
> >         char * end;
> >+        keyparamsString = ToNewCString(aKeyParams);
> >         type = dsaKey;
> >         keyGenMechanism = CKM_DSA_KEY_PAIR_GEN;
> >+        if (strcmp(keyparamsString, "null") == 0)
> you just crashed in libc because no one null checked ToNewCString (yes, i know,
> you're just changing blame and variable names. i don't care.)

Somebody already added a null check in the latest patch


> >@@ -424,6 +558,13 @@
> > found_match:
> >         pqgParams = decode_pqg_params(str);
> >+#ifdef NSS_ENABLE_ECC
> >+    } else if (aKeyType.EqualsIgnoreCase("ec")) {
> >+        keyparamsString = ToNewCString(aKeyParams);
> i'm guessing this will cause issues if you don't check the allocation for
> failure.

Null check added


> >     printableItem = new nsNSSASN1PrintableItem();
> no one null checked this for alloc failure, so you're now responsible for the
> crash that follows.

Null check added

Comment 44

[Kai Engert [:KaiE:]]

Attachment: Patch 2006-03-23 for 1.8 branch

Comment 45

[Kai Engert [:KaiE:]]

Attachment: Screenshot, new SeaMonkey ciphers tab 1 of 2

Comment 46

[Kai Engert [:KaiE:]]

Attachment: Screenshot, new SeaMonkey ciphers tab 2 of 2

Comment 47

[Nelson Bolyard (seldom reads bugmail)]

Can any one think of a reason to enable an ECDH suite and not also the corresponding ECDHE suite, or vice versa?  

I think we could just combine the two, and enable ECDH(e)_ECDSA together 
in one check box.  

Also, I still think a "suite B" button is a reasonable idea, despite 
lack of curve controls (at present).  
Initially, it can mean "exclude any non-suite-B algorithms", and then 
later it can also imply "exclude any non-suite-B curves".  

Comment 48

[Kai Engert [:KaiE:]]

> Can any one think of a reason to enable an ECDH suite and not also the
> corresponding ECDHE suite, or vice versa?  
> 
> I think we could just combine the two, and enable ECDH(e)_ECDSA together 
> in one check box.  

If it does not make sense to enable them individually, I wonder why NSS offers them as separate cipher suites.

The current pref system has an automatic mapping from one checkbox to one NSS cipher suite. If you want to change that, to map one checkbox to multiple cipher suite preferences, that would require more work than I'd personally be willing to invest in a SeaMonkey only patch.

Comment 49

[Vipul Gupta]

(In reply to comment #39)
> I am proposing to undo the change you proposed to the
> "Extra SSL3/TLS" tab, but add all EC ciphers in new
> tabs of the dialog.
 
> I didn't think much about it, but split the 20 ciphers
> into groups and tabs of 10 ECDHE and 10 ECDH ciphers.

My main concern with this approach is that it treats
ECC ciphers differently than existing ciphers using
other key exchange mechanisms, e.g. RSA, DHE.

I do realize that adding another 20 ciphers in the
Extra SSL3/TLS tab is also not an attractive option.
So I'm out of ideas on this one and perhaps we
should just add a new tab labelled "Extra ECC"
(or similar) and put all the ECC ciphers there.

thoughts?

vipul

Comment 50

[Kai Engert [:KaiE:]]

More than 12 ciphers per page will make the dialog too large for small screens. That was the reason why I split the list.

Now that we have much more ciphers, and offering them all exceeds the limits of our current UI, and it would require more thinking and work to come up with a good solution, that will be SeaMonkey only, we have one more option: Remove the "edit ciphers" button and the dialog, and point people to about:config - which works in SeaMonkey, too.

Comment 51

[timeless]

Comment on attachment 215954 [details] [diff] [review]
Patch 2006-03-23 for 1.8 branch

just a minor note, there seem to be places where you have 4/2 and 4/3 indentation instead of simply 4 space or 2 space...

Comment 52

[Kai Engert [:KaiE:]]

Vipul wrote:
> We need to be careful about this.  "Suite B" only includes
> a small subset of the total curves supported in NSS and currently
> there is no API in NSS to enable/disable specific curves. One can
> enable/disable specific ECC cipher suites in TLS (for example) 
> but there's no knob to choose specific curves.

Nelson wrote:
> Also, I still think a "suite B" button is a reasonable idea, despite 
> lack of curve controls (at present).  
> Initially, it can mean "exclude any non-suite-B algorithms", and then 
> later it can also imply "exclude any non-suite-B curves".  

Nelson, you are proposing a UI that uses a wording "Suite B only",
but for the time being, allow it to have curves enabled, that
are not Suite B?

I think that would be confusing.

What is the motivation for exposing a "Suite B" UI to end users?

The only motivation I can see is, if a user decides,
 "I only want this cool new stuff", am I right?

How likely is it a user will want to enable this any time soon?
As ECC support is just starting to come up, it appears very unlikely
to me that users want to disable "old SSL/TLS stuff" any time soon,
because they want to communicate with all the servers they find in 
the wild.

If my understanding is right, I'd say, we don't need this UI
short term, but could postpone adding that UI to a later version.

If somebody really wants to use only Suite-B immediately,
they can change the prefs.

If you think, disabling several individual ciphers prefs
(using about:config) is too much of a hassle for admins or end
users, we could start using a new "Suite B only" hidden pref.

If you think we should add that pref, I propose we make that pref
correct with regards to its meaning, right from the beginning.

That means, I propose to have a "positive list" of the ciphers
known to be part of Suite B at compile time
(because Vipul explained, we want to enable EC ciphers that
are not part of Suite B).


So I see the following options.

Let's start with the simplest, Thunderbird.
As of today, Thunderbird does not even have a UI to enable/disable
SSL versions. It's unlikely that we will get approval to add
a "Suite B SSL" UI.
However, a "Suite B hidden pref" solution would apply to TB, too.


For Firefox (and SeaMonkey) I see the following options:

a) Do not add a checkbox to Firefox.
   Do not add a hidden "only Suite B" pref. 
   Just have all the individual cipher prefs, so users and
   admins can control which ciphers they want.

b) Do not add a checkbox to Firefox.
   Add a new "only Suite B" hidden pref,
   in addition to all the individual ciphers prefs,
   Add a positive list to our C++ files that explicitly
   list all Suite B ciphers/curves known at compile time.
   This pref will be disabled by default.
   If user enables pref, 
     only the Suite B ciphers/curves will be enabled, 
     all other curves disabled,
     and all non EC ciphers disabled.

c) Add a checkbox that says "Suite B" only,
   that controls the same positive list as described above in (b)
 

For SeaMonkey (Mozilla Suite) ciphers I see the following options:

d) Take the simple patch and screenshot as is.

e) Invest more time and work to make the "edit ciphers" dialog
   "more reasonable".

f) Remove the "Edit Ciphers" button and dialog,
   just have users go to about:config


My personal votes are:

(a) (because I think the UI is not required initially)
    but I could live with (b),
    and I think (c) will be difficult to get into FireFox 2,
    because we'll have to convince the Firefox UI owners
    this control makes sense. Seems unlikely to me at
    this point of time, where EC is not yet used.

and

(f) (because I think working on "SeaMonkey only" logic
     is not worth it)


What are you voting for?

Comment 53

[Kai Engert [:KaiE:]]

Attachment: Patch, Seamonkey only, removes UI to edit and show cipher info

This Seamonkey-only patch removes the "edit ciphers" button from preferences UI.
It also removes the dialogs that edit and information dialogs that can be accessed using the button.

It seems reasonable to remove this UI, 
- as the list of ciphers will grow, 
- the UI will soon be incomplete, 
- all ciphers (old and new) can be controlled using about:config anyway

Neil, are you ok with this change?

Comment 54

[Kai Engert [:KaiE:]]

Attachment: Patch v11

This Patch v11 is based on Patch 2006-03-32, with the following differences:

- removed the code that changes the "edit ciphers" UI

- set NSS_ENABLE_ECC from within PSM Makefile, eliminating the need to set environment variables

Comment 55

[Robert Relyea]

Comment on attachment 217431 [details] [diff] [review]
Patch v11

r+=relyea

Comment 56

[Kai Engert [:KaiE:]]

Attachment: Patch v12

This version incorporates changes proposed by Bob.

- in nsKeygenHandler.cpp, there was an inconsistency between the curves
  menionted in the comment, and curves used in the actual code.
  I adjusted the comment.

- I changed the Low/512 security choice, 
  we no longer use secp160r1, but secp192r1.
  Bob argued, this curve is faster in our code, and is more likely 
  to be compatible with other's code.
  Although this change does not have much real world effect,
  as our UI does not offer the user the Low security choice,
  but medium and high only.
  (But still, this mapping helps when the code is executed in the
  context of the crypto.generateCRMFRequest function)

- Finally I removed #define DEFAULT_CURVE_OID_TAG
  because it is never used.

Comment 57

[Kai Engert [:KaiE:]]

Attachment: Patch v13

The previous patch revision v12 contained code, that is related to key generation. I moved those portions over to patch v4 in bug 326159.

In addition, this patch now enables all the new ciphers by default (expect the NULL ciphers).

Comment 58

[Darin Fisher]

Comment on attachment 219899 [details] [diff] [review]
Patch v13

r=darin for the necko part

Comment 59

[Robert Relyea]

Comment on attachment 219899 [details] [diff] [review]
Patch v13

r+=rrelyea.

Comment 60

[Kai Engert [:KaiE:]]

Attachment: PSM makefile patch to enable ECC in NSS

Comment 61

[Kai Engert [:KaiE:]]

Attachment: patch to check out NSS_3_11_2_BETA_20060428_TAG

Comment 62

[Wan-Teh Chang]

Comment on attachment 220164 [details] [diff] [review]
patch to check out NSS_3_11_2_BETA_20060428_TAG

r=wtc.  I verified that the tag is the same as
the tip of the NSS_3_11_BRANCH.

Comment 63

[Wan-Teh Chang]

Comment on attachment 220163 [details] [diff] [review]
PSM makefile patch to enable ECC in NSS

This patch works, but I'd do it in a different way
if PSM will always have ECC enabled.

In mozilla/security/manager/Makefile.in, add

  DEFAULT_GMAKE_FLAGS += NSS_ENABLE_ECC=1

In the PSM source files, just get rid of
#ifdef NSS_ENABLE_ECC.

Comment 64

[Kai Engert [:KaiE:]]

Attachment: PSM makefile patch2 to enable ECC in NSS

Comment 65

[Wan-Teh Chang]

Comment on attachment 220210 [details] [diff] [review]
PSM makefile patch2 to enable ECC in NSS

r=wtc.

Comment 66

[Robert Relyea]

Comment on attachment 217425 [details] [diff] [review]
Patch, Seamonkey only, removes UI to edit and show cipher info

r+=rrelyea

Comment 67

[Kai Engert [:KaiE:]]

Attachment: Patch v13 b

This patch is mostly the same as Patch v13, however, it is a less intrusive patch. In this patch the new ciphers are now DISABLED by default (set to false).

This change has r=wtchang over the phone.

I'm carrying forward the reviews for the remained of the patch.

Comment 68

[Kai Engert [:KaiE:]]

Attachment: Patch to change ECC prefs from disabled to enabled

Comment 69

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 222058 [details] [diff] [review]
Patch v13 b

What is the motivation for this patch?

Comment 70

[Kai Engert [:KaiE:]]

Comment on attachment 222061 [details] [diff] [review]
Patch to change ECC prefs from disabled to enabled

sorry, there was a misunderstanding on my part

Comment 71

[Kai Engert [:KaiE:]]

Comment on attachment 219899 [details] [diff] [review]
Patch v13

I'm taking back my obsolete flag on this patch. This is still what we want.

Comment 72

[Kai Engert [:KaiE:]]

Comment on attachment 219899 [details] [diff] [review]
Patch v13

This patch has been checked in on the trunk with checkin comment:

"This check in will make PSM aware of ECC cipher suites. Nightly builds will not yet contain ECC, because NSS is still being compiled with ECC disabled."

Comment 73

[Kai Engert [:KaiE:]]

Comment on attachment 217425 [details] [diff] [review]
Patch, Seamonkey only, removes UI to edit and show cipher info

In addition to Neil's UI approval for this SeaMonkey only UI-removal patch, I just received 1.8 branch approval from CTho (cst at andrew.cmu.edu) on IRC.

Comment 74

[Kai Engert [:KaiE:]]

Comment on attachment 219899 [details] [diff] [review]
Patch v13

This patch has been checked in to MOZILLA_1_8_BRANCH with checkin comment:

"This check in will make PSM aware of ECC cipher suites. Nightly builds will not yet contain ECC, because NSS is still being compiled with ECC disabled."

Comment 75

[Ian Neal]

Comment on attachment 217425 [details] [diff] [review]
Patch, Seamonkey only, removes UI to edit and show cipher info

Checking in (1.8 branch)
locales/en-US/chrome/pippki/pref-ssl.dtd;
new revision: 1.1.8.3; previous revision: 1.1.8.2
pki/resources/content/pref-ssl.xul;
new revision: 1.27.8.3; previous revision: 1.27.8.2
done
Removing (1.8 branch)
pki/resources/content/cipherinfo.js;
new revision: delete; previous revision: 1.1
pki/resources/content/cipherinfo.xul;
new revision: delete; previous revision: 1.2
pki/resources/content/pref-ciphers.js;
new revision: delete; previous revision: 1.4
pki/resources/content/pref-ciphers.xul;
new revision: delete; previous revision: 1.13.18.1
pki/resources/content/ssl2ciphers.xul;
new revision: delete; previous revision: 1.1.92.1
pki/resources/content/ssl3tlsciphers.xul;
new revision: delete; previous revision: 1.1
done

Comment 76

[Ian Neal]

Attachment: Missing jar diff patch

Missing jar diff changes.
Note to kaie should ssl3tlsciphers2.xul still be in the tree?

Comment 77

[Ian Neal]

Comment on attachment 222814 [details] [diff] [review]
Missing jar diff patch

Checking in (1.8 branch)
jar.mn;
new revision: 1.45.4.4; previous revision: 1.45.4.3
done

Comment 78

[Kai Engert [:KaiE:]]

Comment on attachment 222814 [details] [diff] [review]
Missing jar diff patch

ssl3tlsciphers2.xul can be cleared/removed, too, and the jar.mn change should be applied to the trunk, too.

Comment 79

[Ian Neal]

(In reply to comment #78)
> (From update of attachment 222814 [details] [diff] [review] [edit])
> ssl3tlsciphers2.xul can be cleared/removed, too, and the jar.mn change should
> be applied to the trunk, too.
> 
Checking in (1.8 branch)
jar.mn;
new revision: 1.45.4.5; previous revision: 1.45.4.4
Removing (1.8 branch)
content/ssl3tlsciphers2.xul;
new revision: delete; previous revision: 1.3
done

Comment 80

[Ian Neal]

Attachment: Trunk jar patch

Jar change patch for trunk, carrying forward r=

Comment 81

[Ian Neal]

Comment on attachment 222862 [details] [diff] [review]
Trunk jar patch

Checking in (trunk)
jar.mn;
new revision: 1.49; previous revision: 1.48
Removing (trunk)
content/cipherinfo.js;
new revision: delete; previous revision: 1.2
content/cipherinfo.xul;
new revision: delete; previous revision: 1.3
content/pref-ciphers.js;
new revision: delete; previous revision: 1.5
content/pref-ciphers.xul;
new revision: delete; previous revision: 1.15
content/ssl2ciphers.xul;
new revision: delete; previous revision: 1.3
content/ssl3tlsciphers.xul;
new revision: delete; previous revision: 1.2
content/ssl3tlsciphers2.xul;
new revision: delete; previous revision: 1.3
done

Comment 82

[Darin Fisher]

Comment on attachment 220210 [details] [diff] [review]
PSM makefile patch2 to enable ECC in NSS

a=darin on behalf of drivers