craftcms / cms

Build bespoke content experiences with Craft.
https://craftcms.com
Other
3.21k stars 619 forks source link

Backend is using `eval` which is incompatible with CSP #3122

Open XhmikosR opened 6 years ago

XhmikosR commented 6 years ago

Description

It seems the backend is loading frabric.js, which is using eval. In order for one to apply a CSP, they need to use 'unsafe-eval which sort of beats the point IMO.

Steps to reproduce

  1. Visit the dashboard with a CSP applied without 'unsafe-eval'
  2. Enjoy the console errors

If you guys want to reproduce this, you can try using @april's laboratory extension and you will see this:

2018-07-20_18-37-41

Additional info

There is an open issue on https://github.com/fabricjs/fabric.js/issues/1621 since 2014.

brandonkelly commented 6 years ago

Are you able to identify where the eval() call is?

XhmikosR commented 6 years ago

@brandonkelly: it's in fabric.js, see the screenshot or the link to fabric.js above :)

brandonkelly commented 6 years ago

@andris-sevcenko if I’m reading the GH issue right, it sounds like this issue is mostly solved in Fabric 2.0, as the Function code was moved into separate modules that aren't included by default and may not be necessary in our case.

Here’s the search results for "new function" in Fabric: https://github.com/fabricjs/fabric.js/search?q=%22new+function%22&unscoped_q=%22new+function%22

So if we can use Fabric 2 without those files we should be able to close this issue.

XhmikosR commented 6 years ago

Alternatively, I was looking into a way to enforce a different CSP for the backend, but I don't think it's possible with .htaccess (yes, I don't want to use htaccess either but I don't have control of it :)).

So, I'd need to do this via PHP in order to separate the frontend and the backend CSPs. So, if we could get rid of this, I might just get away with just a few extra bytes in a common CSP :)

brandonkelly commented 6 years ago

Yeah you should be able to set a custom CSP from a module. If you haven’t made any changes to the module scaffolding in your Craft project yet, you can do it like this:

  1. Uncomment this line from config/app.php:

    //'bootstrap' => ['my-module'],
  2. Add this to your init() method in modules/Module.php:

    if (Craft::$app->request->isCpRequest) {
        Craft::$app->response->headers->add('<HeaderName>', '<HeaderValue>');
    }
XhmikosR commented 6 years ago

Thanks for the help, @brandonkelly! I'm gonna try it on later or Monday.

PS. I wonder which of the two CSPs would take precedence though, if I still have a CSP set in .htaccess. I guess I'll need to try it and see!

EDIT:

Duh, nevermind, I can just exclude the CSP header for admin URLs :)

andris-sevcenko commented 6 years ago

So if we can use Fabric 2 without those files we should be able to close this issue.

Before we can get to that, the entire Asset Image Editor code base would need to accommodate at least two breaking changes that will affect it (removal of fabric.PathGroup and change of image.width/image.height behaviour). After that, it still seems that an instance of new Function is part of every fabric.js build.

It's still proaby worth the update to Fabric.js 2.0, but it'll take some work to get it working.

XhmikosR commented 5 years ago

So, any updates about this?

andris-sevcenko commented 5 years ago

@XhmikosR Fabric.js 2.0 still will include at least one new Function in every build, so not sure what updates we can have until that changes :/

XhmikosR commented 5 years ago

The least you could do is comment https://github.com/fabricjs/fabric.js/issues/1621

Not sure how you guys should proceed, but this needs to be fixed because it's a huge security issue. I have no idea how you are using fabric.js exactly so I can't tell what will break. But it's a fact that allowing eval opens the doors to everything.

nettum commented 4 years ago

+1 on this.
We try to enforce as secure CSP headers as possible, but if we do not add 'unsafe-eval' at least the image editor will break:
image

kbergha commented 5 months ago

Still relevant in 4.7.2 from the login screen.

Uncaught EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'self' 'unsafe-inline'".

    at new Function (<anonymous>)
    at Object.createAccessors (fabric.js?v=1707402833:2:7751)
    at fabric.js?v=1707402833:2:127989
    at fabric.js?v=1707402833:2:128427
bharat-bigscal commented 1 month ago

Using Cradt 3, it's still issue in All Admin pages.