fix: two security vulnerabilities allowing execution of arbitrary JavaScript via the expression parser (#3656)

* fix: validate that the `index` in the `.get()` methods is an Array

* fix: improve the internal `setSafeProperty` to not allow setting properties other than numeric indices or `length` on arrays

Author: josdejong

src/utils/array.js

@@ -844,6 +844,9 @@ export function stretch (arrayToStretch, sizeToStretch, dimToStretch) {
 */
 export function get (array, index) {
   if (!Array.isArray(array)) { throw new Error('Array expected') }
+  if (!Array.isArray(index)) {
+    throw new Error('Array expected for index')
+  }
   const size = arraySize(array)
   if (index.length !== size.length) { throw new DimensionError(index.length, size.length) }
   for (let x = 0; x < index.length; x++) { validateIndex(index[x], size[x]) }

src/utils/customs.js

@@ -1,79 +1,83 @@
 import { hasOwnProperty } from './object.js'
 
 /**
- * Get a property of a plain object
+ * Get a property of a plain object or array
  * Throws an error in case the object is not a plain object or the
  * property is not defined on the object itself
  * @param {Object} object
  * @param {string} prop
  * @return {*} Returns the property value when safe
  */
-function getSafeProperty (object, prop) {
-  // only allow getting safe properties of a plain object
-  if (isSafeProperty(object, prop)) {
+export function getSafeProperty (object, prop) {
+  if (isSafeObjectProperty(object, prop) || isSafeArrayProperty(object, prop)) {
     return object[prop]
   }
 
-  if (typeof object[prop] === 'function' && isSafeMethod(object, prop)) {
-    throw new Error('Cannot access method "' + prop + '" as a property')
+  if (isSafeMethod(object, prop)) {
+    throw new Error(`Cannot access method "${prop}" as a property`)
+  }
+
+  if (object === null || object === undefined) {
+    throw new TypeError(`Cannot access property "${prop}": object is ${object}`)
   }
 
   throw new Error('No access to property "' + prop + '"')
 }
 
 /**
- * Set a property on a plain object.
+ * Set a property on a plain object or array.
  * Throws an error in case the object is not a plain object or the
  * property would override an inherited property like .constructor or .toString
  * @param {Object} object
  * @param {string} prop
  * @param {*} value
  * @return {*} Returns the value
  */
-// TODO: merge this function into access.js?
-function setSafeProperty (object, prop, value) {
-  // only allow setting safe properties of a plain object
-  if (isSafeProperty(object, prop)) {
+export function setSafeProperty (object, prop, value) {
+  if (isSafeObjectProperty(object, prop) || isSafeArrayProperty(object, prop)) {
     object[prop] = value
     return value
   }
 
-  throw new Error('No access to property "' + prop + '"')
+  throw new Error(`No access to property "${prop}"`)
 }
 
 /**
- * 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
+ * Test whether a property is safe for reading and writing on an object
+ * For example .constructor and .__proto__ are not safe
+ * @param {Object} object
  * @param {string} prop
  * @return {boolean} Returns true when safe
  */
-function isSafeProperty (object, prop) {
-  if (!isPlainObject(object) && !Array.isArray(object)) {
+export function isSafeObjectProperty (object, prop) {
+  if (!isPlainObject(object)) {
     return false
   }
-  // SAFE: whitelisted
-  // e.g length
-  if (hasOwnProperty(safeNativeProperties, prop)) {
-    return true
-  }
-  // UNSAFE: inherited from Object prototype
-  // e.g constructor
-  if (prop in Object.prototype) {
-    // 'in' is used instead of hasOwnProperty for nodejs v0.10
-    // which is inconsistent on root prototypes. It is safe
-    // here because Object.prototype is a root object
-    return false
-  }
-  // UNSAFE: inherited from Function prototype
-  // e.g call, apply
-  if (prop in Function.prototype) {
-    // 'in' is used instead of hasOwnProperty for nodejs v0.10
-    // which is inconsistent on root prototypes. It is safe
-    // here because Function.prototype is a root object
+
+  return !(prop in Object.prototype)
+}
+
+/**
+ * Test whether a property is safe for reading and writing on an Array
+ * For example .__proto__ and .constructor are not safe
+ * @param {unknown} array
+ * @param {string | number} prop
+ * @return {boolean} Returns true when safe
+ */
+export function isSafeArrayProperty (array, prop) {
+  if (!Array.isArray(array)) {
     return false
   }
-  return true
+
+  return (
+    typeof prop === 'number' ||
+    (typeof prop === 'string' && isInteger(prop)) ||
+    prop === 'length'
+  )
+}
+
+function isInteger (prop) {
+  return /^\d+$/.test(prop)
 }
 
 /**
@@ -83,7 +87,7 @@ function isSafeProperty (object, prop) {
  * @param {string} method
  * @return {function} Returns the method when valid
  */
-function getSafeMethod (object, method) {
+export function getSafeMethod (object, method) {
   if (!isSafeMethod(object, method)) {
     throw new Error('No access to method "' + method + '"')
   }
@@ -98,22 +102,32 @@ function getSafeMethod (object, method) {
  * @param {string} method
  * @return {boolean} Returns true when safe, false otherwise
  */
-function isSafeMethod (object, method) {
-  if (object === null || object === undefined || typeof object[method] !== 'function') {
+export function isSafeMethod (object, method) {
+  if (
+    object === null ||
+    object === undefined ||
+    typeof object[method] !== 'function'
+  ) {
     return false
   }
+
   // UNSAFE: ghosted
   // e.g overridden toString
   // Note that IE10 doesn't support __proto__ and we can't do this check there.
-  if (hasOwnProperty(object, method) &&
-      (Object.getPrototypeOf && (method in Object.getPrototypeOf(object)))) {
+  if (
+    hasOwnProperty(object, method) &&
+    Object.getPrototypeOf &&
+    method in Object.getPrototypeOf(object)
+  ) {
     return false
   }
+
   // SAFE: whitelisted
   // e.g toString
-  if (hasOwnProperty(safeNativeMethods, method)) {
+  if (safeNativeMethods.has(method)) {
     return true
   }
+
   // UNSAFE: inherited from Object prototype
   // e.g constructor
   if (method in Object.prototype) {
@@ -122,6 +136,7 @@ function isSafeMethod (object, method) {
     // here because Object.prototype is a root object
     return false
   }
+
   // UNSAFE: inherited from Function prototype
   // e.g call, apply
   if (method in Function.prototype) {
@@ -130,27 +145,12 @@ function isSafeMethod (object, method) {
     // here because Function.prototype is a root object
     return false
   }
+
   return true
 }
 
-function isPlainObject (object) {
+export function isPlainObject (object) {
   return typeof object === 'object' && object && object.constructor === Object
 }
 
-const safeNativeProperties = {
-  length: true,
-  name: true
-}
-
-const safeNativeMethods = {
-  toString: true,
-  valueOf: true,
-  toLocaleString: true
-}
-
-export { getSafeProperty }
-export { setSafeProperty }
-export { isSafeProperty }
-export { getSafeMethod }
-export { isSafeMethod }
-export { isPlainObject }
+const safeNativeMethods = new Set(['toString', 'valueOf', 'toLocaleString'])

src/utils/map.js

@@ -1,4 +1,8 @@
-import { getSafeProperty, isSafeProperty, setSafeProperty } from './customs.js'
+import {
+  getSafeProperty,
+  isSafeObjectProperty,
+  setSafeProperty
+} from './customs.js'
 import { isMap, isObject } from './is.js'
 
 /**
@@ -32,7 +36,7 @@ export class ObjectWrappingMap {
   }
 
   has (key) {
-    return isSafeProperty(this.wrappedObject, key) && key in this.wrappedObject
+    return isSafeObjectProperty(this.wrappedObject, key) && key in this.wrappedObject
   }
 
   entries () {
@@ -46,7 +50,7 @@ export class ObjectWrappingMap {
   }
 
   delete (key) {
-    if (isSafeProperty(this.wrappedObject, key)) {
+    if (isSafeObjectProperty(this.wrappedObject, key)) {
       delete this.wrappedObject[key]
     }
   }

test/unit-tests/expression/security.test.js

@@ -76,11 +76,55 @@ describe('security', function () {
   it('should not allow calling Function via Object.assign', function () {
     // TODO: simplify this test case, let it output console.log('hacked...')
     assert.throws(function () {
-      math.evaluate('{}.constructor.assign(cos.constructor, {binding: cos.bind})\n' +
+      math.evaluate(
+        '{}.constructor.assign(cos.constructor, {binding: cos.bind})\n' +
           '{}.constructor.assign(cos.constructor, {bind: null})\n' +
           'f=cos.constructor.binding()("console.log(\'hacked...\')")\n' +
-          'f()')
-    }, /Error: No access to property "bind/)
+          'f()'
+      )
+    }, /Error: No access to property "constructor/)
+  })
+
+  it('should not allow misusing setSafeProperty to set properties on Arrays', function () {
+    assert.throws(function () {
+      const exploit = `
+        constantNode = reviver('',{'mathjs':'ConstantNode','value':1});
+        
+        # get a reference to a raw JavaScript array
+        array = reviver('',{'mathjs':'ArrayNode'}).map().toJSON()['items'];
+        array.push({'name':'a','type':{}});
+        
+        # override params.map to hijack this.params and this.types
+        array.map = f(callback)=callback({'name':{'name':'a','map':f2(callback2)=callback2({},'constructor')},'type':cos});
+        
+        # trigger callback and get the pointer to Function.constructor
+        functionAssignmentNode = reviver('',{'mathjs':'FunctionAssignmentNode','name':'a','params':array,'expr':constantNode});
+        func = functionAssignmentNode.toJSON()['params']['type'];
+        
+        getProcess = func('return process');
+        getProcess()`
+
+      console.log(
+        'Hacked! node.js version:',
+        math.evaluate(exploit).entries[0].version
+      )
+    }, /Error: No access to property "map"/)
+  })
+
+  it('should not allow getting access to constructor via DenseMatrix.get index argument', function () {
+    assert.throws(function () {
+      const exploit = `
+        m = matrix();
+        func = m.get({"length":1,"reduce":f(callback,a)=callback(cos,"constructor")});
+        getProcess = func("return process");
+        getProcess()
+        `
+
+      console.log(
+        'Hacked! node.js version:',
+        math.evaluate(exploit).entries[0].version
+      )
+    }, /Error: Array expected for index/)
   })
 
   it('should not allow disguising forbidden properties with unicode characters', function () {

test/unit-tests/type/matrix/DenseMatrix.test.js

@@ -440,6 +440,16 @@ describe('DenseMatrix', function () {
       assert.throws(function () { m.get(math.index(1, 1)) })
       assert.throws(function () { m.get([[1, 1]]) })
     })
+
+    it('should throw an error when getting a value given an index that is not an array', function () {
+      assert.throws(function () {
+        m.get({ length: 1, reduce: () => {} })
+      }, /Error: Array expected for index/)
+
+      assert.throws(function () {
+        m.get(new Date())
+      }, /Error: Array expected for index/)
+    })
   })
 
   describe('set', function () {

test/unit-tests/type/matrix/SparseMatrix.test.js

@@ -730,6 +730,21 @@ describe('SparseMatrix', function () {
       assert.strictEqual(m.get([4, 5]), 13)
       assert.strictEqual(m.get([5, 5]), -1)
     })
+
+    it('should throw an error when getting a value given an index that is not an array', function () {
+      const m = new SparseMatrix([
+        [1, 2],
+        [3, 4]
+      ])
+
+      assert.throws(function () {
+        m.get({ length: 1, reduce: () => {} })
+      }, /Error: Array expected/)
+
+      assert.throws(function () {
+        m.get(new Date())
+      }, /Error: Array expected/)
+    })
   })
 
   describe('set', function () {

test/unit-tests/utils/array.test.js

@@ -678,6 +678,16 @@ describe('util.array', function () {
       assert.throws(function () { get(m, null) })
       assert.throws(function () { get(m, [[1, 1]]) })
     })
+
+    it('should throw an error when getting a value given an index that is not an array', function () {
+      assert.throws(function () {
+        get(m, { length: 1, reduce: () => {} })
+      }, /Error: Array expected for index/)
+
+      assert.throws(function () {
+        get(m, new Date())
+      }, /Error: Array expected for index/)
+    })
   })
 
   describe('checkBroadcastingRules', function () {