-
Notifications
You must be signed in to change notification settings - Fork 411
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
Make big.js work when the Object prototype is frozen #202
base: master
Are you sure you want to change the base?
Conversation
x.constructor = Big; | ||
if (typeof Object.defineProperty === 'function') { | ||
// If the Object prototype is frozen, the "constructor" property is non-writable. This means that any objects which inherit this | ||
// property cannot have the property changed using an assignment. If using strict mode, attempting that will cause an error. If not | ||
// using strict mode, attempting that will be silently ignored. | ||
// However, we can still explicitly shadow the prototype's non-writable property by defining a new property on this object. | ||
Object.defineProperty(x, 'constructor', { value: Big }); | ||
} else { | ||
// If Object.defineProperty() doesn't exist, attempt to shadow this property using the assignment operator. | ||
x.constructor = Big; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that this package is designed to only use ECMAScript 3, so it works on all browsers.
I looked in the ECMAScript 3 specification and did not find mention of Object.defineProperty()
or Object.freeze()
.
I'm not sure exactly when these global functions were introduced in the ECMAScript specification, but here is are the MDN compatibility pages for both of them:
- https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperty#browser_compatibility
- https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/freeze#browser_compatibility
So, Object.defineProperty()
was supported before Object.freeze()
. This means we can assume that if Object.defineProperty()
does not exist, then the global Object prototype has not been frozen either.
I think the safest thing to do here is to check if Object.defineProperty()
exists and use it, otherwise fall back to shadowing these properties using the assignment operator.
I don't think this defensive code is necessary for the ES module though, because ES modules were introduced in ES2015, well after Object.defineProperty()
and Object.freeze()
.
Thanks for the detailed analysis. I think this been raised here before, or perhaps at one of my other libs. Can you say anything about how common it is for |
Sure thing!
I'm not sure I can accurately estimate how common it is for If I had to hazard a guess, I'd say it's probably fairly uncommon. Unfortunately it's very difficult to freeze global prototypes in this way for large JS applications, as a not-insignificant amount of 3rd-party packages like this one will break! There are several ways to defend against Prototype Pollution, but the most consistent way to do it across the board is to freeze the global prototypes (namely
For the application I'm working on, I'm using patch-package to patch dependencies piecemeal as I run into these issues, but it's something of a last resort. |
@MikeMcl friendly bump on this, do you think this change can be merged? |
Background
Note: examples use the Node.js REPL with strict mode:
node --use_strict
.Inheritance and Shadowing
Objects in JavaScript inherit properties from their prototype chain. For example, the "toString" property can be accessed on all objects, but it doesn't actually exist on each object, it exists on the global Object prototype:
Under normal circumstances, you can assign a property to an object using the
=
operator, and any property of the same name in the object's prototype chain will not be modified, but will be "shadowed" by the new property:Prototype Pollution
From Snyk:
There are a few different ways to mitigate Prototype Pollution, and one way to do it across the board is to freeze the global "root" objects and their prototypes (Object, Function, Array, etc.)
From MDN:
This means that any attempt to change the Object prototype will fail. If using strict mode, it will throw an error; otherwise, it will be silently ignored.
If the Object prototype becomes frozen, all of its properties are no longer writable or configurable:
This also prevents shadowing properties with assignment. If an object doesn't already have a property defined (such as "toString"), and it inherits a non-writable property of that name from its prototype chain, any attempt to assign the property on that object will fail:
This behavior is described in the ECMAScript 2016 specification:
The Problem
Unfortunately, this package uses assignment to shadow the "constructor", "toString", and "valueOf" functions:
big.js/big.js
Lines 112 to 114 in 9c6c959
big.js/big.js
Line 963 in 9c6c959
big.js/big.js
Line 1014 in 9c6c959
This means that projects cannot use this package if they have frozen the global Object prototype.
The Solution
You can still shadow non-writable prototype properties by explicitly defining a new data property on the object:
The module can be changed to use this method of shadowing so it is compatible with this approach of mitigating Prototype Pollution 🎉