fix Prototype-polluting function `jqplot.themeEngine`

[odaysec]

https://github.com/matomo-org/matomo/blob/6b6ab13f3197015a39e36ce80520cca84ce72bc6/libs/jqplot/jqplot.themeEngine.js#L691-L691

fix the prototype pollution vulnerability in the $.jqplot.extend function:

1. Add a check to block dangerous property names (__proto__ and constructor) from being copied.
2. Ensure that the function only processes properties that are safe and do not affect the prototype chain.

The fix involves modifying the loop that iterates over the properties of options (line 674) to skip these dangerous property names. This ensures that malicious inputs cannot modify Object.prototype.

Most JavaScript objects inherit the properties of the built-in Object.prototype object. Prototype pollution is a type of vulnerability in which an attacker is able to modify Object.prototype. Since most objects inherit from the compromised Object.prototype, the attacker can use this to tamper with the application logic, and often escalate to remote code execution or cross-site scripting. One way to cause prototype pollution is through use of an unsafe merge or extend function to recursively copy properties from one object to another, or through the use of a deep assignment function to assign to an unverified chain of property names. Such a function has the potential to modify any object reachable from the destination object, and the built-in Object.prototype is usually reachable through the special properties __proto__ and constructor.prototype.

POC

This function recursively copies properties from src to dst:

function merge(dst, src) {
    for (let key in src) {
        if (!src.hasOwnProperty(key)) continue;
        if (isObject(dst[key])) {
            merge(dst[key], src[key]);
        } else {
            dst[key] = src[key];
        }
    }
}

However, if src is the object {"__proto__": {"isAdmin": true}}, it will inject the property isAdmin: true in Object.prototype.

function merge(dst, src) {
    for (let key in src) {
        if (!src.hasOwnProperty(key)) continue;
        if (dst.hasOwnProperty(key) && isObject(dst[key])) {
            merge(dst[key], src[key]);
        } else {
            dst[key] = src[key];
        }
    }
}

Alternatively, block the __proto__ and constructor properties:

function merge(dst, src) {
    for (let key in src) {
        if (!src.hasOwnProperty(key)) continue;
        if (key === "__proto__" || key === "constructor") continue;
        if (isObject(dst[key])) {
            merge(dst[key], src[key]);
        } else {
            dst[key] = src[key];
        }
    }
}

References

lodash, jQuery, extend, just-extend, merge.recursive CWE-78 CWE-79 CWE-94 CWE-400 CWE-471 CWE-915

Review

• [x] Functional review done
• [x] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
• [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
• [x] Security review done
• [x] Wording review done
• [x] Code review done
• [x] Tests were added if useful/possible
• [x] Reviewed for breaking changes
• [ ] Developer changelog updated if needed
• [ ] Documentation added if needed
• [ ] Existing documentation updated if needed

[github-actions[bot]]

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

[github-actions[bot]]

This PR was last updated more than one month ago, maybe it's time to close it. Please check if there is anything we still can do or close this PR. ping @matomo-org/core-reviewers

[github-actions[bot]]

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.