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 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?

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

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.

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

(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 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.

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.

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 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 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 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.

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.

(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

(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."