Closed Bug 1539598 Opened 1 year ago Closed 1 year ago

Restrict mozAddonManager's special privileges to calls from the discovery pane

Categories

(Toolkit :: Add-ons Manager, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla68
Tracking Status
firefox-esr60 68+ verified
firefox68 + verified

People

(Reporter: kmag, Assigned: aswan)

References

Details

(Keywords: sec-moderate, Whiteboard: [adv-main68+][adv-esr60.8+])

Attachments

(3 files)

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.

Type: enhancement → defect
Priority: -- → P1
Blocks: 1540173

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.

(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.

(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

Also makes

"we should limit those privileges to usage in the discovery pane ... rather than offering it to AMO in general."

a non-starter.

(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.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a05ac03465cb38ba5a798cb762ccf20a6635f23
Bug 1539598 Require pre-install confirmation on all addons installs from outside about:addons r=kmag
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Backout by opoprus@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/75d0bd84d83f
Backed out changeset 7a05ac03465c for turning bug 1531406 into perma on a CLOSED TREE

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?

Flags: needinfo?(aswan) → needinfo?(nalexander)

(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-44

So 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!

Flags: needinfo?(nalexander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7e2018b550c9e0cb80644b23791e673d152ac3f
Bug 1539598 Require pre-install confirmation on all addons installs from outside about:addons r=kmag
Status: REOPENED → RESOLVED
Closed: 1 year ago1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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.

Flags: needinfo?(aswan)

Manual testing can be pretty simple:

  1. Install a theme from the discovery pane ("Get Extensions" in about:addons), no prompt should be shown before installation
  2. Install a theme from addons.mozilla.org, a prompt should be shown before installation
Flags: needinfo?(aswan)
Attached image Bug1539598.gif

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.

Status: RESOLVED → VERIFIED

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
Attachment #9055647 - Flags: approval-mozilla-esr60?
Whiteboard: [adv-main68+][adv-esr60.8+]

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.

Attachment #9055647 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
No longer blocks: CVE-2019-9811
Alias: CVE-2019-11722
QA Whiteboard: [qa-triaged]
Attached image Bug1539598.gif

This issue is verified as fixed on Firefox 60.8.0esr (20190702220624) under Win 7 64-bit and Mac OS X 10.14.1.

Flags: qe-verify+

Removing CVE and marking it as "burned" for public purposes.

Alias: CVE-2019-11722
You need to log in before you can comment on or make changes to this bug.