1750565 - (CVE-2022-22754) Extension permission prompts skipped via dictionary

Status: RESOLVED FIXED

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

Description

[Rob Wu [:robwu]]

Extensions should only be able to use powerful extension APIs after users have consented via a permission prompt that lists the requested permissions.
I found that these checks can be bypassed, by getting the user to install an innocent-looking (unsigned) dictionary, and then exploiting the update mechanism to convert it to a (powerful) extension.

This works because of the following implementation details:

• Auto-updates for add-ons are enabled by default
  • auto-update starts in AddonManager.jsm's backgroundUpdateCheck
  • manual update with a similar bug starts at aboutaddons.js's checkForUpdate
• A code path to skip permission prompts for upgrades from legacy extensions to WebExtensions, introduced in bug 1339552:
  • Background updates skips prompts in AddonManager.jsm's _updatePromptHandler
  • Manually triggered updates skips prompts in aboutaddonsCommon.js's installPromptHandler
• The logic that computes permissions for WebExtensions returns null for non-extension WebExtensions, in Extension.jsm's manifestPermissions getter
• At add-on installation/updates, add-ons may freely change addon type
  • Firefox does not check addon.type when an update is loaded, by XPIInstall.jsm's loadManifest method of AddonInstall.
  • While AMO prevents add-ons from changing types, this AMO check can be bypassed if it is possible to get the user to install an add-on that has not been submitted to AMO. That means that the add-on is not signed.
  • Signatures are required (on release) for all add-on types, except the "dictionary" add-on type (because it is not listed in SIGNED_TYPES).

To reproduce this:

1. Create a manifest.json file with the following content. Note that I have picked an arbitrary ID from an add-on hosted on AMO (and Firefox uses AMO to check for updates by default), but I could also have added an update_url that points to a self-hosted update endpoint.

{
    "name": "Dictate crx",
    "version": "0.0.0.1",
    "manifest_version": 2,
    "dictionaries": {},
    "applications": {
        "gecko": {
            "id": "[e-mail]"
        }
    }
}

2. Create a zip file with the name unsigned.xpi containing the above manifest.json file. Note that this is an unsigned add-on.
3. Open the xpi in Firefox. Firefox shows an installation prompt, but no special warnings.
   • Note that Firefox will show an icon from the add-on from AMO, because Firefox fetches additional metadata from AMO by default.
4. Visit about:addons, click on the Gear icon and click on the "Check for updates" menu item.
5. Click on the "Dictionaries" list at about:addons and look for the above add-on, and similarly look at the "Extensions" list in about:addons.

Expected:

• At step 5, the "Dictionaries" list should still show the "Dictate crx" add-on (dictionary).
• Or, after step 4, the update should at least be blocked on a permission prompt.

Actual:

• At step 5, the "Dictionaries" list is empty, and the "Extensions" list shows the extension from AMO.

Comment 1

[Rob Wu [:robwu]]

Attachment: Bug 1750565 - Reject addon type changes in updates

Comment 2

[William Durand [:willdurand]]

(In reply to Rob Wu [:robwu] from comment #0)

While AMO prevents add-ons from changing types, this AMO check can be bypassed if it is possible to get the user to install an add-on that has not been submitted to AMO. That means that the add-on is not signed.
[...]
Signatures are required (on release) for all add-on types, except the "dictionary" add-on type (because it is not listed in SIGNED_TYPES).
[...]
Create a zip file with the name unsigned.xpi containing the above manifest.json file. Note that this is an unsigned add-on.

This only works because the initial add-on is unsigned, right? But because this add-on is a dictionary, nothing prevents a user to install it as signing isn't required? As a separate task, should we sign dictionaries?

Comment 3

[Rob Wu [:robwu]]

(In reply to William Durand [:willdurand] from comment #2)

This only works because the initial add-on is unsigned, right? But because this add-on is a dictionary, nothing prevents a user to install it as signing isn't required?

Yes. There is of course an initial install prompt like any other, but there are no further doorhangers with permission requests.

As a separate task, should we sign dictionaries?

Dictionaries are intentionally not signed, but I cannot find why exactly (https://wiki.mozilla.org/Add-ons/Extension_Signing).
I guess that signing was enforced to guard against (privileged) code execution (with some work being done to reduce the capabilities of legacy dictionaries in bug 1193926).

Comment 4

[Rob Wu [:robwu]]

Attachment: Bug 1750565 - Test that addon types cannot be changed in updates

Comment 5

[Andreas Wagner [:TheOne] [use NI]]

I am not sure we intentionally and actually excluded dictionaries from signing. As far as I can remember, we started with extensions and extended the list from there when we saw the need. I don't have the bugs handy at the moment, but I recall for both themes and langpacks, they were specific use-cases or vulnerabilities that signing solved or mitigated.
IMHO, we should sign all add-on types and consciously exclude only when there is a strong need not to sign, rather than waiting for a need to start signing.

Comment 6

[Rob Wu [:robwu]]

Comment on attachment 9259526 [details]
Bug 1750565 - Reject addon type changes in updates

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easily. A key to exploiting the reported vulnerability is to use the dictionary addon type, which is not obvious from the patch with the fix, nor from the unit test. The unit test tests the theme->extension addon type change, which is not even possible in practice, because both addon types require signing, and AMO rejects submissions of mismatching types.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: ESR91
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause regressions. Switching between add-on types is not supported, and AMO already rejects add-on submissions that switch types.

Comment 7

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9259526 [details]
Bug 1750565 - Reject addon type changes in updates

Approved to land and uplift.

I think it would be good to explore what defensive programming we could add to this area in a followup bug. e.g. besides just signing dictionaries (which would be a longer-term goal), instead of an opt-in SIGNED_TYPES structure, why not an opt-out structure?

Comment 8

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9259539 [details]
Bug 1750565 - Test that addon types cannot be changed in updates

Clearing sec-approval from the test. It can land after March 8th assuming this patches goes out in 97.

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Reject addon type changes in updates r=rpl
https://hg.mozilla.org/integration/autoland/rev/c00713da5d85bb7ea4dfebaef729faf1a7e8dddc
https://hg.mozilla.org/mozilla-central/rev/c00713da5d85

Comment 10

[Rob Wu [:robwu]]

Comment on attachment 9259526 [details]
Bug 1750565 - Reject addon type changes in updates

Beta/Release Uplift Approval Request

• User impact if declined: Add-ons can escalate privileges without the user's consent.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Issue is well understood; Very precise patch that fixes the problem without side effects.
• String changes made/needed:

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Add-ons can escalate privileges without the user's consent.
• Fix Landed on Version: 98 with pending uplift to beta/release
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Issue is well understood; Very precise patch that fixes the problem without side effects.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 12

[Rob Wu [:robwu]]

In the response to the sec-survey: static analysis could not have caught this bug.

"Are you able to summarize what pattern caused this bug?"

Migration logic relied on incorrect assumptions. When stricter security controls were implemented, migration logic was added to silently convert from the previous insecure format for backwards-compatibility. The condition for migration was not specific enough, which allows attackers to craft a sample to abuse this logic to bypass the security controls.

In this reply,

• "security controls" refers to the extension permission system.
• "migration logic ... convert from the previous insecure format" refers to the code path to migrate from legacy extensions to WebExtensions (from bug bug 1339552).
• "condition for migration was not specific enough" refers to the fact that the logic was meant for legacy extensions, but the check tested the presence of permissions in a database, and did not account for the fact that there are add-on types without permissions.
• "craft a sample to abuse this logic to bypass the security controls." refers to the proof of concept from the bug report.

The use of unsigned dictionaries in the exploit has raised questions about this practice (e.g. comment 5, comment 7). While the requirement of signing for most add-on types has somewhat mitigated this bug, it is only because of extra checks on AMO that run before signing. This AMO-side check is the reason that the comparable test case in D136238 is not a working exploit in the wild.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9259526 [details]
Bug 1750565 - Reject addon type changes in updates

Approved for 97.0b6 and 91.6esr.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/236f7fc1af37

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/deb38ff00378

Comment 16

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Test that addon types cannot be changed in updates r=rpl
https://hg.mozilla.org/integration/autoland/rev/e4333eb8c6defd50546c955ca05418ccb5c5d688
https://hg.mozilla.org/mozilla-central/rev/e4333eb8c6de