1752979 - (CVE-2022-26387) TOCTOU flaw when verifying addon signatures: actual install might have different content

Status: VERIFIED FIXED

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

Description

[Daniel Veditz [:dveditz]]

The primary flaw of bug 1752888 is a TOCTOU violation when verifying add-on signatures. We initially validate the signature and read enough metadata to show the install permission prompt, but apparently release the file at that point and are none-the-wiser if it gets swapped out with a tampered version before we install it.

This appears to be especially problematic when installing from a local file because we do this right in-place, where the triggering content knows exactly where it is. For add-ons installed from the web we download it to a temp file somewhere to process. It still has the TOCTOU problem, but it's harder to find.

Comment 1

[Rob Wu [:robwu]]

While bug 1752888 shows a PoC for local installations, the issue also applies to non-local installs. PoC description: Extensions can realistically exploit this if the user chooses the tempdir as the download destination. The extension API offers the ability to overwrite files within the download directory. It is also possible to determine the random file name (e.g. bug 1598946, or simply by blindly writing up to 47k files because there are only 36 * 36 * 36 possibilities for the file name). Together, it is possible for extensions to hijack arbitrary extension installs and updates (and escalate privileges by hijacking the installation of a privileged add-on).

I am going to fix this bug by verifying the xpi content right before it's being installed. That is, before actually installing the add-on in its final location, but after copying the xpi file from its original location (local file for local installs, a temp file for non-local installs) to a staging directory (between stageInstall and installAddon).

I have considered but decided against the following alternatives:

• Keep the downloaded data in memory.
  • Issues: Implementation complexity and increased memory usage, for a potentially long while in case of updates in the background.
• Copy the local file to a temporary location like we do for web installations.
  • Issues: while it fixes privilege escalation via local installs (bug 1752888), it does not fix the issue for web installs (explained above).
• Copy the local file to the staging directory before installing.
  • Issues: Implementation complexity due to the re-use of the staging directory as a temp dir, e.g. ensuring that temp files are not mistakenly interpreted as an actually staged installation. Still susceptible to TOCTOU if the xpi in the staging directory is overwritten. Furthermore, there is also a separate issue with lingering files (bug 1753110), so taking this approach will cause the profile directory to accumulate junk over time (especially if we use this approach not only for local installs, but also web installs).

Comment 2

[Rob Wu [:robwu]]

Attachment: Bug 1752979 - Check damaged files

Comment 3

[Rob Wu [:robwu]]

Attachment: Bug 1752979 - Tests for file changes during addon installs

Comment 4

[Rob Wu [:robwu]]

Comment on attachment 9261987 [details]
Bug 1752979 - Check damaged files

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easily. From the patch it is clear that there is a possibility of a content mismatch (which is a security problem), but it is not immediately obvious what the impact is. A skilled and motivated attacker may eventually be able to figure out that there is a TOCTOU in the install flow that could be used to bypass security checks. This patch on its own does not offer enough information to construct a full exploit (other gadgets are needed).
• 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?: All, including release and ESR
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: There is a rebase conflict due to a refactor that replaced OS.File with IOUtils (bug 1733540). Either approach is fine (IOUtils.copy already exists since Firefox 81), and I locally confirmed that the new test still passes with the patches applied on top of the ESR91 branch.
• How likely is this patch to cause regressions; how much testing does it need?: There is a very small risk of regressions. The functionality is covered by unit tests (both old and new ones). The most significant change of the patch is the verification of the file hash immediately before an add-on is installed in the profile. This builds on top of existing optional content verification logic that is already reached in practice (by all installations off AMO), which gives great confidence that the hash can be assumed to be reliable. I have also audited relevant code paths to minimize the chance of regressions.

Comment 5

[Andrew McCreight [:mccr8]]

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 9261987 [details]
Bug 1752979 - Check damaged files

sec-approval=dveditz.

DO NOT land the companion test patch until after we ship the fix.

Comment 7

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

Landed: https://hg.mozilla.org/integration/autoland/rev/c4339dd5c34f8b56fa479d303ce54062b43851a0

Backed out for causing multiple mochitest failures:
https://hg.mozilla.org/integration/autoland/rev/0d1ebe4f9d6eec169c1c1da3582fdcf88eed547f

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=c4339dd5c34f8b56fa479d303ce54062b43851a0

Mochitest failures
Telemetry failures

Comment 8

[Rob Wu [:robwu]]

I believe that the failure (NS_ERROR_FILE_IS_LOCKED) is caused by the file still being open after reading the hash.
I re-used the existing logic from https://searchfox.org/mozilla-central/rev/8b46752d1e583b2f817c451f93ba515fb865554d/toolkit/mozapps/extensions/internal/XPIInstall.jsm#2036-2038, but it looks like that was already missing a fis.close() call before. I've updated the patch, will run it on Windows and then retry.

Comment 9

[Rob Wu [:robwu]]

Comment on attachment 9261987 [details]
Bug 1752979 - Check damaged files

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Users would be vulnerable to privilege escalation.
• Fix Landed on Version: 98. This should land on ESR91 when 98 enters release.
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch blocks unsupported (i.e. vulnerable) behavior and is covered by unit tests.

Comment 10

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

Check damaged files r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/a82e15f5c31956ffccce3fd7a4e824e6083c586c
https://hg.mozilla.org/mozilla-central/rev/a82e15f5c319

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

[:Gijs (he/him)]

Rob, could you confirm how QA can verify this? Would the following steps work?

1. save the rally XPI file and another XPI file locally
2. drag/drop the rally XPI file to the tabstrip
3. while the install prompt is up in Firefox, move the other XPI file into the place of the rally XPI file
4. accept the install prompt in Firefox
5. the xpi file shouldn't be installed.

Comment 13

[Alex Cornestean]

Hello @Gijs

I’ve attempted to verify the fix using the STR from Comment 12 however I’ve come across a small problem, in the sense that the STR don’t seem to be valid or I did not understand them correctly.

To verify the fix I’ve done the following, according to the STR:

1. downloaded the latest Mozilla Rally and uBlock Origin .xpis from AMO and put them in 2 separate folders on my machine
2. dragged and dropped the Mozilla Rally .xpi onto the browser window to install it. The install panel is displayed. I do NOT click the “Add” button yet.
3. I move the uBlock Origin .xpi from its folder to the one where the Mozilla Rally .xpi is located. Also, I move the Mozilla Rally .xpi from its original folder to the one where uBlock used to be in. So I swapped the .xpis locations.
4. I click “Add” on the install panel in the browser
5. A panel with “Mozilla Rally could not be installed because Firefox cannot modify the needed file.” is displayed and the add-on install does not proceed.

This behavior occurs on the latest Beta (98.0b4/20220213185901) and Nightly (99.0a1/20220213214259), however it occurs on the latest Release (97.0/20220202182137) as well, where the fix should not have landed. Tested on Windows 10 x64 for the moment.

Would you be able to clarify whether what I’m doing is correct as per your suggestion in Comment 12? Or if the provided STR are not valid to verify this?

Thank you !

Comment 14

[Rob Wu [:robwu]]

Attachment: not-ublock.xpi

STR:

1. Download uBlock Origin's XPI file, e.g. saved as addon.xpi in your Downloads directory.
2. Open the XPI file in Firefox, e.g. by drag & drop, or by opening the file:-URL of the xpi file.
3. When the permission prompt appears, replace the addon.xpi from step 1 with the attached file (rename not-ublock.xpi to addon.xpi).
4. Confirm the installation in Firefox.
5. Open the console and look at the output.

Expected:

• The extension installation should not happen.
• With the fix, step 4 will trigger a doorhanger with the message "... could not be installed because Firefox cannot modify the needed file",
• With the fix, step 5 shows "Failed to install /path/to/addon.xpi from file:///path/to/addon.xpi to ...: Refusing to stage add-on because it has been damaged"

Actual (before the fix):

• The installation continued with the modified xpi file (not-ublock.xpi). The console contains "=== LOG === This is not uBlock Origin === LOG ==="

Comment 15

[Alex Cornestean]

Hello and thank you @Rob for the STR !

I managed to verify the fix based on the STR from Comment 14 on the latest Nightly (99.0a1/20220214213941) and Beta (98.0b4/20220213185901) under Windows 10 x64 and Ubuntu 16.04 LTS.

Proceeding with the STR, on the latest Nightly and Beta, the following occur as expected:

• the extension installation does not happen
• a doorhanger stating that “... could not be installed because Firefox cannot modify the needed file" is displayed
• the console logs "Failed to install /path/to/addon.xpi from file:///path/to/addon.xpi to ...: Refusing to stage add-on because it has been damaged"
  , confirming the fix.

Also tested against the latest Release ( 97.0/20220202182137) where the fix is not present, and as per the above “Actual (before the fix)”, the installation of the dummy extensions continues and the console logs “=== LOG === This is not uBlock Origin === LOG ===”.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9261987 [details]
Bug 1752979 - Check damaged files

Approved for 91.7esr.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/537e1fca9a43

Comment 18

[Alex Cornestean]

Hello,

I verified the fix based on the STR from Comment 14 on the latest ESR (91.7.0esr/20220228225400) under Windows 10 x64 and Ubuntu 16.04 LTS.

Proceeding with the STR, on the latest ESR, the following occur as expected:

• the extension installation does not happen
• a doorhanger stating that “... could not be installed because Firefox cannot modify the needed file" is displayed
• the console logs "Failed to install /path/to/addon.xpi from file:///path/to/addon.xpi to ...: Refusing to stage add-on because it has been damaged"
  , confirming the fix.

Comment 19

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 20

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

Tests for file changes during addon installs r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/d8df5666ec7dab641fc5f321fac71d63229ca867
https://hg.mozilla.org/mozilla-central/rev/d8df5666ec7d