Prototype issues with documents containing elements named <__proto__>

[bawolff]

Hi,

This issue might have a security impact (albeit I personally think the severity is minor). Since most people prefer to receive security reports privately, I attempted to email first. However I received no response, so I decided to file an issue.

If the XML document being parsed uses names like __proto__, it could cause the returned objects prototype to be replaced. This could cause confusion, and might result in documents being misinterpreted. Note, as far as i can tell, this only allows replacing the prototype, and not modifying the existing prototype, which I believe limits the severity of this bug.

As an example, say you wanted to know if the root element of a document is <foo>. You might do something like

require('xml2js').parseString( '<foo></foo>', {},
  (e,r) => {
    if('foo' in r) {
      console.log( "We have a foo document");
    }
  }
);

However, the code won't work as expected with the following document:

require('xml2js').parseString( '<__proto__><foo></foo></__proto__>', {},
  (e,r) => {
    if('foo' in r) {
      console.log( "We have a foo document");
    }
  }
);

This will say its a foo document, eventhough the root element is not foo.

One potential fix for this issue, would be to create the objects using object.create( null ); so that keys named __proto__ don't have special meaning.

Credit for discovering this issue should go to Dylan Katz of Leviathan Security.

[Leonidas-from-XIV]

Thanks for the report. Closed by #603.

[thetumper]

Thanks for the report. Closed by #603.

I'm seeing a similar issue with these changes. It results in "[Object: null prototype]" in each level of the result object, which wasn't there before. So my test was failing, until I resolved by surrounding with "JSON.parse(JSON.stringify(result))".

Example, for this input:
"<?xml version="1.0" encoding="UTF-8" standalone="yes"?><lsResponse><status><code>200</code><reason>Accepted</reason></status></lsResponse>"

Expected:
{ lsResponse: { status: [ { code: [ '200', [length]: 1 ], reason: [ 'Accepted', [length]: 1 ] }, [length]: 1 ] } }

But received:
[Object: null prototype] { lsResponse: [Object: null prototype] { status: [ [Object: null prototype] { code: [ '200', [length]: 1 ], reason: [ 'Accepted', [length]: 1 ] }, [length]: 1 ] } }

With jest, "expect(result).toStrictEqual(expected);" was now failing, until the above mentioned parse/stringify was added.

[Leonidas-from-XIV]

I assume this is just how "data-only" object (that is such without prototypes) are rendered in string form, it's not additional data (if anything, it is less data). So you'd need to change your expected object to be one that also doesn't have prototypes.

[thetumper]

I may be misunderstanding this stuff, but I don't think my expected object has prototypes (unless it is implicit?). The result from parseString is coming with the "null prototype" now, and did not come with that until this 0.5.0 version. I suppose that is explicitly saying there is no prototype, but is it needed to produce that?

[Leonidas-from-XIV]

Every JS object has a prototype implicitly when you create it via e.g. {}.

The reason why xml2js was bumped from 0.4 to 0.5 was the breaking change from #603 which started creating the objects returned by xml2js without prototypes.

So you're comparing objects without prototypes to objects which have prototypes and they're not equal. So you either need to use a comparison function that ignores it or remove the prototypes from your expected result.