Open Bug 1013088 Opened 6 years ago Updated 6 years ago

ECC cipher suites in SSL3 mode work on server side but not client with NSS_ECC_MORE_THAN_SUITE_B


(NSS :: Libraries, defect, P2)



(Not tracked)



(Reporter: julien.pierre, Assigned: julien.pierre)



(3 files, 1 obsolete file)

Some of our applications' SSL tests started failing when moving from NSS 3.14.3 to 3.16 recently .

The failures occur when the protocol is set to SSL3 and ECC cipher suites are used. The client is configured with V3 hello, with just one ECC cipher suite enabled, and all other cipher suites disabled. The client experiences an SSL_ERROR_SSL_IS_DISABLED when trying to send the client hello.

I narrowed the problem down to a code change that was made in NSS 3.15 .
as part of the fix for bug 838769, specifically attachment 710877 [details] [diff] [review] .

The test got changed from

#if defined(NSS_ENABLE_ECC) && !defined(NSS_ECC_MORE_THAN_SUITE_B)
if (!total_exten_len || !isTLS) {
/* not sending the elliptic_curves and ec_point_formats extensions */
ssl3_DisableECCSuites(ss, NULL); /* disable all ECC suites */

#if defined(NSS_ENABLE_ECC)
(identical code)

At first, I thought the original code may have well been a typo, and one might consider the current code to be a bug fix.

The idea seems to have been that ECC cipher suites should be disabled in SSL3 mode because the Client Hello cannot include the "supported points" and "points format" extensions.

I looked into this further.

1) First is bug 341707

Apparently, the original implementation by Nelson considered that if the client supported all 25 curves, ie. in the NSS_ECC_MORE_THAN_SUITE_B case, then the client was not required to send the supported curve extension to the server, and thus it was OK for ECC cipher suites to still be negotiated even with SSL3 in this case. That was intentional, and not a bug/typo.

2) RFC 4492 makes it clear that the curves extensions are "SHOULD", not "MUST", ie. they are optional.

In that context, it doesn't really make sense to forcibly disable all ECC cipher suites in SSL3 mode, even though the ECC client hello extensions can't be sent.

The SSL handshake will still fail later on, if the server chooses a curve and point format not supported by the client.

3) The current state of things in NSS 3.15/3.16 is the following :
ECC cipher suites get disabled on the client side in SSL3 mode.
However, they don't get disabled on the server side !

I am able to successfully negotiate ECC with SSL 3.0 mode if I use a client built with NSS 3.14 and NSS_ECC_MORE_THAN_SUITE_B, and restricted to SSL 3.0, against a server built with NSS 3.16 and NSS_ECC_MORE_THAN_SUITE_B, provided the server has both SSL 3.0 and TLS enabled . The NSS 3.16 server is not sending the ECC points format extension in the serverhello in this case. Oddly, the handshake fails with "SSL_NO_CIPHER_OVERLAP" if the server is configured for SSL 3.0 only rather than SSL 3.0 + TLS .

This situation is impossible to test if both client and server are using NSS 3.16 - one has to be roll back the client to NSS 3.14 in order to test this case.

I don't think the current situation is really tenable. NSS should support each cipher suite/protocol combination on both sides or neither - unless one side of the implementation is missing (as it is for some DHE suites - but this is not the case here).

From my point of view, this is is a binary compatibility regression - NSS 3.14 supported ECC with SSL 3.0 on both sides, and NSS 3.16 only supports it on the server side. Our shipping applications - and test client - had both sides working and tested before, but no longer do.

a) my preference would be for attachment 710877 [details] [diff] [review] to be rolled back.
If this is done, then test cases should be added back to test ECC cipher suites with SSL3, and not just TLS  . This probably means rolling back the attachment 710871 [details] [diff] [review] as well - or modifying it to run only in the "Extended ECC" NSS build.

b) one could make the case that Nelson's original decision to assume all curves are supported in the "extended ECC" build wasn't a good one. Indeed, some more curves (Brainpool) have been defined since, in RFC 7027 . And they are not yet supported by NSS even in 3.16 .

Also, NSS's past support for ECC with SSL3 may be considered non-standard.
While RFC 4492 says the ECC hello extensions are optional, it also only defines ECC cipher suites to be supported with TLS 1.0 and 1.1, not SSL 3.0. Some other SSL libraries indeed don't support ECC cipher suites with SSL 3.0.

Those would be not unreasonable arguments for NSS to decide to drop support for ECC with SSL 3.0, even though it's a binary compatibility regression. But if this case, IMO, it should drop it on both sides, not just on one side as was done in the fix for bug 838768 .

If that's the intention, the better fix would be to add all ECC cipher suites to ssl3_CipherSuiteAllowedForVersionRange and mark them as requiring at least TLS 1.0 . In that case, the code that causes ECC cipher suites to be disabled with SSL3 on the client side can be deleted, as it would be redundant.
Assignee: nobody → julien.pierre
Note that this only fixes libssl, but does not restore SSL3 ECC tests in .

This would need further work if we want to restore it only for Extended ECC but not Basic ECC . Right now, the test suite does not seem to have the concept of different tests for Extended ECC vs Basic ECC.

It probably should, if not for this test, to test SSL with sets using the extra set of curves. But I think that's out of scope for this bug.
Attachment #8426147 - Flags: review?(wtc)
Attachment #8426162 - Attachment description: Option 2 : disable ECC cipher suites on both siedes if TLS is disabled. → Option 2 : disable ECC cipher suites on both sides if TLS is disabled.
Priority: -- → P2
Target Milestone: --- → 3.16.2
Attachment #8426147 - Attachment description: Option 1 : Restore ECC ciphers with SSL3 for Extended ECC build only → Option 1 : restore ECC ciphers with SSL3 for Extended ECC build only
Comment on attachment 8426147 [details] [diff] [review]
Option 1 : restore ECC ciphers with SSL3 for Extended ECC build only

Wan-Teh confirmed that support for ECC was intentionally removed, so I'm withdrawing this patch.
Attachment #8426147 - Flags: review?(wtc) → review-
r=wtc. I edited the comment in Julien's patch and checked it in:
Attachment #8426162 - Attachment is obsolete: true
Attachment #8426162 - Flags: review?(wtc)
Attachment #8427352 - Flags: review+
Attachment #8427352 - Flags: checked-in+

Either your option 2 patch or this patch will solve
the problem on the server side.

Your option 2 patch is useful to the client side in
one scenario:

Client sends a TLS 1.0 ClientHello, containing ECC
cipher suites, to a server that only supports SSL 3.0.

The server responds with a SSL 3.0 ServerHello, but
selects an ECC cipher suite. For example, a server
using older versions of NSS in "Extended ECC" mode
will do this.

Your option 2 patch will allow the client to abort
the handshake in this case.

So we definitely need your option 2 patch. But this
patch is optional. I think this patch is only worthwhile
if it improves performance. But I haven't studied the
code carefully to know if that's the case.
Attachment #8427356 - Flags: review?(julien.pierre)
Comment on attachment 8427356 [details] [diff] [review]
disable ECC cipher suites on server side if SSL 3.0 is negotiated

Review of attachment 8427356 [details] [diff] [review]:

Nit: I agree with the code change, but I think we also should improve the comments to something like :

/* we are unable to sending the elliptic_curves and ec_point_formats extensions with SSL 3.0 .
   Therefore, disable all ECC cipher suites */

I think it should also be done for the 2 corresponding sendclient hello code paths.
Attachment #8427356 - Flags: review?(julien.pierre) → review-
Attachment #8427352 - Flags: superreview+
Comment on attachment 8427356 [details] [diff] [review]
disable ECC cipher suites on server side if SSL 3.0 is negotiated

While this patch is corret, I don't think it is actually necessary. 

I tested this case yesterday with a 3.14 client configured with only SSL3 and one ECC cipher suite.
The server was using 3.16 with my "option 2" patch, and was configured with SSL3 + TLS 1.0 .

The connection failed with no cipher overlap.

Ie. my "option 2" patch was sufficiently effective at disabling ECC on the server side with SSL3 being negotiated. If we are only worried about performance, we may not need to check this patch in.

But it doesn't hurt, in any case.
You need to log in before you can comment on or make changes to this bug.