327105 - With ECC enabled, servers negotiate _DHE_ cipher suites

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

Will ask for review once this is tested.

Comment 2

[Julien Pierre]

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

[Neil Williams]

Attachment: ssltap output without patch applied

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

Comment 4

[Neil Williams]

Attachment: 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

[Neil Williams]

(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."

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

*** Bug 325347 has been marked as a duplicate of this bug. ***

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 211822 [details] [diff] [review]
patch v1

Now that this patch has been tested, I'm asking for reviews.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 211822 [details] [diff] [review]
patch v1

Vipul, please review this tiny patch to your EC code.
Thanks.

Comment 9

[Vipul Gupta]

(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

[Wan-Teh Chang]

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?

Comment 11

[Wan-Teh Chang]

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

[Vipul Gupta]

(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

[Vipul Gupta]

Comment on attachment 211822 [details] [diff] [review]
patch v1

Please see comment #9 and
comment #10 and decide what
you think is more appropriate.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

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