[BUG] Issue with eval usage in codebase

[NicolasCARPi]

Hello everyone 👋

Description

3Dmol.js uses eval() to execute arbitrary strings.

3Dmol.js/src/autoload.ts

Lines 233 to 238 in b038d7b

	//evaluate javascript in the string, if it resolves to a function,
	//call it with the viewer
	/*jshint -W061 */ var runres = eval(viewerdiv.dataset.callback);
	if (typeof (runres) == 'function') {
	runres(glviewer);
	}

This means that if the data-callback attribute is controlled by an attacker, they can execute any JS code they want on the victim computer (the victim loads the page that contains the nefarious attribute, and 3Dmol.js will happily execute it).

This is a (major) security vulnerability in my book, but @dkoes disagrees, so I'll try and argument my point.

I'm skipping the arguments about eval's speed and worsen DX, because that's not the point.

The main problems are:

1. the mere presence of this eval() call that will be blocked by most Content Security Policy, unless unsafe-eval is set but this is clearly not a possibility for any web application serious about security. This means that the application will not work properly in this context, and code should be modified to avoid the use of eval() completely.

2. It is entirely possible for an attacker to have control of the data attributes, you cannot know how apps can behave and as such should not dismiss this problem by assuming only the dev can modify data attributes of elements. A first bug/vuln could also allow an attacker to get this access, and then they use this problematic eval() in 3dmol code to inject the payload to the visitor of the infected page.

Further points:

Simply look at this MDN page for eval

Does it look like something that should exist in a codebase? I'd say no. If a big red banner on MDN website is not enough, feel free to read more about it:

https://javascript.plainenglish.io/javascript-eval-is-it-evil-a8cd935d0daa
https://security.stackexchange.com/questions/94017/what-are-the-security-issues-with-eval-in-javascript
https://www.codiga.io/blog/javascript-eval-best-practices/
https://www.digitalocean.com/community/tutorials/js-eval
https://medium.com/@eric_lum/the-dangerous-world-of-javascripts-eval-and-encoded-strings-96fd902af2bd

...

I mean there are thousands of articles out there explaining why it is bad.

Fortunately, its usage is pretty limited in 3dmol, so it shouldn't cause too much troubles to address this issue.

To Reproduce

<div class='viewer_3Dmoljs data-callback='alert("arbitrary code execution")'></div>

Expected behavior

Arbitrary javascript code present in HTML must not be executed by a library present on the page.

Concluding remarks

I hope you will give sufficient interest towards this issue and consider the workarounds I suggested in my email or think about a way to still keep a similar feature but without this problematic eval usage.

Also, I couldn't find any documentation about this feature so I'm not sure it is being used outside of your own usage.

Finally, from the tests folder, it looks like the expected functions are all from 3dmol, so it is not designed for completely arbitrary code execution but rather from a list of existing 3dmol functions. So the "allowlist" approach would work IMHO.

Instead of data-callback="$3dmol.foo()" and data-callback="$3dmol.bar()", one could use: data-foo and data-bar instead.

I understand using eval is a quick and dirty working solution, but I hope you understand that it is a bad solution that must be removed from the codebase.

Best regards,
~Nicolas

[dkoes]

My argument is that an attacker needs to have control of the DOM at the time of the initial page load in order to exploit the eval. In order for the code to be untrusted, the user would have to load untrusted html during the initial page load (could not come from user input). The one issue is the developer may think they have properly sanitized this HTML to remove script tags only for data-callback to provide a workaround for the sanitation. Of course, if they are aware of this issue they can easily sanitize away the data-callback.