What bad coding practices makes a browser extension vulnerable?

Question

I'm trying to scan JavaScript files for vulnerabilities using JSHint. Specifically, I'm scanning the JavaScript files of browser extensions. To look for possible vulnerabilities I'm looking for bad JavaScript coding practices such as the use of eval. (JSHint has option to detect the use of eval.)

I'm currently scanning benign extensions and I'll be looking into vulnerabilities that could compromise the security of the user in some way (may be trivial). However, I don't know what more things (other than the use of eval, unused and undefined variables) that I should be looking for.

Could anybody suggest what I should be looking for?

Thnx! That's just for Firefox extensions (which, I guess, uses XPCOM API). But what about browsers like Chrome and IE? — TheRookierLearner, Apr 11 '13 at 05:33
Before I write a more detailed answer, please take some time and adjust your question to make it less broad. It's not clear _why_ you're auditing those browser extensions. `eval` is bad practice, yes, but in that context, it's not a security issue all the time. Are looking for critical resource access that depends on user input? (code injection). Are you looking for an intentionally-harmful code in a rouge extension? Or are you just looking for bad practices in JavaScript in general? — Adi, Apr 11 '13 at 05:43
I'm auditing to check how secure are legitimate browser extensions. Looking for intentionally harmful code in some rogue extension would be interesting but that's not what I'm doing right now. I hope this narrows down the scope of the question. — TheRookierLearner, Apr 11 '13 at 05:51
Using `==`/`!=` instead of `===` and `!==`. *Always* use the latter. The former often give unexpected behavior when confronted with unexpected types, leading to subtle vulnerabilities. — CodesInChaos, Apr 11 '13 at 08:17

score 9 · Accepted Answer · edited May 23 '17 at 12:40

In browser extensions, the impact of a vulnerability highly depend on the context and the requested permissions. Static tools (JSHint, AMO validator) to check code exist, but none of them are 100% reliable, especially for obfuscated code.

So, I'm not going to show a magic regular expression which tells you whether some code is vulnerable or not, but give a brief overview on how extensions work, and a small list of potential extension-specific issues.

Existing tools

Mozilla's static code analysis tool, used on addons.mozilla.org is publicly available as AMO Validator. This tool works for all Jetpack-based add-ons. First, it identifies (by hash) and excludes all whitelisted JS libraries. Then, it runs these test cases on the remaining content of the XPI archive.

None of the other vendors have published such a tool. Fortunately, extensions (not plug-ins) for Chrome / Safari / Opera cannot get as much privileges as Firefox add-ons.

Extracting

Firefox (xpi) and Opera (oex) add-ons are zip files with a different extension.
Chrome (crx) extensions are zip files with a preceding CRX header.
Safari (safariextz) extensions are signed xar archives.

7-zip is capable of extracting all of these archives.

Permissions

Chrome, Safari and Opera require the extension developer to declare permissions to access certain origins or features. These declarations can be found at:

Chrome: /manifest.json. See match patterns and permission warnings
Safari: /<extension-name>.safariextension/Info.plist. See Access and permissions.
Opera: /config.xml. See config.xml.

Origin access cannot be configured in Firefox add-ons.

Content scripts / Injected scripts

All extension environments support execution of code on pages which match a pre-defined URL pattern. These are called Content scripts in Chrome / Firefox, and Injected scripts in Opera / Safari. These scripts have direct access to the page's DOM, but no direct access to the page's JavaScript variables, because the execution contexts differ.

Background pages

These pages are the most privileged part of an extension.

Vulnerabilities

I'm not going to mention obvious mistakes such as using setTimeout with strings (these are already mentioned in bobince's answer), or far-fetched methods which are unlikely to be accidental, such as an extension which grabs credentials from a page and sends it to a rogue web service.

Using .innerHTML or jQuery to parse external HTML (see this question on Stack Overflow):

// Example 1:
var heading = $(xhr.responseText).find('h1').text();
// Example 2:
document.body.innerHTML += '';
// For: <img src="bogus" onerror="do_something_evil_with_privileges()">

Use of the window.onmessage event without validating the data or checking event.origin. In the following example, any third-party can use window.open and postMessage to send a message to Gmail, and abuse the unintentionally leaked features:
```
// Content script running on https://mail.google.com/mail/
addEventListener('message', function(event) {
    if (event.data.type === 'send-mail') doSomething();
    // or:
    notifyBackgroundPage(event.data);
});
```
Loading external (JS) code (worse: loading external code over http). If the external source is compromised, all users will be susceptible to abuse.
Use of cross-browser extension frameworks such as Crossrider and Kango. These are convenient to developers, but the generated extensions request an unnecessary number of permissions.
Not sufficiently restricting the influence of an extension API. An extreme example: Removing the httpOnly cookie flag for all sites, instead of just the relevant site. This also includes running content scripts on too many pages.
Privacy issues:
- Use of services like Google Analytics (search for _gaq.push)
- Loading external assets, such as social media buttons.

Most of the regular vectors are also relevant for extensions. Use the cheatsheets on https://www.owasp.org/ or take a look at http://html5sec.org/.

+1 Finally! An answer that _actually_ makes sense. Thank you for taking the time and effort to write it. — Adi, Apr 11 '13 at 11:23

score 6 · Answer 2 · edited Apr 11 '13 at 10:11

Common ways to inject JS from JS.

In almost every circumstance the wrong thing:

eval
new Function()
setTimeout and setInterval with a string for the first parameter
setAttribute (or framework equivalent eg attr()) on an on... event handler attribute (or variable attribute name)
javascript: URLs

Can be necessary in limited circumstances, but should be kept to a minimum and the remaining instances carefully audited:

createElement('script') and setting the body text
assigning a variable external URL to <script> or <link>
via HTML injection - setting innerHTML to a string that was created with a non-escaped variable, eg div.innerHTML= '<p>'+message+'</p>'. Usually better replaced with DOM-style content creation methods (especially given a framework to make it easy); for some cases like huge long tables a solution involving markup creation can be necessary
HTML injection hidden behind a framework function, eg any of the jQuery manipulation functions (.html(), .append(), .before() etc) called with a string argument (use $('<div>', {dynamic attributes}) shortcut instead)

Can be unavoidable in some cases, but need checking for:

using a variable URL when navigating location, creating a link, or setting image/object/iframe/etc src (or new Image(), or CSS url() properties). There are dangerous URL schemes (including but not limited to javascript:) so these need whitelisting. This is very common problem in browser extensions, which often ends up doing a universal XSS
CSS injection in content set in style element/attribute (using dangerous properties like bindings and expressions to elevate to JS injection)

Most of these are arguably *good* javascript practice - you'll find lots of examples of them in tools such as YUI and jquery. — symcbean, Apr 11 '13 at 08:42
Split into categories by forgivability. I would object to any of them being described as good practice. It's true that there are many JS tutorials doing stuff like markup injection but that doesn't make it acceptable - it has been the cause of many, many DOM-XSS holes including in browser extensions. jQuery in particular has a convenient/readable/secure creation alternative which is usually preferable. — bobince, Apr 11 '13 at 09:01
Here are some things you might want to add for extra paranoia: Using dynamic keys to access elements of an object not created using `Object.create(null)`, doing any http requests (it's a browser extension, so it should not be so careless – IMO any browser extension should use https). Also, scan for variables leaking into the global scope – usually a bad thing and might cause unexpected behavior. — thejh, Apr 11 '13 at 09:17
Adnan - Could you please elaborate on why this is close to useless in the case of browser extensions? I actually read it and was about to come to a conclusion that this *might* be the answer I'm looking for. — TheRookierLearner, Apr 11 '13 at 10:02
@TheRookierLearner I take my comment back. I probably was mistaken after all. — Adi, Apr 11 '13 at 10:08

score 1 · Answer 3 · answered Apr 11 '13 at 05:42

I'm not sure how it would apply to browser extensions, but in web apps I usually look for hardcoded strings getting concatenated with variables. It's often not a problem, but it helps me to find places where HTML is getting created from raw user input. For example the code might look like:

var myHtml = "<h1>" + userinput + "</h1>";

This is not safe because the user input might contain HTML (including script), and it should be HTML escaped before being used in this way.

Here is a regular expression that finds cases of hardcoded strings getting concatenated with variables:

["']\s*\+|\+\s*["']