1483602 - (CVE-2018-12396) Extensions can run content scripts anywhere when the document navigates during content script execution

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P1)

Description

[Rob Wu [:robwu]]

There are multiple TOCOU bugs in extension the scheduler of extension content scripts that allow extensions to run content scripts in principals that it should not have access to.


TOCTOU bug 1:
When an extension is just installed, or when a content process starts up,
injectExtensionScripts is called to run content scripts in existing pages.
https://searchfox.org/mozilla-central/rev/2466b82b/toolkit/components/extensions/extension-process-script.js#202-221

It first checks matchesWindow and then runs scripts in three batches, document_start, document_end and document_idle. There is no guarantee that the inner window has not changed.

TOCTOU bug 2:
When a new document is loaded, ExtensionPolicyService::CheckContentScripts checks whether a script should run via script->Matches(aDocInfo), and then loads a script via loadContentScript:
https://searchfox.org/mozilla-central/rev/2466b82b/toolkit/components/extensions/ExtensionPolicyService.cpp#338-354
https://searchfox.org/mozilla-central/rev/2466b82b/toolkit/components/extensions/extension-process-script.js#499-503

The problem is that aDocInfo has a fixed value, while the document is not guaranteed to have a constant value.

TOCTOU bug 3:
When a dynamic content script is executed (via "Extension:Execute" message), there is no check that the extension is still valid (i.e. not disabled or reloaded). Consequently, it is possible for a dynamic content script to execute after having uninstalled an add-on.
https://searchfox.org/mozilla-central/rev/2466b82b/toolkit/components/extensions/ExtensionContent.jsm#865-876

In short:
- TOCTOU bug 1 and 2 allows extensions to escalate privileges.
- TOCTOU 3 allows extensions to run code after an extension is disabled (any of these bug do).


Bug 1 was introduced in Firefox 57 by https://hg.mozilla.org/mozilla-central/rev/ce17d1d232f1
Bug 2 was introduced in Firefox 55 by https://hg.mozilla.org/mozilla-central/rev/32a3b7c39207
ESR 52 is not affected (tested with 52.9), ESR 60 and current release branches are affected.

Comment 1

[Rob Wu [:robwu]]

Attachment: toctou-scripts.zip

STR:
1. Load attached extension at about:debugging.
2. Visit example.com
3. A dialog appears. If the dialog is not automatically closed after the navigation completes, close the dialog.
4. Wait until the new navigation (addons.mozilla.org) has finished.

Expected:
= No additional dialogs.

Actual:
- Dialog with: "Managed to run script at https://addons.mozilla.org/en-US/firefox/


If you don't see the actual result, try navigating back to example.com (and refresh the page).

Comment 2

[Rob Wu [:robwu]]

The first paragraph of the bug looks unintelligible. It should read:

> There are multiple time of check to time to use (TOCTOU) bugs in the scheduler of extension content scripts that allow an extension to run content scripts in principals that it should not have access to.

Comment 3

[Rob Wu [:robwu]]

I'm going to address the third bug as part of bug 1346941, because it has no significant security impact* and the relevant code needs to be refactored anyway.

* The third part of the bug is a race condition that allows extensions to run content scripts after an extension has shut down, but only in pages where extensions already had the permission to do so. Extensions already have this capability (they can simply inject a script in a web page).

Comment 4

[Rob Wu [:robwu]]

Attachment: Bug 1483602 - Skip unnecessary content scripts

Comment 5

[Rob Wu [:robwu]]

Attachment: Bug 1483602 - Add tests that interrupt content script execution

Depends on D3772

Comment 6

[Rob Wu [:robwu]]

The relevant code is being refactored by bug 1484373.

Comment 7

[Rob Wu [:robwu]]

This has been fixed on trunk by bug 1484373:
- "TOCTOU bug 1" fixed by https://hg.mozilla.org/mozilla-central/rev/a7848dcb339e
- "TOCTOU bug 2" fixed by https://hg.mozilla.org/mozilla-central/rev/eef3785a6b44
  (this second fix still leaves open one (non-security?) bug though: The second loop uses aDocInfo.GetWindow() again even though the inner window might have changed.).

Comment 8

[Rob Wu [:robwu]]

Verified fixed in Firefox 63, buildID 20180827220123 .

I want to uplift a fix to Beta and ESR because the bug is easy to exploit and allows extensions to escalate privileges (and even run code on AMO).

The changes from bug 1484373 are however too large for uplifting, so try a minimal fix such as comment 4.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Note that we're already build Fx62 release candidates, so this is going to have to ship with Fx63 and ESR 60.3 in the next cycle.

Comment 10

[Victor Carciu]

Verified as fixed in Firefox 63.0b3,Build ID 20180904170936, using Windows10x64 and macOS 10.13.6.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

What needs to be done for ESR 60.3 here?

Comment 12

[Rob Wu [:robwu]]

I've rebased https://phabricator.services.mozilla.com/D3772 to the ESR60 branch and requested review again.

Comment 13

[Shane Caraveo (:mixedpuppy)]

esr60 will need manual qa using the extension.  I've verified the fix with the extension on esr60.  The test patch will only apply and work on nightly.

Comment 14

[Rob Wu [:robwu]]

Comment on attachment 9002424 [details]
Bug 1483602 - Skip unnecessary content scripts

r=mixedpuppy at https://phabricator.services.mozilla.com/D3772 , but this r+ is somehow not mirrored here.

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: Privilege escalation in extensions; extensions can run code on any website.

User impact if declined: Extensions with minimal permissions can run JavaScript code on any website, even privileged websites such as addons.mozilla.org .

Fix Landed on Version: 63

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The cpp changes from the patch have already landed in 63 in bug 1484373. The JS-specific part has not, but its correctness was verified using the automated test in 63.

String or UUID changes made by this patch: none

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9002424 [details]
Bug 1483602 - Skip unnecessary content scripts

Thanks for the rebase and testing. Approved for ESR 60.3.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/f27145bd5502

Rob, how/where are you planning to land the tests?

Comment 17

[Rob Wu [:robwu]]

After a few releases (if at all). I don't want to put users who haven't updated yet at risk, and the tests show that a nested event loop is all that it takes to exploit this vulnerability.

Comment 18

[Victor Carciu]

Verified as fixed using macOS with FF 60.2.3esr(buildid 20181012094822) and Win10x64 with FF 60.2.3esr(buildid  	20181012094822)

Comment 19

[Shane Caraveo (:mixedpuppy)]

Rob, the test patch here never progressed. Any status on this?

Comment 20

[Rob Wu [:robwu]]

I planned to update the comments, rebase it and land it when this bug becomes public.

Comment 21

[Shane Caraveo (:mixedpuppy)]

It's a year since this landed, what is the process to close this out?

Comment 22

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

Bugbug thinks this bug is a regression, but please revert this change in case of error.