Closed Bug 341707 Opened 15 years ago Closed 15 years ago

curve-limited clients must not negotiate ECC ciphersuites unless they send the supported curve extension

Categories

(NSS :: Libraries, defect, P1)

3.11.1

Tracking

(Not tracked)

RESOLVED FIXED
3.11.3

People

(Reporter: nelson, Assigned: nelson)

References

Details

Attachments

(2 files)

When a TLS server receives a client hello that attempts to negotiate ECC curves
but does not include a "supported curve" hello extension, the server rightly
assumes that the client can support any of the 25 named curves defined in 
RFC 4492.  

So, a client that doesn't support all those 25 named curves MUST NOT list any
ECC cipher suites in its client hello UNLESS it also includes the "supported
curve" client hello extension.  Otherwise, the server is likely to negotiate 
a curve the client does not support.

Unfortunately, NSS's "Suite B Only" implementation doesn't implement that rule,
and it will negotiate ECC cipher suites even when it is sending an SSL2 style
client hello, or an SSL3 client hello (which doesn't use hello extensions).
This causes handshake failures when a "SUITE B ONLY" client attempts to 
handshake with a full-ECC server.  This is a shortcoming in the present
"SUITE B ONLY" implementation.
If the browser is going to use "suite B only" in 3.11.2, then IMO, this bug 
is P1 for 3.11.2.  But I leave that up to RedHat to decide.
I agree with nelson's assessment here.

bob
Priority: -- → P1
Target Milestone: 3.11.3 → 3.11.2
This bug is holding the release of 3.11.2, but isn't affecting our builds at Sun which have all 25  curves supported, so I'm assigning it to Bob .

If I understand correctly, the fix should be :

If a V2 client HELO is used, the ECC ciphers should all be disabled by the client, since it doesn't support all curves but can't convey that message.

However, I ran into the problem with a V3-style HELO message, with a version 3.0  - ie. I had TLS disabled and SSL3 enabled. Is it legal to have the curve extension in such a message ? If yes, we should add it for this case. 
If not, we should disable the ECC ciphers in this case just like we need to do for the V2 HELO case.
Assignee: nobody → rrelyea
(In reply to comment #3)
 
> However, I ran into the problem with a V3-style HELO message, with a version
> 3.0  - ie. I had TLS disabled and SSL3 enabled. Is it legal to have the curve
> extension in such a message ? If yes, we should add it for this case. 
> If not, we should disable the ECC ciphers in this case just like we need to do
> for the V2 HELO case.

  Technically, the ECC ciphers and hello extensions are only defined 
  for TLS (because SSL was never an IETF RFC) although the
  implementations in NSS and OpenSSL are fine negotiating ECC
  ciphers with SSL 3.0.

  However, from what I remember, Microsoft's implementation
  only handles ECC ciphers and extensions in the context of TLS.
  (Ari Medvinsky should be able to confirm this).
  Applying the guideline "be conservative in what you send, 
  generous in what you receive", I'd suggest not proposing
  ECC ciphers if TLS is disabled.

  vipul
We have deliberately chosen to NOT send any TLS hello extensions when TLS
is disabled (when we're only attempting to negotiate SSL 3.0 or below).
This is to ensure maximum interoperability if/when the client "falls back"
to SSL 3.0 after a TLS handshake attempt fails.
Any code changes for this bug need to be :

#ifndef NSS_ECC_MORE_THAN_SUITE_B

So that they won't affect Sun builds.

Wan-Teh just walked by and suggested that this isn't P1 for RH either in 3.11.2 because the browser is still not building with ECC turned on (ie. it has 0 curves). Both Bob and Nelson are at the CA meeting today, so we know this bug won't be fixed today.

We are looking really bad for not having 3.11.2 at Sun yet. So, I'm
moving the target for this to 3.11.3 .
Target Milestone: 3.11.2 → 3.11.3
So just to be clear it is a p1 for redhat. Since 3.12.2 has a Sun time constraint, it's fine to move it to 3.12.3 knowing that 3.12.3 will probably have to be released relatively soon. (The browser will need this patch when it releases).

bob
Blocks: 340724
Bob, I believe this patch will do the job, but it is untested.
Will you test it in a client with a MORE_THAN_SUITE_B server?
Attachment #226529 - Flags: review?(rrelyea)
Comment on attachment 226529 [details] [diff] [review]
patch v1 (checked in)

r+ relyea.
looks good.
Thanks for building the patch nelson!


bob
Attachment #226529 - Flags: review?(rrelyea) → review+
Taking, so this will appear on my radar.
Assignee: rrelyea → nelson
Curve-limited clients must not negotiate ECC ciphersuites unless they send the supported curve extension.  This means that when they are nogotiating SSL 3.0
and not TLS, they should not negotiate ECC ciphersuites at all.  
Bug 341707.  r=rrelyea.

On trunk:
Checking in ssl3con.c; new revision: 1.94; previous revision: 1.93
Checking in ssl3ecc.c; new revision: 1.15; previous revision: 1.14
Checking in sslcon.c;  new revision: 1.32; previous revision: 1.31
Checking in sslimpl.h; new revision: 1.53; previous revision: 1.52

On branch:
Checking in ssl3con.c; new revision: 1.76.2.15; previous revision: 1.76.2.14
Checking in ssl3ecc.c; new revision: 1.3.2.8;   previous revision: 1.3.2.7
Checking in sslcon.c;  new revision: 1.28.2.4;  previous revision: 1.28.2.3
Checking in sslimpl.h; new revision: 1.42.2.8;  previous revision: 1.42.2.7
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment on attachment 226529 [details] [diff] [review]
patch v1 (checked in)

ssl3ecc.c should have a comment for ssl3_DisableECCSuites
that says a NULL 'suite' argument means all ECC suites.
Patch v1 breaks the default non-ECC build because the
ssl3_DisableECCSuites function is only declared and
defined when NSS_ENABLE_ECC is defined.  A fix is
to change
    #ifndef NSS_ECC_MORE_THAN_SUITE_B
to
    #if defined(NSS_ENABLE_ECC) && !defined(NSS_ECC_MORE_THAN_SUITE_B)
in ssl3con.c and sslcon.c.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Wan-Teh and/or Bob, Please ensure that this patch satisfies your (RH)
requirements before giving it r+.  Thanks.
Attachment #229853 - Flags: review?(wtchang)
Attachment #226529 - Attachment description: patch v1 (untested) → patch v1 (checked in)
Comment on attachment 229853 [details] [diff] [review]
ifdef changes suggested by Wan-Teh

r=wtc. My build failure report didn't have any Red Hat
requirement behind it. Until we remove the make variable
NSS_ENABLE_ECC, NSS needs to build with and without setting
NSS_ENABLE_ECC=1.  Perhaps it is time to eliminate the
NSS_ENABLE_ECC make variable and macro from the NSS source
tree.
Attachment #229853 - Flags: review?(wtchang) → review+
Correct ifdefs so that non-ECC builds will continue to build correctly.
r=wtchang  bug 341707.

Checking in ssl3con.c; new revision: 1.76.2.16; previous revision: 1.76.2.15
Checking in sslcon.c;  new revision: 1.28.2.5; previous revision: 1.28.2.4

Checking in ssl3con.c; new revision: 1.95; previous revision: 1.94
Checking in sslcon.c;  new revision: 1.33; previous revision: 1.32
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.