Skip to content

WebExtensions and code review

All add-on files listed on addons.mozilla.org (AMO for short) have undergone code review and testing by a team of volunteer and hired add-on reviewers. The review team’s goal is to ensure these add-ons do what they claim to do and are safe to use.

Most safety issues come from add-on code that unintentionally puts users at risk of attack, for example XSS. We rarely encounter outright malicious add-ons attempting to be listed. Also, while the broad APIs exposed by XPCOM give add-ons the power to do lots of harm, most security problems we discover come from front end code (touching web content or managing the add-on’s UI), which also affect SDK add-ons and WebExtensions.

WebExtensions give us the opportunity to start over in some ways, so this is the ideal time to rethink what we can do to ensure that these add-ons are safer than their predecessors. By making them safer, we will also make them much easier to review, possibly even automatically in some cases.

So, what kinds of things do we look for in code review?

eval()

eval is a powerful function that lets you execute the contents of a string as JavaScript code. It is often used to parse JSON (and JSONP) responses from APIs. JSON parsing can be done trivially now, so it’s not allowed to use eval in add-ons for this purpose.

Due to its risks, we rarely allow eval in an add-on. Since eval can execute arbitrary code, these risks are clear. Using it in privileged add-on code is even more dangerous, so it needs to be done sparingly and very carefully. For review purposes we need to ensure that the string being evaluated is trusted (doesn’t come from an external source) and that there’s no better way to handle this in the add-on.

A common exception we grant is when an add-on overrides a native JS function to splice code into it. For example, if an add-on wanted to modify tab behavior, it could need to override an internal function in the Firefox tab code to accomplish its goal. In this case the add-on converts the function into a string, makes string replacements in it and then overrides it. If this sounds flaky and error-prone to you, you’re right, but in some cases it’s unavoidable. I don’t expect this to apply much to WebExtensions because they don’t have access to Firefox internals.

Another common use is code obfuscation. We could mitigate this for all add-ons (not just WebExtensions), by providing code obfuscation on AMO. This has been brought up before as a useful feature to offer, but has never been considered a priority.

setTimeout(), setInterval(), event handlers

Just like eval, these APIs can execute arbitrary JS code from strings. Unlike eval, they can be used in ways that don’t evaluate strings, which we strongly recommend.

So, instead of:

window.setTimeout("doSomething()", 1000);

You can do this:

window.setTimeout(function() { doSomething() }, 1000);

While the first example clearly isn’t doing anything unsafe, this requires manual inspection to determine. Our code analysis tool doesn’t inspect code in string literals, and the first example can easily become much more complicated by adding string concatenation and dynamic values. Thus we have to flag and manually inspect most uses of setTimeout() and similar in add-ons. Not a great use of anyone’s time, considering 99% of the time they’re perfectly safe.

One possible solution here is to offer “safe” versions of all of these methods for WebExtensions, which force using a function like in the second example. A more radical solution would be to force the “safe” behavior on the existing functions, but that would break lots of existing code, including libraries, so that seems very unlikely.

innerHTML, outerHTML

innerHTML is the easiest way to inject a string of HTML code into a document. It’s a very convenient object property, but it comes with security and performance costs. Like in previous examples, the string being injected could contain untrusted executable code (innerHTML prevents script elements from being executed, but there are ways around that). Because of the way innerHTML is often used, it can be very difficult for reviewers to determine the origin and safety of the string being injected. On the other hand, it’s a very convenient shortcut that developers use all too often.

In these cases, reviewers request developers to make sure the string being passed is sanitized (stripped from any code) before injection. But, like in the setTimeout() case, it’d be better if we could provide a safer equivalent so developers can opt into it and don’t get flagged for review.

Remote scripts / frames

Loading remote scripts significantly undermines the review process. If an add-on can load arbitrary code dynamically then it can be unsafe – or become unsafe – without the reviewers being able to detect it.

Injecting remote code into a privileged context – in which add-on code runs – is very dangerous. This gives the untrusted code control over what the add-on can do, so we never allow it. This should be a smaller problem for WebExtensions, since they have more limited permissions. However, there’s still real harm that can come from this, so I think it should be blocked on WebExtensions. Remote code execution should be limited to unprivileged scopes.

Injecting remote code into content is a smaller problem. Bad code and content injection used to break secure sites, but that’s less of a problem now with CSP and mixed content blocking. However, it’s still possible to inject remote scripts into arbitrary sites, potentially stealing private data or performing unwanted actions on behalf of the user. This is very common behavior in malicious add-ons. On the other hand, there are some valid use cases for remote scripts in content, so instead of blocking, this should probably be restricted for WebExtensions. I think only HTTPS URLs should be allowed, and the allowed origins be specified in the permissions.

Wrap up

WebExtensions will be safer than XUL and SDK add-ons by virtue of more limited APIs. However, they are still potentially dangerous to users, so we can’t completely do away with code reviews for AMO. Hopefully we can make some changes in the API that can minimize the need for code reviews and make developers’ lives easier.

{ 1 } Comments

  1. alex | 2016/03/31 at 8:35 AM | Permalink

    well , i know i had to cope with this shit already for normal addons .. i don´t think theres much merrit for even making addons if you can´t use eval , unsafewindow, access and modify chrome and xpcom . the ONLY thing which makes firefox better than otehr browsers are themany awesome xul and xpcom addons.

Post a Comment

Your email is never published nor shared. Required fields are marked *