1539595 - (CVE-2019-11741) Consider an origin-whitelist for early site isolation for AMO and accounts.firefox.com

Status: VERIFIED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Alex Gaynor [:Alex_Gaynor]]

These were factors in the sandbox escapes seen at pwn2own -- if UXSSing to them was impossible, that'd be great.

This might be tractable since I don't think iframes need to work for either of them.

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

After talking with Nika and Neha, I think the best path here is to let Fission M2 happen, since that brings a lot of infrastructure around tab switching and other bits that we'll want, and then pursue implementing this. I think doing this at that point will a) make this easier, b) make the UI more seamless, c) let us build this in a way that's more forward looking (i.e. not introducing a new special remote type).

Comment 2

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

I've made some progress on this; so I'm going to post my in-progress patches for feedback. I don't have tests for the overall isolation yet; and I haven't finished a test run covering the add-on restrictions; but I believe I don't have any other failures at the moment.

Comment 3

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1539595 - Rename Priveleged Content Process to PrivlegedAbout Content Process r=mconley

Comment 4

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1539595 - Create a separate content process type for privleged mozilla content r=nika

Depends on D30274

Comment 5

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1539595 - Reject FXA messages if they don't come from the correct type of process r=rfkelly

Depends on D30276

Comment 6

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1539595 - Check the remoteType when we receive a privileged AMO message r=aswan

Depends on D30277

Comment 7

[Ryan Kelly [:rfkelly]]

Shane, we should think about how to QA this change from an FxA perspective, i.e. what Sync login flows and other account operations we should test out to ensure no breakage here. ni? myself to come back to this, but any suggestions from your side would be most welcome as well.

Comment 8

[Shane Tomlinson [:stomlinson]]

The flows that I can imagine:

• Sign in/up from the hamburger menu
• Sign in/up from the about:whatsnew page
• Sign in/up in a private tab
• Open /settings from a private tab via about:preferences#sync
• Open /settings in a normal tab via about:preferences#sync
• Ensure pairing still works in a normal tab
• Ensure pairing still works in a private tab

Comment 9

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

I'm going to land this after -central is 69; let it bake for 1/2 to 3/4 of a cycle, then request uplift to beta so it goes out in 68.

Comment 10

[Ryan Kelly [:rfkelly]]

Note to self: need to check whether/how this interacts with various self-hosted setups, including Stage and Mozilla China.

Comment 11

[Ryan Kelly [:rfkelly]]

Note to self: need to check whether/how this interacts with various self-hosted setups, including Stage and Mozilla China.

After thinking it through, I fear this is going to interact badly with non-standard account setups, including:

• The Mozilla China repack, which switches the default accounts server from "accounts.firefox.com" to "accounts.firefox.com.cn"
• Self-hosters, who switch various account-related URLs to point to their own server
• QA, who regularly use the identity.fxaccounts.autoconfig.uri pref to point the browser dev or stage account servers.

Digging into the code a bit, the origin from which we accept account-related messages is identified by the pref identity.fxaccounts.remote.root:

https://dxr.mozilla.org/mozilla-central/rev/3c0f78074b727fbae112b6eda111d4c4d30cc3ec/services/fxaccounts/FxAccountsWebChannel.jsm#560

So I think we'll need to either:

• Arrange for the origin in identity.fxaccounts.remote.root to be site-isolated, or
• Disable the site-isolation check if identity.fxaccounts.remote.root does not have its default value

The second option is probably the simpler place to start, and would still provide a lot of value to users in its default configuration.

Comment 12

[:Gijs (he/him)]

Is this going to get us a separate process even when the current privileged about process is preffed off? Because AIUI we're still not shipping it, which might be an issue for the 68 uplift...

Comment 13

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

(In reply to Ryan Kelly [:rfkelly] from comment #11)

• Arrange for the origin in identity.fxaccounts.remote.root to be site-isolated, or
• Disable the site-isolation check if identity.fxaccounts.remote.root does not have its default value

The second option is probably the simpler place to start, and would still provide a lot of value to users in its default configuration.

I decided to do what you suggested in the comment and if identity.fxaccounts.remote.root is not in the isolated domains list, disable the remoteType check. This doesn't preclude the first option (Mozilla China or individual users can add their domain to the isolated domain list; and I think Mozilla China at least should do so) - but it doesn't take it by default. This ensures that we aren't adding potentially concerning domains into the same bucket as AMO.

(In reply to :Gijs (he/him) from comment #12)

Is this going to get us a separate process even when the current privileged about process is preffed off? Because AIUI we're still not shipping it, which might be an issue for the 68 uplift...

Yes, this is independent of the existing privileged content process (it just renames it.) Enabling this with the other disabled works fine in my testing.

Comment 14

[Ryan Kelly [:rfkelly]]

I built this locally and ran through a bunch of account flows, including the ones from Comment 8 and including changing my password from a signed-in device. AFAICT it's all working properly with both the default account setup, and a custom fxa server. Nice work!

Comment 15

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Thanks Ryan. Alright, let's see if this sticks...

Comment 16

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

Rejected by the server for .ftl changes, requested reviews by flod.

Comment 17

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

ftl changes needing review.

Comment 18

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

https://hg.mozilla.org/integration/autoland/rev/ba3e353c69578a9d9bea098d31961ba89b0f8d5a
https://hg.mozilla.org/integration/autoland/rev/6310e6dabceb7812c896831edc75f91231ac9913
https://hg.mozilla.org/integration/autoland/rev/95f0b82ec253c0b4d46b39b002bca0b022b051ee
https://hg.mozilla.org/integration/autoland/rev/2b0bb889b08780466624ddff1dc217053c8dbdc5

Comment 19

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Looks like this is getting backed out. Some stuff landed between the checkin-needed and the l10n review; I've rebased and fixed the compile issue; doing a try run to make sure there aren't test failures; then I'll remark it tonight.

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

Comment 20

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

Backout for build bustage in nsAboutRedirector.cpp: https://hg.mozilla.org/integration/autoland/rev/4ede32687503be6d101d6af5ea8103fad93a85ab

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Ctestfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&group_state=expanded&selectedJob=248780841&revision=2b0bb889b08780466624ddff1dc217053c8dbdc5
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=248745853&repo=autoland

/builds/worker/workspace/build/src/docshell/base/nsAboutRedirector.cpp:94:26: error: no member named 'URI_CAN_LOAD_IN_PRIVILEGED_CHILD' in 'nsIAboutModule'
/builds/worker/workspace/build/src/docshell/base/nsAboutRedirector.cpp:138:32: error: no matching function for call to 'ArrayLength'

Comment 21

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

https://hg.mozilla.org/integration/autoland/rev/7f30c1db8835cf11237b51835ca26522f8d0c06d
https://hg.mozilla.org/integration/autoland/rev/d97e735a0ee9e6c4e3f39b445d2653c766290826
https://hg.mozilla.org/integration/autoland/rev/69ca808a0ff34f2088beb70ffcb7fe9b6dc17782
https://hg.mozilla.org/integration/autoland/rev/a707cab2f14d5a33998bdaa3fbbbdc19e5317af1

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/7f30c1db8835
https://hg.mozilla.org/mozilla-central/rev/d97e735a0ee9
https://hg.mozilla.org/mozilla-central/rev/69ca808a0ff3
https://hg.mozilla.org/mozilla-central/rev/a707cab2f14d

Comment 23

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

[Tracking Requested - why for this release]: Once this has baked in Nightly and ideally been tested by PI, I'd like to uplift to beta so we have this sandbox escape vector closed in ESR 68.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

This is something I assume we'll want to backport to 68 ahead of the next ESR. Please nominate this for Beta approval when you feel it's had sufficient Nightly bake time.

Comment 25

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

We have to disable this for now. Bug 1557074 will track re-enabling it. At that point we'll see if we can uplift this to 68.

That is far from certain though, considering we would also need to uplift some number of currently-in-progress Fission-related patches. At the same time it would not be ideal to leave these sandbox escapes open in ESR...

Comment 26

[Julien Cristau [:jcristau] (back April 22)]

Sounds like this is wontfix for 68.0.

Comment 27

[Daniel Bodea [:danibodea]]

(In reply to Ryan Kelly [:rfkelly] from comment #14)

I built this locally and ran through a bunch of account flows, including the ones from Comment 8 and including changing my password from a signed-in device. AFAICT it's all working properly with both the default account setup, and a custom fxa server. Nice work!

This bug got the "qe-verify+" tag. Ryan, did you verify this implementation in a local build? I think it should be verified again in the latest Nightly and then set as verified. Can you please provide more information on how this implementation should be verified correctly? Comment 8 lists a number of flows to be checked, which are to sign in or sign up to Firefox and ensure sync works correctly, right? Are there other flows that need to be checked? Thanks!

Comment 28

[Ryan Kelly [:rfkelly]]

Ryan, did you verify this implementation in a local build?

Correct, in a local build.

If I understand correctly, this is actually off by default in Nightly due to a number of issues, and re-enablement is tracked in Bug 1557074. So I wonder if we should hold off on re-verifying until those issue have been resolved.

Comment 29

[Daniel Bodea [:danibodea]]

Alright. Will you take responsibility of QA contact on this bug and verify it when needed?
If not, please leave the information needed to test it. Thanks!

Comment 30

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

(In reply to Ryan Kelly [:rfkelly] from comment #28)

If I understand correctly, this is actually off by default in Nightly due to a number of issues, and re-enablement is tracked in Bug 1557074. So I wonder if we should hold off on re-verifying until those issue have been resolved.

The reason it's disabled is mostly do to redirects not working well with process switching right now. So I think it could be tested now in Nightly by flipping the pref; but I think the only reason to do so instead of waiting was if people had the time now and wanted to use it efficiently :)

Comment 31

[Daniel Bodea [:danibodea]]

It's been 3 months since this security bug got a fix and 2 months since it got a "qe-verify+" tag. I believe this should have some priority. This being said, Kelly, can you help details the steps to take in order to verify this fix? Otherwise, please invest some time and verify it yourself.
P.S. Consider Tom's comment 30. Thank you.

Comment 32

[Ryan Kelly [:rfkelly]]

Verifying this change involves setting browser.tabs.remote.separatePrivilegedMozillaWebContentProcess to true and then confirming that Firefox Account signin and Addon management works correctly. Here's what i did to check it again in latest nightly:

• Set browser.tabs.remote.separatePrivilegedMozillaWebContentProcess to true in about:config, then restart Firefox.
• Sign in to Sync, and observe that it worked correctly and the browser got connected to sync.
• Select "manage account" from the Firefox Account toolbar menu to visit the account settings page
• Change the account password, and observe that the browser stays connected to sync.
  • Use the Accounts toolbar menu to "sync now" and confirm that it's still connected to sync.
• Go to https://addons.mozilla.org, install some addons, and observe that this worked correctly.

If you have additional QA procedures for testing Sync signin or Addons installation, it may be worth running through those as well for additional coverage.

Comment 33

[Daniel Bodea [:danibodea]]

I have also performed the steps in the comment above on Beta v69.0 RC. No regressions or unwanted behavior were observed. Thank you.

Comment 34

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

I'll discuss enabling this on Nightly 71 during the Fission meeting.

Comment 35

[Ioana Rusiczki]

(In reply to Tom Ritter [:tjr] from comment #34)

I'll discuss enabling this on Nightly 71 during the Fission meeting.

Unfortunately I don't have access to https://bugzilla.mozilla.org/show_bug.cgi?id=1557074
Let us know when this can be tested on Nightly 71.