Closed
Bug 1279479
Opened 8 years ago
Closed 7 years ago
Hide DHE cipher suites from the first handshake
Categories
(Core :: Security: PSM, defect, P3)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla50
People
(Reporter: emk, Assigned: emk)
References
Details
(Keywords: dev-doc-needed, site-compat, Whiteboard: [psm-assigned])
Attachments
(2 files)
Spun off from bug 1113974. I'll request review again because I had to add a support for creating RSA certs to LocalCertService.
Assignee: nobody → VYV03354
Priority: -- → P3
Whiteboard: [psm-assigned]
Assignee | ||
Updated•8 years ago
|
Attachment #8762004 -
Flags: review?(dkeeler)
Comment on attachment 8762004 [details] [diff] [review] patch Review of attachment 8762004 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, but I think we should constants defined in the idl rather than a boolean to indicate EC vs RSA. Also, could you use https://reviewboard.mozilla.org/ for this and future reviews? It does a much better job with whitespace-only changes and showing more context when desired, as well as inter-diffs. ::: security/manager/ssl/LocalCertService.cpp @@ +136,2 @@ > // Use the well-known NIST P-256 curve > SECOidData* curveOidData = SECOID_FindOIDByTag(SEC_OID_SECG_EC_SECP256R1); Can this be moved inside the conditional? ::: security/manager/ssl/nsILocalCertService.idl @@ +25,3 @@ > */ > + void getOrCreateCert(in ACString nickname, in nsILocalCertGetCallback cb, > + [optional] in boolean rsa); Instead of a boolean, let's use uint32_t constants like KEY_TYPE_EC = 0, KEY_TYPE_RSA = 1.
Attachment #8762004 -
Flags: review?(dkeeler)
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/60308/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60308/
Attachment #8764389 -
Flags: review?(dkeeler)
Comment on attachment 8764389 [details] Bug 1279479 - Remove DHE cipher suites from TLS 1.3 ClientHello. https://reviewboard.mozilla.org/r/60308/#review57124 This looks good. I'm a little concerned about the possibility of having to back out the RC4 patch, but that's probably unlikely and if we do, we can just back this out first. Let's be sure that Matt does a TLS canary run for this change. ::: security/manager/ssl/LocalCertService.cpp:437 (Diff revision 1) > return NS_ERROR_INVALID_ARG; > } > if (NS_WARN_IF(!aCallback)) { > return NS_ERROR_INVALID_POINTER; > } > + if (aKeyType != KEY_TYPE_EC && aKeyType != KEY_TYPE_RSA) { nit: I think it might be more clear to use the fully-qualified nsILocalCertService::KEY_TYPE_{EC,RSA} names here ::: security/manager/ssl/nsILocalCertService.idl:27 (Diff revision 1) > * and is valid for 1 year. If an expired or otherwise invalid cert is found > * with the nickname supplied here, it is removed and a new one is made. > * > * @param nickname Nickname that identifies the cert > * @param cb Callback to be notified with the result > + * @param keyType A KEY_TYPE_* constant that specified the key type. nit: s/specified/specifies/ ::: security/manager/ssl/tests/unit/test_fallback_cipher.js:31 (Diff revision 1) > .getService(Ci.nsIWeakCryptoOverride); > const socketTransportService = > Cc["@mozilla.org/network/socket-transport-service;1"] > .getService(Ci.nsISocketTransportService); > > -function getCert() { > +function getCert(keyType = certService.KEY_TYPE_EC) { nit: I find it more clear to use the full "nsILocalCertService.KEY_TYPE_EC" identifier ::: security/manager/ssl/tests/unit/test_fallback_cipher.js:34 (Diff revision 1) > .getService(Ci.nsISocketTransportService); > > -function getCert() { > +function getCert(keyType = certService.KEY_TYPE_EC) { > let deferred = Promise.defer(); > - certService.getOrCreateCert("tls-test", { > + let nickname = "tls-test"; > + if (keyType == certService.KEY_TYPE_RSA) { (same idea here)
Attachment #8764389 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 4•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=925361a6114f
Assignee | ||
Comment 5•8 years ago
|
||
Comment on attachment 8764389 [details] Bug 1279479 - Remove DHE cipher suites from TLS 1.3 ClientHello. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60308/diff/1-2/
Comment 7•8 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #6) > Matt, please do a TLS canary run. To be clear, this is against the build in comment 4, yes? And with no special prefs, just using defaults - correct?
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Matt Wobensmith [:mwobensmith][:matt:] from comment #7) > (In reply to Masatoshi Kimura [:emk] from comment #6) > > Matt, please do a TLS canary run. > > To be clear, this is against the build in comment 4, yes? And with no > special prefs, just using defaults - correct? Yes.
Comment 9•8 years ago
|
||
I ran the canary against your try build, and I found no new errors. It produced the exact same results as the Nightly run I did a few days ago. Please let me know if you'd like anything else.
Flags: needinfo?(mwobensmith)
Comment 10•8 years ago
|
||
Pushed by VYV03354@nifty.ne.jp: https://hg.mozilla.org/integration/autoland/rev/1a1d7ef3cb0e Hide DHE cipher suites from the first handshake. r=keeler
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1a1d7ef3cb0e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 12•8 years ago
|
||
This appears to have cause DTLS setup failures for Cisco Spark (perhaps they use DHE?) See bug 1288246
Comment 14•8 years ago
|
||
I think we are using NSS directly: https://dxr.mozilla.org/mozilla-central/source/media/mtransport/transportlayerdtls.cpp#459 But from bisecting the Nightly builds, checking with the DHE disable add-on and looking at the ciphers list offered in FF DTLS client hello it is pretty clear that this change here has taken DHE ciphers out of WebRTC's cipher list. I suggest we will continue the discussion over in bug 1288246 how to solve this.
Assignee | ||
Comment 15•8 years ago
|
||
Permalink for future reference: https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/media/mtransport/transportlayerdtls.cpp#459
Comment 16•8 years ago
|
||
Uh, it was premature to make this change without consulting all the stakeholders. EMK, please prepare a backout of this patch while we determine how to resolve.
Assignee | ||
Comment 17•8 years ago
|
||
I prepared a bustage fix instead. Being disabled DHE in WebRTC was an unexpected side effect.
Comment 18•8 years ago
|
||
A bustage fix is not what is needed. Please back out the entire patch.
Comment 19•8 years ago
|
||
DH usage for HTTPS is still at over 2%. https://mzl.la/2a5x1sb The breakage will not be limited to WebRTC. I agree with EKR. This needs to be backed out ASAP.
Comment 20•8 years ago
|
||
Following up: I know that there's a desire to deprecate these cipher suites entirely both here and at other browsers (especially Chrome). I think that's a reasonable goal, but we actually need to study the impact of the change on our specific cipher suite profile and our timing with respect to other browsers
Comment 21•8 years ago
|
||
Backout by dkeeler@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d09b4de97527 backout changeset 1a1d7ef3cb0e for causing WebRTC compatibility issues
https://hg.mozilla.org/mozilla-central/rev/d09b4de97527
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•8 years ago
|
Flags: needinfo?(rlb)
Updated•8 years ago
|
Keywords: dev-doc-needed,
site-compat
Comment hidden (mozreview-request) |
Assignee | ||
Comment 24•7 years ago
|
||
Comment on attachment 8764389 [details] Bug 1279479 - Remove DHE cipher suites from TLS 1.3 ClientHello. Bug 1195661 sucks.
Attachment #8764389 -
Flags: review+ → review?(dkeeler)
Comment 25•7 years ago
|
||
mozreview-review |
Comment on attachment 8764389 [details] Bug 1279479 - Remove DHE cipher suites from TLS 1.3 ClientHello. https://reviewboard.mozilla.org/r/60308/#review117980 ::: security/manager/ssl/nsNSSIOLayer.cpp:2460 (Diff revision 3) > SSL_CipherPrefSet(fd, TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, false); > SSL_CipherPrefSet(fd, TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, false); I didn't realize that we were disabling CBC suites on first connect. I guess that we have GCM, so it's working out fine.
Attachment #8764389 -
Flags: review?(martin.thomson) → review+
Comment on attachment 8764389 [details] Bug 1279479 - Remove DHE cipher suites from TLS 1.3 ClientHello. Ok - let's see how this goes.
Attachment #8764389 -
Flags: review?(dkeeler) → review+
Comment 27•7 years ago
|
||
Pushed by VYV03354@nifty.ne.jp: https://hg.mozilla.org/integration/autoland/rev/63b5159df4e5 Remove DHE cipher suites from TLS 1.3 ClientHello. r=keeler,mt
Comment 28•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/63b5159df4e5
Status: REOPENED → RESOLVED
Closed: 8 years ago → 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Nils, would you be able to check that Spark still works as expected with this change? Thanks!
Flags: needinfo?(drno)
Comment 30•7 years ago
|
||
I verified with a local build of eb2364853477 from mozilla-central that Fx can still establish a connection with Cisco Spark (Fx still offer DHE as the DTLS client and Spark still selects it as the chosen cipher).
Flags: needinfo?(drno)
Thanks!
Comment 32•7 years ago
|
||
Can someone provide a little more detail as to what this patch does, in particular as it relates to bug 1180526 and the ability to use devices that only support weak diffie-hellman keys?
Comment 33•7 years ago
|
||
Mike, weak DH keys (by which I presume you mean those below 2048 bits in size) remain unacceptable. All this does is add a few extra round trips. If a server won't allow a connection because it only supports DHE, then we will reconnect.
Blocks: 1487517
You need to log in
before you can comment on or make changes to this bug.
Description
•