Disable TLS 1.3 for FF52 Release

VERIFIED FIXED

Status

()

defect
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: ekr, Assigned: ekr)

Tracking

({dev-doc-complete})

38 Branch
Points:
---

Firefox Tracking Flags

(firefox52 verified, firefox-esr52 fixed, firefox53 wontfix, firefox54 wontfix)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

2 years ago
Posted patch 13.patch (obsolete) — Splinter Review
Per discussion on e-mail, I want to disable TLS 1.3 for FF 52 Release. This patch does that.
Attachment #8840459 - Flags: review?(dkeeler)
Comment on attachment 8840459 [details] [diff] [review]
13.patch

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

Looks like I wasn't in that discussion, but I trust your judgement.

::: netwerk/base/security-prefs.js
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  pref("security.tls.version.min", 1);
> +pref("security.tls.version.max", 3);

This should be sufficient, but I think it would be safest to set PSM_DEFAULT_MAX_TLS_VERSION to 3 at https://dxr.mozilla.org/mozilla-central/rev/32dcdde1fc64fc39a9065dc4218265dbc727673f/security/manager/ssl/nsNSSComponent.cpp#1627 as well.
Attachment #8840459 - Flags: review?(dkeeler) → review+
Assignee

Comment 3

2 years ago
I didn't know about that setting. Keeler, I am about to get on a plane and I know relman would like to get this done today. Do you think you could make that other change and get this landed? Sorry about the rush?
Flags: needinfo?(dkeeler)
Sure thing - I'll get this landed.
Flags: needinfo?(dkeeler)
Assignee

Comment 6

2 years ago
Thanks, Keeler. I will be in PDX this afternoon and I owe you a drink
Assignee

Comment 7

2 years ago
Relman people, here is the disabling patch
Flags: needinfo?(lhenry)
Flags: needinfo?(jcristau)
Keeler do you mind requesting uplift so we keep all our flags in a row?
Flags: needinfo?(lhenry) → needinfo?(dkeeler)
Comment on attachment 8840538 [details] [diff] [review]
1342082-disable-tls-1.3.diff

(In reply to Eric Rescorla (:ekr) from comment #6)
> Thanks, Keeler. I will be in PDX this afternoon and I owe you a drink

Just my luck this is the one day I'm not in the office :(

In any case, the try run isn't quite done, but it's looking green, so:

Approval Request Comment
[Feature/Bug causing the regression]: bug 1310516 enabled TLS 1.3 by default
[User impact if declined]: interoperability concerns, iiuc
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no - this is for 52 only
[Needs manual test from QE? If yes, steps to reproduce]: no, but you could test by going to https://www.cloudflare.com/, opening the page info dialog, going to the security tab, and checking that under "Technical Details" it says TLS 1.2 and not TLS 1.3
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it just changes the default to a previously-known good value
[String changes made/needed]: none
Attachment #8840538 - Flags: approval-mozilla-beta?
Assignee

Comment 10

2 years ago
Also in office tomorrow!
Comment on attachment 8840538 [details] [diff] [review]
1342082-disable-tls-1.3.diff

As discussed in email let's uplift this to beta, hoping to get it in today's build. If it doesn't make it in, then we will uplift to m-r next week for the RC build.
Attachment #8840538 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee: nobody → ekr
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(jcristau)
Resolution: --- → FIXED
Reproduced the issue with an affected build (52.0b8-build1, 20170220070057) using Ubuntu 16.04 x64 with the instructions from Comment 9.

This is verified fixed on 52.0b9-build2 (20170223185858) using:

    - Ubuntu 16.04 x64,
    - Windows 10 x64,
    - macOS 10.12.3

cloudflare.com now shows TLS 1.2 in Page Info :: Security :: Technical Details.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.