Rob Wu [:robwu] Assignee
	Description • 6 years ago

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.

	Rob Wu [:robwu] Assignee
	Comment 1 • 6 years ago
str

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

	Rob Wu [:robwu] Assignee
	Comment 2 • 6 years ago

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.

	Rob Wu [:robwu] Assignee
	Comment 3 • 6 years ago

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

	Rob Wu [:robwu] Assignee
	Comment 5 • 6 years ago

Depends on D3772

	Rob Wu [:robwu] Assignee
	Comment 6 • 6 years ago

The relevant code is being refactored by bug 1484373.

	Rob Wu [:robwu] Assignee
	Comment 7 • 6 years ago

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

	Rob Wu [:robwu] Assignee
	Comment 8 • 6 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 6 years ago

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.

	Victor Carciu
	Comment 10 • 6 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 6 years ago

What needs to be done for ESR 60.3 here?

	Rob Wu [:robwu] Assignee
	Comment 12 • 6 years ago

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

	Shane Caraveo (:mixedpuppy)
	Comment 13 • 6 years ago

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.

	Rob Wu [:robwu] Assignee
	Comment 14 • 6 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 6 years ago

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

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

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 6 years ago
uplift

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

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

	Rob Wu [:robwu] Assignee
	Comment 17 • 6 years ago

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.

	Victor Carciu
	Comment 18 • 6 years ago

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