Restrict mozAddonManager's special privileges to calls from the discovery pane
Categories
(Toolkit :: Add-ons Manager, defect, P1)
Tracking
()
People
(Reporter: kmag, Assigned: aswan)
References
Details
(Keywords: sec-moderate, Whiteboard: [adv-main68+][adv-esr60.8+])
Attachments
(3 files)
Bug 1539598 Require pre-install confirmation on all addons installs from outside about:addons r=kmag
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-esr60+
|
Details | Review |
547.72 KB,
image/gif
|
Details | |
405.13 KB,
image/gif
|
Details |
mozAddonManager has special powers above those of InstallTrigger to allow one-click installs/enables/disables of add-ons from the discovery pane. Since that functionality isn't meant to be used outside of the discovery pane, we should limit those privileges to usage in the discovery pane, when loaded in about:addons, rather than offering it to AMO in general.
We should also limit it to listed add-ons being installed from AMO, since the discovery pane will never offer unlisted add-ons.
Reporter | ||
Updated•6 years ago
|
Updated•6 years ago
|
Comment 1•6 years ago
|
||
mozAddonManager has special powers above those of InstallTrigger to allow one-click installs/enables/disables of add-ons from the discovery pane. Since that functionality isn't meant to be used outside of the discovery pane
Currently, AMO and the current web-based discovery pane use mozAddonManager
as the main (and only) mechanism to install add-ons. InstallTrigger
is not used anymore since August/September 2018.
Assignee | ||
Comment 2•6 years ago
|
||
(In reply to William Durand [:willdurand] from comment #1)
Currently, AMO and the current web-based discovery pane use
mozAddonManager
as the main (and only) mechanism to install add-ons.InstallTrigger
is not used anymore since August/September 2018.
Not sure what bearing that has on this bug? InstallTrigger is exposed to all web sites and is publicly documented so we can't remove it. This bug is about changing the behavior of mozAddonManager when it is used outside of the disco pane.
Comment 3•6 years ago
|
||
(In reply to Andrew Swan [:aswan] from comment #2)
Not sure what bearing that has on this bug?
Sorry, maybe I misunderstood the original issue but the quote thereafter is not entirely accurate and that's what I meant: AMO and disco pane both have enhanced install buttons with the install/remove/enable/disable features. It's the same button on both apps currently.
mozAddonManager has special powers [...] to allow one-click installs/enables/disables of add-ons from the discovery pane
Comment 4•6 years ago
|
||
Also makes
"we should limit those privileges to usage in the discovery pane ... rather than offering it to AMO in general."
a non-starter.
Reporter | ||
Comment 5•6 years ago
|
||
(In reply to Andrew Williamson [:eviljeff] from comment #4)
Also makes
"we should limit those privileges to usage in the discovery pane ... rather than offering it to AMO in general."
a non-starter.
It is, in fact, a finisher.
Assignee | ||
Comment 6•6 years ago
|
||
Assignee | ||
Comment 7•6 years ago
|
||
Comment 8•6 years ago
|
||
bugherder |
Updated•6 years ago
|
Comment 10•6 years ago
|
||
Backed out changeset 7a05ac03465c (Bug 1539598) for turning Bug 1531406 into perma
Backout: https://hg.mozilla.org/integration/autoland/rev/75d0bd84d83f607fde8ed552a7c38489e848b249
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=238024885&repo=mozilla-inbound&lineNumber=1807
Comment 11•6 years ago
|
||
Assignee | ||
Comment 12•6 years ago
|
||
Nick, can you help me out here? With this patch we now show a confirmation prompt before installing any addons. The existing test of theme installation knows nothing about this prompt so it times out, I could use some help figuring out how to get the test to detect and respond to the prompt.
The prompt is actually launched indirectly through xpcom:
https://searchfox.org/mozilla-central/rev/14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/toolkit/mozapps/extensions/AddonManager.jsm#2575-2584
https://searchfox.org/mozilla-central/rev/14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/mobile/android/components/XPIDialogService.js#16-44
So a terribly hacky solution would just be to override that component for the duration of this test. Can we find something better?
Comment 13•6 years ago
|
||
(In reply to Andrew Swan [:aswan] from comment #12)
Nick, can you help me out here? With this patch we now show a confirmation prompt before installing any addons. The existing test of theme installation knows nothing about this prompt so it times out, I could use some help figuring out how to get the test to detect and respond to the prompt.
The prompt is actually launched indirectly through xpcom:
https://searchfox.org/mozilla-central/rev/14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/toolkit/mozapps/extensions/AddonManager.jsm#2575-2584
https://searchfox.org/mozilla-central/rev/14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/mobile/android/components/XPIDialogService.js#16-44So a terribly hacky solution would just be to override that component for the duration of this test. Can we find something better?
I see no examples of interacting with Gecko prompts in the existing Robocop tests. Looking at PromptService.java it's clear that we could watch for those native messages and do appropriate things in a text context, but this is pointless: the GeckoView API is different and already tested.
And given that this Robocop test is already mostly expressed in JS, I'd do exactly as you say and use XPCOM wizardry to mock out the prompt around https://searchfox.org/mozilla-central/source/mobile/android/tests/browser/robocop/testThemeInstall.js#46.
For review, flag :petru, :vbaicu, or (as a last resort) me. Good luck!
Assignee | ||
Comment 14•6 years ago
|
||
Comment 15•6 years ago
|
||
bugherder |
Comment 16•6 years ago
|
||
Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.
Assignee | ||
Comment 17•6 years ago
|
||
Manual testing can be pretty simple:
- Install a theme from the discovery pane ("Get Extensions" in about:addons), no prompt should be shown before installation
- Install a theme from addons.mozilla.org, a prompt should be shown before installation
Comment 18•6 years ago
|
||
This issue is verified as fixed on Firefox 68.0a1 (20190421212635) under Win 7 64-bit and Mac OS X 10.14.1.
Please see the attached video.
Updated•5 years ago
|
Updated•5 years ago
|
Comment 19•5 years ago
|
||
Comment on attachment 9055647 [details]
Bug 1539598 Require pre-install confirmation on all addons installs from outside about:addons r=kmag
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration: mitigating sec-high/sec-crit sandbox escape paths
- User impact if declined: see above
- Fix Landed on Version: 68
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): We end up prompting in more cases, but it's a targeted patch that applies cleanly, and in theory nothing much besides about:addons should be using mozAddonManager anyway. The main risk here for esr is automated test coverage on an aging branch needing special hand-holding for the change in behaviour.
- String or UUID changes made by this patch: nope
Updated•5 years ago
|
Updated•5 years ago
|
Comment 20•5 years ago
|
||
Comment on attachment 9055647 [details]
Bug 1539598 Require pre-install confirmation on all addons installs from outside about:addons r=kmag
Helps mitigate a sandbox escape. Approved for 60.8esr.
Comment 21•5 years ago
|
||
uplift |
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 22•5 years ago
|
||
This issue is verified as fixed on Firefox 60.8.0esr (20190702220624) under Win 7 64-bit and Mac OS X 10.14.1.
Comment 23•5 years ago
|
||
Removing CVE and marking it as "burned" for public purposes.
Description
•