Skip to content

Commit

Permalink
Close security issue in !!js/function constructor.
Browse files Browse the repository at this point in the history
Prevent it from code execution on parsing.
Thanks to @nealpoole for report.
  • Loading branch information
Dervus Grim committed Apr 26, 2013
1 parent 6853fa1 commit 430f880
Show file tree
Hide file tree
Showing 6 changed files with 60 additions and 5 deletions.
7 changes: 7 additions & 0 deletions HISTORY.md
Original file line number Diff line number Diff line change
@@ -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
------------------

Expand Down
29 changes: 25 additions & 4 deletions lib/js-yaml/type/js/function.js
Original file line number Diff line number Diff line change
@@ -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;
}
}
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
}
1 change: 1 addition & 0 deletions test/issues.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
3 changes: 3 additions & 0 deletions test/issues/data/issue-parse-function-security.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
tests:
- !!js/function 'makeBadThing("BAD THING 1")'
- !!js/function 'function () { makeBadThing("BAD THING 2") }.call(this)'
22 changes: 22 additions & 0 deletions test/issues/issue-parse-function-security.js
Original file line number Diff line number Diff line change
@@ -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.