Node-redis: Prototype pollution attack

[mostafa]

• Version: 2.8.0
• Platform: Node v12.3.0
• Description: Proof of concept: https://github.com/mostafa/js-pp-poc

[stockholmux]

I'm pretty sure this should be closed as it's actually JSON.parse which is causing the prototype pollution...

[mostafa]

Nope, already traced the code, it's about your code, which should be deprecated, but not. See my raw Readme.md for clues.

[jaamison]

@mostafa let me know if I'm missing something, but in order to exploit this an attacker needs to be able to execute code in the current Node process?

[mostafa]

You are right. I tried JSON.parse and it seems to be patched. But what I am trying to achieve is to let you know that if SET is used with an object instead of an stringified object, the whole __proto__ notation would go deep down the library without any part of the code handling and removing it and would get executed just before being passed to redis itself by overriding the toString function.
It seems that the library still uses the toString for backward compatibility, but I strongly suggest to remove it or at least prevent overridden toString to be executed.

[stockholmux]

So, this is an easy fix (Object.setPrototypeOf(args[i],Object.prototype) in RedisClient.prototype.internal_send_command), but considering that there are legit uses for this overriding Object.toString (and there is no way to tell if it is legit or nefarious) AND the exploit means that you have to have a compromised node process in some why, I see a few of courses of action:

1. Leave as-is since it is deprecated and a pretty slim vector.
2. Make it an optional but default behaviour to Object.setPrototypeOf(args[i],Object.prototype)
3. Change it fully and release as a breaking change.

@BridgeAR - thoughts? I'd go for 2.

[mostafa]

@stockholmux

1. Add a global flag that disables/enables potentially dangerous methods (kinda filtering) and warns of bad security practice, too.

[stockholmux]

@mostafa I am not in favour of that. "dangerous methods" is too broad.

[19h]

Or just use the methods meant to do this...

JSON.stringify(JSON.parse('{"b":2,"__proto__":{"a":1}}'), (k, v) => {
    if (k === '__proto__') return;
    return v;
}); // {"b":2}