With ECC enabled, servers negotiate _DHE_ cipher suites

RESOLVED FIXED in 3.11.1

Status

NSS
Libraries
P1
blocker
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: ECC)

Attachments

(3 attachments)

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

12 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

12 years ago
Created attachment 211822 [details] [diff] [review]
patch v1

Will ask for review once this is tested.

Comment 2

12 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

12 years ago
Created attachment 212057 [details]
ssltap output without patch applied

Ssltap output of connection between tstclnt and selfserv with client and server specifying the same set of cipher suites.

Updated

12 years ago
Attachment #212057 - Attachment description: ssltap output with patch applied → ssltap output without patch applied

Comment 4

12 years ago
Created attachment 212060 [details]
ssltap output with patch applied

The previous attachment was created BEFORE the patch was applied. This attachment is ssltap output after the patch.

Comment 5

12 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

12 years ago
*** Bug 325347 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 7

12 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

12 years ago
Attachment #211822 - Flags: review?(julien.pierre.bugs) → review+
(Assignee)

Comment 8

12 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

12 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

12 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

12 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

12 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

12 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

12 years ago
Whiteboard: ECC
(Assignee)

Comment 14

12 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

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.