Prevent prototype pollution chaining to code execution via _.template, part 2

[alexbrasetvik]

This is a followup to #4355 which unfortunately was incomplete, because of …

options = assignInWith({}, options, settings, customDefaultsAssignIn);

... which happens prior to the hasOwnProperty guard would merge in whatever happens to be on the prototype into options, so it'd all be considered as part of options "own" properties. That might be a bug in assignInWith, it's not clear whether that's intentional behaviour.

My testing focused on the sourceURL part (which was easier to inject with than variable), and it got stopped by the newline stripping. The newline stripping was also incomplete, as you can provide a unicode newline.

I added a utility to get a property without looking at the prototype, which I guess might be a bit too much for this PR. Happy to remove that, but I think it'd be nice to have a utility like that in lodash. :)

[jdalton]

I want to be careful endorsing this as any kind of real issue. I accepted the previous PR as kind of a throw away thought. However, I believe it's mostly academic and it's something we've not supported in the past. Traditionally, if you mutate Object.prototype you've YOLO'd out of our support. That said if you fix this, please fix it for every-single-place we accept options in the code base then I can see this as having at least the value of providing consistent expectations throughout our methods.

[alexbrasetvik]

@jdalton I would agree that an application intentionally carelessly mutating Object.prototype should expect that things break.

For this particular issue, my concern is when an application unintentionally has a prototype pollution vulnerability, such as any application that used a slightly older version of lodash and used e.g. deep merge or clone would be. It's a hard problem to avoid in large code bases, as anyObject[a][b] = c is a prototype pollution for user controlled a, b and c, though it rarely presents itself as that "obvious". Most implementations of deep merge or deep-set-by-path that I've seen have been vulnerable to this at some point, which includes lodash.

Function/eval are super high value targets, and an application that has a prototype pollution bug and subsequently invokes lodash.template now has a remote code execution bug. The pollution is a vulnerability by itself that warrants fixing in the application, but at least we can make it harder for an attacker to escalate to arbitrary code execution.

Giving the same treatment to all other options might be a larger breaking change and is more likely to hit legitimate uses of options that inherit something.

At the very least I'd urge accepting the change to the sanitizing of sourceURL. If variable and imports are polluted, they still seem to be masked by the default settings, whereas sourceURL is not. sourceURL is also more likely to be coming from something user provided even without a pollution issue.

I'm happy to make a PR that only does the sourceURL sanitizing.

[alexbrasetvik]

I created #4518 which I hope is easier to accept. :)

[jdalton]

The pollution is a vulnerability by itself that warrants fixing in the application, but at least we can make it harder for an attacker to escalate to arbitrary code execution.

Existing JS APIs are susceptible to this today including descriptors for Object.defineProperty/Reflect.defineProperty, trap handlers for new Proxy(o, ...), and array index property access.

I created #4518 which I hope is easier to accept. :)

For #4518 you can continue to preserve the hasOwnProperty checks.

[alexbrasetvik]

Existing JS APIs are susceptible to this today including descriptors for Object.defineProperty/Reflect.defineProperty, trap handlers for new Proxy(o, ...), and array index property access.

I'm not sure I'm following. With defineProperty and proxy traps you can certainly change objects, but I'm not aware of it accepting strings in an options object that in turn execute as Javascript, which could be susceptible to a default being read off the prototype?

An attacker that is able to influence objects through these calls would already be in a pretty privileged position, much more so than someone just able to pass a JSON object that passes through pollution-prone merge-like calls.

[jdalton]

I'm not sure I'm following.

If you do Object.prototype.enumerable = true then if you don't provide the descriptor property enumerable to the Object.defineProperty method it will search the prototype of the descriptor object you provide and use the value found instead. Many folks don't provide the full descriptor or handle traps since they usually default to undefined.

[alexbrasetvik]

@jdalton Ok, so we close this in favour of #4518 ? :)