1323538 - use conservative TLS settings for XHR in chrome code

Status: RESOLVED INCOMPLETE

(Core :: DOM: Core & HTML, defect)

Description

[Robert Helmer [:rhelmer]]

TLS 1.3 has landed on Aurora, and there's evidence that it's breaking updates when going through certain middleboxes (see bug 1321783)

Per that bug, app update has disabled TLS 1.3 and we should for AddonManager updates as well, which includes:

* System Add-ons (including GMP) from AUS
* add-on and compat updates from AMO

Comment 1

[Eric Rescorla (:ekr)]

Rob: any chance you could produce a patch here quickly (today would be ideal) for Telemetry and Add-ons. See bug 1305970 for context.

Comment 2

[Eric Rescorla (:ekr)]

Attachment: Be conservative for add-on updates and telemetry

Comment 3

[Eric Rescorla (:ekr)]

Attachment: 67e3a908373f1b4895a50396c0b6df9edef914be.txt

Comment 4

[Robert Helmer [:rhelmer]]

Comment on attachment 8820363 [details] [diff] [review]
Be conservative for add-on updates and telemetry

Thanks! Sorry I did not get to this yet, was out today/yesterday.

Comment 5

[Robert Helmer [:rhelmer]]

Hm there are a bunch of places where add-ons manager uses XHR:

toolkit/mozapps/extensions/LightweightThemeManager.jsm
toolkit/mozapps/extensions/content/extensions.js
toolkit/mozapps/extensions/content/extensions.xml
toolkit/mozapps/extensions/content/update.js
toolkit/mozapps/extensions/internal/AddonRepository.jsm
toolkit/mozapps/extensions/internal/AddonUpdateChecker.jsm
toolkit/mozapps/extensions/internal/ProductAddonChecker.jsm
toolkit/mozapps/extensions/nsBlocklistService.js

Andrew and Kris, I am out til tomorrow - any thoughts on the above? I think the patch in this bug is OK but won't actually cover the GMP/system or regular add-on updates.

We have lots of other types of updates in the tree too (blocklist, etc) so I wonder if there is a more central place we can set this.

Comment 6

[Eric Rescorla (:ekr)]

Oh good. I would love to see a fix by someone who knew what they were doing

Comment 7

[Eric Rescorla (:ekr)]

Rob, is the telemetry one right?

Comment 8

[Robert Helmer [:rhelmer]]

(In reply to Eric Rescorla (:ekr) from comment #7)
> Rob, is the telemetry one right?

I think we need patches for ./toolkit/components/telemetry/ to get all of Telemetry :/

I wonder if we could do this in a more central place, would it be harmful to do it in e.g. https://dxr.mozilla.org/mozilla-central/source/dom/xhr/ ?

Maybe have beConservative be the default, with a way to override per-request if that's useful.

Comment 9

[Eric Rescorla (:ekr)]

I would be totally satisfied with "Any internal XHRs are conservative and any from web content are non-conservative". Is that something we could implement easily?

Comment 10

[Robert Helmer [:rhelmer]]

(In reply to Eric Rescorla (:ekr) from comment #9)
> I would be totally satisfied with "Any internal XHRs are conservative and
> any from web content are non-conservative". Is that something we could
> implement easily?

From a quick look we should be abe to tell if it's being called from chrome (IsSystemXHR which just checks IsSystemPrincipal) but I'll track down some folks more familiar with this code right now.

Comment 11

[(no longer active)]

Rob and I discussed how to do this centrally in XHR.  I suggested checking mPrincipal for being system in XMLHttpRequestMainThread::CreateChannel().

Comment 12

[Robert Helmer [:rhelmer]]

OK it looks like this approach will work, and it's testable (we can at least check that beConservative is set correctly for chrome vs. content in different scenarios such as with workers etc)

I am working up a patch now, going to move the bug component to something more appropriate.

Comment 13

[Robert Helmer [:rhelmer]]

Attachment: Bug 1323538 - use conservative TLS settings for XHR in chrome code f?ehsan

With the recent introduction of TLS 1.3, there's evidence of updates not working
due to middleboxes that haven't caught up. Since there are many places in
the tree that use XHR from chrome code for updates, using the "conservative"
setting in TLS for chrome XHR seems appropriate.

Note that content still gets bleeding-edge TLS changes, this is mostly a
hedge against updates and other internal Firefox features being broken.

Review commit: https://reviewboard.mozilla.org/r/100014/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/100014/

Comment 14

[Robert Helmer [:rhelmer]]

Comment on attachment 8820534 [details]
Bug 1323538 - use conservative TLS settings for XHR in chrome code f?ehsan

I am pretty confident about the code change, but this is missing the worker tests that we discussed - I can't figure out any way to get at the internal channel inside the worker (SpecialPowers don't work inside workers AFAICT), and we can't pass the regular channel object out, so I am not sure how we can verify that the flag is set correctly.

I am pretty sure that we don't use workers for the primary reason we want to land this now (XHR is used by Firefox update mechanisms) - I do agree that it would be ideal to have an automated test here though.

So, asking for feedback on the above rather than review :)

Comment 15

[Masatoshi Kimura [:emk]]

How about registering "http-on-modify-request" observer?

Comment 16

[(no longer active)]

(In reply to Masatoshi Kimura [:emk] from comment #15)
> How about registering "http-on-modify-request" observer?

That's a fantastic idea!  You can easily get your hands on the channel object there.

Comment 17

[(no longer active)]

Comment on attachment 8820534 [details]
Bug 1323538 - use conservative TLS settings for XHR in chrome code f?ehsan

https://reviewboard.mozilla.org/r/100014/#review100720

Looks good, but we need moar tests.  f+ (but MozReview doesn't let me set the flag.)

::: dom/xhr/XMLHttpRequestMainThread.cpp:2545
(Diff revision 1)
> +    // Disable cutting edge features in chrome requests, like TLS 1.3,
> +    // where middleboxes might brick us.
> +    // Allow these features in content requests.
> +    nsCOMPtr<nsIHttpChannelInternal> httpInternal = do_QueryInterface(httpChannel);
> +    if (httpInternal) {
> +      httpInternal->SetBeConservative(nsContentUtils::IsSystemPrincipal(mPrincipal));

Can you please call this once in the beginning of the function and use it both here and above?

::: dom/xhr/tests/test_xhr_conservative_tls.html:38
(Diff revision 1)
> +  ok("beConservative" in channel, "conservative TLS settings are set in internal channel");
> +  ok(!channel.beConservative, "conservative TLS settings are not used from content");
> +});
> +
> +add_task(function* test_xhr_chrome() {
> +  let xhr = SpecialPowers.Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance()

For testing chrome privileged workers, and ChromeWorkers you should use a mochitest-chrome, I think.

Comment 18

[Kris Maglione [:kmag]]

Comment on attachment 8820534 [details]
Bug 1323538 - use conservative TLS settings for XHR in chrome code f?ehsan

https://reviewboard.mozilla.org/r/100014/#review100802

::: dom/xhr/XMLHttpRequestMainThread.cpp:2545
(Diff revision 1)
> +    // Disable cutting edge features in chrome requests, like TLS 1.3,
> +    // where middleboxes might brick us.
> +    // Allow these features in content requests.
> +    nsCOMPtr<nsIHttpChannelInternal> httpInternal = do_QueryInterface(httpChannel);
> +    if (httpInternal) {
> +      httpInternal->SetBeConservative(nsContentUtils::IsSystemPrincipal(mPrincipal));

Would it be better to only set this if the request principal is the system principal, and keep the default otherwise?

::: dom/xhr/tests/test_xhr_conservative_tls.html:26
(Diff revision 1)
> +</div>
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +"use strict";
> +
> +const NORMAL_URL = "http://example.com/tests/dom/xhr/tests/test_xhr_conservative_tls.html";

Shouldn't this be an HTTPS URL?

Comment 19

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #18)
> Comment on attachment 8820534 [details]
> Bug 1323538 - use conservative TLS settings for XHR in chrome code f?ehsan
> 
> https://reviewboard.mozilla.org/r/100014/#review100802
> 
> ::: dom/xhr/XMLHttpRequestMainThread.cpp:2545
> (Diff revision 1)
> > +    // Disable cutting edge features in chrome requests, like TLS 1.3,
> > +    // where middleboxes might brick us.
> > +    // Allow these features in content requests.
> > +    nsCOMPtr<nsIHttpChannelInternal> httpInternal = do_QueryInterface(httpChannel);
> > +    if (httpInternal) {
> > +      httpInternal->SetBeConservative(nsContentUtils::IsSystemPrincipal(mPrincipal));
> 
> Would it be better to only set this if the request principal is the system
> principal, and keep the default otherwise?

What do you mean by the request principal?  mPrincipal here is the principal of the worker creating the XHR for the worker exposed API and the principal of the global constructing the XHR in the main thread case.

> ::: dom/xhr/tests/test_xhr_conservative_tls.html:26
> (Diff revision 1)
> > +</div>
> > +<pre id="test">
> > +<script class="testbody" type="text/javascript">
> > +"use strict";
> > +
> > +const NORMAL_URL = "http://example.com/tests/dom/xhr/tests/test_xhr_conservative_tls.html";
> 
> Shouldn't this be an HTTPS URL?

Interesting...  Are we trying to only set this flag for TLS connections?  If yes, we should probably add a scheme check to the code as well.

Also, thinking a bit more broadly on this, is it reasonable to expect that we will never do something based on the "be conservative" flag that is unacceptable to other chrome callers (such as add-ons)?

Comment 20

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #19)
> What do you mean by the request principal?  mPrincipal here is the principal
> of the worker creating the XHR for the worker exposed API and the principal
> of the global constructing the XHR in the main thread case.

I mean mPrincipal. What I'm suggesting is that we only set beConservative in
the case where it is the principal, rather than always setting it, based on
whether the principal is the system principal.

> > ::: dom/xhr/tests/test_xhr_conservative_tls.html:26
> > (Diff revision 1)
> > > +</div>
> > > +<pre id="test">
> > > +<script class="testbody" type="text/javascript">
> > > +"use strict";
> > > +
> > > +const NORMAL_URL = "http://example.com/tests/dom/xhr/tests/test_xhr_conservative_tls.html";
> >
> > Shouldn't this be an HTTPS URL?
>
> Interesting...  Are we trying to only set this flag for TLS connections?  If
> yes, we should probably add a scheme check to the code as well.

I think that setting it for all connections should be fine, but HTTPS is the
case that we really care about, and I don't think we should automatically
assume that an HTTP URL will behave exactly the same way, here.

> Also, thinking a bit more broadly on this, is it reasonable to expect that
> we will never do something based on the "be conservative" flag that is
> unacceptable to other chrome callers (such as add-ons)?

Probably not. I'm personally still leaning towards handling this with a helper
module for these kinds of requests rather than adding a special case to the
XHR code.

Comment 21

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #20)
> (In reply to :Ehsan Akhgari from comment #19)
> > What do you mean by the request principal?  mPrincipal here is the principal
> > of the worker creating the XHR for the worker exposed API and the principal
> > of the global constructing the XHR in the main thread case.
> 
> I mean mPrincipal. What I'm suggesting is that we only set beConservative in
> the case where it is the principal, rather than always setting it, based on
> whether the principal is the system principal.

Hmm, but SetBeConservative() is no-op if you call it with false again...  I guess we can avoid a QI for non-chrome XHRs if we check that first.

> > > ::: dom/xhr/tests/test_xhr_conservative_tls.html:26
> > > (Diff revision 1)
> > > > +</div>
> > > > +<pre id="test">
> > > > +<script class="testbody" type="text/javascript">
> > > > +"use strict";
> > > > +
> > > > +const NORMAL_URL = "http://example.com/tests/dom/xhr/tests/test_xhr_conservative_tls.html";
> > >
> > > Shouldn't this be an HTTPS URL?
> >
> > Interesting...  Are we trying to only set this flag for TLS connections?  If
> > yes, we should probably add a scheme check to the code as well.
> 
> I think that setting it for all connections should be fine, but HTTPS is the
> case that we really care about, and I don't think we should automatically
> assume that an HTTP URL will behave exactly the same way, here.

Actually we should definitely ignore the URL here, since for example the channel can get HSTS upgraded, or redirected to a TLS site, etc.

> > Also, thinking a bit more broadly on this, is it reasonable to expect that
> > we will never do something based on the "be conservative" flag that is
> > unacceptable to other chrome callers (such as add-ons)?
> 
> Probably not. I'm personally still leaning towards handling this with a
> helper
> module for these kinds of requests rather than adding a special case to the
> XHR code.

Honestly the right answer depends on what the "be conservative" flag means.  Is it intended to mean "don't do the fancy nice things which we may want to expose to Web pages, just stick to the basics"?  If yes, then we should probably set it for all loads triggered via chrome.  If it means "avoid really risky stuff which are currently experimental" then maybe we don't need to fix it centrally in XHR.  Needinfoing ekr about that...

Another alternative is to do this in XHR only if the chrome caller passes in a flag, so that only chrome callers that are worried about something can opt into it.

That all being said, if this flag only controls whether we use TLS 1.3, these distinctions are only theoretically interesting, and if it means that while we don't *have to* fix it in XHR, it's just quicker and safer, that's fine by me.  This was in fact my original understanding based on my conversation with Robert.

Comment 22

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #21)
> > I mean mPrincipal. What I'm suggesting is that we only set beConservative in
> > the case where it is the principal, rather than always setting it, based on
> > whether the principal is the system principal.
>
> Hmm, but SetBeConservative() is no-op if you call it with false again...  I
> guess we can avoid a QI for non-chrome XHRs if we check that first.

My thought was more that at some point it may have some other initial value
for some requests, and we shouldn't override that when not necessary.

> > I think that setting it for all connections should be fine, but HTTPS is the
> > case that we really care about, and I don't think we should automatically
> > assume that an HTTP URL will behave exactly the same way, here.
>
> Actually we should definitely ignore the URL here, since for example the
> channel can get HSTS upgraded, or redirected to a TLS site, etc.

Well, my concern is more that we currently only care about this in cases where
we know the starting URL is HTTPS, so if we're only going to test one case,
that should be the one we test.

> > Probably not. I'm personally still leaning towards handling this with a
> > helper
> > module for these kinds of requests rather than adding a special case to the
> > XHR code.
> 
> Honestly the right answer depends on what the "be conservative" flag means.
> Is it intended to mean "don't do the fancy nice things which we may want to
> expose to Web pages, just stick to the basics"? If yes, then we should
> probably set it for all loads triggered via chrome.  If it means "avoid
> really risky stuff which are currently experimental" then maybe we don't
> need to fix it centrally in XHR.  Needinfoing ekr about that...

It means "don't use any fancy new TLS features that our endpoints may support
but MitM proxies may not". Which means that we probably only want to use it
for requests that we're already verifying in other ways, to avoid degrading
security in the others.

Comment 23

[Masatoshi Kimura [:emk]]

I agree with Kris. It is not a good idea to depend on the fact that currently updaters happen to use XHR. For example, it will break if we refactor updaters so that they use fetch. Updaters should opt-in the beConservative option.

Comment 24

[Robert Helmer [:rhelmer]]

(In reply to Masatoshi Kimura [:emk] from comment #23)
> I agree with Kris. It is not a good idea to depend on the fact that
> currently updaters happen to use XHR. For example, it will break if we
> refactor updaters so that they use fetch. Updaters should opt-in the
> beConservative option.

Kris and I have been talking about consolidating code and data updates to a common module anyway, so that is certainly a valid option. It's more upfront cost since we need to audit all callers and switch the appropriate ones over, but it's a more local change and feels less likely to have unintended consequences.

We have quite a bit of chrome code using XHR, and I can't imagine we *want* any of that to break on TLS 1.3 middlebox related errors, but I think we could carve out the most critical services and move those first.

I'm most concerned about helping people writing new code to do the right thing - if we implement this as a new JSM then we need to be really proactive about getting everything moved over and communicating that raw fetch/xhr is not the right thing to use from chrome.

Comment 25

[Eric Rescorla (:ekr)]

Rob, could we potentially do this in two stages:

1. Land approximately the patch we have now.
2. Do a bigger refactor (which I agree is a good idea) later.

This would give us the confidence to experiment with new stuff in TLS while being sure that we're not breaking the most critical stuff.

Comment 26

[(no longer active)]

(In reply to Robert Helmer [:rhelmer] from comment #24)
> (In reply to Masatoshi Kimura [:emk] from comment #23)
> > I agree with Kris. It is not a good idea to depend on the fact that
> > currently updaters happen to use XHR. For example, it will break if we
> > refactor updaters so that they use fetch. Updaters should opt-in the
> > beConservative option.
> 
> Kris and I have been talking about consolidating code and data updates to a
> common module anyway, so that is certainly a valid option. It's more upfront
> cost since we need to audit all callers and switch the appropriate ones
> over, but it's a more local change and feels less likely to have unintended
> consequences.

OK, so how about we add an option to MozXMLHttpRequestParameters for use that in the updater code?

> We have quite a bit of chrome code using XHR, and I can't imagine we *want*
> any of that to break on TLS 1.3 middlebox related errors, but I think we
> could carve out the most critical services and move those first.

Well, we don't want anything to break on TLS 1.3 MITM proxy errors in the ideal situation.  My understanding was that we just want to do this while we're testing TLS 1.3 so that in case we get users to a state where no TLS connection works for them, we can update them to a good state.

> I'm most concerned about helping people writing new code to do the right
> thing - if we implement this as a new JSM then we need to be really
> proactive about getting everything moved over and communicating that raw
> fetch/xhr is not the right thing to use from chrome.

I can't really think of any other case where we really want to do this, chrome XHR is kind of an arbitrary distinction and it doesn't map to anything of actual value given that add-ons can do that for all sorts of purposes.  And I think Kris' point about only doing this for cases where we verify the downloaded content through other means too is a valid one.  So given that people who work on the updater code take all sorts of considerations like this into account already, this doesn't worry me much.

Comment 27

[Robert Helmer [:rhelmer]]

I just chatted with Kris and Ehsan in IRC, and if we're going to make this behavior optional (so it needs to be enabled by the caller), I don't think there's a benefit to having it implemented as a MozXMLHttpRequestParameter in the XHR impl directly, we might as well just set beConservative on the channel from JS.

Kris and I have been planning to make a new JSM to centralize the way we do these sorts of updates anyway (to simplify things like this, as well as telemetry and diagnostics on update failures), we can put a minimal version of this together and do that now.

This simplifies the testing story here, since we don't need to worry about supporting workers etc.

I'll file a new bug to track this and close the current one.

Comment 28

[Robert Helmer [:rhelmer]]

Closing this in favor of bug 1325501.