1321783 - Make updater use conservative TLS settings

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Eric Rescorla (:ekr)]

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.

Comment 1

[Joe Hildebrand [:jhildebrand] (UTC-6)]

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.

Comment 2

[Eric Rescorla (:ekr)]

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

Comment 3

[Benjamin Smedberg]

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

Comment 4

[Joe Hildebrand [:jhildebrand] (UTC-6)]

My guess is we need a new nsIRequest.loadFlags, like "INHIBIT_FANCY_PROTOCOLS", then support in necko for same?

Comment 5

[Eric Rescorla (:ekr)]

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.

Comment 6

[Patrick McManus [:mcmanus]]

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?

Comment 7

[Eric Rescorla (:ekr)]

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

Comment 8

[Patrick McManus [:mcmanus]]

is the updater service js running in main or child process?

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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

Comment 10

[Robert Strong (they/them - no direct email)]

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

Comment 11

[Robert Strong (they/them - no direct email)]

Will the changes outside of app update that allow the one line change in nsUpdateService.js be uplifted to 52?

Comment 12

[Eric Rescorla (:ekr)]

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

Comment 13

[Robert Strong (they/them - no direct email)]

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.

Comment 14

[Eric Rescorla (:ekr)]

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

Comment 15

[Robert Strong (they/them - no direct email)]

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?

Comment 16

[Eric Rescorla (:ekr)]

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.

Comment 17

[Patrick McManus [:mcmanus]]

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.

Comment 18

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b082aa43086a

Comment 19

[Patrick McManus [:mcmanus]]

Attachment: Make updater be networking conservative

Comment 20

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbc6a03b9c44

Comment 21

[Patrick McManus [:mcmanus]]

Attachment: Make updater be networking conservative

Comment 22

[Robert Strong (they/them - no direct email)]

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

r=me for the change in nsUpdateService.js

Comment 23

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 24

[Patrick McManus [:mcmanus]]

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

Comment 25

[Eric Rescorla (:ekr)]

This is great. Now that we know how, can we wire up to Shield (?) to allow us to frob the pref...

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d13d437a39a1

Comment 27

[Patrick McManus [:mcmanus]]

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]:

Comment 28

[Julien Cristau [:jcristau]]

tracking this for 52 since that's where we're planning on shipping tls1.3

Comment 29

[Julien Cristau [:jcristau]]

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

disable tls1.3 in the updater, for aurora52

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d20a6ec3db1e

Comment 31

[Robert Strong (they/them - no direct email)]

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;

Comment 32

[Robert Helmer [: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.

Comment 33

[Dana Keeler (she/her) (use needinfo) [:keeler]]

I finally figured out why my test was not working correctly. It's up for review now in bug 1323843.

Comment 34

[Patrick McManus [:mcmanus]]

Attachment: MP

patch applies cleanly to 51

Comment 35

[Patrick McManus [:mcmanus]]

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

Comment 36

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8824090 [details] [diff] [review]
MP

Needed on beta for TLS/NSS update. This should go in today for beta 12

Comment 37

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/46f4f8ec3cb4