Kestrel Reporter
	Description • 6 years ago

I noticed an unusual permission warning when first visiting about: pages with an extension installed.

> Error: Permission denied to access property "defaultOptions"  defaults.js:1:1

By looking at the loaded moz-extension:// sources in Browser Toolbox I noticed that content scripts were being loaded as background scripts on visiting an about: page. It only happens with manifests that have both "match_about_blank" and "run_at" "document_start". "matches" does not seem to make any difference. The about: page must be visited for the first time in that window and it does not happen with about:blank.

I modified the manifest of the Borderify example extension and attached it as a test case.

STR:
1. Install modified Borderify extension.
2. Open Browser Toolbox, go to Debugger and note moz-extension:// list of sources.
3. Create a new window and visit about:about.
4. Note borderify.js in list of moz-extension:// sources despite it being a content script.

I'm unsure of security implications so marking this as hidden just in case.

	Andrew Swan [:aswan]
	Comment 1 • 6 years ago

Can you explain what you mean by "loaded as background scripts"?  If this is just about how the scripts appear in devtools, this should be moved to a devtools component and made public, its not clear to me if you're reporting something else however...

	Kestrel Reporter
	Comment 2 • 6 years ago

The content script is being loaded in about:blank before the destination about: page is loaded, if the destination is in the main process then the script is also loaded in the main process. This can be observed by putting an infinite loop in the script and monitoring which process shows high CPU usage. Content scripts being loaded in to the main process undermines process sandboxing and the script can interfere with the loading of important about: pages, delaying or redirecting them.

The script still appears to be sandboxed as a content script, limited to WE APIs and CSP is not blocking eval(). It remains in the main process after uninstallation so restarting the browser is usually required between tests.

Attempting to breakpoint the script causes a crash:

> MOZ_RELEASE_ASSERT(js::SavedFrame::isSavedFrameOrWrapperAndNotProto(*stack))

Accessing a variable in another script file produces a permission error (it doesn't happen with very small script files, I had to pad the script with text for this to appear).

> Error: Permission denied to access property "defaultOption" defaults.js:1:1

If it contains browser.storage.onChanged.addListener it produces an error for about:blank pages regardless of which process it gets loaded into:

> Error: WebExtension context not found!  ExtensionParent.jsm:863:13

Attached is an example extension demonstrating the above errors.

	Tomislav Jovanovic :zombie Assignee
	Comment 3 • 6 years ago

I think in general we want to stop loading content pages in the parent process, but until that's true, we can probably do something locally for content scripts.

Hopefully this doesn't impact system addons, though I'm afraid one of PDF.js or reader view is still in the parent process.

	Kris Maglione [:kmag]
	Comment 4 • 6 years ago

(In reply to Tomislav Jovanovic :zombie from comment #3)
> though I'm afraid one of PDF.js or reader view is still in the parent process.

Nope.

	Tomislav Jovanovic :zombie Assignee
	Comment 5 • 6 years ago

IMO this bug doesn't look like sec-crit or sec-high, and Kris agrees.

Given that, I will probably want to push a patch for this to Try, to suss out all the possible corner cases this change might break.  Obviously, I'll find or file an innocent looking bug to disguise as.

Let me know if anyone disagrees or objects.

	Tomislav Jovanovic :zombie Assignee
	Comment 6 • 6 years ago

Daniel, can you formally categorize the bug, and let me know if you have objections to above?

	Daniel Veditz [:dveditz]
	Comment 7 • 6 years ago

Call it a low "sec-moderate" because it's violating our sandbox policies, but for bad things to happen the user has installed a malicious extension (that can already do bad data-stealing things to the user) and the WE has to have an RCE exploit. If that happens then this is a sandbox bypass.

	Tomislav Jovanovic :zombie Assignee
	Comment 9 • 6 years ago

So there are better ways to do this, but they require more intrusive changes (probably across c++ and js), and would be harder to make look innocuous in a public bug.  We can land a better patch after this goes through release/ESR.

Also, I have manually verified this fixes the problem, but can't think of any way to write a test for it. :/

	Tomislav Jovanovic :zombie Assignee
	Comment 10 • 6 years ago

Got r+, filed public bug 1389241, and will post the patch there in a bit.

	Tomislav Jovanovic :zombie Assignee
	Comment 11 • 6 years ago

err, filed bug 1496532.

	Tomislav Jovanovic :zombie Assignee
	Comment 12 • 6 years ago

Resolving, this was fixed in bug 1496532 (and followup bug 1498311).

Hey Ryan, I don't think we we're uplifting "low sec-moderate" to ESR, right?  Anyway, this fix wouldn't even apply across all platforms since we only had OOP extensions on Windows in Firefox 60.

Also, we probably want to QA to verify this (they already asked in the public bugs).

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

We have some leeway for taking sec-moderate patches on ESR60 (note that ESR52 is already EOL, so we wouldn't be looking at any older releases), largely a matter of balancing the severity of the bug, the riskiness of the patch, and the long-term cost of leaving it unfixed on that branch (since ESR60 isn't EOL for roughly another year).

If in doubt, I'd run it past dveditz and see what he thinks. Thanks for the ping!

	Victor Carciu
	Comment 14 • 6 years ago

Verified as fixed using Firefox 64(buildid 20181105164654) on Winsows 10x64 and macOS 10.13.6.

I also confirmed that issue was reproducible on earlier FF versions.

	Tomislav Jovanovic :zombie Assignee
	Comment 15 • 6 years ago

Thanks Victor!

Daniel, would you insist in uplifting this to ESR?  I don't recommend it because:

1) it's low sec-moderate,
2) this fix is only possible on Windows for ESR (we didn't have OOP extensions on other platforms in 60),
2) and it's likely to break something unknown / that doesn't live in m-c / something enterprises might be doing.

This is generally a major change for some corner cases, and IMO somewhat risky, which is why I originally opted to do it in a public bug 1496532 (and it did evidently break on unexpected stuff even in m-c).

	Daniel Veditz [:dveditz]
	Comment 16 • 6 years ago

Thanks for asking -- those are all good reasons not to take this fix on ESR.