Add __proto__ to objects and arrays

[jordanbtucker]

Closes #199

[jdmarshall]

Nope, I'm still confused.

The test is asserting that you can overwrite proto from JSON5.parse(). How is this PR fixing the prototype pollution problem instead of marking it as a feature?

[jordanbtucker]

@jdmarshall The confusion is understandable. The bug was reported in #199 without the realization that it was also a vulnerability at the time, and #200 was written to address that bug but not merged because it was a borderline breaking change. Once the vulnerability aspect was realized, the fix in #200 was recreated in this PR and merged, but it did not contain any information about the vulnerability as to avoid potential zero-day exploits.

If you read the discussions in #199 and #200, you'll see that the issue was never to prevent JSON5 from representing an object with a property named __proto__, but it was about removing the special behavior that happens when you set the value of __proto__ on an object.

Normally in JavaScript, the __proto__ property of an object gives you access to its prototype. (Note that __proto__ is a deprecated legacy feature and Object.getPrototypeOf and Object.setPrototypeOf should be used instead.) For example:

const data = {foo: 1}

data.__proto__ = {bar: 2} // data's prototype is now {bar:2}

data.foo // returns 1

data.bar // returns 2 because bar is found up data's prototype chain
Object.keys(data) // returns ['foo'] because bar only exists on data's prototype

So you can see how setting __proto__ can have the affect of hiding properties from certain results including Object.keys, Object.values, and Object.entries, which has user input sanitization implications. This is discussed in the security advisory.

In JavaScript, you can also set an object's prototype while constructing the object like this:

const data = {
  foo: 1,
  __proto__: { bar: 2 }
}

data.foo // returns 1

data.bar // returns 2 because bar is found up data's prototype chain
Object.keys(data) // returns ['foo'] because bar only exists on data's prototype

However this does not work in JSON:

const json = `{
  "foo": 1,
  "__proto__": { "bar": 2 }
}`

const data = JSON.parse(json)

data.foo // returns 1

data.bar // returns undefined because bar is neither found on data nor up its prototype chain
Object.keys(data) // returns ['foo', '__proto__'] because __proto__ is just a regular property
data.__proto__ // returns {bar: 2} but it does not represent data's prototype
Object.getPrototypeOf(data) // returns {} (the default) because data's prototype
                            // was not set via the __proto__ property

The bug was that JSON5 was behaving like JavaScript when it should have been behaving like JSON. By making JSON5 use Object.defineProperty, which is what JSON does, we are able to create a __proto__ property that doesn't exhibit the special behavior that can lead to hidden properties.

Now it might seem like setting __proto__ to 1 and then asserting that it's still 1 is not a good test case. Granted, it probably would be clearer if the test case checked for __proto__ via Object.getOwnPropertyNames or something. However in JavaScript, if you set __proto__ to something that is not an object, then it silently fails to set the prototype. So the test case is good enough to answer the question: Is it behaving like JavaScript or is it behaving like JSON?

[aseemk]

^ This is such a fantastic explanation, @jordanbtucker. Thank you for taking the time to write it! ❤️

[jdmarshall]

So does this prevent { "__proto__": { "keys": 1 } } or not?

[jordanbtucker]

So does this prevent { "__proto__": { "keys": 1 } } or not?

No, it does not prevent using __proto__ as a property name. It just prevents the prototype-related side effects inherent to JavaScript when using the built-in __proto__ property.