New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prototyped Scopes #888

Merged
merged 8 commits into from Jul 5, 2017

Conversation

Projects
None yet
2 participants
@ThomasBrierley
Contributor

ThomasBrierley commented Jun 29, 2017

Restore access to inherited properties and methods removed by the recent security fixes.

Fixes #886

I am not a security expert so please do review these changes carefully.

The most significant changes are in c4951c2, commit message:

To safely restore inherited properties and methods on plain objects e.g
with Object.create, some overly broad conditions need to be removed and
others added to more explicitly exclude unsafe properties.

isSafeMethod() has been modified as bellow, roughly the same conditions
are also now used in isSafeProperty() for get/setSafeProperty() which
previously restricted all inherited properties.

- Require __proto__ to have own-method
	Intended to prevent ghosting of class methods, but also prevents
	access to properties from further up the chain.

+ Require any own-method to not be in __proto__
	Explicitly prevents ghosting but not inheritance. Possible to
	defeat only if proto chaining through Object.create is allowed.

- Require object to not be function
	Intended to prevent unsafe function methods like 'bind', but
	also restricts function own-properties.

+ Require method not be in Function.prototype
	Explicitly prevents unsafe function methods like 'bind',
	without restricting function own properties.

Other conditions should be equivalent. The overall affect should be
that inherited properties and methods that are safe and not ghosted
should be allowed.

ThomasBrierley added some commits Jun 29, 2017

Refactor isSafe checks to restore inheritance
To safely restore inherited properties and methods on plain objects e.g
with Object.create, some overly broad conditions need to be removed and
others added to more explicitly exclude unsafe properties.

isSafeMethod() has been modified as bellow, roughly the same conditions
are also now used in isSafeProperty() for get/setSafeProperty() which
previously restricted all inherited properties.

- Require __proto__ to have own-method
	Intended to prevent ghosting of class methods, but also prevents
	access to properties from further up the chain.

+ Require any own-method to not be in __proto__
	Explicitly prevents ghosting but not inheritance. Possible to
	defeat only if proto chaining through Object.create is allowed.

- Require object to not be function
	Intended to prevent unsafe function methods like 'bind', but
	also restricts function own-properties.

+ Require method not be in Function.prototype
	Explicitly prevents unsafe function methods like 'bind',
	without restricting function own properties.

Other conditions should be equivalent. The overall affect should be
that inherited properties and methods that are safe and not ghosted
should be allowed.
Update tests for isSafe changes
+ Fix "calling Function via Object.assign" test. Now fails on accesing
"bind" first because new implementation is a bit more restrictive.

+ Fix "extend the class instance". Custom methods on instances are now
allowed and ghosting (overridding) is explicitly not.

+ Add ghosting tests for class instances and plain objects.
@ThomasBrierley

This comment has been minimized.

Contributor

ThomasBrierley commented Jun 29, 2017

A note on ghosting:

As noted in the commit message, the condition as implemented can be defeated if access to Object.create can be gained through the parser...

It's possible to make a foolproof ghosting condition by walking the prototype chain but this would be far more expensive - I chose not to because these checks are likely very hot functions.

[EDIT]

Gah, that close and comment button is so annoying!

@ThomasBrierley

This comment has been minimized.

Contributor

ThomasBrierley commented Jun 29, 2017

Travis failing on Nodejs v0.1 ... allowing access to __proto__

I don't have this version of Nodejs handy to test but __proto__ is prevented through Object.prototype having own property.

I can only guess that Nodejs v0.1 doesn't put this property on Object.prototype... although I can't see how that would have passed before either 😕

I'm tempted to say it doesn't matter because AFAIK Node v0.1 is not maintained anymore and thus is insecure.

@josdejong

This comment has been minimized.

Owner

josdejong commented Jun 30, 2017

Thanks Thomas, looks like a good start, I will look into it this soon.

Testing against nodejs v0.1 should be quite easy using nvm, could indeed be that __proto__ didn't exist back then.

@josdejong

This comment has been minimized.

Owner

josdejong commented Jul 3, 2017

I've looked more in-depth into your PR, I think it works nicely and I think it's safe solution. Looking forward to merge this improvement.

I've checked why one unit test fails on node v0.10, this boils down to:

Object.prototype.hasOwnProperty('__proto__')  
   // returns false on node.js v0.10, true on newer versions

I think we can either not care about node.js v0.10 and ignore this unit test when running v0.10, or we could introduce an unsafeNativeProperties object which explictly lists __proto__, and maybe add all properties of Object.prototype (like constructor). What do you think?

One question: do you know of actual cases for the places in the code where you check the following?

  // UNSAFE: ghosted
  // e.g length
  if (hasOwnProperty(object, prop) && (prop in object.__proto__)) {
    return false;
  }

You mention length, which is a good example but also one that's safe in practice. And all unit tests also succeed without this code. Would be nice to have a unit test that fails without this code :)

@ThomasBrierley

This comment has been minimized.

Contributor

ThomasBrierley commented Jul 4, 2017

I've looked more in-depth into your PR, I think it works nicely and I think it's safe solution. Looking forward to merge this improvement.

Great, and thanks 👍

we could introduce an unsafeNativeProperties object which explictly lists __proto__, and maybe add all properties of Object.prototype (like constructor). What do you think?

I had a look at __proto__ in v0.10... interestingly __proto__ is in Object.prototype, but hasOwnProperty is misbehaving.

I'm wondering if hasOwnProperty can just be replaced with in, because both Function.prototype and Object.prototype are roots, (they have no prototype) so using in should be equivalent.

Can you think of any reason using in here would be unsafe?

I think it would be good to keep the unsafe list dynamic so that it automatically capture new additions to the native prototypes before mathjs has a chance to consider them.

One question: do you know of actual cases for the places in the code where you check the following?

Good question, I don't have any solid examples, I just felt like it might be possible to defeat some (non-mathjs) app logic if a user was able to ghost properties.

Additionally this might be considered more of a preventative measure for bugs, as ghosting scopes in any context tends to be quite confusing and error prone.

I will add a test if you are ok with keeping it.

@ThomasBrierley

This comment has been minimized.

Contributor

ThomasBrierley commented Jul 4, 2017

Added tests for isSafeProperty, the ghosted one is a bit contrived but it does the job. JavaScript actually protects invalid array lengths so you don't need to worry about own properties and infinite loops or anything.

@josdejong

This comment has been minimized.

Owner

josdejong commented Jul 4, 2017

In this specific case it's perfectly fine to use the more "liberate" in instead of hasOwnProperty. In most of the security related code it's important though to use hasOwnProperty and not in. Can you add a comment explain why we use in at these specific places?

I'm definitely ok with keeping the ghosted checks, just wanted to see an example where it actually saves the day :), so if you manage to create a unit test hitting this code it's 👍

@josdejong

This comment has been minimized.

Owner

josdejong commented Jul 4, 2017

Thanks for working out unit tests for isSafeProperty, we still have to do that too for all other security methods, which are mostly only tested implicitly by the tests containing exploits which (should) hit one or a couple of barriers.

ThomasBrierley added some commits Jul 4, 2017

Use 'in' on root prototype checks
Because on NodeJS <= v0.10 hasOwnProperty returns false for __proto__ on
Object.prototype. Function.prototype and Object.prototype are roots so
it should be safe to use 'in' instead.
@ThomasBrierley

This comment has been minimized.

Contributor

ThomasBrierley commented Jul 4, 2017

Added comment to that commit for in conditions, a bit verbose but should be clear.

@ThomasBrierley

This comment has been minimized.

Contributor

ThomasBrierley commented Jul 4, 2017

There is a bug in that test.

@ThomasBrierley

This comment has been minimized.

Contributor

ThomasBrierley commented Jul 4, 2017

Ok that should do it, let me know if there is anything else you want changing.

@josdejong josdejong merged commit 69f3e88 into josdejong:develop Jul 5, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk No new vulnerabilities
Details
@josdejong

This comment has been minimized.

Owner

josdejong commented Jul 5, 2017

Great job, thanks!

I will play around with it a bit more in the develop branch, try to play the hacker and find holes in it. Will release it coming weekend.

@ThomasBrierley

This comment has been minimized.

Contributor

ThomasBrierley commented Jul 5, 2017

🍺 No rush, thanks for your help on this one.

@josdejong

This comment has been minimized.

Owner

josdejong commented Jul 5, 2017

LOL will do the 🍺 for sure

@josdejong

This comment has been minimized.

Owner

josdejong commented Jul 9, 2017

I've been doing some more testing with support for protototyped scopes. Looks good so far. One thing may need some attention. Right now, you can't access properties which are overwriting a property of a prototype object:

var object1 = {foo: 'bar'};
var object2 = Object.create(object1);
object2.foo = 'baz';

Now, in math.js, you can't access foo on object2. I think this is quite a common use case, like when you have a default config object and override some of the properties in an inherited, so we may need to enable that. What do you think @ThomasBrierley ?

@ThomasBrierley

This comment has been minimized.

Contributor

ThomasBrierley commented Jul 10, 2017

Sorry I think I may not have adequately explained what ghosting is supposed to be... The effect you are describing is an intentional result of the ghosting condition (the intent being to disallow overriding any inherited property.)

This is probably more of a security concern for methods (or at least app logic defeating concern). Overridden methods on instances might cause strange or possibly insecure behaviour when called internally.

I thought it was a good idea to apply to properties because ghosting in this sense is also something discouraged for the sake of clarity, however your config example shows reasonable and practical uses.

What if ghosting was still disallowed for methods but not other properties?

I'm not sure whether or not overriding properties is a security concern.

@josdejong

This comment has been minimized.

Owner

josdejong commented Jul 10, 2017

Indeed. For methods we shouldn't allow ghosted/overwritten methods, i.e. the method must not be defined on the object itself, only on it's prototype.

For plain objects though it is find to access/assign properties as long as they are not "special" properties inherited from a base prototype.

There is already a separation between checking whether to allow a method invocations vs. checking access for a property, and calling a method must indeed be more strict in this regard.

@ThomasBrierley

This comment has been minimized.

Contributor

ThomasBrierley commented Jul 12, 2017

Ok, if I understand correctly isSafeProperty ghosted properties are also required to be in Object.prototype (to be excluded).

@ThomasBrierley ThomasBrierley deleted the ThomasBrierley:prototyped-scopes branch Jul 12, 2017

@josdejong

This comment has been minimized.

Owner

josdejong commented Jul 13, 2017

👍 thanks for the PR. Hope to do a release this weekend

@ThomasBrierley

This comment has been minimized.

Contributor

ThomasBrierley commented Jul 14, 2017

Sorry, I really didn't think that through. [].length will error because length is an own property. I think other properties on the root prototypes also become own properties, which is a little anti-prototype but I guess they are special ones.

I need to think more carefully about how this affects individual properties on Object.prototype and Function.prototype. It may be that it's best to simply remove the ghosting condition from isSafeProperty... I can't actually think of a scenario where it would be unsafe yet.

@ThomasBrierley

This comment has been minimized.

Contributor

ThomasBrierley commented Jul 14, 2017

Yes, I think property ghosting should be removed, in it's current form it's pointless because the only property that exists in both the root prototypes and the allowed native properties list is length which is actually a special own-property.

The previous form of condition while restricting more would also have broken length because it also exists in __proto__.

@ThomasBrierley

This comment has been minimized.

Contributor

ThomasBrierley commented Jul 14, 2017

Ok that should hopefully be the last, sorry for the fuss.

I checked the ghosted methods in-case there were any quirky own inherited methods that might act like length. There don't seem to be any:

Object.getOwnPropertyNames(Object.prototype).forEach(key => {
    if (typeof ({})[key] === 'function' && ({}).hasOwnProperty(key)) {
        console.log(key)
    }
})
// none loged
@josdejong

This comment has been minimized.

Owner

josdejong commented Jul 14, 2017

makes sense, now I understand why you wanted to check for "ghosted" properties better and it's indeed essential for methods but not for properties (except that native properties like constructor and __proto__ should never be accessible, neither for properties nor as method).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment