Mapbox-gl-js: Remove all uses of "new Function" and "eval"

[jfirebaugh]

Mapbox GL JS is included as part of applications, and CSP is a very effective security measure. It would be great if we can enable people who build those applications to use all reasonable CSP rules. Right now we use the Function form of eval via 2 downstream modules.

filter-function upstream in feature-filter

The bucket filter evaluator uses new Function() (a form of eval) which is prohibited under Content Security Policy. We should switch to a non-eval'ing interpreter.

web workers

webworkify also uses the Function constructor to bundle webworker sources in the bundle.

[mourner]

Note that this is a hot function, it should be really fast. Are you seeing a non-eval approach that is not considerably slower? I'd drop CSP environment support for now instead.

[jfirebaugh]

I haven't tried anything out yet, just wanted to flag this.

[robertberry]

Is there any chance this will be fixed? Many organizations, including the one I work for, operate with a strict CSP, and if Mapbox is introducing potential security holes through forcing us to make our CSP more lax, it's unlikely we can consider using it.

[lucaswoj]

It is unlikely that we will resolve the issues outlined in this ticket soon. Eval, via new Function(), has been critical to making mapbox-gl-js performant.

[robertberry]

Might be worthwhile adding somewhere on the website what CSP rules you need to be able to use MapBox. I had to figure this out by trial and error.

FYI they are

connect-src https://api.mapbox.com https://*.tiles.mapbox.com data: blob:;
child-src data: blob:;

The nasty ones in question here are the 'data: blob:' ones, which are required for you to use new Function(). This means arbitrary code loaded from other websites can be executed, opening your site to cross site scripting attacks.

[mourner]

This means arbitrary code loaded from other websites can be executed, opening your site to cross site scripting attacks.

Can you demonstrate this in practice?

[lucaswoj]

CSP allows a developer to completely disable the new Function() and eval APIs (via the data: blob: rules). @robertberry isn't indicating that he knows of an exploit, just that he wants to follow CSP best practice by disabling those APIs.

[scothis]

The nasty ones in question here are the 'data: blob:' ones, which are required for you to use new Function(). This means arbitrary code loaded from other websites can be executed, opening your site to cross site scripting attacks.

You're correct that this would increase the attack surface area, however, the code needs to get to the page somehow and trick existing code on the page to eval it. Limiting the script-src, connect-src and other directives to trusted domains will prevent a malicious script that finds a way to execute from communicating with a malicious server.

CSP is about defense in depth. Without a CSP policy, or with a wide-open CSP policy, the browser's default safeguards are still protecting your site. Mapbox Studio uses a fairly restrictive CSP, you can view the source of https://www.mapbox.com/studio (it's a meta tag) to see what hosts we've white-listed.

As always, it's up to you to decide how much risk is acceptable for your use case.

[jfirebaugh]

I believe the non-'self' directives required by mapbox-gl-js are:

child-src blob: ;
img-src data: blob: ;
script-src 'unsafe-eval' ;

Plus whatever connect-src directives you need for the resources used by your style. If you're using a Mapbox hosted style, that's connect-src https://*.tiles.mapbox.com https://api.mapbox.com.

@scothis, please correct me if I'm wrong here.

child-src blob: is for web worker initialization. img-src data: blob: is for raster tiles. script-src 'unsafe-eval' is for fast filter evaluation.

It's true that child-src blob: and script-src 'unsafe-eval' are weaker from a security perspective than a policy that omits those directives, although the risk is limited (there would need to be a vulnerability in the code relating to filters or web workers specifically, not a typical XSS vulnerability). We'd like to find a technical solution that doesn't require them, but are not aware of any viable ones. We're not aware of any substantial risks associated with img-src data: blob:.

[blq]

FWIW, these are the CSP errors I'm getting now using Chrome 51.0.2704.84, Mapbox GL v0.20.0 and loading scripts from https://api.tiles.mapbox.com/mapbox-gl-js Has worked previously.

(not my expertise, feel free to delete this if I'm not adding anything new to this thread)

[lucaswoj]

We have determined that removing the remaining uses of new Function would necessarily incur an unacceptable performance penalty. I'm open to the idea of a CSP-safe flag for users who understand and accept this penalty.

[mourner]

Just a note for future: we can get rid of new Function in webworkify, it's not really necessary there. (Update: https://github.com/substack/webworkify/pull/31)

[anandthakker]

StructArray also uses new Function to generate emplaceBack

[winstonhowessc]

Is there any update on the CSP-safe version?

Having this CSP requirement is definitely a strain for our organization. It's not that there is necessarily a vulnerability in mapbox itself, but any page which wants to use mapbox, doesn't have the CPS's guarantee that certain malicious content won't run. So if there's a vulnerability in any code on that page (mapbox or not) it's difficult to use the CSP to detect/prevent it.

[anandthakker]

@winstonhowessc (and others), besides 'unsafe-eval', are any of the other CSP directives that Mapbox GL JS requires problematic?

• worker-src blob:
• img-src data: blob:
• connect-src https://*.tiles.mapbox.com https://api.mapbox.com

[winstonhowessc]

worker-src blob: is a little problematic, but much less so than unsafe-eval.