1563327 - Prevent users from uploading langpacks containing exploits to AMO

Status: RESOLVED FIXED

(addons.mozilla.org :: Security, defect)

Description

[:Gijs (he/him)]

In bug 1538007, we were faced with the possibility of a sandbox escape executed by installing a malicious language pack. While we've made a number of changes to Firefox, especially on 68 onwards, to defeat this attack in the product, as part of defense in depth improvements and to help protect users on older versions (especially ESR), it would be desirable to refuse to sign language packs that contain markup in their locale strings that doesn't pass some sanitizer list of allowed element/attribute/value combinations.

Comment 1

[Axel Hecht [:Pike]]

Technical nit, we shouldn't make this part of "signing", but some other step of addon submission. "Signing" is part of our release engineering flow, and we've had numerous problems in langpack signing refusing to publish already reviewed content, based on criteria that didn't apply to moco release automation langpacks.

Comment 2

[Kris Maglione [:kmag]]

Jorge, Stewart, can one of you please prioritize and assign to the appropriate person? We'd ideally like to get this done before ZDI blogs about the sandbox escapes, which they are apparently eager to do.

Comment 3

[Kris Maglione [:kmag]]

(Note: We're coming up on their standard disclosure deadline next week, so we really can't bank on more than a few days)

Comment 4

[Kris Maglione [:kmag]]

(Also bumping the importance based on the deadline. Sorry for the bugspam.)

Comment 5

[Jorge Villalobos [:jorgev] (he/him)]

Per a discussion we had with Callek in Whistler, the release process will soon change to a model where langpacks will be signed before they are submitted to AMO, so AMO is no longer a blocker in the process. Also, it sounds like there are some very specific rules that are intended to be implemented for these checks, so I'm not clear why this validation should happen on the AMO side, instead of some other tools designed for this specific purpose.

Is there a spec somewhere of what's supposed to be allowed or disallowed in a langpack?

Comment 6

[Kris Maglione [:kmag]]

This is about third-party language packs uploaded for signing by users, not langpacks generated in automation. We're relatively confident in the safety of the latter.

(In reply to Jorge Villalobos [:jorgev] from comment #5)

Is there a spec somewhere of what's supposed to be allowed or disallowed in a langpack?

Essentially, we just need to disallow any entities with markup that contains scripts, either as <script> tags, or elements with event handler attributes or javascript:/data: URLs. Running them through a standard markup sanitizer and checking that it doesn't remove anything would probably be sufficient.

Comment 7

[[e-mail]]

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

In bug 1538007, we were faced with the possibility of a sandbox escape executed by installing a malicious language pack. While we've made a number of changes to Firefox, especially on 68 onwards, to defeat this attack in the product, as part of defense in depth improvements and to help protect users on older versions (especially ESR), it would be desirable to refuse to sign language packs that contain markup in their locale strings that doesn't pass some sanitizer list of allowed element/attribute/value combinations.

Can you provide that list? How are the mitigations done in Firefox?

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

(Note: We're coming up on their standard disclosure deadline next week, so we really can't bank on more than a few days)

I think that a few days is a bit unrealistic since we'd probably be needing to make something from scratch to sanitize these formats, unless something pre-exists, also there are some issues with sanitization - see my next comment:

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

This is about third-party language packs uploaded for signing by users, not langpacks generated in automation. We're relatively confident in the safety of the latter.

(In reply to Jorge Villalobos [:jorgev] from comment #5)

Is there a spec somewhere of what's supposed to be allowed or disallowed in a langpack?

Essentially, we just need to disallow any entities with markup that contains scripts, either as <script> tags, or elements with event handler attributes or javascript:/data: URLs. Running them through a standard markup sanitizer and checking that it doesn't remove anything would probably be sufficient.

Unfortunately it's not that easy, because if I look at the english lang pack on AMO the file chrome/en-US/locale/en-US/global/dom/dom.properties contains the string <script> for translations like so:

ScriptSourceLoadFailed=Loading failed for the <script> with source “%S”.

If I ran that through a markup sanitizer like DOMPurify that would just strip out the <script> and everything after it but in this case it's benign. Since sanitization is context sensitive so I'm not sure how viable it is to sanitize up front since we don't have knowledge of the way each string is being consumed.

I presume backporting the client mitigations to older versions to alleviate the potential threat has been already discounted?

Comment 8

[Kris Maglione [:kmag]]

(In reply to Stuart Colville [:scolville] [:muffinresearch] from comment #7)

Unfortunately it's not that easy, because if I look at the english lang pack on AMO the file chrome/en-US/locale/en-US/global/dom/dom.properties contains the string <script> for translations like so:

ScriptSourceLoadFailed=Loading failed for the <script> with source “%S”.

That's a properties file. Most entries in properties files aren't used as markup, and if they are, they're automatically sanitized anyway.

This only check would to DTD entities, which are guaranteed to be treated as markup, and where the markup in each entity is guaranteed to be well-formed.

Comment 9

[:Gijs (he/him)]

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

This only check would to DTD entities, which are guaranteed to be treated as markup, and where the markup in each entity is guaranteed to be well-formed.

I mean, this is on me for the summary which did mention the other file types. Ideally we should look at those too, but Stuart's point is a good one and so let's focus on DTDs here (also given that's what we ran into with 1538007).

(In reply to Stuart Colville [:scolville] [:muffinresearch] from comment #7)

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

In bug 1538007, we were faced with the possibility of a sandbox escape executed by installing a malicious language pack. While we've made a number of changes to Firefox, especially on 68 onwards, to defeat this attack in the product, as part of defense in depth improvements and to help protect users on older versions (especially ESR), it would be desirable to refuse to sign language packs that contain markup in their locale strings that doesn't pass some sanitizer list of allowed element/attribute/value combinations.

Can you provide that list?

Yes. Though I agree with Kris that any reasonable sanitizer on all entity values in DTD files should normally work (I think) and would do nicely, I can produce a more complete list of allowed element/attribute combinations based on what we have in-product today if that was desired.

How are the mitigations done in Firefox?

We moved the things that still needed markup to fluent, which has very tight restrictions on what can be inserted. Then we made the DTD parser just throw away all markup in DTDs if invoked in privileged contexts. Unfortunately we can't do the exact same thing here because there are still DTD files with markup that get loaded in unprivileged documents (and we won't know the difference server-side), plus langpacks sometimes want to be backwards-compatible with older versions which would break langpacks for 67 or older.

I presume backporting the client mitigations to older versions to alleviate the potential threat has been already discounted?

Yes. We can't really change localization things on esr60 to that degree, and Fluent was less mature, so the option to do the same thing there doesn't really work - it'd break a bunch of Firefox UI.

We've backported some other things that should hamper using this approach, but we'd be more confident if we broke things earlier on.

Comment 10

[Axel Hecht [:Pike]]

Forgive me for asking a couple of distracting questions here, trying to hone in to ESR 60 specifically:

Can we just not take new language packs for any version of Firefox for which we don't have in-code protections, and then audit the existing ones on AMO to see if there's a problem right now?

Comment 11

[:Gijs (he/him)]

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

Forgive me for asking a couple of distracting questions here, trying to hone in to ESR 60 specifically:

Can we just not take new language packs for any version of Firefox for which we don't have in-code protections, and then audit the existing ones on AMO to see if there's a problem right now?

This wfm, but I dunno how feasible that is on the AMO side.

Comment 12

[[e-mail]]

Attachment: poc-dtd-sanitization-diff.txt

We've tried out the suggestion of trying to sanitize the key values of entities in DTD files, but using DOMPurify we're seeing a lot of changes for benign content, such as removal of useless empty markup <p></p>, Converting & to &amp;, making double quotes ", and changing double-quotes to single etc.

Simply looking for changes of sanitized vs unsanitized is going to create a lot of false positives without additional filtering.

In addition the context that DOMPurify is sanitizing for is HTML and it's not clear that is always the right context since we don't know anything about where the strings are consumed.

See the attachment for an example of the differences generated from running against the en langpack from AMO.

Instead of trying to block based on santization differences, :mat (cc'd) is going to work on looking at blocking 3rd party langpacks from being uploaded for old FF versions as per comment 10.

Additionally we will work on auditing existing langpacks using the tool above to pin-point anything that was removed by the sanitization, since that will certainly help narrow down what we need to look through.

Comment 13

[Axel Hecht [:Pike]]

Thanks for uploading the diff. This sanitizes away &brandShortName;, that's not what we want for sure. That'd be breaking all references to "Firefox" in the product.

Happy to hear that we might have a path forward on blocking 3rd party langpacks for old versions.

Comment 14

[Mathieu Pillard [:mat]]

Can we just not take new language packs for any version of Firefox for which we don't have in-code protections, and then audit the existing ones on AMO to see if there's a problem right now?

What are those versions exactly ? I need an exact version number below which I should disallow 3rd party langpack submissions.

Comment 15

[Axel Hecht [:Pike]]

bug 1539759 is fixed in 68, so 67 and older, and in particular 60 esr.

Comment 16

[Mathieu Pillard [:mat]]

After talking with :jorgev and :theone we decided to solve this by... completely disallowing 3rd party language packs submissions on AMO, at least for now. There are very few of them currently, so it's not worth the extra effort. This has been pushed a couple hours ago. Users with a mozilla.com email can still submit them as normal.

Comment 17

[Jorge Villalobos [:jorgev] (he/him)]

We actually have one third-party langpack with users, and it's been that way for years. We're looking into ways of making that langpack official, but either way I believe it's not worth the engineering effort to continue supporting this on AMO.

Comment 18

[Mathieu Pillard [:mat]]

Small update: because the user release automation uses to submit langpack does not have a mozilla.com email, we ended up making another push today and used a specific permission instead to guard langpack submission. We gave that permission to a group that this user is a part of, but we could also make other users a part of it as well.