Skip to content

Commit

Permalink
fix: various security vulnerabilities (#3255)
Browse files Browse the repository at this point in the history
* fix: disable parser functions in the CLI (security issue)

* fix: ensure `ObjectWrappingMap` doesn't allow deleting unsafe properties (security issue)

* fix: enable using methods and (safe) properties on plain arrays

* docs: update the "Less vulnerable expression parser" section in the docs

* chore: fix typos and linting issues

* chore: keep functions like `simplify` enabled in the CLI

* docs: update the security page

* fix: ensure `ObjectWrappingMap.keys` cannot list unsafe properties

* fix: when overwriting a rawArgs function with a non-rawArgs function it was still called with raw arguments

* docs: fix a typo
  • Loading branch information
josdejong authored Aug 27, 2024
1 parent 00a985d commit ed2cce4
Show file tree
Hide file tree
Showing 8 changed files with 182 additions and 122 deletions.
34 changes: 24 additions & 10 deletions bin/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,23 @@ const PRECISION = 14 // decimals
* "Lazy" load math.js: only require when we actually start using it.
* This ensures the cli application looks like it loads instantly.
* When requesting help or version number, math.js isn't even loaded.
* @return {*}
* @return {{ evalute: function, parse: function, math: Object }}
*/
function getMath () {
return require('../lib/browser/math.js')
const { create, all } = require('../lib/browser/math.js')

const math = create(all)
const parse = math.parse
const evaluate = math.evaluate

// See https://mathjs.org/docs/expressions/security.html#less-vulnerable-expression-parser
math.import({
'import': function () { throw new Error('Function import is disabled') },
'createUnit': function () { throw new Error('Function createUnit is disabled') },
'reviver': function () { throw new Error('Function reviver is disabled') }
}, { override: true })

return { math, parse, evaluate }
}

/**
Expand All @@ -68,7 +81,7 @@ function getMath () {
* @param {*} value
*/
function format (value) {
const math = getMath()
const { math } = getMath()

return math.format(value, {
fn: function (value) {
Expand All @@ -88,7 +101,7 @@ function format (value) {
* @return {[Array, String]} completions
*/
function completer (text) {
const math = getMath()
const { math } = getMath()
let matches = []
let keyword
const m = /[a-zA-Z_0-9]+$/.exec(text)
Expand Down Expand Up @@ -183,7 +196,7 @@ function runStream (input, output, mode, parenthesis) {
}

// load math.js now, right *after* loading the prompt.
const math = getMath()
const { math, parse } = getMath()

// TODO: automatic insertion of 'ans' before operators like +, -, *, /

Expand Down Expand Up @@ -214,7 +227,7 @@ function runStream (input, output, mode, parenthesis) {
case 'evaluate':
// evaluate expression
try {
let node = math.parse(expr)
let node = parse(expr)
let res = node.evaluate(scope)

if (math.isResultSet(res)) {
Expand Down Expand Up @@ -288,7 +301,7 @@ function runStream (input, output, mode, parenthesis) {
* @return {string | null} Returns the name when found, else returns null.
*/
function findSymbolName (node) {
const math = getMath()
const { math } = getMath()
let n = node

while (n) {
Expand Down Expand Up @@ -412,9 +425,10 @@ if (version) {
// run a stream, can be user input or pipe input
runStream(process.stdin, process.stdout, mode, parenthesis)
} else {
fs.stat(scripts[0], function (e, f) {
if (e) {
console.log(getMath().evaluate(scripts.join(' ')).toString())
fs.stat(scripts[0], function (err) {
if (err) {
const { evaluate } = getMath()
console.log(evaluate(scripts.join(' ')).toString())
} else {
// work through the queue of scripts
scripts.forEach(function (arg) {
Expand Down
16 changes: 11 additions & 5 deletions docs/expressions/security.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,10 @@ There is a small number of functions which yield the biggest security
risk in the expression parser:

- `import` and `createUnit` which alter the built-in functionality and
allow overriding existing functions and units.
- `evaluate`, `parse`, `simplify`, and `derivative` which parse arbitrary
input into a manipulable expression tree.
allow overriding existing functions and units, and `reviver` which parses
values into class instances.
- `evaluate`, `parse`, `simplify`, `derivative`, and `resolve` which parse
arbitrary input into a manipulable expression tree.

To make the expression parser less vulnerable whilst still supporting
most functionality, these functions can be disabled:
Expand All @@ -53,12 +54,17 @@ const math = create(all)
const limitedEvaluate = math.evaluate

math.import({
// most important (hardly any functional impact)
'import': function () { throw new Error('Function import is disabled') },
'createUnit': function () { throw new Error('Function createUnit is disabled') },
'reviver': function () { throw new Error('Function reviver is disabled') },

// extra (has functional impact)
'evaluate': function () { throw new Error('Function evaluate is disabled') },
'parse': function () { throw new Error('Function parse is disabled') },
'simplify': function () { throw new Error('Function simplify is disabled') },
'derivative': function () { throw new Error('Function derivative is disabled') }
'derivative': function () { throw new Error('Function derivative is disabled') },
'resolve': function () { throw new Error('Function resolve is disabled') },
}, { override: true })

console.log(limitedEvaluate('sqrt(16)')) // Ok, 4
Expand All @@ -71,7 +77,7 @@ console.log(limitedEvaluate('parse("2+3")')) // Error: Function parse is disable
You found a security vulnerability? Awesome! We hope you don't have bad
intentions and want to help fix the issue. Please report the
vulnerability in a private way by contacting one of the maintainers
via mail or an other private channel. That way we can work together
via mail or another private channel. That way we can work together
on a fix before sharing the issue with everybody including the bad guys.

## Stability risks
Expand Down
13 changes: 10 additions & 3 deletions src/expression/node/FunctionNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,15 @@ export const createFunctionNode = /* #__PURE__ */ factory(name, dependencies, ({
const rawArgs = this.args
return function evalFunctionNode (scope, args, context) {
const fn = resolveFn(scope)
return fn(rawArgs, math, createSubScope(scope, args))

// the original function can be overwritten in the scope with a non-rawArgs function
if (fn.rawArgs === true) {
return fn(rawArgs, math, createSubScope(scope, args))
} else {
// "regular" evaluation
const values = evalArgs.map((evalArg) => evalArg(scope, args, context))
return fn(...values)
}
}
} else {
// "regular" evaluation
Expand All @@ -196,8 +204,7 @@ export const createFunctionNode = /* #__PURE__ */ factory(name, dependencies, ({
}
default: return function evalFunctionNode (scope, args, context) {
const fn = resolveFn(scope)
const values = evalArgs.map(
(evalArg) => evalArg(scope, args, context))
const values = evalArgs.map((evalArg) => evalArg(scope, args, context))
return fn(...values)
}
}
Expand Down
19 changes: 5 additions & 14 deletions src/utils/customs.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { hasOwnProperty } from './object.js'
*/
function getSafeProperty (object, prop) {
// only allow getting safe properties of a plain object
if (isPlainObject(object) && isSafeProperty(object, prop)) {
if (isSafeProperty(object, prop)) {
return object[prop]
}

Expand All @@ -33,30 +33,23 @@ function getSafeProperty (object, prop) {
// TODO: merge this function into access.js?
function setSafeProperty (object, prop, value) {
// only allow setting safe properties of a plain object
if (isPlainObject(object) && isSafeProperty(object, prop)) {
if (isSafeProperty(object, prop)) {
object[prop] = value
return value
}

throw new Error('No access to property "' + prop + '"')
}

function getSafeProperties (object) {
return Object.keys(object).filter((prop) => hasOwnProperty(object, prop))
}

function hasSafeProperty (object, prop) {
return prop in object
}

/**
* Test whether a property is safe to use for an object.
* Test whether a property is safe to use on an object or Array.
* For example .toString and .constructor are not safe
* @param {Object | Array} object
* @param {string} prop
* @return {boolean} Returns true when safe
*/
function isSafeProperty (object, prop) {
if (!object || typeof object !== 'object') {
if (!isPlainObject(object) && !Array.isArray(object)) {
return false
}
// SAFE: whitelisted
Expand Down Expand Up @@ -158,8 +151,6 @@ const safeNativeMethods = {
export { getSafeProperty }
export { setSafeProperty }
export { isSafeProperty }
export { hasSafeProperty }
export { getSafeProperties }
export { getSafeMethod }
export { isSafeMethod }
export { isPlainObject }
12 changes: 8 additions & 4 deletions src/utils/map.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { getSafeProperty, hasSafeProperty, setSafeProperty } from './customs.js'
import { getSafeProperty, isSafeProperty, setSafeProperty } from './customs.js'
import { isMap, isObject } from './is.js'

/**
Expand All @@ -17,7 +17,9 @@ export class ObjectWrappingMap {
}

keys () {
return Object.keys(this.wrappedObject).values()
return Object.keys(this.wrappedObject)
.filter(key => this.has(key))
.values()
}

get (key) {
Expand All @@ -30,7 +32,7 @@ export class ObjectWrappingMap {
}

has (key) {
return hasSafeProperty(this.wrappedObject, key)
return isSafeProperty(this.wrappedObject, key) && key in this.wrappedObject
}

entries () {
Expand All @@ -44,7 +46,9 @@ export class ObjectWrappingMap {
}

delete (key) {
delete this.wrappedObject[key]
if (isSafeProperty(this.wrappedObject, key)) {
delete this.wrappedObject[key]
}
}

clear () {
Expand Down
8 changes: 7 additions & 1 deletion test/unit-tests/expression/node/FunctionNode.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -169,12 +169,18 @@ describe('FunctionNode', function () {
const b = new mymath.ConstantNode(5)
const n = new mymath.FunctionNode(s, [a, b])

let actualArgs
const scope = {
myFunction: function () {
myFunction: function (...args) {
actualArgs = args
return 42
}
}

assert.strictEqual(n.compile().evaluate(scope), 42)
assert.strictEqual(actualArgs[0], a.value)
assert.strictEqual(actualArgs[1], b.value)
assert.deepStrictEqual(actualArgs.length, 2)
})

it('should filter a FunctionNode', function () {
Expand Down
16 changes: 9 additions & 7 deletions test/unit-tests/utils/customs.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -150,14 +150,14 @@ describe('customs', function () {
assert.strictEqual(isSafeProperty(object, 'arguments'), false)
assert.strictEqual(isSafeProperty(object, 'caller'), false)

// non existing property
// non-existing property
assert.strictEqual(isSafeProperty(object, 'bar'), true)

// property with unicode chars
assert.strictEqual(isSafeProperty(object, 'co\u006Estructor'), false)
})

it('should test inherited properties on plain objects ', function () {
it('should test inherited properties on plain objects', function () {
const object1 = {}
const object2 = Object.create(object1)
object1.foo = true
Expand All @@ -172,11 +172,13 @@ describe('customs', function () {
assert.strictEqual(isSafeProperty(object2, 'constructor'), false)
})

it('should test for ghosted native property', function () {
const array1 = []
const array2 = Object.create(array1)
array2.length = Infinity
assert.strictEqual(isSafeProperty(array2, 'length'), true)
it('should test properties on an array', function () {
const array = [3, 2, 1]
assert.strictEqual(isSafeProperty(array, 'length'), true)
assert.strictEqual(isSafeProperty(array, 'foo'), true)
assert.strictEqual(isSafeProperty(array, 'sort'), true)
assert.strictEqual(isSafeProperty(array, '__proto__'), false)
assert.strictEqual(isSafeProperty(array, 'constructor'), false)
})
})

Expand Down
Loading

0 comments on commit ed2cce4

Please sign in to comment.