Prototype Pollution vulnerability affecting @thi.ng/paths, versions <=5.1.62

[tariqhawis]

Prototype Pollution vulnerability affecting @thi.ng/paths (1.3.8)

Details
Prototype Pollution is a vulnerability affecting JavaScript. Prototype Pollution refers to the ability to inject properties into existing JavaScript language construct prototypes, such as objects. JavaScript allows all Object attributes to be altered, including their magical attributes such as proto, constructor and prototype. An attacker manipulates these attributes to overwrite, or pollute, a JavaScript application object prototype of the base object by injecting other values. Properties on the Object.prototype are then inherited by all the JavaScript objects through the prototype chain. When that happens, this leads to either denial of service by triggering JavaScript exceptions, or it tampers with the application source code to force the code path that the attacker injects, thereby leading to remote code execution.

Vulnerable function:

1. mutIn() in paths/mut-in.js
2. mutInManyUnsafe() in paths/mut-in-many.js in

POC:
import * as paths from "@thi.ng/paths";

var victim = {}
console.log("Before Attack: ", JSON.stringify(victim.proto));
try {
paths.mutIn({}, [["proto"], "polluted"], true, true)
paths.mutInManyUnsafe({}, [["proto"], "polluted"], true)
} catch (e) { }
console.log("After Attack: ", JSON.stringify(victim.proto));
delete Object.prototype.polluted;

Mitigation:

Freeze the prototype— use Object.freeze (Object.prototype).
Validation of JSON inputs.
Use Map instead of Object.
Crete objects without prototype, that will break the prototype chain and preventing pollution. Example:
let obj = Object.create(null);
obj.proto // undefined
obj.constructor // undefined

[postspectacular]

Hi @tariqhawis - can you please provide some actual working proof of this issue (i.e. link to a working example) since the code above has several issues:

1. Cannot reproduce the issue running the above code (i.e. victim.proto is undefine before/after the attack), neither in node nor the browser
2. The params passed to the functions are wrong (i.e. the path [["proto"], "polluted"] is invalid), also mutIn() only ever takes 3 args, not 4
3. We already have a prototype pollution check in place, using isProtoPath()

Can you please clarify? Thanks!

[postspectacular]

I just realized this is about a version from almost 6 years ago (~April 2018, v1.3.8 vs current version 5.1.61)! Thanks for wasting people's time! Closing!

[tariqhawis]

Hi @postspectacular ,

I investigated the problem and it seems there are two npm packages for this project published on two separate pages. The outdated one that the vulnerability test has targeted located here. In any case, this link should not be publicly available for the users to prevent such confusion.

[postspectacular]

That linked project is a fork (@allforabit scope) of the @thi.ng/paths project not under my control...

[tariqhawis]

I checked the latest published version (5.1.62). It's also vulnerable to prototype pollution via mutIn() and mutInManyUnsafe(). The following PoC invoked both functions separately for separate effects. Note that __proto__ property in victim.__proto__ should be written with double underscores not like this: victim.proto, it might filtered out by github in my previous post.
Similar thing with the argument : [["__proto__"], "polluted"]. About this one, it's a one array object.

Following the code, the sink is traced from mutIn() in mut-in.js to defMutator() in mutator.js which triggered prototype pollution at the assignment code:

return s ? (t = s[a]) ? (t[b] = x, s) : void 0 : void 0;

Here is the PoC:

import * as paths from "@thi.ng/paths";

var victim = {}
console.log("Before Attack: ", JSON.stringify(victim.__proto__));
try {
paths.mutIn({},[["__proto__"], "polluted"], true)
} catch (e) { }
console.log("After Attack: ", JSON.stringify(victim.__proto__));
delete Object.prototype.polluted;

var victim = {}
console.log("Before Attack: ", JSON.stringify(victim.__proto__));
try {
paths.mutInManyUnsafe({},[["__proto__"], "polluted"], true)
} catch (e) { }
console.log("After Attack: ", JSON.stringify(victim.__proto__));
delete Object.prototype.polluted;

[postspectacular]

Okay. This is interesting in that this is only an error if misused like this in JS, but it would not even compile in TypeScript, since the offending path [["__proto__"], "polluted"] is an simply an invalid argument for either of these functions. The TS function signature does not allow individual path items which are arrays and checks for the syntactically valid ["__proto__", "polluted"] are already in place and will throw an assertion error via:

umbrella/packages/paths/src/path.ts

Lines 61 to 78 in ded7c5e

	/**
	* Helper function to analyze given `path` using
	* [`isProtoPath()`](https://docs.thi.ng/umbrella/checks/functions/isProtoPath.html).
	* Throws an error if path contains any property which might lead to prototype
	* poisoning.
	*
	* @remarks
	* The following properties are considered illegal.
	*
	* - `__proto__`
	* - `prototype`
	* - `constructor`
	*
	* @param path -
	*/
	export const disallowProtoPath = (path: Path) => (
	assert(!isProtoPath(path), `unsafe path: '${path}'`), path
	);

and

umbrella/packages/checks/src/is-proto-path.ts

Line 4 in ded7c5e

const ILLEGAL_KEYS = new Set(["__proto__", "prototype", "constructor"]);

So considering this only remaining vulnerability is due to a complete misuse and passing of illegal arguments to the function, I'll update the internally used toPath() helper to validate a given path and ensure it doesn't contain any array elements (or otherwise will throw an error)...

[postspectacular]

@tariqhawis I just released an update with the above changes (and updated tests) as v5.1.63