Permalink
Browse files

Close security issue in !!js/function constructor.

Prevent it from code execution on parsing.
Thanks to @nealpoole for report.
  • Loading branch information...
1 parent 6853fa1 commit 430f88097233695f6bc0cdfa6f594ad126930345 @dervus dervus committed Apr 26, 2013
View
@@ -1,3 +1,10 @@
+2.0.5 / --
+------------------
+
+* Close security issue in !!js/function constructor.
+ Big thanks to @nealpoole for security audit.
+
+
2.0.4 / 2013-04-08
------------------
@@ -1,18 +1,39 @@
'use strict';
+var esprima = require('esprima');
+
+
var NIL = require('../../common').NIL;
var Type = require('../../type');
function resolveJavascriptFunction(object /*, explicit*/) {
/*jslint evil:true*/
- var func;
try {
- func = new Function('return ' + object);
- return func();
- } catch (error) {
+ var source = '(' + object + ')',
+ ast = esprima.parse(source, { range: true }),
+ params = [],
+ body;
+
+ if ('Program' !== ast.type ||
+ 1 !== ast.body.length ||
+ 'ExpressionStatement' !== ast.body[0].type ||
+ 'FunctionExpression' !== ast.body[0].expression.type) {
+ return NIL;
+ }
+
+ ast.body[0].expression.params.forEach(function (param) {
+ params.push(param.name);
+ });
+
+ body = ast.body[0].expression.body.range;
+
+ // Esprima's ranges include the first '{' and the last '}' characters on
+ // function expressions. So cut them out.
+ return new Function(params, source.slice(body[0]+1, body[1]-1));
+ } catch (err) {
return NIL;
}
}
View
@@ -32,7 +32,8 @@
"bin" : { "js-yaml": "bin/js-yaml.js" },
"scripts" : { "test": "make test" },
- "dependencies" : { "argparse": "~ 0.1.11" },
+ "dependencies" : { "argparse": "~ 0.1.11",
+ "esprima": "~ 1.0.2" },
"devDependencies" : { "mocha": "*" },
"engines" : { "node": ">= 0.6.0" }
}
View
@@ -11,4 +11,5 @@ describe('Issues.', function () {
require('./issues/issue-46.js');
require('./issues/issue-54.js');
require('./issues/issue-64.js');
+ require('./issues/issue-parse-function-security.js');
});
@@ -0,0 +1,3 @@
+tests:
+ - !!js/function 'makeBadThing("BAD THING 1")'
+ - !!js/function 'function () { makeBadThing("BAD THING 2") }.call(this)'
@@ -0,0 +1,22 @@
+'use strict';
+/*global it */
+
+
+var assert = require('assert');
+
+
+var badThings = [];
+
+
+global.makeBadThing = function (thing) {
+ badThings.push(thing);
+};
+
+
+it('Function constructor must not allow to execute any code while parsing.', function () {
+ assert.throws(function () {
+ require('./data/issue-parse-function-security.yml');
+ });
+
+ assert.deepEqual(badThings, []);
+});

0 comments on commit 430f880

Please sign in to comment.