Upload security

[mdaffin]

This is not an issue with multer directly, but more an issue with applications using multer.

There is little to no mention of any security issues with file uploads handled by multer leading to a lot of example code around the web based off the simple examples in the readme that completely ignore any security issues, notably things like:

var express = require('express')
var multer  = require('multer')

var app = express()
app.use(multer({ dest: './uploads/'}))

app.post('/api/uploads', ...handles maby one expected file...)

The first major issue here is that multer will be executed on any post request, even for post requests that are not expected to handle files, meaning that you could potentially upload files to the server bypassing the upload url if there exists another post url.

This is a major concern as if people are using these examples directly in their code they will be vulnerable to to file upload exploits if they do not already know about them.

The example should probably be changed to app.use('/api/upload', multer({ dest: './uploads/'})) so that multer only handles files to locations that are known to handle uploads.

It also might be worth adding a filter on what files are expected so that the user does not have to worry about unexpected files being uploaded. Something long the lines of:

app.use('/api/uploads', multer({ dest: './uploads', filter: ['someFile']})

so that multer only handles files found in the POST requests someFile variable and rejects/errors for any other files found. This could be partly done by limits.files, but this does not ensure that the expected file ends up in the right variable, and people generally only check for what they are expecting.

[jpfluger]

This was a thoughtful post. If multer is used globally, multer is exposed to these vulnerabilities. But if the middleware is placed directly on the route, the security concerns are mitigated. I just wrote a doc explaining how to do this.

[mdaffin]

Not entirely, you still have to check all the files in req.files and delete any you are not expecting. I have seen quite a few examples that only check for req.files.images, for example, with no limit set (I have not seen any set a limit) which means an attacker could upload a file to req.files.bad to bypass any check made. Even with a limit set you could just upload to req.files.bad and watch the code return an error as you did not set req.files.images... but by this point your file is already on the server. Unless you check all files in req.files.images and set multer middle ware on only the expect routes you are vulnerable.

[jpfluger]

Yes, I see now. I've setup some tests and verified this. As a side-effect, one of the tests showed that surpassing the file limit creates a condition where multer never "finishes" the current process of parsing files: so the server hangs. I'm gonna try to fix this now and then add in support for "explicitFields." I already have a pull request to add in-memory parsing (no automatic write-to-disk). So hopefully these can get merged in or else I may clutter up the code. When I get my tests done, I'll drop a message on this thread to get your thoughts if what I'm looking at is right/wrong/needs nudging.

[jpfluger]

I added support for fieldNameSize limitations in busboy and initiated a pull request. Once that lib gets updated, we can work through hardening multer and will report back here.

[hacksparrow]

👍

[LinusU]

@James147 I believe this to be solved by the newly released version 1.0.0. Please reopen if you have any further comments on how we could improve the security.