1587336 - Consider disallowing data: URLs for dynamic imports in content-scripts

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P2)

Description

[Frederik Braun [:freddy]]

A lot of extensions are using eval, which is a bad idea. We block this through a CSP, except in content-scripts.
AFAIU we feel uncomfortable blocking eval in content scripts, due to the prevalence of eval in existing add-ons and widely used frontend frameworks.

But import('data:text/javascript,'+sourcecode) is equivalent to eval and much less popular. So I'd like to explore the idea of disallowing dynamic import of data URLs in content-scripts before there is adoption.

The idea is to add a check in the implementation of import such that we check the current global (e.g., whether we are in a content-script) and throw for resources starting with 'data'.

I'm filing this as a security bug, not to alert malicious add-on authors. But there's no immediate risk here (as reflected in the sec-other security rating).

Comment 1

[Frederik Braun [:freddy]]

TIL we don't actually support dynamic imports. See https://bugzilla.mozilla.org/show_bug.cgi?id=1536094.
Setting this one here as a blocker, so we make a decision before we implement.

Comment 2

[Rob Wu [:robwu]]

A lot of extensions are using eval, which is a bad idea. We block this through a CSP, except in content-scripts.

FYI there is work in progress to block eval by default in content scripts as well, per bug 1323630 .

If we support dynamic imports in content scripts, we're likely not going to support data:-URLs (only files from the extension package).

Comment 3

[Daniel Veditz [:dveditz]]

there's no immediate risk here (as reflected in the sec-other security rating).

for an enhancement that could improve security sec-want is preferable (more specific than "other").

Comment 4

[Shane Caraveo (:mixedpuppy)]

I don't see any work in progress on bug 1323630 (conversation ended 2 years ago). However, with Bug 1581608 for manifest v3, eval might be (probably be) blocked by default. Extensions using eval would have to specify a csp to allow that in the manifest, making them easy to identify.

Comment 5

[Rob Wu [:robwu]]

Since the feature is not implemented yet, the concern expressed here is not a security bug.

I do believe that when we ship dynamic imports in content scripts, that we should only allow URLs with the moz-extension scheme, which excludes data:-URLs.

Comment 6

[Tomislav Jovanovic :zombie]

Agree with Rob, it looks like Freddy filed as a security bug before realizing this is not implemented yet, so I'll open it up.

Comment 7

[Tom S [:evilpie]]

I do believe that when we ship dynamic imports in content scripts, that we should only allow URLs with the moz-extension scheme, which excludes data:-URLs.

This seems like a valid approach to me. This would also remove some concerns about WebExtension content-script importing modules used by the webpage and running into potential global/compartment mismatches.

Comment 8

[Tom S [:evilpie]]

The patch in bug 1536094 disallows loading non moz-extension URLs.