Some functions blocked by CSP unless 'unsafe-eval' is allowed

[j-dobr]

problem

There are few places where JS is evaluating strings and Content Security Policy blocks it unless 'unsafe-eval' is allowed - which is, obviously, unsafe.

I found some new Function( "event", funcStr ) when some of these were blocked by CSP in my case, but there is also one usage of eval().

demonstration

Official X3DOM example with added CSP header demonstrating the problem: 📦 HelloX3DOM.zip Open in browser and hover mouse over canvas to see error appearing in dev-tools issues.

[andreasplesch]

Thanks. Here is a live demonstration page:

https://gist.githack.com/andreasplesch/a6573c6b3fd2f54e49b0e00efd65766b/raw/23a5d20e8e6008b4a2c9f15b98af759f05585ac1/x3dom-csp.html

In chrome Version 101.0.4951.64 I could replicate the problem. The function which are deemed are unsafe are indeed used in a few places.

What are the recommended fixes ? Perhaps it makes sense to start to list the lines where new Function() and eval() is used:

eval:

https://github.com/x3dom/x3dom/blob/83ce36310eda365a35ece8391e9a8c3f78800c38/src/nodes/Grouping/Scene.js#L111

shadowIdMap is rarely used. Looks like improper json parsing. Could be probably fixed.

new Function:

https://github.com/x3dom/x3dom/blob/136648008beca63b9f0e8c4bd38f7ec20b3249f3/src/nodes/Networking/Inline.js#L195

call event handler function provided in element on... attribute. Is there another way to do that ? Disallowing classic attribute event handlers would break a lot, and is very unfriendly. (But csp is targeting disallowing such event handlers. User code can already use .addEventListener - which is intercepted - to handler "load" event. See https://raw.githack.com/x3dom/x3dom/136648008beca63b9f0e8c4bd38f7ec20b3249f3/test/regression-suite/test/cases/inlineEvents/inlineEvents.html)

https://github.com/x3dom/x3dom/blob/47610df99d1e2d46649c0a5e64b5aab6a21066a7/src/nodes/Core/X3DNode.js#L636

same (instead of on.. attribute it seems it is possible to add an onevent property to the dom node, which has the function as an actual function, not string. This seems like an acceptable solution).

https://github.com/x3dom/x3dom/blob/83ce36310eda365a35ece8391e9a8c3f78800c38/src/X3DCanvas.js#L1117

same (downloadsfinished is also fired as an actual dom event, so addEventListener can already be used.)

https://github.com/x3dom/x3dom/blob/83ce36310eda365a35ece8391e9a8c3f78800c38/src/Viewarea.js#L1149

same (the one listed in dev-tools issue). (important since it deals with "onclick"; dom.onclick property with function value should also work already; perhaps convert a few tests to demonstrate).

https://github.com/x3dom/x3dom/blob/83ce36310eda365a35ece8391e9a8c3f78800c38/src/nodes/Shaders/ComposedShader.js#L115

could be probably replaced going through all addField_ type options.

(jQuery seems to be using new Function(), only used in tests)

Any ideas, feedback or fixes appreciated.

In summary, the json parsing should be fixed. An Inline load event handler can be added via .addEventListener.

So try to use the .onevent properties instead of the onevent attributes, or the .addEventListener method. Most functionality is then available.

[andreasplesch]

I could replace the eval() and Function() for ComposedShader. The others could stay blocked since there are better ways to get the functionality. The dev-tools issue listing is appropriate. Could you please put together a CSP example which actually uses some blocked functionality (for example onclick and inline onload) using .onevent property handlers or .addEventListener to demonstrate how to properly use x3dom with CSP 'unsafe-eval' ? That would be grand.

[andreasplesch]

Here is the demo page modified to show how to use CSP with x3dom: https://gist.githack.com/andreasplesch/a6573c6b3fd2f54e49b0e00efd65766b/raw/4be48eabac4660dcedf1ed19b1ebf967882d25ed/x3dom-csp.html

Please feel free to reopen whenever appropriate.

[j-dobr]

Thanks for fixes, i understand it is not possible to make breaking changes.

The problem with error originally in question is that it is triggered by default only by hovering mouse over canvas, without even using that handler. JS is trying to create function anyway. To prevent this (if i undestand that correctly) it would be needed to create own blank handler the "right" way, thus preventing JS from trying to create it from attribute.

Having it blocked is not a problem for client since it is not used (defined) anyway, but i use Reporting API and these CSP violations are reported - every time someone hovers mouse over canvas...

I suggest to check if attribute to create handler from exists in the first place and skip whole function creation if not. That would prevent this default CSP violating behavior. If someone opts-in to use attributes to create handlers, then it is their choice to use "unsafe-eval" with CSP.

Patch fixing this: 1214-check-defined-attribute.zip

[andreasplesch]

Ok, makes sense. Here is the patch:

diff --git a/src/Viewarea.js b/src/Viewarea.js
index f1db6e8d9..98a920ef7 100755
--- a/src/Viewarea.js
+++ b/src/Viewarea.js
@@ -1143,7 +1143,7 @@ x3dom.Viewarea.prototype.callEvtHandler = function ( node, eventType, event )
         {
             attrib.call( node._xmlNode, event );
         }
-        else
+        else if ( node._xmlNode.hasAttribute( eventType ) )
         {
             var funcStr = node._xmlNode.getAttribute( eventType );
             var func = new Function( "event", funcStr );

It may make sense to have the same guard for the other uses of new Function. Let's check.

[j-dobr]

Here is the patch to add attribute check on all 3 places where it is missing (including the one in previous patch):

diff --git a/src/Viewarea.js b/src/Viewarea.js
index f1db6e8d9..98a920ef7 100755
--- a/src/Viewarea.js
+++ b/src/Viewarea.js
@@ -1143,7 +1143,7 @@ x3dom.Viewarea.prototype.callEvtHandler = function ( node, eventType, event )
         {
             attrib.call( node._xmlNode, event );
         }
-        else
+        else if ( node._xmlNode.hasAttribute( eventType ) )
         {
             var funcStr = node._xmlNode.getAttribute( eventType );
             var func = new Function( "event", funcStr );
diff --git a/src/nodes/Core/X3DNode.js b/src/nodes/Core/X3DNode.js
index d265b539e..1d82797e8 100644
--- a/src/nodes/Core/X3DNode.js
+++ b/src/nodes/Core/X3DNode.js
@@ -629,7 +629,7 @@ x3dom.registerNodeType(
                     {
                         attrib.call( node._xmlNode, event );
                     }
-                    else
+                    else if ( node._xmlNode.hasAttribute( eventType ) )
                     {
                         var funcStr = node._xmlNode.getAttribute( eventType );
                         var func = new Function( "event", funcStr );
diff --git a/src/nodes/Networking/Inline.js b/src/nodes/Networking/Inline.js
index d800ff2ba..92c522c5e 100644
--- a/src/nodes/Networking/Inline.js
+++ b/src/nodes/Networking/Inline.js
@@ -189,7 +189,7 @@ x3dom.registerNodeType(
                         {
                             attrib.call( this._xmlNode, event );
                         }
-                        else
+                        else if ( node._xmlNode.hasAttribute( "on" + eventType ) )
                         {
                             var funcStr = this._xmlNode.getAttribute( "on" + eventType );
                             var func = new Function( "event", funcStr );

One other usage was already changed by #1215 and one usage already checks the attribute.

[andreasplesch]

Please feel free to reopen.