From 759e953a5ef5121371181ae7f7d57d0d8e99f021 Mon Sep 17 00:00:00 2001 From: Ben Date: Mon, 22 Jun 2020 07:56:18 -0400 Subject: [PATCH 1/7] Unit test with payload from vulnerability PoC. --- test/saferEval.spec.js | 49 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/test/saferEval.spec.js b/test/saferEval.spec.js index d29597a..63eeaa4 100644 --- a/test/saferEval.spec.js +++ b/test/saferEval.spec.js @@ -370,6 +370,55 @@ describe('#saferEval', function () { }) describe('harmful context', function () { + // Testing security mitigation + // https://github.com/commenthol/safer-eval/issues/10 + it('tries to breakout', function () { + let res + const theFunction = function () { + const f = Buffer.prototype.write + const ft = { + length: 10, + utf8Write () { + + } + } + function r (i) { + var x = 0 + try { + x = r(i) + } catch (e) {} + if (typeof (x) !== 'number') { return x } + if (x !== i) { return x + 1 } + try { + f.call(ft) + } catch (e) { + return e + } + return null + } + var i = 1 + while (1) { + try { + i = r(i).constructor.constructor('return process')() + break + } catch (x) { + i++ + } + } + return i.mainModule.require('child_process').execSync('id').toString() + } + + const evil = `(${theFunction})()` + try { + res = saferEval(evil) + } catch (e) { + console.log(e) + assert.strictEqual(e, RangeError) + } + assert.strictEqual(res, undefined) + console.log(res) + }) + describeNode('in node', function () { it('evaluates global.eval if passing global as context - which is a bad idea', function () { var res = saferEval('global.eval(9 + 25)', { global: global }) // !!! try to avoid passing global as context this way From 5af1f17cd8e534ca6d1f6e97f427ce2e86432c03 Mon Sep 17 00:00:00 2001 From: Ben Date: Mon, 22 Jun 2020 08:00:15 -0400 Subject: [PATCH 2/7] WIP: Attempting code execution vuln fix, but I think in the wrong way. --- src/index.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/index.js b/src/index.js index 8846eee..8bf44bc 100644 --- a/src/index.js +++ b/src/index.js @@ -6,6 +6,7 @@ 'use strict' const vm = require('vm') +const babel = require("@babel/core"); const { createContext, allow } = require('./common') /** @@ -45,6 +46,10 @@ class SaferEval { src += 'return ' + code + ';\n' src += '})()' + const ast = babel.parse(src); + // this is probably the wrong place to try and fix, TBH + console.log('SRC\n', src.toString(), '\nLENGTH:', src.toString().length); + console.log('AST\n', ast.toString(), '\nLENGTH:', ast.toString().length); return vm.runInContext(src, this._context, this._options) } } From fe5316dd20cbd274791369804b3be9feb291d58a Mon Sep 17 00:00:00 2001 From: Ben Date: Thu, 30 Jul 2020 07:48:50 -0400 Subject: [PATCH 3/7] Added a unit test for the other, simpler breakout PoC on GitHub. --- test/saferEval.spec.js | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/test/saferEval.spec.js b/test/saferEval.spec.js index 63eeaa4..23f3795 100644 --- a/test/saferEval.spec.js +++ b/test/saferEval.spec.js @@ -370,11 +370,9 @@ describe('#saferEval', function () { }) describe('harmful context', function () { - // Testing security mitigation - // https://github.com/commenthol/safer-eval/issues/10 + // security mitigation: https://github.com/commenthol/safer-eval/issues/10 it('tries to breakout', function () { - let res - const theFunction = function () { + const maliciousFunction = function () { const f = Buffer.prototype.write const ft = { length: 10, @@ -408,15 +406,19 @@ describe('#saferEval', function () { return i.mainModule.require('child_process').execSync('id').toString() } - const evil = `(${theFunction})()` - try { - res = saferEval(evil) - } catch (e) { - console.log(e) - assert.strictEqual(e, RangeError) + const evil = `(${maliciousFunction})()` + const res = saferEval(evil) + assert.strictEqual(res, undefined) + }) + + it('tries to breakout another way', function () { + const anotherMaliciousFunction = function () { + const process = clearImmediate.constructor('return process;')() + return process.mainModule.require('child_process').execSync('whoami').toString() } + const evil = `(${anotherMaliciousFunction})()` + const res = saferEval(evil) assert.strictEqual(res, undefined) - console.log(res) }) describeNode('in node', function () { From c38f0a2a612420b06d10cd8595054261def36832 Mon Sep 17 00:00:00 2001 From: Ben Date: Thu, 30 Jul 2020 07:55:48 -0400 Subject: [PATCH 4/7] Backtracked on my previous convoluted attempt and went with a simpler fix to try and prevent user supplied template literals from accessing processes they don't need to be accessing. --- src/index.js | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/index.js b/src/index.js index 8bf44bc..53527f3 100644 --- a/src/index.js +++ b/src/index.js @@ -6,7 +6,6 @@ 'use strict' const vm = require('vm') -const babel = require("@babel/core"); const { createContext, allow } = require('./common') /** @@ -35,7 +34,7 @@ class SaferEval { /** * @param {String} code - a string containing javascript code - * @return {Any} evaluated code + * @return {String} evaluated code */ runInContext (code) { if (typeof code !== 'string') { @@ -46,11 +45,13 @@ class SaferEval { src += 'return ' + code + ';\n' src += '})()' - const ast = babel.parse(src); - // this is probably the wrong place to try and fix, TBH - console.log('SRC\n', src.toString(), '\nLENGTH:', src.toString().length); - console.log('AST\n', ast.toString(), '\nLENGTH:', ast.toString().length); - return vm.runInContext(src, this._context, this._options) + const dangerous = src.includes('require(\'child_process\')') || src.includes('require("child_process")') || src.includes('return process') + + if (dangerous) { + console.log('potentially unsafe system command') + } else { + return vm.runInContext(src, this._context, this._options) + } } } @@ -67,7 +68,7 @@ class SaferEval { * @throws Error * @param {String} code - a string containing javascript code * @param {Object} [context] - define globals, properties for evaluation context -* @return {Any} evaluated code +* @return {String} evaluated code * @example * var code = `{d: new Date('1970-01-01'), b: new Buffer('data')}` * var res = saferEval(code, {Buffer: Buffer}) From cbf170b4abbc2ba28b587fa2f7e8371a1f4820fc Mon Sep 17 00:00:00 2001 From: Ben Date: Thu, 30 Jul 2020 08:00:28 -0400 Subject: [PATCH 5/7] Minor console output wording update. --- src/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/index.js b/src/index.js index 53527f3..15f61af 100644 --- a/src/index.js +++ b/src/index.js @@ -48,7 +48,7 @@ class SaferEval { const dangerous = src.includes('require(\'child_process\')') || src.includes('require("child_process")') || src.includes('return process') if (dangerous) { - console.log('potentially unsafe system command') + console.log('potentially unsafe template literal') } else { return vm.runInContext(src, this._context, this._options) } From 8fb5a34fe764db71b51fca79ec6b8c4f8ad42edb Mon Sep 17 00:00:00 2001 From: Ben Date: Thu, 30 Jul 2020 08:02:29 -0400 Subject: [PATCH 6/7] Removed an extra function arg. --- src/common.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/common.js b/src/common.js index b4065ba..7faa9e5 100644 --- a/src/common.js +++ b/src/common.js @@ -52,7 +52,7 @@ exports.createContext = function () { context.console.constructor.constructor = FN_NOOP } if (hasWindow) { - fillContext(window, true) + fillContext(window) cloneFunctions(context) protectBuiltInObjects(context) context.console = clones(console, console) // console needs special treatment From f4368e9a1f2b3163fad76e341fef3bc2a948bad1 Mon Sep 17 00:00:00 2001 From: Ben Date: Thu, 30 Jul 2020 08:24:59 -0400 Subject: [PATCH 7/7] Added two more PoC unit tests. --- test/saferEval.spec.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/saferEval.spec.js b/test/saferEval.spec.js index 23f3795..48820d2 100644 --- a/test/saferEval.spec.js +++ b/test/saferEval.spec.js @@ -421,6 +421,20 @@ describe('#saferEval', function () { assert.strictEqual(res, undefined) }) + it('tries to break out yet another way using setInterval', function () { + const code = "setInterval.constructor('return" + + " process')().mainModule.require('child_process').execSync('whoami').toString();" + const res = saferEval(code) + assert.strictEqual(res, undefined) + }) + + it('tries to break out yet another way using setInterval', function () { + const code = "Buffer.of.constructor('return" + + " process')().mainModule.require('child_process').execSync('whoami').toString();" + const res = saferEval(code) + assert.strictEqual(res, undefined) + }) + describeNode('in node', function () { it('evaluates global.eval if passing global as context - which is a bad idea', function () { var res = saferEval('global.eval(9 + 25)', { global: global }) // !!! try to avoid passing global as context this way