[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.

[dflock]

There's also a certain amount of anooying side effects - vite build warns you on every build:

vite v5.3.5 building for staging...
node_modules/.pnpm/[email protected]/node_modules/3dmol/build/3Dmol.js (21264:59): Use of eval in "node_modules/.pnpm/[email protected]/node_modules/3dmol/build/3Dmol.js" is strongly discouraged as it poses security risks and may cause issues with minification.
node_modules/.pnpm/[email protected]/node_modules/3dmol/build/3Dmol.js (21293:51): Use of eval in "node_modules/.pnpm/[email protected]/node_modules/3dmol/build/3Dmol.js" is strongly discouraged as it poses security risks and may cause issues with minification.
node_modules/.pnpm/[email protected]/node_modules/3dmol/build/3Dmol.js (27467:19): Use of eval in "node_modules/.pnpm/[email protected]/node_modules/3dmol/build/3Dmol.js" is strongly discouraged as it poses security risks and may cause issues with minification.

This also violates any and every security scanning policy of any kind, so causes CI pipelines that include SAST or other security scanning to fail, or emit warnings, depending on policy.

This does make it somewhat harder to use this component in my projects, regardless of any discussion about actual vulnerabilities.

[dkoes]

Is there an equivalent of a preprocessor macro that would allow this code to be omitted within your build system (prior to linting) while maintaining backwards compatibility?

[NicolasCARPi]

@dkoes Is there a place where the usage of data-callback attribute is mentioned in the documentation?

Corresponding code:

3Dmol.js/src/autoload.ts

Line 239 in ceda936

/*jshint -W061 */ var runres = eval(viewerdiv.dataset.callback);

and

3Dmol.js/src/autoload.ts

Line 269 in ceda936

/*jshint -W061 */ var runres = eval(viewerdiv.dataset.callback);

It seems redundant with the callback function argument that is called just below, with the same argument. IMHO these two blocks of code can be removed, and users that want a to execute a function on the glviewer can simply use the callback argument of the autoload function. Or, what about returning the glviewer object from the function so calling code can decide what they want to do with it. And you remove all the callback related code?

Is there an equivalent of a preprocessor macro that would allow this code to be omitted within your build system

Please let's not take this road. We must work to improve the library's code and not rely on end user hacking away vulnerable code.