Regression with CVE-2023-0842 fix

[guimard]

Hi,

replacing {} by Object.create(null) does not produce the same value. This causes regression at least for node-rest-client tests (some Uncaught TypeError: Cannot read properties of undefined...). You should use Object.create({}) to have exactly the same behavior than previously:

$ node -p -e 'Object.create(null)'
[Object: null prototype] {}
$ node -p -e 'Object.create({})'
{}

[Leonidas-from-XIV]

But the whole point was not to do the same and fix the CVE. Does Object.create({}) fix the CVE without breaking backwards compatibility?

[guimard]

You're right @Leonidas-from-XIV, it doesn't fix the issue because you worked around the prototype pollution by setting prototype to "null" instead of filtering forbidden keys like __proto__

[guimard]

A workaround :

--- a/src/parser.coffee
+++ b/src/parser.coffee
@@ -181,8 +181,9 @@ class exports.Parser extends events
           # append current node onto parent's <childKey> array
           s[@options.childkey] = s[@options.childkey] or []
           # push a clone so that the node in the children array can receive the #name property while the original obj can do without it
-          objClone = Object.create(null)
+          objClone = {}
           for own key of obj
+            if key != '__proto__'
             objClone[key] = obj[key]
           s[@options.childkey].push objClone
           delete obj["#name"]
@@ -198,8 +199,11 @@ class exports.Parser extends events
         if @options.explicitRoot
           # avoid circular references
           old = obj
-          obj = Object.create(null)
-          obj[nodeName] = old
+          obj = {}
+          if nodeName === '__proto__'
+            console.error 'Node name __proto__ forbibben'
+          else
+            obj[nodeName] = old

         @resultObject = obj
         # parsing has ended, mark that so we won't throw exceptions from

[markwhitfeld]

Potentially a better fix would be to check the default javascript prototype for the property. We could check using this code:

  if (!Object.prototype.hasOwnProperty(key)) //.... then parse the property

[markwhitfeld]

Ah, I understand the issue now. Ignore my last suggestion, unless you are in fact interested in blocking all the default prototype properties.

The CVE is demonstrated through this:

// simulated exploit
({}).__proto__.foo = 1;
({})['constructor'].prototype.bar = 2;
// now any javascript object will contain these properties and values!
console.log({}.foo); // outputs 1
console.log({}.bar); // outputs 2

The best solution is to explicitly block __proto__ and constructor. I don't think it is necessary to block prototype because that is only destructive through access via constructor.

[Leonidas-from-XIV]

I've released 0.6.0 which filters the dangerous fields and recommend everyone to use 0.6.2, which should fix the CVE while not breaking things.