Closed Bug 1449548 (CVE-2018-5168) Opened 6 years ago Closed 6 years ago

Lightweight themes can be installed automatically, without user's consent

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr52 60+ verified
firefox59 --- wontfix
firefox60 + verified
firefox61 + verified

People

(Reporter: jwkbugzilla, Assigned: Gijs)

Details

(Keywords: csectype-priv-escalation, sec-moderate, Whiteboard: [adv-main60+][adv-esr52.8+])

Attachments

(3 files)

Attached file Proof of concept page
The security checks determining which websites are allowed to install lightweight themes and particularly which are allowed to install them without user's consent are based on the baseURI property of the theme element. Unlike the document URI, this property can be easily manipulated - e.g. by using a <base href="..."> tag. This is what the attached proof of concept page does - opening it will immediately install a theme named "Hacked you" because the security checks consider that page to be discovery.addons.mozilla.org. This is also problematic in light of bug 527463: a theme might not even change the appearance of Firefox but invisibly track the user via updateURL instead.
Forgot to mention, I reproduced this in Firefox 61.0a1 (2018-03-28) nightly on Linux.
(In reply to Wladimir Palant (for Adblock Plus info Cc bugzilla@adblockplus.org) from comment #0)
> Unlike the document URI, this property can be easily manipulated

Using the document URI would still be wrong. We've audited for this in the past. Ugh.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attached patch Patch v0.1Splinter Review
Attachment #8963107 - Flags: review?(mconley)
Attachment #8963107 - Flags: review?(aswan)
Comment on attachment 8963107 [details] [diff] [review]
Patch v0.1

Review of attachment 8963107 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-addons.js
@@ +667,4 @@
>        return false;
>      }
>  
> +    if (!principal.URI.schemeIs("https")) {

Self-nit: this can probably be unified with the previous condition.

@@ +674,5 @@
>      let pm = Services.perms;
> +    return pm.testPermission(principal.URI, "install") == pm.ALLOW_ACTION;
> +  },
> +
> +  _showUndoPrompt(principal) {

Self-nit: should probably call this _shouldShowUndoPrompt. Also, perhaps it's better off being re-inlined into the 1 callsite. *shrug*. Up to y'all what you think is better.
Out of curiosity, how did you find this? I would like us to audit for more mistaken checks here, but it's not clear to me how to go about that - obviously not all uses of 'baseURI' are suspect, and there are rather a lot of them. :-\

Separately, we're already looking at how to do a better job of doing a unified set of security checks for things like this, which would have helped... See also discussion at https://groups.google.com/forum/?fromgroups=&hl=en#!topic/firefox-dev/naSYfA-e2Jw , for which (coincidentally) there's a meeting later today.
Flags: needinfo?(gaubugzilla)
(In reply to :Gijs from comment #5)
> Out of curiosity, how did you find this?

Nothing fancy - I am going through all code interacting with web pages, looking for wrong assumptions. In particular, this means all addEventListener(...,...,...,true) calls.
Flags: needinfo?(gaubugzilla)
Comment on attachment 8963107 [details] [diff] [review]
Patch v0.1

Review of attachment 8963107 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch, Wladimir, and thanks for hopping on this, Gijs.

Patch looks good to me - though I wonder if we need to be sending baseURI at all now.

::: browser/base/content/content.js
@@ +999,5 @@
>    handleEvent(event) {
>      switch (event.type) {
>        case "InstallBrowserTheme": {
>          sendAsyncMessage("LightWeightThemeWebInstaller:Install", {
>            baseURI: event.target.baseURI,

Should we even keep sending the baseURI here? Looking at the patch and the rest of LightweightThemeManager.jsm, it looks like it's only used by _sanitizeTheme... maybe we should use the principal URI there as well instead?

@@ +1007,5 @@
>          break;
>        }
>        case "PreviewBrowserTheme": {
>          sendAsyncMessage("LightWeightThemeWebInstaller:Preview", {
>            baseURI: event.target.baseURI,

Same as above.
Attachment #8963107 - Flags: review?(mconley) → review+
(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #7)
> Should we even keep sending the baseURI here? Looking at the patch and the
> rest of LightweightThemeManager.jsm, it looks like it's only used by
> _sanitizeTheme... maybe we should use the principal URI there as well
> instead?

It gets used to resolve relative URIs in the theme, and nothing else AFAICT. So if the theme specified a background image as 'foo.jpg', it'd get resolved against the base URI defined in the document that specified the lwtheme. I think that's probably fine, and potentially needs to keep doing what it's doing to avoid a case where some webpage hypothetically had:

// on www.foo.com/mythemes/

<base href="mythemeresources/">

<body data-lwthemestuff="... foo.jpg">



because if we use the principal's URI that won't use the baseURI, and so the image URIs won't resolve correctly.

It's an edgecase, but because this isn't technically a security check, I think it's OK.
Makes sense, okay. Thanks.
Once again bitten in the butt by giving special powers to our own web pages.

The actual security damage here is low (easily reset from the addons dialog) but the potential reputational damage could be horrible if script-kiddies weaponize competing offensive images and spread via malicious ads.
Comment on attachment 8963107 [details] [diff] [review]
Patch v0.1

Review of attachment 8963107 [details] [diff] [review]:
-----------------------------------------------------------------

I assume you flagged me for the webapi.testing bits, r=me on that.
Between Gijs and mconley I think the baseURI/principal bits have been carefully thought through but if you want another reviewer with a deep enough understanding to spot potential issues, I don't think I have the depth to provide that...
Attachment #8963107 - Flags: review?(aswan) → review+
https://hg.mozilla.org/mozilla-central/rev/a578a9117c19
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8963107 [details] [diff] [review]
Patch v0.1

Approval Request Comment
[Feature/Bug causing the regression]: n/a
[User impact if declined]: security issue where people can driveby-install lightweight themes
[Is this code covered by automated tests?]: generally yes, in https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_bug592338.js .
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: 

1. open testcase on this bug
2. theme shouldn't automatically install (ie only install after explicit user confirmation in the doorhanger)

[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: small JS-only change in a feature that we only really use for lwt install/preview on AMO. Also, this is a security fix and we still have quite some beta runway left, so I'm not too worried...
[String changes made/needed]: no
Attachment #8963107 - Flags: approval-mozilla-beta?
Comment on attachment 8963107 [details] [diff] [review]
Patch v0.1

Approved for 60.0b11.
Attachment #8963107 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/ba71ad255604

Are we thinking about rebasing this for ESR52 too?
Flags: needinfo?(gijskruitbosch+bugs)
Attached patch Patch for esrSplinter Review
(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)
> https://hg.mozilla.org/releases/mozilla-beta/rev/ba71ad255604
> 
> Are we thinking about rebasing this for ESR52 too?

Can do.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
sec-moderate with driveby negative effects for users, relatively simple patch
User impact if declined: security issue.
Fix Landed on Version: 61, uplift to 60
Risk to taking this patch (and alternatives if risky): relatively low... small-ish JS-only patch, has automated tests for the basics not breaking.
String or UUID changes made by this patch: none
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8965509 - Flags: review+
Attachment #8965509 - Flags: approval-mozilla-esr52?
Comment on attachment 8965509 [details] [diff] [review]
Patch for esr

If kids these days are anything like me and my friends were back in school, comment 10 sounds frighteningly plausible in situations where ESR is likely being used. Approved for 52.8.0.
Attachment #8965509 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Flags: sec-bounty? → sec-bounty+
Group: toolkit-core-security → core-security-release
Whiteboard: [adv-main60+][adv-esr52.8+]
Flags: qe-verify+
I have managed to reproduce the issue described in comment 0 using Firefox 61.0a1 (BuildId:20180328220110).

This issue is verified fixed using Firefox 61.0a1 (BuildId:20180429220443) and Firefox 60.0b16 on Windows 10 64bit, macOS 10.12.6 and Ubuntu 16.04 64bit.

Leaving a ni on myself to verify this on 52.8.0.
Flags: needinfo?(emil.ghitta)
Alias: CVE-2018-5168
This issue is verified fixed using Firefox 52.8.0 esr on Windows 10 64bit, macOS 10.13.3 and Ubuntu 16.06 64bit.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(emil.ghitta)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.