Closed Bug 332350 Opened 18 years ago Closed 18 years ago

SSL ECC negotiates wrong cipher suite for cert signature

Categories

(NSS :: Libraries, defect, P1)

3.11

Tracking

(Not tracked)

RESOLVED FIXED
3.11.1

People

(Reporter: nelson, Assigned: nelson)

References

Details

(Whiteboard: ECC)

Attachments

(3 files, 2 obsolete files)

This reports a bug in the NSS ECC TLS implementation.
It is *NOT* related to the new TLS ECC Hello extensions.

~half of the TLS ECC cipher suites are for use with certs that
have ECDSA signatures on the certs themselves.  
~half are for certs that have RSA signatures on them.  

The bug is that libSSl will happily negotiate a cipher suite
that specifies an RSA-signed cert, even though it has only an
ECDSA-signed cert, or vice versa.  

I fixed this while working on the ECC Hello extensions, but
I realized this bug is independent of ECC Hello extensions, 
because it also occurs when using a SSLV2 compatible client
hello to negotiate ECC cipher suites.  

So, I will separate the patch for this bug from the ECC 
hello patch, so it can be separately reviewed.

This bug blocks bug 332222, but the ECC hello extensions
work does NOT block that bug.
No longer blocks: 332222
Depends on: 332222
In particular, libSSL will negotiate an ECDH_RSA cipher suite even when the 
server's one-and-only ECC public key cert does not have an RSA signature.

Likewise, I believe it will netogiate an ECDH_ECDSA cipher suite even when
the server's one-and-only ECC cert does not have an ECDSA signature. 

I have a patch that (I believe) fixes this, but I cannot commit it until bug
332222 is fixed. 
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → 3.11.1
This patch disbles cipher suites that are incompatible with the 
configured server certs.
Comment on attachment 217131 [details] [diff] [review]
patch v1, disable cipher suites incompatible with our certs

Vipul, please review this patch.
Attachment #217131 - Attachment description: patch v1, dsiable cimpher suites incompatible with our certs → patch v1, disable cipher suites incompatible with our certs
Attachment #217131 - Flags: review?(vipul.gupta)
nelson, any ETA on when we can have a fix for this checked in ?. 

Since we are doing ECC interop testing now, I would like to test with this fix checked in , if possible.

Comment on attachment 217131 [details] [diff] [review]
patch v1, disable cipher suites incompatible with our certs

Hi Nelson,

As per our phone call, the only suggestions I have are renaming the method to ssl3_FilterECCipherSuitesBySrvrCert and the ecCert variable to srvrCert (because the filtering can also be based on an RSA cert).

Everything else looks good. Also note that I haven't had the time to compile and test the patch (all.sh doesn't have the appropriate tests yet as per bug 332222).

vipul
Attachment #217131 - Flags: review?(vipul.gupta) → review+
Requesting second review for 3.11 branch
Attachment #217131 - Attachment is obsolete: true
Attachment #217311 - Flags: review?(julien.pierre.bugs)
Comment on attachment 217311 [details] [diff] [review]
patch v2, with Vipul's suggestions (checked in)

Patch committed on trunk.
Checking in ssl3con.c; new revision: 1.84; previous revision: 1.83
Checking in ssl3ecc.c; new revision: 1.5;  previous revision: 1.4
Checking in sslimpl.h; new revision: 1.47; previous revision: 1.46

Awaiting second review for 3.11 branch
Whiteboard: ECC
Comment on attachment 217311 [details] [diff] [review]
patch v2, with Vipul's suggestions (checked in)

I don't like this patch because there is no way for a single socket to support all cipher suites.  At least one set of ECC cipher suites has to be disabled. We have never had such a precedent before of mutually exclusive cipher suites.

It will cause confusion for the end-users of our applications to figure out why they can't ever get those cipher suites to work in an application at the same time, unless the application implements logic that explicitly precludes that configuration by automatically disabling one or the other visually, therefore depending on internal knowledge of the NSS limitation, and making it hard to fix later - or at least not just with an NSS change.

I know you explained that this limitation was put in to help meet rapidly approaching deadlines. I believe in this case, the sanity of our API should have priority.
Comment on attachment 217311 [details] [diff] [review]
patch v2, with Vipul's suggestions (checked in)

The patch accomplishes what was intended, so r+ on that basis.

However, before this bug is closed as fixed, I would really like to see some documentation of the mutual exclusion of the DH/RSA and DH/DSA cipher suites in the header files, probably where these cipher suites are defined. This is a caveat that anyone who tries to use this code should know about.

Also, it would be good if SSL_ConfigSecureServer() failed if an application tried to register both types of certs. 

Presumably, given the way the API is currently defined, an application would do this by making two calls, passing different certs (and possibly the same ECDH private key) and ssl_kea_ecdh as the SSLKeaType .

Currently, I see that SSL_ConfigSecureServer() replaces the key and cert for each key type if they already exist in the socket. So we probably can't just return an "already exists" type of error. Maybe the check would belong in the validor function in bug 134122 instead...
Attachment #217311 - Flags: review?(julien.pierre.bugs) → review+
Comment on attachment 217311 [details] [diff] [review]
patch v2, with Vipul's suggestions (checked in)

Patch checked in on branch
Checking in ssl3con.c; new revision: 1.76.2.8; previous revision: 1.76.2.7
Checking in ssl3ecc.c; new revision: 1.3.2.2; previous revision: 1.3.2.1
Checking in sslimpl.h; new revision: 1.42.2.4; previous revision: 1.42.2.3
Attachment #217311 - Attachment description: patch v2, with Vipul's suggestions → patch v2, with Vipul's suggestions (checked in)
This patch makes 3 changes:
1) it adds a new ifdef which enables SSL to limit itself to the 3 Suite B curves.
2) it corrects the creation and parsing of the Supported Curve extension to
   conform with the lastest definition, by using 2 bytes to encode the list 
   length,
3) it changes the algorithm that picks the curve for ECDHE to choose a curve
   that is at least as strong as the "weakest link", is mutually supported 
   by client and server, and is the fastest for its size.  

Note that we can improve the curve selection by adding entries to this new
bits2curve table, as we learn more about the relative speeds of the 
different curves.

Bob or Alexei or Julien, please review
Attachment #219399 - Flags: superreview?(rrelyea)
Attachment #219399 - Flags: review?
Attachment #219399 - Flags: review?(julien.pierre.bugs)
Attachment #219399 - Flags: review?(alexei.volkov.bugs)
Attachment #219399 - Flags: review?
Comment on attachment 219399 [details] [diff] [review]
Choose ECDHE curve to match "weakest link" - ifdef limit to 3 curves

/* XXX SHOULD CALL ssl3_CreateECDHEphemeralKeys here, instead! */
Attachment #219399 - Flags: superreview?(rrelyea) → superreview+
Comment on attachment 219399 [details] [diff] [review]
Choose ECDHE curve to match "weakest link" - ifdef limit to 3 curves

r+ rrelyea with comments:

None of these comments should preclude checkin or even shipping.

The following curve should be at the top of our list. It is our fastest curve (only exception may be platforms that use the 
Floating point curves, then it should be just below ec_secp160r2):
{	192,     ec_secp192r1    /*  = 19,  */       },

Answer to the comment:
/* XXX SHOULD CALL ssl3_CreateECDHEphemeralKeys here, instead! */
So.. for the clients I think we want to go ahead and continue to generate new keys on the fly, however, and ifdef might be nice when we want to do server performance testing. To be able to saturate the server with a finite clients, it's useful to skip the generate keys (as well as the signature verification on ECDHE) on the clients we use to load the server. We definately do not want to skip these steps (especially the latter) in production, however.

bob
Comment on attachment 219399 [details] [diff] [review]
Choose ECDHE curve to match "weakest link" - ifdef limit to 3 curves

Thanks for the review, Bob.
Checked in on trunk
Checking in ssl3ecc.c; new revision: 1.11; previous revision: 1.10
Checking in sslimpl.h; new revision: 1.51; previous revision: 1.50
Attachment #219399 - Flags: review?(julien.pierre.bugs)
Attachment #219399 - Flags: review?(alexei.volkov.bugs)
Bob, I believe this patch causes secp192r1 to be considered as a preferred
curve for several lesser sizes.  Does this accomplish what you had in mind?

Should we also do something similar for secp256<something> ?
Attachment #219471 - Flags: superreview?(rrelyea)
it does but is it necessary to have three entries? I don't think the table necessarily needs to be ordered by bit size, but simply by speed of algorithm.

Your code simply looks for the first algorithm that in the list that provides sufficient key strength and is a supported curve.

bob
Is this what you meant, Bob?
Attachment #219471 - Attachment is obsolete: true
Attachment #219714 - Flags: review?
Attachment #219471 - Flags: superreview?(rrelyea)
Attachment #219714 - Flags: review? → review?(rrelyea)
Comment on attachment 219714 [details] [diff] [review]
promote secp192r1, v2 

r+ = relyea
yup, looks good.

bob
Attachment #219714 - Flags: review?(rrelyea) → review+
Checked in patch to promote secp192r1.
Checking in ssl3ecc.c; new revision: 1.12; previous revision: 1.11
Checking in ssl3ecc.c; new revision: 1.3.2.7; previous revision: 1.3.2.6

I believe this bug is now fixed.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment on attachment 219399 [details] [diff] [review]
Choose ECDHE curve to match "weakest link" - ifdef limit to 3 curves

This patch has a typo in the comment:

>+/* Prefabricated TLS client hello extension, Elliptic Curves List,
>+ * offers only 3 curves, the Suite B curves, 23-35 
>+ */

"23-35" should be "23-25".  I fixed it on the NSS trunk (NSS 3.12).

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

Bob fixed it on the NSS_3_11_BRANCH in revision 1.3.2.6.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: