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)
Toolkit
Add-ons Manager
Tracking
()
VERIFIED
FIXED
mozilla61
People
(Reporter: jwkbugzilla, Assigned: Gijs)
Details
(Keywords: csectype-priv-escalation, sec-moderate, Whiteboard: [adv-main60+][adv-esr52.8+])
Attachments
(3 files)
1.69 KB,
text/html
|
Details | |
8.09 KB,
patch
|
aswan
:
review+
mconley
:
review+
RyanVM
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
8.29 KB,
patch
|
Gijs
:
review+
RyanVM
:
approval-mozilla-esr52+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•6 years ago
|
||
Forgot to mention, I reproduced this in Firefox 61.0a1 (2018-03-28) nightly on Linux.
Assignee | ||
Comment 2•6 years ago
|
||
(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
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #8963107 -
Flags: review?(mconley)
Attachment #8963107 -
Flags: review?(aswan)
Assignee | ||
Comment 4•6 years ago
|
||
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.
Assignee | ||
Comment 5•6 years ago
|
||
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)
Reporter | ||
Comment 6•6 years ago
|
||
(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 7•6 years ago
|
||
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+
Assignee | ||
Comment 8•6 years ago
|
||
(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.
Comment 9•6 years ago
|
||
Makes sense, okay. Thanks.
Comment 10•6 years ago
|
||
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.
status-firefox59:
--- → wontfix
status-firefox60:
--- → affected
status-firefox-esr52:
--- → affected
tracking-firefox60:
--- → +
tracking-firefox61:
--- → +
tracking-firefox-esr52:
--- → 60+
Flags: sec-bounty?
Keywords: csectype-priv-escalation,
sec-moderate
Comment 11•6 years ago
|
||
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+
Assignee | ||
Comment 12•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a578a9117c193ecfd6c7abdca5fbb841e46155ab
Comment 13•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a578a9117c19
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Comment 14•6 years ago
|
||
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 15•6 years ago
|
||
Comment on attachment 8963107 [details] [diff] [review] Patch v0.1 Approved for 60.0b11.
Attachment #8963107 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 16•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/ba71ad255604 Are we thinking about rebasing this for ESR52 too?
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 17•6 years ago
|
||
(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 18•6 years ago
|
||
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+
Comment 19•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/48a678d7cb81
Updated•6 years ago
|
Flags: sec-bounty? → sec-bounty+
Updated•6 years ago
|
Group: toolkit-core-security → core-security-release
Updated•6 years ago
|
Whiteboard: [adv-main60+][adv-esr52.8+]
Updated•6 years ago
|
Flags: qe-verify+
Comment 20•6 years ago
|
||
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.
Updated•6 years ago
|
Alias: CVE-2018-5168
Comment 21•6 years ago
|
||
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)
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•