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)
Tracking
(Not tracked)
RESOLVED
FIXED
3.11.1
People
(Reporter: nelson, Assigned: nelson)
References
Details
(Whiteboard: ECC)
Attachments
(3 files, 2 obsolete files)
7.30 KB,
patch
|
julien.pierre
:
review+
|
Details | Diff | Splinter Review |
13.25 KB,
patch
|
rrelyea
:
superreview+
|
Details | Diff | Splinter Review |
1.19 KB,
patch
|
rrelyea
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•18 years ago
|
Assignee | ||
Comment 1•18 years ago
|
||
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
Assignee | ||
Updated•18 years ago
|
Priority: -- → P1
Target Milestone: --- → 3.11.1
Assignee | ||
Comment 2•18 years ago
|
||
This patch disbles cipher suites that are incompatible with the configured server certs.
Assignee | ||
Comment 3•18 years ago
|
||
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)
Comment 4•18 years ago
|
||
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 5•18 years ago
|
||
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+
Assignee | ||
Comment 6•18 years ago
|
||
Requesting second review for 3.11 branch
Attachment #217131 -
Attachment is obsolete: true
Attachment #217311 -
Flags: review?(julien.pierre.bugs)
Assignee | ||
Comment 7•18 years ago
|
||
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
Assignee | ||
Updated•18 years ago
|
Whiteboard: ECC
Comment 8•18 years ago
|
||
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 9•18 years ago
|
||
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+
Assignee | ||
Comment 10•18 years ago
|
||
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)
Assignee | ||
Comment 11•18 years ago
|
||
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?
Assignee | ||
Updated•18 years ago
|
Attachment #219399 -
Flags: review?(julien.pierre.bugs)
Attachment #219399 -
Flags: review?(alexei.volkov.bugs)
Attachment #219399 -
Flags: review?
Comment 12•18 years ago
|
||
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 13•18 years ago
|
||
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
Assignee | ||
Comment 14•18 years ago
|
||
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)
Assignee | ||
Comment 15•18 years ago
|
||
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)
Comment 16•18 years ago
|
||
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
Assignee | ||
Comment 17•18 years ago
|
||
Is this what you meant, Bob?
Attachment #219471 -
Attachment is obsolete: true
Attachment #219714 -
Flags: review?
Attachment #219471 -
Flags: superreview?(rrelyea)
Assignee | ||
Updated•18 years ago
|
Attachment #219714 -
Flags: review? → review?(rrelyea)
Comment 18•18 years ago
|
||
Comment on attachment 219714 [details] [diff] [review] promote secp192r1, v2 r+ = relyea yup, looks good. bob
Attachment #219714 -
Flags: review?(rrelyea) → review+
Assignee | ||
Comment 19•18 years ago
|
||
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 20•18 years ago
|
||
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.
Description
•