Closed Bug 1310061 Opened 7 years ago Closed 7 years ago

TLS Exporters Always use SHA-256 in TLS 1.2 even w/ SHA-384 algorithms


(NSS :: Libraries, defect)

Not set


(firefox49 wontfix, firefox-esr45 unaffected, firefox50 fixed, firefox51 fixed, firefox52 fixed)

Tracking Status
firefox49 --- wontfix
firefox-esr45 --- unaffected
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- fixed


(Reporter: ekr, Unassigned)


(Depends on 1 open bug)



(1 file, 3 obsolete files)

      No description provided.
SSL_ExportKeyingMaterial() calls

It calls it with a hash function argument derived from:

However, the argument is ignored. Instead the hash function is set to:

    if (spec->master_secret) {
        SECItem param = { siBuffer, NULL, 0 };
        PK11Context *prf_context;
        unsigned int retLen;

        if (spec->version >= SSL_LIBRARY_VERSION_TLS_1_2) {
            mech = CKM_NSS_TLS_PRF_GENERAL_SHA256;

The impact is that the exporter only uses SHA-256 for TLS 1.2 and (it appears) the MD5/SHA1 hash for < 1.2, though I haven't completely worked through that.

The question arises: why is Finished computed correctly?
The answer is that ssl3_ComputeTLSFinished() only calls ssl3_TLSPRFWithMasterSecret() if spec->master_secret is NULL:

    if (!spec->master_secret) {
        const char *label = isServer ? "server finished" : "client finished";
        unsigned int len = 15;
        HASH_HashType hashType = ssl3_GetTls12HashType(ss);
        return ssl3_TLSPRFWithMasterSecret(spec, label, len, hashes->u.raw,
                                           hashes->len, tlsFinished->verify_data,
                                           sizeof tlsFinished->verify_data, hashType);

Otherwise it calls PKCS#11 directly and uses hash function derived from ssl3_GetPrfHashMechanism(ss); which has the correct value.

Note: this call to ssl3_TLSPRFWithMasterSecret() in ssl3_ComputeTLSFinished() is entirely bogus because it is only called if !spec->master_secret but ssl3_TLSPRFWithMasterSecret() asserts that spec->master_secret is non-zero:
Martin, can you verify this analysis? If I am right, we have an interop problem and maybe some kind of other problem because of the spurious branch.
Flags: needinfo?(martin.thomson)
Looks like bug 923089 introduce this problem.

Just manually verified that 48 is unaffected and 49 is the first release which offers SHA384 ciphers as WebRTC DTLS client.
Attachment #8801310 - Flags: review?(ekr)
Comment on attachment 8801310 [details] [diff] [review]
Disable SHA384 ciphers for WebRTC

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

Can you bounce this to MT?
Attachment #8801310 - Flags: review?(ekr) → review?(martin.thomson)
Patch from hg export instead of just diff.
Attachment #8801310 - Attachment is obsolete: true
Attachment #8801310 - Flags: review?(martin.thomson)
Attachment #8801330 - Flags: review?(martin.thomson)
Comment on attachment 8801330 [details] [diff] [review]
Disable SHA384 ciphers for WebRTC

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

::: media/mtransport/transportlayerdtls.cpp
@@ +687,5 @@
>  // Disable all NSS suites modes without PFS or with old and rusty ciphersuites.
>  // Anything outside this list is governed by the usual combination of policy
>  // and user preferences.
>  static const uint32_t DisabledCiphers[] = {
> +  TLS_AES_256_GCM_SHA384,

please add a comment with a bug number on it
Attachment #8801330 - Flags: review?(martin.thomson) → review+
I can confirm that ekr's analysis is correct (from code inspection, anyway).  We should:
1) remove the spurious branch (!spec->master_secret)
2) use the same direct calls to PKCS#11 for exporters
3) write a bunch of tests for exporters, I think that we have approximately zero
Flags: needinfo?(martin.thomson)
I think we have one in transporlayerdtls.cpp
(In reply to Martin Thomson [:mt:] from comment #8)
> please add a comment with a bug number on it

Given this is a sec bug I thought I better not draw attention to the bug. But I'm happy to add it if it's deemed not that critical.
Added the requested comment.
Attachment #8801330 - Attachment is obsolete: true
Attachment #8801444 - Flags: review+
Do I need sec-approval to land attachment 8801444 [details] [diff] [review] (because of lack of sec-severity rating)?
This is not a security issue, so I would prefer that we include the bug number, land the patch, and open this. In no particularly order.
Flags: needinfo?(mwobensmith)
(In reply to Martin Thomson [:mt:] from comment #14)
> This is not a security issue, so I would prefer that we include the bug
> number, land the patch, and open this. In no particularly order.

First two are done. But I'm lacking the permission to open this bug report.

also open this bug per comment #14
Group: crypto-core-security
Closed: 7 years ago
Resolution: --- → FIXED
Flags: needinfo?(mwobensmith)
Sorry forgot to put the leave-open keyword, as attachment 8801444 [details] [diff] [review] is only a temporary fix and the real fix still pending.
Resolution: FIXED → ---
Comment on attachment 8801444 [details] [diff] [review]
Disable SHA384 ciphers for WebRTC

Approval Request Comment
[Feature/regressing bug #]: Bug 923089 added support for SHA384 to NSS. But in case of WebRTC that SHA384 support is broken.

[User impact if declined]: Anytime another WebRTC implementation negotiates a DTLS cipher suite which contains SHA384 the connection will get established, but data can not be received nor can the remote side decipher the data send by Firefox. This causes a known interoperability problem with MS Edge on Win 10. Luckily Google Chrome does not appear to support SHA384 yet.

[Describe test coverage new/current, TreeHerder]: I manually verified that with this patch applied WebRTC calls with MS Edge work as expected and up to version 48.

[Risks and why]: The risk is pretty low as it only removes all ciphers which contain SHA384 from the WebRTC DTLS handshake. No other crypto parts of Firefox are affected by this patch.

[String/UUID change made/needed]: N/A
Attachment #8801444 - Flags: approval-mozilla-beta?
Attachment #8801444 - Flags: approval-mozilla-aurora?
Comment on attachment 8801444 [details] [diff] [review]
Disable SHA384 ciphers for WebRTC

This seems like something we need to uplift before we ship, Aurora51+, Beta50+
Attachment #8801444 - Flags: approval-mozilla-beta?
Attachment #8801444 - Flags: approval-mozilla-beta+
Attachment #8801444 - Flags: approval-mozilla-aurora?
Attachment #8801444 - Flags: approval-mozilla-aurora+
Nils, that is because we don't have the latest NSS on aurora.  You can drop the offending line and it should be OK.
Assignee: nobody → drno
Flags: needinfo?(drno)
The same patch as before, minus the TLS 1.3 cipher suite which doesn't exist on aurora.

Carrying forward r=mt.
Attachment #8803216 - Flags: review+
Attachment 8803216 [details] [diff] needs to be landed on aurora only.
Keywords: checkin-needed
This was already uplifted to Aurora in comment 23?
Flags: needinfo?(drno)
Now the other question is: should this bug be used to work on the actual fix in NSS (or should we create a new bug for that) and who can do that work?
Assignee: drno → nobody
Flags: needinfo?(ekr)
Keywords: checkin-needed
Depends on: 1312976
Looks like remaining work will be done in bug 1312976.
Closed: 7 years ago7 years ago
Flags: needinfo?(ekr)
Resolution: --- → FIXED
Attachment #8803216 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.