Fix prototype-polluting assignments

[MajorLift]

Explanation

Fixes prototype-polluting assignments by validating that dynamic-string property keys do not evaluate to __proto__, constructor, or prototype at runtime.

Defines isSafeDynamicKey validator and PROTOTYPE_POLLUTION_BLOCKLIST constant in @metamask/controller-utils.

References

• Closes [name-controller] Fix prototype-polluting assignments #3981
• Closes [address-book-controller, ens-controller] Fix prototype-polluting delete operations #3982

Changelog

N/A

Checklist

• I've updated the test suite for new or updated code as appropriate
• I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
• I've highlighted breaking changes using the "BREAKING" category above as appropriate

[naugtur]

How does this relate to SES? If the polluiton is on intrinsic Object.prototype, lockdown would prevent it.
If not, can we harden the objects in question? __proto__ is not the only way to pollute an object.

[Gudahtt]

This work was motivated by our new security scanner, which highlighted this problem. I don't think the prototype pollution problem addressed here affects applications using lockdown.

[mcmire]

Adding a function to check whether a single property makes sense to address this vulnerability now. It could be a bit confusing out of context, but I left some comments as to how we could explain it better.

As a future change, though, what are your thoughts about baking the check into a new function that would take an object and a property name and either set the property if it's safe or else throw an error?

[mcmire]

Should we add a test for this? Seems important to check.

[legobeat]

The type as well as the following code assumes that chainId is Hex, which if true implies that isSafeDynamicKey9chainId) would also be true.

Maybe adding a runtime-check that the chainId actually starts with 0x will do just as well here as well as in packages/address-book-controller/src/AddressBookController.ts?

A function like isSafeDynamicKey still could be useful for other cases where untrusted input string can't be assumed to be hex (indexing on method names anywhere?) but it seems like a simpler hex-prefix check will solve for all the cases covered in this PR?

[MajorLift]

It's true that isValidHexAddress for address and isSafeChainId for chainId would be sufficient validation to fix the violations currently found by the scanner.

However, it seems worth it to implement more generalized blocklisting logic for this vulnerability (https://portswigger.net/web-security/prototype-pollution/preventing#sanitizing-property-keys), which will also directly placate the CodeQL scanner.

[mcmire]

Sure, we could definitely make more improvements that would ensure we can protect against this vulnerability more generally.

That seems unrelated to my comment, though. Should we add tests which attempt to call delete with __proto__, constructor, and prototype and verify that they don't do anything?

[MajorLift]

I added tests for all three files here: a145237. Coverage is up to 100 for the three modules. I didn't test every string in the blocklist, but it seems like verifying that the validation works should be enough?

[mcmire]

Testing that it essentially just gets called is fine. Do we need tests for the function itself, though?

[MajorLift]

Ah of course I'll add that right now.

[MajorLift]

@SocketSecurity ignore npm/[email protected]

New maintainer is a member of the vscode team and maintainer of 96 npm packages, many of them under the @microsoft/ namespace.

@SocketSecurity ignore npm/@lavamoat/[email protected]

Internal package and maintainer.

[mcmire]

Looks good!

[Mrtenz]

Not intending to block this PR, but this seems like it could be useful for @metamask/utils.

[MajorLift]

Not opposed to moving it down the line if it fits better there!