Fix prototype pollution in utilities.js

[zwade]

Hi! Last weekend I found a low-impact prototype pollution in this module. It's only problematic in conjunction with certain antipatterns, but it is probably good to fix if possible. This PR makes a change that will fix it under normal usage, along with a few tests to verify that it remains fixed.

A couple of caveats, the first being that it will not address the problem if the caller manually sets req.files (or req.body) to an object. In this case, however, it is less trivial to determine what the correct behaviour should be, so I did not attempt to address it.

Secondly, this diff is a bit messy because VSCode decided to trim extra whitespace in a couple of places. If that's an issue, please let me know and I can manually revert those.

Thanks!
Zach

[cgvwzq]

It is funny that I was, just right now, writing a fix for this. I'll just comment here instead. As you mention, the Object.create(null) is good practice, but will only protect against some cases. I would suggest, on top of that, to move the inlined prototype pollution check from processNested.js into a function isSafeFromPollution(obj, key) that is exported by utilities.js and use that check in all the places doing sensitive merge-like things, like processNested(), buildFields().

It is probably also a good idea to use Object.create(null) in processNested too (on variables d and current[k]).

I disagree on the low-impact issue, and this can become real bad with certain flags. So it is probably sensible to get the patch and security advisory quickly.

Cheers,
Pepe

[cgvwzq]

I would add an isSafeFromPollution(instance, field) check right here. And move https://github.com/richardgirges/express-fileupload/blob/master/lib/processNested.js#L24-L27 into a function exported by utilities.

[zwade]

That sounds pretty reasonable, let me take a look at that now

[richardgirges]

Thank you very much for taking the time on this! This is a high priority, so I'm interested in getting this merged soon. Agree with @cgvwzq's feedback.

Additionally, if you wouldn't mind, can you double-check that linting and tests are passing? CircleCI is temporarily disconnected so I'm merging blind.

[zwade]

Here's an update that hopefully makes the checks more robust. There are, naturally, some more edge cases due to this being fundamentally challenging to determine correctly, but I think the weird behavior should be effectively documented in the test I added.

Also regarding tests, I'm having trouble running some of the unrelated tests on my machine, so I'll need a minute or two more to verify that they're passing correctly.

[zwade]

Ahh ok, this might actually be a regression in saveBufferToFile. Specifically on my system (macOS 11.1, node v16.13.1) fs.createWriteStream is firing both an error and close event causing the callback to be invoked twice (once with an error once without).

This is probably largely undetected because most modern code will be using the promise interface which naturally silences duplicate calls to resolve/reject.

Since I would imagine you would like to have the tests passing, I'm happy to submit a separate PR that addresses this.

[zwade]

For cleanliness' sake I moved this into a separate PR: #302

Once merged (or rebased) on top of that one, this PR should have passing tests/lints.

[cgvwzq]

That looks good to me. Thanks a lot @zwade :)

[zwade]

Glad I could help!

[richardgirges]

Thank you @zwade! v1.3.1 published on NPM

https://github.com/richardgirges/express-fileupload/releases/tag/v1.3.1