Closed Bug 1269016 Opened 5 years ago Closed 4 years ago

Two DHE ciphers in cipher_suite_defs need to be surrounded by proper conditional

Categories

(NSS :: Libraries, defect)

defect
Not set
normal

Tracking

(firefox49 affected)

RESOLVED FIXED
Tracking Status
firefox49 --- affected

People

(Reporter: elio.maldonado.batiz, Assigned: elio.maldonado.batiz)

Details

Attachments

(1 file, 1 obsolete file)

In the ssl3CipherSuiteDef cipher_suite_defs[] array
{TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, ... kea_ecdhe_rsa}, and {TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, ... kea_ecdhe_ecdsa},
should be inside the #ifndef NSS_DISABLE_ECC ... #endif conditional
Attachment #8747313 - Flags: review?(martin.thomson)
Assignee: nobody → emaldona
Comment on attachment 8747313 [details] [diff] [review]
reclassify two EC ciphers as such

Review of attachment 8747313 [details] [diff] [review]:
-----------------------------------------------------------------

This seems harmless, but what is the consequence of this?

This has clearly been this way for ages.  So was it doing any harm to have them there?
Attachment #8747313 - Flags: review?(martin.thomson) → review+
No, it wasn't doing any harm and looking at places that refer to entries in the table I see
ssl_LookupCipherSuiteDef(ssl3CipherSuite suite) which care whether is ECC or not. The othe function is ssl3_ApplyNSSPolicy(void)
...
    /* disable every ciphersuite */
    for (i = 1; i < PR_ARRAY_SIZE(cipher_suite_defs); ++i) {
        const ssl3CipherSuiteDef *suite = &cipher_suite_defs[i];
that one examines the various fields to all ssl_CipherPrefSetDefault but I doubt is affected.

Blame it on me being a bit paranoid as I'm touching that table, adding a field, for other work I am doing.
This patch cannot be checked in as is since it caused many test to fail for me. Apparently additional changes would be required. Let me investigate what's needed. It's low priority at the moment.
Looks like we'd need to decrease |ssl_V3_SUITES_IMPLEMENTED| by two in case NSS_DISABLE_ECC=1. Maybe that's where your failures are coming from?
Thank you Tim, you a right, with this extra change all test are passing.
Attachment #8747313 - Attachment is obsolete: true
Attachment #8748717 - Flags: review?(ttaubert)
Attachment #8748717 - Flags: review?(ttaubert) → review+
Target Milestone: --- → 3.25
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.