Improved fix for CVE-2023-0842

[markwhitfeld]

This is potentially a better fix for CVE-2023-0842 that does not have the regressions mentioned in issue #672

The idea here is that the CVE (slightly more details here ) is specifically referring to the modification of the base object prototype through the merging of objects.

I think that this is the type of modification it is referring to:

// simulated exploit
({}).__proto__.foo = 1;
({})['constructor'].prototype.bar = 2;
// now any javascript object will contain these properties and values!
console.log({}.foo); // outputs 1
console.log({}.bar); // outputs 2

So, the solution in this PR is to explicitly block the properties __proto__ and constructor. I don't think it is necessary to block prototype because that is only destructive through access via constructor.

I tried to cover all locations in the code that seemed to be merging objects, but I may have missed a location or two in this codebase which is quite unfamiliar to me, and written in a language I don't know.

PS. I was not able to verify if this fix covers all possibilities through tests because it doesn't seem like there are any explicit tests for the CVE. CoffeeScript is very foreign to me, so I wouldn't even know where to look for these!

[markwhitfeld]

@guimard Are there any other places in the code that should be guarded against bad property names?

[guimard]

@guimard Are there any other places in the code that should be guarded against bad property names?

Hi @markwhitfeld, I'm not the author, but AKAIK it's enough now

[Leonidas-from-XIV]

Thanks @markwhitfeld, I'll take a look!

[markwhitfeld]

@Leonidas-from-XIV Thank you for taking a look. It would really be ideal to have tests that attempt to exploit the vulnerability, and then showcase how it doesn't exist anymore, but I am not familiar with CoffeeScript, so I wasn't able to do this.

[beckyconning]

This needs to be merged.

We don't need to test the CVE here as it was already resolved and there weren't tests for that.

The package is currently next-to-unusable.

Please merge this.

[beckyconning]

This is a simple and straightforward change that corrects undesired and unspecified behaviour of the package.

[Leonidas-from-XIV]

Yes, it was a bit unfortunate that we didn't add tests for the CVE, would be nice to know that we don't create regressions.

[Leonidas-from-XIV]

I'm currently on vacation but will try to take a look afterwards. If someone wants to submit a regression test, that would be appreciated :-)

[markwhitfeld]

Changes were included in PR #681 by @Leonidas-from-XIV I see that this fix is included in the v0.6.0 release. Thank you! 🎉