1193926 - Don't register chrome for dictionary add-ons

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

Looks like dictionary add-ons (type 64) inadvertently register chrome.manifest files when used. There is no need for this. I think you could override parts of the browser with this and so there is a way to bypass signing and run unverified code.

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch

This restricts registering chrome to only the types that need it. Technically that is extensions and locale packs. But locale packs aren't signed, so they can inject chrome into the browser :(

Sounds like there aren't many (if any?) locale packs that aren't owned by us so maybe we can just require those to be signed. The alternative is to change the manifest parser so locale packs can only define locale chrome, I don't know if that might break things.

Comment 2

[Robert Kaiser]

Well, locale packs should only be able to register locale and full themes (which also need to support chrome.manifest) can only register skin-related stuff. I thought we already had code for that.

Comment 3

[Dave Townsend [:mossop]]

(In reply to Robert Kaiser (:[e-mail]) from comment #2)
> Well, locale packs should only be able to register locale and full themes
> (which also need to support chrome.manifest) can only register skin-related
> stuff. I thought we already had code for that.

We do for full themes, I'm not seeing it for locale packs though

Comment 4

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

I favor restricting language packs to only locale entries in chrome.manifest. I believe we already have that restriction on AMO.

Comment 5

[Daniel Veditz [:dveditz]]

I also thought locales were restricted to registering locale types, and themes only skin types. We should enforce that in the client, not just as an AMO rule.

What about "experiments"? Do they need to register chrome? nothing else should be able to.

Even restricted to locale chrome resources, though, we have some places where we read raw HTML out of localizations. I'm not a fan but it's a reality that even locale types can inject scripts into the browser. I'd like to see them signed if we could.

Comment 6

[Dave Townsend [:mossop]]

Attachment: patch

This blocks registering chrome for dictionaries and adds a new chrome location type for locale packs where only locale directives are registered. The boolean flags in the manifest parsing directive list were getting a bit unwieldy so I've instead switched to type flags to split them up and say what is allowed for each chrome location type.

Comment 7

[Benjamin Smedberg]

Comment on attachment 8649012 [details] [diff] [review]
patch

I don't think the NS_LOCALE_TYPE restriction is actually useful. If you register a locale package you can get chrome privileges via the DTD anyway. So can we just not do that part?

Comment 8

[Dave Townsend [:mossop]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> Comment on attachment 8649012 [details] [diff] [review]
> patch
> 
> I don't think the NS_LOCALE_TYPE restriction is actually useful. If you
> register a locale package you can get chrome privileges via the DTD anyway.
> So can we just not do that part?

How is that? If we don't do that part then we have to make signing a requirement for locale packs.

Comment 9

[Benjamin Smedberg]

DTDs are XML. So it's possible to inject arbitrary script into chrome-privilege pages using any entity that is included directly in the page (not in an attribute). For example:

<!ENTITY changeSearchSettings.button  "<script>Cc[...];</script>Change Search Settings">

Comment 10

[Benjamin Smedberg]

Signing should be a requirement for locale packs, at least until we move away from DTD-based localization completely.

Comment 11

[Dave Townsend [:mossop]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #10)
> Signing should be a requirement for locale packs, at least until we move
> away from DTD-based localization completely.

Ok, spun that out into bug 1197876

Comment 12

[Dave Townsend [:mossop]]

Attachment: patch

Removed the unnecessary churn here

Comment 13

[Dave Townsend [:mossop]]

Attachment: patch

The rest of the patch for posterity

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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