Closed
Bug 327105
Opened 19 years ago
Closed 18 years ago
With ECC enabled, servers negotiate _DHE_ cipher suites
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)
1.02 KB,
patch
|
julien.pierre
:
review+
vipul.gupta
:
review+
wtc
:
superreview+
|
Details | Diff | Splinter Review |
2.54 KB,
text/plain
|
Details | |
2.73 KB,
text/plain
|
Details |
When NSS is built with ECC disabled, (as 3.11.0 was built) if a server enables any _DHE_RSA cipher suites, they are not negotiated. The server behaves as if they were disabled. This is due to a benign bug in ssl3_config_match_init in ssl3con.c. I call it benign because actually ssl3con does not implement the server side of the _DHE_ cipher suites, so if/when NSS does negotiate that cipher suite as a server, NSS doesn't correctly emit the server key exchange messsage needed for _DHE_, resulting in a protocol error. So, until the server side of _DHE_ is implemented, it's actually to our benefit for this bug to remain. Now, here's the problem. When NSS is built with NSS_ENABLE_ECC defined, the conditionally compiled code corrects the above error. Consequently, when running with an NSS build that has ECC enabled, SSL servers that have enabled _DHE_ cipher suites find that NSS negotiates a _DHE_ cipher suite and then causes a protocol error. In this case, fixing one bug reveals another. Of course, there is a trivial workaround. Servers can simply not enable _DHE_ cipher suites. But some server products have been enabling the _DHE_ cipher suites for years. If they now "upgrade" to an ECC-enabled NSS, they stop working. The real fix is to implement server side DHE in NSS, and fix ssl3_config_match_init to properly check for the presences of BOTH the "key exhange" mechanism AND the server authentication mechanism. In the meantime, a tiny change to the source of ssl3_config_match_init will restore the benign bug. Patch forthcoming.
Assignee | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P1
Summary: When ECC enabled, servers negotiate _DHE_ cipher suites → With ECC enabled, servers negotiate _DHE_ cipher suites
Assignee | ||
Comment 1•19 years ago
|
||
Will ask for review once this is tested.
Comment 2•18 years ago
|
||
Nelson, I would call the behavior in the #ifndef NSS_ENABLE_ECC block correct - the DHE RSA cipher suites are never negotiated on the server-side. I think it's not a bug but a feature. The bug is in the code in the #else block, which your patch corrects, where the DHE RSA cipher suites get negotiated even though we don't have code behind them. This seems like a fine short-term workaround. Long-term, it would be better to have all the cipher suites implemented for both client and server, so that there is no confusion for our customers.
Comment 3•18 years ago
|
||
Ssltap output of connection between tstclnt and selfserv with client and server specifying the same set of cipher suites.
Updated•18 years ago
|
Attachment #212057 -
Attachment description: ssltap output with patch applied → ssltap output without patch applied
Comment 4•18 years ago
|
||
The previous attachment was created BEFORE the patch was applied. This attachment is ssltap output after the patch.
Comment 5•18 years ago
|
||
(In reply to comment #3) > Created an attachment (id=212057) [edit] > ssltap output with patch applied > > Ssltap output of connection between tstclnt and selfserv with client and server > specifying the same set of cipher suites. > Note that comment #3 should say "...before patch was applied."
Assignee | ||
Comment 6•18 years ago
|
||
*** Bug 325347 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 7•18 years ago
|
||
Comment on attachment 211822 [details] [diff] [review] patch v1 Now that this patch has been tested, I'm asking for reviews.
Attachment #211822 -
Attachment description: patch v1 (untested) → patch v1
Attachment #211822 -
Flags: superreview?(wtchang)
Attachment #211822 -
Flags: review?(julien.pierre.bugs)
Updated•18 years ago
|
Attachment #211822 -
Flags: review?(julien.pierre.bugs) → review+
Assignee | ||
Comment 8•18 years ago
|
||
Comment on attachment 211822 [details] [diff] [review] patch v1 Vipul, please review this tiny patch to your EC code. Thanks.
Attachment #211822 -
Flags: review?(vipul.gupta)
Comment 9•18 years ago
|
||
(In reply to comment #8) > (From update of attachment 211822 [details] [diff] [review] [edit]) > Vipul, please review this tiny patch to your EC code. If the server never negotiates DHE, the condition on line 6350 will never be true. But, for consistency, do you think it makes sense to enclose that line within #if NSS_SERVER_DHE_IMPLEMENTED #endif vipul
Comment 10•18 years ago
|
||
Comment on attachment 211822 [details] [diff] [review] patch v1 r=wtc. I'd just delete the kea_dhe_rsa case from that switch statement. Will we need to add a kea_dhe_dss case to that switch statement when we implement the _DHE_ cipher suites on the server side?
Attachment #211822 -
Flags: superreview?(wtchang) → superreview+
Comment 11•18 years ago
|
||
Vipul, The code you asked about in comment 9 has this XXX comment: /* XXX SSLKEAType isn't really a good choice for * indexing certificates (it breaks when we deal * with (EC)DHE-* cipher suites. This hack ensures * the RSA cert is picked for (EC)DHE-RSA. * Revisit this when we add server side support * for ECDHE-ECDSA or client-side authentication * using EC certificates. */ if ((ss->ssl3.hs.kea_def->kea == kea_ecdhe_rsa) || (ss->ssl3.hs.kea_def->kea == kea_dhe_rsa)) { certIndex = kt_rsa; } else { certIndex = ss->ssl3.hs.kea_def->exchKeyType; } Since NSS has server side support for ECDHE-ECDSA and client-side authentication using EC certificates, should we revisit this now?
Comment 12•18 years ago
|
||
(In reply to comment #11) Wan-Teh, The code snippet in comment #11 is only executed on the server (see line 6338: if ss->sec.isServer) and does not impair a client's ability to send an ECC cert for ECDSA-based authentication (I'm not sure if this answers your question). BTW, I'll go ahead and mark this patch with review+ and let Nelson decide what he wants to do about line 6350. vipul
Comment 13•18 years ago
|
||
Comment on attachment 211822 [details] [diff] [review] patch v1 Please see comment #9 and comment #10 and decide what you think is more appropriate.
Attachment #211822 -
Flags: review?(vipul.gupta) → review+
Assignee | ||
Updated•18 years ago
|
Whiteboard: ECC
Assignee | ||
Comment 14•18 years ago
|
||
Comment on attachment 211822 [details] [diff] [review] patch v1 Committed on NSS_3_11_BRANCH: ssl3con.c new revision: 1.76.2.3; previous revision: 1.76.2.2 Committed on trunk: ssl3con.c new revision: 1.79; previous revision: 1.78
Assignee | ||
Updated•18 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•