Alternative to unsafe-eval

svalorzen · May 15, 2018, 10:05am

My extension has recently been disabled due to the enabling of unsafe-eval in the CSP. The extension provides a shell-like environment for the web, and unsafe-eval was used as a very important part of the addon is to enable users to write their own commands. The commands are written inside a text field in the option page of the addon, and I don’t require to use eval anywhere else. This is very similar to the old Ubiquity addon, which was made by Mozilla.

Without this feature, the extension loses most of its reason of existence. Is there any way to enable users to write their own scripts in a safe way?

freaktechnik · May 15, 2018, 12:25pm

You can have your own script parser that understands just the things you need. See something like esprime for an exsiting parser that would give you an AST that you can then handle.

svalorzen · May 15, 2018, 1:34pm

This seems a weird solution though. It’s equivalent to executing eval in the end, is it not? As the scripts would be written by the users, I do need a full js parser, and then I might as well use eval right?

Is there any way to obtain an exception for such a use case?

freaktechnik · May 15, 2018, 1:36pm

It’s not the same, since it can’t do anything, it can only do what you permit it to do.

erosman · May 15, 2018, 2:29pm

unsafe-eval/eval/blob are not allowed due to security issues. In rare cases, it can be allowed by Admin Reviewers only. If you wish to discuss the issues further, you can reach admin reviewers on IRC #addon-reviewers on irc://mozilla.org/

NilkasG · May 15, 2018, 4:07pm

One thing I am pretty sure you need to do to pass the review is to properly sandbox the call to eval, I don’t think that just calling eval in the background script will ever be accepted. (Besides, it is not a good idea because all your local abd global variables will be exposed, e.g. the a further down in the function. Stuff like that is pretty bad for forward compatibility.)

I do something similar in a not yet released extension (even though I think I will remove it, but that is mostly for UX reasons).
What I do is create an iframe with sandbox="allow-scripts", then send the code I want to eval to it. This lets you selectively choose the things you want to expose to the user supplied scripts, and there is no way for the scripts to escape the sandbox. Here is the code:

github.com

NiklasGollenstede/wikipedia-peek/blob/master/background/evaluator.js#L6-L35


class Evaluator {
	constructor() {
		return EvaluatorInit.apply(this, arguments);
	}


	async eval(code) {
		return Self.get(this).request('eval', code);
	}


	newFunction(...args) {
		const sandbox = Self.get(this), _this = this;
		const id = Math.random().toString(32).slice(2);
		const stub = async function() {
			if (!Self.has(_this)) { throw new TypeError(`Dead remote function called`); }
			return sandbox.request('call', id, ...arguments);
		};
		Object.defineProperty(stub, 'ready', { value: sandbox.request('create', id, ...args).then(length => {
			Object.defineProperty(stub, 'length', { value: length, }); return stub;
		}), });
		Object.defineProperty(stub, 'destroy', { value() { Self.has(this) && sandbox.post('destroy', id); }, });

This file has been truncated. show original

And the linked makeSandbox:

github.com

NiklasGollenstede/wikipedia-peek/blob/master/common/sandbox.js#L8-L76


/**
* Creates a sandboxed iframe and returns a es6lib/Port connected to it.
* @param  {function}  init              Function that does the initial setup inside the sandbox.
*                                       Gets re-compiled and called with one end of the Port connection as the only argument.
* @param  {object}    options           Optional Object with all optional properties to customize the sandbox.
* @param  {string}    options.html      Custom HTML for the frame, must not include the closing `</html>` tag.
* @param  {Boolean}   options.strict    Whether to include a global 'use strict' directive. Defaults to true.
* @param  {string}    options.srcUrl    Logical URL of `init` for debugging and stack traces.
* @param  {String}    options.sandbox   Custom value for the `sandbox` attribute of the iframe. Defaults to `"allow-scripts"`.
* @param  {Element}   options.host      Element to attach the frame to. Defaults to `document.head`.
* @return {Port}                        es6lib/Port instance with `.frame` as additional property.
*/
async function makeSandbox(init, {
	html = `<!DOCTYPE html><html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body></body>`, // without </html>,
	strict = true,
	srcUrl = rootUrl +'eval',
	sandbox = 'allow-scripts',
	host = global.document.head,
} = { }) {

This file has been truncated. show original

svalorzen · May 15, 2018, 7:52pm

Thanks for the tip, I’ll try to dissect your code later, as it seems it does quite a lot of things that are probably not useful to me (correct me if I’m wrong!).

A question on your approach: the user script I get should simply contain functions (added via an API I provide) which I index and then call as needed when the user wants to call them. So I guess I could give the iframe access to my API to register the functions, but then where would the functions reside when I want to actually call them? Should I have the iframe contain some messaging code so that I only get the names of the functions to register out of it, and then I can pass to it something like name-of-function + arguments, and get back the results of the functions?

Thanks in advance for the help.

NilkasG · May 16, 2018, 4:22am

your code […] seems it does quite a lot of things that are probably not useful to me (correct me if I’m wrong!).

I think you should actually use most of it.

A question on your approach: the user script I get should simply contain functions (added via an API I provide) which I index and then call as needed when the user wants to call them. So I guess I could give the iframe access to my API to register the functions, but then where would the functions reside when I want to actually call them? Should I have the iframe contain some messaging code so that I only get the names of the functions to register out of it, and then I can pass to it something like name-of-function + arguments, and get back the results of the functions?

Pretty much. But you don’t need to do it explicitly. The Evaluator class takes care of most of it. Here is an example:

const evaluator = await new (await require.async('background/evaluator'))({ init: port => {
	// ... create stubs for all functions that should be exposed and
	// forward the calls o the `port` (which is only available in this function)
	window.openTab = url => port.request('browser.tabs.create', { url, });
}, });
// ... add the handlers that the stubs call
evaluator.addHandlers('browser.tabs.', browser.tabs); // should be as selective as possible

// and then as often as you need it:

userFunction = evaluator.newFunction('url', 'openTab(url)'); // pass user input here
value = await userFunction('https://example.com'); // args and return value as structured clones
userFunction.destroy(); // release the resources in the sandbox

I re-wrote makeSandbox as class and Evaluator to inherit from it. The code should be clearer now.
And I moved the Evaluator file: https://github.com/NiklasGollenstede/wikipedia-peek/blob/master/common/evaluator.js

erosman · May 16, 2018, 5:46am

Note: unsafe-eval in the CSP is not allowed because eval() is not allowed.

Using eval() by any other means would be the same and subject to rejection.

NilkasG · May 16, 2018, 4:40pm

One would assume that if unsafe-eval gets allowed, it’ll only be allowed for extensions that do it securely.

And if this is not one of the rare cases (I think it should, because the extension really relies on it), my approach would also work to eval code in any content tab (which one does not matter at all, because of the sandboxing, and all the remote calls can easily be piped to the background script. It would be pretty inefficient, though, because the sandbox would need to be re-created all the time).

erosman · May 16, 2018, 6:00pm

There are only a limited examples where eval() make sense and thus would be allowed.

One example is calculators where the entry by the user is evaluated, although even then that requires safeguards.

Giving access to users to enter arbitrary code can result in browser (or computer) crash and other complications (depending on how the code is written). Evaluating an endless loop will cause a crash.

eval() in content scope has the additional security risks of being hijacked by other scripts in the same content.

That is the reason, Admin needs to decide how safe it is before allowing it.

svalorzen · May 16, 2018, 9:46pm

The scripts of the users are already wrapped in such a way that if they timeout without responding the function is interrupted and aborted, so in theory there shouldn’t be any way to crash (this is also done since the scripts may query network resources that may take a while to respond).

I agree with the security risks and I will definitely try to alter the code in order to sandbox the eval code, but I will ask the admins first whether it is worth it.

Thanks a lot in advance for your responses, I really appreciate the help =)

I’ll let you know whether I can manage to make the extension allowed again!

NilkasG · May 16, 2018, 9:52pm

I’d be genuinely interested in an accepted example and its “safeguards” (against infinite loops. Do you have one?

Giving access to users to enter arbitrary [JavaScript] code can result in browser (or computer) crash.

Giving users access to the power button can result in an unexpected power down as well. We are not removing those.
I think the approach here should be to tell users, who are only able to handle a power button, to just stay away.

arbitrary code can result in browser (or computer) crash […] (depending on how the code is written).

No matter how the code is written, that should not be possible. The worst possible outcome should be the loss of browser data (if the APIs are available) or 100% CPU utilization in a limited number of processes. If a browser crashes the computer, then there is something wrong with the browser and/or computer/OS.

eval() in content scope has the additional security risks of being hijacked by other scripts in the same content.

Oh. Yeah. Doing it in a content context is worse in a lot of ways. But it does not require a custom CSP …

That wont work. Your timeout will not fire before the current task has completed. If that never happens, the timeout will never fire either. (It will help with slow asynchronous tasks, though.)

erosman · May 17, 2018, 3:18am

For example, in case of calculators, limiting the content to numbers and mathematical signs will prevent functions/methods/etc being evaluated.

NilkasG · May 17, 2018, 2:06pm

If your “mathematical signs” include ()+[]!, you are still Turing complete.

That is why I’d like to see an actual concrete example, as in implementation. Do you have one?

erosman · May 17, 2018, 3:45pm

I said safeguards, to reduce not eliminate, the chance of problems.
That is the reason eval() should be avoided.

NilkasG · May 17, 2018, 4:03pm

Hm. I generally dislike have-baked solutions like that, but I see your point.

What about a Worker based solution, that you can simply kill after a timeout (which works here because it runs in a different thread)?
I don’t really know why I didn’t use a worker in the first place …

erosman · May 18, 2018, 4:45am

There is a way to make calculators considerably safe but with general JS and eval() not really. Whatever scope it runs on it can cause issues.

As they often say in programming circles, eval is evil.

Note: tabs.executeScrip() is also eval()

NilkasG · May 18, 2018, 6:12am

As they often say in programming circles, eval is evil.

Yes. It is often said and often true, because it can cross the line between data and code. But in cases where the evaled string is meant as code from the start, that is IMHO not the case.

But I think you can really create an airtight sandbox with a Worker, worker-src blob:, some basic modifications of the worer scope (e.g. remove all references to importScripts) and a timeout. Worst case the script sends incorrect data (structured clones) or keeps one CPU thread busy for a limited time.
And it would have no further impact on the security of the rest of the extension. script-src remains unchanged.

Obviously assuming that the browsers sandboxing of JavaScript works in general and side channel attacks like Rowhammer don’t apply. Sufficiently short timeouts should help here as well.

svalorzen · May 22, 2018, 11:56am

Just as an update, I’ve asked for help on the IRC channel and they said that I’ll hopefully be contacted soon about this.

@NilkasG Would you be interested in any case to work together to create a reusable component to do this, which can maybe get approved as a kind-of stand alone solution by Mozilla?