AMO:Editors/EditorGuide/AddonReviews/Security

Security Code Review

The table on the Review Guide gives a summary of issues we check for. The sections below replicate these rejection reasons but with more detail and examples.

(This page is still in progress; some sections are empty and others may contain incorrect examples at the moment!)

Using eval, Function(), setTimeout, setInterval to evaluate remote code

Using eval() at all in an add-on is generally prohibited, which some exceptions (notably patching Firefox functions where there are no other alternatives).

eval("code;") is effectively the same as setTimeout("code;",1) or setInterval("code;",1);

Examples of common (mis)-uses of eval and similar functions

var output = eval(r.responseText);  //mistakenly used to parse JSON - any code will be executed.

var data=r.responseText;
setTimeout("doSomething('"+data+"');",100);  //would be executed if data contained a ' character.

In the case of JSON parsing there is a canned response referring developers to https://developer.mozilla.org/en/Using_JSON_in_Firefox

<browser> or <iframe> elements with no type

If a <browser> or <iframe> tag is used in a xul document then by default it loads any content inside with the same privileges as the xul document containing it. So a <iframe> tag in an overlay will load pages with full chrome access by default.

A tag with type="content", type="content-targetable" or type="content-primary" will safely load any page within as the user would normally load a webpage.

There is a canned response that points developers to this page: https://developer.mozilla.org/En/XUL/Iframe#a-browser.type

Bad (and Good) examples

Bad - no type parameter:

<iframe src="https://www.google.com/">

Good - type set to content:

<iframe src="https://www.google.com/" type="content">

The type attribute must be set before any page is loaded or it is ignored. (There is a request to stop this)

Bad - src set before type:

In overlay.xul

<iframe id="ifr">

In overlay.js

var ifr=getElementById("ifr");
ifr.setAttribute("src","http://www.google.com"); //google now has control of Firefox!
ifr.setAttribute("type","content"); //ignored because src set on the line above

Good - src set after type:

In overlay.xul

<iframe id="ifr">

In overlay.js

var ifr=getElementById("ifr");
ifr.setAttribute("type","content"); 
ifr.setAttribute("src","http://www.google.com");

Looks Good but Bad also (about:blank loaded so type setting ignored):

In overlay.xul

<iframe src="about:blank" id="ifr">

In overlay.js

var ifr=getElementById("ifr");
ifr.setAttribute("type","content"); //ignored because about:blank was loaded when overlay.xul was loaded
ifr.setAttribute("src","http://www.google.com"); //google now has control of Firefox!

Remote script injection

We generally don't allow addons to insert remote JavaScript into web pages as we can't guarantee the content will stay the same as when we review it. HTTP insertion is also vulnerable to MITM attacks (where the content is tampered with in transit) and will break the seal on HTTPS pages.

Examples of remote script insertion

Literal tag in xul/html:

<script src="http://www.google.com/script.js />

Insertion of tag from a script into a document:

var s=document.createElement("script");
s.setAttribute("src","http://www.google.com/script.js");
document.appendChild(s);

Retrieving remote content and then setting the tag to the content

var data=r.responseText;
var s=document.createElement("script");
s.innerHTML=data;
document.appendChild(s);

Inserting remote content with innerHTML

...

Remote code download or execution

...

Custom update code

...

HTTP security violations

...