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

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Comment 1

[Eric Rescorla (:ekr)]

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

Comment 2

[Eric Rescorla (:ekr)]

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.

Comment 3

[Nils Ohlmeier [:drno]]

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.

Comment 4

[Nils Ohlmeier [:drno]]

Attachment: Disable SHA384 ciphers for WebRTC

Comment 5

[Nils Ohlmeier [:drno]]

Try run for attachment 8801310 [details] [diff] [review]
https://treeherder.mozilla.org/#/jobs?repo=try&revision=efd6d578c8c49c5b786b0f87cfbb0f7ebe5e72c7

Comment 6

[Eric Rescorla (: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?

Comment 7

[Nils Ohlmeier [:drno]]

Attachment: Disable SHA384 ciphers for WebRTC

Patch from hg export instead of just diff.

Comment 8

[Martin Thomson [:mt:]]

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

Comment 9

[Martin Thomson [:mt:]]

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

Comment 10

[Eric Rescorla (:ekr)]

I think we have one in transporlayerdtls.cpp

Comment 11

[Nils Ohlmeier [:drno]]

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

Comment 12

[Nils Ohlmeier [:drno]]

Attachment: Disable SHA384 ciphers for WebRTC

Added the requested comment.

Comment 13

[Nils Ohlmeier [:drno]]

Do I need sec-approval to land attachment 8801444 [details] [diff] [review] (because of lack of sec-severity rating)?

Comment 14

[Martin Thomson [:mt:]]

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.

Comment 15

[Nils Ohlmeier [:drno]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60695923e60db47f2abc48eaf1c31c0c666c5cf9
Bug 1310061: avoid interop issues with SHA384. r=mt

Comment 16

[Nils Ohlmeier [:drno]]

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

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/60695923e60d

also open this bug per comment #14

Comment 18

[Nils Ohlmeier [:drno]]

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.

Comment 19

[Nils Ohlmeier [:drno]]

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

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 21

[Carsten Book [:Tomcat]]

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=3910208&repo=mozilla-aurora

Comment 22

[Martin Thomson [:mt:]]

Nils, that is because we don't have the latest NSS on aurora.  You can drop the offending line and it should be OK.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/47b221aee73a

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d69e6eb5d19f

Comment 25

[Nils Ohlmeier [:drno]]

Attachment: Disable SHA384 ciphers for WebRTC on Aurora

The same patch as before, minus the TLS 1.3 cipher suite which doesn't exist on aurora.

Carrying forward r=mt.

Comment 26

[Nils Ohlmeier [:drno]]

Attachment 8803216 [details] [diff] needs to be landed on aurora only.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

This was already uplifted to Aurora in comment 23?

Comment 28

[Nils Ohlmeier [:drno]]

Thanks Ryan, I got confused by comment #21.

For future reference the temporary patch appears to be present in Nightly, Aurora and Beta:
https://dxr.mozilla.org/mozilla-central/rev/b1b18f25c0ea69d9ee57c4198d577dfcd0129ce1/media/mtransport/transportlayerdtls.cpp#692
https://dxr.mozilla.org/mozilla-aurora/rev/b82a286b69e040dc2e8f180e33cffd25f7a09ff4/media/mtransport/transportlayerdtls.cpp#675
https://dxr.mozilla.org/mozilla-beta/rev/c45f9369c95cb492a2dee21e9c5cefde192f573c/media/mtransport/transportlayerdtls.cpp#676

Comment 29

[Nils Ohlmeier [: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?

Comment 30

[Nils Ohlmeier [:drno]]

Looks like remaining work will be done in bug 1312976.