fix(cve): bump busboy to fix CVE-2022-24434

[jlourenc]

This PR bumps busboy to at least 1.0.0 to remove dicer from the transitive dependencies as it contains a denial of service vulnerability: https://security.snyk.io/vuln/SNYK-JS-DICER-2311764.

The remaining of the PR is about adapting the code to the breaking changes introduced in busboy with the bump to v1 and fixing the tests:

• reverted https://github.com/expressjs/multer/pull/205 to fix should report errors from busboy parsing test as the error was no longer forwarded. I have not really looked into the details but reverting this workaround for an issue related to Node v0 seems like a win anyway,
• updated should handle unicode filenames test to have a more pertinent example of a unicode filename using both filename and filename* Content-Disposition directives.

This PR supersedes: https://github.com/expressjs/multer/pull/1096, https://github.com/expressjs/multer/pull/1092 and https://github.com/expressjs/multer/pull/1056.

[LinusU]

As far as I'm aware, this would be a breaking change, right? I think we need to do this in 2.x instead?

[jlourenc]

If the aim is to keep supporting Node v0 which is end-of-life since 2016, then yes it is a breaking change. However, such an aim is, in my opinion, counter-productive. Unless you're considering something else a breaking change?

Also, I've tried and run npm run test using node v0.12.12 and the project is already not compatible with Node v0 I believe:

$ node -v
v0.12.12

$ npm run test

> [email protected] test /Users/jeremylourenco/Code/multer
> mocha --harmony

/Users/jeremylourenco/Code/multer/node_modules/busboy/lib/index.js:3
const { readdirSync } = require('fs');
      ^
SyntaxError: Unexpected token {
    at exports.runInThisContext (vm.js:73:16)
    at Module._compile (module.js:443:25)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at Object.<anonymous> (/Users/jeremylourenco/Code/multer/lib/make-middleware.js:2:14)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at Object.<anonymous> (/Users/jeremylourenco/Code/multer/index.js:1:84)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at Object.<anonymous> (/Users/jeremylourenco/Code/multer/test/disk-storage.js:9:14)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at /Users/jeremylourenco/Code/multer/node_modules/mocha/lib/mocha.js:231:27
    at Array.forEach (native)
    at Mocha.loadFiles (/Users/jeremylourenco/Code/multer/node_modules/mocha/lib/mocha.js:228:14)
    at Mocha.run (/Users/jeremylourenco/Code/multer/node_modules/mocha/lib/mocha.js:514:10)
    at Object.<anonymous> (/Users/jeremylourenco/Code/multer/node_modules/mocha/bin/_mocha:480:18)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Function.Module.runMain (module.js:501:10)
    at startup (node.js:129:16)
    at node.js:814:3

I'm not a Node expert but it seems "Object Destructuring" is a v6+ feature.

[erano067]

Hi, why not update to busboy atleast version ^1.6. 0?

[jlourenc]

Hi, why not update to busboy atleast version ^1.6. 0?

The aim of this PR is to remove dicer from the dependencies, which is achieved by upgrading to at least v1.0.0. There is no reason to enforce a higher version as far as this PR is concerned. You may chose to enforce a higher version in your project pulling multer.

[jlourenc]

I have now enforced the node engine to be >= 6.0.0.

[vaibhavkumar-sf]

Hi, Thanks for all the updates, can you please bump the busboy version to the latest 1.6.0 otherwise we may need to upgrade it again sooner for other vulnerabilities in the near future?

[Bozzzzzo]

Hi, Thanks for all the updates, can you please bump the busboy version to the latest 1.6.0 otherwise we may need to upgrade it again sooner for other vulnerabilities in the near future?

Hum, may be you did not realize but the semver "^1.0.0" being used for "busboy" will eventually pickup "[email protected]" (well unless a stronger constraint - like a package-lock.json explicitly prevents it).

In fact it will even pick up future versions like "[email protected]" ... It will just not pick major version change like "[email protected]". So I think this is reasonably open to future bug/security fixes.

Now the question for me is whether this fix will be published any time soon ? And in which version ?

That is will it be in a "[email protected]" or in the already announced "next" version which would be "[email protected]"

I hope it will be in a "[email protected]" because the other being a major release WILL NOT fix much of the existing package being dependent on "multer".

Best regards, Didier Reis.

[harshagarwal00]

im stuck on this as well

[ansonallard]

+1 on getting this merged in.

[vaibhavkumar-sf]

Yes @Bozzzzzo , thanks, I didn't notice that it is ^ instead of ~ @LinusU please merge it

[achilehero]

+1 on merging and releasing

[kutluturk]

+1 on merging. This is a serious security problem

[dizhenlyu]

+1 on the merge and release ASAP. Thank you!

[zacharytyhacz]

 _______________________ 
< let's get this merged >
 ----------------------- 
        \   ^__^
         \  (oo)\_______
            (__)\       )\/\
                ||----w |
                ||     ||

[eran10]

+1, can this be merged please ?

[senpai-notices]

lgtm. i approve this mr. do let any of us know if we can help you with any blockers.

[zSakuraEvilz]

+1, can this be merged and release v1.4.5 please ?

[progsmile]

Let's release bro :)

[NormandoHall]

c'mon!

[Dany-C]

This PR bumps busboy to at least 1.0.0 to remove dicer from the transitive dependencies as it contains a denial of service vulnerability: https://security.snyk.io/vuln/SNYK-JS-DICER-2311764.

The remaining of the PR is about adapting the code to the breaking changes introduced in busboy with the bump to v1 and fixing the tests:

• reverted lib: work around node.js bug #205 to fix should report errors from busboy parsing test as the error was no longer forwarded. I have not really looked into the details but reverting this workaround for an issue related to Node v0 seems like a win anyway,
• updated should handle unicode filenames test to have a more pertinent example of a unicode filename using both filename and filename* Content-Disposition directives.

This PR supersedes: #1096, #1092 and #1056.

My NestJs app didn't compile, so i found this temporary fix:

As of npm cli v8.3.0 (2021-12-09) this can be solved using the overrides field of package.json

For NestJs :
overrides": { "@nestjs/platform-express": { "multer": { "busboy": "1.0.0" } } }

For Express : overrides": { "multer": { "busboy": "1.0.0" } }

[vaibhavkumar-sf]

override is a temporary solution to use until the package owner does not upgrade the package, also override is okay only if you are sure that the required version will not break the package @Dany-C

[LinusU]

Hey everyone!

I understand that this is frustrating for everyone involved. Unfortunately, I also see it as a big problem to release this as 1.x since it would break compatibility with older Node.js versions. Older versions of Node.js is unfortunately still used in many production environment, and it's Express policy to not break Node.js compatibility in non-major changes.

It also doesn't make sense to release this as 2.x since we already have several release candidates of Multer 2.x already, and they include an all new API.

Btw. I have released version 2.0.0-rc.4 which uses @fastify/busboy instead, and thus shouldn't be vulnerable.

I would love to get some input on how to proceed, some alternatives that I see:

1. Publish this branch as e.g. multer-classic on Npm
2. Publish this branch as something like 1.4.5-node.12, which I think won't get picked up as automatic update in semver?
3. Release this as 1.4.5 as a breaking change
4. Fork dicer (& busboy) and just fix the CVE, but keep Node.js compatibility
5. Inline the code for busboy and then restore old Node.js compatibility to it

@dougwilson do you have any input or guidance?

[cnorthwood]

An alternate to consider- release as version 3.x and then the current 2.x RCs when ready for release are released as version 4? This would be semver compatible but feels a bit nasty to then essentially "skip" a version but that is an aesthetic consideration imo so the need for pragmatism might overrule it.

[jlourenc]

Hey @LinusU,

Unfortunately, I also see it as a big problem to release this as 1.x since it would break compatibility with older Node.js versions.

I've tested again compatibility with node locally and yes, you're right this is a breaking change.

My 2 cents would be to:

• use option 2 if that does not get picked up automatically
• mark all other v1 versions as deprecated to encourage people moving off of them, to the v1.4.5-node.12 in the short term but to the v2 in the long term
• stop v1 development and only accept security patches while v2 gets released

[zSakuraEvilz]

So is there any way to use v1 without security? Because the number of sources is quite large, if you upgrade to v2, you have to modify the current code, which is very difficult for big projects.

[rudxde]

Hi, i think if you would publish the fix first under the 1.4.5-node.12 tag, most user would be able to fix this issue in their application, since upgrading to any v2-rc is not possible for every project and will leave many applications longer vulnerable. Afterwards it's still possible to work out, how to continue on this issue.

[vaibhavkumar-sf]

voting for 1.4.5-node.12

[mathertel]

MUST rate security issues higher than compatibility. fixing is forward only. @jlourenc did a good job. install using

npm i "jlourenc/multer.git#fix/CVE-2022-24434"

and test before complaining. I had no problem for my applications.

[ghost]

MUST rate security issues higher than compatibility. fixing is forward only. @jlourenc did a good job. install using

npm i "jlourenc/multer.git#fix/CVE-2022-24434"

and test before complaining. I had no problem for my applications.

Nice work. I've uploaded successfully multiple files. When I attempt to use the malicious code now I get this error.

 Malformed part header

Caught in the error handler. No more server crash.

Code to test:

 fetch('<- upload endpoint ->', {
      method: 'POST',
      headers: {
        ['content-type']: 'multipart/form-data; boundary=----WebKitFormBoundaryoo6vortfDzBsDiro',
        ['content-length']: '145',
        connection: 'keep-alive',
      },
      body: '------WebKitFormBoundaryoo6vortfDzBsDiro\r\n Content-Disposition: form-data; name="bildbeschreibung"\r\n\r\n\r\n------WebKitFormBoundaryoo6vortfDzBsDiro--'
    });

An official patch with the fix would be nice. Thank you.

[inPhoenix]

つ ◕＿◕ ༽つ つ ◕＿◕ ༽つつ ◕＿◕ ༽つ +1