Mathjs: Allow property access to arbitrary objects

[mblinsitu]

It is currently only possible to access properties of direct instances of Object.
This is highly constraining. For example I cannot use a class Point like so:

class Point {
  constructor(x, y) { this.x = x; this.y = y}
}

let scope = {
  p: new Point(10, 20)
}
math.eval('p.x', scope )

Why is this? I assume there is some security issue?
Could this constraint be somehow relaxed?

For now I have patched my version by removing the test object.constructor === Object in function isPlainObject, but this is not a satisfying solution...

[josdejong]

Thanks for reporting. That's indeed a common use case. These restrictions are there for security reasons, we've created them quite tight initially, let's look into whether we can relax them.

[ThomasBrierley]

I believe this limitation is from the isPlainObject check, which is currently required for both getting and setting any properties. (class/constructor Instances are not plain objects but {} or Object.create()ed objects are).

We could look into the consequences of relaxing this condition: Allowing the manipulation of properties on non-plain objects would potentially affect any MathJS class instances and function properties, we need to determine if that is a security concern. I think we should exclude set/get properties of functions unless anyone has any objections, which leaves object class instances, specifically:

1. Is it safe to set/get non-method properties on instance of any built-in MathJS classes? ...If not an alternative might be to blacklist built-in classes before removing the isPlainObject check.
2. Is it safe to set/get non-method properties on instance of custom classes/constructors in general?

The second question may have more to do with application specific security (e.g defeating some logic defined on the custom class via the parser), I have no examples this is pure speculation.

Note: calling of ghosted methods is dissalowed separately in the isSafeMethod check so we only need to consider non-method properties.

[josdejong]

Maybe it is just fine for class instances (objects which are not plain objects) to allow accessing properties of any type as long as they are defined on the object instance itself and do not exist on the the prototype?

[ThomasBrierley]

Accessing inherited properties is currently allowed on plain objects, I think it would be a shame to lose that but I'm a bit biased because that is the use case that got me involved in this part in the first place :stuck_out_tongue:

If dissalowing inherited properties is the way to allow class instance property access then different rules could be used for different object types, (we already use different rules for different property types).

This might actually make it a bit more clear by grouping the conditions more explicitly (prop method rules also become unified through an access mode parameter rather than using a separate function), e.g:

// object type filter
//     generic prop conditions
//     generic mode conditions
//     prop type filter
//         prop type conditions
//         prop mode conditions

f = (object, prop, mode = set | get | call) =>

    if typeof object == plain
        if prop in natives AND prop NOT in whitelist
            UNSAFE
        if typeof prop == method
            if mode != call
                UNSAFE
        if typeof prop != method
            if mode == call
                UNSAFE
        SAFE // single SAFE return point for each object type to prevent holes

    if typeof object == instance
        if prop in natives AND prop NOT in whitelist
            UNSAFE
        if typeof prop == method
            if mode != call
                UNSAFE
            if prop is ghosted
                UNSAFE
        if typeof prop != method
            if mode == call
                UNSAFE
            if prop is inherited
                UNSAFE
        SAFE // single SAFE return point for each object type to prevent holes

    UNSAFE // Catch all e.g typeof object == function

Note this is just a rough outline and may have holes. Also I've blurred the line between "safe" and "valid" by requiring the access type to be valid for the property type (specifically mode=call is only allowed for method props, currently this is handled by a separate validation function), but I suppose there could be a third return mode to differentiate invalid from unsafe if needed.

I prefer this more holistic organisation and have already noticed a hole in the existing implementation after looking at it from this new perspective. I think this organisation is also more accommodating to future additions and changes beyond this issue.

We could make this internal to customs.js if you wanted to retain existing interfaces it exports.

What do you think?

[EDITED] (I was asking too many silly questions)

[EDITED]...

I'll try to simplify above into how this could move forward:

1. Re-Implement into form resembling above pseudo code to facilitate extension
2. Check unit tests
3. Add new object instance rules
4. Add units tests

Then (assuming above goes ok) It would be worth having a poke around in various mathjs built ins to see if anything can be broken via an expression due to the new instance rules before merging.

[mblinsitu]

Thanks for your interest in this topic.

I am not sure I understand the issue with ghosted properties.

I have a use case that may or may not by related where setters/getters are defined in the class and reference 'shadow' properties in the instance. This is to provide reactive behavior at the property level, e.g. redisplaying a view when a model changes. (I am using mobx, and this is basically how it works under the hood).

class Point {
   set x(v) { this._x = v; /* trigger reactions */ }
   get x() { return this._x }

   set y(v) {this._y = v; /* trigger reactions */ }
   get y() { return this._y }

  constructor(x, y) { this.x = x; this.y = y /* triggers the setters */}
}

p = new Point(10, 20)
p.x = 30 // triggers reactions, e.g., redisplay

I would like this to work when p.x is set in a math.js expression.

Maybe one approach to allow full access to instance objects while limiting the risks would be for the application to explicitly whitelist constructors whose instances are legit, e.g. by adding them to a Set or WeakSe (to make the test efficient).

[ThomasBrierley]

Just to clarify: I use "ghosted" for lack of a better word to describe when an own-property is overriding an inherited property of the same name. It originates from a similar scenario with variable scopes in linting.

I have a use case that may or may not by related where setters/getters are defined in the class and reference 'shadow' properties in the instance

The ghosting check is only used for methods (more specifically, properties where typeof prop === 'function'). This would be used to prevent overriding a class method on an instance from within an expression for example.

JavaScript property setters and getters are not only transparent to assignment like p.x = val but also to typeof, using your example typeof p.x === 'number' is true (typeof uses the return value). Additionally when you set with an inherited setter you cannot override it as an own-property anyway. So the ghosting checks should not affect setters provided the values being set are not functions and should not affect getters regardless of the type.

I think it might be possible to unintentionally override setters with Object.defineProperty though (which is more explicit than an = assignment), so it might be worth checking separately whether mathjs use this internally to assign the result of an expression (separate issue to this one if it does).

On a side note: It's been a while since I've used JavaScript setters and getters, but I learned one thing from writing some small physics engines once which is: setters and getters are not very performant. So if your example is anything like your use case (it looks like a cartesian point of some kind), if you are accessing x and y many times a second (i.e doing some numerical integration in some simulation or something) you may want to consider that aspect.

[mblinsitu]

Thanks for the clarification. I now understand why you don't want to redefine a function from within a mathjs expression.

Any thoughts about my suggestion to use a whitelist of constructor functions? The implementation is trivial:

// in custom.js
var ctorWhiteList = new Set([Object]);
function isPlainObject (object) {
  return typeof object === 'object' && object && ctorWhiteList.has(object.constructor);
}

export function allowInstancesOf(ctor) {
    ctorWhiteList.add(ctor)
}

Side note: I am fully aware of the performance hit of setters/getters, although it varies greatly across browsers (see, e.g., this performance test).

[ThomasBrierley]

Any thoughts about my suggestion to use a whitelist of constructor functions? The implementation is trivial:

I don't see the need for it yet, unless i'm missing something? The pseudo code in my earlier post allows any instances of classes without the need for an explicit white list. Is there a particular behaviour i've left out that you need?

[edit]

Admittedly I haven't specified how I'm going to determine typeof object == instance but I imagine it should be possible through elimination...

object = {}
object.constructor === Object // true
object.constructor === Function // false

object = function () {}
object.constructor === Object // false
object.constructor === Function // true

object = () => {}
object.constructor === Object // false
object.constructor === Function // true

const MyConstructor = function () {}
object = new MyConstructor()
object.constructor === Object // false
object.constructor === Function // false

class MyClass {}
object = new MyClass()
object.constructor === Object // false
object.constructor === Function // false

So an Instance could be defined as an object whom's constructor is _not_ one of [Object, Function]. This does not distinguish between built-ins such as Array etc but I can't see any reason to need to.

Also I realise technically both {} and function () {} are instances of Object and Function respectively, but we don't really care since all the dangerous native properties on those are implicitly blocked unless they are in the native property whitelist and calling properties on functions is not allowed anyway.

[mblinsitu]

If you are OK with giving access to properties of any instance object (except ghosted function properties), that's great!

However in your pseudo code, accessing (non-method) inherited properties of instance objects is listed as unsafe. I believe this would not work for my "reactive property" use case since the setter/getter is defined in the class (i.e., the prototype), not the instance. So checking access to p.x in my example would not work because the getter for x is inherited by the instance. Am I missing something?

[ThomasBrierley]

However in your pseudo code, accessing (non-method) inherited properties of instance objects is listed as unsafe

Ah, you are correct, since the get/set type is determined by it's return value it would fall into the non-method block. The inherited property restriction was suggested by Jos.

allow accessing properties of any type as long as they are defined on the object instance itself and do not exist on the the prototype?

@josdejong could you tell us what the thinking behind that idea was? would it be possible to remove that condition safely?

[josdejong]

For example, if you can replace the _compile methods defined on Nodes (result of math.parse('2+3')), you can get access to node.js/browser via the expression parser with a smartly prepared expression. (There is a security.test.js file for inspiration :) )

Coming weekend I hope to dive deeper in this discussion.

[ThomasBrierley]

It the target property is a method I think that should be restricted by the typeof prop === method section for the object instance block:

if typeof prop == method
    if mode != call
        UNSAFE

In other words, on instances the only allowed access mode for method property types is 'call' (disallowing set and get).

To be honest I find the security test cases difficult to follow and will need to study them with a bit more effort.

[ThomasBrierley]

If it's easier I could just go ahead and implement this and then we can poke around with some examples afterwards to try and break it. It's probably more intuitive to just see the code in action... I realise this isn't exactly the most exciting type of feature for mathjs :stuck_out_tongue: I'm hoping to be able to do this next weekend (shouldn't take much time).

[josdejong]

Sorry for the late reply

I realise this isn't exactly the most exciting type of feature for mathjs

ha ha, yes, it's far from my favorite but very important.

@ThomasBrierley it would be great if you could play around with it. The code customs.js part of the code can use a refactoring anyway, but we need to be careful of course.

Just to try, I have changed if (isPlainObject(object) && isSafeProperty(object, prop)) into if ( isSafeProperty(object, prop)) in both getSafeProperty and setSafeProperty. That doesn't directly cause tests to fail (except that the exception messages differs), I think this is because isSafeProperty does it's job correctly. Now, removing isPlainObject(object) is too unrestrictive: we don't want to allow methods defined on classes to be replaced, so that needs to be refined.

So what we aim for now is the following right?

• for classes: do not allow overriding methods, allow overriding non-function properties.
• for plain objects: allow overriding properties and functions in plain objects, unless they are defined on Object.prototype or Function.prototype.

[ThomasBrierley]

That sounds about right.

I'd like to do this by going down the re-factor route based on my pseudo code from earlier if that's ok. This should make it much clearer to reason about the various rules as well as modifying them. I'll have a go over the weekend.

[josdejong]

Great, curious to see what you will come up with!

[ThomasBrierley]

I've done some experimentation, didn't quite turn out like I expected, I might have a few questions to ask later to save me some time.

My first attempt didn't break as many test as I expected once I removed checking for specific assertion messages... I have about 11 broken, a handful are to be expected because they test exactly what we are trying to allow (I will list them once i'm happy with the implementation)... others I need to work on because they are due to differences in the implementation.

RE #1019

Moving away from native eval sounds nice. Do you think that would completely obviate any property access checking for security? or would it just turn it into something more specific to mathjs expressions?

[josdejong]

Moving away from native eval sounds nice. Do you think that would completely obviate any property access checking for security? or would it just turn it into something more specific to mathjs expressions?

Property access checking remains essential (see https://github.com/josdejong/mathjs/issues/1019#issuecomment-359355305), basically we must prevent from getting access to Function via the constructor of a function or class, which would allow arbitrary code execution. For example sqrt.constructor("console.log('hacked!!!')")().

[josdejong]

@ThomasBrierley did you manage to make any progress in refactoring the accessor functions? What is the current state?

[ThomasBrierley]

Sorry, things got critical at work and then I was also sick so haven't made any progress. I'll try to fit some more in this week.

[ThomasBrierley]

I'll carry on with this idea for now and if mike's idea in #1019 becomes the way to go then we can just take the rules and put them in a class instead. Maybe as a separate PR to keep things simple.

[josdejong]

Sorry, things got critical at work and then I was also sick so haven't made any progress.

Take it easy, no need to hurry. I have these weeks too, I guess everyone has. :)

[alexkwolfe]

Did this ever find a resolution? I'm trying to do something like this, which is failing because the property is inaccessible:

const range = new String('1-10');
range.min = 1;
range.max = 1;
math.eval('1 + range.min', { range });
Error: No access to property "min"
    at getSafeProperty (node_modules/mathjs/lib/utils/customs.js:25:9)
    at evalAccessorNode (node_modules/mathjs/lib/expression/node/AccessorNode.js:74:16)
    at evalOperatorNode (node_modules/mathjs/lib/expression/node/OperatorNode.js:88:51)
    at Object.evalNode [as eval] (node_modules/mathjs/lib/expression/node/Node.js:49:16)
    at stringObject (node_modules/mathjs/lib/expression/function/eval.js:47:36)
    at Object.compile (node_modules/typed-function/typed-function.js:1086:85)

The "easy" fix is to use an Object rather than a String, but I'm working inside another system where making this change is not possible. Are there other work arounds that don't compromise security?

[josdejong]

No progress on this issue yet. If anyone is interested in picking this up please let me know.

As for the problem at hand: using a new String object with properties on it sounds tricky in general though, would be great if you could find a way to use regular string primitives and objects instead, maybe you can somehow translate between your code and the external code that you can't control.

[ThomasBrierley]

My colleagues are also stuck on an old version of MathJS due to this issue but i haven't had the time, I'll see if I can get some allotted time to spend on it.

However i'm happy to surrender this if someone gets to it first :smiley:

[EDIT]

This finally blocked something important enough to get me some time to spend on it :smiley: I'll be picking it up this month to completion.