236245 - Update ECC/TLS to conform to RFC 4492

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[John G. Myers]

Update the Elliptic Curve Cipher support to draft 5 or later.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Add dependency.  ECC client hello extensions are blocked by the absense of 
support for client hello extensions in general in NSS.

Comment 2

[Douglas Stebila]

Attachment: Update NSS to draft 12 plus upcoming revisions

Updates NSS to the latest draft as follows:

1) Update cipher suite numbers to draft 12, with the modification that the DES cipher suite is removed and the first two cipher suites are renumbered accordingly (expected in draft 13).

2) Update ClientCertificateType numbers.

3) Change NamedCurve data structure to be two bytes long.

4) Remove SHA-1 as the key derivation function for small curves.

5) Change test programs tstclnt, strsclnt, selfserv, and vfyserv to allow cipher suites to be specified on the command line using five character hex codes instead of only single letters.

This patch does not add complete compliance with the draft.  The following is lacking:

1) No support for ECC TLS extensions, since NSS does not support any TLS extensions (see Bug 226271).  According to the draft, this is a mandatory requirement for the server.

2) No support for ECDH_anon key exchange (not required for compliance).

3) No support for anything other than named curves (not required for compliance).

4) No support for point compression (not required for compliance).

5) No support for client authentication using ECDSA_fixed_ECDH or RSA_fixed_ECDH (not required for compliance).

Comment 3

[Wan-Teh Chang]

Comment on attachment 205063 [details] [diff] [review]
Update NSS to draft 12 plus upcoming revisions

I've reviewed all the changes to security/nss/lib/ssl in
this patch.  I have some questions for Douglas, Vipul, and
Nelson.

1. security/nss/lib/ssl/ssl3con.c

We need to review the ordering of the SSL3 cipher suites
in the cipherSuites array.  There is an inconsistency between
the two blocks of ECC cipher suites below:

> #ifdef NSS_ENABLE_ECC
>+ { TLS_ECDHE_ECDSA_WITH_RC4_128_SHA,       SSL_NOT_ALLOWED, PR_FALSE,PR_FALSE},
>+ { TLS_ECDHE_RSA_WITH_RC4_128_SHA,         SSL_NOT_ALLOWED, PR_FALSE,PR_FALSE},
>  { TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,   SSL_NOT_ALLOWED, PR_FALSE,PR_FALSE},
>  { TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,     SSL_NOT_ALLOWED, PR_FALSE,PR_FALSE},
> #endif /* NSS_ENABLE_ECC */

> #ifdef NSS_ENABLE_ECC
>- { TLS_ECDH_RSA_WITH_RC4_128_SHA,          SSL_NOT_ALLOWED, PR_FALSE,PR_FALSE},
>- { TLS_ECDH_RSA_WITH_AES_128_CBC_SHA,      SSL_NOT_ALLOWED, PR_FALSE,PR_FALSE},
>  { TLS_ECDH_ECDSA_WITH_RC4_128_SHA,        SSL_NOT_ALLOWED, PR_FALSE,PR_FALSE},
>  { TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA,    SSL_NOT_ALLOWED, PR_FALSE,PR_FALSE},
>+ { TLS_ECDH_RSA_WITH_RC4_128_SHA,          SSL_NOT_ALLOWED, PR_FALSE,PR_FALSE},
>+ { TLS_ECDH_RSA_WITH_AES_128_CBC_SHA,      SSL_NOT_ALLOWED, PR_FALSE,PR_FALSE},
> #endif /* NSS_ENABLE_ECC */

In the first block, we have RC4,RC4,AES,AES, whereas in the
second block we have RC4,AES,RC4,AES.

Question for Nelson: the MAX_CIPHER_SUITES macro is defined
(= 20) but not used.  Should we delete it?

2. security/nss/lib/ssl/ssl3ecc.c

>     /* Fail if the curve is not a named curve */
>     if ((ec_params.data[0] != ec_type_named) || 
>-	!supportedCurve(ec_params.data[1])) {
>+	!supportedCurve(ec_params.data[2])) {
> 	    errCode = SEC_ERROR_UNSUPPORTED_ELLIPTIC_CURVE;
> 	    desc = handshake_failure;
> 	    goto alert_loser;

I suggest that you check that ec_params.data[1] is 0:

>     if ((ec_params.data[0] != ec_type_named) || 
>-	!supportedCurve(ec_params.data[1])) {
>+	ec_params.data[1] != 0 ||
>+	!supportedCurve(ec_params.data[2])) {

Alternatively, change supportedCurve() to take a two-byte
array or a short int as the input argument.

>     if (curve != ec_noName) {
> 	ec_params.data[0] = ec_type_named;
>-	ec_params.data[1] = curve;
>+	ec_params.data[1] = 0xC0;
>+	ec_params.data[2] = curve;

ec_params.data[1] should be set to 0 instead of 0xC0, right?
(0xC0 is for cipher suite numbers.)

3. security/nss/lib/ssl/ssl3prot.h

>     kea_ecdh_ecdsa,
>     kea_ecdhe_ecdsa,
>     kea_ecdh_rsa,
>-    kea_ecdhe_rsa
>+    kea_ecdhe_rsa,
>+	kea_ecdh_anon
> } SSL3KeyExchangeAlgorithm;

The indentation is wrong.  In NSS source code, the most
common tab stop is 8, not 4.  In the above, use four spaces
instead of a tab.

4. security/nss/lib/ssl/sslimpl.h
 
> #ifdef NSS_ENABLE_ECC
>-#define ssl_V3_SUITES_IMPLEMENTED 40
>+#define ssl_V3_SUITES_IMPLEMENTED 47
> #else
> #define ssl_V3_SUITES_IMPLEMENTED 26
> #endif /* NSS_ENABLE_ECC */

I have a question for Nelson and a question for Douglas
and Vipul.

Douglas,Vipul: the delta of your changes to the cipherSuites
array is +6 (7 lines removed, 13 lines added).  Why is
the new value of ssl_V3_SUITES_IMPLEMENTED 47, not 46?

Nelson: the cipherSuites array is initialized with 23 entries
without ECC and 37 entries with ECC.  Why is the value of
ssl_V3_SUITES_IMPLEMENTED 26 and 40, respectively, resulting
in 3 zero entries at the end of the cipherSuites array?

Comment 4

[Wan-Teh Chang]

Comment on attachment 205063 [details] [diff] [review]
Update NSS to draft 12 plus upcoming revisions

I've completed the review of this patch.

First, I found out why the value of ssl_V3_SUITES_IMPLEMENTED
is off by 3.  It is a bug introduced in NSS 3.11 when we removed
FORTEZZA support (bug 319240).

I have some additional comments on this patch.

1. security/nss/cmd/selfserv/selfserv.c

My comments on this file also apply to the other files in
security/nss/cmd that you changed.

>+":WXYZ  Use cipher with hex code 0xWX,0xYZ in TLS1\n"

I suggest that you say:

>+":WXYZ  Use cipher with hex code { 0xWX,0xYZ } in TLS\n"

Note that I added braces { } around 0xWX,0xYZ to match the
exact notation used in the TLS RFC.  I also removed the
version 1 from "TLS1".

>+	fprintf(stderr, "Non-hex char in cipher string (-c :WXYZ arg).\n"); \

It is better to say "(-c :WXYZ)", without "arg", because
":WXYZ" is the argument.

>+			    "Invalid length of cipher specified by : in cipher string (-c arg).\n");

Here it is better to say "(-c :WXYZ)" than "(-c arg)".

> 	    if (cipher > 0) {
> 		SECStatus status;
> 		status = SSL_CipherPrefSetDefault(cipher, SSL_ALLOWED);

This is existing code.  I'm wondering if we should add an "else"
block that exits after printing an error message; right now we
silently ignore a cipher "letter" that we don't support.

2. security/nss/cmd/strsclnt/strsclnt.c

I have one additional comment about this file.

The original code uses

    Usage("strsclnt");

if the cipher "letter" is not a letter.  In the new code, we
use fprintf(stderr) statements.  We should either change the
fprintf(stderr) statements to Usage("strsclnt"), or add the
program name "strsclnt" to the fprintf(stderr) statements.

3. security/nss/cmd/tstclnt/tstclnt.c and
    security/nss/cmd/vfyserv/vfyserv.c

Similar to my comment for strsclnt.c, except that these two
programs use the variable 'progName' instead of a hardcoded
string for the program name.

4. security/nss/tests/ssl/ecssl.sh

>-  elif [ sparam = "-c ABCDEFGHIJKLMNOPQRSTabcdefghijklmnvy" ] ; then # "$1" = "cov" ] ; then
>+  elif [ sparam = "-c ABCDEF:C001:C002:C003:C004:C005:C006:C007:C008:C009:C00A:C00B:C00C:C00D:C00E:C00F:C010:C011:C012:C013:C014:C015:C001abcdefghijklmnvyz" ] ; then # "$1" = "cov" ] ; then

>-  sparam="-c ABCDEFGHIJKLMNOPQRSTabcdefghijklmnvyz"
>+  sparam="-c ABCDEF:C001:C002:C003:C004:C005:C006:C007:C008:C009:C00A:C00B:C00C:C00D:C00E:C00F:C010:C011:C012:C013:C014:C015:C001abcdefghijklmnvyz"

Two problems.
- ":C001" appears twice.  There is a redundant ":C001" at the end
  of the :WXYZ sequence.
- ":C015" (TLS_ECDH_anon_NULL_WITH_SHA) is specified but not implemented

5. security/nss/tests/ssl/ecsslauth.txt

Just curious: why are some of the expected return codes changed?
Is it because you are just matching their current values in
sslauth.txt?

6. security/nss/tests/ssl/ecsslstress.txt

Just curious: can we use the ECC cipher suites in SSL3?
I asked because we aren't doing ECC SSL3 stress test.

Does our code only allow the ECC cipher suites to be used
in TLS?

Comment 5

[Robert Relyea]

RE test cases:

I'm pretty sure the EC ciphers are only available in TLS, since officially they need the TLS extensions to function properly.

I would also like to see the EC tests run automatically from all.sh if NSS_ENABLE_ECC is turned on in the environment.

bob

Comment 6

[Vipul Gupta]

I noticed that the patch also reordered some existing
cipher suites (e.g. ecdh-ecdsa-aes256-sha was moved
ahead of ecdh-rsa-aes256-sha). These also ought to 
be corrected. From my recollection of the ordering rules 
(as explained by Nelson a few years ago).

- the ciphers are sorted in decreasing order of symmetric 
   encryption key size
- for a given symmetric key size, more efficient algorithms are placed
   first (e.g. 128-bit RC4 should always be ordered before AES128)
- within these ordering rules, key exchange methods are
   sorted first by whether or not they provide forward secrecy
   and then by efficiency

Based on these rules, I had ordered
 - ECDHE-ECDSA before ECDHE-RSA
    (RSA signing of serverkeyexchange params is more expensive
    than ECDSA signing).
 - ECDH-RSA before ECDH-ECDSA
    (the resoning for this is that in both methods, the server does
    an ECC op but for the client, RSA verification is typically faster
    than ECDSA verification)
(of course, both ECDHE methods should be before their
ECDH versions)

I'll sit down with Douglas and make sure the ordering follows
these rules consistently. I forgot to tell Douglas about this earlier.

Thanks for catching this.

vipul

(In reply to comment #3)
> 1. security/nss/lib/ssl/ssl3con.c
> 
> We need to review the ordering of the SSL3 cipher suites
> in the cipherSuites array.  There is an inconsistency between
> the two blocks of ECC cipher suites below:
> 
> > #ifdef NSS_ENABLE_ECC
> >+ { TLS_ECDHE_ECDSA_WITH_RC4_128_SHA,       SSL_NOT_ALLOWED, PR_FALSE,PR_FALSE},
> >+ { TLS_ECDHE_RSA_WITH_RC4_128_SHA,         SSL_NOT_ALLOWED, PR_FALSE,PR_FALSE},
> >  { TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,   SSL_NOT_ALLOWED, PR_FALSE,PR_FALSE},
> >  { TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,     SSL_NOT_ALLOWED, PR_FALSE,PR_FALSE},
> > #endif /* NSS_ENABLE_ECC */
> 
> > #ifdef NSS_ENABLE_ECC
> >- { TLS_ECDH_RSA_WITH_RC4_128_SHA,          SSL_NOT_ALLOWED, PR_FALSE,PR_FALSE},
> >- { TLS_ECDH_RSA_WITH_AES_128_CBC_SHA,      SSL_NOT_ALLOWED, PR_FALSE,PR_FALSE},
> >  { TLS_ECDH_ECDSA_WITH_RC4_128_SHA,        SSL_NOT_ALLOWED, PR_FALSE,PR_FALSE},
> >  { TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA,    SSL_NOT_ALLOWED, PR_FALSE,PR_FALSE},
> >+ { TLS_ECDH_RSA_WITH_RC4_128_SHA,          SSL_NOT_ALLOWED, PR_FALSE,PR_FALSE},
> >+ { TLS_ECDH_RSA_WITH_AES_128_CBC_SHA,      SSL_NOT_ALLOWED, PR_FALSE,PR_FALSE},
> > #endif /* NSS_ENABLE_ECC */
> 
> In the first block, we have RC4,RC4,AES,AES, whereas in the
> second block we have RC4,AES,RC4,AES.

Comment 7

[Douglas Stebila]

Attachment: Update NSS to draft 12 plus upcoming revisions based on comments

Thank you all for your detailed and quick review of this patch.  I will respond to all of the previous comments in this comment.  If I do not address a comment, then I fully incorporated its suggestion into the new patch.

(In reply to comment #3)

> 4. security/nss/lib/ssl/sslimpl.h
> 
> > #ifdef NSS_ENABLE_ECC
> >-#define ssl_V3_SUITES_IMPLEMENTED 40
> >+#define ssl_V3_SUITES_IMPLEMENTED 47
> > #else
> > #define ssl_V3_SUITES_IMPLEMENTED 26
> > #endif /* NSS_ENABLE_ECC */
> 
> I have a question for Nelson and a question for Douglas
> and Vipul.
> 
> Douglas,Vipul: the delta of your changes to the cipherSuites
> array is +6 (7 lines removed, 13 lines added).  Why is
> the new value of ssl_V3_SUITES_IMPLEMENTED 47, not 46?

Because I can't count.  It has been corrected to 46.

(In reply to comment #4)

> First, I found out why the value of ssl_V3_SUITES_IMPLEMENTED
> is off by 3.  It is a bug introduced in NSS 3.11 when we removed
> FORTEZZA support (bug 319240).

Please feel free to adjust the number as appropriate.

> > 	    if (cipher > 0) {
> > 		SECStatus status;
> > 		status = SSL_CipherPrefSetDefault(cipher, SSL_ALLOWED);
> 
> This is existing code.  I'm wondering if we should add an "else"
> block that exits after printing an error message; right now we
> silently ignore a cipher "letter" that we don't support.

I have done this.

After doing this, I noticed a failure in all.sh without ECC enabled; it seems that the Fortezza cipher suites were not removed from tests/ssl/ssl.sh.  I have done this.

> 2. security/nss/cmd/strsclnt/strsclnt.c
> 
> I have one additional comment about this file.
> 
> The original code uses
> 
>     Usage("strsclnt");
> 
> if the cipher "letter" is not a letter.  In the new code, we
> use fprintf(stderr) statements.  We should either change the
> fprintf(stderr) statements to Usage("strsclnt"), or add the
> program name "strsclnt" to the fprintf(stderr) statements.
> 
> 3. security/nss/cmd/tstclnt/tstclnt.c and
>     security/nss/cmd/vfyserv/vfyserv.c
> 
> Similar to my comment for strsclnt.c, except that these two
> programs use the variable 'progName' instead of a hardcoded
> string for the program name.

This occurs throughout the program and I am unsure on how you want to resolve.  Feel free to adjust the patch accordingly.

> 5. security/nss/tests/ssl/ecsslauth.txt
> 
> Just curious: why are some of the expected return codes changed?
> Is it because you are just matching their current values in
> sslauth.txt?

Yes.

> 6. security/nss/tests/ssl/ecsslstress.txt
> 
> Just curious: can we use the ECC cipher suites in SSL3?
> I asked because we aren't doing ECC SSL3 stress test.
> 
> Does our code only allow the ECC cipher suites to be used
> in TLS?

No, ECC cipher suites can be used in SSL3 as well.  This is done in the ecsslcov.txt tests as part of all.sh.  I have made a similar change in ecsslstress.txt.

(In reply to comment #5)

> I'm pretty sure the EC ciphers are only available in TLS, since officially they
> need the TLS extensions to function properly.

Not true in our implementation; see above.

> I would also like to see the EC tests run automatically from all.sh if
> NSS_ENABLE_ECC is turned on in the environment.

(In reply to comment #6)

> I noticed that the patch also reordered some existing
> cipher suites (e.g. ecdh-ecdsa-aes256-sha was moved
> ahead of ecdh-rsa-aes256-sha). These also ought to 
> be corrected. From my recollection of the ordering rules 
> (as explained by Nelson a few years ago).

I have reordered the cipher suites to match the order that I think your algorithm specifies.  Could you check to make sure?

Douglas

Comment 8

[Douglas Stebila]

Attachment: Update NSS to draft 12 plus upcoming revisions, patch version 3

One more change -- the ECDHE curve is changed from being hardcoded to secp224r1 to secp256r1.

Comment 9

[Vipul Gupta]

(In reply to comment #8)
> One more change -- the ECDHE curve is changed from being hardcoded to secp224r1
> to secp256r1.

This is just a short term solution to enable interoperability
with clients that are known to support only secp256r1 and 
secp384r1 (NSA's suite B curves). In the longer term, we 
need a flexible mechanism that lets an application specify 
the curve to be used for the SSL server's ephemeral key 
generation. I've opened up a new bug 319327 for this.

vipul

Comment 10

[Wan-Teh Chang]

Comment on attachment 205125 [details] [diff] [review]
Update NSS to draft 12 plus upcoming revisions, patch version 3

Douglas, I understand that you will attach a new
version of the patch soon.  I found an error in this
patch:

In security/nss/lib/ssl/sslenum.c

>@@ -57,16 +59,18 @@
> 
>     /* 128-bit */
> #ifdef NSS_ENABLE_ECC
>-    TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,
>+    TLS_ECDHE_ECDSA_WITH_RC4_128_SHA,
>+    TLS_ECDHE_RSA_WITH_RC4_128_SHA,
>     TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,
>+    TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,
> #endif /* NSS_ENABLE_ECC */

the last two cipher suites should be reversed to match
the ordering in the cipherSuites array in ssl3con.c.

I also suggest that in security/nss/lib/ssl/ssl3ecc.c
you add a reference to bug 319327.

Comment 11

[Douglas Stebila]

Attachment: Update NSS to draft 12 plus upcoming revisions, patch version 4

Changes from previous patch:

Cipher suite order changed in accordance with comment #6 and to resolve comment #10.

ssl3ecc.c cleaned up to remove unnecessary variable.

No conditional tests added in all.sh based on discussion with Wan-Teh.

Comment 12

[Wan-Teh Chang]

Attachment: Update NSS to draft 12 plus upcoming revisions, patch as checked in

I made the following changes to Douglas's patch v4
before I checked it in.

1. I did not reorder SSL_RSA_WITH_RC4_128_MD5 and
SSL_RSA_WITH_RC4_128_SHA per our conference call
this morning.  Only the ECC cipher suites were
reordered.

2. I fixed ssl_V3_SUITES_IMPLEMENTED (bug 319240).

3. In the four command-line tools, I avoided copying
the cipher string to the cipherCode buffer.

Checking in cmd/selfserv/selfserv.c;
/cvsroot/mozilla/security/nss/cmd/selfserv/selfserv.c,v  <--  selfserv.c
new revision: 1.69; previous revision: 1.68
done
Checking in cmd/strsclnt/strsclnt.c;
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v  <--  strsclnt.c
new revision: 1.44; previous revision: 1.43
done
Checking in cmd/tstclnt/tstclnt.c;
/cvsroot/mozilla/security/nss/cmd/tstclnt/tstclnt.c,v  <--  tstclnt.c
new revision: 1.43; previous revision: 1.42
done
Checking in cmd/vfyserv/vfyserv.c;
/cvsroot/mozilla/security/nss/cmd/vfyserv/vfyserv.c,v  <--  vfyserv.c
new revision: 1.12; previous revision: 1.11
done
Checking in lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.77; previous revision: 1.76
done
Checking in lib/ssl/ssl3ecc.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ecc.c,v  <--  ssl3ecc.c
new revision: 1.4; previous revision: 1.3
done
Checking in lib/ssl/ssl3prot.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3prot.h,v  <--  ssl3prot.h
new revision: 1.11; previous revision: 1.10
done
Checking in lib/ssl/sslenum.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslenum.c,v  <--  sslenum.c
new revision: 1.13; previous revision: 1.12
done
Checking in lib/ssl/sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.43; previous revision: 1.42
done
Checking in lib/ssl/sslinfo.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslinfo.c,v  <--  sslinfo.c
new revision: 1.15; previous revision: 1.14
done
Checking in lib/ssl/sslproto.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslproto.h,v  <--  sslproto.h
new revision: 1.10; previous revision: 1.9
done
Checking in lib/ssl/sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.45; previous revision: 1.44
done
Checking in tests/ssl/ecssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ecssl.sh,v  <--  ecssl.sh
new revision: 1.4; previous revision: 1.3
done
Checking in tests/ssl/ecsslauth.txt;
/cvsroot/mozilla/security/nss/tests/ssl/ecsslauth.txt,v  <--  ecsslauth.txt
new revision: 1.2; previous revision: 1.1
done
Checking in tests/ssl/ecsslcov.txt;
/cvsroot/mozilla/security/nss/tests/ssl/ecsslcov.txt,v  <--  ecsslcov.txt
new revision: 1.2; previous revision: 1.1
done
Checking in tests/ssl/ecsslstress.txt;
/cvsroot/mozilla/security/nss/tests/ssl/ecsslstress.txt,v  <--  ecsslstress.txt
new revision: 1.2; previous revision: 1.1
done
Checking in tests/ssl/ssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v  <--  ssl.sh
new revision: 1.62; previous revision: 1.61
done

Comment 13

[Wan-Teh Chang]

Attachment: Update NSS_3_11_BRANCH to draft 12 plus upcoming revisions

As we agreed in the NSS meeting Thursday morning, I
carried this ECC TLS patch back to the NSS_3_11_BRANCH.

Checking in cmd/selfserv/selfserv.c;
/cvsroot/mozilla/security/nss/cmd/selfserv/selfserv.c,v  <--  selfserv.c
new revision: 1.68.2.1; previous revision: 1.68
done
Checking in cmd/strsclnt/strsclnt.c;
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v  <--  strsclnt.c
new revision: 1.43.2.3; previous revision: 1.43.2.2
done
Checking in cmd/tstclnt/tstclnt.c;
/cvsroot/mozilla/security/nss/cmd/tstclnt/tstclnt.c,v  <--  tstclnt.c
new revision: 1.42.2.1; previous revision: 1.42
done
Checking in cmd/vfyserv/vfyserv.c;
/cvsroot/mozilla/security/nss/cmd/vfyserv/vfyserv.c,v  <--  vfyserv.c
new revision: 1.11.2.1; previous revision: 1.11
done
Checking in lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.76.2.1; previous revision: 1.76
done
Checking in lib/ssl/ssl3ecc.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ecc.c,v  <--  ssl3ecc.c
new revision: 1.3.2.1; previous revision: 1.3
done
Checking in lib/ssl/ssl3prot.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3prot.h,v  <--  ssl3prot.h
new revision: 1.10.2.1; previous revision: 1.10
done
Checking in lib/ssl/sslenum.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslenum.c,v  <--  sslenum.c
new revision: 1.12.2.1; previous revision: 1.12
done
Checking in lib/ssl/sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.42.2.2; previous revision: 1.42.2.1
done
Checking in lib/ssl/sslinfo.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslinfo.c,v  <--  sslinfo.c
new revision: 1.14.2.1; previous revision: 1.14
done
Checking in lib/ssl/sslproto.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslproto.h,v  <--  sslproto.h
new revision: 1.9.2.1; previous revision: 1.9
done
Checking in lib/ssl/sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.44.2.1; previous revision: 1.44
done
Checking in tests/ssl/ecssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ecssl.sh,v  <--  ecssl.sh
new revision: 1.3.28.1; previous revision: 1.3
done
Checking in tests/ssl/ecsslauth.txt;
/cvsroot/mozilla/security/nss/tests/ssl/ecsslauth.txt,v  <--  ecsslauth.txt
new revision: 1.1.82.1; previous revision: 1.1
done
Checking in tests/ssl/ecsslcov.txt;
/cvsroot/mozilla/security/nss/tests/ssl/ecsslcov.txt,v  <--  ecsslcov.txt
new revision: 1.1.82.1; previous revision: 1.1
done
Checking in tests/ssl/ecsslstress.txt;
/cvsroot/mozilla/security/nss/tests/ssl/ecsslstress.txt,v  <--  ecsslstress.txt
new revision: 1.1.82.1; previous revision: 1.1
done
Checking in tests/ssl/ssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v  <--  ssl.sh
new revision: 1.61.2.1; previous revision: 1.61
done

Comment 14

[Wan-Teh Chang]

Douglas described the remaining work for complete
compliance with the draft in comment 1.  The only
mandatory requirement we still need to implement is
the support for ECC TLS extensions.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Taking off Wan-Teh's plate.

Comment 16

[Julien Pierre]

This is P1 per our meeting.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Attachment: implement ECC hello extensions, v1, client and partial server

This patch adds support for the client sending the ECC hello extensions,
for the server receiving and parsing the hello extensions, and the for
server sending back the supportedPointFormat extension in response to the 
client's extension.  

I believe the work for the client is effectively complete with this patch.
The server work is not yet complete.  The code to use the negotiated set of
curves remains to be completed.  Nevertheless, I want to see this go into 
the tree so that the browser can make use of it more-or-less immediately.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 217980 [details] [diff] [review]
implement ECC hello extensions, v1, client and partial server

Sorry, this patch is not ready for prime time.  Another patch forthcoming soon, I think.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Attachment: implement TLS/ECC hello extensions, v2

The server side of this patch is much more complete than before, and a bug
in the client side is fixed.  This still isn't perfect, but it passses all.sh.

Comment 20

[Robert Relyea]

Comment on attachment 218017 [details] [diff] [review]
implement TLS/ECC hello extensions, v2

r+ with the following comments.

The #ifdef PARANOIA code probably should be #ifdef REMOVABLE_INSTALLABLE_CIPHERS or something like that. For servers it's fine. Currently we don't have any REMOVABLE_INSTALLABLE_CIPHERS (since we removed FORTEZZA), so we are OK for clients. If we ever have a case where a client-like application needs to negotiate with as a server (AIM doing peer to peer, for instance), and we ever have any conditional cipher suites that may be based on removable tokens (like FORTEZZA), we may need to turn this code on again (OK to have it off right now).

------------------------------------------------

>+ static const PRUint8 EClist[55] = 

It's a little more maintainable if we declar this as:

static const PRUint8 EClist[] = 

and let the compiler pick the size. Same with ECPtFmt below.

------------------------------------

In ssl3_CreateECDHEphemeralKeys(sslSocket *ss)

You added code which checks to make sure the ECC curve of the server is in the negotiated curve list for kea_ecdhe_ecdsa, however you did not use that value to find an appropriate curve in the kea_ecdhe_rsa case. I gave this an r+ because I thought you said you were looking in that code, and getting the rest of this code in is better than what we have. Before we ship, though, we should do something 'reasonable' here (like pick a minimum curve size based on the RSA key or the symetric key size, select the smallest negotiated curve at or above that size, if none found, pick the largest negotiated curve).

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 218017 [details] [diff] [review]
implement TLS/ECC hello extensions, v2

Checked in on trunk.
Checking in ssl3con.c; new revision: 1.86; previous revision: 1.85
Checking in ssl3ecc.c; new revision: 1.7; previous revision: 1.6
Checking in sslimpl.h; new revision: 1.49; previous revision: 1.48

Log Comment:
Implement TLS Hello extensions for ECC.  Bug 236245. r=rrelyea. 
This patch has a known problem, choosing ephemeral ECDH curves 
according to the wrong (suboptimal, non-FIPS) criteria.  
Modified Files: ssl3con.c ssl3ecc.c sslimpl.h

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Attachment: correct parsing of ECC params in server key exchange

This corrects an error that was introduced by attachment 205783 [details] [diff] [review]

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 205783 [details] [diff] [review]
Update NSS to draft 12 plus upcoming revisions, patch as checked in

The checkin of this patch introduced a regression.

>@@ -445,7 +425,7 @@
>     /* XXX This works only for named curves, revisit this when
>      * we support generic curves.
>      */
>-    ec_params.len = 2;
>+    ec_params.len = 3;
>     ec_params.data = paramBuf;
>     rv = ssl3_ConsumeHandshake(ss, ec_params.data, ec_params.len, 
> 			       &b, &length);

Problem is that paramBuf is an array of 2 bytes, so this change now reads
3 bytes into an array of two bytes.  

Patch for this forthcoming.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Fix buffer overflow regression (checked in on trunk)

Wan-Teh, 
asking you for review since you checked in the code being changed here.
But one review will suffice.

Comment 25

[Wan-Teh Chang]

Comment on attachment 219068 [details] [diff] [review]
Fix buffer overflow regression (checked in on trunk)

It seems that we can use a stack buffer for ec_params.data
in ssl3_SendECDHServerKeyExchange, too.

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 219068 [details] [diff] [review]
Fix buffer overflow regression (checked in on trunk)

Checking in ssl3ecc.c; new revision: 1.9; previous revision: 1.8

Comment 27

[Wan-Teh Chang]

Attachment: Use a stack buffer for ec_params.data in ssl3_SendECDHServerKeyExchange

This patch implements my suggestion in comment 25.

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 219207 [details] [diff] [review]
Use a stack buffer for ec_params.data in ssl3_SendECDHServerKeyExchange

r=nelsonb

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Encode length of curve list as two bytes

alexei, Please review.

Comment 30

[Wan-Teh Chang]

Comment on attachment 219207 [details] [diff] [review]
Use a stack buffer for ec_params.data in ssl3_SendECDHServerKeyExchange

I checked in this patch (use a stack buffer in
ssl3_SendECDHServerKeyExchange) on the NSS trunk
(3.12) and the NSS_3_11_BRANCH (3.11.1).

Checking in ssl3ecc.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ecc.c,v  <--  ssl3ecc.c
new revision: 1.10; previous revision: 1.9
done

Checking in ssl3ecc.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ecc.c,v  <--  ssl3ecc.c
new revision: 1.3.2.4; previous revision: 1.3.2.3
done

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

There is one portion of RFC 4492 that remains unimplemented in NSS.
RFC 4492 defines 3 methods of client authentication, which are:

          Client Authentication Method   
          ---------------------   
          ECDSA_sign              
          ECDSA_fixed_ECDH        
          RSA_fixed_ECDH    

Presently, NSS implements only the first one of those 3.
unlike the second and third methods, the first one is compatible with any
cert with an ECC public key.  So it is the most general of the three.

I was going to leave this bug open until all 3 methods were implemented,
but now I will file a new RFE for the other two methods.  

Comment 32

[Vipul Gupta]

(In reply to comment #31)
> I was going to leave this bug open until all 3 methods were implemented,
> but now I will file a new RFE for the other two methods.  

That makes a lot of sense. The other two methods require
the client to have a cert on the same curve as the server.

vipul

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #31)

One correction/clarification.  I wrote:
> 
> Unlike the second and third methods, the first one is compatible with any
> cert with an ECC public key.
Should read:
"... compatible with any cert with an *ECDSA-capable* ECC public key."

Comment 34

[Nelson Bolyard (seldom reads bugmail)]

Markign resolved/fixed in 3.11.1, with the observation that certain work 
remains to be done, as noted in bug 340043, bug 340044, and bug 340046.