Skip to content

Conversation

@odaysec
Copy link
Contributor

@odaysec odaysec commented Apr 11, 2025

else base[key] = extend[key];

fix for problem, we need to ensure that the merge function does not allow prototype pollution. This can be achieved by:

  1. Blocking the properties __proto__ and constructor from being merged.
  2. Ensuring that only own properties of the destination object are merged recursively.

We will modify the merge function to include these checks. Specifically, we will:

  • Add a check to skip the properties __proto__ and constructor.
  • Ensure that the property being merged is an own property of the destination object.

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. The issue can be fixed by ensuring that only own properties of the destination object are merged recursively:

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

@ornicar
Copy link
Collaborator

ornicar commented Apr 12, 2025

Thanks!

@ornicar ornicar merged commit 62137a3 into lichess-org:master Apr 12, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants