Make updater use conservative TLS settings

RESOLVED FIXED in Firefox 51

Status

()

Toolkit
Application Update
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ekr, Assigned: mcmanus, NeedInfo)

Tracking

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox51 fixed, firefox52+ fixed, firefox53+ fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
We want to make sure that the updater is very conservative so we don't get bustage. This means turning off TLS 1.3, etc.
Priority for this is to ensure that we're not stranding users who have gotten the TLS 1.3 (or other newfangled networking stuff like QUIC) on versions that don't deal with "interesting" network configurations, such as MITM boxes that subtly mangle protocol.  One day, we'd like to be able to detect these issues, and enlist the users' aid to encourage their network admin to upgrade or reconfigure.
(Reporter)

Comment 2

2 years ago
Correct. And this has a fairly high level of urgency because TLS 1.3 is already in Aurora.

Comment 3

2 years ago
ekr what exact changes should we be making to the HTTP connections involved? The main update check is performed using an XHR from here: http://searchfox.org/mozilla-central/rev/0f92c398ce929a3f3d9dad30132c218def154e39/toolkit/mozapps/update/nsUpdateService.js#3256
Component: Untriaged → Application Update
Flags: needinfo?(ekr)
Product: Firefox → Toolkit

Updated

2 years ago
status-firefox51: --- → unaffected
status-firefox52: --- → affected
status-firefox53: --- → affected
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
My guess is we need a new nsIRequest.loadFlags, like "INHIBIT_FANCY_PROTOCOLS", then support in necko for same?
(Reporter)

Comment 5

2 years ago
Yeah, we're going to need to plumb this through Necko and PSM. At the end of the day we need
SSL_VersionSetRange(fd, ...) with version max = TLS 1.2. At the end of the day it may just be easiest to mark the Mozilla servers as having fallen back to TLS 1.2.

Needinfoing McManus and keeler.
Flags: needinfo?(mcmanus)
Flags: needinfo?(ekr)
Flags: needinfo?(dkeeler)
(Assignee)

Comment 6

2 years ago
I can plumb BE_CONSERVATIVE (defined to be a moving target) through necko easily enough if I knew of a PSM interface.. from what I recall PSM just interacts with NSS versioning based on the prefs and indeed might not even do it on a per socket basis, not based on a xpcom interface. there are a couple readonly attributes for reading it out after the fact. maybe I've forgotten it? keeler?

fwiw I don't think we have reason to believe our hello is going to cause massive incompatibility and we need to have some fallbacks anyhow. but actual broken 1.3 selection (as seems to be the case with irccloud from one vantage point) would seem to be a very slow ramp (as sites have to adopt 1.3) that we can protect against by just not updating critical servers or reverting them if problems come to light. 1.2 and spdy and h2 all managed more or less ok with that approach even though each had some interop problems along the way.

can the irccloud fail mode be added to the fallback path?
Flags: needinfo?(mcmanus)
(Reporter)

Comment 7

2 years ago
Yeah, so PSM sets the basic behavior on a global basis but then has per-socket ways to downgrade the version to handle fallback. My sense is that the only server we absolutely must protect is upgrade because otherwise if anyone is behind a sufficiently terrible middlebox (like supported_versions intolerant) they will get stuck. This shouldn't happen, but...
(Assignee)

Comment 8

2 years ago
is the updater service js running in main or child process?
Flags: needinfo?(robert.strong.bugs)
We should be able to set the max version in nsSSLIOLayerSetOptions[0] based on the nsISocketProvider flags.

[0] https://dxr.mozilla.org/mozilla-central/rev/f65ad27efe839ce9df0283840a1a40b4bbc9ead0/security/manager/ssl/nsNSSIOLayer.cpp#2465
Flags: needinfo?(dkeeler)
(In reply to Patrick McManus [:mcmanus] from comment #8)
> is the updater service js running in main or child process?
Should only run in the main process.
Flags: needinfo?(robert.strong.bugs)
Will the changes outside of app update that allow the one line change in nsUpdateService.js be uplifted to 52?
Flags: needinfo?(ekr)
(Reporter)

Comment 12

2 years ago
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #11)
> Will the changes outside of app update that allow the one line change in
> nsUpdateService.js be uplifted to 52?

What changes are you referring to? The code Keeler is pointing to looks pretty old.
Flags: needinfo?(ekr) → needinfo?(dkeeler)
From that I will assume that client changes outside of app update that will allow it to use TLS 1.2 are not known yet.

What I'm concerned with is that with a server side fix there will still be clients that won't fallback when the server side change is reverted. This is why I'd like a client side fix uplifted to aurora.
(Reporter)

Comment 14

2 years ago
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #13)
> From that I will assume that client changes outside of app update that will
> allow it to use TLS 1.2 are not known yet.
> 
> What I'm concerned with is that with a server side fix there will still be
> clients that won't fallback when the server side change is reverted. This is
> why I'd like a client side fix uplifted to aurora.

I'm not following.

- Our update servers should use TLS 1.2.
- As of 52, Firefox will offer TLS 1.3, but will be willing to negotiate any version down to TLS 1.0, with the impact being that with most sites we will get TLS 1.2 (there are a small number of 1.3 servers)
- The scenario of concern is middleboxes which for some reason choke on a TLS 1.3 ClientHello even if the server is TLS 1.2. In order to address that we want to make sure that update starts by offering 1.2. Yes, this will need to be a client-side change, but it seems like it needs to be done in Necko and the updater code above, as NSS (and PSM apparently) have settings to do 1.2.
- Yes, the above client change to the updater (and if it's easy, Telemetry, and other phone-home functions)  will need to be uplifted to Aurora.

Are you talking about something else beyond this?
Flags: needinfo?(robert.strong.bugs)
Thanks... I mistakenly thought the aus server would also need to be changed to not offer TLS 1.3.

From what I've read it seems that vast majority of the client-side change will need to be done in necko with the updater change being along the lines of changing the loadFlags... is that right?
Flags: needinfo?(robert.strong.bugs)
(Reporter)

Comment 16

2 years ago
Not sure this is a mcmanus question. I would suggest we all get in a room in Hawaii and see if we can work out all the pieces.
Flags: needinfo?(mcmanus)
(Assignee)

Comment 17

2 years ago
I have a breezy patch that seems to work (next comment). hand tested by looking for 7f12 in the client hello to aus5.

dkeeler - maybe you could write a testcase that's a bit more meaningful on the server side for confirmation?

I've defined the new flag(s) e2e as be_conservative rather than any api to set version levels outside of psm. That lets us tag code that should be_conservative and shift the definition of what that means over time throughout all of networking.
Flags: needinfo?(mcmanus)
(Assignee)

Comment 19

2 years ago
Created attachment 8816706 [details] [diff] [review]
Make updater be networking conservative
Attachment #8816706 - Flags: review?(dkeeler)
(Assignee)

Updated

2 years ago
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Attachment #8816706 - Flags: review?(robert.strong.bugs)
Attachment #8816706 - Flags: review?(dd.mozilla)
(Assignee)

Comment 21

2 years ago
Created attachment 8816711 [details] [diff] [review]
Make updater be networking conservative
Attachment #8816711 - Flags: review?(dkeeler)
(Assignee)

Updated

2 years ago
Attachment #8816706 - Attachment is obsolete: true
Attachment #8816706 - Flags: review?(robert.strong.bugs)
Attachment #8816706 - Flags: review?(dkeeler)
Attachment #8816706 - Flags: review?(dd.mozilla)
(Assignee)

Updated

2 years ago
Attachment #8816711 - Flags: review?(robert.strong.bugs)
Attachment #8816711 - Flags: review?(dd.mozilla)
Comment on attachment 8816711 [details] [diff] [review]
Make updater be networking conservative

r=me for the change in nsUpdateService.js
Attachment #8816711 - Flags: review?(robert.strong.bugs) → review+
Attachment #8816711 - Flags: review?(dd.mozilla) → review+
Comment on attachment 8816711 [details] [diff] [review]
Make updater be networking conservative

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

The PSM changes look good to me. I need a little more time to come up with a test.
Attachment #8816711 - Flags: review?(dkeeler) → review+
(Assignee)

Comment 24

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d13d437a39a14fa81221dfac4556c9ef347f5fa3
Bug 1321783 - Make updater be networking conservative r=dkeeler r=rstrong r=dragana
(Reporter)

Comment 25

2 years ago
This is great. Now that we know how, can we wire up to Shield (?) to allow us to frob the pref...
Flags: needinfo?(laura)

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d13d437a39a1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
tracking-firefox53: ? → +
(Assignee)

Comment 27

2 years ago
Comment on attachment 8816711 [details] [diff] [review]
Make updater be networking conservative

Approval Request Comment
[Feature/Bug causing the regression]: tls.13 in gecko 52 creates the need for this patch, which protects our updater in cases of network mitm that fails in the presence of the new tls handshake
[User impact if declined]: conceivably a user within such a network would have irreparably damaged firefox
[Is this code covered by automated tests?]: the updater is well tested for regression - the new functionality is pending a CI test from dkeeler
[Has the fix been verified in Nightly?]: y
[Needs manual test from QE? If yes, steps to reproduce]: n
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:
Attachment #8816711 - Flags: approval-mozilla-aurora?
tracking this for 52 since that's where we're planning on shipping tls1.3
tracking-firefox52: ? → +
Comment on attachment 8816711 [details] [diff] [review]
Make updater be networking conservative

disable tls1.3 in the updater, for aurora52
Attachment #8816711 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 30

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/d20a6ec3db1e
status-firefox52: affected → fixed
rhelmer, I think you will want to do the same for add-ons and uplift to aurora.

// Disable cutting edge features, like TLS 1.3, where middleboxes might brick us
this._request.channel.QueryInterface(Ci.nsIHttpChannelInternal).beConservative = true;
Flags: needinfo?(rhelmer)
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #31)
> rhelmer, I think you will want to do the same for add-ons and uplift to
> aurora.
> 
> // Disable cutting edge features, like TLS 1.3, where middleboxes might
> brick us
> this._request.channel.QueryInterface(Ci.nsIHttpChannelInternal).
> beConservative = true;

Thanks, filed bug 1323538.

We have quite a few different update mechanisms that might be impacted by this, I'll go over the list and file bugs as necessary.
Flags: needinfo?(rhelmer)
I finally figured out why my test was not working correctly. It's up for review now in bug 1323843.
Flags: needinfo?(dkeeler)
(Assignee)

Comment 34

2 years ago
Created attachment 8824090 [details] [diff] [review]
MP

patch applies cleanly to 51
Attachment #8824090 - Flags: review+
(Assignee)

Comment 35

2 years ago
Comment on attachment 8824090 [details] [diff] [review]
MP

Approval Request Comment
[Feature/Bug causing the regression]: tls 1.3 has been uplifted to beta
[User impact if declined]: see comment 27 
[Is this code covered by automated tests:see comment 27
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: airtime on 52/53 - fairly simple - verified by a number of parties
[String changes made/needed]: none
Attachment #8824090 - Flags: approval-mozilla-beta?
Blocks: 1325501
status-firefox51: unaffected → affected
Comment on attachment 8824090 [details] [diff] [review]
MP

Needed on beta for TLS/NSS update. This should go in today for beta 12
Attachment #8824090 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.