search

richardgirges / express-fileupload

Simple express file upload middleware that wraps around busboy
MIT License
1.52k stars 261 forks source link

Fix prototype pollution in utilities.js #301

Closed zwade closed 2 years ago

zwade commented 2 years ago

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 commented 2 years ago

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

zwade commented 2 years ago

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

richardgirges commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

For cleanliness' sake I moved this into a separate PR: https://github.com/richardgirges/express-fileupload/pull/302

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

cgvwzq commented 2 years ago

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

zwade commented 2 years ago

Glad I could help!

richardgirges commented 2 years ago

Thank you @zwade! v1.3.1 published on NPM

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