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

RESOLVED FIXED

Status

defect
RESOLVED FIXED
3 years ago
8 months ago

People

(Reporter: ekr, Unassigned)

Tracking

(Depends on 1 bug)

3.18

Firefox Tracking Flags

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

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

3 years ago
SSL_ExportKeyingMaterial() calls
ssl3_TLSPRFWithMasterSecret().
http://searchfox.org/nss/source/lib/ssl/ssl3con.c#11010

It calls it with a hash function argument derived from:
ssl3_GetTls12HashType()
http://searchfox.org/nss/source/lib/ssl/ssl3con.c#2110

However, the argument is ignored. Instead the hash function is set to:
CKM_NSS_TLS_PRF_GENERAL (for TLS < 1.2)
or
CKM_NSS_TLS_PRF_GENERAL_SHA256 (for TLS >= TLS 1.2)

    if (spec->master_secret) {
        SECItem param = { siBuffer, NULL, 0 };
        CK_MECHANISM_TYPE mech = CKM_TLS_PRF_GENERAL;
        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);
    }

http://searchfox.org/nss/source/lib/ssl/ssl3con.c#10960

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:

http://searchfox.org/nss/source/lib/ssl/ssl3con.c#11038
(Reporter)

Comment 2

3 years ago
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)
(Reporter)

Comment 6

3 years ago
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)
(Reporter)

Comment 10

3 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/60695923e60d

also open this bug per comment #14
Group: crypto-core-security
Status: NEW → RESOLVED
Last Resolved: 3 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.
Status: RESOLVED → REOPENED
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.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 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.